Skip to content

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+35 −3
Draft

fix(jsonSchema): Declare properties as required for Output #7481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/JsonSchema/SchemaFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,10 @@ public function buildSchema(string $className, string $format = 'json', string $

$validationGroups = $operation ? $this->getValidationGroups($operation) : [];
$version = $schema->getVersion();

$definitionName = $this->definitionNameFactory->create($className, $format, $inputOrOutputClass, $operation, $serializerContext);
$definitionName .= '.'.$type;

$method = $operation instanceof HttpOperation ? $operation->getMethod() : 'GET';
if (!$operation) {
$method = Schema::TYPE_INPUT === $type ? 'POST' : 'GET';
Expand Down Expand Up @@ -143,12 +146,18 @@ public function buildSchema(string $className, string $format = 'json', string $
foreach ($this->propertyNameCollectionFactory->create($inputOrOutputClass, $options) as $propertyName) {
$propertyMetadata = $this->propertyMetadataFactory->create($inputOrOutputClass, $propertyName, $options);

if (false === $propertyMetadata->isReadable() && false === $propertyMetadata->isWritable()) {
if (
Copy link
Contributor Author

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 ?

Copy link
Member

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).

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

Copy link
Member

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 ?

Copy link
Contributor Author

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.

Schema::TYPE_INPUT === $type && false === $propertyMetadata->isWritable()
|| Schema::TYPE_OUTPUT === $type && false === $propertyMetadata->isReadable()
) {
continue;
}

$normalizedPropertyName = $this->nameConverter ? $this->nameConverter->normalize($propertyName, $inputOrOutputClass, $format, $serializerContext) : $propertyName;
if ($propertyMetadata->isRequired() && !$isJsonMergePatch) {

// Property should be considered as required for output since they are "always" in the response body.
// @see https://github.com/api-platform/core/issues/7457
if (Schema::TYPE_OUTPUT === $type || $propertyMetadata->isRequired() && !$isJsonMergePatch) {
$definition['required'][] = $normalizedPropertyName;
}

Expand Down
3 changes: 3 additions & 0 deletions tests/Functional/JsonSchema/JsonLdJsonSchemaTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public function testSubSchemaJsonLd(): void
'$ref' => '#/definitions/TestEntity.jsonld-read',
]),
],
'required' => ['id', 'description', 'tests', 'nonResourceTests', 'type'],
]),
],
]);
Expand All @@ -105,6 +106,7 @@ public function testSubSchemaJsonLd(): void
],
]),
],
'required' => ['id', 'nullableString', 'nullableInt'],
]);

$expectedTestEntitySchema = new \ArrayObject([
Expand Down Expand Up @@ -132,6 +134,7 @@ public function testSubSchemaJsonLd(): void
],
]),
],
'required' => ['id', 'nullableString', 'nullableInt'],
]),
],
]);
Expand Down
22 changes: 21 additions & 1 deletion tests/Functional/OpenApiTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ public function testErrorsAreDocumented(): void
['$ref' => '#/components/schemas/HydraItemBaseSchema'],
[
'type' => 'object',
'required' => ['title', 'detail', 'status', 'instance', 'type', 'description'],
'properties' => [
'title' => [
'readOnly' => true,
Expand Down Expand Up @@ -243,6 +244,7 @@ public function testHasSchemasForMultipleFormats(): void
['$ref' => '#/components/schemas/HydraItemBaseSchema'],
[
'type' => 'object',
'required' => ['id'],
'properties' => [
'id' => ['type' => 'string'],
],
Expand Down Expand Up @@ -326,7 +328,25 @@ public function testRetrieveTheOpenApiDocumentation(): void
$this->assertArrayHasKey('put', $json['paths']['/api/custom-call/{id}']);

$this->assertArrayHasKey('id', $json['components']['schemas']['Dummy']['properties']);
$this->assertSame(['name'], $json['components']['schemas']['Dummy']['required']);
$this->assertSame([
'id',
'name',
'alias',
'foo',
'description',
'dummy',
'dummyBoolean',
'dummyDate',
'dummyFloat',
'dummyPrice',
'relatedDummy',
'relatedDummies',
'jsonData',
'arrayData',
'name_converted',
'relatedOwnedDummy',
'relatedOwningDummy',
], $json['components']['schemas']['Dummy']['required']);
$this->assertArrayHasKey('genderType', $json['components']['schemas']['Person']['properties']);
$this->assertEquals([
'default' => 'male',
Expand Down
Loading