Merge pull request #19686 from tsun1215/index_errors

Errors can be indexed with nested attributes

Close #8638

8 files changed, 95 insertions, 4 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 64532e90c7..9961bfc99c 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,31 @@
+*   Add option to index errors in nested attributes
+
+    For models which have nested attributes, errors within those models will
+    now be indexed if :index_errors is specified when defining a
+    has_many relationship, or if its set in the global config.
+
+    E.X.
+
+    ```ruby
+    class Guitar < ActiveRecord::Base
+      has_many :tuning_pegs
+      accepts_nested_attributes_for :tuning_pegs
+    end
+
+    class TuningPeg < ActiveRecord::Base
+      belongs_to :guitar
+      validates_numericality_of :pitch
+    end
+    ```
+
+     - Old style
+     - `guitar.errors["tuning_pegs.pitch"] = ["is not a number"]`
+
+     - New style (if defined globally, or set in has_many_relationship)
+     - `guitar.errors["tuning_pegs[1].pitch"] = ["is not a number"]`
+
+    *Michael Probber and Terence Sun*
+
 *   Exit with non-zero status for failed database rake tasks.
 
     *Jay Hayes*
diff --git a/activerecord/lib/active_record/associations/builder/has_many.rb b/activerecord/lib/active_record/associations/builder/has_many.rb
index f71b10120f..7864d4c536 100644
--- a/activerecord/lib/active_record/associations/builder/has_many.rb
+++ b/activerecord/lib/active_record/associations/builder/has_many.rb
@@ -5,7 +5,7 @@ module ActiveRecord::Associations::Builder # :nodoc:
     end
 
     def self.valid_options(options)
-      super + [:primary_key, :dependent, :as, :through, :source, :source_type, :inverse_of, :counter_cache, :join_table, :foreign_type]
+      super + [:primary_key, :dependent, :as, :through, :source, :source_type, :inverse_of, :counter_cache, :join_table, :foreign_type, :index_errors]
     end
 
     def self.valid_dependent_options
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index d35bc3e794..fc12c3f45a 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -140,6 +140,8 @@ module ActiveRecord
 
     included do
       Associations::Builder::Association.extensions << AssociationBuilderExtension
+      mattr_accessor :index_nested_attribute_errors, instance_writer: false
+      self.index_nested_attribute_errors = false
     end
 
     module ClassMethods # :nodoc:
@@ -315,7 +317,7 @@ module ActiveRecord
       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 { |record| association_valid?(reflection, record) }
+            records.each_with_index { |record, index| association_valid?(reflection, record, index) }
           end
         end
       end
@@ -323,14 +325,18 @@ module ActiveRecord
       # 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)
+      def association_valid?(reflection, record, index=nil)
         return true if record.destroyed? || (reflection.options[:autosave] && record.marked_for_destruction?)
 
         validation_context = self.validation_context unless [:create, :update].include?(self.validation_context)
         unless valid = record.valid?(validation_context)
           if reflection.options[:autosave]
             record.errors.each do |attribute, message|
-              attribute = "#{reflection.name}.#{attribute}"
+              if index.nil? || (!reflection.options[:index_errors] && !ActiveRecord::Base.index_nested_attribute_errors)
+                attribute = "#{reflection.name}.#{attribute}"
+              else
+                attribute = "#{reflection.name}[#{index}].#{attribute}"
+              end
               errors[attribute] << message
               errors[attribute].uniq!
             end
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index 37ec3f1106..0df8f1f798 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -24,6 +24,8 @@ require 'models/molecule'
 require 'models/member'
 require 'models/member_detail'
 require 'models/organization'
+require 'models/guitar'
+require 'models/tuning_peg'
 
 class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase
   def test_autosave_validation
@@ -397,6 +399,40 @@ class TestDefaultAutosaveAssociationOnAHasManyAssociationWithAcceptsNestedAttrib
     assert_not molecule.persisted?, 'Molecule should not be persisted when its electrons are invalid'
   end
 
+  def test_errors_should_be_indexed_when_passed_as_array
+    guitar = Guitar.new
+    tuning_peg_valid = TuningPeg.new
+    tuning_peg_valid.pitch = 440.0
+    tuning_peg_invalid = TuningPeg.new
+
+    guitar.tuning_pegs = [tuning_peg_valid, tuning_peg_invalid]
+
+    assert_not tuning_peg_invalid.valid?
+    assert tuning_peg_valid.valid?
+    assert_not guitar.valid?
+    assert_equal ["is not a number"], guitar.errors["tuning_pegs[1].pitch"]
+    assert_not_equal ["is not a number"], guitar.errors["tuning_pegs.pitch"]
+  end
+
+  def test_errors_should_be_indexed_when_global_flag_is_set
+    old_attribute_config = ActiveRecord::Base.index_nested_attribute_errors
+    ActiveRecord::Base.index_nested_attribute_errors = true
+
+    molecule = Molecule.new
+    valid_electron = Electron.new(name: 'electron')
+    invalid_electron = Electron.new
+
+    molecule.electrons = [valid_electron, invalid_electron]
+
+    assert_not invalid_electron.valid?
+    assert valid_electron.valid?
+    assert_not molecule.valid?
+    assert_equal ["can't be blank"], molecule.errors["electrons[1].name"]
+    assert_not_equal ["can't be blank"], molecule.errors["electrons.name"]
+  ensure
+    ActiveRecord::Base.index_nested_attribute_errors = old_attribute_config
+  end
+
   def test_valid_adding_with_nested_attributes
     molecule = Molecule.new
     valid_electron = Electron.new(name: 'electron')
diff --git a/activerecord/test/models/guitar.rb b/activerecord/test/models/guitar.rb
new file mode 100644
index 0000000000..cd068ff53d
--- /dev/null
+++ b/activerecord/test/models/guitar.rb
@@ -0,0 +1,4 @@
+class Guitar < ActiveRecord::Base
+  has_many :tuning_pegs, index_errors: true
+  accepts_nested_attributes_for :tuning_pegs
+end
diff --git a/activerecord/test/models/tuning_peg.rb b/activerecord/test/models/tuning_peg.rb
new file mode 100644
index 0000000000..1252d6dc1d
--- /dev/null
+++ b/activerecord/test/models/tuning_peg.rb
@@ -0,0 +1,4 @@
+class TuningPeg < ActiveRecord::Base
+  belongs_to :guitar
+  validates_numericality_of :pitch
+end
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 690ab1af4b..ca1757acd0 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -348,6 +348,10 @@ ActiveRecord::Schema.define do
     t.column :key, :string
   end
 
+  create_table :guitar, force: true do |t|
+    t.string :color
+  end
+
   create_table :inept_wizards, force: true do |t|
     t.column :name, :string, null: false
     t.column :city, :string, null: false
@@ -851,6 +855,11 @@ ActiveRecord::Schema.define do
     t.belongs_to :ship
   end
 
+  create_table :tuning_pegs, force: true do |t|
+    t.integer :guitar_id
+    t.float :pitch
+  end
+
   create_table :tyres, force: true do |t|
     t.integer :car_id
   end
diff --git a/guides/source/configuring.md b/guides/source/configuring.md
index e2206667e8..e2125cae2e 100644
--- a/guides/source/configuring.md
+++ b/guides/source/configuring.md
@@ -313,6 +313,10 @@ All these configuration options are delegated to the `I18n` library.
   by a query exceeds the threshold, a warning is logged. This can be used to
   identify queries which might be causing memory bloat.
 
+* `config.active_record.index_nested_attribute_errors` allows errors for nested
+  has_many relationships to be displayed with an index as well as the error.
+  Defaults to false.
+
 The MySQL adapter adds one additional configuration option:
 
 * `ActiveRecord::ConnectionAdapters::MysqlAdapter.emulate_booleans` controls whether Active Record will consider all `tinyint(1)` columns in a MySQL database to be booleans and is true by default.