fix: validators checking aliased param instead of original one#1852
Conversation
| subject.get('/alias-not-blank-with-value') {} | ||
| get '/alias-not-blank-with-value', foo: 'any' | ||
|
|
||
| expect(last_response.status).to eq(200) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'd tend to agree that this looks like a bug to me. Could you open an issue for this?
|
Looks like a bug. |
glaucocustodio
changed the title
| # type casted values | ||
| coerce_type validations, attrs, doc_attrs, opts | ||
|
|
||
| move_alias_to_end(validations) |
There was a problem hiding this comment.
This isn't reused, and we do a ton of things above, so I would just inline the two lines of the method here.
I think delete also always makes a copy, and we should assume a nil value is a thing, so to be consistent I what we do above:
# explain why
validations[:as] = validations.delete(:as) if validations.key?(:as)There was a problem hiding this comment.
Errrr.... re-reading the code we move that :as to the "end". I understand why it works, but feels to me that it's wrong to rely on the order in validations which is not a visibly ordered array or hash.
There was a problem hiding this comment.
I agree that relying on the order is not good, but that was the easier way I found to fix the bug so far.
Another approach would be set a variable to let others validators know about alias, but it seems to be a bigger change and I am not sure whether the outcome would be worth..
Please let me know whether you have more ideas.
There was a problem hiding this comment.
I just don't think we can use on this approach to work reliably. There're no ordering guarantees. I think extracting as somehow is the proper way of doing it. Give it a try! We're here to help.
There was a problem hiding this comment.
Check the new approach now please. Build is failing because of rubocop lib/grape/endpoint.rb:6:3: C: Class has too many lines. [295/288].
There was a problem hiding this comment.
Run rubocop -a ; rubocop --auto-gen-config for this.
dblock
left a comment
dblock
left a comment
There was a problem hiding this comment.
So why does this work? We remove aliased params in the validators, shouldn't that accomplish the same thing?
|
Merged, thank you. |
3 participants
Hi folks.
I've pointed a scenario and I would like to hear from you whether it is a bug or not.
Please, see the comments next the code.