fix(ModelRegistry): schemas deduplication now compare generated schemas to reduce automatically named schemas

[xavierleune]

Description

This pull requests implements a better deduplication of schemas. It now compares the generated schemas, so even with different hashes (because of 2 different serializationContexts), we can have only 1 component if the output is identical.

The common use case for me is the following:

<?php

use Nelmio\ApiDocBundle\Attribute\Model;
use OpenApi\Attributes as OA;
use Symfony\Component\Routing\Attribute\Route;
use Symfony\Component\Serializer\Attribute\Groups;

class FirstLevelEntity
{
    #[Groups(['all'])]
    public string $title;

    #[Groups(['default'])]
    public SecondLevelEntity $subEntity;
}

class SecondLevelEntity {
    #[Groups(['default'])]
    public string $title;

    #[Groups(['default'])]
    public string $description;
}

class Controller {
    #[Route('/all', methods: ['GET'])]
    #[OA\Response(response: 200, description: 'all', content: new OA\JsonContent(ref: new Model(type: FirstLevelEntity::class, groups: ['all', 'default'])))]
    public function all(FirstLevelEntity $entity) {

    }

    #[Route('/default', methods: ['GET'])]
    #[OA\Response(response: 200, description: 'default', content: new OA\JsonContent(ref: new Model(type: FirstLevelEntity::class, groups: ['default'])))]
    public function default(FirstLevelEntity $entity) {

    }
}

You can notice that SecondLevelEntity is always included in the response, because of the group "default".
The current output is something like this:

{
    "openapi": "3.0.0",
    "info": {
        "title": "ANS",
        "description": "Another Notification System - CRM & notifications @CCMBG",
        "version": "1.0.0"
    },
    "servers": [
        {
            "url": "https://api.ans.local.ccmbg.com"
        }
    ],
    "paths": {
        "/api/all": {
            "get": {
                "tags": [
                    "Service accounts"
                ],
                "operationId": "get_app_api_private_serviceaccounts_all",
                "responses": {
                    "200": {
                        "description": "all",
                        "content": {
                            "application/json": {
                                "schema": {
                                    "$ref": "#/components/schemas/FirstLevelEntity"
                                }
                            }
                        }
                    }
                }
            }
        },
        "/api/default": {
            "get": {
                "tags": [
                    "Service accounts"
                ],
                "operationId": "get_app_api_private_serviceaccounts_default",
                "responses": {
                    "200": {
                        "description": "default",
                        "content": {
                            "application/json": {
                                "schema": {
                                    "$ref": "#/components/schemas/FirstLevelEntity2"
                                }
                            }
                        }
                    }
                }
            }
        }
    },
    "components": {
        "schemas": {
            "FirstLevelEntity": {
                "required": [
                    "title",
                    "subEntity"
                ],
                "properties": {
                    "title": {
                        "type": "string"
                    },
                    "subEntity": {
                        "$ref": "#/components/schemas/SecondLevelEntity"
                    }
                },
                "type": "object"
            },
            "FirstLevelEntity2": {
                "required": [
                    "subEntity"
                ],
                "properties": {
                    "subEntity": {
                        "$ref": "#/components/schemas/SecondLevelEntity2"
                    }
                },
                "type": "object"
            },
            "SecondLevelEntity": {
                "required": [
                    "title",
                    "description"
                ],
                "properties": {
                    "title": {
                        "type": "string"
                    },
                    "description": {
                        "type": "string"
                    }
                },
                "type": "object"
            },
            "SecondLevelEntity2": {
                "required": [
                    "title",
                    "description"
                ],
                "properties": {
                    "title": {
                        "type": "string"
                    },
                    "description": {
                        "type": "string"
                    }
                },
                "type": "object"
            }
        }
    }
}

Descriptions for SecondLevelEntity and SecondLevelEntity2 are the same (makes sense). With that pull request we get rid of SecondLevelEntity2 and the reference in FirstLevelEntity2 will be SecondLevelEntity.

You may notice a few changes in other tests, the current change alters the order of the Schemas, so the generated names may vary (like User becoming User2 and vice-versa).

What type of PR is this? (check all applicable)

• Bug Fix
• Feature ?
• Refactor
• Deprecation
• Breaking Change
• Documentation Update
• CI

Checklist

• I have made corresponding changes to the documentation (docs/)
• I have made corresponding changes to the changelog (CHANGELOG.md)

[xavierleune]

I missed the release of the version 5.0, congrats to everyone involved 🎉 !
I'm not sure about your policy regarding the version 4.x, for that kind of changes (and for #2435), should I target 4.x or 5.x ?

[xavierleune]

I decided to target 5.x as tests were failing for SF 5.4 ^^

[DjordyKoert]

I'm not sure about your policy regarding the version 4.x, for that kind of changes (and for #2435), should I target 4.x or 5.x ?

Either is fine really 😄
Any PR merged in 4.x will also be commited to 5.x by myself, but I would personally recommend 5.x simply because the codebase is easier to work with (php 8.1, no more annotations, e.t.c.)

[xavierleune]

Ok I'll stick with 5.x so ^^ Thanks !

[xavierleune]

@DjordyKoert branch rebased on 5.x. Do you have any thoughts to share on this pull request ? Thks !

[DjordyKoert]

I don't think this is correct, is it?

The order of the generated schema's has been swapped.

[xavierleune]

Well it's been swapped because now when you register a Model, the whole model is described and names are reserved for all dependencies. So in this example when the first route in ApiController is parsed (fetchArticleAction), the model new Model(type: Article::class, groups: ['light']) is registered and the name User is reserved for the empty User model (because User has no property with the serialization group light).
When it comes to /swagger/implicit, the name User has already been taken, so the full entity User with all properties (because no serialization group has been applied) is described as User2.
It may be pretty hard to avoid that, however I think that this behavior is more predictable. WDYT ?

[DjordyKoert]

Can we re-use this new method in the registerSchemas method aswell?

[xavierleune]

Good idea, I renamed this method describeSchema and used it in registerSchemas and register

[DjordyKoert]

Can we not store the Schema object directly instead of json encoding it?

Suggested change

	$this->schemasPerType[$type][] = ['schema' => json_encode($schema), 'model' => $model];
	$this->schemasPerType[$type][] = ['schema' => $schema, 'model' => $model];

[xavierleune]

We can do that yes, but we have to use json_encode for comparison. Because:

• We cannot use strict equality here as instances are different
• We cannot use lose equality either, because of a cyclic dependency we go into an infinite loop

So if we decide to store the schema and not it's json representation, we would have to run json_encode and there is a lot of ops in OpenApi\Annotations\AbstractAnnotation::jsonSerialize, so I assumed it would be better to json_encode only once.
WDYT ?