-
Notifications
You must be signed in to change notification settings - Fork 1.4k
MONGOID-5890 Handle BSON::Decimal128 specially when checking for changes #6042
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes dirty tracking issues when using BSON::Decimal128
with the map_big_decimal_to_decimal128
option enabled. When identical floating point values were assigned to fields, Mongoid incorrectly marked records as changed due to equality comparison failures between BSON::Decimal128
and numeric types.
- Introduces normalization logic to convert
BSON::Decimal128
toBigDecimal
for comparison purposes - Replaces direct equality comparison with a new
attribute_will_not_change?
method that handles type normalization - Adds test coverage for the scenario where identical numbers are assigned to fields with the decimal128 mapping enabled
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
lib/mongoid/attributes.rb | Adds normalization methods and updates attribute change detection logic |
spec/mongoid/attributes_spec.rb | Adds test case for the decimal128 dirty tracking fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
lib/mongoid/attributes.rb
Outdated
# comparison purposes. This is necessary because `BSON::Decimal128` does | ||
# not implement `#==` in a way that is compatible with `BigDecimal`. | ||
def normalize_value(value) | ||
value.is_a?(BSON::Decimal128) ? value.to_d : value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.to_d
should be considered canonical, and there may be future differences between .to_d
and BigDecimal(value.to_s)
.
Therefore, this should detect .respond_to?(:to_d)
and use it if available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canonical, yes, but whether we use #to_d
here or not, we still have to do the BigDecimal(value.to_s)
bit as well, because of the case where #to_d
is not available. In the interest of keeping the code simpler and easier to read, I'm opting to use the lowest common denominator, here.
Thanks for your input, Johnny. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamis I mean this very politely, but this line of thinking has consistently gotten Mongoid--and my company, by exstension--into trouble over the years.
"Easy to read" is not the highest virtue of code--nor is DRY, etc. The goal should be "intuitive/correct behavior for users"--in this case the user is the "Mongoid developer".
Sometimes it takes an extra line or two to achieve both backwards compatibility BigDecimal(value.to_s)
and forward behavioral correctness #to_d
. This is ok--a terse code comment #to_d was added in bson-ruby 5.0.0
is enough to help with readability.
By doing things your way, you are burying a technical debt landmine in the code that future users will walk across. Granted, not using #to_d
in-and-of-itself is not catastrophic, but since you are aware there is an opportunity to delegate to the driver--why not do it now? Your way is to wait until MONGOID-19328 is raised "Mongoid does not normalize BigDecimals properly, but driver does.", and someone will see the git blame
"Oh yeah, the developer back then, some guy named @jamis thought the code would be easier to read if he masked the driver behavior."
And the karmic cycle continues... 🛞
…ges (mongodb#6042) * handle BSON::Decimal128 specially when checking for changes * Reference the correct option in the context name Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apparently #to_d is a recent addition to ruby-bson --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
With the
map_big_decimal_to_decimal128
option enabled, Mongoid will translate betweenBSON::Decimal128
andBigDecimal
when records are loaded and saved. However, if you assign an identical floating point value to the field, it will mark the record as changed, even though it (functionally) did not. This is becauseBSON::Decimal128
is not an actual numeric type, and equality comparisons with numeric types will return false.This PR fixes the dirty tracking with regard to
BigDecimal
andBSON::Decimal128
by adding a normalization step to the comparison and handling theDecimal128
type specially.