incorrect version accept header does not return a 406 response #1101
Conversation
|
Add to CHANGELOG and squash the commits please? |
| end | ||
|
|
||
| def media_type | ||
| @media_type ||= header.best_of available_media_types |
There was a problem hiding this comment.
I would add a pair of ( ), so header.best_of(available_media_types), just more readable. There're a couple of other instances like this,
elliotlarson
force-pushed
the
master
branch
3 times, most recently
from
8d3b7cb
to
b31da8c
Compare
|
Thanks for your code review, dblock! I've added these updates in. |
|
This looks good. Can I have someone else confirm that I am not missing anything here, cause this bug is a bit big to be true :) Maybe @dm1try? Thx. |
|
Yeah, that makes sense. Thanks for your time on this. |
| env['api.version'] = Regexp.last_match[2] | ||
| # weird that Grape::Middleware::Formatter also does this | ||
| env['api.format'] = Regexp.last_match[3] | ||
| elsif versions.size > 0 |
There was a problem hiding this comment.
this condition and expectation seems strange to me.
if size of predefined versions more than zero then fail with parsed "vendor or version not found"
as we can see the versions has a predefined value: options[:versions] => [:v1]
so if you remove in the context of your spec version value :v1 then it'll fail again.
I mean it's not expected behaviour.
There was a problem hiding this comment.
Agreed. This was a bit of a kludgy fix on my part. I tried to make the smallest possible change to get the desired result. Now that I've gone down the road of doing a bit of refactoring, I feel like this is the wrong place for this logic. I've updated the code a bit to reflect what I think is probably a better approach.
|
Anyway the original behaviour was implemented here see the so ATM it's can be easily fixed by adding maybe a better way is removing the logic which allow to work with default content types from this middleware at all but it's a breaking change. (or move this to |
|
@elliotlarson Can you please see @dm1try's comments above? @dm1try What do you think of the code change itself, better? worse? no different? |
|
@dblock definitely better! |
If you've configured Grape to use accept header versioning with strict equal to true, Grape should return a 406 response when making a request with an incorrect version header. However, the system was not responding with a 406 when using the header 'Accept:application/xml'. Adding relevant line to changelog. Refactoring the middleware version header class: - Breaking up the before method into more manageable pieces, removing some of the conditional (cyclomatic) complexity - fixing some line lengths - removing some local variables from methods
|
@dm1try Thanks for taking the time to review and provide feedback. I agree that there is a bit of muddled responsibility in this middleware. This could use some more time and refactoring. My hope is to fix this bug with a reasonable, incremental improvement to the code, and then perhaps dig in a bit further after that's out of the way and see if there is a good way to untangle the responsibilities. I've refactored my fix a bit. I feel like returning a Also, the original code had some logic for media type header checking if Anyway, I hope this is a better solution that meets with your approval. I look forward to your feedback. |
| expect(exception.message).to include('Accept header must not contain ranges ("*").') | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
For the record, would you be so kind to explain why this is no longer something we need?
There was a problem hiding this comment.
Yeah, sorry, I tried to explain this in the third paragraph above, but this may need a bit more explanation.
The existing code is doing some checking to make sure that you're not passing in media type ranges. However, these checks are only happening in the case when strict is set to true. If strict is set to true and you're passing in an accept header like Accept:application/*, it's my contention that returning an error about not allowing media ranges is less important that returning an error about not passing in a correct version/vendor header.
In other words, with strict set to true, I think using Accept:application/* should result in:
406: API vendor or version not found.
instead of:
406: Accept header must not contain ranges ("*").
... because the larger issue is that the vendor/version accept header is missing.
As I was refactoring the code, I placed the version/vender header checking mechanism before the media range checks. After I did this, it became clear that these media range checks were never getting exercised because the version vendor checks were always returning the 406 error first. So, pulling out the media range checks and the related specs seemed prudent.
Of course, there may be some untested/un-documented outcome that I'm not seeing here related to media type accept headers. Beyond the README and the specs, is there some kind of written spec or standard that you're intending to follow that might provide more guidance here?
There was a problem hiding this comment.
This makes perfect sense, thanks.
|
@dm1try Would you mind making calls around merging this? I don't feel like I'm qualified :) Thanks. |
|
@elliotlarson this is great, thanks! |
|
Merged via d7718f0, thank you. |
|
Thanks for hanging along @elliotlarson, great job! |
If you've configured Grape to use accept header versioning with strict
equal to true, Grape should return a 406 response when making a request
with an incorrect version header. However, the system was not
responding with a 406 when using the header 'Accept:application/xml'.