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

[NEEDS FEEDBACK] MONGOID-5382 RawValue for mongoize/demongoize #5368

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Jul 10, 2022

This PR is a work-in-progress, code doesn't match spec below yet

  • TODO: define === and ensure Mongoid::Matcher::Type works

MONGOID-5408 introduced a new class Mongoid::RawValue which is used to represent uncastable values. MONGOID-5408 added support for the #evolve (cast-to-query) method, however, the concept can be extended to #mongoize and #demongoize.

The following is intended:

  1. Introduce a new deprecation flag "use_raw_value_for_type_casting" (name TBD). The legacy behavior of this flag is "false" and the new behavior will be "true".
  2. mongoize and demongoize will now return Mongoid::RawValue in case the object cannot be casted.
  3. Mongoid::RawValue will delegate all methods to its underlying raw_value object. This is done primarily for demongoize, so that you can easily work with RawValues.
  4. Type assignment will raise an InvalidValue (or InvalidAssignment, name TBD) error if mongoize returns a Mongoid::RawValue. To get around this error You will still be able to assign a RawValue directly.

To illustrate all this:

class Person
  field :age, type: Integer
end

bob = Person.new
bob.age = 42     # normal case
bob.age = "Foo"  # raises Mongoid::Errors::InvalidValue because Integer.mongoize("Foo")
                 # returns a Mongoid::RawValue
bob.age = Mongoid::RawValue("Foo")  # This is allowed, and it will force persisting "Foo" to this field.

bob.reload
bob.age  # returns Mongoid::RawValue("Foo"), since #demongoize can't cast
         # persisted value "Foo" to Integer

bob.age.gsub('F', 'B')  # allowed, since RawValue delegates to its underlying member

For reference, the current Mongoid 8.0 behavior is that both #mongoize and #demongoize coerce uncastable values to nil, which is dangerous. Previously, in Mongoid 7.x and earlier, some cases of assignment resulted in errors being raised, however the logic was inconsistent and unintentional (e.g. errors happened to be raised because of bugs, which had the side effect of preventing some cases uncastable assignment.)

@johnnyshields johnnyshields marked this pull request as draft July 10, 2022 16:21
@johnnyshields johnnyshields changed the title Proof-of-concept: Mongoid::RawValue Proof-of-concept: MONGOID-5382 strict mode +MONGOID-5408 evolve Mongoid::RawValue wrapper Jul 10, 2022
@Neilshweky
Copy link
Contributor

I haven't thought about this PR yet but just from an initial look.... Wouldn't it be better to have Mongoid::RawValue have its own evolve method which just returns the inner value? Instead of hard coding checks everywhere?

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Jul 10, 2022

Wouldn't it be better to have Mongoid::RawValue have its own evolve method which just returns the inner value?

Yeah that thought crossed my mind too. Hmm... 🤔

@p-mongo
Copy link
Contributor

p-mongo commented Jul 25, 2022

@johnnyshields Can you please either rebase this PR on master or close it if no longer relevant.

I also get the impression that this is a "solution in search of a problem", so to speak, namely that you are trying to get your idea of mongoize returning variant values into Mongoid. This is not what either of the tickets you referenced is asking for (https://jira.mongodb.org/browse/MONGOID-5382, https://jira.mongodb.org/browse/MONGOID-5408). Please ensure that you are starting from the requirements and are working on a solution toward the requirements.

@johnnyshields johnnyshields changed the title Proof-of-concept: MONGOID-5382 strict mode +MONGOID-5408 evolve Mongoid::RawValue wrapper Proof-of-concept: MONGOID-5382 strict mode Feb 15, 2023
@johnnyshields johnnyshields changed the title Proof-of-concept: MONGOID-5382 strict mode Proof-of-concept: MONGOID-5382 RawValue for mongoize/demongoize Feb 15, 2023
@johnnyshields
Copy link
Contributor Author

@jamis can you please give me some feedback on the spec written above and the draft code in this PR? If I invest my time to complete this, will the MongoDB team merge it?

@johnnyshields johnnyshields changed the title Proof-of-concept: MONGOID-5382 RawValue for mongoize/demongoize [NEEDS FEEDBACK] MONGOID-5382 RawValue for mongoize/demongoize Mar 5, 2023
@johnnyshields johnnyshields marked this pull request as ready for review April 14, 2024 21:41
@johnnyshields johnnyshields marked this pull request as draft April 14, 2024 21:42
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.

3 participants