Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support JSON-derived media types #1583

Merged
merged 4 commits into from Apr 30, 2014
Merged

Support JSON-derived media types #1583

merged 4 commits into from Apr 30, 2014

Conversation

@joshua-mcginnis
Copy link
Contributor

@joshua-mcginnis joshua-mcginnis commented Apr 21, 2014

By default, Hapi returns a 415: Unsupported Media Type error if you try and send a content-type header that is a json derived media type.

Examples include the media types for various hypermedia specs and json patch:

  • application/json-patch+json
  • application/vnd.collection+json
  • application/hal+json
  • application/vnd.siren+json

This PR will go ahead and json parse these, as well as the more typical application/json.

@joshua-mcginnis joshua-mcginnis changed the title Support JSON-derived media types PR: Support JSON-derived media types Apr 22, 2014
@joshua-mcginnis joshua-mcginnis changed the title PR: Support JSON-derived media types Support JSON-derived media types Apr 22, 2014
@joshua-mcginnis
Copy link
Contributor Author

@joshua-mcginnis joshua-mcginnis commented Apr 29, 2014

@hueniverse any eta on when this might be merged in?

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Apr 29, 2014

Working my way through the backlog

Loading

@nlf
Copy link
Member

@nlf nlf commented Apr 29, 2014

FWIW it would be nice to see some tests for this, making sure that your examples and application/json all work. Aside from that, the patch looks good to me.

Loading

@joshua-mcginnis
Copy link
Contributor Author

@joshua-mcginnis joshua-mcginnis commented Apr 30, 2014

There's already tests which test application/json. I could add a test for application/foo+json, but I initially thought it might be redundant. If you think it'd be useful, I'll do it.

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Apr 30, 2014

If we take your code out, all the tests still pass, which means that it is useless code... :-)

Loading

@joshua-mcginnis
Copy link
Contributor Author

@joshua-mcginnis joshua-mcginnis commented Apr 30, 2014

Test added.

Loading

@@ -254,9 +254,9 @@ internals.object = function (payload, mime, next) {

// JSON

if (mime === 'application/json') {
if (mime.match(/^application\/(?:.+[+])?json$/)) {
Copy link
Contributor

@hueniverse hueniverse Apr 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch match() with test() (faster) and we're good to go.

Loading

Copy link
Contributor Author

@joshua-mcginnis joshua-mcginnis Apr 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All set.

Loading

@hueniverse hueniverse self-assigned this Apr 30, 2014
@@ -254,9 +254,9 @@ internals.object = function (payload, mime, next) {

// JSON

if (mime === 'application/json') {
if (/^application\/(?:.+[+])?json$/.test(mime)) {
Copy link
Contributor

@hueniverse hueniverse Apr 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just noticed: /^application\/(?:.+[+])?json$/ should be /^application\/(?:.+\+)?json$/

Loading

Copy link
Contributor Author

@joshua-mcginnis joshua-mcginnis Apr 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference? One matches the character + while the other makes + literal. Both are wrapped in an optional match. Happy to make the change, just curious.

Edit: It's speed:
http://jsperf.com/char-match-vs-escape-match

Change pushed.

Loading

@hueniverse hueniverse added this to the 4.1.0 milestone Apr 30, 2014
hueniverse pushed a commit that referenced this issue Apr 30, 2014
@hueniverse hueniverse merged commit 1662ff2 into hapijs:master Apr 30, 2014
1 check passed
Loading
@joshua-mcginnis joshua-mcginnis deleted the jsonDerivedMediaTypes branch May 9, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants