Skip to content

Loosen Accept header check to allow starts with :api_json#869

Merged
lgebhardt merged 1 commit intoJSONAPI-Resources:masterfrom
pixelhandler:loosen-accept-header-check
Oct 12, 2016
Merged

Loosen Accept header check to allow starts with :api_json#869
lgebhardt merged 1 commit intoJSONAPI-Resources:masterfrom
pixelhandler:loosen-accept-header-check

Conversation

@pixelhandler
Copy link
Copy Markdown
Contributor

@pixelhandler pixelhandler commented Oct 8, 2016

  • Allow the check for the Accept header to begin with */* (All media types)
    • For example, "*/*;q=0.8"

Fixes #799

@pixelhandler
Copy link
Copy Markdown
Contributor Author

@lgebhardt just checking in, what do you think about loosening up the check for a valid Accept header so that the check matches that the header string begins with the "all media types" or :json_api mime type?

@lgebhardt
Copy link
Copy Markdown
Contributor

@pixelhandler The first change for allowing "*/*;q=0.8" seems fine to me. But I have my concerns that by loosening things up regarding the media type parameters on "application/vnd.api+json;version=1" we are in violation of the spec:

Clients that include the JSON API media type in their Accept header MUST specify the media type there at least once without any media type parameters.

But maybe I'm reading it wrong. I'd like to get @dgeb's opinion.

@lgebhardt
Copy link
Copy Markdown
Contributor

I should add I have no issue with allowing a configuration option to allow this, but I want to make sure the defaults adhere to the spec.

@pixelhandler
Copy link
Copy Markdown
Contributor Author

pixelhandler commented Oct 12, 2016

@pixelhandler
Copy link
Copy Markdown
Contributor Author

@lgebhardt I can change this PR to remove the support for application/vnd.api+json;version=1 and do that in a separate PR cc\ @dgeb

@dgeb
Copy link
Copy Markdown
Contributor

dgeb commented Oct 12, 2016

I can change this PR to remove the support for application/vnd.api+json;version=1 and do that in a separate PR

@pixelhandler Please do so, but open an additional issue instead of PR to discuss.

There's a lot to unpack here, not least of which would be my preference to get JSONAPI out of the vendor tree and into the standards tree. But regardless, we can have a discussion about media type params in a separate issue if you like. I would like to get the change to */* merged sooner than later.

@dgeb
Copy link
Copy Markdown
Contributor

dgeb commented Oct 12, 2016

@pixelhandler I see you just commented here: json-api/json-api#406 (comment)

That's probably the best place for the discussion, since JR will simply follow the spec. Thanks for bringing this up!

@pixelhandler pixelhandler force-pushed the loosen-accept-header-check branch from 12606ad to ee8cf2c Compare October 12, 2016 16:53
@pixelhandler
Copy link
Copy Markdown
Contributor Author

pixelhandler commented Oct 12, 2016

@lgebhardt @dgeb I revised the PR and squashed the commit, thanks for the feedback. I also created https://github.com/cerebris/jsonapi-resources/issues/874 to track any possibility of configuring for using a version media type parameter.

@lgebhardt lgebhardt merged commit 54cd05d into JSONAPI-Resources:master Oct 12, 2016
@lgebhardt
Copy link
Copy Markdown
Contributor

@pixelhandler Thanks!

@pixelhandler
Copy link
Copy Markdown
Contributor Author

@lgebhardt you're welcome. It's a pleasure to use this gem !

@lgebhardt
Copy link
Copy Markdown
Contributor

And I generally agree with http://blog.steveklabnik.com/posts/2011-07-03-nobody-understands-rest-or-http#i_want_my_api_to_be_versioned

@pixelhandler I tend to agree with @steveklabnik's comment in the intro:

Since I've posted this, I've refined a few of my positions on things. Everyone learns and grows, and while I still stand by most of what I said, I specifically don't agree that versioning the media type is how to properly version APIs. Hypermedia APIs should not actually use explicit versioning, but I'd rather see a version in the URI with HATEOAS than no HATEOAS and versioned media types.

I'm approaching the versioning from the standpoint that a server will support multiple versions simultaneously. In JR this is easily done with the URI and name-spaced resources. It would not be an easy retrofit to allow version switching based on the media type. However I can see using the media type version as a version check. Are you designing your server to speak multiple versions, or using the version to indicate the current version that the server is speaking? Hope that question makes sense.

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