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.

Sign up for GitHub

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

Jump to bottom

MONGOID-5408 .evolve should support wrapper class #5448

Merged
merged 6 commits into from
Feb 14, 2023
Merged

MONGOID-5408 .evolve should support wrapper class #5448

merged 6 commits into from
Feb 14, 2023

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Aug 18, 2022 •
edited
Loading

This is now possible:

# queries for the Integer 42, even though :name is type String.
User.where(name: Mongoid::RawValue(42))

# queries for the String "42", even though :age is type Integer.
User.where(age: Mongoid::RawValue(42))

@johnnyshields
Copy link
Contributor Author

@p-mongo this is ready for initial review.

@johnnyshields johnnyshields mentioned this pull request Aug 18, 2022
Draft
1 task
@p-mongo
Copy link
Contributor

p-mongo commented Aug 24, 2022

Hi @johnnyshields , thank you for the PR.

Can you explain why you are evolving raw values? The ticket says to skip evolution when querying, this is opposite behavior.

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Aug 25, 2022 •
edited
Loading

@p-mongo we still need to evolve Ruby classes into Mongo driver-compatible ones, e.g. BigDecimal into BSON::Decimal128. Otherwise, the driver won't be able to handle it AFAIK. The evolution in this case ignores any field type definition, and instead evolves purely using the value's class.

@p-mongo
Copy link
Contributor

p-mongo commented Aug 25, 2022

The ticket is to not perform evolution. If you have a use case where evolution is required, please create a new ticket describing it.

@johnnyshields
Copy link
Contributor Author

Raised: MONGOID-5468

@johnnyshields johnnyshields changed the title MONGOID-5408 .evolve should support wrapper class MONGOID-5408 / MONGOID-5468: .evolve should support wrapper class Aug 26, 2022
@johnnyshields johnnyshields mentioned this pull request Aug 30, 2022
Merged
@johnnyshields
Copy link
Contributor Author

@p-mongo on this PR I'll remove the evolution logic for RawValues. Can be done as a separate ticket follow-up if needed pending discussion in MONGOID-5468

@johnnyshields johnnyshields changed the title MONGOID-5408 / MONGOID-5468: .evolve should support wrapper class MONGOID-5408: .evolve should support wrapper class Aug 31, 2022
@johnnyshields johnnyshields force-pushed the evolve-raw-value-wrapper branch from 98eebc5 to dd07d91 Compare August 31, 2022 11:15
@johnnyshields
Copy link
Contributor Author

johnnyshields commented Aug 31, 2022 •
edited
Loading

@p-mongo this is ready for another review. I've removed the raw value evolve logic which commented about.

I have a few extra specs which I'll add once you approve the code structure here.

p-mongo
p-mongo reviewed Sep 1, 2022
View reviewed changes
lib/mongoid/extensions/raw_value.rb Outdated Show resolved Hide resolved
spec/integration/criteria/raw_value_spec.rb Outdated Show resolved Hide resolved
spec/mongoid/criteria/queryable/selector_spec.rb Show resolved Hide resolved
@johnnyshields johnnyshields force-pushed the evolve-raw-value-wrapper branch from a9277b7 to 19f97ec Compare September 18, 2022 13:59
@johnnyshields johnnyshields force-pushed the evolve-raw-value-wrapper branch from 2cfa478 to 483e8ad Compare September 29, 2022 15:36
johnnyshields
johnnyshields commented Oct 6, 2022
View reviewed changes
docs/reference/queries.txt
@@ -245,6 +232,33 @@ Since `id` and `_id` fields are aliases, either one can be used for queries:
# embedded: false>


Embedded Documents
Copy link
Contributor Author

@johnnyshields johnnyshields Oct 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I copy-pasted the "Embedded Documents" section to be after "Fields" w/out edits to it, as I felt it made more sense order-wise vis-a-vis additions to Fields section in this PR.

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Feb 2, 2023 •
edited
Loading

@alexbevi please unmark this PR as "Blocked". It is ready for final review (I have addressed all earlier comments from MongoDB team.)

@alexbevi alexbevi removed the blocked label Feb 3, 2023
jamis
jamis requested changes Feb 3, 2023
View reviewed changes
Copy link
Contributor

@jamis jamis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, @johnnyshields! Just a couple of minor comments on things I wanted to clarify, but once those are answered I'm happy to approve this.

lib/mongoid/criteria/queryable/selector.rb Outdated
@@ -150,6 +150,8 @@ def evolve_multi(specs)
#
# @return [ Object ] The serialized object.
def evolve(serializer, value)
return value.raw_value if value.is_a?(Mongoid::RawValue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @johnnyshields -- I hate to keep drawing this PR out, but I wanted to ask if there was a specific reason for using a guard here, rather than adding when Mongoid::RawValue as a clause in the case statement below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it. Good catch.

lib/mongoid/extensions/raw_value.rb Show resolved Hide resolved
@johnnyshields
Copy link
Contributor Author

@jamis thanks for your review! Comments addressed.

@jamis jamis requested a review from comandeo February 10, 2023 15:32
@johnnyshields johnnyshields changed the title MONGOID-5408: .evolve should support wrapper class [READY FOR REVIEW] MONGOID-5408: .evolve should support wrapper class Feb 10, 2023
@comandeo comandeo changed the title [READY FOR REVIEW] MONGOID-5408: .evolve should support wrapper class MONGOID-5408 .evolve should support wrapper class Feb 10, 2023
@comandeo comandeo merged commit 67c211d into mongodb:master Feb 14, 2023
@johnnyshields johnnyshields deleted the evolve-raw-value-wrapper branch February 14, 2023 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p-mongo p-mongo p-mongo left review comments

@comandeo comandeo comandeo approved these changes

@jamis jamis jamis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@johnnyshields @p-mongo @jamis @comandeo @alexbevi