Resolve Content-Type from custom and common MIME types via Accept header #179

russCloak · 2016-09-30T22:28:46Z

The #content_type method was not taking into account when we have a custom MIME type registered for a potential controller format - instead it was overlooking these values and simply skipping to the default content type.

Now, we honor the ordered preference of choosing which content type to use:

Explicit set value (from #format=)
Weighted value from Accept header (common MIME types and registered custom MIME types)
Configured default content type
Hard-coded default content type

Examples:

Given the following controller:

module Web
  class Application < Hanami::Application
    configure do
      controller.format v1: 'application/vnd.custom.v1+json'
      controller.format v2: 'application/vnd.custom.v2+json'
      controller.prepare do
        accept :v1, :v2, :json
      end
    end
  end
end

Before this PR
When a request was made with Accept: application/vnd.custom.v2+json it would respond with Content-Type: application/json.

With this PR
When a request is made with Accept: application/vnd.custom.v2+json, it responds with Content-Type: application/vnd.custom.v2+json. Likewise, v1 returns v1, and application/json returns application/json.

Weighting also works now for custom types:

Before this PR
When a request was made with Accept: application/json; q=0.5, application/vnd.custom.v1+json; q=0.9, it would respond with Content-Type: application/json.

With this PR
When a request is made with Accept: application/json; q=0.5, application/vnd.custom.v1+json; q=0.9, it responds with Content-Type: application/vnd.custom.v1+json. Likewise, a request made with Accept: application/json; q=0.8, application/vnd.custom.v1+json; q=0.4 responds with Content-Type: application/json.

This is meant to clarify the intent and overall create a more focused and useful method. Realistically, this is a private API for the class, and it's only used in one place - but nonetheless the intent and purpose should be clear. This is now (clearly) responsible for looking at the current Accept header value (default or not) and identifying (with weight compensation) if the value is a known (non-custom) MIME type. If it is, the type is returned, otherwise nil is returned.

This is really meant to be another one of those "clearly demonstrated intent" methods. When we're looking to the Accept header for a potential Content-Type value, we need to first check for normal header types, then consider custom types. This method is exclusively meant for the latter.

This method is simply replacing the logic that used to exist in the methods that look to the header for a certain MIME type (either common or custom). Now, instead of checking for the header to exist, then looking at type, they are simpler and only look at type. The check for the Accept headers presence is now taken care of external to those methods (since they shouldn't be aware of that state anyway).

The method was not taking into account when we have a custom MIME type registered for a potential controller format - instead it was overlooking these values and simply skipping to the default content type. Now, we honor the ordered preference of choosing which content type to use: 1. Explicit set value (from #format=) 2. Weighted value from Accept header (common MIME types) 3. Value from Accept header (custom registered MIME types) 4. Configured default content type 5. Hard-coded default content type

coveralls · 2016-09-30T22:32:09Z

Coverage decreased (-0.02%) to 98.482% when pulling 49ce85e on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-09-30T22:32:09Z

Coverage decreased (-0.02%) to 98.482% when pulling 49ce85e on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-09-30T22:32:09Z

Coverage decreased (-0.02%) to 98.482% when pulling 49ce85e on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

russCloak · 2016-09-30T23:12:49Z

I would add more coverage, but there isn't really any isolated/unit tests as is - I'm not sure where they'd be incorporated.

Also, the integration tests I've added are more than enough for the scenario - I suppose I could add more?

Not sure what the requirements are, please advise.

drewdeponte · 2016-10-01T05:41:22Z

First off let me say this awesome. I see this being extremely valuable.

drewdeponte · 2016-10-01T06:16:32Z

lib/hanami/action/mime.rb

@@ -207,7 +211,14 @@ def format
      #     end
      #   end
      def content_type


Thinking about this purely conceptually the Content-Type and response body format should be set based on a couple things according to the Accepts header section in the HTTP RFC.

Scenarios

If the service supports one of the mime types specified in Accepts header it should respond with the most preferred based on the q scoring that also happens to be supported. See RFC 2616 Section 14.1 for details.

If the client specifies one or more mime types and the service does not support any of them then the service should respond with a 406 status code.

If the client does not specify any accept mime types then the service should default to its ideal mime type.

If I remember correctly the 406 status code is already handled for scenario 2 above, or at least partially handled. Maybe we should revisit that to make sure it is completely handled.

I am a little confused by the priority order of get_common_mime_type_from_accept_header and get_custom_mime_type_from_accept_header.

It seems in my mind based on the RFC if an accept header is provided with one or more acceptable mime types you have two possible scenarios.

the service doesn't support any of them and a 406 status code is responded. This already happens.

the service has been identified as supporting one or more of the mime types specified in the Accepts header and therefore needs to identified the out of the supported ones which is the most preferred based on the q score stuff and set the content type type to that.

I am not sure why we need get_common_mime_type_from_accept_header used at all when we know an accept header has been specified so we have to be in one of the above two states. Am I missing something?

It also seems instead of the get_custom_mime_type_from_accept_header, or maybe not instead, but maybe the inside of that method needs to be different. It seems it would need to be something like best_q_match(accept, get_acceptable_mime_types(accept)). That way you would be first getting the matching mime types specified by the client in the Accept header and then taking that resulting list and picking the most preferred one using the q score matching.

Great feedback @cyphactor, I'm going to make some modifications based on this today.

Looking at the changes you made. It looks like it should be covering the RFC now.

In my earlier commits the preference for Content-Type resolution was focused on the types that are common (from Rack), THEN on the types that were registered through the controller configuration. Now we are going to look at both of those with the `q` weighting that is part of the RFC (and currently part of how Hanami Controller works).

We need to be able to know what MIME types have been registered with a custom controller configuration, so we can appropriately get the correct Content-Type to respond with (given an Accept header). There was no accessor method other than the protected `#formats` which only returns a hash including both MIME type and format key (we only need the former).

russCloak · 2016-10-04T18:35:38Z

@cyphactor I've made some adjustments, and added a few more tests to prove the changes work. Can you please take another look?

coveralls · 2016-10-04T18:38:25Z

Coverage decreased (-0.01%) to 98.488% when pulling c06373f on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

drewdeponte · 2016-10-04T22:33:17Z

test/configuration_test.rb

+      @configuration.format custom: 'custom/format'
+    end
+
+    it 'returns all known format MIME types' do


This description should probably read "return all configured format MIME types".

Good thought, I've added this change in 9bc4e2e

Technically we aren't dealing with all known types, it's only the ones known to the controller configuration, so this makes a lot more sense.

coveralls · 2016-10-04T22:51:03Z

Coverage decreased (-0.01%) to 98.488% when pulling 9bc4e2e on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-10-04T22:51:03Z

Coverage decreased (-0.01%) to 98.488% when pulling 9bc4e2e on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

drewdeponte · 2016-10-04T23:35:14Z

Everything looks good to me at this point. I give it a 👍 .

cllns · 2016-10-05T06:57:15Z

lib/hanami/action/mime.rb

+      #
+      # @api private
+      def get_mime_type_from_accept_header
+        all_types = (MIME_TYPES + configuration.format_mime_types).uniq


Not sure if it's worth it, but rather than running uniq on a new Array, would it be worth trying out Set (probably SortedSet in this case) and seeing how that performs in comparison?

I did a comparison with a few different ways to make a list unique, though there's a difference it's relatively minor.

Running benchmark with array1(10000 elements, 3675 dupes) and array2(10000 elements, 3642 dupes) user system total real Add Arrays 0.000000 0.000000 0.000000 (0.006018) New Set (merge) 0.010000 0.000000 0.010000 (0.004714) New Set with add 0.000000 0.000000 0.000000 (0.005160) Hash, keys only 0.010000 0.000000 0.010000 (0.004712)

With that said, I don't mind switching if it's a real concern.

@cllns what do you think?

The difference looks pretty insignificant, especially considering that there will usually be around 10 elements max in the Accept header, never 1000.

My bad I spoke too fast - there are indeed around 500 mime types registered in rack. The difference is still pretty small.

@beauby I agree, especially considering the impact of making the code less intuitive (Set with merged values vs a simple Array).

My vote is for simple understandable code in this case rather than a small performance increase.

One other thing to keep in mind is that the configuration.format_mime_types may typically contain very few values, in which case it is probably less performant to ensure uniqueness than to simply concatenate the arrays (with duplicates) and search in that.

Also (not related to this PR), is there a size limit on the Accept header at some level, or can we simply make the CPU spin by supplying a very long list of random values?

Thanks for looking into this. benchmark-ips is a better way of comparing, IMO since it digests the data for you.

I'm generally in favor of understandability over minor performance gains, but this code would affect every request (as I understand it), where even small gains add up.

I'll let @jodosha or @joneslee85 determine whether they think this is OK :)

beauby · 2016-10-10T20:55:07Z

lib/hanami/action/mime.rb

+        return @content_type if @content_type
+
+        if accept_header?
+          type = get_mime_type_from_accept_header


The get in the name seems superfluous and not in line with the rest of this file. How about mime_type_from_accept_header instead?

It's honestly going to rarely be the case when there are many custom MIME types specified in the Accept header - so it is probably best to just always combine and search through them - since it's not really an issue if there are duplicates anyway.

coveralls · 2016-10-10T21:19:56Z

Coverage decreased (-0.2%) to 98.333% when pulling 89552e6 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-10-10T21:19:56Z

Coverage decreased (-0.2%) to 98.333% when pulling 89552e6 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-10-10T21:19:56Z

Coverage decreased (-0.2%) to 98.333% when pulling 89552e6 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

beauby · 2016-10-10T21:21:14Z

lib/hanami/action/mime.rb

-      # @since 0.1.0
+      # Checks if there is an Accept header for the current request.
+      #
+      # @return [Boolean] true if there's an Accept header to look at


Nitpicking:

tTrue if ~~there's~~ there is an Accept header ~~to look at~~ in the current request.

Fair enough, I don't mind making these changes - just a note though, I did it this way based on other documentation in the same file (https://github.com/Acornsgrow/controller/blame/resolve_content_type_from_custom_formats/lib/hanami/action/mime.rb#L409).

Should I clean up all of the documentation in the file to match?

I don't think it's worth the effort but let's see what the maintainers think.

beauby · 2016-10-10T21:22:04Z

lib/hanami/action/mime.rb

+      # or the custom registered ones (see Hanami::Controller::Configuration#format).
+      #
+      # @return [Nil] when the Accept header does not match any known MIME
+      # Types.


Nitpicking

wWhen the Accept header does not match any known MIME ~~Types~~ type.

beauby · 2016-10-10T21:22:29Z

lib/hanami/action/mime.rb

+      #
+      # @return [Nil] when the Accept header does not match any known MIME
+      # Types.
+      # @return [String] the matched MIME type for the given Accept header.


Nitpicking

tThe matched MIME type for the given Accept header.

beauby · 2016-10-10T21:25:17Z

test/integration/mime_type_test.rb

+    end
+
+    it 'sets "Content-Type" header according to "Accept"' do
+      response = @app.get('/custom_from_accept', 'HTTP_ACCEPT' => 'application/custom;q=0.9, application/json;q=0.5')


I've noticed that there does not seem to be any enforced constraint on line size in Hanami but it does improve readability when lines do not exceed 80 columns (or 90, or 100 – 80 just seems to be the inherited standard from the physical terminal days).

I was actually wondering about this, since it seemed like the lines generally are shorter, but definitely not always.

Based on what you said, it seems like the standard is to just make sure the lines are as readable as possible... I can run through all my changes and see where they can be shortened to make it better.

I think it makes more sense to have one big separate rubocop cleanup PR that does not change behavior.

russCloak · 2016-10-10T22:25:58Z

@beauby thank you again for the review - I've made all of the changes you suggested. Would you please take another look and see if there's anything else?

coveralls · 2016-10-10T22:26:17Z

Coverage decreased (-0.2%) to 98.259% when pulling cd10781 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-10-10T22:26:17Z

Coverage decreased (-0.2%) to 98.259% when pulling cd10781 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-10-10T22:26:17Z

Coverage decreased (-0.2%) to 98.259% when pulling cd10781 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-10-10T22:39:06Z

Coverage decreased (-0.2%) to 98.339% when pulling 1a82655 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-10-10T22:39:06Z

Coverage decreased (-0.2%) to 98.339% when pulling 1a82655 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-10-10T22:39:06Z

Coverage decreased (-0.2%) to 98.339% when pulling 1a82655 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

coveralls · 2016-10-10T22:39:06Z

Coverage decreased (-0.2%) to 98.339% when pulling 1a82655 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

beauby · 2016-10-10T22:55:09Z

As far as I'm concerned it looks good 👍

russCloak · 2016-10-10T23:04:37Z

@cllns There have been some changes added since you last looked - would you mind giving it a once-over?

cllns · 2016-10-12T21:35:23Z

Seems fine to me, but I can't merge on this repo 😃

@jodosha is busy with rom integeration for hanami-model. @joneslee85 can you take a look?

russCloak · 2016-10-24T17:10:55Z

@cllns and @joneslee85 - any chance we could move forward with this PR?

I'm going to be using it either way, and I prefer that my company does not need to use only our fork of hanami/controller in order to continue using it.

Thanks in advance!

p.s. Please let me know if there's anything I can do to move this forward.

cllns · 2016-10-25T16:53:08Z

Deferring to @jodosha :)

russCloak · 2016-10-27T20:32:55Z

@jodosha or @joneslee85: Is there no way to get some motion on this PR? 😢

russCloak · 2016-10-28T18:34:19Z

Well, bummer. I'm going to close this PR since I can't get any momentum toward merging or changing it.

It's a shame, I do wish there were more active core members that could review it. In any case, the branch will persist, and it can be looked up if needed.

For now we'll just switch our company to our own fork and use that moving forward. Thanks to all who helped improve this code!

beauby · 2016-10-28T18:36:14Z

IMO this should be merged (whether in its current form or not is up to the maintainers, but the underlying issue is a bug).

cllns · 2016-10-28T20:18:38Z

@russCloak I understand you're frustrated at the lack of attention, but the core team members are all volunteers working in their spare time.

I expect this will be merged and you can get back over to using regular ole' hanami-controller soon :)

I do recommend, if you want this PR to get merged, that you should leave it open. I imagine it'll get overlooked if it's closed. 🌸

russCloak · 2016-10-28T20:25:20Z

@cllns Fair enough - I'll reopen. I just figured a stale PR is just as bad as a closed one.

Ideally it will get in eventually, and we'll head back over to this remote - thanks for the reply!

coveralls · 2016-10-28T20:29:45Z

Coverage decreased (-0.2%) to 98.339% when pulling 1a82655 on Acornsgrow:resolve_content_type_from_custom_formats into fde8975 on hanami:master.

jodosha · 2016-11-09T16:48:51Z

@russCloak I'm sorry if we're merging only today.

All of you @russCloak, @cyphactor, @beauby, and @cllns and it would be a shame that good collaboration like this would be lost. 💯

Please understand that we all do voluntary work and other stuff can take higher priority. For instance other parts of Hanami, our day jobs, our families.. It takes time the right to digest a month of discussions, to check the code, tweak it if needed. That's the reasons for the waiting. 🌸

That being said, thank you for this contribution. This is a great fix. 💚

russCloak added 4 commits September 30, 2016 15:06

drewdeponte reviewed Oct 1, 2016

View reviewed changes

russCloak added 2 commits October 4, 2016 11:25

drewdeponte reviewed Oct 4, 2016

View reviewed changes

Improve wording on test description

9bc4e2e

Technically we aren't dealing with all known types, it's only the ones known to the controller configuration, so this makes a lot more sense.

cllns reviewed Oct 5, 2016

View reviewed changes

beauby reviewed Oct 10, 2016

View reviewed changes

russCloak added 2 commits October 10, 2016 14:13

Remove extra "get_" from the method name

7999ba3

beauby reviewed Oct 10, 2016

View reviewed changes

russCloak added 2 commits October 10, 2016 15:20

Grammar improvements to documentation for mime.rb

35483e5

Bring lines in accept test under 80 characters

cd10781

Fix typo in test

1a82655

russCloak closed this Oct 28, 2016

russCloak reopened this Oct 28, 2016

jodosha self-assigned this Nov 9, 2016

jodosha added this to the v0.8.0 milestone Nov 9, 2016

jodosha added the fix label Nov 9, 2016

jodosha merged commit eb66e98 into hanami:master Nov 9, 2016

jodosha added a commit that referenced this pull request Nov 9, 2016

Cleanup for #179

306780e

Resolve Content-Type from custom and common MIME types via Accept header #179

Resolve Content-Type from custom and common MIME types via Accept header #179

Conversation

russCloak commented Sep 30, 2016 • edited Loading

coveralls commented Sep 30, 2016 • edited Loading

coveralls commented Sep 30, 2016

coveralls commented Sep 30, 2016

russCloak commented Sep 30, 2016

drewdeponte commented Oct 1, 2016

Choose a reason for hiding this comment

Scenarios

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russCloak commented Oct 4, 2016

coveralls commented Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 4, 2016

coveralls commented Oct 4, 2016 • edited Loading

drewdeponte commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 10, 2016 • edited Loading

coveralls commented Oct 10, 2016

coveralls commented Oct 10, 2016

Choose a reason for hiding this comment

russCloak Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russCloak commented Oct 10, 2016

coveralls commented Oct 10, 2016 • edited Loading

coveralls commented Oct 10, 2016

coveralls commented Oct 10, 2016

coveralls commented Oct 10, 2016

coveralls commented Oct 10, 2016

coveralls commented Oct 10, 2016

coveralls commented Oct 10, 2016

beauby commented Oct 10, 2016

russCloak commented Oct 10, 2016

cllns commented Oct 12, 2016 • edited Loading

russCloak commented Oct 24, 2016

cllns commented Oct 25, 2016

russCloak commented Oct 27, 2016

russCloak commented Oct 28, 2016

beauby commented Oct 28, 2016

cllns commented Oct 28, 2016 • edited Loading

russCloak commented Oct 28, 2016

coveralls commented Oct 28, 2016 • edited Loading

jodosha commented Nov 9, 2016

russCloak commented Sep 30, 2016 •

edited

Loading

coveralls commented Sep 30, 2016 •

edited

Loading

coveralls commented Oct 4, 2016 •

edited

Loading

coveralls commented Oct 4, 2016 •

edited

Loading

coveralls commented Oct 10, 2016 •

edited

Loading

russCloak Oct 10, 2016 •

edited

Loading

coveralls commented Oct 10, 2016 •

edited

Loading

cllns commented Oct 12, 2016 •

edited

Loading

cllns commented Oct 28, 2016 •

edited

Loading

coveralls commented Oct 28, 2016 •

edited

Loading