Default OpenAPI Version from RootContext overwrites OpenAPI Version from OpenApi Property

[bastianschwarz]

Hey There,

I found a possible bug (possible because maybe that's intentional? I hope not 😆 ):

Currently all our Docs are generated with OpenAPI 3.0.0 even though some of them should have 3.1.0

I know this has worked in the past so I did some digging:

I have a command that wraps the Generator and basically calls Generator::scan() with different source directories so I can easily create multiple API Docs.

    public function handle(): void
    {
        $this->generate('some-namespace/some-api-name', ['V1']);
    }

    public function generate(string $api, array $versions): void
    {
        foreach($versions as $version) {
            $versionBase = __DIR__ . "/../$api/$version";
            $spec = Generator::scan([__DIR__ . '/../Common', __DIR__ . "/../$api/V0", $versionBase], [
                'config' => [
                    'cleanUnusedComponents' => [
                        'enabled' => true,
                    ],
                ],
            ]);

            $filename = base_path('generated/restapi/' . mb_strtolower("$api-$version"));
            File::makeDirectory(dirname($filename), 0777, true, true);
            File::put("$filename.json", $spec->toJson());
            File::put("$filename.yaml", $spec->toYaml());
        }
    }

Each of the APIs has a API Class with an OpenApi Annotation including the Version for the API:

#[OpenApi(
    openapi: OpenApi::VERSION_3_1_0,
    info: new Info(
      ...
    ),
    ...
)]
final class Api
{
   ...
}

Now, when the Generator::generate() Method is called it creates a root context: https://github.com/zircote/swagger-php/blob/5ba05b8eabb28568e5946438012feb62573b49b8/src/Generator.php#L496-L499 The version in the Generator is empty as I want it to use the Version from the Property later on.

The Context defaults back to the Default Version in the constructor, so there is ALWAYS a version set: https://github.com/zircote/swagger-php/blob/5ba05b8eabb28568e5946438012feb62573b49b8/src/Context.php#L72-L75

The ALWAYS part becomes relevant when the Property is later analyzed: There's an if for the OpenApi instance that checks if the version in the root context is set: https://github.com/zircote/swagger-php/blob/5ba05b8eabb28568e5946438012feb62573b49b8/src/Annotations/AbstractAnnotation.php#L144-L151

Now since the constructor defaults back to default version the if can never be false (unless I missed something) so it always takes precedence over the version from the Property (the else branch).

Shouldn't it be the other way around? So, the more specific value in the property is used if set and the default from root only if it doesn't have one?

I assume this changed with https://github.com/zircote/swagger-php/commit/b46a36d006f4db4d761995a5add1e7ab0386ed1d and is at least somewhat intentional but probably an unintended side effect?

I will probably fix it on my end by trying to find the Api class, get the property and pass the version as config to to generate() but it feels like a workaround.

Let me know what you think.

[DerManoMann]

Good analysis. This looks like it really is a regressoon. The version is a pain as it is a classic hen/egg problem. Code might need/access it before the OpenApi annotation has been parsed/processed in which case there needs to be a fallback.

I think maybe explicitly setting it in the context is wrong and there should be an accessor method instead that knows to use a fallback. That way there is a safe way to access the best guess at the time while allowing to set the most specific one when possible.

Thanks for bringing this up - been annoying me for quite some time :)

[DerManoMann]

Could you try #1644 - that should work for you.

[bastianschwarz]

Just tried it, the branch works. 😄 Thanks for looking into this so fast. I have a workaround where I pair the APIs with the OpenAPI version and pass it to the generator for each API but it's cleaner without the redundancy. :)

My 2 cents on this (even though no one asked 😆 ): While I'm not that familiar with the code but it seems like the root context is jsut there for the version and the logger? I think the logger should just live in the generator and then passed down.

For the version: Might be worth to find the OpenApi Attribute after the sources are scanned and fetch the version from there. Then you probably could get rid of the root context entirely?

Another thing would be to do some sorting after the sources are scanned but before the whole analysis is run. Then you would know that the order is correct. If you want to get really fancy even a Topo Sort would work. Since the hierarchy is defined by the OpenApi Spec (which elements are allowed in which parents) you would know that all nodes are analyzed in the correct order (top to bottom). Not sure if it's worth it 🤷 Probably not as I image you don't have infinite time to spend on this 😆

[DerManoMann]

Version and logger have been late additions to the context. It's a bit abuse at the context is supposed to be more like metadata - file/line details, etc. and also to what PHP construct the annotation is related. However, the context is the best option to share things across all annotations since there is no clean way to manage instance creation, in particular of the nested instances. So, passing things around is hard otherwise. One day...

I think the issue you had is a regression, the PR re-instantiated the previous behviour. I suspect some downstream code might need to switch to using getVersion() instead of accessing it directly to avoid null issues :shrug:

Not sure about the sorting, but I definitly do only have finite time :)