From 7b085952ee8c52582426aa67c0427d1e8e622146 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 6 Nov 2023 09:52:28 -0700 Subject: [PATCH 1/5] refactor and deprecate Hash#__consolidate__ --- lib/mongoid/atomic_update_preparer.rb | 91 +++++++++++++++++++++ lib/mongoid/contextual/mongo.rb | 3 +- lib/mongoid/extensions/hash.rb | 69 ++-------------- spec/mongoid/atomic_update_preparer_spec.rb | 77 +++++++++++++++++ spec/mongoid/extensions/hash_spec.rb | 57 ------------- 5 files changed, 177 insertions(+), 120 deletions(-) create mode 100644 lib/mongoid/atomic_update_preparer.rb create mode 100644 spec/mongoid/atomic_update_preparer_spec.rb diff --git a/lib/mongoid/atomic_update_preparer.rb b/lib/mongoid/atomic_update_preparer.rb new file mode 100644 index 0000000000..9cd058b490 --- /dev/null +++ b/lib/mongoid/atomic_update_preparer.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +module Mongoid + # A singleton class to assist with preparing attributes for atomic + # updates. + # + # Once the deprecated Hash#__consolidate__ method is removed entirely, + # these methods may be moved into Mongoid::Contextual::Mongo as private + # methods. + # + # @api private + class AtomicUpdatePreparer + class << self + # Convert the key/values in the attributes into a hash of atomic updates. + # Non-operator keys are assumed to use $set operation. + # + # @param [ Class ] klass The model class. + # @param [ Hash ] attributes The attributes to convert. + # + # @return [ Hash ] The prepared atomic updates. + def prepare(attributes, klass) + attributes.each_pair.with_object({}) do |(key, value), atomic_updates| + if key.to_s.start_with?('$') + (atomic_updates[key] ||= {}).update(prepare_operation(klass, key, value)) + else + (atomic_updates['$set'] ||= {})[key] = mongoize_for(key, klass, key, value) + end + end + end + + private + + # Treats the key as if it were a MongoDB operator and prepares + # the value accordingly. + # + # @param [ Class ] klass the model class + # @param [ String | Symbol ] key the operator + # @param [ Hash ] value the operand + # + # @return [ Hash ] the prepared value. + def prepare_operation(klass, key, value) + value.each_with_object({}) do |(key2, value2), hash| + key2 = klass.database_field_name(key2) + hash[key2] = value_for(key, klass, value2) + end + end + + # Get the value for the provided operator, klass, key and value. + # + # This is necessary for special cases like $rename, $addToSet and $push. + # + # @param [ String ] operator The operator. + # @param [ Class ] klass The model class. + # @param [ Object ] value The original value. + # + # @return [ Object ] Value prepared for the provided operator. + def value_for(operator, klass, value) + case operator + when '$rename' then value.to_s + when '$addToSet', '$push' then value.mongoize + else mongoize_for(operator, klass, operator, value) + end + end + + # Mongoize for the klass, key and value. + # + # @api private + # + # @example Mongoize for the klass, field and value. + # {}.mongoize_for("$push", Band, "name", "test") + # + # @param [ String ] operator The operator. + # @param [ Class ] klass The model class. + # @param [ String | Symbol ] key The field key. + # @param [ Object ] value The value to mongoize. + # + # @return [ Object ] The mongoized value. + def mongoize_for(operator, klass, key, value) + field = klass.fields[key.to_s] + return value unless field + + mongoized = field.mongoize(value) + if Mongoid::Persistable::LIST_OPERATIONS.include?(operator) && field.resizable? && !value.is_a?(Array) + return mongoized.first + end + + mongoized + end + end + end +end diff --git a/lib/mongoid/contextual/mongo.rb b/lib/mongoid/contextual/mongo.rb index 916d8a2a72..bf2e32be62 100644 --- a/lib/mongoid/contextual/mongo.rb +++ b/lib/mongoid/contextual/mongo.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true # rubocop:todo all +require 'mongoid/atomic_update_preparer' require "mongoid/contextual/mongo/documents_loader" require "mongoid/contextual/atomic" require "mongoid/contextual/aggregable/mongo" @@ -818,7 +819,7 @@ def load_async def update_documents(attributes, method = :update_one, opts = {}) return false unless attributes attributes = Hash[attributes.map { |k, v| [klass.database_field_name(k.to_s), v] }] - view.send(method, attributes.__consolidate__(klass), opts) + view.send(method, AtomicUpdatePreparer.prepare(attributes, klass), opts) end # Apply the field limitations. diff --git a/lib/mongoid/extensions/hash.rb b/lib/mongoid/extensions/hash.rb index f1d1a27e22..1cf19d539d 100644 --- a/lib/mongoid/extensions/hash.rb +++ b/lib/mongoid/extensions/hash.rb @@ -32,31 +32,20 @@ def __mongoize_object_id__ end # Consolidate the key/values in the hash under an atomic $set. + # DEPRECATED. This was never intended to be a public API and + # the functionality will no longer be exposed once this method + # is eventually removed. # # @example Consolidate the hash. # { name: "Placebo" }.__consolidate__ # # @return [ Hash ] A new consolidated hash. + # + # @deprecated def __consolidate__(klass) - consolidated = {} - each_pair do |key, value| - if key =~ /\$/ - value.keys.each do |key2| - value2 = value[key2] - real_key = klass.database_field_name(key2) - - value.delete(key2) if real_key != key2 - value[real_key] = value_for(key, klass, real_key, value2) - end - consolidated[key] ||= {} - consolidated[key].update(value) - else - consolidated["$set"] ||= {} - consolidated["$set"].update(key => mongoize_for(key, klass, key, value)) - end - end - consolidated + Mongoid::AtomicUpdatePreparer.prepare(self, klass) end + Mongoid.deprecate(self, :__consolidate__) # Checks whether conditions given in this hash are known to be # unsatisfiable, i.e., querying with this hash will always return no @@ -166,50 +155,6 @@ def to_criteria private - # Get the value for the provided operator, klass, key and value. - # - # This is necessary for special cases like $rename, $addToSet and $push. - # - # @param [ String ] operator The operator. - # @param [ Class ] klass The model class. - # @param [ String | Symbol ] key The field key. - # @param [ Object ] value The original value. - # - # @return [ Object ] Value prepared for the provided operator. - 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, operator, value) - end - end - - # Mongoize for the klass, key and value. - # - # @api private - # - # @example Mongoize for the klass, field and value. - # {}.mongoize_for("$push", Band, "name", "test") - # - # @param [ String ] operator The operator. - # @param [ Class ] klass The model class. - # @param [ String | Symbol ] key The field key. - # @param [ Object ] value The value to mongoize. - # - # @return [ Object ] The mongoized value. - def mongoize_for(operator, klass, key, value) - field = klass.fields[key.to_s] - if field - val = field.mongoize(value) - if Mongoid::Persistable::LIST_OPERATIONS.include?(operator) && field.resizable? - val = val.first if !value.is_a?(Array) - end - val - else - value - end - end - module ClassMethods # Turn the object from the ruby type we deal with to a Mongo friendly diff --git a/spec/mongoid/atomic_update_preparer_spec.rb b/spec/mongoid/atomic_update_preparer_spec.rb new file mode 100644 index 0000000000..c017f05cbe --- /dev/null +++ b/spec/mongoid/atomic_update_preparer_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Mongoid::AtomicUpdatePreparer do + describe '#prepare' do + let(:prepared) { described_class.prepare(hash, Band) } + + context 'when the hash already contains $set' do + context 'when the $set is first' do + let(:hash) do + { '$set' => { name: 'Tool' }, likes: 10, '$inc' => { plays: 1 } } + end + + it 'moves the non hash values under the provided key' do + expect(prepared).to eq({ + '$set' => { 'name' => 'Tool', likes: 10 }, '$inc' => { 'plays' => 1 } + }) + end + end + + context 'when the $set is not first' do + let(:hash) do + { likes: 10, '$inc' => { plays: 1 }, '$set' => { name: 'Tool' }} + end + + it 'moves the non hash values under the provided key' do + expect(prepared).to eq({ + '$set' => { likes: 10, 'name' => 'Tool' }, '$inc' => { 'plays' => 1 } + }) + end + end + end + + context 'when the hash does not contain $set' do + let(:hash) do + { likes: 10, '$inc' => { plays: 1 }, name: 'Tool' } + end + + it 'moves the non hash values under the provided key' do + expect(prepared).to eq({ + '$set' => { likes: 10, name: 'Tool' }, '$inc' => { 'plays' => 1 } + }) + end + end + + context 'when the hash contains $rename' do + let(:hash) { { likes: 10, '$rename' => { old: 'new' } } } + + it 'preserves the $rename operator' do + expect(prepared).to eq({ + '$set' => { likes: 10 }, '$rename' => { 'old' => 'new' } + }) + end + end + + context 'when the hash contains $addToSet' do + let(:hash) { { likes: 10, '$addToSet' => { list: 'new' } } } + + it 'preserves the $addToSet operator' do + expect(prepared).to eq({ + '$set' => { likes: 10 }, '$addToSet' => { 'list' => 'new' } + }) + end + end + + context 'when the hash contains $push' do + let(:hash) { { likes: 10, '$push' => { list: 14 } } } + + it 'preserves the $push operator' do + expect(prepared).to eq({ + '$set' => { likes: 10 }, '$push' => { 'list' => 14 } + }) + end + end + end +end diff --git a/spec/mongoid/extensions/hash_spec.rb b/spec/mongoid/extensions/hash_spec.rb index cc4135a742..bcc1a58d6f 100644 --- a/spec/mongoid/extensions/hash_spec.rb +++ b/spec/mongoid/extensions/hash_spec.rb @@ -163,63 +163,6 @@ end end - describe "#__consolidate__" do - - context "when the hash already contains the key" do - - context "when the $set is first" do - - let(:hash) do - { "$set" => { name: "Tool" }, likes: 10, "$inc" => { plays: 1 }} - end - - let(:consolidated) do - hash.__consolidate__(Band) - end - - it "moves the non hash values under the provided key" do - expect(consolidated).to eq({ - "$set" => { 'name' => "Tool", likes: 10 }, "$inc" => { 'plays' => 1 } - }) - end - end - - context "when the $set is not first" do - - let(:hash) do - { likes: 10, "$inc" => { plays: 1 }, "$set" => { name: "Tool" }} - end - - let(:consolidated) do - hash.__consolidate__(Band) - end - - it "moves the non hash values under the provided key" do - expect(consolidated).to eq({ - "$set" => { likes: 10, 'name' => "Tool" }, "$inc" => { 'plays' => 1 } - }) - end - end - end - - context "when the hash does not contain the key" do - - let(:hash) do - { likes: 10, "$inc" => { plays: 1 }, name: "Tool"} - end - - let(:consolidated) do - hash.__consolidate__(Band) - end - - it "moves the non hash values under the provided key" do - expect(consolidated).to eq({ - "$set" => { likes: 10, name: "Tool" }, "$inc" => { 'plays' => 1 } - }) - end - end - end - describe ".demongoize" do let(:hash) do From e478d0882fe9f6ad66816d49b33ce0ef0e806c82 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 6 Nov 2023 10:02:33 -0700 Subject: [PATCH 2/5] fix linter complaints --- spec/mongoid/atomic_update_preparer_spec.rb | 44 ++++++++++++--------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/spec/mongoid/atomic_update_preparer_spec.rb b/spec/mongoid/atomic_update_preparer_spec.rb index c017f05cbe..2fe6421784 100644 --- a/spec/mongoid/atomic_update_preparer_spec.rb +++ b/spec/mongoid/atomic_update_preparer_spec.rb @@ -13,21 +13,23 @@ end it 'moves the non hash values under the provided key' do - expect(prepared).to eq({ - '$set' => { 'name' => 'Tool', likes: 10 }, '$inc' => { 'plays' => 1 } - }) + expect(prepared).to eq( + '$set' => { 'name' => 'Tool', likes: 10 }, + '$inc' => { 'plays' => 1 } + ) end end context 'when the $set is not first' do let(:hash) do - { likes: 10, '$inc' => { plays: 1 }, '$set' => { name: 'Tool' }} + { likes: 10, '$inc' => { plays: 1 }, '$set' => { name: 'Tool' } } end it 'moves the non hash values under the provided key' do - expect(prepared).to eq({ - '$set' => { likes: 10, 'name' => 'Tool' }, '$inc' => { 'plays' => 1 } - }) + expect(prepared).to eq( + '$set' => { likes: 10, 'name' => 'Tool' }, + '$inc' => { 'plays' => 1 } + ) end end end @@ -38,9 +40,10 @@ end it 'moves the non hash values under the provided key' do - expect(prepared).to eq({ - '$set' => { likes: 10, name: 'Tool' }, '$inc' => { 'plays' => 1 } - }) + expect(prepared).to eq( + '$set' => { likes: 10, name: 'Tool' }, + '$inc' => { 'plays' => 1 } + ) end end @@ -48,9 +51,10 @@ let(:hash) { { likes: 10, '$rename' => { old: 'new' } } } it 'preserves the $rename operator' do - expect(prepared).to eq({ - '$set' => { likes: 10 }, '$rename' => { 'old' => 'new' } - }) + expect(prepared).to eq( + '$set' => { likes: 10 }, + '$rename' => { 'old' => 'new' } + ) end end @@ -58,9 +62,10 @@ let(:hash) { { likes: 10, '$addToSet' => { list: 'new' } } } it 'preserves the $addToSet operator' do - expect(prepared).to eq({ - '$set' => { likes: 10 }, '$addToSet' => { 'list' => 'new' } - }) + expect(prepared).to eq( + '$set' => { likes: 10 }, + '$addToSet' => { 'list' => 'new' } + ) end end @@ -68,9 +73,10 @@ let(:hash) { { likes: 10, '$push' => { list: 14 } } } it 'preserves the $push operator' do - expect(prepared).to eq({ - '$set' => { likes: 10 }, '$push' => { 'list' => 14 } - }) + expect(prepared).to eq( + '$set' => { likes: 10 }, + '$push' => { 'list' => 14 } + ) end end end From 854ec9f9257d3225b765dfa3aa555cec15b23b72 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Tue, 7 Nov 2023 08:49:52 -0700 Subject: [PATCH 3/5] another minor refactoring for optimization --- lib/mongoid/atomic_update_preparer.rb | 2 ++ lib/mongoid/contextual/mongo.rb | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/mongoid/atomic_update_preparer.rb b/lib/mongoid/atomic_update_preparer.rb index 9cd058b490..249c5ba242 100644 --- a/lib/mongoid/atomic_update_preparer.rb +++ b/lib/mongoid/atomic_update_preparer.rb @@ -20,6 +20,8 @@ class << self # @return [ Hash ] The prepared atomic updates. def prepare(attributes, klass) attributes.each_pair.with_object({}) do |(key, value), atomic_updates| + key = klass.database_field_name(key.to_s) + if key.to_s.start_with?('$') (atomic_updates[key] ||= {}).update(prepare_operation(klass, key, value)) else diff --git a/lib/mongoid/contextual/mongo.rb b/lib/mongoid/contextual/mongo.rb index bf2e32be62..3fceba9dfc 100644 --- a/lib/mongoid/contextual/mongo.rb +++ b/lib/mongoid/contextual/mongo.rb @@ -818,7 +818,7 @@ def load_async # @return [ true | false ] If the update succeeded. def update_documents(attributes, method = :update_one, opts = {}) return false unless attributes - attributes = Hash[attributes.map { |k, v| [klass.database_field_name(k.to_s), v] }] + view.send(method, AtomicUpdatePreparer.prepare(attributes, klass), opts) end From 1483e10d988124658a06a113d8a315da6fb7f79a Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Tue, 7 Nov 2023 08:51:41 -0700 Subject: [PATCH 4/5] remove pre-refactoring docs --- lib/mongoid/atomic_update_preparer.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/mongoid/atomic_update_preparer.rb b/lib/mongoid/atomic_update_preparer.rb index 249c5ba242..c4655eefd1 100644 --- a/lib/mongoid/atomic_update_preparer.rb +++ b/lib/mongoid/atomic_update_preparer.rb @@ -66,11 +66,6 @@ def value_for(operator, klass, value) # Mongoize for the klass, key and value. # - # @api private - # - # @example Mongoize for the klass, field and value. - # {}.mongoize_for("$push", Band, "name", "test") - # # @param [ String ] operator The operator. # @param [ Class ] klass The model class. # @param [ String | Symbol ] key The field key. From d9dfe77057eabfa7c3402a1349e11bb8dc77f6d5 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Tue, 7 Nov 2023 09:17:31 -0700 Subject: [PATCH 5/5] fix failing specs --- spec/mongoid/atomic_update_preparer_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/mongoid/atomic_update_preparer_spec.rb b/spec/mongoid/atomic_update_preparer_spec.rb index 2fe6421784..6c39ac3aa6 100644 --- a/spec/mongoid/atomic_update_preparer_spec.rb +++ b/spec/mongoid/atomic_update_preparer_spec.rb @@ -14,7 +14,7 @@ it 'moves the non hash values under the provided key' do expect(prepared).to eq( - '$set' => { 'name' => 'Tool', likes: 10 }, + '$set' => { 'name' => 'Tool', 'likes' => 10 }, '$inc' => { 'plays' => 1 } ) end @@ -27,7 +27,7 @@ it 'moves the non hash values under the provided key' do expect(prepared).to eq( - '$set' => { likes: 10, 'name' => 'Tool' }, + '$set' => { 'likes' => 10, 'name' => 'Tool' }, '$inc' => { 'plays' => 1 } ) end @@ -41,7 +41,7 @@ it 'moves the non hash values under the provided key' do expect(prepared).to eq( - '$set' => { likes: 10, name: 'Tool' }, + '$set' => { 'likes' => 10, 'name' => 'Tool' }, '$inc' => { 'plays' => 1 } ) end @@ -52,7 +52,7 @@ it 'preserves the $rename operator' do expect(prepared).to eq( - '$set' => { likes: 10 }, + '$set' => { 'likes' => 10 }, '$rename' => { 'old' => 'new' } ) end @@ -63,7 +63,7 @@ it 'preserves the $addToSet operator' do expect(prepared).to eq( - '$set' => { likes: 10 }, + '$set' => { 'likes' => 10 }, '$addToSet' => { 'list' => 'new' } ) end @@ -74,7 +74,7 @@ it 'preserves the $push operator' do expect(prepared).to eq( - '$set' => { likes: 10 }, + '$set' => { 'likes' => 10 }, '$push' => { 'list' => 14 } ) end