Merge pull request #48727 from lulalala/24390-fix-3

Fix index_errors and provide :nested_attributes_order mode

activerecord/CHANGELOG.md

@@ -1,3 +1,13 @@
+*   Fix `index_errors` having incorrect index in association validation errors.
+
+    *lulalala*
+
+*   Add `index_errors: :nested_attributes_order` mode.
+
+    This indexes the association validation errors based on the order received by nested attributes setter, and respects the `reject_if` configuration. This enables API to provide enough information to the frontend to map the validation errors back to their respective form fields.
+
+    *lulalala*
+
 *   Association option `query_constraints` is deprecated in favor of `foreign_key`.
 
     *Nikita Vasilevsky*

activerecord/lib/active_record/associations.rb

@@ -1506,6 +1506,13 @@ module ClassMethods
         #   Serves as a composite foreign key. Defines the list of columns to be used to query the associated object.
         #   This is an optional option. By default Rails will attempt to derive the value automatically.
         #   When the value is set the Array size must match associated model's primary key or +query_constraints+ size.
+        # [:index_errors]
+        #   Allows differentiation of multiple validation errors from the association records, by including
+        #   an index in the error attribute name, e.g. `roles[2].level`.
+        #   When set to +true+, the index is based on association order, i.e. database order, with yet to be
+        #   persisted new records placed at the end.
+        #   When set to +:nested_attributes_order+, the index is based on the record order received by
+        #   nested attributes setter, when accepts_nested_attributes_for is used.
         #
         # Option examples:
         #   has_many :comments, -> { order("posted_on") }
@@ -1519,6 +1526,7 @@ module ClassMethods
         #   has_many :subscribers, through: :subscriptions, disable_joins: true
         #   has_many :comments, strict_loading: true
         #   has_many :comments, query_constraints: [:blog_id, :post_id]
+        #   has_many :comments, index_errors: :nested_attributes_order
         def has_many(name, scope = nil, **options, &extension)
           reflection = Builder::HasMany.build(self, name, scope, options, &extension)
           Reflection.add_reflection self, name, reflection

activerecord/lib/active_record/associations/collection_association.rb

@@ -28,6 +28,8 @@ module Associations
     # If you need to work on all current children, new and existing records,
     # +load_target+ and the +loaded+ flag are your friends.
     class CollectionAssociation < Association # :nodoc:
+      attr_accessor :nested_attributes_target
+
       # Implements the reader method, e.g. foo.items for Foo.has_many :items
       def reader
         ensure_klass_exists!

activerecord/lib/active_record/associations/nested_error.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+# Validation error class to wrap association records' errors,
+# with index_errors support.
+module ActiveRecord
+  module Associations
+    class NestedError < ::ActiveModel::NestedError
+      def initialize(association, inner_error)
+        @base = association.owner
+        @association = association
+        @inner_error = inner_error
+        super(@base, inner_error, { attribute: compute_attribute(inner_error) })
+      end
+
+      private
+        attr_reader :association
+
+        def compute_attribute(inner_error)
+          association_name = association.reflection.name
+
+          if index_errors_setting && index
+            "#{association_name}[#{index}].#{inner_error.attribute}".to_sym
+          else
+            "#{association_name}.#{inner_error.attribute}".to_sym
+          end
+        end
+
+        def index_errors_setting
+          @index_errors_setting ||=
+            association.options.fetch(:index_errors, ActiveRecord.index_nested_attribute_errors)
+        end
+
+        def index
+          @index ||= ordered_records&.find_index(inner_error.base)
+        end
+
+        def ordered_records
+          case index_errors_setting
+          when true # default is association order
+            association.target
+          when :nested_attributes_order
+            association.nested_attributes_target
+          end
+        end
+    end
+  end
+end

activerecord/lib/active_record/autosave_association.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "active_record/associations/nested_error"
+
 module ActiveRecord
   # = Active Record Autosave Association
   #
@@ -315,7 +317,7 @@ def nested_records_changed_for_autosave?
       def validate_single_association(reflection)
         association = association_instance_get(reflection.name)
         record      = association && association.reader
-        association_valid?(reflection, record) if record && (record.changed_for_autosave? || custom_validation_context?)
+        association_valid?(association, record) if record && (record.changed_for_autosave? || custom_validation_context?)
       end
 
       # Validate the associated records if <tt>:validate</tt> or
@@ -324,48 +326,33 @@ def validate_single_association(reflection)
       def validate_collection_association(reflection)
         if association = association_instance_get(reflection.name)
           if records = associated_records_to_validate_or_save(association, new_record?, reflection.options[:autosave])
-            records.each_with_index { |record, index| association_valid?(reflection, record, index) }
+            records.each { |record| association_valid?(association, record) }
           end
         end
       end
 
       # Returns whether or not the association is valid and applies any errors to
       # the parent, <tt>self</tt>, if it wasn't. Skips any <tt>:autosave</tt>
       # enabled records if they're marked_for_destruction? or destroyed.
-      def association_valid?(reflection, record, index = nil)
-        return true if record.destroyed? || (reflection.options[:autosave] && record.marked_for_destruction?)
+      def association_valid?(association, record)
+        return true if record.destroyed? || (association.options[:autosave] && record.marked_for_destruction?)
 
         context = validation_context if custom_validation_context?
 
         unless valid = record.valid?(context)
-          if reflection.options[:autosave]
-            indexed_attribute = !index.nil? && (reflection.options[:index_errors] || ActiveRecord.index_nested_attribute_errors)
-
-            record.errors.group_by_attribute.each { |attribute, errors|
-              attribute = normalize_reflection_attribute(indexed_attribute, reflection, index, attribute)
-
-              errors.each { |error|
-                self.errors.import(
-                  error,
-                  attribute: attribute
-                )
-              }
+          if association.options[:autosave]
+            record.errors.each { |error|
+              self.errors.objects.append(
+                Associations::NestedError.new(association, error)
+              )
             }
           else
-            errors.add(reflection.name)
+            errors.add(association.reflection.name)
           end
         end
         valid
       end
 
-      def normalize_reflection_attribute(indexed_attribute, reflection, index, attribute)
-        if indexed_attribute
-          "#{reflection.name}[#{index}].#{attribute}"
-        else
-          "#{reflection.name}.#{attribute}"
-        end
-      end
-
       # Is used as an around_save callback to check while saving a collection
       # association whether or not the parent was a new record before saving.
       def around_save_collection_association

activerecord/lib/active_record/nested_attributes.rb

@@ -514,7 +514,7 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
           attribute_ids.empty? ? [] : association.scope.where(association.klass.primary_key => attribute_ids)
         end
 
-        attributes_collection.each do |attributes|
+        records = attributes_collection.map do |attributes|
           if attributes.respond_to?(:permitted?)
             attributes = attributes.to_h
           end
@@ -537,11 +537,14 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
               end
 
               assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
+              existing_record
             end
           else
             raise_nested_attributes_record_not_found!(association_name, attributes["id"])
           end
         end
+
+        association.nested_attributes_target = records
       end
 
       # Takes in a limit and checks if the attributes_collection has too many

activerecord/test/cases/associations/nested_error_test.rb

@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+require "cases/helper"
+require "models/guitar"
+require "models/tuning_peg"
+
+class AssociationsNestedErrorInAssociationOrderTest < ActiveRecord::TestCase
+  test "index in association order" do
+    guitar = Guitar.create!
+    guitar.tuning_pegs.create!(pitch: 1)
+    peg2 = guitar.tuning_pegs.create!(pitch: 2)
+    peg2.pitch = nil
+    guitar.valid?
+
+    error = guitar.errors.objects.first
+
+    assert_equal ActiveRecord::Associations::NestedError, error.class
+    assert_equal peg2.errors.objects.first, error.inner_error
+    assert_equal :'tuning_pegs[1].pitch', error.attribute
+    assert_equal :not_a_number, error.type
+    assert_equal "is not a number", error.message
+    assert_equal guitar, error.base
+  end
+end
+
+class AssociationsNestedErrorInNestedAttributesOrderTest < ActiveRecord::TestCase
+  def setup
+    tuning_peg_class = Class.new(ActiveRecord::Base) do
+      self.table_name = "tuning_pegs"
+      def self.name; "TuningPeg"; end
+
+      validates_numericality_of :pitch
+    end
+
+    @guitar_class = Class.new(ActiveRecord::Base) do
+      has_many :tuning_pegs, index_errors: :nested_attributes_order, anonymous_class: tuning_peg_class
+      accepts_nested_attributes_for :tuning_pegs, reject_if: lambda { |attrs| attrs[:pitch]&.odd? }
+
+      def self.name; "Guitar"; end
+    end
+  end
+
+  test "index in nested attributes order" do
+    guitar = @guitar_class.create!
+    guitar.tuning_pegs.create!(pitch: 1)
+    peg2 = guitar.tuning_pegs.create!(pitch: 2)
+    guitar.update(tuning_pegs_attributes: [{ id: peg2.id, pitch: nil }])
+
+    error = guitar.errors.objects.first
+
+    assert_equal ActiveRecord::Associations::NestedError, error.class
+    assert_equal peg2.errors.objects.first, error.inner_error
+    assert_equal :'tuning_pegs[0].pitch', error.attribute
+    assert_equal :not_a_number, error.type
+    assert_equal "is not a number", error.message
+    assert_equal guitar, error.base
+  end
+
+  test "index unaffected by reject_if" do
+    guitar = @guitar_class.create!
+
+    guitar.update(
+      tuning_pegs_attributes: [
+        { pitch: 1 }, # rejected
+        { pitch: nil },
+      ]
+    )
+
+    error = guitar.errors.objects.first
+
+    assert_equal ActiveRecord::Associations::NestedError, error.class
+    assert_equal :'tuning_pegs[1].pitch', error.attribute
+    assert_equal :not_a_number, error.type
+    assert_equal "is not a number", error.message
+    assert_equal guitar, error.base
+  end
+end

activerecord/test/cases/nested_attributes_test.rb

@@ -868,6 +868,21 @@ def test_numeric_column_changes_from_zero_to_no_empty_string
     end
   end
 
+  def test_assigning_nested_attributes_target
+    params = @alternate_params[association_getter].values
+    @pirate.public_send(association_setter, params)
+    @pirate.save
+    assert_equal [@child_1, @child_2], @pirate.association(@association_name).nested_attributes_target
+  end
+
+  def test_assigning_nested_attributes_target_with_nil_placeholder_for_rejected_item
+    params = @alternate_params[association_getter].values
+    params.insert(1, {})
+    @pirate.public_send(association_setter, params)
+    @pirate.save
+    assert_equal [@child_1, nil, @child_2], @pirate.association(@association_name).nested_attributes_target
+  end
+
   private
     def association_setter
       @association_setter ||= "#{@association_name}_attributes=".to_sym
@@ -1124,6 +1139,51 @@ def setup
   end
 end
 
+class TestIndexErrorsWithNestedAttributesOnlyMode < ActiveRecord::TestCase
+  def setup
+    tuning_peg_class = Class.new(ActiveRecord::Base) do
+      self.table_name = "tuning_pegs"
+      def self.name; "TuningPeg"; end
+
+      validates_numericality_of :pitch
+    end
+
+    @guitar_class = Class.new(ActiveRecord::Base) do
+      has_many :tuning_pegs, index_errors: :nested_attributes_order, anonymous_class: tuning_peg_class
+      accepts_nested_attributes_for :tuning_pegs, reject_if: lambda { |attrs| attrs[:pitch]&.odd? }
+
+      def self.name; "Guitar"; end
+    end
+  end
+
+  test "index in nested_attributes_order order" do
+    guitar = @guitar_class.create!
+    guitar.tuning_pegs.create!(pitch: 1)
+    peg2 = guitar.tuning_pegs.create!(pitch: 2)
+
+    assert_predicate guitar, :valid?
+
+    guitar.update(tuning_pegs_attributes: [{ id: peg2.id, pitch: nil }])
+
+    assert_not_predicate guitar, :valid?
+    assert_equal [:"tuning_pegs[0].pitch"], guitar.errors.messages.keys
+  end
+
+  test "index unaffected by reject_if" do
+    guitar = @guitar_class.create!
+
+    guitar.update(
+      tuning_pegs_attributes: [
+        { pitch: 1 },
+        { pitch: nil },
+      ]
+    )
+
+    assert_not_predicate guitar, :valid?
+    assert_equal [:"tuning_pegs[1].pitch"], guitar.errors.messages.keys
+  end
+end
+
 class TestNestedAttributesWithExtend < ActiveRecord::TestCase
   setup do
     Pirate.accepts_nested_attributes_for :treasures