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

Fix broken multiple mounts. #2053

Merged
merged 1 commit into from
May 18, 2020

Conversation

Jack12816
Copy link
Contributor

- What is it good for

There is a nasty bug on the Grape::API::Instance#add_head_not_allowed_methods_and_options_methods method which causes a FrozenError when an API class is mounted multiple times. I'm not quite sure why the mounts happens multiple times at my setup. My application includes:

1.3.2  grape
0.8.0  grape-entity
1.3.0  grape-jwt-authentication
2.1.0  grape-route-helpers
1.1.0  grape-swagger
0.3.4  grape-swagger-entity
0.3.1  grape-swagger-rails

This is the actual stack trace of the issue:

FrozenError (can't modify frozen Array):

grape (1.3.2) lib/grape/api/instance.rb:208:in `block (2 levels) in add_head_not_allowed_methods_and_options_methods'
grape (1.3.2) lib/grape/api/instance.rb:199:in `each'
grape (1.3.2) lib/grape/api/instance.rb:199:in `block in add_head_not_allowed_methods_and_options_methods'
grape (1.3.2) lib/grape/api/instance.rb:197:in `each'
grape (1.3.2) lib/grape/api/instance.rb:197:in `add_head_not_allowed_methods_and_options_methods'
grape (1.3.2) lib/grape/api/instance.rb:156:in `initialize'
grape (1.3.2) lib/grape/api/instance.rb:52:in `new'
grape (1.3.2) lib/grape/api/instance.rb:52:in `compile'
grape (1.3.2) lib/grape/api/instance.rb:85:in `block in compile!'
grape (1.3.2) lib/grape/api/instance.rb:85:in `synchronize'
grape (1.3.2) lib/grape/api/instance.rb:85:in `compile!'
grape (1.3.2) lib/grape/api/instance.rb:65:in `call'
grape (1.3.2) lib/grape/api.rb:68:in `call'  

- What I did

The offending line at the Grape::API::Instance just assigns a frozen array constant to the routing settings. When using CONST.dup we can simply unfreeze it for later changes.

# Causes FrozenError's when later touched by +#<<+ for example
route_settings[:methods] = Grape::Http::Headers::SUPPORTED_METHODS

- A picture of a cute animal (not mandatory but encouraged)

Download

@Jack12816 Jack12816 force-pushed the any-route-frozen-array branch from 4502ef0 to 9b1512b Compare May 14, 2020 08:21
@Jack12816 Jack12816 force-pushed the any-route-frozen-array branch from e350198 to 0089ddd Compare May 14, 2020 09:16
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Good, thank you. Some nitpicky comments from me please before we can merge?

lib/grape/api/instance.rb Outdated Show resolved Hide resolved
lib/grape/api/instance.rb Outdated Show resolved Hide resolved
end
end

it 'does not raise a FrozenError on first instance' do
Copy link
Member

Choose a reason for hiding this comment

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

I understand that the bug is a frozen error raised, but let's rewrite this as a positive test of what you expect the response to be. Otherwise in the future when the behavior changes but no exceptions are raised we won't catch this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct me if I'm getting you wrong, but there is a positive and a negative test?

it 'responds the correct body at the first instance' 
it 'does not raise a FrozenError on first instance'

Could you explain this in more detail please? :)

Copy link
Member

Choose a reason for hiding this comment

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

The first covers the second in terms of functionality.

The second standalone is a misleading test. It would make sense if the call in some cases was supposed to raise FrozenError and we explicitly wanted to ensure that in certain conditions that's not the case.

CHANGELOG.md Show resolved Hide resolved
spec/grape/api/instance_spec.rb Outdated Show resolved Hide resolved
@Jack12816 Jack12816 force-pushed the any-route-frozen-array branch from 0089ddd to 4bd6502 Compare May 18, 2020 11:55
@dblock
Copy link
Member

dblock commented May 18, 2020

You still have to remove the manually added # rubocop:enable... in the code, I am afraid -a doesn't get rid of them.

@Jack12816 Jack12816 force-pushed the any-route-frozen-array branch from 4bd6502 to 415630c Compare May 18, 2020 11:59
Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com>
@Jack12816 Jack12816 force-pushed the any-route-frozen-array branch from 415630c to c141628 Compare May 18, 2020 12:32
@dblock dblock merged commit 8b60289 into ruby-grape:master May 18, 2020
@dblock
Copy link
Member

dblock commented May 18, 2020

I merged this, thank you.

@Jack12816
Copy link
Contributor Author

Thank you!

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