fix: ensure valid jsonapi error members are accessible #8669

Techn1x · 2023-07-05T01:14:14Z

Description

An attempt to resolve issues detailed in #8188

Notes for the release

The error handling was changed in 4.7.3 to require the use of these functions, so I think this fix for those functions to correctly parse jsonapi error members should be backported to 4.8 LTS and 4.12 LTS

I think this fix is important, as it will help folks get to 4.8 and beyond to ember-data 5.

I would also like to point out the Ember guides are incorrect, but I'm not sure where/how to edit those at this time.
https://jsonapi.org/format/#error-objects spec says it should be /data but the guides say data
https://api.emberjs.com/ember-data/4.12/classes/JSONAPISerializer/methods/extractErrors?anchor=extractErrors

Techn1x · 2023-07-05T01:17:52Z

packages/serializer/src/json.js

+          // Keep only what are valid attr's for a jsonapi error
+          extracted[key].push({
+            id: error.id,
+            links: error.links ? { about: error.links.about, type: error.links.type } : undefined,
+            status: error.status,
+            code: error.code,
+            detail: error.detail,
+            title: error.title,
+            meta: error.meta
+          });


Each of these keys might be undefined. That's a bit messy, and I would clean this up before seeking final approval / merge by only defining keys that are set in the error.

At this stage I just put this together to demonstrate the approach and show that it works.

Techn1x · 2023-07-05T01:18:59Z

packages/legacy-compat/src/legacy-network-handler/legacy-network-handler.ts

      let errorsHash = serializer.extractErrors(store, store.modelFor(identifier.type), error, identifier.id) as Record<
        string,
-        string | string[]
+        string | string[] | JsonApiError[]
      >;
      error.errors = errorsHashToArray(errorsHash);


our internal extractErrors() hook now returns JsonApiError[] but is still backwards compatible with hooks that users might have defined that return string | string[]

Techn1x · 2023-07-05T01:20:21Z

packages/legacy-compat/src/legacy-network-handler/legacy-network-handler.ts

+        if (typeof error === 'string') {
+          out.push({
+            title: title,
+            detail: error,
+            source: {
+              pointer: pointer,
+            },
+          });


backwards compatible - this should produce the exact same error as before if their extractErrors hook returned a string or string[]

Techn1x · 2023-07-05T01:22:15Z

packages/legacy-compat/src/legacy-network-handler/legacy-network-handler.ts

+        } else {
+          out.push({
+            ...error,
+            title: title,
+            detail: error.detail || error.title,
+            source: {
+              pointer: pointer,
+            },


If the full JsonApiError array was handed to this function, we produce an error that contains all the other (spec-complaint) error members, but we make sure it still has the title/detail/source as this function produced before.

Techn1x · 2023-07-05T01:25:32Z

packages/serializer/src/json.js

-            extracted[key] = extracted[key] || [];
-            extracted[key].push(error.detail || error.title);
-          }
+        const errorPointer = error.source?.pointer ?? '/data'


see: https://jsonapi.org/format/#error-objects
allows for source to be omitted

source: an object containing references to the primary source of the error. It SHOULD include one of the following members or be omitted:

runspired · 2023-07-06T06:08:56Z

Overall +1 on the approach, lets refine and add some tests.

runspired · 2023-07-06T06:12:30Z

packages/legacy-compat/src/legacy-network-handler/legacy-network-handler.ts

+  if (errorsHash) {
+    Object.keys(errorsHash).forEach((key) => {
+      let errorsItem = errorsHash[key]
+      let errors = typeof errorsItem === 'string' ? [errorsItem] : errorsItem;


probably we should assert if not an array and not a string

runspired · 2023-07-06T06:13:49Z

packages/legacy-compat/src/legacy-network-handler/legacy-network-handler.ts

+          });
+        } else {
+          out.push({
+            ...error,


the only thing about this that gives me pause is that this results in a grey middle ground of having passed the error through unchanged but also having changed it into json:api form.

But since the overall recommendation is to not have the extractErrors hook (and also more broadly we are beginning to move away from these legacy patterns) this seems fine as a compromise.

runspired · 2023-07-31T23:44:42Z

@Techn1x were you going to continue on this one?

Techn1x · 2023-08-01T06:15:51Z

@runspired I think I'm going to drop it. Since then, I've just set extractErrors = false in my jsonapi application serializer as a workaround. I figure it's legacy / going away, so I guess that will do.

Happy for you to close this, or carry it forward, up to you.

At the time I pushed this I was a little frustrated at the LTS versions of ember-data;

4.4 didn't work due to known compatibility issues between ember-concurrency and ember-data 4.4
4.8 didn't work with embroider (and big error handling changes ^ that were made from 4.7.3 onwards, unmentioned in changelog)
4.12 still required big error handling changes

Our upgrade path was 3.28 -> 4.6 (back in Dec 2022), and then 4.6 -> 4.12 (about a month ago). I was having to avoid the 4.4/4.8 LTS versions to get the fixes I needed, which doesn't make a whole lot of sense.

So I was keen to fix the error handling thing without needing to rewrite my backend, and give back to help improve the LTS versions specifically because of the pain I went through.

But... time passed and I've come around to just setting extractErrors = false even though I'm not supposed to know that internal logic thing. It doesn't seem to affect that many people, and those that it does affect can just do the same thing, so I've lost the drive to get this across the line

I've been running 4.12 for a while now (with extractErrors = false), it's working great btw. Looking forward to 5

Techn1x · 2023-08-01T06:16:47Z

Thanks for reviewing the PR and being receptive to the changes ❤️

runspired · 2023-08-01T07:04:22Z

@Techn1x totally get it. I do think 3.28 -> 4.4 -> 4.6 -> 4.12 was a better path than 4.8, I tried hard to patch 4.8 but ultimately the shift's effects took till 4.12 to fully shake out. TBH the EC issue was a big part of that, it was hard to figure out the right fix - we tried a lot of things - and it was also annoying to be adding a hack to work around what an external library shouldn't have been doing in the first place.

But... time passed and I've come around to just setting extractErrors = false even though I'm not supposed to know that internal logic thing.

I think you missed what I said about it here in the original issue: #8188 (comment)

Or maybe it's the nuance. extractErrors was never part of the public API. In an ideal world, the default for most people should be that they were never using it and it didn't exist on their serializer. For these people, nothing breaks.

Where idealism broke down is too many people blindly extended RESTSerializer and JSONApiSerializer when they should have written their own. But even for these folks if they were only using errors the way that EmberData had intended to surface errors then also nothing breaks.

You fell into a camp that I hoped no one was in (and still suspect not many people are in) where you did not write your own serializer and also did not use errors the way EmberData had intended to surface them. The unfortunate reality of Javascript being that whether something is documented or not if you stick a property on an object people will see it.

I'm still obviously sympathetic and very willing to ease the path for the next poor fellow, but also as you noted the fix really is as simple as "delete that method from your serializer and move on".

Techn1x · 2023-08-01T09:43:52Z

I tried hard to patch 4.8 but ultimately the shift's effects took till 4.12 to fully shake out

Yeah I can only imagine the challenges of trying to backport certain fixes in any repo that has LTS versions..

I think you missed what I said about it here in the original issue: #8188 (comment)

I didn't miss it, but may have misinterpreted this line
"I suspect the real solution here is for you to simply delete the serializer's extractErrors method as it serves you no purpose it seems and is the reason this property is lost to you."

Didn't realise (at the time) that I had to actually set extractErrors to false to override the function set in JSONAPISerializer that I was/am extending

Where idealism broke down is too many people blindly extended RESTSerializer and JSONApiSerializer when they should have written their own.

I thought that was what we're supposed to do... definitely been doing that for many years and seen it at multiple workplaces. Seems the guides indicate it too, but I suspect I might be misunderstanding your words here
https://guides.emberjs.com/release/models/customizing-serializers/#toc_customizing-serializers

You fell into a camp that I hoped no one was in (and still suspect not many people are in) where you did not write your own serializer and also did not use errors the way EmberData had intended to surface them.

💯 and I understand that now

runspired · 2023-08-21T08:30:13Z

I thought that was what we're supposed to do... definitely been doing that for many years and seen it at multiple workplaces. Seems the guides indicate it too, but I suspect I might be misunderstanding your words here

The guides have never been in the control of the project, and unfortunately the folks that wrote them felt differently than the library authors on what constituted a good experience. The guides largely attempt to minimize setup, configuration and writing code. I find this unrealistic when it comes to APIs, every API is very different and has nuances only its authors can understand.

fix: ensure valid jsonapi error members are accessible

1ea88c7

Techn1x commented Jul 5, 2023

View reviewed changes

Techn1x mentioned this pull request Jul 5, 2023

Store service filters erros provided in exception #8188

Open

runspired reviewed Jul 6, 2023

View reviewed changes

runspired added 🎯 canary PR is targeting canary (default) 🏷️ feat This PR introduces a new feature good-first-issue labels Sep 29, 2023

Techn1x closed this Nov 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure valid jsonapi error members are accessible #8669

fix: ensure valid jsonapi error members are accessible #8669

Techn1x commented Jul 5, 2023 •

edited

Loading

Techn1x Jul 5, 2023

Techn1x Jul 5, 2023 •

edited

Loading

Techn1x Jul 5, 2023

Techn1x Jul 5, 2023

Techn1x Jul 5, 2023

runspired commented Jul 6, 2023

runspired Jul 6, 2023

runspired Jul 6, 2023

runspired commented Jul 31, 2023

Techn1x commented Aug 1, 2023

Techn1x commented Aug 1, 2023

runspired commented Aug 1, 2023

Techn1x commented Aug 1, 2023

runspired commented Aug 21, 2023

fix: ensure valid jsonapi error members are accessible #8669

fix: ensure valid jsonapi error members are accessible #8669

Conversation

Techn1x commented Jul 5, 2023 • edited Loading

Description

Notes for the release

Techn1x Jul 5, 2023

Choose a reason for hiding this comment

Techn1x Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Techn1x Jul 5, 2023

Choose a reason for hiding this comment

Techn1x Jul 5, 2023

Choose a reason for hiding this comment

Techn1x Jul 5, 2023

Choose a reason for hiding this comment

runspired commented Jul 6, 2023

runspired Jul 6, 2023

Choose a reason for hiding this comment

runspired Jul 6, 2023

Choose a reason for hiding this comment

runspired commented Jul 31, 2023

Techn1x commented Aug 1, 2023

Techn1x commented Aug 1, 2023

runspired commented Aug 1, 2023

Techn1x commented Aug 1, 2023

runspired commented Aug 21, 2023

Techn1x commented Jul 5, 2023 •

edited

Loading

Techn1x Jul 5, 2023 •

edited

Loading