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

OpenAPI $ref not resolved #21

Open
tuarrep opened this issue Mar 17, 2024 · 1 comment
Open

OpenAPI $ref not resolved #21

tuarrep opened this issue Mar 17, 2024 · 1 comment

Comments

@tuarrep
Copy link

tuarrep commented Mar 17, 2024

Issue

When the OpenAPI schema contains referenced parameters, they are not resolved and not included in the generated SDK.

Here is a test that fails to reproduce this (based on https://github.com/crescat-io/saloon-sdk-generator/blob/master/tests/Samples/spotify.yml) :

test('Resolved references in parameters', function () {
    $specFile = sample_path('spotify.yml');
    $parser = OpenApiParser::build($specFile);
    $spec = $parser->parse();

    expect($spec->endpoints)->not()->toBeNull()
        ->and($spec->endpoints[0]->pathParameters)->toHaveCount(1)
        ->and($spec->endpoints[0]->queryParameters)->toHaveCount(1);
});

For reference, here is the expected SDK (based on the source schema):
CleanShot 2024-03-17 at 00 58 35

Resolution & discussion

At the first attempt, I tried:
https://github.com/crescat-io/saloon-sdk-generator/blob/master/src/Parsers/OpenApiParser.php

    public static function build($content): self
    {
        return new self(
            Str::endsWith($content, '.json')
-              ? Reader::readFromJsonFile(fileName: realpath($content), resolveReferences: ReferenceContext::RESOLVE_MODE_INLINE)
-              : Reader::readFromYamlFile(fileName: realpath($content), resolveReferences: ReferenceContext::RESOLVE_MODE_INLINE)
+              ? Reader::readFromJsonFile(fileName: realpath($content), resolveReferences: ReferenceContext::RESOLVE_MODE_ALL)
+              : Reader::readFromYamlFile(fileName: realpath($content), resolveReferences: ReferenceContext::RESOLVE_MODE_ALL)
        );
    }

This amply slows down generation.
On my machine, the SmokeTest test takes about 12 seconds before the change and times out after (60 seconds)

I also found this commit bd9d325 which changes the resolution mode from RESOLVE_MODE_ALL to RESOLVE_MODE_INLINE without explaining why but surely on purpose.

My second thought was to resolve references in the mapPrams method by overriding context at that time to change its mode:

if(is_a($parameter->schema, Reference::class)) {
    $context = $parameter->schema->getContext();
    $context->mode = 'ReferenceContext::RESOLVE_MODE_ALL';

    $parameter->schema = $parameter->schema->resolve($context);
}

But according to the OpenAPI specification, references can be used at various locations, for example responses.
I know responses are not yet parsed, but I think it should be taken in account for the future.


As I think there are choices to make, I preferred to submit an issue first. I'll submit a PR when this discussion converges to a solution.

@brunolanevik
Copy link

Is there any updates on this?

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

No branches or pull requests

2 participants