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

API version overriding version parameter #394

Closed
wants to merge 1 commit into from
Closed

API version overriding version parameter #394

wants to merge 1 commit into from

Conversation

tmornini
Copy link
Contributor

Hello there fine fellows!

Beginning to use and love Grape, but have run into a serious issue which I am unable to resolve myself.

I have an endpoint described as such:

params do
  optional :description, :type => String,  :desc => 'Book description'
  optional :reference,   :type => String,  :desc => 'Book reference'
  requires :version,     :type => Integer, :desc => 'Book current version'
end

This works fine with API versioning via headers when I pass in this body:

{"description":"Updated Description","reference":"b:a","version":2}

However, when I use API versioning via path, the version parameter is set to the API version from the path, which results in a 403 with this body:

{"error":"invalid parameter: version"}

I verified this at coerce.rb by adding these lines after line 14:

puts attr_name
puts params[attr_name]

This produces:

version
pacioli

where pacioli is my API version from:

class App < ::Grape::API
  version 'pacioli', :using => :path

I believe this to be 100% incorrect. :-)

Let me know if I can help in any way!

P.S. The 403 is weird in itself as line 15 of coerce.rb certainly appears to throw a 400.

@tmornini
Copy link
Contributor Author

Hacked on this all day and came up with this fix.

Thanks for Grape, I really appreciate it!

@tmornini
Copy link
Contributor Author

Hold on this, though specs are green, this issue does not appear to be entirely fixed...

@tmornini
Copy link
Contributor Author

Strike that! Reverse it... (said in a Gene Wilder as Willy Wonka voice)

This is ready to go, issue was header-related on my side. :-)

@tmornini
Copy link
Contributor Author

Newer and simpler, and no longer breaks(!) grape-swagger :-)

@dblock
Copy link
Member

dblock commented Apr 28, 2013

Thanks! Can you please update CHANGELOG, too?

@dblock
Copy link
Member

dblock commented Apr 28, 2013

There're a ton of whitespace additions. I am sure you feel that it looks better, but would you mind cleaning up those, the implicit coding style is less sparse in Grape :) Nothing person, just want to minimize change.


versions = settings[:version]

version = versions.nil? ? nil : versions.join( '|' )
Copy link
Member

Choose a reason for hiding this comment

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

This can be written more concisely as version = versions.join('|') if versions.

@tmornini
Copy link
Contributor Author

@dblock Thanks the input, I'd be more than happy to make the changes you've suggested!

@dblock
Copy link
Member

dblock commented Apr 28, 2013

Thanks, looking forward to an update.

@tmornini
Copy link
Contributor Author

OK, how's that?

@dblock
Copy link
Member

dblock commented Apr 29, 2013

I committed a fix for this in c065f33, using your code, but without all the other unrelated changes. Let me know if I missed anything. Thx.

@dblock dblock closed this Apr 29, 2013
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.

None yet

2 participants