Issues generating multiple schema formats when decorating SchemaFactory

[GwendolenLynch]

API Platform version(s) affected: 3.2

Description
I am working on a PR to introduce a JSON:API schema factory and hit a couple of related bugs that feel better suited to a separate issue/PR combination.

I have working "fixes" laid out below, but would like them reviewed & sanity checked before I create a PR for this.

First problem was that ApiPlatform\Hydra\JsonSchema\SchemaFactory#addDistinctFormat() checks for ApiPlatform\JsonSchema\SchemaFactory, where as ApiPlatform\Hal\JsonSchema\SchemaFactory simply does a method_exists call.

No priority is set on any of the service decorators, and Hydra's, by chance, is currently the one with the base service .inner. However, my new service took first place and it became impossible to build both schema formats at the same time as the last one "won out". Changing Hydra's method behaviour to be a method_exists call fixes this.

The second is just as subtle. The base ApiPlatform\JsonSchema\SchemaFactory calls the injected decorator service to enable subschema generation (see #6055). When the Hydra service is the one decorating the base this works fine, but the introduction of another service causes that one to be called instead, meaning Hydra misses out (see #5950)

How to reproduce

final class SchemaFactory implements SchemaFactoryInterface
{
    public function __construct(private readonly SchemaFactoryInterface $schemaFactory)
    {
        $this->addDistinctFormat('jsonapi');
        if ($this->schemaFactory instanceof SchemaFactoryAwareInterface) {
            $this->schemaFactory->setSchemaFactory($this);
        }
    }

    public function buildSchema(string $className, string $format = 'jsonapi', string $type = Schema::TYPE_OUTPUT, ?Operation $operation = null, ?Schema $schema = null, ?array $serializerContext = null, bool $forceCollection = false): Schema
    {
        $schema = $this->schemaFactory->buildSchema($className, $format, $type, $operation, $schema, $serializerContext, $forceCollection);

        if ('jsonapi' !== $format) {
            return $schema;
        }

        // Do schema updates here …

        return $schema;
    }

    public function addDistinctFormat(string $format): void
    {
        // The Hydra schema factory does:
        //  if ($this->schemaFactory instanceof SchemaFactoryAwareInterface) {
        //
        // The HAL one does as below:
        if (method_exists($this->schemaFactory, 'addDistinctFormat')) {
            $this->schemaFactory->addDistinctFormat($format);
        }
    }
}

Obligatory service definition 😇

    api_platform.jsonapi.json_schema.schema_factory:
        class: 'ApiPlatform\JsonApi\JsonSchema\SchemaFactory'
        decorates: 'api_platform.json_schema.schema_factory'
        arguments:
            - '@api_platform.jsonapi.json_schema.schema_factory.inner'

And API Platform config:

api_platform:
    formats:
        json: [ 'application/json' ]
        jsonld: [ 'application/ld+json' ]
        jsonhal: [ 'application/hal+json' ]
        jsonapi: [ 'application/vnd.api+json' ]

Possible Solution

1. Change Hydra SchemaFactory#addDistinctFormat() to use a method_exists call
2. Have all SchemaFactorys implement SchemaFactoryAwareInterface 😞

Obviously neither of those are particularly elegant, hence this discussion. Help and guidance most welcome!

[GwendolenLynch]

With respect to the SchemaFactory#addDistinctFormat() problem, changing these lines in the fixture app's config:

core/tests/Fixtures/app/config/config_common.yml

Lines 36 to 39 in 1cc114b

	formats:
	jsonld: ['application/ld+json']
	jsonhal: ['application/hal+json']
	jsonapi: ['application/vnd.api+json']

To something like this:

    formats:
        jsonapi:                       ['application/vnd.api+json']
        jsonhal:                       ['application/hal+json']
        jsonld:                        ['application/ld+json']

Then running vendor/bin/simple-phpunit --filter JsonSchemaGenerateCommandTest:: will make that one trigger, and can be fixed with the method_exists suggestion above.

I'm trying to work out how to add specific tests for this and then I'll PR Suggestions gratefully welcomed.

Edit: fixing above requires both changes.

[soyuka]

First problem was that ApiPlatform\Hydra\JsonSchema\SchemaFactory#addDistinctFormat() checks for ApiPlatform\JsonSchema\SchemaFactory, where as ApiPlatform\Hal\JsonSchema\SchemaFactory simply does a method_exists call.

We definitely missed that when reviewing 1056410. And I really dislike the addDistinctFormat thing, I think that we should:

• register a new configuration jsonschema_formats, that defaults to our formats keys
• inject that in the schema factory

The second is just as subtle. The base ApiPlatform\JsonSchema\SchemaFactory calls the injected decorator service to enable subschema generation (see #6055). When the Hydra service is the one decorating the base this works fine, but the introduction of another service causes that one to be called instead, meaning Hydra misses out (see #5950)

Not sure I got that, the whole decoration chain should be called even in the subschema no?

[GwendolenLynch]

The second is just as subtle. The base ApiPlatform\JsonSchema\SchemaFactory calls the injected decorator service to enable subschema generation (see #6055). When the Hydra service is the one decorating the base this works fine, but the introduction of another service causes that one to be called instead, meaning Hydra misses out (see #5950)

Not sure I got that, the whole decoration chain should be called even in the subschema no?

Yes, it should. What I am observing happen is that the decoration chain I had was HAL -> Hydra -> JSON:API -> JSON, and when JSON would call the sub-schema factory it would short circuit to I think Hydra, missing the others. It all came down to what order the formats: key was in.

[soyuka]

Lmk if you can reproduce that as I don't understand the link between the formats order and the decoration call order.

[GwendolenLynch]

With your suggested changes above I can't reproduce it (decoration call order). I've updated #6236 to incorporate your idea, and rollback the implements changes.

Currently, I have left empty addDistinctFormat() methods. The base one had an @internal annotation, the others not, so I'd appreciate feedback on remove/deprecate decision.

[GwendolenLynch]

OK, after a lot of head scratching, I've got the reproducer! I've made a branch https://github.com/GwendolenLynch/api-platform-core/tree/testcase-6228 with two commits.

To show the bug, revert/drop the second commit and run vendor/bin/simple-phpunit --filter JsonSchemaGenerateCommandTest::testSubSchemaJsonLd

[GwendolenLynch]

Fixed in #6236