MONGOID-5228 disallow _id to be updated on persisted documents (porte…

…d to 8.1-stable) (#5545)

* port #5542 to 8.1-stable

* test tweak

* update 8.1 release notes to mention immutable_ids

docs/release-notes/mongoid-8.1.txt

@@ -398,3 +398,23 @@ criteria in a ``$nor`` operation. For example:
 
 This would query all buildings in Portland, excluding apartments, buildings less than
 100 units tall, and buildings with an occupancy greater than 2500 people.
+
+
+Added ``Mongoid::Config.immutable_ids``
+---------------------------------------
+
+Coming in Mongoid 9.0, the ``_id`` field will be immutable in both top-level
+and embedded documents. This addresses some inconsistency in how mutations
+to the ``_id`` field are treated currently. To prepare for this potentially
+breaking change, the ``Mongoid::Config.immutable_ids`` flag has been added. It
+defaults to ``false``, preserving the existing behavior, but you may set it to
+``true`` to prepare your apps for Mongoid 9.0. When this is set to ``true``,
+attempts to mutate the ``_id`` of a document will raise an exception.
+
+.. code:: ruby
+
+  # The default in Mongoid 8.1
+  Mongoid::Config.immutable_ids = false
+
+  # The default in Mongoid 9.0
+  Mongoid::Config.immutable_ids = true

lib/config/locales/en.yml

@@ -108,6 +108,13 @@ en:
           summary: "Your mongoid.yml configuration file appears to be empty."
           resolution: "Ensure your configuration file contains the correct contents.
             Refer to: https://www.mongodb.com/docs/mongoid/current/reference/configuration/"
+        immutable_attribute:
+          message: "Attempted to change the immutable attribute '%{name}' with
+            the value: %{value}."
+          summary: "Immutable attributes can only have values set when the
+            document is a new record."
+          resolution: "Do not attempt to update the value of '%{name}' after
+            the document is persisted."
         invalid_collection:
           message: "Access to the collection for %{klass} is not allowed."
           summary: "%{klass}.collection was called, and %{klass} is an embedded

lib/mongoid/association/nested/one.rb

@@ -32,6 +32,8 @@ def build(parent)
             parent.send(association.setter, Factory.build(@class_name, attributes))
           elsif delete?
             parent.send(association.setter, nil)
+          else
+            check_for_id_violation!
           end
         end
 
@@ -54,6 +56,17 @@ def initialize(association, attributes, options)
 
         private
 
+        # Extracts and converts the id to the expected type.
+        #
+        # @return [ BSON::ObjectId | String | Object | nil ] The converted id,
+        #   or nil if no id is present in the attributes hash.
+        def extracted_id
+          @extracted_id ||= begin
+            id = association.klass.extract_id_field(attributes)
+            convert_id(existing.class, id)
+          end
+        end
+
         # Is the id in the attributes acceptable for allowing an update to
         # the existing association?
         #
@@ -64,8 +77,7 @@ def initialize(association, attributes, options)
         #
         # @return [ true | false ] If the id part of the logic will allow an update.
         def acceptable_id?
-          id = association.klass.extract_id_field(attributes)
-          id = convert_id(existing.class, id)
+          id = extracted_id
           existing._id == id || id.nil? || (existing._id != id && update_only?)
         end
 
@@ -110,6 +122,32 @@ def replace?
         def update?
           existing && !destroyable? && acceptable_id?
         end
+
+        # Checks to see if the _id attribute (which is supposed to be
+        # immutable) is being asked to change. If so, raise an exception.
+        #
+        # If Mongoid::Config.immutable_ids is false, this will do nothing,
+        # and the update operation will fail silently.
+        #
+        # @raise [ Errors::ImmutableAttribute ] if _id has changed, and
+        #   the document has been persisted.
+        def check_for_id_violation!
+          # look for the basic criteria of an update (see #update?)
+          return unless existing&.persisted? && !destroyable?
+
+          # if the id is either absent, or if it equals the existing record's
+          # id, there is no immutability violation.
+          id = extracted_id
+          return if existing._id == id || id.nil?
+
+          # otherwise, an attempt has been made to set the _id of an existing,
+          # persisted document.
+          if Mongoid::Config.immutable_ids
+            raise Errors::ImmutableAttribute.new(:_id, id)
+          else
+            Mongoid::Warnings.warn_mutable_ids
+          end
+        end
       end
     end
   end

lib/mongoid/config.rb

@@ -150,6 +150,13 @@ module Config
     # reload, but when it is turned off, it won't be.
     option :legacy_readonly, default: true
 
+    # When this flag is true, any attempt to change the _id of a persisted
+    # document will raise an exception (`Errors::ImmutableAttribute`).
+    # This will be the default in 9.0. When this flag is false (the default
+    # in 8.x), changing the _id of a persisted document might be ignored,
+    # or it might work, depending on the situation.
+    option :immutable_ids, default: false
+
     # Returns the Config singleton, for use in the configure DSL.
     #
     # @return [ self ] The Config singleton.

lib/mongoid/errors.rb

@@ -8,6 +8,7 @@
 require "mongoid/errors/document_not_destroyed"
 require "mongoid/errors/document_not_found"
 require "mongoid/errors/empty_config_file"
+require "mongoid/errors/immutable_attribute"
 require "mongoid/errors/in_memory_collation_not_supported"
 require "mongoid/errors/invalid_async_query_executor"
 require "mongoid/errors/invalid_collection"

lib/mongoid/errors/immutable_attribute.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module Mongoid
+  module Errors
+
+    # This error is raised when attempting the change the value of an
+    # immutable attribute. For example, the _id attribute is immutable,
+    # and attempting to change it on a document that has already been
+    # persisted will result in this error.
+    class ImmutableAttribute < MongoidError
+
+      # Create the new error.
+      #
+      # @example Create the new error.
+      #   ImmutableAttribute.new(:_id, "1234")
+      #
+      # @param [ Symbol | String ] name The name of the attribute.
+      # @param [ Object ] value The attempted set value.
+      def initialize(name, value)
+        super(
+          compose_message("immutable_attribute", { name: name, value: value })
+        )
+      end
+    end
+  end
+end

lib/mongoid/persistable/updatable.rb

@@ -97,6 +97,7 @@ def init_atomic_updates
       # @return [ true | false ] The result of the update.
       def prepare_update(options = {})
         raise Errors::ReadonlyDocument.new(self.class) if readonly? && !Mongoid.legacy_readonly
+        enforce_immutability_of_id_field!
         return false if performing_validations?(options) &&
           invalid?(options[:context] || :update)
         process_flagged_destroys
@@ -184,6 +185,31 @@ def process_touch_option(options, children)
           children.each(&:timeless)
         end
       end
+
+      # Checks to see if the _id field has been modified. If it has, and if
+      # the document has already been persisted, this is an error. Otherwise,
+      # returns without side-effects.
+      #
+      # Note that if `Mongoid::Config.immutable_ids` is false, this will do
+      # nothing.
+      #
+      # @raise [ Errors::ImmutableAttribute ] if _id has changed, and document
+      #   has been persisted.
+      def enforce_immutability_of_id_field!
+        # special case here: we *do* allow the _id to be mutated if it was
+        # previously nil. This addresses an odd case exposed in
+        # has_one/proxy_spec.rb where `person.create_address` would
+        # (somehow?) create the address with a nil _id first, before then
+        # saving it *again* with the correct _id.
+
+        if _id_changed? && !_id_was.nil? && persisted?
+          if Mongoid::Config.immutable_ids
+            raise Errors::ImmutableAttribute.new(:_id, _id)
+          else
+            Mongoid::Warnings.warn_mutable_ids
+          end
+        end
+      end
     end
   end
 end

lib/mongoid/warnings.rb

@@ -25,5 +25,6 @@ def warning(id, message)
     warning :as_json_compact_deprecated, '#as_json :compact option is deprecated. Please call #compact on the returned Hash object instead.'
     warning :symbol_type_deprecated, 'The BSON Symbol type is deprecated by MongoDB. Please use String or StringifiedSymbol field types instead of the Symbol field type.'
     warning :legacy_readonly, 'The readonly! method will only mark the document readonly when the legacy_readonly feature flag is switched off.'
+    warning :mutable_ids, 'In Mongoid 9.0 the _id field will be immutable. In earlier versions of 8.x, mutating the _id field was supported inconsistently. Prepare your code for 9.0 by setting Mongoid::Config.immutable_ids to true.'
   end
 end

spec/mongoid/association/embedded/embeds_many/proxy_spec.rb

@@ -4566,7 +4566,7 @@ class TrackingIdValidationHistory
     before do
       band.collection.
           find(_id: band.id).
-          update_one("$set" => { records: [{ name: "Moderat" }]})
+          update_one("$set" => { records: [{ _id: BSON::ObjectId.new, name: "Moderat" }]})
     end
 
     context "when loading the documents" do

spec/mongoid/association/embedded/embeds_one/proxy_spec.rb

@@ -971,7 +971,7 @@ class << person
     before do
       band.collection.
           find(_id: band.id).
-          update_one("$set" => { label: { name: "Mute" }})
+          update_one("$set" => { label: { _id: BSON::ObjectId.new, name: "Mute" }})
     end
 
     context "when loading the documents" do

spec/mongoid/changeable_spec.rb

@@ -1349,20 +1349,10 @@
 
       context "when the document is embedded" do
 
-        let(:person) do
-          Person.instantiate(title: "Grand Poobah")
-        end
+        let(:person) { Person.create(title: "Grand Poobah") }
+        let(:address) { person.addresses.create(street: "Oxford St") }
 
-        let(:address) do
-          Address.instantiate(street: "Oxford St")
-        end
-
-        before do
-          person.addresses << address
-          person.instance_variable_set(:@new_record, false)
-          address.instance_variable_set(:@new_record, false)
-          address.street = "Bond St"
-        end
+        before { address.street = "Bond St" }
 
         it "returns a hash of field names and new values" do
           expect(address.setters).to eq(
@@ -1371,17 +1361,9 @@
         end
 
         context "when the document is embedded multiple levels" do
-
-          let(:location) do
-            Location.new(name: "Home")
-          end
-
-          before do
-            location.instance_variable_set(:@new_record, false)
-            address.locations << location
-            location.name = "Work"
-          end
-
+          let(:location) { address.locations.create(name: "Home") }
+          before { location.name = "Work" }
+
           it "returns the proper hash with locations" do
             expect(location.setters).to eq(
               { "addresses.0.locations.0.name" => "Work" }