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: validators checking aliased param instead of original one #1852

Merged
merged 3 commits into from Dec 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions .rubocop_todo.yml
@@ -1,6 +1,6 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2018-12-06 21:06:59 -0500 using RuboCop version 0.51.0.
# on 2018-12-20 21:38:24 +0000 using RuboCop version 0.51.0.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
Expand Down Expand Up @@ -38,13 +38,13 @@ Metrics/BlockLength:
# Offense count: 9
# Configuration parameters: CountComments.
Metrics/ClassLength:
Max: 288
Max: 295

# Offense count: 31
Metrics/CyclomaticComplexity:
Max: 14

# Offense count: 1227
# Offense count: 1234
# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
# URISchemes: http, https
Metrics/LineLength:
Expand All @@ -55,7 +55,7 @@ Metrics/LineLength:
Metrics/MethodLength:
Max: 33

# Offense count: 11
# Offense count: 12
# Configuration parameters: CountComments.
Metrics/ModuleLength:
Max: 220
Expand Down
4 changes: 2 additions & 2 deletions CHANGELOG.md
Expand Up @@ -5,11 +5,11 @@
* Your contribution here.
* [#1850](https://github.com/ruby-grape/grape/pull/1850): Adds `same_as` validator - [@glaucocustodio](https://github.com/glaucocustodio).
* [#1833](https://github.com/ruby-grape/grape/pull/1833): Allows to set the `ParamBuilder` globally - [@myxoh](https://github.com/myxoh).
* [#1844](https://github.com/ruby-grape/grape/pull/1844): Fix: enforce `:tempfile` to be a `Tempfile` object in `File` validator - [@Nyangawa](https://github.com/Nyangawa).

#### Fixes

* Your contribution here.
* [#1852](https://github.com/ruby-grape/grape/pull/1852): `allow_blank` called after `as` when the original param is not blank - [@glaucocustodio](https://github.com/glaucocustodio).
* [#1844](https://github.com/ruby-grape/grape/pull/1844): Enforce `:tempfile` to be a `Tempfile` object in `File` validator - [@Nyangawa](https://github.com/Nyangawa).

### 1.2.2 (2018/12/07)

Expand Down
10 changes: 9 additions & 1 deletion lib/grape/endpoint.rb
Expand Up @@ -258,6 +258,7 @@ def run
else
run_filters before_validations, :before_validation
run_validators validations, request
remove_aliased_params
run_filters after_validations, :after_validation
response_object = @block ? @block.call(self) : nil
end
Expand Down Expand Up @@ -319,7 +320,14 @@ def build_helpers
Module.new { helpers.each { |mod_to_include| include mod_to_include } }
end

private :build_stack, :build_helpers
def remove_aliased_params
return unless route_setting(:aliased_params)
glaucocustodio marked this conversation as resolved.
Show resolved Hide resolved
route_setting(:aliased_params).flat_map(&:keys).each do |aliased_param|
@params.delete(aliased_param)
end
end

private :build_stack, :build_helpers, :remove_aliased_params

def helpers
lazy_initialize! && @helpers
Expand Down
1 change: 0 additions & 1 deletion lib/grape/validations/validators/as.rb
Expand Up @@ -8,7 +8,6 @@ def initialize(attrs, options, required, scope, opts = {})

def validate_param!(attr_name, params)
params[@alias] = params[attr_name]
params.delete(attr_name)
end
end
end
Expand Down
10 changes: 10 additions & 0 deletions spec/grape/validations/params_scope_spec.rb
Expand Up @@ -155,6 +155,16 @@ def initialize(value)
expect(last_response.status).to eq(400)
expect(last_response.body).to eq('foo is empty')
end

it do
subject.params do
requires :foo, as: :bar, allow_blank: false
end
subject.get('/alias-not-blank-with-value') {}
get '/alias-not-blank-with-value', foo: 'any'

expect(last_response.status).to eq(200)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get status 400 here with the last_response.body = foo is empty.

As the alias (as) is called before the allow_blank, it deletes the foo key from params and sets params[:bar] with its value.

Given that alias is just for internal usage (inside get block I mean), I think this should not fail, cause the foo param was given as expected. Do you agree?

Copy link
Member

Choose a reason for hiding this comment

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

I'd tend to agree that this looks like a bug to me. Could you open an issue for this?

Copy link
Member

Choose a reason for hiding this comment

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

(Thanks for looking at this!)

end
end

context 'array without coerce type explicitly given' do
Expand Down