api-platform / core

The server component of API Platform: hypermedia and GraphQL APIs in minutes
https://api-platform.com
MIT License
2.45k stars 875 forks source link

RFC-7807 compatible hydra error response #4994

Closed nesl247 closed 1 year ago

nesl247 commented 2 years ago

Description

Finally, the server SHOULD provide error descriptions using an [RFC7807] standard by using an application/problem+json response. When doing so, the server also MUST provide an additional header pointing to either the built-in Hydra http://www.w3.org/ns/hydra/error context or any JSON-LD context that maps the terms type, title, detail, status and instance the same way as the standard one.

The existing response is the Non RFC-7807 compliant error description using raw Hydra JSON-LD representation. as described here https://www.hydra-cg.com/spec/latest/core/#example-32-non-rfc-7807-compliant-error-description-using-raw-hydra-json-ld-representation.

dunglas commented 2 years ago

Indeed. Compatibility with RFC7807 is new in Hydra and we didn't updated our implementation since it has been added. PR welcome!

nesl247 commented 2 years ago

@dunglas how would you want it to be implemented? Would it be a BC breaking change or would it need to be a new error format jsonld+problem? Or would it be more appropriate to be called hydra because it's the format described by hydra itself, and not jsonld?

dunglas commented 2 years ago

I think that we can make our current implementation of RFC7807 compatible with Hydra without introducing breaking changes (it's only new keys). Then we'll deprecate the old Hydra error format. WDYT?

nesl247 commented 2 years ago

I think that would work. Let me take a look and submit a PR for that. If I run into any issues I’ll reply back here.

nesl247 commented 2 years ago

I'm starting to look into it, and the bigger problem that we need to figure out is the type property. This MUST be a URI format, which means we need to have some sort of response if someone follows it.

I have the following questions:

Unfortunately it seems while I intended this to be a rather simple change, it's actually quite complicated. We do have the option of skipping type, but that defeats one of the biggest purposes of 7807. Adding just the new detail field doesn't seem like a worthwhile change IMO.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

nesl247 commented 1 year ago

This isn't stale. It's requires feedback.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

soyuka commented 1 year ago

Indeed this is missing feedbacks, @dunglas any idea?

soyuka commented 1 year ago

Hi, we talked about this with @dunglas and we definitely need to be in sync with the specification on this.

Basically this all means that we'll have a new type of resource (ErrorResource). The first step is to add this feature with a default ErrorResource on /errors that'll use the http code as IRI (/errrors/404). Your ErrorResource basically is an ApiResource and it will have a Provider that fetches whatever information we need. I suggest that we have a default ErrorProvider that has indeed the title of the error. Overriding will then be done using a special ErrorResource attribute that one may use on throwables.

How do we add the required Link: http://www.w3.org/ns/hydra/error; rel="http://www.w3.org/ns/json-ld#context" to the headers? The Normalizer doesn't work on the response, just on the body.

You need to add this to the AddLinkHeaderListener.

soyuka commented 1 year ago

@nesl247 if you have some comments please check the PR we started at #5433 would love your insight !

jotwea commented 1 year ago

I realized that PR #5433 also introduced changes on the GraphQL side which are not valid. { "errors": [ { "message": "Names must only contain [_a-zA-Z0-9] but \"hydra:title\" does not.", "extensions": {}, "stack": "GraphQLError: Names must only contain [_a-zA-Z0-9] but \"hydra:title\" does not." } ] }

What would be the desired behaviour for GraphQL? Should the error types also occur there, then we should fix this. If they are not relevant for GraphQL then we should just add graphqlOperations: [] to the #[ErrorResource]s and the schema will be valid again.