[bug?] fields in update_all did not get mongoized

[midnight-wonderer]

I am using Mongoid 8.1.5.

By declaring the following document:

class BuggyModel
  include ::Mongoid::Document
  store_in collection: 'buggy_models'

  class MapperType
    MAPPING = {
      'A' => 1,
      'B' => 2,
    }.freeze
    INVERSE_MAPPING = MAPPING.invert.freeze

    class << self
      def mongoize(string)
        return string unless string.is_a?(::String)
        MAPPING[string]
      end

      def demongoize(integer)
        return integer unless integer.is_a?(::Integer)
        INVERSE_MAPPING[integer]
      end

      def evolve(hex_string)
        mongoize(hex_string)
      end
    end
  end

  field :_t, as: :testing, type: MapperType
end

I expect

::BuggyModel.create({ _id: 1 })
::BuggyModel.update_all({ '$set': { testing: 'B' } })

to produce

{ "_id": 1, "_t": 2 }

in the database.

Instead, it produces:

{ "_id": 1, "_t": "B" }

Not sure if this is intentional, but the "B" value doesn't get mongoized.

Also, unrelated to the bug, I audited some code there.
This line does not look right.

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

should be

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

it does not cause any bugs now, though.
But it might in the future.

If you happen to touch some code in that file, you might consider using The Boy Scout Rule to make an edit there also.

[dem]

@midnight-wonderer we hit into this problem as well during our journey of mongoid upgrade :-)

Custom type classes are not mongoized in update_all with $set operator.

We have a solution:
For master - #5814
For 8.1 branch - #5815

Currently we are using monkey patch on 8.1.5:

module Mongoid
  module Extensions
    module Hash
      def value_for(operator, klass, key, value)
        case operator
        when "$rename" then value.to_s
        when "$addToSet", "$push" then value.mongoize
        else mongoize_for(operator, klass, key, value)
        end
      end
    end
  end
end

It solves problem for us (quite serious in terms of the app functionality). Hopefully it will help.

And thanks for pointing to the line, which does not looks right:

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

[alexbevi]

Thanks for raising this discussion @midnight-wonderer, and for the PR @dem. Our team will have a look and circle back.

[jamis]

Thank you very much. I've approved both PRs and am currently working through some infrastructure issues getting the 8.1-stable PR merged. Might have to wait until Monday for it. 👍