Content-Type and Rack::Lint fixes. #68

mostlyobvious · 2012-09-11T20:02:09Z

I've run into problems when using Rack adapter with Webmachine due to Rack::Lint throwing errors with regard to Content-Type header (see https://github.com/pawelpacana/webmachine-example-with-rack).

According to RFC:

Any HTTP/1.1 message containing an entity-body SHOULD include a
Content-Type header field defining the media type of that body.

Along the way it turned out some of the specs were bogus. I've fixed them and introduced ensure_content_type similar to ensure_content_length.

Maybe Content-Type should be set elsewhere. What do you think?

ghost · 2012-09-11T20:35:48Z

spec/webmachine/adapters/rack_spec.rb

@@ -51,7 +52,7 @@ def from_json; end
  let(:adapter) do
    described_class.new(configuration, dispatcher)
  end
-  let(:app) { adapter }
+  let(:app) { Rack::Lint.new(adapter) }


👍

I added these Lint wrappers to the rack test suite some time ago and am a big fan of complying to specs! :)

seancribbs · 2012-09-11T20:55:25Z

I wouldn't do a blanket ensure_content_type at the end of the request processing. If the Content-Type is not set in the decision flow, the flow is wrong and we should fix those places.

mostlyobvious · 2012-09-11T21:09:36Z

From what I understand Content-Type is set in states c3/c4, therefore processing stopped in states before is broken regarding to spec and lint.

statonjr · 2012-09-14T18:46:15Z

I'm still not sure why the Rack Spec says that you must not provide a Content-Type header for 1##, 204, or 304 responses, but it appears that this will be changed in Rack 1.5: rack/rack#367

To deal with the error, I strip out the "Content-Type" in the "finish_request" method.

seancribbs · 2012-09-16T15:00:49Z

@pawelpacana C3/C4 is where the content-type is decided, but not set until F6. Perhaps the problems are error responses that are produced. It might be appropriate for the Response to have Content-Type already set to "text/html" by default. In my cursory glance at the Erlang version, it seems that's what it does.

samwgoldman · 2012-09-16T19:08:40Z

Using a default Content-Type of "text/html" is also consistent with the default resource behavior w.r.t. content_types_provided.

seancribbs · 2012-09-17T00:02:45Z

I ran into this today as I was working on my own app. In my case, Rack::Lint blew up because I used HTTP Basic Auth, which returned a 401. I've been scouring the RFC for any mentions of when the Content-Type header MUST be included. Here's what I found:

7.2.1 Type
...
Any HTTP/1.1 message containing an entity-body SHOULD include a
Content-Type header field defining the media type of that body. If
and only if the media type is not given by a Content-Type field, the
recipient MAY attempt to guess the media type via inspection of its
content and/or the name extension(s) of the URI used to identify the
resource. If the media type remains unknown, the recipient SHOULD
treat it as type "application/octet-stream".

The MUST statements regarding those response codes are about the entity (response body), not the header field. While applications SHOULD include a Content-Type field, it is not required. I know it's a pedantic distinction, but producing an internal error for a SHOULD seems wrong to me. Maybe logging the problem would be sufficient. However, that's a pull-request for Rack, not Webmachine!

(╯°□°）╯︵ dʇʇɥ

ghost · 2012-09-17T00:12:25Z

Rack::Lint follows its SPEC, so why not attack this within the Rack adapter? After all the adapter's purpose is establishing compatibility between Webmachine and the RACK spec. It'd nicely fit around Line 60.

ghost · 2012-09-17T00:57:32Z

@lgierth Agree 100%.

seancribbs · 2012-09-17T02:19:56Z

I guess my point is, WM should be a good citizen and return a proper response, but a PR to Rack might also be in order. Just wanted to bring up how nuanced (vague) the RFC is on the matter.

ghost · 2012-09-17T12:25:05Z

Rack::Lint requiring a response with Content-Type has been bothering me for some time, but I don't see James and Konstantin removing this requirement. Apart form that, Webmachine is a perfectly good citizen :)

mostlyobvious · 2012-09-17T12:47:56Z

To sum up:

RFC says SHOULD not MUST so Webmachine is working fine
Rack::Lint behaviour may be changed as pointed in Mime type corrections rack/rack#367
current status: Rack apps on Webmachine are broken

I second @lgierth on fixing this in Rack adapter for Webmachine. I'll refine this PR later.

ghost · 2012-09-17T12:52:05Z

Rack::Lint behaviour may be changed as pointed in rack/rack#367

Ah cool, missed that one, thanks!

seancribbs · 2012-09-17T13:38:32Z

@lgierth Yes, line 60 seems like a good place to handle the problem in the short-term. I'd still like to audit all the places that non-resource-produced bodies are created by WM and make sure they set the content type appropriately.

ghost · 2012-09-17T15:27:07Z

I'd still like to audit all the places that non-resource-produced bodies are created by WM and make sure they set the content type appropriately.

Definitely! If a non-resource-produced response has an entity body, WM should make sure it also has a Content-Type.

seancribbs · 2012-12-12T17:15:56Z

@pawelpacana @lgierth This has sat for a while, has the change been made to the Rack adapter as discussed? Let's get this closed out.

mostlyobvious · 2012-12-13T23:45:32Z

@seancribbs @lgierth Let me get back to this over weekend.

Rack::MockResponse exposes two methods with regard to header manipulation: - #original_headers that contains all headers passed in constructor https://github.com/rack/rack/blob/master/lib/rack/mock.rb#L157 - #headers that applies some initial processing on headers like setting default Content-Type and removing some headers in case of empty entity-body https://github.com/rack/rack/blob/master/lib/rack/response.rb#L24-25 https://github.com/rack/rack/blob/master/lib/rack/response.rb#L75-76 While this processing in #headers is good in general we don't use Rack::Response instances in Rack adapter (which Rack::MockResponse subclasses). Instead we take Webmachine::Response and turn it straight into array compatible with Rack spec: https://github.com/seancribbs/webmachine-ruby/blob/master/lib/webmachine/adapters/rack.rb#L62-64 All this leads to bogus tests which this commit fixes.

…r spec.

mostlyobvious · 2012-12-28T12:11:47Z

@seancribbs @lgierth I've removed irrelevant commits, rebased and moved response transformations needed to pass rack lint into rack adapter, wdyt?

ghost · 2012-12-28T12:19:33Z

Looks good!

seancribbs · 2012-12-28T13:19:44Z

Very 👍, merge it!

Content-Type and Rack::Lint fixes.

seancribbs · 2012-12-28T13:39:54Z

@pawelpacana As is my policy, you now have commit bits. Use them for great good!

ghost reviewed Sep 11, 2012
View reviewed changes

mostlyobvious added 4 commits December 28, 2012 12:23

Wrap adapter in Lint middleware in Rack adapter spec.

7439207

Add another failing test case for Rack and content-type.

f336778

Let Rack::Response handle response transformations conforming to thei…

41fb388

…r spec.

seancribbs added a commit that referenced this pull request Dec 28, 2012

Merge pull request #68 from pawelpacana/content-type-and-rack-lint

5f77aef

Content-Type and Rack::Lint fixes.

seancribbs merged commit 5f77aef into webmachine:master Dec 28, 2012

mostlyobvious mentioned this pull request Jul 14, 2013

Streaming in Rack adapter #110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content-Type and Rack::Lint fixes. #68

Content-Type and Rack::Lint fixes. #68

mostlyobvious commented Sep 11, 2012

ghost Sep 11, 2012

seancribbs commented Sep 11, 2012

mostlyobvious commented Sep 11, 2012

statonjr commented Sep 14, 2012

seancribbs commented Sep 16, 2012

samwgoldman commented Sep 16, 2012

seancribbs commented Sep 17, 2012

ghost commented Sep 17, 2012

ghost commented Sep 17, 2012

seancribbs commented Sep 17, 2012

ghost commented Sep 17, 2012

mostlyobvious commented Sep 17, 2012

ghost commented Sep 17, 2012

seancribbs commented Sep 17, 2012

ghost commented Sep 17, 2012

seancribbs commented Dec 12, 2012

mostlyobvious commented Dec 13, 2012

mostlyobvious commented Dec 28, 2012

ghost commented Dec 28, 2012

seancribbs commented Dec 28, 2012

seancribbs commented Dec 28, 2012

Content-Type and Rack::Lint fixes. #68

Content-Type and Rack::Lint fixes. #68

Conversation

mostlyobvious commented Sep 11, 2012

ghost Sep 11, 2012

Choose a reason for hiding this comment

seancribbs commented Sep 11, 2012

mostlyobvious commented Sep 11, 2012

statonjr commented Sep 14, 2012

seancribbs commented Sep 16, 2012

samwgoldman commented Sep 16, 2012

seancribbs commented Sep 17, 2012

ghost commented Sep 17, 2012

ghost commented Sep 17, 2012

seancribbs commented Sep 17, 2012

ghost commented Sep 17, 2012

mostlyobvious commented Sep 17, 2012

ghost commented Sep 17, 2012

seancribbs commented Sep 17, 2012

ghost commented Sep 17, 2012

seancribbs commented Dec 12, 2012

mostlyobvious commented Dec 13, 2012

mostlyobvious commented Dec 28, 2012

ghost commented Dec 28, 2012

seancribbs commented Dec 28, 2012

seancribbs commented Dec 28, 2012