Merge pull request #1049 from cerebris/raise_errors_earlier

Tightens the error handling on request parsing.

Author: dgeb

lib/jsonapi/request_parser.rb

@@ -281,7 +281,6 @@ def parse_fields(resource_klass, fields)
         fail JSONAPI::Exceptions::InvalidFieldFormat.new(error_object_overrides)
       end
 
-      errors = []
       # Validate the fields
       validated_fields = {}
       extracted_fields.each do |type, values|
@@ -293,31 +292,27 @@ def parse_fields(resource_klass, fields)
           end
           type_resource = Resource.resource_klass_for(resource_klass.module_path + underscored_type.to_s)
         rescue NameError
-          errors.concat(JSONAPI::Exceptions::InvalidResource.new(type, error_object_overrides).errors)
-        rescue JSONAPI::Exceptions::InvalidResource => e
-          errors.concat(e.errors)
+          fail JSONAPI::Exceptions::InvalidResource.new(type, error_object_overrides)
         end
 
         if type_resource.nil?
-          errors.concat(JSONAPI::Exceptions::InvalidResource.new(type, error_object_overrides).errors)
+          fail JSONAPI::Exceptions::InvalidResource.new(type, error_object_overrides)
         else
           unless values.nil?
             valid_fields = type_resource.fields.collect { |key| format_key(key) }
             values.each do |field|
               if valid_fields.include?(field)
                 validated_fields[type].push unformat_key(field)
               else
-                errors.concat(JSONAPI::Exceptions::InvalidField.new(type, field, error_object_overrides).errors)
+                fail JSONAPI::Exceptions::InvalidField.new(type, field, error_object_overrides)
               end
             end
           else
-            errors.concat(JSONAPI::Exceptions::InvalidField.new(type, 'nil', error_object_overrides).errors)
+            fail JSONAPI::Exceptions::InvalidField.new(type, 'nil', error_object_overrides)
           end
         end
       end
 
-      fail JSONAPI::Exceptions::Errors.new(errors) unless errors.empty?
-
       validated_fields.deep_transform_keys { |key| unformat_key(key) }
     end
 
@@ -389,7 +384,7 @@ def parse_filters(resource_klass, filters)
         if resource_klass._allowed_filter?(filter)
           parsed_filters[filter] = value
         else
-          @errors.concat(JSONAPI::Exceptions::FilterNotAllowed.new(filter).errors)
+          fail JSONAPI::Exceptions::FilterNotAllowed.new(filter)
         end
       end
 
@@ -432,8 +427,7 @@ def check_sort_criteria(resource_klass, sort_criteria)
       sort_field = sort_criteria[:field]
 
       unless resource_klass.sortable_field?(sort_field.to_sym, context)
-        @errors.concat(JSONAPI::Exceptions::InvalidSortCriteria
-                           .new(format_key(resource_klass._type), sort_field).errors)
+        fail JSONAPI::Exceptions::InvalidSortCriteria.new(format_key(resource_klass._type), sort_field)
       end
     end
 
@@ -582,16 +576,14 @@ def unformat_value(resource_klass, attribute, value)
     def verify_permitted_params(params, allowed_fields)
       formatted_allowed_fields = allowed_fields.collect { |field| format_key(field).to_sym }
       params_not_allowed = []
-      param_errors = []
 
       params.each do |key, value|
         case key.to_s
           when 'relationships'
             value.keys.each do |links_key|
               unless formatted_allowed_fields.include?(links_key.to_sym)
                 if JSONAPI.configuration.raise_if_parameters_not_allowed
-                  param_errors.concat JSONAPI::Exceptions::ParameterNotAllowed.new(
-                      links_key, error_object_overrides).errors
+                  fail JSONAPI::Exceptions::ParameterNotAllowed.new(links_key, error_object_overrides)
                 else
                   params_not_allowed.push(links_key)
                   value.delete links_key
@@ -602,8 +594,7 @@ def verify_permitted_params(params, allowed_fields)
             value.each do |attr_key, _attr_value|
               unless formatted_allowed_fields.include?(attr_key.to_sym)
                 if JSONAPI.configuration.raise_if_parameters_not_allowed
-                  param_errors.concat JSONAPI::Exceptions::ParameterNotAllowed.new(
-                      attr_key, error_object_overrides).errors
+                  fail JSONAPI::Exceptions::ParameterNotAllowed.new(attr_key, error_object_overrides)
                 else
                   params_not_allowed.push(attr_key)
                   value.delete attr_key
@@ -614,27 +605,23 @@ def verify_permitted_params(params, allowed_fields)
           when 'id'
             unless formatted_allowed_fields.include?(:id)
               if JSONAPI.configuration.raise_if_parameters_not_allowed
-                param_errors.concat JSONAPI::Exceptions::ParameterNotAllowed.new(
-                    :id, error_object_overrides).errors
+                fail JSONAPI::Exceptions::ParameterNotAllowed.new(:id, error_object_overrides)
               else
                 params_not_allowed.push(:id)
                 params.delete :id
               end
             end
           else
             if JSONAPI.configuration.raise_if_parameters_not_allowed
-              param_errors += JSONAPI::Exceptions::ParameterNotAllowed.new(
-                  key, error_object_overrides).errors
+              fail JSONAPI::Exceptions::ParameterNotAllowed.new(key, error_object_overrides)
             else
               params_not_allowed.push(key)
               params.delete key
             end
         end
       end
 
-      if param_errors.length > 0
-        fail JSONAPI::Exceptions::Errors.new(param_errors)
-      elsif params_not_allowed.length > 0
+      if params_not_allowed.length > 0
         params_not_allowed_warnings = params_not_allowed.map do |param|
           JSONAPI::Warning.new(code: JSONAPI::PARAM_NOT_ALLOWED,
                                title: 'Param not allowed',

test/controllers/controller_test.rb

@@ -1818,7 +1818,6 @@ def test_update_unpermitted_attributes
       }
 
     assert_response :bad_request
-    assert_match /author is not allowed./, response.body
     assert_match /subject is not allowed./, response.body
   end
 

test/unit/jsonapi_request/jsonapi_request_test.rb

@@ -153,7 +153,7 @@ def test_parse_dasherized_with_underscored_fields
       }
     )
 
-    e = assert_raises JSONAPI::Exceptions::Errors do
+    e = assert_raises JSONAPI::Exceptions::InvalidField do
       request.parse_fields(ExpenseEntryResource, params[:fields])
     end
     refute e.errors.empty?
@@ -178,7 +178,7 @@ def test_parse_dasherized_with_underscored_resource
         key_formatter: JSONAPI::Formatter.formatter_for(:dasherized_key)
       }
     )
-    e = assert_raises JSONAPI::Exceptions::Errors do
+    e = assert_raises JSONAPI::Exceptions::InvalidResource do
       request.parse_fields(ExpenseEntryResource, params[:fields])
     end
     refute e.errors.empty?
@@ -194,10 +194,10 @@ def test_parse_filters_with_valid_filters
 
   def test_parse_filters_with_non_valid_filter
     setup_request
-    filters = @request.parse_filters(CatResource, {breed: 'Whiskers'}) # breed is not a set filter
-    assert_equal(filters, {})
-    assert_equal(@request.errors.count, 1)
-    assert_equal(@request.errors.first.title, "Filter not allowed")
+    e = assert_raises JSONAPI::Exceptions::FilterNotAllowed do
+        @request.parse_filters(CatResource, {breed: 'Whiskers'}) # breed is not a set filter
+    end
+    assert_equal 'breed is not allowed.', e.errors[0].detail
   end
 
   def test_parse_filters_with_no_filters
@@ -224,11 +224,10 @@ def test_parse_sort_with_valid_sorts
 
   def test_parse_sort_with_resource_validated_sorts
     setup_request
-    sort_criteria = @request.parse_sort_criteria(TreeResource, "sort66,name")
-    assert_equal(@request.errors.count, 1)
-    assert_equal(@request.errors.first.title, "Invalid sort criteria")
-    assert_equal(@request.errors.first.detail, "name is not a valid sort criteria for trees")
-    assert_equal(sort_criteria, [{:field=>"sort66", :direction=>:asc}, {:field=>"name", :direction=>:asc}])
+    e = assert_raises JSONAPI::Exceptions::InvalidSortCriteria do
+      @request.parse_sort_criteria(TreeResource, "sort66,name")
+    end
+    assert_equal 'name is not a valid sort criteria for trees', e.errors[0].detail
   end
 
   def test_parse_sort_with_relationships