MONGOID-5212 Support for Decimal128 type

[Neilshweky]

No description provided.

[p-mongo]

I would like the numeric? condition to be dropped and this path be taken for non-numeric input also. I see that non-numeric input was already trashed previously but I think this is not good behavior.

Test with a time field:

  class Foo
    include Mongoid::Document

    field :a
  end

  Foo.delete_all

  Foo.create!(a: 'zz')

  class Foo
    field :a, type: BigDecimal
  end

  p Foo.first, Foo.first.a

produces:

NoMethodError: undefined method `getlocal' for "zz":String

Test with the code as proposed with BigDecimal field:

  class Foo
    include Mongoid::Document

    field :a
  end

  Foo.delete_all

  Foo.create!(a: 'zz')

  class Foo
    field :a, type: Time
  end

  p Foo.first, Foo.first.a

produces:

#<Foo _id: 61d76a95a15d5d4e9c038186, a: "zz">
nil

I think it would be better to always attempt to construct a BigDecimal, allowing that call to raise whatever exceptions on problems.

@comandeo thoughts?

[johnnyshields]

I disagree. I think demongoize should return nil unless the object can be represented as a BigDecimal. Should check the behavior of demongoize on other classes.

[Neilshweky]

@p-mongo I kind of agree with @johnnyshields here... Here are some examples of invalid values to demongoize:

irb(main):004:0> Float.demongoize("1.2asda")
=> 0.0
irb(main):005:0> Integer.demongoize("1234rqwdd")
=> 0

Now, I'm not sure why 0 is the right return value here but the important thing is that it doesn't raise an error. Let me know what you think...

[johnnyshields]

@Neilshweky is correct should make this return 0. Important thing is that it doesn't raise error.

[Neilshweky]

Actually after rereading your comment, I agree @p-mongo. Let's let BigDecimal throw an error

[johnnyshields]

Since the two existing numeric types (Float and Integer) currently return 0, I would be inclined to say BigDecimal should return 0. Fine to raise a follow-up ticket/PR if we wish to change it later.

[Neilshweky]

I think for now it's better practice to return nil, and we should file a ticket to follow upon Float and Integer.

[p-mongo]

I created https://jira.mongodb.org/browse/MONGOID-5221 to address the demongoization issue across all types.

[p-mongo]

I am unresolving this discussion thread because it's referenced from https://jira.mongodb.org/browse/MONGOID-5221.

[johnnyshields]

I've crossed out my earlier comment here--BigDecimal correctly returns nil today for non-numeric string, we should do the same for Integer/Float. It's ready to merge in PR: #5220

[Neilshweky]

The tests in this PR will not work until BSON is upgraded to 4.14.0

[Neilshweky]

This test does not work. It asserts that a BigDecimal that was originally stored as a string, can be queried with a BigDecimal, with map_big_decimal_to_decimal128 turned on. We have to fix this. Here are the solutions I can think of:

1. Change the implementation of searching for a BigDecimal field to search for both the string representation and the BigDecimal representation.
   A problem arises when some people purposely have BigDecimals and Strings stored in the database under the same field, and don't want querying for a BigDecimal to return the string. But I think this is a pretty niche case.
2. We can just leave it how it is. Instruct the user how to query for both string and BigDecimal representations of their data if that's what they want. This is a little annoying for the user.
3. I also wrote up this quick little query that changes all values in a db to a BigDecimal but this seems a little reckless, especially for big data sets:

db.bands.updateMany({ 
  "field": { "$exists": true } 
}, [
  { "$set": { 
    "field": { "$toDecimal": "$field" } 
  } }
])

[p-mongo]

The first idea would be a really good one but I expect it to be either difficult or perhaps not even possible.

There are two contexts where attribute values are used: implicit equality (e.g. {field: 42}) and operators (e.g. {field: {$eq: 42}}). While the implicit equality can be transformed to e.g. {field: {$in: ['42', Decimal128('42')]}}, this won't work in the operator context.

mongoize currently doesn't have context knowledge at all. There is probably a way to communicate some contexts to it but Mongoid does generally permit users to provide MQL in a bunch of places and I'm thinking those might be impossible to change programmatically short of doing much more rewriting of MQL than Mongoid is presently doing.

[Neilshweky]

So does that leave us with option 2?

[p-mongo]

I think at this point what we need is migration guidance and documentation for how to move from string to bigdecimal field storage. The migration query above is excellent and can go into the migration guidance. Additionally it would be good to have a similar snippet for how to query on individual fields with type conversion into bigdecimal, e.g. if I want to find all documents where the value is > 0 with both types how I would do that.

[Neilshweky]

Okay! I can't put that all into the documentation PR.... That leaves us with no further code changes then.

[p-mongo]

This test then can be changed to assert that the document is not found when its type in the database is different from how Mongoid is presently configured, for our future reference.

[p-mongo]

LGTM, I'll leave this unapproved as a reminder that this PR is blocked on bson release.

[p-mongo]

This test then can be changed to assert that the document is not found when its type in the database is different from how Mongoid is presently configured, for our future reference.

[johnnyshields]

Looks good to me