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

fix: ensure valid jsonapi error members are accessible #8669

Closed
wants to merge 1 commit into from
Closed

fix: ensure valid jsonapi error members are accessible #8669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 26 additions & 17 deletions packages/legacy-compat/src/legacy-network-handler/legacy-network-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ function adapterDidInvalidate(
if (serializer && typeof serializer.extractErrors === 'function') {
let errorsHash = serializer.extractErrors(store, store.modelFor(identifier.type), error, identifier.id) as Record<
string,
string | string[]
string | string[] | JsonApiError[]
>;
error.errors = errorsHashToArray(errorsHash);
Comment on lines 232 to 236
Copy link
Author

@Techn1x Techn1x Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
Expand All @@ -256,31 +256,40 @@ function adapterDidInvalidate(
}
}

function makeArray<T>(value: T | T[]): T[] {
return Array.isArray(value) ? value : [value];
}

const PRIMARY_ATTRIBUTE_KEY = 'base';
function errorsHashToArray(errors: Record<string, string | string[]>): JsonApiError[] {
function errorsHashToArray(errorsHash: Record<string, string | string[] | JsonApiError[]>): JsonApiError[] {
const out: JsonApiError[] = [];

if (errors) {
Object.keys(errors).forEach((key) => {
let messages = makeArray(errors[key]);
for (let i = 0; i < messages.length; i++) {
if (errorsHash) {
Object.keys(errorsHash).forEach((key) => {
let errorsItem = errorsHash[key]
let errors = typeof errorsItem === 'string' ? [errorsItem] : errorsItem;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

for (let i = 0; i < errors.length; i++) {
let title = 'Invalid Attribute';
let pointer = `/data/attributes/${key}`;
if (key === PRIMARY_ATTRIBUTE_KEY) {
title = 'Invalid Document';
pointer = `/data`;
}
out.push({
title: title,
detail: messages[i],
source: {
pointer: pointer,
},
});
let error = errors[i]
if (typeof error === 'string') {
out.push({
title: title,
detail: error,
source: {
pointer: pointer,
},
});
Comment on lines +275 to +282
Copy link
Author

Choose a reason for hiding this comment

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

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

} else {
out.push({
...error,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

title: title,
detail: error.detail || error.title,
source: {
pointer: pointer,
},
Comment on lines +283 to +290
Copy link
Author

Choose a reason for hiding this comment

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

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.

});
}
}
});
}
Expand Down
32 changes: 19 additions & 13 deletions packages/serializer/src/json.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1476,19 +1476,25 @@ const JSONSerializer = Serializer.extend({
const extracted = {};

payload.errors.forEach((error) => {
if (error.source && error.source.pointer) {
let key = error.source.pointer.match(SOURCE_POINTER_REGEXP);

if (key) {
key = key[2];
} else if (error.source.pointer.search(SOURCE_POINTER_PRIMARY_REGEXP) !== -1) {
key = PRIMARY_ATTRIBUTE_KEY;
}

if (key) {
extracted[key] = extracted[key] || [];
extracted[key].push(error.detail || error.title);
}
const errorPointer = error.source?.pointer ?? '/data'
Copy link
Author

Choose a reason for hiding this comment

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

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:

let key = errorPointer.match(SOURCE_POINTER_REGEXP);
if (key) {
key = key[2];
} else if (errorPointer.search(SOURCE_POINTER_PRIMARY_REGEXP) !== -1) {
key = PRIMARY_ATTRIBUTE_KEY;
}
if (key) {
extracted[key] = extracted[key] || [];
// 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
});
Comment on lines +1488 to +1497
Copy link
Author

Choose a reason for hiding this comment

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

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.

}
});

Expand Down