-
-
Notifications
You must be signed in to change notification settings - Fork 894
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(openapi): add error resources schemes #6332
feat(openapi): add error resources schemes #6332
Conversation
This is nuts! Love it! Will review shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small change
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. |
Please don't close this, this is a much needed feature. |
I'm so sorry I completely forgot about this, I'll get back to it ASAP |
ef96434
to
de3e2cb
Compare
@@ -88,6 +90,13 @@ private function buildResourceOperations(array $metadataCollection, string $reso | |||
foreach ($resource->getOperations() ?? new Operations() as $operation) { | |||
[$key, $operation] = $this->getOperationWithDefaults($resource, $operation); | |||
$operations[$key] = $operation; | |||
if (null !== $operation->getErrors()) { | |||
foreach ($operation->getErrors() as $error) { | |||
if (!(new $error()) instanceof ProblemExceptionInterface) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_a($error, ProblemExceptionInterface::class, true)
if ($operation instanceof HttpOperation && null !== $operation->getErrors()) { | ||
foreach ($operation->getErrors() as $error) { | ||
/** @var ProblemExceptionInterface $exception */ | ||
$exception = new $error(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that we should call the constructor here, I'd suggest to use $operation = $this->resourceMetadataCollectionFactory->create($error::class)->getOperation()
and to read $operation->getStatus()
although I prefer the getStatus
on that instance but I'm afraid we can't enforce that:
- the constructor has no arguments
getStatus
always exists (or we could've used Reflection->newInstanceWithoutConstructor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be mistaken, but won't $this->resourceMetadataCollectionFactory->create($error::class)->getOperation()
throw a OperationNotFoundException
for an ErrorResource ?
I do agree that the new $error()
is itself error-prone if the constructor has arguments, I had not thought of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrorResources
declares an ApiResource with an Error operation.
$this->appendSchemaDefinitions($schemas, $operationErrorSchema->getDefinitions()); | ||
} | ||
|
||
$openapiOperation = $this->buildOpenApiResponse($existingResponses, $exception->getStatus(), $exception->getType(), $openapiOperation, $operation, $responseMimeTypes, $operationErrorSchemas, $resourceMetadataCollection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type
on problem details is:
The "type" member is a JSON string containing a URI reference [URI] that identifies the problem type. Consumers MUST use the "type" URI (after resolution, if necessary) as the problem type's primary identifier.When this member is not present, its value is assumed to be "about:blank".
https://datatracker.ietf.org/doc/html/rfc9457#name-type
we should probably use something else like $operation->getDescription()
or $exception->getDetails()
.
7d33a40
to
28b0c42
Compare
$this->appendSchemaDefinitions($schemas, $operationErrorSchema->getDefinitions()); | ||
} | ||
|
||
$openapiOperation = $this->buildOpenApiResponse($existingResponses, $exception->getStatus(), $exception->getDetail(), $openapiOperation, $operation, $responseMimeTypes, $operationErrorSchemas, $resourceMetadataCollection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$openapiOperation = $this->buildOpenApiResponse($existingResponses, $exception->getStatus(), $exception->getDetail(), $openapiOperation, $operation, $responseMimeTypes, $operationErrorSchemas, $resourceMetadataCollection); | |
$openapiOperation = $this->buildOpenApiResponse($existingResponses, $exception->getStatus(), $exception->getTitle(), $openapiOperation, $operation, $responseMimeTypes, $operationErrorSchemas, $resourceMetadataCollection); |
if ($operation instanceof HttpOperation && null !== $operation->getErrors()) { | ||
foreach ($operation->getErrors() as $error) { | ||
/** @var ProblemExceptionInterface $exception */ | ||
$exception = (new \ReflectionClass($error))->newInstanceWithoutConstructor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's catch ReflectionException and fallback on something else:
$status = null; $description = null;
try {
$exception = (new \ReflectionClass($error))->newInstanceWithoutConstructor();
$status = $exception->getStatus();
$title = $exception->getTitle();
} catch (\ReflectionException) {
}
if (!$status || !$description) {
// try to get the operation
}
//below
if (!$status || !$description) {
// log that error is misconfigured ?
}
$openapiOperation = $this->buildOpenApiResponse($existingResponses, $status, $title, $openapiOperation, $operation, $responseMimeTypes, $operationErrorSchemas, $resourceMetadataCollection);
2e9410a
to
01846bd
Compare
Allow linking ErrorResources to ApiResources to automatically generate openapi documentation for errors
f626998
to
e320856
Compare
e320856
to
6693402
Compare
$description = null; | ||
try { | ||
/** @var ProblemExceptionInterface */ | ||
$exception = (new \ReflectionClass($error))->newInstanceWithoutConstructor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new $error();
should do the trick. No need for reflection here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, it's possible that an exception in userland has constructor arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then constructing an instance without calling the constructor will likely create an object in an invalid sate, which is worse than an error right?
What if the title is passed as parameter in constructor for instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we fallback to the error operation
$status ??= $errorOperation->getStatus(); | ||
$description ??= $errorOperation->getDescription(); | ||
} catch (ResourceClassNotFoundException|OperationNotFoundException) { | ||
$this->logger?->warning(\sprintf('The error class %s is not an ErrorResource', $error)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. It's probably useful to not catch this exception and be sure the developer handles this case.
Cool feature! |
64f7fbe
to
ad8bd49
Compare
1d69c55
to
649adf2
Compare
649adf2
to
f51203e
Compare
f51203e
to
6bbd6c2
Compare
Many thanks @JacquesDurand ! We need some documentation for that awesome feature! |
Allow linking ErrorResources to ApiResources to automatically generate openapi documentation for errors:
with the following Exception (currently needs to be an
ErrorResource
and to implementProblemExceptionInterface
):would allow the following:
Do you think it might be an interesting feature ?
There might still be a few things I am not too familiar with, any advice is more than welcome !