-
Notifications
You must be signed in to change notification settings - Fork 61
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.
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
ActiveModel error serializer improvements #45
Conversation
details[attr] << error.merge(message: message) | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johvet would you be kind to explain or provide a test for this changeset please. As far as I know, the error handling is supported for rails 4-6...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am terribly sorry for my late reply. I don't recall the very details, but we have noticed that the current solution in master does not work consistently between Rails 5.x and 6.x -- It also makes a difference on whether symbols are used or text messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC it's because the previous implementation didn't work for something like:
errors.add(:foo, :blank)
errors.add(:foo, :some_custom_key)
but did work with
errors.add(:foo, 'some string')
IIRC you'd end up with message: 'some_custom_key'
instead of the I18n'ed string that exists in resource.messages
. I think this is a Rails bug though.
But looking above, the current[:error] ||= :invalid
is redundant/impossible, so I think this needs some comments explaining the different ways the data might be available
else | ||
{ pointer: '' } | ||
{ pointer: nil } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johvet let's double check if this is a valid JSONAPI response, more context here:
https://jsonapi.org/examples/#error-objects-source-usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I do not remember the reasons for me to change this to nil, but after reading the JSONAPI spec above, using ''
seems to be the correct way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason for nil
is because the pointer is unknown/unmapped in this situation but ''
means "the root of the document", which is incorrect here
Thanks @johvet! I left some questions re the error handling compatibility. The rest I'd happily merge asap ❤️ |
Superseded by #98 |
What is the current behavior?
The
JSONAPI::ActiveModelErrorSerializer
does not support a (response) status provided to the error render.It also does not handle serializing ActiveModel errors consistently for Rails versions 5, 6 and 6.1.
Last but not least, the source pointer for errors on the model's base is not right.
What is the new behavior?
A status provided to an error serializer is now honored:
This would render the
status
member of the error response with'409'
With an error on a model's base the source pointer rendered is correct now.
This would return a
Finally, all Rails versions starting with 5 are rendering the error responses consistently. For example, an empty but required
belongs_to :user
relationship would have rendered asUser can't be blank
for Rails 5+ and 6.0, but asUser must exist
for Rails 6.1. This is nowUser must exist
consistently. Errors with only a message and no symbol are handled consistently as well now.Checklist
Please make sure the following requirements are complete:
features)