Skip to content
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.

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

Cannot overwrite generated schema #1528

Open
DjordyKoert opened this issue Jan 5, 2024 · 2 comments
Open

Cannot overwrite generated schema #1528

DjordyKoert opened this issue Jan 5, 2024 · 2 comments

Comments

@DjordyKoert
Copy link
Contributor

I encountered an issue while trying to create a fix for our generated enums nelmio/NelmioApiDocBundle#2177

While doing this I noticed that the AugmentProperties processor makes sure to set the oneOf property whenever a ref is set on a nullable === true schema.

protected function applyRef(Analysis $analysis, OA\Property $property, string $ref): void
{
if ($property->nullable === true) {
$property->oneOf = [
$schema = new OA\Schema([
'ref' => $ref,
'_context' => new Context(['generated' => true], $property->_context),
]),
];
$analysis->addAnnotation($schema, $schema->_context);
} else {
$property->ref = $ref;
}
}

I wanted to overwrite this by creating a new processor which would revert this ONLY for schemas based on enums. See implementation:

<?php

declare(strict_types=1);

namespace Nelmio\ApiDocBundle\Processors;

use OpenApi\Analysis;
use OpenApi\Generator;
use OpenApi\Processors\ProcessorInterface;
use OpenApi\Annotations as OA;

final class EnumOneOfToRefProcessor implements ProcessorInterface
{
    public function __invoke(Analysis $analysis): void
    {
        if (\PHP_VERSION_ID < 80100) {
            return;
        }

        $schemas = $analysis->openapi?->components->schemas;

        foreach ($schemas as $schema) {
            if (!is_array($properties = $schema->properties)) {
                continue;
            }

            foreach ($properties as $property) {
                if ($property->nullable !== true) {
                    continue;
                }

                if (!is_array($property->oneOf)) {
                    continue;
                }

                if (count($property->oneOf) !== 1) {
                    continue;
                }

                if (!$refSchema = $this->schemaForRef($schemas, $ref = $property->oneOf[0]->ref)) {
                    continue;
                }

                if (!$class = $analysis->getContext($refSchema)?->class) {
                    continue;
                }

                if (!enum_exists($class)) {
                    continue;
                }

                $property->ref = $ref;
                $property->oneOf = Generator::UNDEFINED;
            }
        }
    }

    /**
     * Find schema for the given ref.
     */
    private function schemaForRef(array $schemas, string $ref): ?OA\Schema
    {
        foreach ($schemas as $schema) {
            if (OA\Components::ref($schema) === $ref) {
                return $schema;
            }
        }

        return null;
    }
}

I assumed this to work because I though that processors were 'absolute', this seems to not be the case because the AbstractAnnotation class overwrites my changes again while generating the json.

if (property_exists($this, 'nullable') && $this->nullable === true) {
$ref = ['oneOf' => [$ref]];
if ($this->_context->version == OpenApi::VERSION_3_1_0) {
$ref['oneOf'][] = ['type' => 'null'];
} else {
$ref['nullable'] = $data->nullable;
}
unset($data->nullable);
// preserve other properties
foreach (get_object_vars($this) as $property => $value) {
if ('_' == $property[0] || in_array($property, ['ref', 'nullable'])) {
continue;
}
if (!Generator::isDefault($value)) {
$ref[$property] = $value;
}
}
}

Solution

A possible solution could be to modify above highlighted code with this:

        // $ref
        if (isset($data->ref)) {
            // Only specific https://github.com/OAI/OpenAPI-Specification/blob/3.1.0/versions/3.1.0.md#reference-object
            $ref = ['$ref' => $data->ref];
            if ($this->_context->version === OpenApi::VERSION_3_1_0) {
                foreach (['summary', 'description'] as $prop) {
                    if (property_exists($this, $prop)) {
                        if (!Generator::isDefault($this->{$prop})) {
                            $ref[$prop] = $data->{$prop};
                        }
                    }
                }
            }

            foreach (get_object_vars($this) as $property => $value) {
                if ('_' == $property[0] || in_array($property, ['ref'])) {
                    continue;
                }
                if (!Generator::isDefault($value)) {
                    $ref[$property] = $value;
                }
            }
            
            $data = (object) $ref;
        }

This would make sure to not overwrite our changes from our processor.
The old logic could then be placed inside of an new processor that this package provides.

But then again there might have been a reason why it was not done this way, which I do not know of. I hope hear some thoughts about this and maybe we can find a solution 😄

For now I have simply fallen back to the previous behaviour nelmio/NelmioApiDocBundle#2178

@DerManoMann
Copy link
Collaborator

Your question highlights the fact that for a long time I didn't really have a clue what I was doing in the codebase :)
Over time a lot of changes have been done in the most convenient place but not necessarily the best place.
I agree with you that this type of modification should be the responsibility of a processor. Serialization should not have to deal with this sort of thing.
Maybe we need an OpenApiVersion processor that handles the version specific modifications?

@DjordyKoert
Copy link
Contributor Author

Something like an OpenApiVersion processor (or OpenApi31Version to make it version specific?) does also make sense to me aswell.

Having a quick look at the jsonSerialize method even makes me think that most if not all logic could potentially be placed inside a processor.

I can even try and have a look at it sometime and see if this would be easy to change this in a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants