-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
Disable type conversion for Set and Array condition values #224
Conversation
@@ -258,6 +258,8 @@ def range_query | |||
end | |||
|
|||
def type_cast_condition_parameter(key, value) | |||
return value if [:array, :set].include?(source.attributes[key.to_sym][:type]) |
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.
Shouldn't this be the else
case below? So really the else
case is wrong because the dump
field won't work correctly since the type is array even though it's acting on individual values of the array.
Just quick glance, almost seems like this really should just always be, right?
source.dump_field(value, source.attributes[key.to_sym]
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.
@richardhsu I think it shouldn't
The value may be collection (Set or Array) in following cases:
in
operator -where("field.in" => [1, 2, 3])
between
operator -where("field.between" => [0, 10])
equal
operator forset
/array
-where(set_field: Set.new([1, 2, 3]))
We have to handle cases 1, 2 and 3 in different way. For cases 1, 2 we have to convert each element of collection. For case 3 we have convert the whole value (to set
or array
)
else
case handles cases 1, 2. And here I disable/postpone handling case 3 in order to simplify code. It's a complex logic because we have to take into account not only field type but operator. For instance for set
field operator equal
expects set
but contains
expects type of set
element - number
or 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.
Thanks for the explanation!
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.
Ah perfect, @andrykonchin thanks for explanation!
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.
Great specs
Fix
contains
operator forset
fieldsExample:
Document.where("set.contains" => "foo").all
Related issue #220