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

feat: follow the same JSON:API structure for errors as the JS template #7

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sergiofenoll
Copy link

@sergiofenoll sergiofenoll commented Nov 16, 2022

In the JS template errors get a message passed in which is returned as the title field of the JSON:API response object.

Note that the JS template should also pass status as a member of the error object, as happens here in the Python template, but it currently doesn't.

In the [JS template](https://github.com/mu-semtech/mu-javascript-template/blob/master/helpers/mu/server.js#L30-L35)
errors get a message passed in which is returned as the title member.

Note that the JS template should also pass status as a member of the
error obejct, as happens here, but it currently doesn't.
@MikiDi
Copy link

MikiDi commented Mar 24, 2023

For consistency with JS template I agree, but on looking closer I remember why doing it this way: the spec states that either one is ok (as a minimum), but that title should be a summary of the problem that SHOULD NOT change from occurrence to occurrence of the problem, which imo is absolutely not guaranteed, since this is an argument provided by the template consumer.

@sergiofenoll
Copy link
Author

I interpreted that as title contains a generic explanation of what went wrong (e.g. File upload failed) and detail contains something like File with id XXX is too large (size YYY of max size ZZZ). So that there are "classes" of errors based on both the status and the title. In practice title would contain some hardcoded string whereas detail would contain call specific info if applicable.

Either way one way or another we're always going to be passing these as parameters, right? I think here's it's just a case of developers needing to be consistent. I don't see much of a point in having a title field that can't be set from application code.

@madnificent
Copy link
Member

Should we support detail, title and status? Should also be added to the mu-javascript-template (but that's a separate topic then).

@sergiofenoll
Copy link
Author

Should we support detail, title and status? Should also be added to the mu-javascript-template (but that's a separate topic then).

Passing in kwargs lets callers provide whatever field they want (maybe we don't actually want that, and we only want to support whatever fields JSON:API defines for valid error responses).

Personally, I'd at the very least add all three field you mention, a "generic" title that shouldn't variate from one error instance to another, a specific detail that may contain call-specific information, and the status that contains the HTTP status code.

It seems to make sense to describe the specific model that may be
followed because we had to look it up ourselves.  Updating the
implementation to that.
@madnificent
Copy link
Member

I got bitten by too small of a context when looking at the change. This was correct indeed.

I've added the currently allowed keys explicitly. I had to look up the available options and I expect others will need to do same.

Not sure if we should still accept *kwargs for backwards compatibility. Thoughts?

@sergiofenoll
Copy link
Author

I got bitten by too small of a context when looking at the change. This was correct indeed.

I've added the currently allowed keys explicitly. I had to look up the available options and I expect others will need to do same.

Not sure if we should still accept *kwargs for backwards compatibility. Thoughts?

🙌 Thanks for the changes!

Maybe leave in kwargs for now but log that it's deprecated and drop it whenever you're planning to do a major release?

Though if I had to guess, the python template probably doesn't see nearly as much use as the other ones, so this might never have been called with kwargs yet.

The error helper now has deprecated support for **kwargs.  Any argument
that does not belong to the runtime will be warned about.
@madnificent
Copy link
Member

Would now look like this in the console:

[DEPRECATION] Supplying args not supported by jsonapi to error helper is deprecated and support will be removed, received cake => is a lie

If that looks good enough then we can pull in master and merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants