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.

Sign up for GitHub

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

Jump to bottom

Fix multiple version definitions for path versioning #1414

Merged
merged 3 commits into from
Jun 8, 2016
Merged

Fix multiple version definitions for path versioning #1414

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fae969a
Fix multiple version definitions for path versioning
304 Jun 5, 2016
b0b9586
Restructure specs
304 Jun 7, 2016
82e1f21
Add local variable to keep versions from args
304 Jun 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* [#1365](https://github.com/ruby-grape/grape/pull/1365): Fix finding exception handler in error middleware - [@ktimothy](https://github.com/ktimothy).
* [#1380](https://github.com/ruby-grape/grape/pull/1380): Fix `allow_blank: false` for `Time` attributes with valid values causes `NoMethodError` - [@ipkes](https://github.com/ipkes).
* [#1384](https://github.com/ruby-grape/grape/pull/1384): Fix parameter validation with an empty optional nested `Array` - [@ipkes](https://github.com/ipkes).
* [#1414](https://github.com/ruby-grape/grape/pull/1414): Fix multiple version definitions for path versioning - [@304](https://github.com/304).

0.16.2 (4/12/2016)
==================
Expand Down
1 change: 1 addition & 0 deletions lib/grape/dsl/routing.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ module ClassMethods
#
def version(*args, &block)
if args.any?
args.flatten!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem was hidden here.
When the version is defined as an array:

version ['v1', 'v2']

then args is presented as an array inside of an array: [["v1", "v2"]], so flatten! here helps to resolve the issue.

Copy link
Member

Choose a reason for hiding this comment

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

You should not be modifying the parameters passed in, so something like args = args.flatten. However, do investigate why you're getting an array of arrays here, it's possible that there's a better fix upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I've extracted version parameter into local variable to describe the intention in a more explicit way => 82e1f21

In this case the upstream is defined by user. I've checked Grape documentation, but I've not found any examples how to properly define several versions for the endpoint. Looks like it is an undocumented feature that was supporting arrays before routing refactoring.

Should it be like that version ['v1', 'v2'] or like that version 'v1', 'v2' or it is not a correct way to define a version at all?

options = args.extract_options!
options = options.reverse_merge(using: :path)

Expand Down
11 changes: 10 additions & 1 deletion spec/grape/api_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,10 @@ def subject.enable_root_route!
end

describe 'path versioned APIs' do
let(:version) { 'v1' }

before do
subject.version 'v1', using: :path
subject.version version, using: :path
subject.enable_root_route!
end

Expand All @@ -313,6 +315,13 @@ def subject.enable_root_route!
it 'with a format' do
get '/v1/.json'
end

context 'when array of versions provided' do
Copy link
Member

Choose a reason for hiding this comment

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

The above should be in its own contect context since for example let(:version) isn't used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done => b0b9586

let(:version) { %w(v1 v2) }

it { versioned_get '/', 'v1', using: :path }
it { versioned_get '/', 'v2', using: :path }
end
end

it 'header versioned APIs' do
Expand Down