OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
20.74k stars 6.32k forks source link

[BUG] Schema $ref with same name is not producing models properly #2729

Open vinodchitrali opened 5 years ago

vinodchitrali commented 5 years ago

Bug Report Checklist

Description

My schema definition are spread across multiple files and they are cross reference(using $ref). In these files there is one schema with same name. I have 5 such instances

When i run code generator, Only 2 models are produced. Rest are ignored. Some of the $ref are pointing to wrong model.

Here is example given below

openapi-generator version

4.0.0-beta

OpenAPI declaration file content or url

https://gist.github.com/vinodchitrali/c5cd3be1c8d6fb0d2fb6bcf6a530ad94

Command line used for generation
java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar  generate -i ./api/openapi.yaml   -g python -o /tmp/python/ --api-package api --model-package model --additional-properties projectName=python
Steps to reproduce

1) Copy the Yaml files from https://gist.github.com/vinodchitrali/c5cd3be1c8d6fb0d2fb6bcf6a530ad94 2) java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -i ./api/openapi.yaml -g python -o /tmp/python/ --api-package api --model-package model --additional-properties projectName=python 3) Check files generated in /tmp/python/openapi_client/model/

Related issues/PRs
Suggest a fix
auto-labeler[bot] commented 5 years ago

👍 Thanks for opening this issue! 🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

cvgaviao commented 5 years ago

Wouldn't be better just to change those names to avoid duplication ?

vinodchitrali commented 5 years ago

@cvgaviao , thanks for look into the issue

Well, I can, but i have 1000+ such instances. Its tedious job.

Also, I get these schema from different sources. I combined in my custom schema by adding external ref($ref: http:///schemas/v1/.yaml.

Any change in the external schema is nightmare.

vinodchitrali commented 5 years ago

I was debugging the issue, i figured out that the issue is https://github.com/swagger-api/swagger-parser

Possibly https://github.com/swagger-api/swagger-parser/blob/master/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/SchemaProcessor.java

The schema duplication is limited to 2.

cvgaviao commented 5 years ago

@vinodchitrali, just note that openapi-generator is using its own branch/fork and not the one from swagger-api account. they are not always in sync. Seems to already exist some opened issues related to schema collision there. I've faced a similar problem of yours. And as I was without time I've used speccy to merge external models into one model.

vinodchitrali commented 5 years ago

Thanks @cvgaviao, Any possible fix available ?

vinodchitrali commented 5 years ago

I see some issue in the following code.

https://github.com/swagger-api/swagger-parser/blob/master/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java#L87

                //We add a number at the end of the definition name
                int i = 2;
                for (String name : schemas.keySet()) {
                    if (name.equals(possiblyConflictingDefinitionName)) {
                        tryName = possiblyConflictingDefinitionName + "_" + i;
                        existingModel = schemas.get(tryName);
                        i++;
                    }

In above code, possiblyConflictingDefinitionName remains same through out the loop. If possiblyConflictingDefinitionName = "model" then the outcome of the loop is always tryName = "model_2". I am hitting same issue.

So above code should be

               //We add a number at the end of the definition name
                int i = 2;
                tryName = possiblyConflictingDefinitionName;
                for (String name : schemas.keySet()) {
                    if (name.equals(tryName)) {
                        tryName = possiblyConflictingDefinitionName+ "_" + i;
                        existingModel = schemas.get(tryName);
                        i++;
                    }

Can some one help me to validate this code ? Also help to build org.openapitools.swagger.parser(2.0.11-OpenAPITools.org-1)

@jmini , I see that u have done recent pom update for 2.0.11. Can check above code and help to build swagger parser for OpenAPI tools

vinodchitrali commented 5 years ago

team@openapitools.org help me build swagger parser

vinodchitrali commented 5 years ago

@wing328 @jimschubert @cbornet @ackintosh , can some one help to build swagger parser for generator ?

vinodchitrali commented 5 years ago

@ackintosh , Thanks for adding test case.

Can you help me to build locally swagger-parser for Openapi generator

cvgaviao commented 5 years ago

@vinodchitrali , do you know how Git and Maven works ? without that knowledge it will be a kind complicated for you... Btw, you just need to clone the swagger-parser repository and do a mvn clean install. Then clone the openapi-generator repository and change its main pom properties to work with the version that you wanted to use:

        <swagger-parser-groupid>io.swagger.parser.v3</swagger-parser-groupid>
        <swagger-parser-version>2.0.12</swagger-parser-version>

In order to build openapi-generator with a snapshot version you need to use this:

mvn clean install -Denforcer.skip=true -Dmaven.test.skip=true
jmini commented 5 years ago

@vinodchitrali: can you please file an issue in the upstream project: https://github.com/swagger-api/swagger-parser/issues

If you produce a patch for Swagger-Parser (including the test case proposed by @ackintosh (if he agree) -- maybe you can adapt it), please file a PR to fix it.

vinodchitrali commented 5 years ago

@jmini , I have already filed issue. https://github.com/swagger-api/swagger-parser/issues/1089

vinodchitrali commented 5 years ago

@cvgaviao , i am able to build from upstream clone. only thing it is breaking lots of test cases, also getting some exception while building

cvgaviao commented 5 years ago

@vinodchitrali, this is because the upstream team has not applied the PR provided by openapi-generator's team branch and vice-versa. What I did was to create my own local branch and I'm trying to keep the commits from upstream and openapi-generator in synch... at least I was able to workaround my problems for now.

jmini commented 5 years ago

@cvgaviao in the swagger-parser fork we maintain on branch 2.0-OpenAPITools in https://github.com/OpenAPITools/swagger-parser/, we are almost in sync with the master branch of https://github.com/swagger-api/swagger-parser

We have one addition to convert a Swagger-v2 Operation name to an OpenAPI-v3 vendor extensionx-codegen-request-body-name.

cvgaviao commented 5 years ago

@jmini , I know and you are doing a great job!. But the specific fix for this issue related to $ref was only released on version 2.0.12.
I could see that the latest released version of your branch is based on version 2.0.11, though...

jmini commented 5 years ago

I have prepared PR https://github.com/OpenAPITools/openapi-generator/pull/2775 to update Swagger-Parser

jmini commented 5 years ago

I have updated Swagger-Parser. Can you check the latest 4.0.0-SNAPSHOT version of OpenAPI Generator?

vinodchitrali commented 5 years ago

@jmini is it up streamed ?

jmini commented 5 years ago

@vinodchitrali: sorry I do not get your question.

If you now use a recent 4.0.0-SNAPSHOT version of "OpenAPI-Generator", this version now rely on org.openapitools.swagger.parser:swagger-parser:2.0.13-OpenAPITools.org-1 instead of org.openapitools.swagger.parser:swagger-parser:2.0.11-OpenAPITools.org-1 as it was previously the case.

This custom 2.0.13-OpenAPITools.org-1 release of Swagger-Parser is: io.swagger.parser.v3:swagger-parser:2.0.12 + 3 additional fixes (see the release notes).

jmini commented 4 years ago

@jmini is it up streamed ?

swagger-api/swagger-parser#1089 seems still to be open, you need to provide a patch there. For the moment the PR in the issue there only references a test case and no fix.