From 374cb570b0364714ec0ed2b014e2880a6989fdaa Mon Sep 17 00:00:00 2001 From: johnnyshields <27655+johnnyshields@users.noreply.github.com> Date: Sun, 27 Aug 2023 16:15:53 +0900 Subject: [PATCH 1/4] Move Hash#__consolidate__ method. It is only used once in the entire codebase, in a private capacity inside Contextual::Mongo class, so there is no reason to monkey-patch Hash for this. The method name has __ prefix so we can assume this is an internal only method that is safe to remove. --- lib/mongoid/contextual/mongo.rb | 46 ++++++++++++++++++++- lib/mongoid/extensions/hash.rb | 50 ----------------------- spec/mongoid/contextual/mongo_spec.rb | 48 ++++++++++++++++++++++ spec/mongoid/extensions/hash_spec.rb | 57 --------------------------- 4 files changed, 92 insertions(+), 109 deletions(-) diff --git a/lib/mongoid/contextual/mongo.rb b/lib/mongoid/contextual/mongo.rb index 7333f059a..0fad123f8 100644 --- a/lib/mongoid/contextual/mongo.rb +++ b/lib/mongoid/contextual/mongo.rb @@ -803,8 +803,8 @@ def documents_loader def update_documents(attributes, method = :update_one, opts = {}) return false unless attributes - attributes = attributes.transform_keys { |k| klass.database_field_name(k.to_s) } - view.send(method, attributes.__consolidate__(klass), opts) + attributes = Hash[attributes.map { |k, v| [klass.database_field_name(k.to_s), v] }] + view.send(method, prepare_atomic_updates(klass, attributes), opts) end # Apply the field limitations. @@ -992,6 +992,48 @@ def retrieve_nth_to_last_with_limit(n, limit) process_raw_docs(raw_docs, limit) end # rubocop:enable Naming/MethodParameterName + + # 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 ] The attributes to convert. + # + # @return [ Hash ] The prepared atomic updates. + def prepare_atomic_updates(klass, attributes) + attributes.each_pair.with_object({}) do |(key, value), atomic_updates| + if key.to_s.start_with?('$') + value = value.each_with_object({}) do |(key2, value2), hash| + key2 = klass.database_field_name(key2) + hash[key2] = key == '$rename' ? value2.to_s : mongoize_for_atomic_update(klass, key, key2, value2) + end + atomic_updates[key] ||= {} + atomic_updates[key].update(value) + else + atomic_updates['$set'] ||= {} + atomic_updates['$set'][key] = mongoize_for_atomic_update(klass, key, key, value) + end + end + end + + # Mongoize a value for an atomic update for the given klass, operator, and field. + # + # @param [ Class ] klass The model class. + # @param [ String ] operator The operator. + # @param [ String | Symbol ] field_name The field key. + # @param [ Object ] value The value to mongoize. + # + # @return [ Object ] The mongoized value. + def mongoize_for_atomic_update(klass, operator, field_name, value) + field = klass.fields[field_name.to_s] + return value unless field + + value = field.mongoize(value) + if Mongoid::Persistable::LIST_OPERATIONS.include?(operator) && field.resizable? && !value.is_a?(Array) + value = value.first + end + value + end end end end diff --git a/lib/mongoid/extensions/hash.rb b/lib/mongoid/extensions/hash.rb index ea0862381..9f866bac7 100644 --- a/lib/mongoid/extensions/hash.rb +++ b/lib/mongoid/extensions/hash.rb @@ -30,28 +30,6 @@ def __mongoize_object_id__ end end - # Consolidate the key/values in the hash under an atomic $set. - # - # @example Consolidate the hash. - # { name: "Placebo" }.__consolidate__ - # - # @return [ Hash ] A new consolidated hash. - def __consolidate__(klass) - each_pair.with_object({}) do |(key, value), consolidated| - if key.start_with?('$') - value = value.each_with_object({}) do |(key2, value2), hash| - key2 = klass.database_field_name(key2) - hash[key2] = key == '$rename' ? value2.to_s : mongoize_for(key, klass, key2, value2) - end - consolidated[key] ||= {} - consolidated[key].update(value) - else - consolidated['$set'] ||= {} - consolidated['$set'].update(key => mongoize_for(key, klass, key, value)) - end - end - end - # Checks whether conditions given in this hash are known to be # unsatisfiable, i.e., querying with this hash will always return no # documents. @@ -182,34 +160,6 @@ def to_criteria criteria end - private - - # 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? && !value.is_a?(Array) - val = val.first - 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/contextual/mongo_spec.rb b/spec/mongoid/contextual/mongo_spec.rb index fb9e7a949..62120e61e 100644 --- a/spec/mongoid/contextual/mongo_spec.rb +++ b/spec/mongoid/contextual/mongo_spec.rb @@ -4640,4 +4640,52 @@ end end end + + describe '#prepare_atomic_updates' do + let(:instance) { described_class.new(Band.where(name: 'Depeche Mode')) } + + subject { instance.send(:prepare_atomic_updates, Band, hash) } + + 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 + + it 'moves the non hash values under the provided key' do + expect(subject).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(subject).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 + + it 'moves the non hash values under the provided key' do + expect(subject).to eq({ + '$set' => { likes: 10, name: 'Tool' }, + '$inc' => { 'plays' => 1 } + }) + end + end + end end diff --git a/spec/mongoid/extensions/hash_spec.rb b/spec/mongoid/extensions/hash_spec.rb index 0f2d91ee4..311061c1e 100644 --- a/spec/mongoid/extensions/hash_spec.rb +++ b/spec/mongoid/extensions/hash_spec.rb @@ -162,63 +162,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 - context 'when the hash key is a string' do let(:hash) do From 336b14b4994591cacd154af0ccc85ba703986c5c Mon Sep 17 00:00:00 2001 From: johnnyshields <27655+johnnyshields@users.noreply.github.com> Date: Sun, 27 Aug 2023 16:28:36 +0900 Subject: [PATCH 2/4] Fix rubocop, get rid of extra loop --- .rubocop_todo.yml | 2 +- lib/mongoid/contextual/mongo.rb | 2 +- spec/mongoid/contextual/mongo_spec.rb | 28 +++++++++++++-------------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 8f1bd42e1..3227b8588 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -25,7 +25,7 @@ Metrics/BlockNesting: # Offense count: 17 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 327 + Max: 351 # Offense count: 50 # Configuration parameters: AllowedMethods, AllowedPatterns. diff --git a/lib/mongoid/contextual/mongo.rb b/lib/mongoid/contextual/mongo.rb index 0fad123f8..8e2d6d2b1 100644 --- a/lib/mongoid/contextual/mongo.rb +++ b/lib/mongoid/contextual/mongo.rb @@ -803,7 +803,6 @@ def documents_loader 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, prepare_atomic_updates(klass, attributes), opts) end @@ -1002,6 +1001,7 @@ def retrieve_nth_to_last_with_limit(n, limit) # @return [ Hash ] The prepared atomic updates. def prepare_atomic_updates(klass, attributes) attributes.each_pair.with_object({}) do |(key, value), atomic_updates| + key = klass.database_field_name(key) if key.to_s.start_with?('$') value = value.each_with_object({}) do |(key2, value2), hash| key2 = klass.database_field_name(key2) diff --git a/spec/mongoid/contextual/mongo_spec.rb b/spec/mongoid/contextual/mongo_spec.rb index 62120e61e..2ff6a74c1 100644 --- a/spec/mongoid/contextual/mongo_spec.rb +++ b/spec/mongoid/contextual/mongo_spec.rb @@ -4642,9 +4642,9 @@ end describe '#prepare_atomic_updates' do - let(:instance) { described_class.new(Band.where(name: 'Depeche Mode')) } + subject(:updates) { instance.send(:prepare_atomic_updates, Band, hash) } - subject { instance.send(:prepare_atomic_updates, Band, hash) } + let(:instance) { described_class.new(Band.where(name: 'Depeche Mode')) } context 'when the hash already contains the key' do @@ -4654,10 +4654,10 @@ end it 'moves the non hash values under the provided key' do - expect(subject).to eq({ - '$set' => { 'name' => 'Tool', likes: 10 }, - '$inc' => { 'plays' => 1 } - }) + expect(updates).to eq({ + '$set' => { 'name' => 'Tool', 'likes' => 10 }, + '$inc' => { 'plays' => 1 } + }) end end @@ -4667,10 +4667,10 @@ end it 'moves the non hash values under the provided key' do - expect(subject).to eq({ - '$set' => { likes: 10, 'name' => 'Tool' }, - '$inc' => { 'plays' => 1 } - }) + expect(updates).to eq({ + '$set' => { 'likes' => 10, 'name' => 'Tool' }, + '$inc' => { 'plays' => 1 } + }) end end end @@ -4681,10 +4681,10 @@ end it 'moves the non hash values under the provided key' do - expect(subject).to eq({ - '$set' => { likes: 10, name: 'Tool' }, - '$inc' => { 'plays' => 1 } - }) + expect(updates).to eq({ + '$set' => { 'likes' => 10, 'name' => 'Tool' }, + '$inc' => { 'plays' => 1 } + }) end end end From a8ed0437bcb7432269994085b24518d8e5b379d6 Mon Sep 17 00:00:00 2001 From: johnnyshields <27655+johnnyshields@users.noreply.github.com> Date: Sun, 27 Aug 2023 18:21:43 +0900 Subject: [PATCH 3/4] Fix spec --- lib/mongoid/contextual/mongo.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mongoid/contextual/mongo.rb b/lib/mongoid/contextual/mongo.rb index 8e2d6d2b1..5ba916755 100644 --- a/lib/mongoid/contextual/mongo.rb +++ b/lib/mongoid/contextual/mongo.rb @@ -1029,7 +1029,7 @@ def mongoize_for_atomic_update(klass, operator, field_name, value) return value unless field value = field.mongoize(value) - if Mongoid::Persistable::LIST_OPERATIONS.include?(operator) && field.resizable? && !value.is_a?(Array) + if Mongoid::Persistable::LIST_OPERATIONS.include?(operator) && field.resizable? && value.is_a?(Array) value = value.first end value From ec519246f2cc0cc0d625edc9a6dc948e3255ddf2 Mon Sep 17 00:00:00 2001 From: johnnyshields <27655+johnnyshields@users.noreply.github.com> Date: Sun, 27 Aug 2023 18:25:31 +0900 Subject: [PATCH 4/4] Fix logic --- lib/mongoid/contextual/mongo.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/mongoid/contextual/mongo.rb b/lib/mongoid/contextual/mongo.rb index 5ba916755..0ba997e2b 100644 --- a/lib/mongoid/contextual/mongo.rb +++ b/lib/mongoid/contextual/mongo.rb @@ -1028,11 +1028,11 @@ def mongoize_for_atomic_update(klass, operator, field_name, value) field = klass.fields[field_name.to_s] return value unless field - value = field.mongoize(value) - if Mongoid::Persistable::LIST_OPERATIONS.include?(operator) && field.resizable? && value.is_a?(Array) - value = value.first + mongoized = field.mongoize(value) + if Mongoid::Persistable::LIST_OPERATIONS.include?(operator) && field.resizable? && !value.is_a?(Array) + mongoized = mongoized.first end - value + mongoized end end end