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

fixes configuration inside namespaced params scope #2152

Merged
merged 2 commits into from
Jan 26, 2021
Merged

fixes configuration inside namespaced params scope #2152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f627044
fixes configuration inside namespaced params scope
fsainz Jan 19, 2021
a55dee9
adds changlog line
fsainz Jan 25, 2021
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 @@ -13,6 +13,7 @@
* [#2137](https://github.com/ruby-grape/grape/pull/2137): Fix typos - [@johnny-miyake](https://github.com/johnny-miyake).
* [#2131](https://github.com/ruby-grape/grape/pull/2131): Fix Ruby 2.7 keyword deprecation warning in validators/coerce - [@K0H205](https://github.com/K0H205).
* [#2132](https://github.com/ruby-grape/grape/pull/2132): Use #ruby2_keywords for correct delegation on Ruby <= 2.6, 2.7 and 3 - [@eregon](https://github.com/eregon).
* [#2152](https://github.com/ruby-grape/grape/pull/2152): Fix configuration method inside namespaced params - [@fsainz](https://github.com/fsainz).

### 1.5.1 (2020/11/15)

Expand Down
2 changes: 1 addition & 1 deletion lib/grape/validations/params_scope.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def initialize(opts, &block)
end

def configuration
@api.configuration.evaluate
@api.configuration.respond_to?(:evaluate) ? @api.configuration.evaluate : @api.configuration
Copy link
Member

@dblock dblock Jan 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This works, but I worry that we're having something together. How do we end up here? What do we end up having here as @api.configuration? What's the call stack?

Copy link
Contributor Author

@fsainz fsainz Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi there dblock :)

This is the callstack without namespace

 "lib/grape/validations/params_scope.rb:42:in `configuration'",
 "lib/grape/validations/params_scope.rb:36:in `instance_eval'",
 "lib/grape/validations/params_scope.rb:36:in `initialize'",
 "lib/grape/dsl/validations.rb:42:in `new'",
 "lib/grape/dsl/validations.rb:42:in `params'",
 "lib/grape/api.rb:155:in `replay_step_on'",
 "lib/grape/api.rb:105:in `block in replay_setup_on'",
 "ruby/2.7.1/lib/ruby/2.7.0/set.rb:328:in `each_key'",
 "ruby/2.7.1/lib/ruby/2.7.0/set.rb:328:in `each'",
 "lib/grape/api.rb:104:in `replay_setup_on'",
 "lib/grape/api.rb:97:in `mount_instance'",
 "lib/grape/dsl/routing.rb:87:in `block in mount'",
 "lib/grape/dsl/routing.rb:84:in `each_pair'",
 "lib/grape/dsl/routing.rb:84:in `mount'",
 "lib/grape/api.rb:155:in `replay_step_on'",
 "lib/grape/api.rb:147:in `block in add_setup'",
 "lib/grape/api.rb:146:in `each'",
 "lib/grape/api.rb:146:in `add_setup'",
 "lib/grape/api.rb:42:in `block (2 levels) in override_all_methods!'",

and with namespace

 "lib/grape/validations/params_scope.rb:42:in `configuration'",
 "lib/grape/validations/params_scope.rb:36:in `instance_eval'",
 "lib/grape/validations/params_scope.rb:36:in `initialize'",
 "lib/grape/dsl/validations.rb:42:in `new'",
 "lib/grape/dsl/validations.rb:42:in `params'",
 "lib/grape/api/instance.rb:120:in `instance_eval'",
 "lib/grape/api/instance.rb:120:in `block in evaluate_as_instance_with_configuration'",
 "lib/grape/util/lazy_block.rb:11:in `evaluate_from'",
 "lib/grape/api/instance.rb:127:in `evaluate_as_instance_with_configuration'",
 "lib/grape/api/instance.rb:107:in `block in nest'",
 "lib/grape/api/instance.rb:107:in `each'",
 "lib/grape/api/instance.rb:107:in `nest'",
 "lib/grape/dsl/routing.rb:173:in `block in namespace'",
 "lib/grape/dsl/settings.rb:160:in `within_namespace'",
 "lib/grape/dsl/routing.rb:170:in `namespace'",
 "lib/grape/api.rb:155:in `replay_step_on'",
 "lib/grape/api.rb:105:in `block in replay_setup_on'",
 "ruby/2.7.1/lib/ruby/2.7.0/set.rb:328:in `each_key'",
 "ruby/2.7.1/lib/ruby/2.7.0/set.rb:328:in `each'",
 "lib/grape/api.rb:104:in `replay_setup_on'",
 "lib/grape/api.rb:97:in `mount_instance'",
 "lib/grape/dsl/routing.rb:87:in `block in mount'",
 "lib/grape/dsl/routing.rb:84:in `each_pair'",
 "lib/grape/dsl/routing.rb:84:in `mount'",
 "lib/grape/api.rb:155:in `replay_step_on'",
 "lib/grape/api.rb:147:in `block in add_setup'",
 "lib/grape/api.rb:146:in `each'",
 "lib/grape/api.rb:146:in `add_setup'",
 "lib/grape/api.rb:42:in `block (2 levels) in override_all_methods!'",

the configuration gets evaluated in this step
"lib/grape/api/instance.rb:127:inevaluate_as_instance_with_configuration'`

def evaluate_as_instance_with_configuration(block, lazy: false)
  lazy_block = Grape::Util::LazyBlock.new do |configuration|
    value_for_configuration = configuration
    if value_for_configuration.respond_to?(:lazy?) && value_for_configuration.lazy?
      self.configuration = value_for_configuration.evaluate
    end
    #...
  end
end

but this does not answer your question. I am not familiar with the internals of the gem and I can imagine that changing how those lazy evaluations flow might lead to performance issues or other side-effects...

To me it seems that this would be a safe patch since the configuration has already been evaluated anyway but it is probably not addressing the underlying issue

end

# @return [Boolean] whether or not this entire scope needs to be
Expand Down
13 changes: 9 additions & 4 deletions spec/grape/api_remount_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,19 +340,24 @@ def app
context 'when the configuration is read within a namespace' do
before do
a_remounted_api.namespace 'api' do
params do
requires configuration[:required_param]
end
get "/#{configuration[:path]}" do
'10 votes'
end
end
root_api.mount a_remounted_api, with: { path: 'votes' }
root_api.mount a_remounted_api, with: { path: 'scores' }
root_api.mount a_remounted_api, with: { path: 'votes', required_param: 'param_key' }
root_api.mount a_remounted_api, with: { path: 'scores', required_param: 'param_key' }
end

it 'will use the dynamic configuration on all routes' do
get 'api/votes'
get 'api/votes', param_key: 'a'
expect(last_response.body).to eql '10 votes'
get 'api/scores'
get 'api/scores', param_key: 'a'
expect(last_response.body).to eql '10 votes'
get 'api/votes'
expect(last_response.status).to eq 400
end
end

Expand Down