From affd474311fe9cb4dbbcb958742b73cb27037ffb Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Tue, 27 Oct 2020 08:48:00 +0200 Subject: [PATCH] fix a performance issue with dependent params MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://github.com/ruby-grape/grape/issues/2100 The reason was in `ActiveSupport::HashWithIndifferentAccess`, it is super expensive. When users use a `Grape::Extensions::ActiveSupport::HashWithIndifferentAccess::ParamBuilder` or `Grape::Extensions::Hashie::Mash::ParamBuilder` parameter builder there is no change. However, users who use `Grape::Extensions::Hash::ParamBuilder` must make sure a parameter to be dependent on must be a symbol. given :matrix do # block here end Benchmark after this fix: Warming up -------------------------------------- Given 1.000 i/100ms Simple 1.000 i/100ms Calculating ------------------------------------- Given 0.804 (± 0.0%) i/s - 49.000 in 61.186831s Simple 0.855 (± 0.0%) i/s - 52.000 in 60.926097s Comparison: Simple: 0.9 i/s Given: 0.8 i/s - 1.06x slower --- CHANGELOG.md | 1 + UPGRADING.md | 22 ++++++++++++++++++++++ lib/grape/validations/params_scope.rb | 5 +++-- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75f599fb33..4aefdb27d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ #### Fixes * Your contribution here. +* [#2127](https://github.com/ruby-grape/grape/pull/2127): Fix a performance issue with dependent params - [@dnesteryuk](https://github.com/dnesteryuk). * [#2126](https://github.com/ruby-grape/grape/pull/2126): Fix warnings about redefined attribute accessors in `AttributeTranslator` - [@samsonjs](https://github.com/samsonjs). * [#2121](https://github.com/ruby-grape/grape/pull/2121): Fix 2.7 deprecation warning in validator_factory - [@Legogris](https://github.com/Legogris). * [#2115](https://github.com/ruby-grape/grape/pull/2115): Fix declared_params regression with multiple allowed types - [@stanhu](https://github.com/stanhu). diff --git a/UPGRADING.md b/UPGRADING.md index 9d63392b96..e5b6947fa2 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,6 +1,28 @@ Upgrading Grape =============== +### Upgrading to >= 1.5.1 + +#### Dependent params + +If you use [dependent params](https://github.com/ruby-grape/grape#dependent-parameters) with +`Grape::Extensions::Hash::ParamBuilder`, make sure a parameter to be dependent on is set as a Symbol. +If a String is given, a parameter that other parameters depend on won't be found even if it is present. + +_Correct_: +```ruby +given :matrix do + # dependent params +end +``` + +_Wrong_: +```ruby +given 'matrix' do + # dependent params +end +``` + ### Upgrading to >= 1.5.0 Prior to 1.3.3, the `declared` helper would always return the complete params structure if `include_missing=true` was set. In 1.3.3 a regression was introduced such that a missing Hash with or without nested parameters would always resolve to `{}`. diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index a51be6e1e1..792762b06a 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -61,8 +61,9 @@ def meets_dependency?(params, request_params) end return params.any? { |param| meets_dependency?(param, request_params) } if params.is_a?(Array) - return false unless params.respond_to?(:with_indifferent_access) - params = params.with_indifferent_access + + # params might be anything what looks like a hash, so it must implement a `key?` method + return false unless params.respond_to?(:key?) @dependent_on.each do |dependency| if dependency.is_a?(Hash)