From 52b4128d1a3538cb19822d4ff90351a62fa27dc5 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sun, 23 Jun 2024 14:10:02 +0200 Subject: [PATCH] Refactor ActiveModel::ErrorSerializer (#98) * CI Matrix: Drop Rails 6, Ruby 2.7 Test Ruby 3.3 and Rails 6.1 instead * Refactor error spec The previous specs were harder to read, and they relied on the ordering of the errors, which is unnecessary. * Add spec for non-interpolated error messages * Simplify ActiveModel::ErrorSerializer We're only running CI for Rails 6.1+ now, and we can now rely on a stable API for ActiveModel::Errors. --- lib/jsonapi/active_model_error_serializer.rb | 28 +--- lib/jsonapi/rails.rb | 28 +--- spec/dummy.rb | 6 + spec/errors_spec.rb | 165 +++++++++++-------- 4 files changed, 107 insertions(+), 120 deletions(-) diff --git a/lib/jsonapi/active_model_error_serializer.rb b/lib/jsonapi/active_model_error_serializer.rb index 59a3e3c..e46521b 100644 --- a/lib/jsonapi/active_model_error_serializer.rb +++ b/lib/jsonapi/active_model_error_serializer.rb @@ -12,35 +12,15 @@ class ActiveModelErrorSerializer < ErrorSerializer end attribute :code do |object| - _, error_hash = object - code = error_hash[:error] unless error_hash[:error].is_a?(Hash) - code ||= error_hash[:message] || :invalid - # `parameterize` separator arguments are different on Rails 4 vs 5... - code.to_s.delete("''").parameterize.tr('-', '_') + object.type.to_s.delete("''").parameterize.tr('-', '_') end - attribute :detail do |object, params| - error_key, error_hash = object - errors_object = params[:model].errors - - # Rails 4 provides just the message. - if error_hash[:error].present? && error_hash[:error].is_a?(Hash) - message = errors_object.generate_message( - error_key, nil, error_hash[:error] - ) - elsif error_hash[:error].present? - message = errors_object.generate_message( - error_key, error_hash[:error], error_hash - ) - else - message = error_hash[:message] - end - - errors_object.full_message(error_key, message) + attribute :detail do |object, _params| + object.full_message end attribute :source do |object, params| - error_key, _ = object + error_key = object.attribute model_serializer = params[:model_serializer] attrs = (model_serializer.attributes_to_serialize || {}).keys rels = (model_serializer.relationships_to_serialize || {}).keys diff --git a/lib/jsonapi/rails.rb b/lib/jsonapi/rails.rb index 836fd90..0983628 100644 --- a/lib/jsonapi/rails.rb +++ b/lib/jsonapi/rails.rb @@ -46,7 +46,6 @@ def self.add_errors_renderer! JSONAPI::ErrorSerializer.new(resource, options) ) unless resource.is_a?(ActiveModel::Errors) - errors = [] model = resource.instance_variable_get(:@base) if respond_to?(:jsonapi_serializer_class, true) @@ -55,31 +54,12 @@ def self.add_errors_renderer! model_serializer = JSONAPI::Rails.serializer_class(model, false) end - details = {} - if ::Rails.gem_version >= Gem::Version.new('6.1') - resource.each do |error| - attr = error.attribute - details[attr] ||= [] - details[attr] << error.detail.merge(message: error.message) - end - elsif resource.respond_to?(:details) - details = resource.details - else - details = resource.messages - end - - details.each do |error_key, error_hashes| - error_hashes.each do |error_hash| - # Rails 4 provides just the message. - error_hash = { message: error_hash } unless error_hash.is_a?(Hash) - - errors << [ error_key, error_hash ] - end - end - JSONAPI::Rails.serializer_to_json( JSONAPI::ActiveModelErrorSerializer.new( - errors, params: { model: model, model_serializer: model_serializer } + resource.errors, params: { + model: model, + model_serializer: model_serializer + } ) ) end diff --git a/spec/dummy.rb b/spec/dummy.rb index 9f714e1..8f222fd 100644 --- a/spec/dummy.rb +++ b/spec/dummy.rb @@ -41,6 +41,7 @@ def self.ransackable_associations(auth_object = nil) end class Note < ActiveRecord::Base + validate :title_cannot_contain_slurs validates_format_of :title, without: /BAD_TITLE/ validates_numericality_of :quantity, less_than: 100, if: :quantity? belongs_to :user, required: true @@ -52,6 +53,11 @@ def self.ransackable_associations(auth_object = nil) def self.ransackable_attributes(auth_object = nil) %w(created_at id quantity title updated_at user_id) end + + private + def title_cannot_contain_slurs + errors.add(:base, 'Title has slurs') if title.to_s.include?('SLURS') + end end class CustomNoteSerializer diff --git a/spec/errors_spec.rb b/spec/errors_spec.rb index 84354aa..a704a63 100644 --- a/spec/errors_spec.rb +++ b/spec/errors_spec.rb @@ -32,16 +32,15 @@ it do expect(response).to have_http_status(:unprocessable_entity) expect(response_json['errors'].size).to eq(1) - - expect(response_json['errors'].first.keys) - .to contain_exactly('status', 'source', 'title', 'detail', 'code') - - expect(response_json['errors'][0]['status']).to eq('422') - expect(response_json['errors'][0]['title']) - .to eq(Rack::Utils::HTTP_STATUS_CODES[422]) - expect(response_json['errors'][0]['source']).to eq('pointer' => '') - expect(response_json['errors'][0]['detail']).to be_nil - expect(response_json['errors'][0]['code']).to be_nil + expect(response_json['errors']).to contain_exactly( + { + 'status' => '422', + 'source' => { 'pointer' => '' }, + 'title' => 'Unprocessable Entity', + 'detail' => nil, + 'code' => nil + } + ) end end @@ -55,19 +54,20 @@ it do expect(response).to have_http_status(:unprocessable_entity) expect(response_json['errors'].size).to eq(1) - expect(response_json['errors'][0]['status']).to eq('422') - expect(response_json['errors'][0]['code']).to include('blank') - expect(response_json['errors'][0]['title']) - .to eq(Rack::Utils::HTTP_STATUS_CODES[422]) - expect(response_json['errors'][0]['source']) - .to eq('pointer' => '/data/relationships/user') - if Rails.gem_version >= Gem::Version.new('6.1') - expect(response_json['errors'][0]['detail']) - .to eq('User must exist') + expected_detail = if Rails.gem_version >= Gem::Version.new('6.1') + 'User must exist' else - expect(response_json['errors'][0]['detail']) - .to eq('User can\'t be blank') + 'User can\'t be blank' end + expect(response_json['errors']).to contain_exactly( + { + 'status' => '422', + 'source' => { 'pointer' => '/data/relationships/user' }, + 'title' => 'Unprocessable Entity', + 'detail' => expected_detail, + 'code' => 'blank' + } + ) end context 'required by validations' do @@ -81,45 +81,51 @@ it do expect(response).to have_http_status(:unprocessable_entity) expect(response_json['errors'].size).to eq(3) - expect(response_json['errors'][0]['status']).to eq('422') - expect(response_json['errors'][0]['code']).to include('invalid') - expect(response_json['errors'][0]['title']) - .to eq(Rack::Utils::HTTP_STATUS_CODES[422]) - expect(response_json['errors'][0]['source']) - .to eq('pointer' => '/data/attributes/title') - expect(response_json['errors'][0]['detail']) - .to eq('Title is invalid') - - expect(response_json['errors'][1]['status']).to eq('422') - - if Rails::VERSION::MAJOR >= 5 - expect(response_json['errors'][1]['code']).to eq('invalid') - else - expect(response_json['errors'][1]['code']).to eq('has_typos') - end - - expect(response_json['errors'][1]['title']) - .to eq(Rack::Utils::HTTP_STATUS_CODES[422]) - expect(response_json['errors'][1]['source']) - .to eq('pointer' => '/data/attributes/title') - expect(response_json['errors'][1]['detail']) - .to eq('Title has typos') - - expect(response_json['errors'][2]['status']).to eq('422') - - if Rails::VERSION::MAJOR >= 5 - expect(response_json['errors'][2]['code']).to eq('less_than') - else - expect(response_json['errors'][2]['code']) - .to eq('must_be_less_than_100') - end - - expect(response_json['errors'][2]['title']) - .to eq(Rack::Utils::HTTP_STATUS_CODES[422]) - expect(response_json['errors'][2]['source']) - .to eq('pointer' => '/data/attributes/quantity') - expect(response_json['errors'][2]['detail']) - .to eq('Quantity must be less than 100') + expect(response_json['errors']).to contain_exactly( + { + 'status' => '422', + 'source' => { 'pointer' => '/data/attributes/title' }, + 'title' => 'Unprocessable Entity', + 'detail' => 'Title is invalid', + 'code' => 'invalid' + }, + { + 'status' => '422', + 'source' => { 'pointer' => '/data/attributes/title' }, + 'title' => 'Unprocessable Entity', + 'detail' => 'Title has typos', + 'code' => 'invalid' + }, + { + 'status' => '422', + 'source' => { 'pointer' => '/data/attributes/quantity' }, + 'title' => 'Unprocessable Entity', + 'detail' => 'Quantity must be less than 100', + 'code' => 'less_than' + } + ) + end + end + + context 'validations with non-interpolated messages' do + let(:params) do + payload = note_params.dup + payload[:data][:attributes][:title] = 'SLURS ARE GREAT' + payload + end + + it do + expect(response).to have_http_status(:unprocessable_entity) + expect(response_json['errors'].size).to eq(1) + expect(response_json['errors']).to contain_exactly( + { + 'status' => '422', + 'source' => { 'pointer' => '' }, + 'title' => 'Unprocessable Entity', + 'detail' => 'Title has slurs', + 'code' => 'title_has_slurs' + } + ) end end @@ -134,8 +140,15 @@ it do expect(response).to have_http_status(:unprocessable_entity) - expect(response_json['errors'][0]['source']) - .to eq('pointer' => '/data/attributes/title') + expect(response_json['errors']).to contain_exactly( + { + 'status' => '422', + 'source' => { 'pointer' => '/data/attributes/title' }, + 'title' => 'Unprocessable Entity', + 'detail' => nil, + 'code' => nil + } + ) end end end @@ -147,11 +160,15 @@ it do expect(response).to have_http_status(:not_found) expect(response_json['errors'].size).to eq(1) - expect(response_json['errors'][0]['status']).to eq('404') - expect(response_json['errors'][0]['title']) - .to eq(Rack::Utils::HTTP_STATUS_CODES[404]) - expect(response_json['errors'][0]['source']).to be_nil - expect(response_json['errors'][0]['detail']).to be_nil + expect(response_json['errors']).to contain_exactly( + { + 'status' => '404', + 'source' => nil, + 'title' => 'Not Found', + 'detail' => nil, + 'code' => nil + } + ) end end @@ -162,11 +179,15 @@ it do expect(response).to have_http_status(:internal_server_error) expect(response_json['errors'].size).to eq(1) - expect(response_json['errors'][0]['status']).to eq('500') - expect(response_json['errors'][0]['title']) - .to eq(Rack::Utils::HTTP_STATUS_CODES[500]) - expect(response_json['errors'][0]['source']).to be_nil - expect(response_json['errors'][0]['detail']).to be_nil + expect(response_json['errors']).to contain_exactly( + { + 'status' => '500', + 'source' => nil, + 'title' => 'Internal Server Error', + 'detail' => nil, + 'code' => nil + } + ) end end end