From f13a1dde90ed6d6fda31ab0fac05268ed579fd5b Mon Sep 17 00:00:00 2001 From: Neil Shweky Date: Mon, 1 Aug 2022 11:46:01 -0400 Subject: [PATCH 1/5] MONGOID-5365 Change #attributes return value to Hash with a flag (#5301) * MONGOID-5365 Change #attributes return value to Hash with a flag * MONGOID-5365 add config_override to tests * MONGOID-5365 rename flag and flip default * MONGOID-5365 fix old flag ref --- docs/reference/configuration.txt | 6 +++ docs/release-notes/mongoid-7.5.txt | 41 ++++++++++++++++- lib/mongoid/config.rb | 6 +++ lib/mongoid/document.rb | 9 +++- .../has_and_belongs_to_many/proxy_spec.rb | 30 +++++++++++++ .../referenced/has_many/proxy_spec.rb | 20 +++++++++ spec/mongoid/attributes_spec.rb | 44 +++++++++++++++++++ spec/mongoid/config_spec.rb | 7 +++ 8 files changed, 161 insertions(+), 2 deletions(-) diff --git a/docs/reference/configuration.txt b/docs/reference/configuration.txt index e7e5d9fcf9..a67990b4ff 100644 --- a/docs/reference/configuration.txt +++ b/docs/reference/configuration.txt @@ -369,6 +369,12 @@ for details on driver options. # to parent contexts by default. (default: false) join_contexts: false + # When this flag is true, the attributes method on a document will return + # a BSON::Document when that document is retrieved from the database, and + # a Hash otherwise. When this flag is false, the attributes method will + # always return a Hash. (default: false) + #legacy_attributes: true + # Maintain legacy behavior of pluck and distinct, which does not demongoize # values on returning them. Setting this option to false will cause # pluck and distinct to return demongoized values. Setting this option to diff --git a/docs/release-notes/mongoid-7.5.txt b/docs/release-notes/mongoid-7.5.txt index bffe3bf46c..e2316ca472 100644 --- a/docs/release-notes/mongoid-7.5.txt +++ b/docs/release-notes/mongoid-7.5.txt @@ -206,7 +206,7 @@ example: .. code-block:: ruby Band.all.map(:name) - + # Equivalent to: Band.pluck(:name) @@ -244,3 +244,42 @@ The ``Document#to_a`` method is deprecated in Mongoid 7.5. Mongoid 7.5 fixes incorrect usage of the driver's ``update_one`` method from Mongoid's ``upsert`` method. Mongoid's ``upsert`` actually performs a replacing upsert, and Mongoid 7.5 correctly calls ``replace_one``. + + +Force the ``attributes`` Method to Always Return a ``Hash`` +----------------------------------------------------------- + +Mongoid 7.5 with the ``Mongoid.legacy_attributes`` option set to ``false`` +will always return a ``Hash`` when calling the ``attributes`` method. +For example: + +.. code-block:: ruby + + class Band + include Mongoid::Document + + field :name + end + + band = Band.create!(name: "The Rolling Stones") + p band.attributes.class + # => Hash + + band = Band.first + p band.attributes.class + # => Hash + +In Mongoid 7.4 and earlier, and in 7.5 with the ``Mongoid.legacy_attributes`` +option set to ``true``, the ``attributes`` method on a document will return a +``BSON::Document`` when retrieving that document from the database, but will +return a ``Hash`` when instantiating a new document: + +.. code-block:: ruby + + band = Band.create!(name: "The Rolling Stones") + p band.attributes.class + # => Hash + + band = Band.first + p band.attributes.class + # => BSON::Document diff --git a/lib/mongoid/config.rb b/lib/mongoid/config.rb index f6dba54d32..045a8d40fc 100644 --- a/lib/mongoid/config.rb +++ b/lib/mongoid/config.rb @@ -116,6 +116,12 @@ module Config # using and's instead of overwriting them. option :overwrite_chained_operators, default: true + # When this flag is true, the attributes method on a document will return + # a BSON::Document when that document is retrieved from the database, and + # a Hash otherwise. When this flag is false, the attributes method will + # always return a Hash. + option :legacy_attributes, default: false + # Has Mongoid been configured? This is checking that at least a valid # client config exists. # diff --git a/lib/mongoid/document.rb b/lib/mongoid/document.rb index 5a0fbba7a1..c475ab1b24 100644 --- a/lib/mongoid/document.rb +++ b/lib/mongoid/document.rb @@ -277,8 +277,15 @@ module ClassMethods # criteria. # # @return [ Document ] A new document. + # + # @api private def instantiate(attrs = nil, selected_fields = nil) - attributes = attrs || {} + attributes = if Mongoid.legacy_attributes + attrs + else + attrs&.to_h + end || {} + doc = allocate doc.__selected_fields = selected_fields doc.instance_variable_set(:@attributes, attributes) diff --git a/spec/mongoid/association/referenced/has_and_belongs_to_many/proxy_spec.rb b/spec/mongoid/association/referenced/has_and_belongs_to_many/proxy_spec.rb index 9578519c3e..e58024501a 100644 --- a/spec/mongoid/association/referenced/has_and_belongs_to_many/proxy_spec.rb +++ b/spec/mongoid/association/referenced/has_and_belongs_to_many/proxy_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "spec_helper" +require_relative "../has_and_belongs_to_many_models.rb" describe Mongoid::Association::Referenced::HasAndBelongsToMany::Proxy do @@ -3770,4 +3771,33 @@ class Distributor expect(p2.d_ids).to match_array([d2.id]) end end + + # This test is for MONGOID-5344 which tests that the initial call to + # signature_ids refers to the same array as subsequent calls to signature_ids. + # Prior to the change in that ticket, this test broke because the array + # returned from write_attribute (which is triggered the first time the + # foreign key array is referenced, to set the default), refers to a different + # array to the one stored in the attributes hash. This happened because, + # when retrieving a document from the database, the attributes hash is actually + # a BSON::Document, which applies a transformation to the array before + # storing it. + context "when executing concat on foreign key array from the db" do + config_override :legacy_attributes, false + + before do + HabtmmContract.create! + HabtmmSignature.create! + end + + let!(:contract) { HabtmmContract.first } + let!(:signature) { HabtmmSignature.first } + + before do + contract.signature_ids.concat([signature.id]) + end + + it "works on the first attempt" do + expect(contract.signature_ids).to eq([signature.id]) + end + end end diff --git a/spec/mongoid/association/referenced/has_many/proxy_spec.rb b/spec/mongoid/association/referenced/has_many/proxy_spec.rb index 646b57c788..a7a326ad57 100644 --- a/spec/mongoid/association/referenced/has_many/proxy_spec.rb +++ b/spec/mongoid/association/referenced/has_many/proxy_spec.rb @@ -4089,4 +4089,24 @@ expect(band.same_name).to eq([agent]) end end + + context "when executing concat on foreign key array from the db" do + config_override :legacy_attributes, false + + before do + Agent.create! + Basic.create! + end + + let!(:agent) { Agent.first } + let!(:basic) { Basic.first } + + before do + agent.basic_ids.concat([basic.id]) + end + + it "works on the first attempt" do + expect(agent.basic_ids).to eq([basic.id]) + end + end end diff --git a/spec/mongoid/attributes_spec.rb b/spec/mongoid/attributes_spec.rb index 141b2566ff..3c21013c19 100644 --- a/spec/mongoid/attributes_spec.rb +++ b/spec/mongoid/attributes_spec.rb @@ -1675,6 +1675,50 @@ end end end + + context "when comparing the object_ids of the written value" do + config_override :legacy_attributes, false + + before do + Person.create! + end + + let(:person) do + Person.first + end + + context "when the field is not resizable" do + let(:test) do + person.write_attribute(:test, "aliased field to test") + end + + it "has the same object_id as the attributes hash value" do + expect(test.object_id).to eq(person.test.object_id) + end + end + + context "when the field is resizable" do + + let(:arrays) do + person.write_attribute(:arrays, []) + end + + it "has the same object_id as the attributes hash value" do + expect(arrays.object_id).to eq(person.arrays.object_id) + end + end + + context "when the field is a HABTM foreign key array" do + + let(:preference_ids) do + person.write_attribute(:preference_ids, []) + end + + it "has the same object_id as the attributes hash value" do + expect(preference_ids.object_id).to eq(person.preference_ids.object_id) + end + end + end end describe "#typed_value_for" do diff --git a/spec/mongoid/config_spec.rb b/spec/mongoid/config_spec.rb index b8e9b6b46a..71e0240610 100644 --- a/spec/mongoid/config_spec.rb +++ b/spec/mongoid/config_spec.rb @@ -335,6 +335,13 @@ it_behaves_like "a config option" end + context 'when setting the legacy_attributes option in the config' do + let(:option) { :legacy_attributes } + let(:default) { false } + + it_behaves_like "a config option" + end + describe "#load!" do before(:all) do From 89397a71be20e1d58fbcb1404ba1dd26e1e3cc97 Mon Sep 17 00:00:00 2001 From: Neil Shweky Date: Mon, 1 Aug 2022 11:47:46 -0400 Subject: [PATCH 2/5] Update lib/mongoid/config.rb --- lib/mongoid/config.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mongoid/config.rb b/lib/mongoid/config.rb index 045a8d40fc..83f9cfc304 100644 --- a/lib/mongoid/config.rb +++ b/lib/mongoid/config.rb @@ -120,7 +120,7 @@ module Config # a BSON::Document when that document is retrieved from the database, and # a Hash otherwise. When this flag is false, the attributes method will # always return a Hash. - option :legacy_attributes, default: false + option :legacy_attributes, default: true # Has Mongoid been configured? This is checking that at least a valid # client config exists. From 2de44ddb5c74284fd3910c82d4c38fdc9fd725e3 Mon Sep 17 00:00:00 2001 From: Neil Shweky Date: Mon, 1 Aug 2022 11:48:11 -0400 Subject: [PATCH 3/5] Update docs/reference/configuration.txt --- docs/reference/configuration.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/configuration.txt b/docs/reference/configuration.txt index a67990b4ff..0d875d9405 100644 --- a/docs/reference/configuration.txt +++ b/docs/reference/configuration.txt @@ -372,7 +372,7 @@ for details on driver options. # When this flag is true, the attributes method on a document will return # a BSON::Document when that document is retrieved from the database, and # a Hash otherwise. When this flag is false, the attributes method will - # always return a Hash. (default: false) + # always return a Hash. (default: true) #legacy_attributes: true # Maintain legacy behavior of pluck and distinct, which does not demongoize From b9fe2d00f61ceb446bfea323fd922ad09da53a16 Mon Sep 17 00:00:00 2001 From: Neil Shweky Date: Mon, 1 Aug 2022 11:48:47 -0400 Subject: [PATCH 4/5] Update docs/reference/configuration.txt --- docs/reference/configuration.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/configuration.txt b/docs/reference/configuration.txt index 0d875d9405..f51ab1ee40 100644 --- a/docs/reference/configuration.txt +++ b/docs/reference/configuration.txt @@ -373,7 +373,7 @@ for details on driver options. # a BSON::Document when that document is retrieved from the database, and # a Hash otherwise. When this flag is false, the attributes method will # always return a Hash. (default: true) - #legacy_attributes: true + legacy_attributes: true # Maintain legacy behavior of pluck and distinct, which does not demongoize # values on returning them. Setting this option to false will cause From f6202eaef46c4cf477f474616f2731e07fcd7a41 Mon Sep 17 00:00:00 2001 From: Neil Shweky Date: Mon, 1 Aug 2022 12:31:29 -0400 Subject: [PATCH 5/5] MONGOID-5365 fix config test --- spec/mongoid/config_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/mongoid/config_spec.rb b/spec/mongoid/config_spec.rb index 71e0240610..96c856d211 100644 --- a/spec/mongoid/config_spec.rb +++ b/spec/mongoid/config_spec.rb @@ -337,7 +337,7 @@ context 'when setting the legacy_attributes option in the config' do let(:option) { :legacy_attributes } - let(:default) { false } + let(:default) { true } it_behaves_like "a config option" end