After upgrade to 2.6.4 - swagger schema uses jsonld for all media types

[guilliamxavier]

API Platform version(s) affected: 2.6.4 (regression from 2.6.3)

Description
(extracted from #4212)
The generated OpenAPI docs use the same schemas for content mime types application/json (and application/html) as for application/ld+json

How to reproduce
Default config, Dummy resource (id, name)

Possible Solution
After #4213, move both occurrences of

            $schema = new Schema('openapi');
            $schema->setDefinitions($schemas);

to inside respectively
foreach ($responseMimeTypes as $operationFormat) { and
foreach ($requestMimeTypes as $operationFormat) {
(just before each call to $this->jsonSchemaFactory->buildSchema(/*...*/))

Additional Context

(the bug is not in Swagger-UI but really in the generated docs.json)

[guilliamxavier]

Now that's interesting... I'm using the default config + denormalizationContext={"allow_extra_attributes": false}, and even after both fixes (which I confirm are effective), the generated schemas don't have "additionalProperties": false.
But if I customize the config like this:

api_platform:
    formats:
        jsonld: ['application/ld+json']
    patch_formats:
        json: ['application/merge-patch+json']

then [well, I lose docs.json and Swagger UI, but I can still run the api:openapi:export command, and] the generated schemas do have "additionalProperties": false 🤔
I have also found out that, after coming back to the default config, I can get "additionalProperties": false by removing the $schema->setDefinitions($schemas); call. Given that it was introduced by a commit titled "Improve openapi performances", maybe it is actually not essential and would be better reverted? I'm not sure of the implications... @soyuka?

[soyuka]

Weird as

core/src/JsonSchema/SchemaFactory.php

Lines 120 to 124 in c81eaae

	// additionalProperties are allowed by default, so it does not need to be set explicitly, unless allow_extra_attributes is false
	// See https://json-schema.org/understanding-json-schema/reference/object.html#properties
	if (false === ($serializerContext[AbstractNormalizer::ALLOW_EXTRA_ATTRIBUTES] ?? true)) {
	$definition['additionalProperties'] = false;
	}

should work your way.

For the patch format to me the only lines modifying something are here: https://github.com/api-platform/core/blob/main/src/Serializer/SerializerContextBuilder.php#L104-L116

From what I know having $schema->setDefinitions($schemas); allows to re-use previously generated schemas (as this is really time-consuming for properties). Maybe that the context is lost in the way?

[guilliamxavier]

Weird indeed 😔 Speaking of context, 49ce534 replaced $context with null (and added , $forceSchemaCollection) in both calls to $this->jsonSchemaFactory->buildSchema(/*...*/) in the body of private function collectPaths, making the array $context parameter not used anymore; but I have tried reverting these null back to $context and it doesn't seem to fix anything, worse actually it cancels the effect of removing $schema->setDefinitions($schemas); 😕 Anyway, maybe I should open a separate issue?

[soyuka]

I wonder though how the schemas differ per-type, maybe that alltogether jsonld should not be a part of the schema name?

[planim]

I came here with same issue, but i have different solution (Im not sure if this is ok, but it solves also #4212):

I think the real issue is that \ApiPlatform\Core\OpenApi\Model\Schema extends \ArrayObject, which makes to pass all $schema objects by reference. To fix this i changed in SchemaFactory.php#L72
line from:
$schema = $schema ?? new Schema();
to
$schema = clone $schema ?? new Schema();

Optionally (simpler solution to fix only this ticket):
I think those two lines added in (link: pull request) OpenApiFactory.php#L258
$schema = new Schema('openapi'); and $schema->setDefinitions($schemas);

should be moved to both foreach loops OpenApiFactory.php#L163 and
OpenApiFactory.php#L163

[guilliamxavier]

I came here with same issue, but i have different solution (Im not sure if this is ok, but it solves also #4212):

I think the real issue is that \ApiPlatform\Core\OpenApi\Model\Schema extends \ArrayObject, which makes to pass all $schema objects by reference. To fix this i changed in SchemaFactory.php#L72
line from:
$schema = $schema ?? new Schema();
to
$schema = clone $schema ?? new Schema();

Mind operator precedence! This is evaluated as
$schema = (clone $schema) ?? new Schema();
which is wrong (and will crash when $schema is null). You must group explicitly
$schema = clone ($schema ?? new Schema());
but that will do $schema = clone new Schema(); (unnecessary clone) when $schema is null. Maybe better would be
$schema = null !== $schema ? clone $schema : new Schema();

But that would also change the behavior of public function buildSchema, and e.g.

core/src/Swagger/Serializer/DocumentationNormalizer.php

Line 640 in d62e86e

$this->jsonSchemaFactory->buildSchema($resourceClass, $format, $type, $operationType, $operationName, $schema, $serializerContext, $forceCollection);

would need to be changed (prepend $schema = )... On the other hand, it's already used like that elsewhere, notably

core/src/Hydra/JsonSchema/SchemaFactory.php

Line 57 in d62e86e

$schema = $this->schemaFactory->buildSchema($className, $format, $type, $operationType, $operationName, $schema, $serializerContext, $forceCollection);

(for jsonld), so maybe that would be for the best after all?

An alternative would be to leave this function as is but change $schema to clone $schema in calls (in both foreach loops inside private function collectPaths).

Thanks for the idea 🙂 WDYT @soyuka?

Optionally (simpler solution to fix only this ticket):
I think those two lines added in (link: pull request) OpenApiFactory.php#L258
$schema = new Schema('openapi'); and $schema->setDefinitions($schemas);

should be moved to both foreach loops OpenApiFactory.php#L163 and
OpenApiFactory.php#L269

That's exactly what I suggested under "Possible Solution" 😉 (and note that it would fix not only this ticket but also #4212, too)

[planim]

I agree with You that
$schema = null !== $schema ? clone $schema : new Schema();
is best solution and is exactly what i had in mind :)

[soyuka]

This looks correct to me as the schema is altered and should not alter the reference. This would help managing a schema per formats so definitely 👍 for the clone.

[guilliamxavier]

So I have submitted a PR #4247 to fix the jsonld issue, and opened a separate issue #4248 for additionalProperties