MONGOID-5228 disallow _id to be updated on persisted documents (#5542)

* MONGOID-5228 - disallow _id to be updated on persisted documents

Previously, updating _id on any document (top-level or embedded) would silently ignore the _id attribute, making it appear as though the operation succeeded. This was misleading, since the _id didn't actually change. This change makes it so that _id is explicitly made immutable once a document has been persisted. The same behavior is intentionally enforced regardless of whether the document is embedded or not. If you find yourself needing to update the _id field in an embedded document, you'll need to use the lower-level driver methods to accomplish this. We believe modifying the _id field (even in embedded documents) is an anti-pattern.

* use a specialized error for immutability violations

* add a feature flag

* change the flag name, and account for existing nested attribute behavior

* wrong default for <9.0

* clean up a test that was unnecessarily sensitive to the _id changing

* another test that was unnecessarily sensitive to _id changes

* more tests that are too sensitive to _id changes

* we need to allow _id mutations if the _id was previously nil

* mention enforcement of _id immutability in the release notes

Author: jamis

docs/release-notes/mongoid-9.0.txt

@@ -204,6 +204,25 @@ This means that, by default, Mongoid 9 will update the existing document and
 will not replace it.
 
 
+The immutability of the ``_id`` field is now enforced
+-----------------------------------------------------
+
+Prior to Mongoid 9.0, mutating the ``_id`` field behaved inconsistently
+depending on whether the document was top-level or embedded, and depending on
+how the update was performed. As of 9.0, changing the ``_id`` field will now
+raise an exception when the document is saved, if the document had been
+previously persisted.
+
+Mongoid 9.0 also introduces a new feature flag, ``immutable_ids``, which
+defaults to ``true``.
+
+.. code-block:: ruby
+
+  Mongoid::Config.immutable_ids = true
+
+When set to false, the older, inconsistent behavior is restored.
+
+
 Bug Fixes and Improvements
 --------------------------
 

lib/config/locales/en.yml

@@ -119,6 +119,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

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

lib/mongoid/config/defaults.rb

@@ -46,11 +46,16 @@ def load_defaults(version)
         when "7.5"
           # flags introduced in 8.0 - old functionality
           self.map_big_decimal_to_decimal128 = false
+          
+          load_defaults "8.0"
         when "8.0"
-          # All flag defaults currently reflect 8.0 behavior.
           self.legacy_readonly = true
+
+          load_defaults "8.1"
         when "8.1"
-          # All flag defaults currently reflect 8.1 behavior.
+          self.immutable_ids = false
+
+          load_defaults "9.0"
         when "9.0"
           # All flag defaults currently reflect 9.0 behavior.
         else

lib/mongoid/errors.rb

@@ -9,6 +9,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
@@ -186,6 +187,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, 'Ignoring updates to immutable attribute `_id`. Please set Mongoid::Config.immutable_ids to true and update your code so that `_id` is never updated.'
   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