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

Mongoize is not called on update_all, when $set operator is used #5814

Merged
merged 5 commits into from
May 10, 2024

Conversation

dem
Copy link
Contributor

@dem dem commented May 10, 2024

During our upgrade of mongoid to 8.x version hit into issue that that custom type field classes stops working in update_all, when
$set operator is used:

class LatLng
  attr_accessor :lat, :lng

  def self.demongoize(object)
    LatLng.new(object[1], object[0])
  end

  def initialize(lat, lng)
    @lat, @lng = lat, lng
  end

  def mongoize
    [ lng, lat ]
  end
end

class Bar
  include Mongoid::Document
  field :lat_lng, type: LatLng
end

Bar.create!

Bar.update_all('$set' => { lat_lng: LatLng.new(52.30, 13.25) })

BSON::Error::UnserializableClass: Value does not define its BSON serialized type: #<LatLng:0x000057678d11ddb0>

This problem is not highlighted by the tests due to mongoize does not work, but BSON still able serialize strings and send it to database:

context.update_all("$set" => { member_count: "1" })

And in test case reloaded results from database - string value transformed to Integer by demongoize. Tests always stays green, however problem exists:

          it "updates the first matching document" do
            expect(depeche_mode.reload.member_count).to eq(1)
          end

It is easy to find with couple of lines console output:

          it "updates the first matching document" do
            depeche_mode.reload
            pp depeche_mode.member_count
            pp depeche_mode.member_count.class
            pp depeche_mode.as_document[:member_count]
            pp depeche_mode.as_document[:member_count].class
            expect(depeche_mode.reload.member_count).to eq(1)
          end

Output of rspec test will be

bundle exec rspec ./spec/mongoid/contextual/mongo_spec.rb:3665
[mongoid] Warning: No private key present, creating unsigned gem.
Run options: include {:locations=>{"./spec/mongoid/contextual/mongo_spec.rb"=>[3665]}}                                                                                                               
W, [2024-05-10T13:16:15.670196 #31184]  WARN -- : Using BSON::Decimal128 as the field type is not supported. In BSON <= 4, the BSON::Decimal128 type will work as expected for both storing and querying, but will return a BigDecimal on query in BSON 5+. To use literal BSON::Decimal128 fields with BSON 5, set Mongoid.allow_bson5_decimal128 to true.
1
Integer
"1"
String
 1/1 |==================================================================================== 100 ====================================================================================>| Time: 00:00:00 

Finished in 0.18469 seconds (files took 0.65765 seconds to load)
1 example, 0 failures

Issue happens due to misuse of the mongoize_for third param. It should be key (i.e. field key), not operator

mongoize_for(operator, klass, operator, value)

In this case in mongoize_for field will be nil and it return original value without demongoization:

field = klass.fields[key.to_s]
return value unless field

This issue exists at least at 8.1-stable branch, but code is different there. I will provide separate PR to fix this issue in 8.1-stable branch.

8.1-stable fix is actually important for us (https://www.onepagecrm.com), we are aiming to make upgrade to 8.1.x, but do not go further for a minute.

This PR also fix a problem describer by midnight-wonderer in discussion #5812
He also right about line with misuse of first attribute:

(atomic_updates['$set'] ||= {})[key] = mongoize_for(key, klass, key, value)

I've included this small suggested fix in this PR.

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.

Excellent catch, and elegant solution. 👍

@jamis jamis merged commit eec835a into mongodb:master May 10, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants