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

JSON Content-Type matcher is more inclusive #242

Merged
merged 1 commit into from
Nov 17, 2015

Conversation

galiat
Copy link
Contributor

@galiat galiat commented Oct 29, 2015

Supports

  • application/json
  • application/vnd.api+json
  • application/vnd.api+json; supported-ext="bulk,jsonpatch"

the JSON API spec uses application/vnd.api+json
http://jsonapi.org/extensions/#extension-negotiation

@galiat
Copy link
Contributor Author

galiat commented Oct 29, 2015

@oestrich - this modifies some of your code.

I realized browsing other pull requests, there is another one, trying to solve the same problem by @AlexandrZ
#236

@jakehow
Copy link
Member

jakehow commented Oct 29, 2015

@galiat I don't think it makes sense to hardcode this here, there are infinite types of vendor specific content types. Would prefer if this was a configurable array w/ the default value of application/json, if the user is using some subset of JSON that they still want to pass to the parser by default, they can add their specific type.

@galiat
Copy link
Contributor Author

galiat commented Oct 29, 2015

@jakehow I'm glad you brought that up. I was considering the same thing, but wasn't sure the best way to set up the configuration.

  1. be verbose
    config.pretty_print_response_as_json_for_content_types = [/application\/json/, /application\/vnd.api+json]

  2. mirror post_body_format.
    config.response_body_format = Proc.new { |params| params }
    config.response_body_format = :json

Are there other options? What do you think?

@oestrich
Copy link
Contributor

I definitely like the configuration approach more, @jakehow beat me to it!

I'd mirror post body format, keep response/request linked.

@galiat galiat force-pushed the more-inclusive-json-content-types branch 4 times, most recently from b9ef8bd to 53bbd65 Compare November 9, 2015 16:36
@galiat
Copy link
Contributor Author

galiat commented Nov 9, 2015

@oestrich & @jakehow
How's this?

I can push up a subsequent PR so that users can specify :json or :xml along with a proc.

What do you think about the default value? If you are ok with not formatting based on application/json content type, I can simplify things by having the proc & record_response_body only have 1 parameter. Downside is that existing users are probably relying on this autoformatting.

@jakehow
Copy link
Member

jakehow commented Nov 9, 2015

@galiat IMO json == application/json

If you are using a specific subset vendor content type it should have a name other than json. Maybe json_api for example.

@galiat
Copy link
Contributor Author

galiat commented Nov 9, 2015

@jakehow We might be misunderstanding each other. Are the symbols (:json, :xml) assigned to post_body_format tied to the Content-Type? This pr doesn't use symbols to specify formatting anyhow.

@jakehow
Copy link
Member

jakehow commented Nov 9, 2015

@galiat sorry, did not notice code changed... it looks good to me, I misunderstood your comment.

I think we should keep the default behavior to preserve backwards compatibility, it could be expanded to include other standard content types if necessary. The config option should allow anyone using a non-standard type like JSON-API the flexibility they need.

You probably always want the proc to get the response_content_type as it is possible an app may respond with > 1 variation.

@galiat
Copy link
Contributor Author

galiat commented Nov 10, 2015

@jakehow is there anything left that I should do before merge?

@@ -138,7 +138,12 @@ RspecApiDocumentation.configure do |config|

# Change how the post body is formatted by default, you can still override by `raw_post`
# Can be :json, :xml, or a proc that will be passed the params
config.post_body_formatter = Proc.new { |params| params }
config.response_body_formatter = Proc.new { |params| params }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be flipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops - 😳 Yeap, I'll fix this.

edit: fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakehow & @oestrich sorry - I edited the original commit - which might not be too obvious via github notifications. I fixed the mistake in documentation. Is this ok to merge now?

…r to the post_body_formatter configuration.

Maintains backwards compatibility with pretty formatting content-types of `application/json`

the JSON API spec uses `application/vnd.api+json`
http://jsonapi.org/extensions/#extension-negotiation
@galiat galiat force-pushed the more-inclusive-json-content-types branch from 53bbd65 to f3475f0 Compare November 10, 2015 22:11
oestrich added a commit that referenced this pull request Nov 17, 2015
JSON Content-Type matcher is more inclusive
@oestrich oestrich merged commit 878ee79 into zipmark:master Nov 17, 2015
@oestrich
Copy link
Contributor

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants