mongoid icon indicating copy to clipboard operation
mongoid copied to clipboard

Don't allow ruby to coerce field types when evaluating _matches?

Open cameronmoreau opened this issue 1 year ago • 3 comments

cameronmoreau avatar May 30 '23 21:05 cameronmoreau

What's the reason for this change? There are 20+ other types that would need to be considered here probably like Symbol, etc.

johnnyshields avatar Jun 19 '23 09:06 johnnyshields

@johnnyshields here's some context: https://jira.mongodb.org/browse/MONGOID-5619

cameronmoreau avatar Jun 20 '23 20:06 cameronmoreau

@cameronmoreau it seems the main case you are taking issue with is implicit Time to String comparison:

Time.current > '2012-01-01' => true
Time.current < '2012-01-01' => false

# Note Date class doesn't do this
Date.today > '2012-01-01'
in `>': comparison of Date with String failed (ArgumentError)

(To be very precise, this behavior is due to a monkey patch from Rails' ActiveSupport library. For reference, just using Time class without AS will result in:)

Time.now < '2012-01-01'
in `<' ArgumentError (comparison of Time with String failed)

None of the other cases (e.g. String vs. Numeric) etc. behave differently than the database because neither Ruby nor AS coerces them, and they will result in an ArgumentError which is rescued and returned as false.

"1" < 2
in `<' ArgumentError (comparison of String with 2 failed)

Moreover, Mongoid actually does coerce queries to field types, e.g. User.gt(created_at: "2023-01-01") in cases where the field type is known. The only cases where the behavior diverges is if you use undefined field types such as Object, Hash, Array field type and do a query on the members--these should be considered "tread with care" edge cases.

It is my opinion the current behavior is actually desirable here--it is a reasonable convenience that a time string is coerced to Time. In several other tickets such as MONGOID-4500 we have establish that the "in-memory" behavior should diverge from the database where it provides additional convenience to the user.

This patch seems likely to break many existing apps. I don't think it's justified to fix an esoteric edge case.

TLDR; my recommendation is that this PR be closed.

johnnyshields avatar Jul 22 '23 15:07 johnnyshields