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

[Golang][client] fix for schema definition name file #433

Conversation

grokify
Copy link
Member

@grokify grokify commented Jul 2, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@antihax
@bvwells

There are some open items listed at the end of this post so it is not yet ready for merging. Please review the general approach.

Description of the PR

When generating a client with a v3.0 spec Schema or v2.0 spec Definition named File, the generator will convert the type to *os.File instead of the model named File. This is a generic issue where a model schema/definition name may collide with a typeMapping key.

An example spec excerpt looks like:

    PoolResponseResource:
      type: "object"
      properties:
          # ...
        sourceFiles:
          type: "array"
          items:
            $ref: "#/definitions/File"
    File:
      type: "object"
      properties:
        sourceFile:
          type: "string"
        originalFileName:
          type: "string"

In the above, PoolResponseResource will have a property SourceFiles with type []*os.File instead of the desired []File.

This occurs because abstract class AbstractGoCodegen converts File to *os.File using typeMapping:

        typeMapping.put("File", "*os.File");
        typeMapping.put("file", "*os.File");

getSchemaType and getTypeDeclaration both check against typeMapping, e.g.:

if (typeMapping.containsKey(openAPIType)) {
            type = typeMapping.get(openAPIType);

getSchemaType gets a baseline type to process from super.getSchemaType which returns a single string that appears to represent 3 things: (a) definition/schema names, (b) types, and (c) formats. typeMapping covers the following:

schema type language type spec (2.0)
integer int32 type only: "type": "integer"
long int64 type and format: "type": "integer", "format": "int64"
File *os.File $ref only: "$ref": "#/definitions/File"

Proposed Fix

This PR includes a minimal fix and test case to consider the value of $ref when determining the type. If $ref is present and matches the Spec 2.0/3.0 definition/schema path, typeMapping is not used.

Notes

The added test, filePropertyTest in GoModelTest.java uses the spec 2.0 model reference: #/definitions/File, however, when building a client, it appears a 2.0 spec using #/definitions/File is converted to a 3.0 spec #/components/schemas/File by super.getSchemaType

References

Open Items / Next Steps

  1. This update generates client code conditionally if a spec definition with name File (or other schema name with typeMapping collision) is present, so an additional fake PetStore test endpoint will be provided to test the results. filePropertyTest added to GoModelTest.java.
  2. This currently generates a regression issue that needs to be resolved. A struct will be created with name ProfileImageInfoUri but be referenced as ProfileImageInfoURI. This is not covered in the Petstore tests, but appears using the RingCentral spec. Will resolve and look to add a test case for this. Fixed in e51623b . Covered in filePropertyTest added to GoModelTest.java.

@grokify grokify changed the title Go/client/fix/file for schema definition name [Golang][client] fix for schema definition name file Jul 2, 2018
@grokify
Copy link
Member Author

grokify commented Jul 2, 2018

Build errors so far:

  • Shippable: Failing due to ShutdownMonitorThread already started race condition
  • CircleCI: Worked previously. Recently failed due to JAR download error

@antihax
Copy link
Contributor

antihax commented Jul 2, 2018

Thanks for looking into this.

Is this a common problem that could affect other languages?

@wing328
Copy link
Member

wing328 commented Jul 2, 2018

I've restarted the shippable job and it's fine now.

For CircleCI, seems like there're some changes and I could no longer restart it: https://circleci.com/gh/grokify/openapi-generator/28?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

The failure has nothing to do with the change and the Go test passed:

[ERROR] Error fetching link: /home/circleci/OpenAPITools/openapi-generator/modules/openapi-generator/home/circleci/OpenAPITools/openapi-generator/modules/openapi-generator-cli/target/apidocs/package-list. Ignored it.
line 2 column 2 - Error: <additionalproperties> is not recognized!
line 3 column 6 - Error: <additionalproperty> is not recognized!
line 2 column 2 - Error: <additionalproperties> is not recognized!
line 3 column 6 - Error: <additionalproperty> is not recognized!
=== RUN   TestOAuth2
--- PASS: TestOAuth2 (0.29s)
=== RUN   TestBasicAuth
--- PASS: TestBasicAuth (0.00s)
=== RUN   TestAccessToken
--- PASS: TestAccessToken (0.00s)
=== RUN   TestAPIKeyNoPrefix
--- PASS: TestAPIKeyNoPrefix (0.00s)
=== RUN   TestAPIKeyWithPrefix
--- PASS: TestAPIKeyWithPrefix (0.00s)
=== RUN   TestDefaultHeader
--- PASS: TestDefaultHeader (0.00s)
=== RUN   TestHostOverride
--- PASS: TestHostOverride (0.01s)
=== RUN   TestAddPet
--- PASS: TestAddPet (0.00s)
=== RUN   TestFindPetsByStatusWithMissingParam
--- PASS: TestFindPetsByStatusWithMissingParam (0.00s)
=== RUN   TestGetPetById
--- PASS: TestGetPetById (0.00s)
=== RUN   TestGetPetByIdWithInvalidID
--- PASS: TestGetPetByIdWithInvalidID (0.00s)
=== RUN   TestUpdatePetWithForm
--- PASS: TestUpdatePetWithForm (0.00s)
=== RUN   TestFindPetsByTag
--- PASS: TestFindPetsByTag (0.00s)
=== RUN   TestFindPetsByStatus
--- PASS: TestFindPetsByStatus (0.00s)
=== RUN   TestUploadFile
--- PASS: TestUploadFile (0.03s)
=== RUN   TestUploadFileRequired
--- PASS: TestUploadFileRequired (0.00s)
=== RUN   TestDeletePet
--- PASS: TestDeletePet (0.00s)
=== RUN   TestPlaceOrder
--- PASS: TestPlaceOrder (0.02s)
=== RUN   TestCreateUser
--- PASS: TestCreateUser (0.01s)
=== RUN   TestCreateUsersWithArrayInput
--- PASS: TestCreateUsersWithArrayInput (0.01s)
=== RUN   TestGetUserByName
--- PASS: TestGetUserByName (0.00s)
=== RUN   TestGetUserByNameWithInvalidID
--- PASS: TestGetUserByNameWithInvalidID (0.00s)
=== RUN   TestUpdateUser
--- PASS: TestUpdateUser (0.00s)
=== RUN   TestDeleteUser
--- PASS: TestDeleteUser (0.00s)
PASS
ok  	_/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/go	0.405s

@grokify
Copy link
Member Author

grokify commented Jul 2, 2018

@antihax

Is this a common problem that could affect other languages?

If the general approach is the same, I think so. If every generator that called getSchemaType and performed language type conversion need to do the following things:

  1. get the schema type from getSchemaType
  2. implement a check for schema/definition name vs. built-in data type
  3. convert built-in schema type to language type
  4. convert schema model name to language model name

It seems like a more generic and less error-prone solution for all generators would be to abstract the second step so it's implemented once. We could have a method like getSchemaType across all generators that returned additional information on whether the returned string was a built-in data type or a schema/definition name so the generators could process them differently without implementing individual checks.

I thought about implementing this approach as well but since this is a deeper change, it's worth a discussion. If we think this is a better way to go, I can look into this as well. There are some nuances like the Outer(Boolean|Number|String) tests that I ran into and would look into more for this approach.

@grokify
Copy link
Member Author

grokify commented Jul 4, 2018

@antihax @bvwells @wing328

All tests are passing with no conflicts.

PR includes additional tests:

Regarding a more generic approach across all clients, I'd want to look into a few more generators to compare approaches so my preference is to have them as separate activities.

@@ -67,7 +67,6 @@ paths:
description: Status values that need to be considered for filter
required: true
style: form
explode: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI. Opened Mermade/oas-kit#76 to track it.

@wing328
Copy link
Member

wing328 commented Jul 5, 2018

The shippable error does not seem to be related to the change.

@wing328 wing328 merged commit 0bffdf2 into OpenAPITools:master Jul 5, 2018
@jmini jmini added this to the 3.1.0 milestone Jul 6, 2018
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jul 7, 2018
* master: (116 commits)
  3.1.0 release (OpenAPITools#486)
  Fix broken link to openapi generator plugin README (OpenAPITools#484)
  show warning message for nodejs server only (OpenAPITools#481)
  Add grokify to Go technical committee (OpenAPITools#479)
  [Slim] Improve codebase decouple (OpenAPITools#438)
  Ensure typescript samples are up to date (OpenAPITools#444)
  Update README.md
  [Golang][client] delete sample output dir before rebuild (OpenAPITools#477)
  update petstore samples (OpenAPITools#478)
  Revert "Improve Docker Tags (OpenAPITools#390)"
  update go client test dependencies (OpenAPITools#468)
  [Golang][client] fix for schema definition name `file` (OpenAPITools#433)
  Fix '.travis' file (syntax)
  make LICENSE GitHub display compatible (OpenAPITools#467)
  Improve Docker Tags (OpenAPITools#390)
  [Golang][client] fix file suffix for _test.go (OpenAPITools#449)
  Remove copy section (OpenAPITools#463)
  Add link to presentation (OpenAPITools#465)
  Use postProcessOperationsWithModels(Map, List) (OpenAPITools#431)
  [C] Adding petstore sample written in C (OpenAPITools#306)
  ...
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
)

* fix schema/definition name as 'file'

* update samples

* Trigger CI due to previous Shippable race condition

* add fix with toModelName(openAPIType)

* update tests for file schema/definition name

* Update 3.0 test spec

* update samples

* update samples for jaxrs-cxf

* Trigger CI due to previous Shippable race condition

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

Successfully merging this pull request may close these issues.

4 participants