Merge pull request #4881 from al2o3cr/compatible_json_errors

Make ActiveResource error parsing interoperate with ActionController. Closes #4881.

activeresource/lib/active_resource/base.rb

@@ -234,7 +234,7 @@ module ActiveResource
   #   ryan.save  # => false
   #
   #   # When
-  #   # PUT https://api.people.com/people/1.json
+  #   # PUT https://api.people.com/people/1.xml
   #   # or
   #   # PUT https://api.people.com/people/1.json
   #   # is requested with invalid values, the response is:
@@ -242,12 +242,21 @@ module ActiveResource
   #   # Response (422):
   #   # <errors><error>First cannot be empty</error></errors>
   #   # or
-  #   # {"errors":["First cannot be empty"]}
+  #   # {"errors":{"first":["cannot be empty"]}}
   #   #
   #
   #   ryan.errors.invalid?(:first)  # => true
   #   ryan.errors.full_messages     # => ['First cannot be empty']
   #
+  # For backwards-compatibility with older endpoints, the following formats are also supported in JSON responses:
+  #
+  #   # {"errors":['First cannot be empty']}
+  #   #   This was the required format for previous versions of ActiveResource
+  #   # {"first":["cannot be empty"]}
+  #   #   This was the default format produced by respond_with in ActionController <3.2.1
+  #
+  # Parsing either of these formats will result in a deprecation warning.
+  #
   # Learn more about Active Resource's validation features in the ActiveResource::Validations documentation.
   #
   # === Timeouts

activeresource/lib/active_resource/validations.rb

@@ -25,10 +25,48 @@ module ActiveResource
       end
     end
 
+    # Grabs errors from a hash of attribute => array of errors elements
+    # The second parameter directs the errors cache to be cleared (default)
+    # or not (by passing true)
+    #
+    # Unrecognized attribute names will be humanized and added to the record's
+    # base errors.
+    def from_hash(messages, save_cache = false)
+      clear unless save_cache
+
+      messages.each do |(key,errors)|
+        errors.each do |error|
+          if @base.attributes.keys.include?(key)
+            add key, error
+          elsif key == 'base'
+            self[:base] << error
+          else
+            # reporting an error on an attribute not in attributes
+            # format and add them to base
+            self[:base] << "#{key.humanize} #{error}"
+          end
+        end
+      end
+    end
+
     # Grabs errors from a json response.
     def from_json(json, save_cache = false)
-      array = Array.wrap(ActiveSupport::JSON.decode(json)['errors']) rescue []
-      from_array array, save_cache
+      decoded = ActiveSupport::JSON.decode(json) || {} rescue {}
+      if decoded.kind_of?(Hash) && (decoded.has_key?('errors') || decoded.empty?)
+        errors = decoded['errors'] || {}
+        if errors.kind_of?(Array)
+          # 3.2.1-style with array of strings
+          ActiveSupport::Deprecation.warn('Returning errors as an array of strings is deprecated.')
+          from_array errors, save_cache
+        else
+          # 3.2.2+ style
+          from_hash errors, save_cache
+        end
+      else
+        # <3.2-style respond_with - lacks 'errors' key
+        ActiveSupport::Deprecation.warn('Returning errors as a hash without a root "errors" key is deprecated.')
+        from_hash decoded, save_cache
+      end
     end
 
     # Grabs errors from an XML response.

activeresource/test/cases/base_errors_test.rb

@@ -5,7 +5,7 @@ class BaseErrorsTest < ActiveSupport::TestCase
   def setup
     ActiveResource::HttpMock.respond_to do |mock|
       mock.post "/people.xml", {}, %q(<?xml version="1.0" encoding="UTF-8"?><errors><error>Age can't be blank</error><error>Name can't be blank</error><error>Name must start with a letter</error><error>Person quota full for today.</error></errors>), 422, {'Content-Type' => 'application/xml; charset=utf-8'}
-      mock.post "/people.json", {}, %q({"errors":["Age can't be blank","Name can't be blank","Name must start with a letter","Person quota full for today."]}), 422, {'Content-Type' => 'application/json; charset=utf-8'}
+      mock.post "/people.json", {}, %q({"errors":{"age":["can't be blank"],"name":["can't be blank", "must start with a letter"],"person":["quota full for today."]}}), 422, {'Content-Type' => 'application/json; charset=utf-8'}
     end
   end
 
@@ -83,7 +83,7 @@ class BaseErrorsTest < ActiveSupport::TestCase
   def test_should_mark_as_invalid_when_content_type_is_unavailable_in_response_header
     ActiveResource::HttpMock.respond_to do |mock|
       mock.post "/people.xml", {}, %q(<?xml version="1.0" encoding="UTF-8"?><errors><error>Age can't be blank</error><error>Name can't be blank</error><error>Name must start with a letter</error><error>Person quota full for today.</error></errors>), 422, {}
-      mock.post "/people.json", {}, %q({"errors":["Age can't be blank","Name can't be blank","Name must start with a letter","Person quota full for today."]}), 422, {}
+      mock.post "/people.json", {}, %q({"errors":{"age":["can't be blank"],"name":["can't be blank", "must start with a letter"],"person":["quota full for today."]}}), 422, {}
     end
 
     [ :json, :xml ].each do |format|
@@ -93,6 +93,36 @@ class BaseErrorsTest < ActiveSupport::TestCase
     end
   end
 
+  def test_should_parse_json_string_errors_with_an_errors_key
+    ActiveResource::HttpMock.respond_to do |mock|
+      mock.post "/people.json", {}, %q({"errors":["Age can't be blank", "Name can't be blank", "Name must start with a letter", "Person quota full for today."]}), 422, {'Content-Type' => 'application/json; charset=utf-8'}
+    end
+
+    assert_deprecated(/as an array/) do
+      invalid_user_using_format(:json) do
+        assert @person.errors[:name].any?
+        assert_equal ["can't be blank"], @person.errors[:age]
+        assert_equal ["can't be blank", "must start with a letter"], @person.errors[:name]
+        assert_equal ["Person quota full for today."], @person.errors[:base]
+      end
+    end
+  end
+
+  def test_should_parse_3_1_style_json_errors
+    ActiveResource::HttpMock.respond_to do |mock|
+      mock.post "/people.json", {}, %q({"age":["can't be blank"],"name":["can't be blank", "must start with a letter"],"person":["quota full for today."]}), 422, {'Content-Type' => 'application/json; charset=utf-8'}
+    end
+
+    assert_deprecated(/without a root/) do
+      invalid_user_using_format(:json) do
+        assert @person.errors[:name].any?
+        assert_equal ["can't be blank"], @person.errors[:age]
+        assert_equal ["can't be blank", "must start with a letter"], @person.errors[:name]
+        assert_equal ["Person quota full for today."], @person.errors[:base]
+      end
+    end
+  end
+
   private
   def invalid_user_using_format(mime_type_reference)
     previous_format = Person.format