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

[JsonSchema] Only build schemas which can be serialized / deserialied #6276

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GeLoLabs
Copy link
Contributor

@GeLoLabs GeLoLabs commented Apr 1, 2024

Q A
Branch? main
Tickets None
License MIT
Doc PR None

Hello,

This PR is a first step for a discussion about a bugfix.

Given the following ApiResource:

#[ApiResource(operations: [new Delete()])]
class MyResource
{
}

If I generate the OpenAPI documentation using php bin/console api:openapi:export, the ApiPlatform\OpenApi\Factory\OpenApiFactory will generate a doc including the components "MyResource" whereas no operation references it (request / response -> no content).

Futhermore, it exposes all MyResource properties which should stay internal as it is not part of our API.

I would expect that the OpenApiFactory only builds schemas that operation can deserialize / serialize and this is what this PR does.

Now, to fix my OpenAPI doc, I can just do the following:

#[ApiResource(operations: [new Delete(
    deserialize: false,
    serialize: false,
)])]
class MyResource
{
}

WDYT ?

@soyuka
Copy link
Member

soyuka commented Apr 2, 2024

use openapi: false?

@GeLoLabs
Copy link
Contributor Author

GeLoLabs commented Apr 2, 2024

@soyuka Using the flag openapi: false would remove the operation from the OpenAPI spec.

What I want is to keep the operation in the OpenAPI spec but skip the associated schema as my DELETE operation does not have request body as well as no response body (204).

Currenlty, it includes the schema of the class tagged as ApiResource in the OpenAPI spec.

@soyuka
Copy link
Member

soyuka commented Apr 3, 2024

Futhermore, it exposes all MyResource properties which should stay internal as it is not part of our API.

This is quite wrong if a class is marked as a resource it is a public API. I like your patch though, is it possible to add a test on that though?

@GeLoLabs
Copy link
Contributor Author

GeLoLabs commented Apr 4, 2024

@soyuka I mean it exposes unwanted properties as I use deserialization / serialization groups everywhere on this ApiResource for all its operations except for the delete operation. Then, it ends by adding the "root" resource (not specific to a (de)serializer group) with all is properties, something I don't want.

I will complete this PR with some tests.

@@ -65,6 +65,14 @@ public function buildSchema(string $className, string $format = 'json', string $

[$operation, $serializerContext, $validationGroups, $inputOrOutputClass] = $metadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

To save you digging too hard, and easing the rebase, after my last PR these are now assigned here:

if (!$this->isResourceClass($className)) {
$operation = null;
$inputOrOutputClass = $className;
$serializerContext ??= [];
} else {
$operation = $this->findOperation($className, $type, $operation, $serializerContext);
$inputOrOutputClass = $this->findOutputClass($className, $type, $operation, $serializerContext);
$serializerContext ??= $this->getSerializerContext($operation, $type);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I just rebase my PR

@GeLoLabs
Copy link
Contributor Author

GeLoLabs commented Jun 1, 2024

@soyuka @GwendolenLynch I'm not familiar with the project test suite. Can you point me where / how I can add tests about this feature ?

@GeLoLabs GeLoLabs force-pushed the schema-factory-insert-unwanted-schema branch from 6e9c3d9 to 97093c0 Compare June 1, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants