-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Avoid coercion of a value if it is valid #1686
Avoid coercion of a value if it is valid #1686
Conversation
Let me take a look at failing specs - didn't encounter them locally... |
Ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions I am not sure about ...
@@ -137,7 +137,7 @@ def infer_type_check(type) | |||
# passed, or if the type also implements a parse() method. | |||
type | |||
elsif type.is_a?(Enumerable) | |||
->(value) { value.all? { |item| item.is_a? type[0] } } | |||
->(value) { value.respond_to?(:all?) && value.all? { |item| item.is_a? type[0] } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enumerable
by definition has all?
, how are we hitting a path where respond_to?(:all?)
is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I'm now checking for valid_type?
before value
is coerced, it means that value
now could be anything. The Proc
that's defined here is used in def success?
on line 96, which has the annotation of # This method is called from somewhere within +Virtus::Attribute::value_coerced?+ in order to assert that the value has been coerced successfully.
valid_type?
directly calls value_coerced?
, bringing this full circle. I think the confusion is around elsif type.is_a?(Enumerable)
, where type
refers to the target/desired post-coercion type (eg. Array
) rather than the type of value
.
Without it, specs that go from '1 2 3'
to [1, 2, 3]
would fail as the type check on '1 2 3'
would attempt to directly call '1 2 3'.all? ...
which can't be done on a String
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
|
||
def requires_coercion?(value) | ||
# JSON types do not require coercion if value is valid | ||
!valid_type?(value) || converter.coercer.respond_to?(:method) && !converter.is_a?(Grape::Validations::Types::Json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain respond_to?(:method)
here?
Then, these seen unrelated, so to make it readable I would break it up.
return true if valid_type?(value)
return true if ...
false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.method
is defined if a custom coerce_with
had been specified by the user, or the destination type
has a defined coercion that everything needs to go through despite validity. I added a test that shows why this is needed, if that's the behavior we want (which is the current behavior, as everything is coerced once before type checking).
The conditionals are all directly related because of the following statements:
If the value is not valid, we require coercion.
Otherwise (if the value is valid), we require coercion it if a `method` has been defined and it is not JSON.
which is written explicitly as:
def requires_coercion?(value)
if !valid_type?(value)
return true
else
if converter.coercer.respond_to?(:method) && !converter.is_a?(Grape::Validations::Types::Json)
return true
else
return false
end
end
end
which could go to:
def requires_coercion?(value)
return true if converter.coercer.respond_to?(:method) && !converter.is_a?(Grape::Validations::Types::Json)
return true if !valid_type?(value)
false
end
which condenses to the logic given. Is the one with explicit return true
s still what is preferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really care much, this is fine as is ;)
Merged, thank you. |
This PR aims to simplify the logic (and expensiveness) of coercion by not coercing the param if it is already a valid value unless a custom coercer is provided, which resolves issues we're run into around JSON until further work can be done to improve the experience. Relates to #1685