Disable Content-Type header on 204 No Content

[stefanoverna]

As Rack::Lint points out, if the response status is 204, there should not be any Content-Type header set. Let me know if everything is alright with this PR @jodosha. I don't really like having to reference @_status outside of Lotus::Action::Rack, but there's no getter :confused:

[jodosha]

@stefanoverna Thanks for this PR. I agree, referencing @_status from there isn't great and that conditional looks "magic". Can you please abstract with something like if send_content_type?.

[rugginoso]

There are others http responses which require an empty body, from the rfc (http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html):
"All 1xx (informational), 204 (no content), and 304 (not modified) responses MUST NOT include a message-body. All other responses do include a message-body, although it MAY be of zero length."

[jodosha]

@rugginoso True, I've put that thing in place for lotusrb full stack, but not here.

I think we should differentiate between "empty headers" policy and "empty body", because some HTTP statuses like 204 want totally empty response.

Also, this looks like a control to move away from Lotus::Action::Mime and put into another custom module like Action::Http where we do those kind of checks.

The reason is simple: we need to check at the end of the pipeline that the body/headers are empty when needed.

Would you guys mind to amend this PR with those changes and rebase? Thank you!

[stefanoverna]

@jodosha yeah, sure. Will try to do it! 👍

[stefanoverna]

@jodosha how about something like this?

[coveralls]

Coverage increased (+0.03%) when pulling 2b6468f on stefanoverna:no-content into 975feb8 on lotus:master.

[jodosha]

@stefanoverna I'm sorry for the late reply here.

RFC 2616

10.2.5 204 No Content

The server has fulfilled the request but does not need to return an
entity-body, and might want to return updated metainformation. The
response MAY include new or updated metainformation in the form of
entity-headers, which if present SHOULD be associated with the
requested variant.

Source

In my understanding, HTTP headers can be sent if they are related to entities (aka custom headers). At the same time, default headers like Content-Type and Content-Length MUST not be sent. At least according to the Rack::Lint interpretation.

Because this isn't mime type related issue, we should extract that test into a separated case under test/integration/ and make sure that the following diff passes.

diff --git a/test/integration/mime_type_test.rb b/test/integration/mime_type_test.rb
index bd7fe18..c339332 100644
--- a/test/integration/mime_type_test.rb
+++ b/test/integration/mime_type_test.rb
@@ -78,6 +78,8 @@ module Mimes

     def call(params)
       self.status = 204
+      headers['Content-Length'] = '0'
+      headers['X-Entity']       = 'OK'
     end
   end
 end
@@ -114,7 +116,9 @@ describe 'Content type' do
   it 'does not produce a "Content-Type" header when the request has a 204 No Content status' do
     response = @app.get('/nocontent')
     response.headers['Content-Type'].must_be_nil
-    response.body.must_equal                    ''
+    response.headers['Content-Length'].must_be_nil
+    response.headers['X-Entity'].must_equal 'OK'
+    response.body.must_equal                ''
   end

Would you like to take care of this? Thank you!

[stefanoverna]

@jodosha I'm not entirely sold on your interpretation.. entity-headers are not custom headers (ie. X-Entity), but these headers:

7.1 Entity Header Fields

   Entity-header fields define metainformation about the entity-body or,
   if no body is present, about the resource identified by the request.
   Some of this metainformation is OPTIONAL; some might be REQUIRED by
   portions of this specification.

       entity-header  = Allow                    ; Section 14.7
                      | Content-Encoding         ; Section 14.11
                      | Content-Language         ; Section 14.12
                      | Content-Length           ; Section 14.13
                      | Content-Location         ; Section 14.14
                      | Content-MD5              ; Section 14.15
                      | Content-Range            ; Section 14.16
                      | Content-Type             ; Section 14.17
                      | Expires                  ; Section 14.21
                      | Last-Modified            ; Section 14.29
                      | extension-header

So, it seems that passing ie. Content-Type or Content-Length together with a 204 response code is perfectly valid. Rack::Lint is, well.. a linter, so it goes a step beyond and assumes it's not the right thing to do, even if valid.

[jodosha]

@stefanoverna @rugginoso There was some Git conflicts now solved. I also cleaned up the code and added new tests.

I'm going to merge this to ease the workflow, but there are still some unresolved points that I would love to discuss in another GH issue.

Thanks for your effort until now! 👍

[stefanoverna]

great @jodosha feel free to mention us :)

[jodosha]

@stefanoverna @rugginoso @lotus/core-team I moved the discussion here: rack/rack#787

[stefanoverna]

👍