From 78e3560b84f3f99942f86b9a45e3c3c494295752 Mon Sep 17 00:00:00 2001 From: Larry Gebhardt Date: Wed, 21 Sep 2016 14:57:47 -0400 Subject: [PATCH] Handle all base spec requests as a single operation Getting ready for multiple operation support in requests (cherry picked from commit ff8b0c9) --- lib/jsonapi/error_codes.rb | 4 +- lib/jsonapi/exceptions.rb | 24 +- lib/jsonapi/processor.rb | 24 +- lib/jsonapi/request_parser.rb | 73 ++---- locales/en.yml | 6 +- test/controllers/controller_test.rb | 234 ++++-------------- test/integration/requests/request_test.rb | 2 +- .../operation/operation_dispatcher_test.rb | 8 +- 8 files changed, 116 insertions(+), 259 deletions(-) diff --git a/lib/jsonapi/error_codes.rb b/lib/jsonapi/error_codes.rb index cf0304aa2..290ee6189 100644 --- a/lib/jsonapi/error_codes.rb +++ b/lib/jsonapi/error_codes.rb @@ -7,7 +7,6 @@ module JSONAPI PARAM_NOT_ALLOWED = '105' PARAM_MISSING = '106' INVALID_FILTER_VALUE = '107' - COUNT_MISMATCH = '108' KEY_ORDER_MISMATCH = '109' KEY_NOT_INCLUDED_IN_URL = '110' INVALID_INCLUDE = '112' @@ -20,6 +19,7 @@ module JSONAPI INVALID_FIELD_FORMAT = '119' INVALID_FILTERS_SYNTAX = '120' SAVE_FAILED = '121' + INVALID_DATA_FORMAT = '122' FORBIDDEN = '403' RECORD_NOT_FOUND = '404' NOT_ACCEPTABLE = '406' @@ -36,7 +36,6 @@ module JSONAPI PARAM_NOT_ALLOWED => 'PARAM_NOT_ALLOWED', PARAM_MISSING => 'PARAM_MISSING', INVALID_FILTER_VALUE => 'INVALID_FILTER_VALUE', - COUNT_MISMATCH => 'COUNT_MISMATCH', KEY_ORDER_MISMATCH => 'KEY_ORDER_MISMATCH', KEY_NOT_INCLUDED_IN_URL => 'KEY_NOT_INCLUDED_IN_URL', INVALID_INCLUDE => 'INVALID_INCLUDE', @@ -49,6 +48,7 @@ module JSONAPI INVALID_FIELD_FORMAT => 'INVALID_FIELD_FORMAT', INVALID_FILTERS_SYNTAX => 'INVALID_FILTERS_SYNTAX', SAVE_FAILED => 'SAVE_FAILED', + INVALID_DATA_FORMAT => 'INVALID_DATA_FORMAT', FORBIDDEN => 'FORBIDDEN', RECORD_NOT_FOUND => 'RECORD_NOT_FOUND', NOT_ACCEPTABLE => 'NOT_ACCEPTABLE', diff --git a/lib/jsonapi/exceptions.rb b/lib/jsonapi/exceptions.rb index 36e789869..bb44fb8a2 100644 --- a/lib/jsonapi/exceptions.rb +++ b/lib/jsonapi/exceptions.rb @@ -2,7 +2,9 @@ module JSONAPI module Exceptions class Error < RuntimeError def errors + # :nocov: raise NotImplementedError, "Subclass of Error must implement errors method" + # :nocov: end end @@ -208,6 +210,17 @@ def errors end end + class InvalidDataFormat < Error + def errors + [JSONAPI::Error.new(code: JSONAPI::INVALID_DATA_FORMAT, + status: :bad_request, + title: I18n.translate('jsonapi-resources.exceptions.invalid_data_format.title', + default: 'Invalid data format'), + detail: I18n.translate('jsonapi-resources.exceptions.invalid_data_format.detail', + default: 'Data must be a hash.'))] + end + end + class InvalidLinksObject < Error def errors [JSONAPI::Error.new(code: JSONAPI::INVALID_LINKS_OBJECT, @@ -324,17 +337,6 @@ def errors end end - class CountMismatch < Error - def errors - [JSONAPI::Error.new(code: JSONAPI::COUNT_MISMATCH, - status: :bad_request, - title: I18n.translate('jsonapi-resources.exceptions.count_mismatch.title', - default: 'Count to key mismatch'), - detail: I18n.translate('jsonapi-resources.exceptions.count_mismatch.detail', - default: 'The resource collection does not contain the same number of objects as the number of keys.'))] - end - end - class KeyNotIncludedInURL < Error attr_accessor :key def initialize(key) diff --git a/lib/jsonapi/processor.rb b/lib/jsonapi/processor.rb index 3cd411ce9..82566568a 100644 --- a/lib/jsonapi/processor.rb +++ b/lib/jsonapi/processor.rb @@ -11,9 +11,9 @@ class Processor :replace_fields, :replace_to_one_relationship, :replace_polymorphic_to_one_relationship, - :create_to_many_relationship, - :replace_to_many_relationship, - :remove_to_many_relationship, + :create_to_many_relationships, + :replace_to_many_relationships, + :remove_to_many_relationships, :remove_to_one_relationship, :operation @@ -276,7 +276,7 @@ def replace_polymorphic_to_one_relationship return JSONAPI::OperationResult.new(result == :completed ? :no_content : :accepted) end - def create_to_many_relationship + def create_to_many_relationships resource_id = params[:resource_id] relationship_type = params[:relationship_type].to_sym data = params[:data] @@ -287,7 +287,7 @@ def create_to_many_relationship return JSONAPI::OperationResult.new(result == :completed ? :no_content : :accepted) end - def replace_to_many_relationship + def replace_to_many_relationships resource_id = params[:resource_id] relationship_type = params[:relationship_type].to_sym data = params.fetch(:data) @@ -298,15 +298,21 @@ def replace_to_many_relationship return JSONAPI::OperationResult.new(result == :completed ? :no_content : :accepted) end - def remove_to_many_relationship + def remove_to_many_relationships resource_id = params[:resource_id] relationship_type = params[:relationship_type].to_sym - associated_key = params[:associated_key] + associated_keys = params[:associated_keys] resource = resource_klass.find_by_key(resource_id, context: context) - result = resource.remove_to_many_link(relationship_type, associated_key) - return JSONAPI::OperationResult.new(result == :completed ? :no_content : :accepted) + complete = true + associated_keys.each do |key| + result = resource.remove_to_many_link(relationship_type, key) + if complete && result != :completed + complete = false + end + end + return JSONAPI::OperationResult.new(complete ? :no_content : :accepted) end def remove_to_one_relationship diff --git a/lib/jsonapi/request_parser.rb b/lib/jsonapi/request_parser.rb index 05f103782..eb0dbf1ee 100644 --- a/lib/jsonapi/request_parser.rb +++ b/lib/jsonapi/request_parser.rb @@ -346,19 +346,19 @@ def add_show_related_resources_operation(relationship_type) ) end - def parse_add_operation(data) - Array.wrap(data).each do |params| - verify_type(params[:type]) + def parse_add_operation(params) + fail JSONAPI::Exceptions::InvalidDataFormat unless params.respond_to?(:each_pair) - data = parse_params(params, @resource_klass.creatable_fields(@context)) - @operations.push JSONAPI::Operation.new(:create_resource, - @resource_klass, - context: @context, - data: data, - fields: @fields, - include_directives: @include_directives - ) - end + verify_type(params[:type]) + + data = parse_params(params, @resource_klass.creatable_fields(@context)) + @operations.push JSONAPI::Operation.new(:create_resource, + @resource_klass, + context: @context, + data: data, + fields: @fields, + include_directives: @include_directives + ) rescue JSONAPI::Exceptions::Error => e @errors.concat(e.errors) end @@ -558,7 +558,7 @@ def verify_permitted_params(params, allowed_fields) def parse_add_relationship_operation(verified_params, relationship, parent_key) if relationship.is_a?(JSONAPI::Relationship::ToMany) - @operations.push JSONAPI::Operation.new(:create_to_many_relationship, + @operations.push JSONAPI::Operation.new(:create_to_many_relationships, resource_klass, context: @context, resource_id: parent_key, @@ -590,13 +590,15 @@ def parse_update_relationship_operation(verified_params, relationship, parent_ke fail JSONAPI::Exceptions::ToManySetReplacementForbidden.new end options[:data] = verified_params[:to_many].values[0] - operation_type = :replace_to_many_relationship + operation_type = :replace_to_many_relationships end @operations.push JSONAPI::Operation.new(operation_type, resource_klass, options) end def parse_single_replace_operation(data, keys, id_key_presence_check_required: true) + fail JSONAPI::Exceptions::InvalidDataFormat unless data.respond_to?(:each_pair) + fail JSONAPI::Exceptions::MissingKey.new if data[:id].nil? key = data[:id].to_s @@ -619,31 +621,16 @@ def parse_single_replace_operation(data, keys, id_key_presence_check_required: t end def parse_replace_operation(data, keys) - if data.is_a?(Array) - fail JSONAPI::Exceptions::CountMismatch if keys.count != data.count - - data.each do |object_params| - parse_single_replace_operation(object_params, keys) - end - else - parse_single_replace_operation(data, [keys], - id_key_presence_check_required: keys.present?) - end - + parse_single_replace_operation(data, [keys], id_key_presence_check_required: keys.present?) rescue JSONAPI::Exceptions::Error => e @errors.concat(e.errors) end def parse_remove_operation(params) - keys = parse_key_array(params.require(:id)) - - keys.each do |key| - @operations.push JSONAPI::Operation.new(:remove_resource, - @resource_klass, - context: @context, - resource_id: key - ) - end + @operations.push JSONAPI::Operation.new(:remove_resource, + @resource_klass, + context: @context, + resource_id: @resource_klass.verify_key(params.require(:id), context)) rescue JSONAPI::Exceptions::Error => e @errors.concat(e.errors) end @@ -656,25 +643,15 @@ def parse_remove_relationship_operation(params, relationship, parent_key) ) if relationship.is_a?(JSONAPI::Relationship::ToMany) + operation_args = operation_base_args.dup keys = params[:to_many].values[0] - keys.each do |key| - operation_args = operation_base_args.dup - operation_args[1] = operation_args[1].merge(associated_key: key) - @operations.push JSONAPI::Operation.new(:remove_to_many_relationship, - *operation_args - ) - end + operation_args[1] = operation_args[1].merge(associated_keys: keys) + @operations.push JSONAPI::Operation.new(:remove_to_many_relationships, *operation_args) else - @operations.push JSONAPI::Operation.new(:remove_to_one_relationship, - *operation_base_args - ) + @operations.push JSONAPI::Operation.new(:remove_to_one_relationship, *operation_base_args) end end - def parse_key_array(raw) - @resource_klass.verify_keys(raw.split(/,/), context) - end - def format_key(key) @key_formatter.format(key) end diff --git a/locales/en.yml b/locales/en.yml index 058449d5c..7a03182a6 100644 --- a/locales/en.yml +++ b/locales/en.yml @@ -37,6 +37,9 @@ en: invalid_field_format: title: 'Invalid field format' detail: 'Fields must specify a type.' + invalid_data_format: + title: 'Invalid data format' + detail: 'Data must be a hash.' invalid_links_object: title: 'Invalid Links Object' detail: 'Data is not a valid Links Object.' @@ -58,9 +61,6 @@ en: parameter_missing: title: 'Missing Parameter' detail: "The required parameter, %{param}, is missing." - count_mismatch: - title: 'Count to key mismatch' - detail: 'The resource collection does not contain the same number of objects as the number of keys.' key_not_included_in_url: title: 'Key is not included in URL' detail: "The URL does not support the key %{key}" diff --git a/test/controllers/controller_test.rb b/test/controllers/controller_test.rb index 6ad761bff..688927afd 100644 --- a/test/controllers/controller_test.rb +++ b/test/controllers/controller_test.rb @@ -688,46 +688,8 @@ def test_create_multiple ] } - assert_response :created - assert json_response['data'].is_a?(Array) - assert_equal json_response['data'].size, 2 - assert_nil json_response['data'][0]['relationships']['author']['data'] - assert_match /JR is Great/, response.body - assert_match /Ember is Great/, response.body - assert_equal nil, response.location - end - - def test_create_multiple_wrong_case - set_content_type_header! - post :create, params: - { - data: [ - { - type: 'posts', - attributes: { - Title: 'JR is Great', - body: 'JSONAPIResources is the greatest thing since unsliced bread.' - }, - relationships: { - author: {data: {type: 'people', id: '3'}} - } - }, - { - type: 'posts', - attributes: { - title: 'Ember is Great', - BODY: 'Ember is the greatest thing since unsliced bread.' - }, - relationships: { - author: {data: {type: 'people', id: '3'}} - } - } - ] - } - assert_response :bad_request - assert_match /Title/, json_response['errors'][0]['detail'] - assert_equal nil, response.location + assert_match /Invalid data format/, response.body end def test_create_simple_missing_posts @@ -1448,12 +1410,30 @@ def test_update_relationship_to_many_missing_tags def test_delete_relationship_to_many set_content_type_header! - put :update_relationship, params: {post_id: 14, relationship: 'tags', data: [{type: 'tags', id: 2}, {type: 'tags', id: 3}]} + put :update_relationship, + params: { + post_id: 14, + relationship: 'tags', + data: [ + {type: 'tags', id: 2}, + {type: 'tags', id: 3}, + {type: 'tags', id: 4} + ] + } + assert_response :no_content p = Post.find(14) - assert_equal [2, 3], p.tag_ids - - delete :destroy_relationship, params: {post_id: 14, relationship: 'tags', data: [{type: 'tags', id: 3}]} + assert_equal [2, 3, 4], p.tag_ids + + delete :destroy_relationship, + params: { + post_id: 14, + relationship: 'tags', + data: [ + {type: 'tags', id: 3}, + {type: 'tags', id: 4} + ] + } p.reload assert_response :no_content @@ -1703,157 +1683,55 @@ def test_update_unknown_key assert_match /body is not allowed/, response.body end - def test_update_multiple_sucess + def test_update_multiple_ids set_content_type_header! javascript = Section.find_by(name: 'javascript') - put :update, params: - { - id: [3, 16], - data: [ - { + put :update, params: { + id: '3,16', + data: { type: 'posts', id: 3, attributes: { - title: 'A great new Post QWERTY' - }, - relationships: { - section: {data: {type: 'sections', id: "#{javascript.id}"}}, - tags: {data: [{type: 'tags', id: 3}, {type: 'tags', id: 4}]} - } - }, - { - type: 'posts', - id: 16, - attributes: { - title: 'A great new Post ASDFG' + title: 'A great new Post QWERTY' }, relationships: { - section: {data: {type: 'sections', id: "#{javascript.id}"}}, - tags: {data: [{type: 'tags', id: 3}, {type: 'tags', id: 4}]} + section: { data: { type: 'sections', id: "#{javascript.id}" } }, + tags: { data: [{ type: 'tags', id: 3 }, { type: 'tags', id: 4 }] } } - } - ], + }, include: 'tags' - } - - assert_response :success - assert_equal json_response['data'].size, 2 - assert_equal json_response['data'][0]['attributes']['title'], 'A great new Post QWERTY' - assert_equal json_response['data'][0]['attributes']['body'], 'AAAA' - assert matches_array?([{'type' => 'tags', 'id' => '3'}, {'type' => 'tags', 'id' => '4'}], - json_response['data'][0]['relationships']['tags']['data']) - - assert_equal json_response['data'][1]['attributes']['title'], 'A great new Post ASDFG' - assert_equal json_response['data'][1]['attributes']['body'], 'Not First!!!!' - assert matches_array?([{'type' => 'tags', 'id' => '3'}, {'type' => 'tags', 'id' => '4'}], - json_response['data'][1]['relationships']['tags']['data']) - end - - def test_update_multiple_missing_keys - set_content_type_header! - javascript = Section.find_by(name: 'javascript') - - put :update, params: - { - id: [3, 9], - data: [ - { - type: 'posts', - attributes: { - title: 'A great new Post ASDFG' - }, - relationships: { - section: {type: 'sections', id: "#{javascript.id}"}, - tags: [{type: 'tags', id: 3}, {type: 'tags', id: 4}] - } - }, - { - type: 'posts', - attributes: { - title: 'A great new Post QWERTY' - }, - relationships: { - section: {type: 'sections', id: "#{javascript.id}"}, - tags: [{type: 'tags', id: 3}, {type: 'tags', id: 4}] - } - } - ]} - - assert_response :bad_request - assert_match /A key is required/, response.body - end - - def test_update_mismatch_multiple_keys - set_content_type_header! - javascript = Section.find_by(name: 'javascript') - - put :update, params: - { - id: [3, 9], - data: [ - { - type: 'posts', - id: 3, - attributes: { - title: 'A great new Post ASDFG' - }, - relationships: { - section: {data: {type: 'sections', id: "#{javascript.id}"}}, - tags: {data: [{type: 'tags', id: 3}, {type: 'tags', id: 4}]} - } - }, - { - type: 'posts', - id: 8, - attributes: { - title: 'A great new Post QWERTY' - }, - relationships: { - section: {data: {type: 'sections', id: "#{javascript.id}"}}, - tags: {data: [{type: 'tags', id: 3}, {type: 'tags', id: 4}]} - } - } - ]} + } assert_response :bad_request - assert_match /The URL does not support the key 8/, response.body + assert_match /The URL does not support the key 3/, response.body end - def test_update_multiple_count_mismatch + def test_update_multiple_array set_content_type_header! javascript = Section.find_by(name: 'javascript') put :update, params: - { - id: [3, 9, 2], - data: [ - { - type: 'posts', + { id: 3, - attributes: { - title: 'A great new Post QWERTY' - }, - relationships: { - section: {type: 'sections', id: "#{javascript.id}"}, - tags: [{type: 'tags', id: 3}, {type: 'tags', id: 4}] - } - }, - { - type: 'posts', - id: 9, - attributes: { - title: 'A great new Post ASDFG' - }, - relationships: { - section: {type: 'sections', id: "#{javascript.id}"}, - tags: [{type: 'tags', id: 3}, {type: 'tags', id: 4}] - } - } - ]} + data: [ + { + type: 'posts', + id: 3, + attributes: { + title: 'A great new Post QWERTY' + }, + relationships: { + section: {data: {type: 'sections', id: "#{javascript.id}"}}, + tags: {data: [{type: 'tags', id: 3}, {type: 'tags', id: 4}]} + } + } + ], + include: 'tags' + } assert_response :bad_request - assert_match /Count to key mismatch/, response.body + assert_match /Invalid data format/, response.body end def test_update_unpermitted_attributes @@ -1917,14 +1795,8 @@ def test_delete_single def test_delete_multiple initial_count = Post.count delete :destroy, params: {id: '5,6'} - assert_response :no_content - assert_equal initial_count - 2, Post.count - end - - def test_delete_multiple_one_does_not_exist - initial_count = Post.count - delete :destroy, params: {id: '5,6,99999'} - assert_response :not_found + assert_response :bad_request + assert_match /5,6 is not a valid value for id/, response.body assert_equal initial_count, Post.count end diff --git a/test/integration/requests/request_test.rb b/test/integration/requests/request_test.rb index 5cfcc8500..99885de21 100644 --- a/test/integration/requests/request_test.rb +++ b/test/integration/requests/request_test.rb @@ -408,7 +408,7 @@ def test_destroy_single def test_destroy_multiple delete '/posts/8,9', headers: { 'Accept' => JSONAPI::MEDIA_TYPE } - assert_equal 204, status + assert_equal 400, status end def test_pagination_none diff --git a/test/unit/operation/operation_dispatcher_test.rb b/test/unit/operation/operation_dispatcher_test.rb index 39ee1d6ee..278ebd6dd 100644 --- a/test/unit/operation/operation_dispatcher_test.rb +++ b/test/unit/operation/operation_dispatcher_test.rb @@ -104,7 +104,7 @@ def test_replace_to_one_relationship assert_equal(saturn.planet_type_id, 5) end - def test_create_to_many_relationship + def test_create_to_many_relationships op = JSONAPI::OperationDispatcher.new betax = Planet.find(5) @@ -120,7 +120,7 @@ def test_create_to_many_relationship betaz.save! operations = [ - JSONAPI::Operation.new(:create_to_many_relationship, + JSONAPI::Operation.new(:create_to_many_relationships, PlanetTypeResource, { resource_id: gas_giant.id, @@ -149,7 +149,7 @@ def test_create_to_many_relationship betaz.save! end - def test_replace_to_many_relationship + def test_replace_to_many_relationships op = JSONAPI::OperationDispatcher.new betax = Planet.find(5) @@ -165,7 +165,7 @@ def test_replace_to_many_relationship betaz.save! operations = [ - JSONAPI::Operation.new(:replace_to_many_relationship, + JSONAPI::Operation.new(:replace_to_many_relationships, PlanetTypeResource, { resource_id: gas_giant.id,