allow dashes, periods, and other RFC6838-compliant characters in header vendor #1170
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is good. It needs a CHANGELOG entry and possibly a README update, please. I would be down with a monkey-patch of rack-accept and a corresponding test. We have some precedent for patches in #1167 where we monkey-patched virtus. |
| # @param [String] media_type a content type | ||
| # @return [Boolean] whether the content type sets an API version | ||
| def version?(media_type) | ||
| _, subtype = Rack::Accept::Header.parse_media_type(media_type) | ||
| subtype[/\Avnd\.[a-z0-9*.]+-[a-z0-9*\-.]+/] | ||
| subtype[/\Avnd\.([a-z0-9.\-_!#\$&\^]+?)(?:-([a-z0-9*.]+))+/] |
There was a problem hiding this comment.
Should we extract this to a constant to avoid recreating the regex every time?
suan
force-pushed
the
better_vendor_resource_handling
branch
2 times, most recently
from
8a43c4f
to
8dac028
Compare
suan
force-pushed
the
better_vendor_resource_handling
branch
from
8dac028
to
460e443
Compare
|
@dblock I've updated the CHANGELOG, added a README section about Grape's limitations with the structure of versioned mediatypes, and converted those regexes to constants like recommended by @aselder I'm thinking I can make a followup PR where the appropriate rack-accept call is monkey-patched to allow all the RFC-compliant characters, once this PR is merged. |
|
Merging. |
dblock
added a commit
that referenced
this pull request
Oct 5, 2015
allow dashes, periods, and other RFC6838-compliant characters in header vendor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current accept header parsing logic treats everything after the first
-as the version, whereas it's not uncommon to have dashes in more complex vendor/resource media type definitions (an IANA-registered example isapplication/vnd.ms-office.activeX+xml)This change correctly handles dashes in the vendor part, while still assuming what's after the last dash is the version.
The current logic also doesn't allow underscores, ampersands, etc, which are allowed media type characters per RFC 6838. This change makes Grape itself handle these characters correctly, however for them to actually work is contingent on mjackson/rack-accept#17 which unfortunately has been open for more than 6 months (I'll ping maintainer again, but any weight you can throw around there would also be appreciated 😄)
P/S: It should be noted that the current header versioning logic is very limited and doesn't support formats such as
application/vnd.github.v3+json(as what github uses now) I'll try to add a caveat in the README while I add the CHANGELOG entry for this