Corrected the endpoint parameter name generation. (#2189)

* Corrected the endpoint parameter name generation. Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com> * Reworked the parameter renaming to be a declared-only feature. Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com> * Corrected already existing, but now broken specs. Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com> * PR fixes and docs. Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com> * Version bump and rephrasing. Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com>
ruby-grape · Sep 15, 2021 · 5b4a2dd · 5b4a2dd
1 parent 0ff93fb
commit 5b4a2dd
Show file tree

Hide file tree

Showing 10 changed files with 369 additions and 54 deletions.
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
@@ -167,7 +167,7 @@ Metrics/BlockLength:
 # Offense count: 11
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
-  Max: 304
+  Max: 310
 
 # Offense count: 30
 # Configuration parameters: IgnoredMethods.

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,4 +1,4 @@
-### 1.5.4 (Next)
+### 1.6.0 (Next)
 
 #### Features
 
@@ -9,7 +9,9 @@
 * [#2176](https://github.com/ruby-grape/grape/pull/2176): Fix: OPTIONS fails if matching all routes - [@myxoh](https://github.com/myxoh).
 * [#2177](https://github.com/ruby-grape/grape/pull/2177): Fix: `default` validator fails if preceded by `as` validator - [@Catsuko](https://github.com/Catsuko).
 * [#2180](https://github.com/ruby-grape/grape/pull/2180): Call `super` in `API.inherited` - [@yogeshjain999](https://github.com/yogeshjain999).
+* [#2189](https://github.com/ruby-grape/grape/pull/2189): Fix: rename parameters when using `:as` (behaviour and grape-swagger documentation) - [@Jack12816](https://github.com/Jack12816).
 * Your contribution here.
+
 ### 1.5.3 (2021/03/07)
 
 #### Fixes

diff --git a/README.md b/README.md
@@ -801,6 +801,7 @@ Grape allows you to access only the parameters that have been declared by your `
 
   * Filter out the params that have been passed, but are not allowed.
   * Include any optional params that are declared but not passed.
+  * Perform any parameter renaming on the resulting hash.
 
 Consider the following API endpoint:
 
@@ -995,8 +996,10 @@ curl -X POST -H "Content-Type: application/json" localhost:9292/users/signup -d
 ````json
 {
   "declared_params": {
-    "first_name": "first name",
-    "last_name": null
+    "user": {
+      "first_name": "first name",
+      "last_name": null
+    }
   }
 }
 ````

diff --git a/UPGRADING.md b/UPGRADING.md
@@ -1,10 +1,50 @@
 Upgrading Grape
 ===============
 
+### Upgrading to >= 1.6.0
+
+#### Parameter renaming with :as
+
+Prior to 1.6.0 the [parameter renaming](https://github.com/ruby-grape/grape#renaming) with `:as` was directly touching the request payload ([`#params`](https://github.com/ruby-grape/grape#parameters)) while duplicating the old and the new key to be both available in the hash. This allowed clients to bypass any validation in case they knew the internal name of the parameter.  Unfortunately, in combination with [grape-swagger](https://github.com/ruby-grape/grape-swagger) the internal name (name set with `:as`) of the parameters were documented.
+
+This behavior was fixed. Parameter renaming is now done when using the [`#declared(params)`](https://github.com/ruby-grape/grape#declared) parameters helper. This stops confusing validation/coercion behavior.
+
+Here comes an illustration of the old and new behaviour as code:
+
+```ruby
+# (1) Rename a to b, while client sends +a+
+optional :a, type: Integer, as: :b
+params = { a: 1 }
+declared(params, include_missing: false)
+# expected => { b: 1 }
+# actual   => { b: 1 }
+
+# (2) Rename a to b, while client sends +b+
+optional :a, type: Integer, as: :b, values: [1, 2, 3]
+params = { b: '5' }
+declared(params, include_missing: false)
+# expected => { }        (>= 1.6.0)
+# actual   => { b: '5' } (uncasted, unvalidated, <= 1.5.3)
+```
+
+Another implication of this change is the dependent parameter resolution. Prior to 1.6.0 the following code produced an `Grape::Exceptions::UnknownParameter` because `:a` was replace by `:b`:
+
+```ruby
+params do
+  optional :a, as: :b
+  given :a do # (<= 1.5.3 you had to reference +:b+ here to make it work)
+    requires :c
+  end
+end
+```
+
+This code now works without any errors, as the renaming is just an internal behaviour of the `#declared(params)` parameter helper.
+
+See [#2189](https://github.com/ruby-grape/grape/pull/2189) for more information.
 
 ### Upgrading to >= 1.5.3
 
-### Nil value and coercion
+#### Nil value and coercion
 
 Prior to 1.2.5 version passing a `nil` value for a parameter with a custom coercer would invoke the coercer, and not passing a parameter would not invoke it.
 This behavior was not tested or documented. Version 1.3.0 quietly changed this behavior, in such that `nil` values skipped the coercion. Version 1.5.3 fixes and documents this as follows:

diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb
@@ -48,15 +48,20 @@ def declared_array(passed_params, options, declared_params, params_nested_path)
         end
 
         def declared_hash(passed_params, options, declared_params, params_nested_path)
+          renamed_params = route_setting(:renamed_params) || {}
+
           declared_params.each_with_object(passed_params.class.new) do |declared_param, memo|
             if declared_param.is_a?(Hash)
               declared_param.each_pair do |declared_parent_param, declared_children_params|
                 params_nested_path_dup = params_nested_path.dup
                 params_nested_path_dup << declared_parent_param.to_s
                 next unless options[:include_missing] || passed_params.key?(declared_parent_param)
 
+                rename_path = params_nested_path + [declared_parent_param.to_s]
+                renamed_param_name = renamed_params[rename_path]
+
+                memo_key = optioned_param_key(renamed_param_name || declared_parent_param, options)
                 passed_children_params = passed_params[declared_parent_param] || passed_params.class.new
-                memo_key = optioned_param_key(declared_parent_param, options)
 
                 memo[memo_key] = handle_passed_param(params_nested_path_dup, passed_children_params.any?) do
                   declared(passed_children_params, options, declared_children_params, params_nested_path_dup)
@@ -65,13 +70,13 @@ def declared_hash(passed_params, options, declared_params, params_nested_path)
             else
               # If it is not a Hash then it does not have children.
               # Find its value or set it to nil.
-              has_renaming = route_setting(:renamed_params) && route_setting(:renamed_params).find { |current| current[declared_param] }
-              param_renaming = has_renaming[declared_param] if has_renaming
+              next unless options[:include_missing] || passed_params.key?(declared_param)
 
-              next unless options[:include_missing] || passed_params.key?(declared_param) || (param_renaming && passed_params.key?(param_renaming))
+              rename_path = params_nested_path + [declared_param.to_s]
+              renamed_param_name = renamed_params[rename_path]
 
-              memo_key = optioned_param_key(param_renaming || declared_param, options)
-              passed_param = passed_params[param_renaming || declared_param]
+              memo_key = optioned_param_key(renamed_param_name || declared_param, options)
+              passed_param = passed_params[declared_param]
 
               params_nested_path_dup = params_nested_path.dup
               params_nested_path_dup << declared_param.to_s

diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb
@@ -262,7 +262,6 @@ def run
           else
             run_filters before_validations, :before_validation
             run_validators validations, request
-            remove_renamed_params
             run_filters after_validations, :after_validation
             response_object = execute
           end
@@ -328,14 +327,7 @@ def build_helpers
       Module.new { helpers.each { |mod_to_include| include mod_to_include } }
     end
 
-    def remove_renamed_params
-      return unless route_setting(:renamed_params)
-      route_setting(:renamed_params).flat_map(&:keys).each do |renamed_param|
-        @params.delete(renamed_param)
-      end
-    end
-
-    private :build_stack, :build_helpers, :remove_renamed_params
+    private :build_stack, :build_helpers
 
     def execute
       @block ? @block.call(self) : nil

diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb
@@ -13,6 +13,8 @@ class ParamsScope
       # @param opts [Hash] options for this scope
       # @option opts :element [Symbol] the element that contains this scope; for
       #   this to be relevant, @parent must be set
+      # @option opts :element_renamed [Symbol, nil] whenever this scope should
+      #   be renamed and to what, given +nil+ no renaming is done
       # @option opts :parent [ParamsScope] the scope containing this scope
       # @option opts :api [API] the API endpoint to modify
       # @option opts :optional [Boolean] whether or not this scope needs to have
@@ -23,13 +25,14 @@ class ParamsScope
       #   validate if this param is present in the parent scope
       # @yield the instance context, open for parameter definitions
       def initialize(opts, &block)
-        @element      = opts[:element]
-        @parent       = opts[:parent]
-        @api          = opts[:api]
-        @optional     = opts[:optional] || false
-        @type         = opts[:type]
-        @group        = opts[:group] || {}
-        @dependent_on = opts[:dependent_on]
+        @element          = opts[:element]
+        @element_renamed  = opts[:element_renamed]
+        @parent           = opts[:parent]
+        @api              = opts[:api]
+        @optional         = opts[:optional] || false
+        @type             = opts[:type]
+        @group            = opts[:group] || {}
+        @dependent_on     = opts[:dependent_on]
         @declared_params = []
         @index = nil
 
@@ -129,18 +132,35 @@ def push_declared_params(attrs, **opts)
         if lateral?
           @parent.push_declared_params(attrs, **opts)
         else
-          if opts && opts[:as]
-            @api.route_setting(:renamed_params, @api.route_setting(:renamed_params) || [])
-            @api.route_setting(:renamed_params) << { attrs.first => opts[:as] }
-            attrs = [opts[:as]]
-          end
+          push_renamed_param(full_path + [attrs.first], opts[:as]) \
+            if opts && opts[:as]
 
           @declared_params.concat attrs
         end
       end
 
+      # Get the full path of the parameter scope in the hierarchy.
+      #
+      # @return [Array<Symbol>] the nesting/path of the current parameter scope
+      def full_path
+        nested? ? @parent.full_path + [@element] : []
+      end
+
       private
 
+      # Add a new parameter which should be renamed when using the +#declared+
+      # method.
+      #
+      # @param path [Array<String, Symbol>] the full path of the parameter
+      #   (including the parameter name as last array element)
+      # @param new_name [String, Symbol] the new name of the parameter (the
+      #   renamed name, with the +as: ...+ semantic)
+      def push_renamed_param(path, new_name)
+        base = @api.route_setting(:renamed_params) || {}
+        base[Array(path).map(&:to_s)] = new_name.to_s
+        @api.route_setting(:renamed_params, base)
+      end
+
       def require_required_and_optional_fields(context, opts)
         if context == :all
           optional_fields = Array(opts[:except])
@@ -191,11 +211,12 @@ def new_scope(attrs, optional = false, &block)
         end
 
         self.class.new(
-          api:      @api,
-          element:  attrs[1][:as] || attrs.first,
-          parent:   self,
-          optional: optional,
-          type:     type || Array,
+          api:             @api,
+          element:         attrs.first,
+          element_renamed: attrs[1][:as],
+          parent:          self,
+          optional:        optional,
+          type:            type || Array,
           &block
         )
       end
@@ -235,6 +256,8 @@ def new_group_scope(attrs, &block)
 
       # Pushes declared params to parent or settings
       def configure_declared_params
+        push_renamed_param(full_path, @element_renamed) if @element_renamed
+
         if nested?
           @parent.push_declared_params [element => @declared_params]
         else
@@ -303,10 +326,6 @@ def validates(attrs, validations)
           next if order_specific_validations.include?(type)
           validate(type, options, attrs, doc_attrs, opts)
         end
-
-        # Apply as validator last so other validations are applied to
-        # renamed param
-        validate(:as, validations[:as], attrs, doc_attrs, opts) if validations.key?(:as)
       end
 
       # Validate and comprehend the +:type+, +:types+, and +:coerce_with+

diff --git a/lib/grape/validations/validators/as.rb b/lib/grape/validations/validators/as.rb
@@ -3,14 +3,10 @@
 module Grape
   module Validations
     class AsValidator < Base
-      def initialize(attrs, options, required, scope, **opts)
-        @renamed_options = options
-        super
-      end
-
-      def validate_param!(attr_name, params)
-        params[@renamed_options] = params[attr_name]
-      end
+      # We use a validator for renaming parameters. This is just a marker for
+      # the parameter scope to handle the renaming. No actual validation
+      # happens here.
+      def validate_param!(*); end
     end
   end
 end