search

mu-semtech / mu-python-template

Template for running Python/Flask microservices
MIT License
4 stars 8 forks source link

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

Open sergiofenoll opened 1 year ago

sergiofenoll commented 1 year ago

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.

MikiDi commented 1 year ago

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 commented 1 year ago

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 commented 11 months ago

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

sergiofenoll commented 11 months ago

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.

madnificent commented 11 months ago

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 commented 11 months ago

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.

madnificent commented 11 months ago

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.