Merge pull request #1056 from cerebris/0_9_1_cherrypick

dgeb · web-flow · commit 7bfa7eb5343a · 2017-05-07T07:24:16.000-04:00
Cherrypicks some features
diff --git a/lib/jsonapi/exceptions.rb b/lib/jsonapi/exceptions.rb
@@ -364,24 +364,21 @@ def errors
       end
     end
 
-    class ParametersNotAllowed < Error
-      attr_accessor :params
+    class ParameterNotAllowed < Error
+      attr_accessor :param
 
-      def initialize(params, error_object_overrides = {})
-        @params = params
+      def initialize(param, error_object_overrides = {})
+        @param = param
         super(error_object_overrides)
       end
 
       def errors
-        params.collect do |param|
-          create_error_object(code: JSONAPI::PARAM_NOT_ALLOWED,
-                              status: :bad_request,
-                              title: I18n.translate('jsonapi-resources.exceptions.parameters_not_allowed.title',
-                                                    default: 'Param not allowed'),
-                              detail: I18n.translate('jsonapi-resources.exceptions.parameters_not_allowed.detail',
-                                                     default: "#{param} is not allowed.", param: param))
-
-        end
+        [create_error_object(code: JSONAPI::PARAM_NOT_ALLOWED,
+                             status: :bad_request,
+                             title: I18n.translate('jsonapi-resources.exceptions.parameter_not_allowed.title',
+                                                   default: 'Param not allowed'),
+                             detail: I18n.translate('jsonapi-resources.exceptions.parameter_not_allowed.detail',
+                                                    default: "#{param} is not allowed.", param: param))]
       end
     end
 
diff --git a/lib/jsonapi/include_directives.rb b/lib/jsonapi/include_directives.rb
@@ -52,7 +52,7 @@ def get_related(current_path)
           current_relationship = current_resource_klass._relationships[fragment]
           current_resource_klass = current_relationship.try(:resource_klass)
         else
-          warn "[RELATIONSHIP NOT FOUND] Relationship could not be found for #{current_path}."
+          raise JSONAPI::Exceptions::InvalidInclude.new(current_resource_klass, current_path)
         end
 
         include_in_join = @force_eager_load || !current_relationship || current_relationship.eager_load_on_include
diff --git a/lib/jsonapi/request_parser.rb b/lib/jsonapi/request_parser.rb
@@ -170,25 +170,23 @@ def parse_fields(fields)
           end
           type_resource = Resource.resource_for(@resource_klass.module_path + underscored_type.to_s)
         rescue NameError
-          @errors.concat(JSONAPI::Exceptions::InvalidResource.new(type).errors)
-        rescue JSONAPI::Exceptions::InvalidResource => e
-          @errors.concat(e.errors)
+          fail JSONAPI::Exceptions::InvalidResource.new(type)
         end
 
         if type_resource.nil?
-          @errors.concat(JSONAPI::Exceptions::InvalidResource.new(type).errors)
+          fail JSONAPI::Exceptions::InvalidResource.new(type)
         else
           unless values.nil?
             valid_fields = type_resource.fields.collect { |key| format_key(key) }
             values.each do |field|
               if valid_fields.include?(field)
                 extracted_fields[type].push unformat_key(field)
               else
-                @errors.concat(JSONAPI::Exceptions::InvalidField.new(type, field).errors)
+                fail JSONAPI::Exceptions::InvalidField.new(type, field)
               end
             end
           else
-            @errors.concat(JSONAPI::Exceptions::InvalidField.new(type, 'nil').errors)
+            fail JSONAPI::Exceptions::InvalidField.new(type, 'nil')
           end
         end
       end
@@ -205,16 +203,15 @@ def check_include(resource_klass, include_parts)
           check_include(Resource.resource_for(resource_klass.module_path + relationship.class_name.to_s.underscore), include_parts.last.partition('.'))
         end
       else
-        @errors.concat(JSONAPI::Exceptions::InvalidInclude.new(format_key(resource_klass._type),
-                                                               include_parts.first).errors)
+        fail JSONAPI::Exceptions::InvalidInclude.new(format_key(resource_klass._type), include_parts.first)
       end
     end
 
     def parse_include_directives(raw_include)
       return unless raw_include
 
       unless JSONAPI.configuration.allow_include
-        fail JSONAPI::Exceptions::ParametersNotAllowed.new([:include])
+        fail JSONAPI::Exceptions::ParameterNotAllowed.new(:include)
       end
 
       included_resources = []
@@ -226,19 +223,24 @@ def parse_include_directives(raw_include)
 
       return if included_resources.empty?
 
-      result = included_resources.compact.map do |included_resource|
-        check_include(@resource_klass, included_resource.partition('.'))
-        unformat_key(included_resource).to_s
-      end
+      begin
+        result = included_resources.compact.map do |included_resource|
+          check_include(@resource_klass, included_resource.partition('.'))
+          unformat_key(included_resource).to_s
+        end
 
-      @include_directives = JSONAPI::IncludeDirectives.new(@resource_klass, result)
+        @include_directives = JSONAPI::IncludeDirectives.new(@resource_klass, result)
+      rescue JSONAPI::Exceptions::InvalidInclude => e
+        @errors.concat(e.errors)
+        @include_directives = {}
+      end
     end
 
     def parse_filters(filters)
       return unless filters
 
       unless JSONAPI.configuration.allow_filter
-        fail JSONAPI::Exceptions::ParametersNotAllowed.new([:filter])
+        fail JSONAPI::Exceptions::ParameterNotAllowed.new(:filter)
       end
 
       unless filters.class.method_defined?(:each)
@@ -251,7 +253,7 @@ def parse_filters(filters)
         if @resource_klass._allowed_filter?(filter)
           @filters[filter] = value
         else
-          @errors.concat(JSONAPI::Exceptions::FilterNotAllowed.new(filter).errors)
+          fail JSONAPI::Exceptions::FilterNotAllowed.new(filter)
         end
       end
     end
@@ -267,7 +269,7 @@ def parse_sort_criteria(sort_criteria)
       return unless sort_criteria.present?
 
       unless JSONAPI.configuration.allow_sort
-        fail JSONAPI::Exceptions::ParametersNotAllowed.new([:sort])
+        fail JSONAPI::Exceptions::ParameterNotAllowed.new(:sort)
       end
 
       sorts = []
@@ -296,9 +298,8 @@ def check_sort_criteria(resource_klass, sort_criteria)
       sort_field = sort_criteria[:field]
       sortable_fields = resource_klass.sortable_fields(context)
 
-      unless sortable_fields.include? sort_field.to_sym
-        @errors.concat(JSONAPI::Exceptions::InvalidSortCriteria
-                         .new(format_key(resource_klass._type), sort_field).errors)
+      unless sortable_fields.include?sort_field.to_sym
+        fail JSONAPI::Exceptions::InvalidSortCriteria.new(format_key(resource_klass._type), sort_field)
       end
     end
 
@@ -473,7 +474,7 @@ def parse_to_one_relationship(link_value, relationship)
 
       unless links_object[:id].nil?
         resource = self.resource_klass || Resource
-        relationship_resource = resource.resource_for(unformat_key(links_object[:type]).to_s)
+        relationship_resource = resource.resource_for(unformat_key(relationship.options[:class_name] || links_object[:type]).to_s)
         relationship_id = relationship_resource.verify_key(links_object[:id], @context)
         if relationship.polymorphic?
           { id: relationship_id, type: unformat_key(links_object[:type].to_s) }
@@ -528,45 +529,52 @@ def verify_permitted_params(params, allowed_fields)
         when 'relationships'
           value.keys.each do |links_key|
             unless formatted_allowed_fields.include?(links_key.to_sym)
-              params_not_allowed.push(links_key)
-              unless JSONAPI.configuration.raise_if_parameters_not_allowed
+              if JSONAPI.configuration.raise_if_parameters_not_allowed
+                fail JSONAPI::Exceptions::ParameterNotAllowed.new(links_key)
+              else
+                params_not_allowed.push(links_key)
                 value.delete links_key
               end
             end
           end
         when 'attributes'
           value.each do |attr_key, attr_value|
             unless formatted_allowed_fields.include?(attr_key.to_sym)
-              params_not_allowed.push(attr_key)
-              unless JSONAPI.configuration.raise_if_parameters_not_allowed
+              if JSONAPI.configuration.raise_if_parameters_not_allowed
+                fail JSONAPI::Exceptions::ParameterNotAllowed.new(attr_key)
+              else
+                params_not_allowed.push(attr_key)
                 value.delete attr_key
               end
             end
           end
         when 'type'
         when 'id'
           unless formatted_allowed_fields.include?(:id)
-            params_not_allowed.push(:id)
-            unless JSONAPI.configuration.raise_if_parameters_not_allowed
+            if JSONAPI.configuration.raise_if_parameters_not_allowed
+              fail JSONAPI::Exceptions::ParameterNotAllowed.new(:id)
+            else
+              params_not_allowed.push(:id)
               params.delete :id
             end
           end
         else
-          params_not_allowed.push(key)
+          if JSONAPI.configuration.raise_if_parameters_not_allowed
+            fail JSONAPI::Exceptions::ParameterNotAllowed.new(key)
+          else
+            params_not_allowed.push(key)
+            params.delete key
+          end
         end
       end
 
       if params_not_allowed.length > 0
-        if JSONAPI.configuration.raise_if_parameters_not_allowed
-          fail JSONAPI::Exceptions::ParametersNotAllowed.new(params_not_allowed)
-        else
-          params_not_allowed_warnings = params_not_allowed.map do |key|
-            JSONAPI::Warning.new(code: JSONAPI::PARAM_NOT_ALLOWED,
-                                 title: 'Param not allowed',
-                                 detail: "#{key} is not allowed.")
-          end
-          self.warnings.concat(params_not_allowed_warnings)
+        params_not_allowed_warnings = params_not_allowed.map do |param|
+          JSONAPI::Warning.new(code: JSONAPI::PARAM_NOT_ALLOWED,
+                               title: 'Param not allowed',
+                               detail: "#{param} is not allowed.")
         end
+        self.warnings.concat(params_not_allowed_warnings)
       end
     end
 
diff --git a/lib/jsonapi/resource.rb b/lib/jsonapi/resource.rb
@@ -453,7 +453,7 @@ def rebuild_relationships(relationships)
 
       def resource_for(type)
         type = type.underscore
-        type_with_module = type.include?('/') ? type : module_path + type
+        type_with_module = type.start_with?(module_path) ? type : module_path + type
 
         resource_name = _resource_name_from_type(type_with_module)
         resource = resource_name.safe_constantize if resource_name
@@ -507,10 +507,12 @@ def attributes(*attrs)
         end
       end
 
-      def attribute(attr, options = {})
+      def attribute(attribute_name, options = {})
+        attr = attribute_name.to_sym
+
         check_reserved_attribute_name(attr)
 
-        if (attr.to_sym == :id) && (options[:format].nil?)
+        if (attr == :id) && (options[:format].nil?)
           ActiveSupport::Deprecation.warn('Id without format is no longer supported. Please remove ids from attributes, or specify a format.')
         end
 
@@ -1041,7 +1043,8 @@ def _add_relationship(klass, *attrs)
         options = attrs.extract_options!
         options[:parent_resource] = self
 
-        attrs.each do |relationship_name|
+        attrs.each do |name|
+          relationship_name = name.to_sym
           check_reserved_relationship_name(relationship_name)
           check_duplicate_relationship_name(relationship_name)
 
diff --git a/locales/en.yml b/locales/en.yml
@@ -55,7 +55,7 @@ en:
       invalid_sort_criteria:
         title: 'Invalid sort criteria'
         detail: "%{sort_criteria} is not a valid sort criteria for %{resource}"
-      parameters_not_allowed:
+      parameter_not_allowed:
         title: 'Param not allowed'
         detail: "%{param} is not allowed."
       parameter_missing:
diff --git a/test/controllers/controller_test.rb b/test/controllers/controller_test.rb
@@ -1803,7 +1803,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
 
@@ -2058,7 +2057,6 @@ def test_expense_entries_show_bad_include_missing_relationship
     assert_cacheable_get :show, params: {id: 1, include: 'isoCurrencies,employees'}
     assert_response :bad_request
     assert_match /isoCurrencies is not a valid relationship of expenseEntries/, json_response['errors'][0]['detail']
-    assert_match /employees is not a valid relationship of expenseEntries/, json_response['errors'][1]['detail']
   end
 
   def test_expense_entries_show_bad_include_missing_sub_relationship
@@ -2067,6 +2065,18 @@ def test_expense_entries_show_bad_include_missing_sub_relationship
     assert_match /post is not a valid relationship of people/, json_response['errors'][0]['detail']
   end
 
+  def test_invalid_include
+    assert_cacheable_get :index, params: {include: 'invalid../../../../'}
+    assert_response :bad_request
+    assert_match /invalid is not a valid relationship of expenseEntries/, json_response['errors'][0]['detail']
+  end
+
+  def test_invalid_include_long_garbage_string
+    assert_cacheable_get :index, params: {include: 'invalid.foo.bar.dfsdfs,dfsdfs.sdfwe.ewrerw.erwrewrew'}
+    assert_response :bad_request
+    assert_match /invalid is not a valid relationship of expenseEntries/, json_response['errors'][0]['detail']
+  end
+
   def test_expense_entries_show_fields
     assert_cacheable_get :show, params: {id: 1, include: 'isoCurrency,employee', 'fields' => {'expenseEntries' => 'transactionDate'}}
     assert_response :success
diff --git a/test/fixtures/active_record.rb b/test/fixtures/active_record.rb
@@ -1462,12 +1462,12 @@ class PersonResource < PersonResource; end
     class PostResource < PostResource; end
 
     class BookResource < JSONAPI::Resource
-      attribute :title
+      attribute "title"
       attributes :isbn, :banned
 
-      has_many :authors
+      has_many "authors"
 
-      has_many :book_comments, relation_name: -> (options = {}) {
+      has_many "book_comments", relation_name: -> (options = {}) {
         context = options[:context]
         current_user = context ? context[:current_user] : nil
 
@@ -1478,7 +1478,7 @@ class BookResource < JSONAPI::Resource
         end
       }, reflect: true
 
-      has_many :aliased_comments, class_name: 'BookComments', relation_name: :approved_book_comments
+      has_many "aliased_comments", class_name: 'BookComments', relation_name: :approved_book_comments
 
       filters :book_comments
       filter :banned, apply: :apply_filter_banned
diff --git a/test/unit/jsonapi_request/jsonapi_request_test.rb b/test/unit/jsonapi_request/jsonapi_request_test.rb
diff --git a/test/unit/resource/resource_test.rb b/test/unit/resource/resource_test.rb
diff --git a/test/unit/serializer/include_directives_test.rb b/test/unit/serializer/include_directives_test.rb
