Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ActiveModel::ErrorSerializer #98

Merged
merged 4 commits into from
Jun 23, 2024
Merged

Refactor ActiveModel::ErrorSerializer #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
88f5109
CI Matrix: Drop Rails 6, Ruby 2.7
mamhoff Jan 31, 2024
be74b9a
Refactor error spec
mamhoff Jan 31, 2024
da08b5b
Add spec for non-interpolated error messages
mamhoff Jan 31, 2024
89f6e61
Simplify ActiveModel::ErrorSerializer
mamhoff Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 4 additions & 24 deletions lib/jsonapi/active_model_error_serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems before this change an array was supported here as object but now it is not. Is there a reason for the change in behavior there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that was certainly not intended, but there was also no test for it. If you depend on that behaviour, as a spec and an Array.wrap or similar as a PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is more correct i can update on my end. we were building up these errors manually (for some unknown reason) which this suddenly broke.

model_serializer = params[:model_serializer]
attrs = (model_serializer.attributes_to_serialize || {}).keys
rels = (model_serializer.relationships_to_serialize || {}).keys
Expand Down
28 changes: 4 additions & 24 deletions lib/jsonapi/rails.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
6 changes: 6 additions & 0 deletions spec/dummy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
165 changes: 93 additions & 72 deletions spec/errors_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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

Expand All @@ -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
Expand All @@ -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

Expand All @@ -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
Expand Down