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

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
@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.

Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com>
@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.

None yet

3 participants