Fix ordering issues between as: and default: validations #2177
Conversation
Catsuko
changed the title
CHANGELOG.md
Outdated
| @@ -7,6 +7,7 @@ | |||
| #### Fixes | |||
|
|
|||
| * [#2176](https://github.com/ruby-grape/grape/pull/2176): Fixes issue-2175 - options call failing if matching all routes - [@myxoh](https://github.com/myxoh). | |||
| * [#2177](https://github.com/ruby-grape/grape/pull/2177): Fixes issue-2094 - Issue with the order of `as:` and `default:` options - [@Catsuko](https://github.com/Catsuko). | |||
There was a problem hiding this comment.
Not sure how we inherited this pattern, but we don't usually have one :), change this to Fix: actual specific problem between as: and default: validators, and for the line above Fix: OPTIONS fails if matching all routes. Thanks.
There was a problem hiding this comment.
sounds good, I reworded it so hopefully it describes it better
|
|
||
| # Before we run the rest of the validators, let's handle | ||
| # whatever coercion so that we are working with correctly | ||
| # type casted values | ||
| coerce_type validations, attrs, doc_attrs, opts | ||
|
|
||
| as = validations.delete(:as) |
There was a problem hiding this comment.
The validations arg that is being passed into the function should not be modified to avoid side effects, make a copy.
There was a problem hiding this comment.
I refactored my changes to avoid mutating the arg however it is still being done within the method outside of my changes:
desc = validations.delete(:desc) || validations.delete(:description)
# ...
doc_attrs[:documentation] = validations.delete(:documentation) if validations.key?(:documentation)I'd like to try refactoring however the scope seems a little scary so perhaps as part of its own issue.
There was a problem hiding this comment.
Agreed. And thank you, do give it a shot.
| def validates_presence(validations, attrs, doc_attrs, opts) | ||
| return unless validations.key?(:presence) && validations[:presence] | ||
| validate(:presence, validations[:presence], attrs, doc_attrs, opts) | ||
| validations.delete(:presence) |
There was a problem hiding this comment.
Same as above, don't alter validates, make a copy. The caller may be removing things if needed.
… and default validations
…e other validation changes are applied to renamed param
… complexity of validates method
|
Thanks for fixing this, merged. It could be an overkill, but maybe we should introduce some kind of Rack-style listing/mounting of validators so that we don't have to do special ordering gymnastics at runtime. Just food for thought. |
|
I think it'd make the behaviour more consistent too, perhaps there are other validators that don't play nicely with having |
Fixes #2094 where it was found that defining
as:beforedefault:would fail to add the default whereasas:afterdefault:would work fine.