Merge pull request #32313 from lulalala/model_error_as_object

Model error as object

5 files changed, 34 insertions, 35 deletions

diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index 50f29a81a6..8d89e7d84a 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -330,21 +330,16 @@ module ActiveRecord
           if reflection.options[:autosave]
             indexed_attribute = !index.nil? && (reflection.options[:index_errors] || ActiveRecord::Base.index_nested_attribute_errors)
 
-            record.errors.each do |attribute, message|
+            record.errors.group_by_attribute.each { |attribute, errors|
               attribute = normalize_reflection_attribute(indexed_attribute, reflection, index, attribute)
-              errors[attribute] << message
-              errors[attribute].uniq!
-            end
-
-            record.errors.details.each_key do |attribute|
-              reflection_attribute =
-                normalize_reflection_attribute(indexed_attribute, reflection, index, attribute).to_sym
 
-              record.errors.details[attribute].each do |error|
-                errors.details[reflection_attribute] << error
-                errors.details[reflection_attribute].uniq!
-              end
-            end
+              errors.each { |error|
+                self.errors.import(
+                  error,
+                  attribute: attribute
+                )
+              }
+            }
           else
             errors.add(reflection.name)
           end
@@ -500,9 +495,7 @@ module ActiveRecord
       end
 
       def _ensure_no_duplicate_errors
-        errors.messages.each_key do |attribute|
-          errors[attribute].uniq!
-        end
+        errors.uniq!
       end
   end
 end
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index c13789f7ec..0ab99aa6cd 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -749,10 +749,14 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
 
       firm = companies(:first_firm)
       lifo = Developer.new(name: "lifo")
-      assert_raises(ActiveRecord::RecordInvalid) { firm.developers << lifo }
+      assert_raises(ActiveRecord::RecordInvalid) do
+        assert_deprecated { firm.developers << lifo }
+      end
 
       lifo = Developer.create!(name: "lifo")
-      assert_raises(ActiveRecord::RecordInvalid) { firm.developers << lifo }
+      assert_raises(ActiveRecord::RecordInvalid) do
+        assert_deprecated { firm.developers << lifo }
+      end
     end
   end
 
@@ -1163,7 +1167,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
   def test_create_should_not_raise_exception_when_join_record_has_errors
     repair_validations(Categorization) do
       Categorization.validate { |r| r.errors[:base] << "Invalid Categorization" }
-      Category.create(name: "Fishing", authors: [Author.first])
+      assert_deprecated { Category.create(name: "Fishing", authors: [Author.first]) }
     end
   end
 
@@ -1176,7 +1180,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     repair_validations(Categorization) do
       Categorization.validate { |r| r.errors[:base] << "Invalid Categorization" }
       assert_raises(ActiveRecord::RecordInvalid) do
-        Category.create!(name: "Fishing", authors: [Author.first])
+        assert_deprecated { Category.create!(name: "Fishing", authors: [Author.first]) }
       end
     end
   end
@@ -1186,7 +1190,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
       Categorization.validate { |r| r.errors[:base] << "Invalid Categorization" }
       c = Category.new(name: "Fishing", authors: [Author.first])
       assert_raises(ActiveRecord::RecordInvalid) do
-        c.save!
+        assert_deprecated { c.save! }
       end
     end
   end
@@ -1195,7 +1199,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     repair_validations(Categorization) do
       Categorization.validate { |r| r.errors[:base] << "Invalid Categorization" }
       c = Category.new(name: "Fishing", authors: [Author.first])
-      assert_not c.save
+      assert_deprecated { assert_not c.save }
     end
   end
 
diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb
index d5057ad381..7b7aa7e9b7 100644
--- a/activerecord/test/cases/persistence_test.rb
+++ b/activerecord/test/cases/persistence_test.rb
@@ -183,7 +183,7 @@ class PersistenceTest < ActiveRecord::TestCase
     assert_not_predicate company, :valid?
     original_errors = company.errors
     client = company.becomes(Client)
-    assert_equal original_errors.keys, client.errors.keys
+    assert_equal assert_deprecated { original_errors.keys }, assert_deprecated { client.errors.keys }
   end
 
   def test_becomes_errors_base
@@ -197,7 +197,7 @@ class PersistenceTest < ActiveRecord::TestCase
     admin.errors.add :token, :invalid
     child = admin.becomes(child_class)
 
-    assert_equal [:token], child.errors.keys
+    assert_equal [:token], assert_deprecated { child.errors.keys }
     assert_nothing_raised do
       child.errors.add :foo, :invalid
     end
diff --git a/activerecord/test/cases/validations/i18n_validation_test.rb b/activerecord/test/cases/validations/i18n_validation_test.rb
index b7c52ea18c..2645776ab7 100644
--- a/activerecord/test/cases/validations/i18n_validation_test.rb
+++ b/activerecord/test/cases/validations/i18n_validation_test.rb
@@ -40,11 +40,11 @@ class I18nValidationTest < ActiveRecord::TestCase
   COMMON_CASES = [
     # [ case,                              validation_options,            generate_message_options]
     [ "given no options",                  {},                            {}],
-    [ "given custom message",              { message: "custom" },        { message: "custom" }],
-    [ "given if condition",                { if: lambda { true } },  {}],
-    [ "given unless condition",            { unless: lambda { false } }, {}],
-    [ "given option that is not reserved", { format: "jpg" },            { format: "jpg" }],
-    [ "given on condition",                { on: [:create, :update] },     {}]
+    [ "given custom message",              { message: "custom" },         { message: "custom" }],
+    [ "given if condition",                { if: lambda { true } },       {}],
+    [ "given unless condition",            { unless: lambda { false } },  {}],
+    [ "given option that is not reserved", { format: "jpg" },             { format: "jpg" }],
+    [ "given on condition",                { on: [:create, :update] },    {}]
   ]
 
   COMMON_CASES.each do |name, validation_options, generate_message_options|
@@ -53,6 +53,7 @@ class I18nValidationTest < ActiveRecord::TestCase
       @topic.title = unique_topic.title
       assert_called_with(@topic.errors, :generate_message, [:title, :taken, generate_message_options.merge(value: "unique!")]) do
         @topic.valid?
+        @topic.errors.messages
       end
     end
   end
@@ -62,6 +63,7 @@ class I18nValidationTest < ActiveRecord::TestCase
       Topic.validates_associated :replies, validation_options
       assert_called_with(replied_topic.errors, :generate_message, [:replies, :invalid, generate_message_options.merge(value: replied_topic.replies)]) do
         replied_topic.save
+        replied_topic.errors.messages
       end
     end
   end
diff --git a/activerecord/test/models/reply.rb b/activerecord/test/models/reply.rb
index b35623a344..f6ab9c8a8f 100644
--- a/activerecord/test/models/reply.rb
+++ b/activerecord/test/models/reply.rb
@@ -34,29 +34,29 @@ class WrongReply < Reply
   validate :check_author_name_is_secret, on: :special_case
 
   def check_empty_title
-    errors[:title] << "Empty" unless attribute_present?("title")
+    errors.add(:title, "Empty") unless attribute_present?("title")
   end
 
   def errors_on_empty_content
-    errors[:content] << "Empty" unless attribute_present?("content")
+    errors.add(:content, "Empty") unless attribute_present?("content")
   end
 
   def check_content_mismatch
     if attribute_present?("title") && attribute_present?("content") && content == "Mismatch"
-      errors[:title] << "is Content Mismatch"
+      errors.add(:title, "is Content Mismatch")
     end
   end
 
   def title_is_wrong_create
-    errors[:title] << "is Wrong Create" if attribute_present?("title") && title == "Wrong Create"
+    errors.add(:title, "is Wrong Create") if attribute_present?("title") && title == "Wrong Create"
   end
 
   def check_wrong_update
-    errors[:title] << "is Wrong Update" if attribute_present?("title") && title == "Wrong Update"
+    errors.add(:title, "is Wrong Update") if attribute_present?("title") && title == "Wrong Update"
   end
 
   def check_author_name_is_secret
-    errors[:author_name] << "Invalid" unless author_name == "secret"
+    errors.add(:author_name, "Invalid") unless author_name == "secret"
   end
 end