[go-experimental] Add oneOf support #5150

bkabrda · 2020-01-29T17:08:52Z

This PR adds oneOf support in the same way it was added for Java clients recently in #5120. It contains a lot of refactoring to be able to use the same mechanisms as were used to implement this feature for Java. Also see #4785 for discussion on semantics of oneOf and thoughts on why it was implemented that way for Java.

Note that this PR doesn't contain any tests right now. I do want to add tests, but would like to do that after the general direction of this PR and the related refactoring is approved.

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

bkabrda · 2020-01-30T08:37:55Z

FTR, I don't think that the Shippable failure is related to this PR:

> Task :compileKotlin FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':compileKotlin'.
> Could not resolve all files for configuration ':compileClasspath'.
   > Could not download jackson-coreutils.jar (com.github.fge:jackson-coreutils:1.8)
      > Could not get resource 'https://jcenter.bintray.com/com/github/fge/jackson-coreutils/1.8/jackson-coreutils-1.8.jar'.
         > Could not GET 'https://repo.jfrog.org/artifactory/libs-release-bintray/com/github/fge/jackson-coreutils/1.8/jackson-coreutils-1.8.jar?referrer'.
            > sun.security.validator.ValidatorException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

jimschubert

This looks fantastic. I'm going to give it a few days or so to get feedback from anyone else on the core team or collaborator team if they'd like to review.

cc @OpenAPITools/openapi-generator-core-team

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

...i-generator/src/main/java/org/openapitools/codegen/utils/OneOfImplementorAdditionalData.java

jimschubert · 2020-02-05T02:45:40Z

I retargeted to master and fixe a merge conflict in model.mustache. Could you look at that merge resolution to see if it might interfere with your changes (map model/parent block right below the new vendor-extension addition).

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

bkabrda · 2020-02-05T08:29:48Z

@jimschubert thanks for the review! I'll try to address the points that you raised during today. I checked the conflict resolution and it LGTM.

Additionally, I recently submitted #5175 which will need to be extended to account for changes in this PR. I'd prefer if it was left until after this is merged, because rebasing this PR is harder. I'll leave a message on that PR as well to not add unnecessary work with rebasing.

bkabrda · 2020-02-05T10:14:51Z

So I fixed the review comments as best I could. I will work on adding tests until EOW.

modules/openapi-generator/src/main/resources/go-experimental/model.mustache

jirikuncar · 2020-02-05T15:04:48Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

@@ -377,8 +377,9 @@ private void registerMustacheLambdas() {
                for (Object _implmo : models) {
                    Map<String, Object> implmo = (Map<String, Object>) _implmo;
                    CodegenModel implcm = (CodegenModel) implmo.get("model");
-                    if (additionalDataMap.containsKey(implcm.name)) {
-                        additionalDataMap.get(implcm.name).addToImplementor(this, implcm, imports, addOneOfInterfaceImports);
+                    String modelName = toModelName(implcm.name);


📛Thank you!

bkabrda · 2020-02-06T13:22:55Z

I added some basic tests for the OneOfImplementorAdditionalData and DefaultCodegen.preprocessOpenAPI. I didn't manage to find out an easy way to test the DefaultCodegen.postProcessAllModels method, since it looks like either I'd need to instantiate and run DefaultGenerator or I'd have to create the whole model structure by hand, none of which seem to be very straightforward. If anyone has recommendations on how to do this, I'd be glad to hear them.

jimschubert · 2020-02-09T05:28:31Z

@bkabrda it's not easy to get at postProcessAllModels. I plan to do some pretty heavy refactoring for the 5.0 release. One of the remaining things I'm trying to complete in 4.x would be a dry-run generator option, which could probably be used for testing those harder accessible methods. I'm maybe a couple weeks away from having time to complete the dry-run generation.

bkabrda · 2020-02-10T13:13:49Z

@jimschubert that sounds great. I'm assuming it's ok to not have this part tested ATM? I'll be happy to add the tests once the refactored code is pushed.

LMK when this is ok to be merged, I'll do a rebase then (since this change now spans several files that tend to change quite often, I don't want to spend too much time rebasing it).

jimschubert · 2020-02-10T13:18:23Z

Yeah, it's ok to leave out that test. I'm fine with a rebase and merge now.

tbh, you've added more tests than most people do!

bkabrda · 2020-02-10T13:19:53Z

Thanks :) let me do the rebase and let's get this merged!

…s the oneOf interface

…odel.mustache Co-Authored-By: Jiri Kuncar <[email protected]>

bkabrda · 2020-02-10T13:42:21Z

Rebased. I retested everything locally, but let's wait for CI anyway to make sure I did everything right.

* [go-experimental] Add oneOf support * Fix docs for the oneOf models * isOneOfInterface => x-is-one-of-interface * Add proper warnings when inline models are used in oneOf choices * Add a convenience method to oneOf implementing structs to cast them as the oneOf interface * Update modules/openapi-generator/src/main/resources/go-experimental/model.mustache Co-Authored-By: Jiri Kuncar <[email protected]> * Fix retrieving data from additionalDataMap * Add basic tests Co-authored-by: Jiri Kuncar <[email protected]>

jimschubert changed the base branch from 4.3.x to master February 5, 2020 02:25

jimschubert approved these changes Feb 5, 2020

View reviewed changes

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java Outdated Show resolved Hide resolved

...i-generator/src/main/java/org/openapitools/codegen/utils/OneOfImplementorAdditionalData.java Show resolved Hide resolved

jimschubert reviewed Feb 5, 2020

View reviewed changes

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java Outdated Show resolved Hide resolved

jimschubert added Client: Go Enhancement: General labels Feb 5, 2020

jimschubert added this to the 4.3.0 milestone Feb 5, 2020

bkabrda mentioned this pull request Feb 5, 2020

[go-experimental] Add model constructors to initialize non-container vars with defaults #5175

Merged

5 tasks

jirikuncar reviewed Feb 5, 2020

View reviewed changes

modules/openapi-generator/src/main/resources/go-experimental/model.mustache Outdated Show resolved Hide resolved

jirikuncar reviewed Feb 5, 2020

View reviewed changes

Slavek Kabrda and others added 8 commits February 10, 2020 14:38

[go-experimental] Add oneOf support

e0f2544

Fix docs for the oneOf models

40dbf3a

isOneOfInterface => x-is-one-of-interface

b94ec8d

Add proper warnings when inline models are used in oneOf choices

dbd5aa1

Add a convenience method to oneOf implementing structs to cast them a…

7fd47d6

…s the oneOf interface

Update modules/openapi-generator/src/main/resources/go-experimental/m…

800bea5

…odel.mustache Co-Authored-By: Jiri Kuncar <[email protected]>

Fix retrieving data from additionalDataMap

b93a747

Add basic tests

347eaf7

bkabrda force-pushed the go-experimental-oneof-support branch from 01968ef to 347eaf7 Compare February 10, 2020 13:41

jimschubert merged commit 0693a83 into OpenAPITools:master Feb 10, 2020

LaysDragon mentioned this pull request Sep 10, 2020

[Go][Experimental] better oneOf and anyOf implementation #6166

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[go-experimental] Add oneOf support #5150

[go-experimental] Add oneOf support #5150

bkabrda commented Jan 29, 2020

bkabrda commented Jan 30, 2020

jimschubert left a comment

jimschubert commented Feb 5, 2020

bkabrda commented Feb 5, 2020

bkabrda commented Feb 5, 2020

jirikuncar Feb 5, 2020 •

edited

Loading

bkabrda commented Feb 6, 2020

jimschubert commented Feb 9, 2020

bkabrda commented Feb 10, 2020

jimschubert commented Feb 10, 2020

bkabrda commented Feb 10, 2020

bkabrda commented Feb 10, 2020

[go-experimental] Add oneOf support #5150

[go-experimental] Add oneOf support #5150

Conversation

bkabrda commented Jan 29, 2020

PR checklist

bkabrda commented Jan 30, 2020

jimschubert left a comment

Choose a reason for hiding this comment

jimschubert commented Feb 5, 2020

bkabrda commented Feb 5, 2020

bkabrda commented Feb 5, 2020

jirikuncar Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

bkabrda commented Feb 6, 2020

jimschubert commented Feb 9, 2020

bkabrda commented Feb 10, 2020

jimschubert commented Feb 10, 2020

bkabrda commented Feb 10, 2020

bkabrda commented Feb 10, 2020

jirikuncar Feb 5, 2020 •

edited

Loading