mongoid icon indicating copy to clipboard operation
mongoid copied to clipboard

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

Open johnnyshields opened this issue 3 years ago • 4 comments

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 avatar Jul 10 '22 16:07 johnnyshields

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?

neilshweky avatar Jul 10 '22 16:07 neilshweky

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

johnnyshields avatar Jul 10 '22 17:07 johnnyshields

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

p-mongo avatar Jul 25 '22 22:07 p-mongo

@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 avatar Feb 26 '23 14:02 johnnyshields