-
Notifications
You must be signed in to change notification settings - Fork 65
ActiveModel error serializer improvements #45
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
Changes from all commits
0ad9e6a
39aadc9
e383a59
6a81a5a
0f67ee5
74f3ac2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,8 +62,22 @@ def self.add_errors_renderer! | |
| details[attr] ||= [] | ||
| details[attr] << error.detail.merge(message: error.message) | ||
| end | ||
| elsif resource.respond_to?(:details) | ||
| details = resource.details | ||
| elsif resource.respond_to?(:details) && resource.respond_to?(:messages) | ||
| resource.details.each do |attr, problems| | ||
| problems.each_with_index do |error, index| | ||
| details[attr] ||= [] | ||
|
|
||
| if error[:error].is_a?(Hash) | ||
| current = error[:error].dup | ||
| current[:error] ||= :invalid | ||
|
|
||
| details[attr] << current | ||
| else | ||
| message = resource.messages[attr][index] | ||
| details[attr] << error.merge(message: message) | ||
| end | ||
| end | ||
| end | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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...
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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: but did work with IIRC you'd end up with But looking above, the |
||
| else | ||
| details = resource.messages | ||
| end | ||
|
|
@@ -79,7 +93,11 @@ def self.add_errors_renderer! | |
|
|
||
| JSONAPI::Rails.serializer_to_json( | ||
| JSONAPI::ActiveModelErrorSerializer.new( | ||
| errors, params: { model: model, model_serializer: model_serializer } | ||
| errors, params: { | ||
| model: model, | ||
| model_serializer: model_serializer, | ||
| status: options[:status] | ||
| } | ||
| ) | ||
| ) | ||
| 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 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
nilis because the pointer is unknown/unmapped in this situation but''means "the root of the document", which is incorrect here