From 88f51096e6789bda0f08dd07058590d92f0501de Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 31 Jan 2024 21:13:11 +0100 Subject: [PATCH 1/4] CI Matrix: Drop Rails 6, Ruby 2.7 Test Ruby 3.3 and Rails 6.1 instead --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fe0ebc0..d9e837c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,8 +8,8 @@ jobs: strategy: matrix: - ruby: [2.7, '3.0', '3.1', 3.2] - rails: [6, '7.0', '7.1'] + ruby: ['3.0', '3.1', '3.2', '3.3'] + rails: ['6.1', '7.0', '7.1'] exclude: - ruby: 3.2 rails: 6 From be74b9af0c99acaea3be44a5ef70cdecd6d43cc3 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 31 Jan 2024 18:06:39 +0100 Subject: [PATCH 2/4] Refactor error spec The previous specs were harder to read, and they relied on the ordering of the errors, which is unnecessary. --- spec/errors_spec.rb | 142 +++++++++++++++++++++----------------------- 1 file changed, 68 insertions(+), 74 deletions(-) diff --git a/spec/errors_spec.rb b/spec/errors_spec.rb index 84354aa..54f3283 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,15 @@ 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') - else - expect(response_json['errors'][0]['detail']) - .to eq('User can\'t be blank') - end + expect(response_json['errors']).to contain_exactly( + { + 'status' => '422', + 'source' => { 'pointer' => '/data/relationships/user' }, + 'title' => 'Unprocessable Entity', + 'detail' => Rails.gem_version >= Gem::Version.new('6.1') ? 'User must exist' : 'User can\'t be blank', + 'code' => 'blank' + } + ) end context 'required by validations' do @@ -81,45 +76,29 @@ 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 @@ -134,8 +113,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 +133,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 +152,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 From da08b5b8804a2ad9df3cbe62fcd010f9e934868f Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 31 Jan 2024 18:18:53 +0100 Subject: [PATCH 3/4] Add spec for non-interpolated error messages --- spec/dummy.rb | 7 +++++++ spec/errors_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/spec/dummy.rb b/spec/dummy.rb index 9f714e1..803bc3b 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,12 @@ 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 54f3283..c78a095 100644 --- a/spec/errors_spec.rb +++ b/spec/errors_spec.rb @@ -102,6 +102,28 @@ 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 + context 'as a param attribute' do let(:params) do payload = note_params.dup From 89f6e61d21ca097f8f6fec7f386a789bf3acdfa0 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 31 Jan 2024 18:11:20 +0100 Subject: [PATCH 4/4] 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 | 1 - spec/errors_spec.rb | 9 +++++-- 4 files changed, 15 insertions(+), 51 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 803bc3b..8f222fd 100644 --- a/spec/dummy.rb +++ b/spec/dummy.rb @@ -55,7 +55,6 @@ def self.ransackable_attributes(auth_object = nil) end private - def title_cannot_contain_slurs errors.add(:base, 'Title has slurs') if title.to_s.include?('SLURS') end diff --git a/spec/errors_spec.rb b/spec/errors_spec.rb index c78a095..a704a63 100644 --- a/spec/errors_spec.rb +++ b/spec/errors_spec.rb @@ -54,12 +54,17 @@ it do expect(response).to have_http_status(:unprocessable_entity) expect(response_json['errors'].size).to eq(1) + expected_detail = if Rails.gem_version >= Gem::Version.new('6.1') + 'User must exist' + else + 'User can\'t be blank' + end expect(response_json['errors']).to contain_exactly( { 'status' => '422', 'source' => { 'pointer' => '/data/relationships/user' }, 'title' => 'Unprocessable Entity', - 'detail' => Rails.gem_version >= Gem::Version.new('6.1') ? 'User must exist' : 'User can\'t be blank', + 'detail' => expected_detail, 'code' => 'blank' } ) @@ -102,7 +107,7 @@ end end - context "validations with non-interpolated messages" do + context 'validations with non-interpolated messages' do let(:params) do payload = note_params.dup payload[:data][:attributes][:title] = 'SLURS ARE GREAT'