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.

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

fix: only apply extractErrors to JSONAPI serializers #9470

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Eredrim
Copy link

@Eredrim Eredrim commented Jun 6, 2024

Description

Currently, ember-data uses a mechanism to extract information from error object in case of "InvalidError" from the server (e.g. HTTP 422 error). This mechanism is uses error format compliant to JSONAPI spec but is unfortunately also applied to REST Serializers.

This pull-request moves the code from parent JSONSerializer to JSONAPISerializer in order to keep usage of free error objects (i.e. not compliant with JSONAPI format).

Notes for the release

Fix: extractErrors is now only applied to JSONAPI Serializers when backend returns an InvalidError

@runspired
Copy link
Contributor

@Eredrim historically: we enforce the same spec for both. If you want to just pass through errors, your serializer should just not implement extractErrors at all (if inheriting this means you have to set the method to undefined), though this may cause model errors to no longer work. If you just want to normalize errors, you likely want to keep the method but adjust it for the format of your API or remove the method but do the error normalization in the adapter.

I'm very hesitant to change the behavior on adapters and serializers generally, both because they are now legacy and being rapidly phased out and because they are subject to hyrum's law.

@Eredrim
Copy link
Author

Eredrim commented Jun 7, 2024

@runspired I know this is easily possible to override this method in our serializers and we did it in our project, that's not the question. Nevertheless, I don't understand the current logic: the extractErrors method is 100% JSONAPI specific. In the jsdoc of the method, it's fully explicit:

This serializer expects this errors object to be an Array similar to the following, compliant with the https://jsonapi.org/format/#errors specification

On the other hand, in REST serializers, a classic implementation of errors is to extract anonymous error objects returned by the backend, thing we can't do with current implementation because this object is destroyed (because not-compliant with expected json-api format).

Finally, I'll add because it has only been moved between existing serializers, this code shouldn't impact anyone. It will continue working for people using json-api and for ones with extractErrors implementation in REST serializers.

That said, if you think this does not have interest because serializers will be removed, I can understand. It's not a problem.

@runspired
Copy link
Contributor

@Eredrim

On the other hand, in REST serializers, a classic implementation of errors is to extract anonymous error objects returned by the backend, thing we can't do with current implementation because this object is destroyed (because not-compliant with expected json-api format).

because there is no standard or even typical convention for errors in the broader REST paradigm, over a decade ago both the RESTSerializer and the JSONAPISerializer were standardized on expecting JSON:API formatted errors by design.

Which .. that isn't totally true fwiw. Its more that EmberData back then created a standard for errors which was later adopted and expanded by the JSON:API spec when the spec was extracted from EmberData.

It could be a significant breaking change for us to stop expecting and handling errors in the JSON:API format by default given thats the only format that has worked for model errors for the past decade. (could be, because its hard to know the scope of breakage sometimes without doing the thing and seeing who complains).

@Eredrim
Copy link
Author

Eredrim commented Jun 7, 2024

I understand your arguments. I let you decide what you want to do with this PR.

@Eredrim Eredrim force-pushed the fix-http-422-rest-serializer branch from 8a5360d to b4c8932 Compare June 12, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: needs triage
Development

Successfully merging this pull request may close these issues.

2 participants