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

Don't use .format suffix in paths if you only have one format #809

Merged
merged 2 commits into from
Nov 13, 2014

Conversation

ajvondrak
Copy link
Contributor

For example, say you have the following API:

require 'grape'

class API < Grape::API
  format :json

  get :hello do
    { hello: 'world' }
  end
end

run API

If you hit /hello, you'd get back the JSON {"hello":"world"}. This makes sense: you specifically say that your format is JSON.

However, before this commit, you could also hit /hello.xml, which would confusingly return the JSON {"hello":"world"}. Why bother exposing this path as a valid API endpoint when we're just going to ignore the format anyway?

In this commit, /hello.xml (or /hello.json or /hello.foobar) will all be 404s. We don't even entertain the notion of an optional format path parameter.

Old behavior is still preserved if you add different content types after your format :xxx declaration. So, if you have the following API:

require 'grape'

class API < Grape::API
  format :json
  content_type :xml, 'application/xml'

  get :hello do
    { hello: 'world' }
  end
end

run API

then /hello, /hello.json, /hello.xml, and /hello.foobar all return the JSON {"hello":"world"}. I don't know why you'd want this behavior either, but it was being enforced by the specs, so I preserved it.

What do you think?

@dblock
Copy link
Member

dblock commented Nov 12, 2014

I like this. It needs a CHANGELOG entry and possibly a README update around format selection.

@ajvondrak
Copy link
Contributor Author

Updated the docs. Do they all seem right?

@dblock
Copy link
Member

dblock commented Nov 12, 2014

Before I merge this...

Don't you think it's a little strange that 1 format = no automatic suffix, multiple formats = automatic suffix? What if default_format made that selection?

@ajvondrak
Copy link
Contributor Author

So if I understand you right...

class MyAPI < Grape::API
  default_format :json
  get :hello do { hello: 'world' } end
end

As it is: /hello returns JSON (that's the default format), /hello.xml returns XML (that's still a supported format), /hello.xls returns JSON (XLS is unsupported, JSON is the default).

Are you saying that if default_format made that selection: /hello returns JSON, /hello.xml is a 404, /hello.xls is a 404...?

It makes sense to me that 1 format = no automatic suffix, because there's nothing the suffix would do - you have no choice but to have the API return JSON (short of env['api.format'] mangling and such). Unless I'm misunderstanding what format :json is supposed to do. The way I understand, it's saying "this is the one and only format for the whole API". Whereas default_format is saying "we have a bunch of formats available, but fall back to this one".

@dblock
Copy link
Member

dblock commented Nov 13, 2014

Ok, I completely agree with you, thanks. Merging.

dblock added a commit that referenced this pull request Nov 13, 2014
Don't use `.format` suffix in paths if you only have one format
@dblock dblock merged commit 2302e26 into ruby-grape:master Nov 13, 2014
@dblock
Copy link
Member

dblock commented Nov 29, 2014

@ajvondrak: This needs an entry to UPGRADING, please. Before you had /foobar.json working and that now has become /foobar whereas .json is going to return a 404.

dblock added a commit to dblock/grape-swagger that referenced this pull request Nov 29, 2014
dblock added a commit to dblock/grape-swagger that referenced this pull request Nov 29, 2014
dblock added a commit to dblock/grape-swagger that referenced this pull request Nov 29, 2014
ajvondrak pushed a commit to ajvondrak/grape that referenced this pull request Dec 3, 2014
@ajvondrak
Copy link
Contributor Author

@dblock Done.

ajvondrak pushed a commit to ajvondrak/grape that referenced this pull request Dec 3, 2014
@dblock
Copy link
Member

dblock commented Dec 3, 2014

Can you make a new PR for that please?

dblock added a commit that referenced this pull request Dec 5, 2014
@dblock dblock mentioned this pull request Dec 22, 2014
@ur5us
Copy link

ur5us commented Jan 27, 2015

I just tried to upgrade one of our apps from Grape 0.9.0 to ~> 0.10.0. It took me hours to debug the issue as to why .json in the URL wouldn't work, I simply missed how important this change was. Also, this change breaks Swagger UI as described in https://github.com/tim-vandecasteele/grape-swagger/issues/187. I'm not sure whether it's just our installation of Swagger UI but pointing the Swagger UI demo at our own API didn't work either (probably that's outdated, too?). Moreover, using grape-swagger-rails is not really an option as it'd downgrade the grape-swagger gem.

Changing format :json to

content_type :json, 'application/json'
default_format :json

actually fixes the problem. Now, with that change the following URLs will work:

/users.json
/users

and both will return the same. To be honest, that's what I'd expect when using format :json. It should actually respond to both URLs, with and without the suffix. Moreover, for any other format I'd actually expect a 406 Not acceptable header instead of an JSON response. So in my opinion this change goes too far for the whole suffix discussion and not far enough when it comes to asking for a response format which is actually not supported.

@dblock
Copy link
Member

dblock commented Jan 27, 2015

I had a similar sentiment a few times. I would be open to undoing this change carefully as you describe, if you want to try a PR.

@ur5us
Copy link

ur5us commented Jan 27, 2015

Alright, though I'll probably aim for several PRs as a lot of the things I've described are actually different/non-related things.

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