-
-
Notifications
You must be signed in to change notification settings - Fork 936
fix(jsonSchema): Declare properties as required for Output #7481
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
base: main
Are you sure you want to change the base?
Conversation
VincentLanglet
commented
Oct 23, 2025
| Q | A |
|---|---|
| Branch? | 4.2 for bug |
| Tickets | Closes #7457 |
| License | MIT |
| Doc PR | api-platform/docs#... |
5ae2b67 to
0e35815
Compare
| $propertyMetadata = $this->propertyMetadataFactory->create($inputOrOutputClass, $propertyName, $options); | ||
|
|
||
| if (false === $propertyMetadata->isReadable() && false === $propertyMetadata->isWritable()) { | ||
| if ( |
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.
Looking at the implementation of OpenApiFactory, there is
$this->jsonSchemaFactory->buildSchema($resourceClass, $operationFormat, Schema::TYPE_OUTPUT, $operation, $schema, null, $forceSchemaCollection);
for the output of an api call (for instance GET)
and
$this->jsonSchemaFactory->buildSchema($resourceClass, $operationFormat, Schema::TYPE_INPUT, $operation, $schema, null, $forceSchemaCollection);
for the input of an api call PATCH/PUT/POST
Therefore I think
- We should expose in the input schema only Writable properties
- We should expose in the output schema only Readable properties (and declare them required)
It makes no sens to me to add writable property in the GET schema (or the opposite). Am i misunderstandind ?
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 should not expose required in my opinion as it locks the schema (for example with the PropertyFilter or GroupFilter, results can be altered).
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.
Oh I understand the issue then. it's hard to document both
- call which doesn't use any propertyFilter
- and calls which are using some
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.
Still it means that we shouldn't use the same schema for input and output and therefor the same cache key.
Should we start with a different cache key without changing anything?
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.
whats the problem with the cache key ?
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.
The cache key is the definition name
core/src/JsonSchema/SchemaFactory.php
Line 113 in 8c27606
| if (isset($definitions[$definitionName])) { |
which is generated without the schema type (input or output)
core/src/JsonSchema/SchemaFactory.php
Line 86 in 8c27606
| $definitionName = $this->definitionNameFactory->create($className, $format, $inputOrOutputClass, $operation, $serializerContext); |
But you can see the type is used later
- as an option for the propertyMetadataFactory
core/src/JsonSchema/SchemaFactory.php
Line 142 in 8c27606
$options = ['schema_type' => $type] + $this->getFactoryOptions($serializerContext, $validationGroups, $operation instanceof HttpOperation ? $operation : null); - as an arg for the buildPropertySchema
core/src/JsonSchema/SchemaFactory.php
Line 158 in 8c27606
$this->buildPropertySchema($schema, $definitionName, $normalizedPropertyName, $propertyMetadata, $serializerContext, $format, $type);
So we could have different values for SchemaFactory::buildSchema(..., Schema::TypeInput and SchemaFactory::buildSchema(..., Schema::TypeOutput but because of the current cache key, as soon as the buildSchema is call with TypeInput, a call with TypeOutput will give the same result.
0e35815 to
d74a7d7
Compare
d74a7d7 to
c5a81e2
Compare
|
Looking at the number of the test to update, I think it will be better to get your opinion on this first @soyuka |