Skip to content
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

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cameronmoreau
Copy link

No description provided.

@johnnyshields
Copy link
Contributor

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

@cameronmoreau
Copy link
Author

@johnnyshields
Copy link
Contributor

johnnyshields commented Jul 22, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants