MONGOID-5740 Fix performance regression on belongs_to validation (backport to 8.1-stable)

.github/workflows/test.yml

@@ -204,7 +204,7 @@ jobs:
     - name: test
       timeout-minutes: 60
       continue-on-error: "${{matrix.experimental}}"
-      run: bundle exec rake spec
+      run: bundle exec rake ci
       env:
         BUNDLE_GEMFILE: "${{matrix.gemfile}}"
         MONGODB_URI: "${{ steps.start-mongodb.outputs.cluster-uri }}"

lib/mongoid/validatable.rb

@@ -37,6 +37,14 @@ def exit_validate
       Threaded.exit_validate(self)
     end
 
+    # Perform a validation within the associated block.
+    def validating
+      begin_validate
+      yield
+    ensure
+      exit_validate
+    end
+
     # Given the provided options, are we performing validations?
     #
     # @example Are we performing validations?

lib/mongoid/validatable/associated.rb

@@ -15,32 +15,110 @@ module Validatable
     #
     #     validates_associated :name, :addresses
     #   end
-    class AssociatedValidator < ActiveModel::EachValidator
+    class AssociatedValidator < ActiveModel::Validator
+      # Required by `validates_with` so that the validator
+      # gets added to the correct attributes.
+      def attributes
+        options[:attributes]
+      end
 
-      # Validates that the associations provided are either all nil or all
-      # valid. If neither is true then the appropriate errors will be added to
-      # the parent document.
+      # Checks that the named associations of the given record
+      # (`attributes`) are valid. This does NOT load the associations
+      # from the database, and will only validate records that are dirty
+      # or unpersisted.
       #
-      # @example Validate the association.
-      #   validator.validate_each(document, :name, name)
+      # If anything is not valid, appropriate errors will be added to
+      # the `document` parameter.
+      #
+      # @param [ Mongoid::Document ] document the document with the
+      #   associations to validate.
+      def validate(document)
+        options[:attributes].each do |attr_name|
+          validate_association(document, attr_name)
+        end
+      end
+
+      private
+
+      # Validates that the given association provided is either nil,
+      # persisted and unchanged, or invalid. Otherwise, the appropriate errors
+      # will be added to the parent document.
       #
       # @param [ Document ] document The document to validate.
       # @param [ Symbol ] attribute The association to validate.
-      # @param [ Object ] value The value of the association.
-      def validate_each(document, attribute, value)
-        begin
-          document.begin_validate
-          valid = Array.wrap(value).collect do |doc|
-            if doc.nil? || doc.flagged_for_destroy?
-              true
+      def validate_association(document, attribute)
+        # grab the proxy from the instance variable directly; we don't want
+        # any loading logic to run; we just want to see if it's already
+        # been loaded.
+        proxy = document.ivar(attribute)
+        return unless proxy
+
+        # if the variable exists, now we see if it is a proxy, or an actual
+        # document. It might be a literal document instead of a proxy if this
+        # document was created with a Document instance as a provided attribute,
+        # e.g. "Post.new(message: Message.new)".
+        target = proxy.respond_to?(:_target) ? proxy._target : proxy
+
+        # Now, fetch the list of documents from the target. Target may be a
+        # single value, or a list of values, and in the case of HasMany,
+        # might be a rather complex collection. We need to do this without
+        # triggering a load, so it's a bit of a delicate dance.
+        list = get_target_documents(target)
+
+        valid = document.validating do
+          # Now, treating the target as an array, look at each element
+          # and see if it is valid, but only if it has already been
+          # persisted, or changed, and hasn't been flagged for destroy.
+          list.all? do |value|
+            if value && !value.flagged_for_destroy? && (!value.persisted? || value.changed?)
+              value.validated? ? true : value.valid?
             else
-              doc.validated? ? true : doc.valid?
+              true
             end
-          end.all?
-        ensure
-          document.exit_validate
+          end
+        end
+
+        document.errors.add(attribute, :invalid) unless valid
+      end
+
+      private
+
+      # Examine the given target object and return an array of
+      # documents (possibly empty) that the target represents.
+      #
+      # @param [ Array | Mongoid::Document | Mongoid::Association::Proxy | HasMany::Enumerable ] target
+      #   the target object to examine.
+      #
+      # @return [ Array<Mongoid::Document> ] the list of documents
+      def get_target_documents(target)
+        if target.respond_to?(:_loaded?)
+          get_target_documents_for_has_many(target)
+        else
+          get_target_documents_for_other(target)
         end
-        document.errors.add(attribute, :invalid, **options) unless valid
+      end
+
+      # Returns the list of all currently in-memory values held by
+      # the target. The target will not be loaded.
+      #
+      # @param [ HasMany::Enumerable ] target the target that will
+      #   be examined for in-memory documents.
+      #
+      # @return [ Array<Mongoid::Document> ] the in-memory documents
+      #   held by the target.
+      def get_target_documents_for_has_many(target)
+        [ *target._loaded.values, *target._added.values ]
+      end
+
+      # Returns the target as an array. If the target represents a single
+      # value, it is wrapped in an array.
+      #
+      # @param [ Array | Mongoid::Document | Mongoid::Association::Proxy ] target
+      #   the target to return.
+      #
+      # @return [ Array<Mongoid::Document> ] the target, as an array.
+      def get_target_documents_for_other(target)
+        Array.wrap(target)
       end
     end
   end

spec/mongoid/validatable/associated_spec.rb

@@ -75,7 +75,6 @@
         end
 
         it "does not run validation on them" do
-          expect(description).to receive(:valid?).never
           expect(user).to be_valid
         end
 
@@ -84,22 +83,22 @@
     end
   end
 
-  describe "#validate_each" do
+  describe "#validate" do
 
     let(:person) do
       Person.new
     end
 
     let(:validator) do
-      described_class.new(attributes: person.attributes)
+      described_class.new(attributes: person.relations.keys)
     end
 
     context "when the association is a one to one" do
 
       context "when the association is nil" do
 
         before do
-          validator.validate_each(person, :name, nil)
+          validator.validate(person)
         end
 
         it "adds no errors" do
@@ -108,14 +107,9 @@
       end
 
       context "when the association is valid" do
-
-        let(:associated) do
-          double(valid?: true, flagged_for_destroy?: false)
-        end
-
         before do
-          expect(associated).to receive(:validated?).and_return(false)
-          validator.validate_each(person, :name, associated)
+          person.name = Name.new(first_name: 'A', last_name: 'B')
+          validator.validate(person)
         end
 
         it "adds no errors" do
@@ -125,13 +119,9 @@
 
       context "when the association is invalid" do
 
-        let(:associated) do
-          double(valid?: false, flagged_for_destroy?: false)
-        end
-
         before do
-          expect(associated).to receive(:validated?).and_return(false)
-          validator.validate_each(person, :name, associated)
+          person.name = Name.new(first_name: 'Jamis', last_name: 'Buck')
+          validator.validate(person)
         end
 
         it "adds errors to the parent document" do
@@ -149,7 +139,7 @@
       context "when the association is empty" do
 
         before do
-          validator.validate_each(person, :addresses, [])
+          validator.validate(person)
         end
 
         it "adds no errors" do
@@ -159,13 +149,9 @@
 
       context "when the association has invalid documents" do
 
-        let(:associated) do
-          double(valid?: false, flagged_for_destroy?: false)
-        end
-
         before do
-          expect(associated).to receive(:validated?).and_return(false)
-          validator.validate_each(person, :addresses, [ associated ])
+          person.addresses << Address.new(street: '123')
+          validator.validate(person)
         end
 
         it "adds errors to the parent document" do
@@ -175,13 +161,10 @@
 
       context "when the association has all valid documents" do
 
-        let(:associated) do
-          double(valid?: true, flagged_for_destroy?: false)
-        end
-
         before do
-          expect(associated).to receive(:validated?).and_return(false)
-          validator.validate_each(person, :addresses, [ associated ])
+          person.addresses << Address.new(street: '123 First St')
+          person.addresses << Address.new(street: '456 Second St')
+          validator.validate(person)
         end
 
         it "adds no errors" do

spec/support/models/name.rb

@@ -4,6 +4,8 @@ class Name
   include Mongoid::Document
   include Mongoid::Attributes::Dynamic
 
+  validate :is_not_jamis
+
   field :_id, type: String, overwrite: true, default: ->{
     "#{first_name}-#{last_name}"
   }
@@ -23,4 +25,12 @@ class Name
   def set_parent=(set = false)
     self.parent_title = namable.title if set
   end
+
+  private
+
+  def is_not_jamis
+    if first_name == 'Jamis' && last_name == 'Buck'
+      errors.add(:base, :invalid)
+    end
+  end
 end