[Go] Interface definitions for api functions #5914

arvindth · 2020-04-13T16:24:12Z

Add golang interface definitions for api functions
- Fix [REQ][Golang][client] interface definitions for api functions #2294 with code suggested by @partkyle
Add new go generator option generateInterfaces, defaulted to false.
Slight updates to method comments in api.mustache to prettify and align better.
Update go-petstore, go-petstore-withXml, and openapi3-go-petstore samples.
Verified that the following pass:
- mvn clean install
- mvn -Pstatic-analysis clean install
- mvn integration-test -rf :GoPetstore in samples/client/petstore/go, both for the default go-petstore sample generation, and when generated with generateInterfaces=true.

Questions I had:

I see that go-petstore-withXml has withGoCodegenComment=true as an additional option, so it looked like that might be where alternate options were being tested, so I've added generateInterfaces=true to those samples. Let me know if that should get reverted.
It looks like the mustache template for the go-experimental generator differs fairly significantly from the go generator, and it's not clear if I should also update go-experimental templates/samples as well.

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.

cc go technical committee: @antihax @bvwells @grokify @kemokemo @bkabrda

arvindth · 2020-04-13T17:00:13Z

modules/openapi-generator/src/test/java/org/openapitools/codegen/go/GoClientOptionsTest.java

@@ -45,10 +45,11 @@ protected void verifyOptions() {
 verify(clientCodegen).setPackageName(GoClientOptionsProvider.PACKAGE_NAME_VALUE);
 verify(clientCodegen).setWithGoCodegenComment(GoClientOptionsProvider.WITH_GO_CODEGEN_COMMENT_VALUE);
 verify(clientCodegen).setWithXml(GoClientOptionsProvider.WITH_XML_VALUE);
- verify(clientCodegen).setWithXml(GoClientOptionsProvider.ENUM_CLASS_PREFIX_VALUE);
+ verify(clientCodegen).setEnumClassPrefix(GoClientOptionsProvider.ENUM_CLASS_PREFIX_VALUE);


I didn't see any reason this repeated the setWithXml from the previous line, so I think this was a bug.

Yeah, apparently this was just previously running this twice:

verify(clientCodegen).setWithXml(true);

arvindth · 2020-04-19T18:05:08Z

Looks like circleci failed with the assumption that go-experimental also needs to support the same command line options, so I'll try to update it.

bpicolo · 2020-05-17T17:36:28Z

@arvindth: Just a spectator, but thanks for taking this on! Hit this in attempts to do some testing with openapi this week.

arvindth · 2020-05-18T00:44:59Z

Hi @antihax @bvwells @grokify @kemokemo @bkabrda
Apologies for the many debug commits the last week while I tried to fix my circleCI failures. I should have held off on cc'ing you until after a green build.

The changes to go-petstore were fairly simple based on the original code snippet from #2294. However, the recent change to go-experimental (#4787 by @zippolyte) that added a builder pattern made it pretty difficult to enable interfaces. I had to make some breaking changes to enable generateInterfaces support.

Summary of changes to go-experimental:

I've broken apart the request builder and the execute method and moved the execute method from the request to the service.
I think that the builder pattern should only concern itself with building the object, and not acting on it, so I feel my change makes sense.
In addition, now we only have to generate an interface for the service, which only has 2 methods: getRequest and execute. Since the goal of adding interfaces is mainly to allow mocking, it makes more sense to me to only mock these 2 methods rather than the entire request.
I've updated the go-experimental samples to actually generate interfaces and added a mocked api and test to utilize one of the generated interfaces.

bkabrda · 2020-05-21T08:26:49Z

@arvindth thanks for sending the PR. I'm not immediately opposed to this change, but I'd like you to provide more context. You state:

I think that the builder pattern should only concern itself with building the object, and not acting on it, so I feel my change makes sense.

But why do you think that? Have you seen this as a pattern elsewhere? The builder pattern similar to what go has right now is used in the Java clients, so changing this would introduce inconsistency across languages, which I don't believe to be beneficial (unless there's a good language-based reason for it).

Additionally, your approach makes using the generated client less practical (2 lines for each API call instead of one). Could you summarize the benefits it brings? Maybe we could satisfy your goals in a different way while keeping the current approach.

arvindth · 2020-05-22T15:20:08Z

Hi @bkabrda thanks for the review. I suppose I really had 2 reasons for this specific change choice:

The builder pattern I've seen pretty much everywhere looks something like:

var obj = new ObjectType().arg1(arg1Val).arg2(arg2Val).build()
// build() is intended to validate the the build obj is actually valid per some criteria, 
// but not really use it in any way.

// here we want to do something with the newly constructed obj

The chained request methods in the generated code are acting on the Request object, not on the Service. But Execute, though, at least to me, feels like it should be on the service object and not the request object, to separate out the service from the data it acts on. Just feels like the right separation of concerns.

As for benefits, the main reason to embark on this change was to allow for easy mocking of the call to the backend. If we leave it as it was, then it looks like the entire request would need to be mocked, which doesn't really make sense to me. Some requests have several setter methods, but only the Execute method calls the backend. I didn't see the need to require mocking the Request methods when it's really only Execute that needs to be mocked in order to unit test applications that use the generated client library.

I do see your point about diverging from the Java client, and I wouldn't be opposed to putting up a PR at some point to do the same thing there to keep it aligned.

I also see your point about needing to use 2 lines for the API instead of one, but it feels like that's the way builders should work, to separate the building from the usage. That said, I'm definitely open to any suggestions. I'm new'ish to go, so it's very possible I'm overlooking a way to achieve (2) without having to do (1).

PS. We currently use a custom fork with this interface support for the go (not experimental) generator. I'm also ok with reverting the go-experimental changes completely and having the interface support only on the main go one. However, I'm not sure if that's the right thing to do, especially if the intention is for go-experimental to become the main generator in the future.

arvindth · 2020-06-16T18:31:45Z

@bkabrda any comments on my response above? Please let me know if you have any additional questions, concerns or suggestions.

jimschubert

The mock will need to be removed from the sample generation script as this will prevent other contributors from generating samples until they've installed go. Those scripts should be used only for executing our CLI and outputting samples.

I think the wget and setup of go1.14 can be removed from CircleCI as it's not used and will only increase our build times.

I also agree with @bkabrda that the double-invocation to make a single API call is not intuitive and complicates the usage of this client. I'd prefer to have the built-in client provide users a simple use case, and users with more complicated mocking requirements that are better suited to invoking multiple functions for a single call do this via custom templates rather than incorporating them into the built-in templates.

jimschubert · 2020-08-01T13:43:13Z

CI/circle_parallel.sh

+ # Install golang version 1.14
+ go version
+ sudo mkdir /usr/local/go1.14
+ wget -c https://dl.google.com/go/go1.14.linux-amd64.tar.gz -O - | sudo tar -xz -C /usr/local/go1.14


Why is all this necessary?

it's probably to run checks against latest version of go, but this looks far out of scope for this projects and IMO shouldn't be here, that's another thing to remember and maintain.

Yeah, I deliberately put this in the last commit as a POC to showcase the usage of the mocks in the test. I can definitely take it out.

jimschubert · 2020-08-01T13:44:19Z

bin/go-experimental-petstore.sh

+
+# Generate mocks for testing the samples
+cd samples/client/petstore/go-experimental/go-petstore/
+echo "which go = $(which go)"
+go version
+gopath=$(go env GOPATH)
+GO111MODULE=on go get github.com/golang/mock/mockgen@v1.4.3
+mockgencmd="${gopath%%:*}/bin/mockgen -source=api_pet.go -destination=mock_api_pet.go -package petstore"
+echo "Running $mockgencmd"
+$mockgencmd
+cd -


This does not belong in the generation script.

arvindth

I'll have to think through how I can change the go-experimental generation to go back to a single api call.

arvindth · 2020-08-05T15:23:08Z

CI/circle_parallel.sh

+ # Install golang version 1.14
+ go version
+ sudo mkdir /usr/local/go1.14
+ wget -c https://dl.google.com/go/go1.14.linux-amd64.tar.gz -O - | sudo tar -xz -C /usr/local/go1.14


Yeah, I deliberately put this in the last commit as a POC to showcase the usage of the mocks in the test. I can definitely take it out.

@partkyle

* Fix OpenAPITools#2294 with code suggested by @partkyle

- Use default generateInterfaces=false to suppress interfaces

* Backward incompatible since it changes the api method signatures significantly. * Adheres to a builder pattern more strictly. Execute now runs on the req object returned by the builder, instead of the builder itself * Prettify/align method comments

* Also update to include prettier method comments

* Samples now contain interfaces, using backward incompatible method signatures. * Add one test for mocked methods * Switch to go 1.14 for circleci to allow generating mocks

- add Execute redirection from request back up to service

jimschubert · 2020-08-23T19:04:43Z

@arvindth I see you're still working on this. Please just ping whenever it's ready for another review.

arvindth

@jimschubert This PR is ready for another review. Changes since the last review are in the last 5 commits:

Updated the go-experimental template to re-add the original Execute method to maintain backward compatibility and single api call invocation pattern.
Undid the changes to CI/circle_parallel.sh & bin/go-experimental-petstore.sh that were used to autogenerate mocks.
Added a manually generated mock in the last commit to test that go-experimental interfaces are generated correctly.
- This no longer requires contributors to install go unless they're actually running the go integration tests.

arvindth · 2020-08-23T21:17:10Z

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

 }
-{{/isPathParam}}
-{{/allParams}}
+


Rather than requiring callers to call the new backward incompatible *Execute methods in Service directly, I've re-added the original Execute method in *Request, which just redirects the call to the Service's *Execute, in order to maintain backward compatibility.

arvindth · 2020-08-31T02:29:45Z

Hi @jimschubert, do you think you may be able to look at this PR soon?

jimschubert · 2020-08-31T02:35:18Z

Yeah, I'll try to take a look in the next few days. I didn't see your last ping, but the past week has been pretty full.

jimschubert

LGTM

jimschubert · 2020-09-01T01:41:37Z

I think this looks good.

It does expose the request object publicly when the new option is false, but as Execute is called indirectly through the request object, I think that's fine.

We'll probably want to have one or two of the samples generate with the interface option set to false, but I'll do that via another PR.

@antihax @bvwells @grokify @kemokemo @bkabrda

jimschubert · 2020-09-01T01:52:00Z

We'll probably want to have one or two of the samples generate with the interface option set to false, but I'll do that via another PR.

Oh, just realized as I went to change this that bin/configs/go-experimental-go-petstore-oas2.yaml is already doing this. :)

arvindth · 2020-09-01T04:20:10Z

Thanks @jimschubert

* master: (89 commits) add structPrefix support to go-experimental (#7327) Add a link to SmartHR Tech Blog (#7324) Revert "Correct allOf with only one child schema (no discriminator)" (#7323) Correct allOf with only one child schema (no discriminator) (#6901) [Go]: Interface definitions for api functions (#5914) Update bug_report.md (#7320) update samples [Java][Client] Use java8 OffsetDateTime for clients (#7190) [java] Intro openApiNullable property to enable/disable OpenAPI Jackson Nullable library (#6154) [Spring Boot] update dependencies, mark java8 option as deprecated (#7306) Remove dot in golang type (#7307) [doc] Document usage of post-process file feature (#7315) fix http bear auth documentation for go clinets (#7312) [Extensions][Go][Java] Test x-auth-id-alias (#6642) [php-slim4] Move config to a separate file (#6971) [C][Client][Clang Static Analyzer] Remove the useless free operation for (#7309) Fix typescript-node generation when only models are generated (#7127) update spring config to use java8 (#7308) [C][Client][Clang Static Analyzer] Fix uninitialized argument value (#7305) [Java] remove deprecated jackson classes (#7304) ...

* master: (984 commits) add structPrefix support to go-experimental (OpenAPITools#7327) Add a link to SmartHR Tech Blog (OpenAPITools#7324) Revert "Correct allOf with only one child schema (no discriminator)" (OpenAPITools#7323) Correct allOf with only one child schema (no discriminator) (OpenAPITools#6901) [Go]: Interface definitions for api functions (OpenAPITools#5914) Update bug_report.md (OpenAPITools#7320) update samples [Java][Client] Use java8 OffsetDateTime for clients (OpenAPITools#7190) [java] Intro openApiNullable property to enable/disable OpenAPI Jackson Nullable library (OpenAPITools#6154) [Spring Boot] update dependencies, mark java8 option as deprecated (OpenAPITools#7306) Remove dot in golang type (OpenAPITools#7307) [doc] Document usage of post-process file feature (OpenAPITools#7315) fix http bear auth documentation for go clinets (OpenAPITools#7312) [Extensions][Go][Java] Test x-auth-id-alias (OpenAPITools#6642) [php-slim4] Move config to a separate file (OpenAPITools#6971) [C][Client][Clang Static Analyzer] Remove the useless free operation for (OpenAPITools#7309) Fix typescript-node generation when only models are generated (OpenAPITools#7127) update spring config to use java8 (OpenAPITools#7308) [C][Client][Clang Static Analyzer] Fix uninitialized argument value (OpenAPITools#7305) [Java] remove deprecated jackson classes (OpenAPITools#7304) ...

arvindth force-pushed the ath-add-interfaces branch from e6538d3 to 70c24e1 Compare April 13, 2020 16:33

arvindth commented Apr 13, 2020

View reviewed changes

arvindth force-pushed the ath-add-interfaces branch from 70c24e1 to 90d4447 Compare April 19, 2020 17:45

arvindth force-pushed the ath-add-interfaces branch 15 times, most recently from 078580e to 7e868a4 Compare May 17, 2020 15:11

arvindth force-pushed the ath-add-interfaces branch from 018af21 to 4769d96 Compare May 17, 2020 22:47

jimschubert requested changes Aug 1, 2020

View reviewed changes

arvindth commented Aug 5, 2020

View reviewed changes

arvindth added 3 commits August 22, 2020 15:20

[Go]: Interface definitions for api functions

bf0968b

* Fix OpenAPITools#2294 with code suggested by @partkyle

Use interface in client struct

a3454a3

Fixups to align with current master

1b50c28

arvindth added 2 commits August 22, 2020 15:20

Add commandline option to enable generateInterfaces

7bca6e0

Update go-petstore & oa3/go-petstore samples with prettier comments

9822d5c

- Use default generateInterfaces=false to suppress interfaces

arvindth force-pushed the ath-add-interfaces branch from 4769d96 to 8f1f408 Compare August 23, 2020 13:12

arvindth added 7 commits August 23, 2020 13:41

Enable generateInterfaces and update samples for go-petstore-withxml

9653a67

Method signature refactors for go-experimental (and openapi3) samples

fc00c98

* Also update to include prettier method comments

Enable generateInterfaces for go-experimental samples

16c2141

* Samples now contain interfaces, using backward incompatible method signatures. * Add one test for mocked methods * Switch to go 1.14 for circleci to allow generating mocks

Undo mock additions in CI/backward incompatibility in tests

9ac3de2

Address PR comments: fix backward incompatibility in templates

3398492

- add Execute redirection from request back up to service

Regenerate go-experimental/oas2 samples w/o interfaces

4945c11

arvindth force-pushed the ath-add-interfaces branch from f26eebb to 4945c11 Compare August 23, 2020 18:42

arvindth added 2 commits August 23, 2020 16:01

Regenerate go-experimental/oas2 samples with interfaces

fdc4599

Add mock to test go-experimental interface

9d6ca29

arvindth force-pushed the ath-add-interfaces branch from af471d8 to 9d6ca29 Compare August 23, 2020 22:00

arvindth commented Aug 23, 2020

View reviewed changes

jimschubert self-assigned this Aug 31, 2020

jimschubert added this to the 5.0.0 milestone Aug 31, 2020

jimschubert approved these changes Sep 1, 2020

View reviewed changes

jimschubert added the Client: Go label Sep 1, 2020

jimschubert merged commit ab5b0fa into OpenAPITools:master Sep 1, 2020

arvindth deleted the ath-add-interfaces branch September 1, 2020 02:21

wing328 changed the title ~~[Go]: Interface definitions for api functions~~ [Go] Interface definitions for api functions Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Go] Interface definitions for api functions #5914

[Go] Interface definitions for api functions #5914

arvindth commented Apr 13, 2020

arvindth Apr 13, 2020

jimschubert Sep 1, 2020

arvindth commented Apr 19, 2020

bpicolo commented May 17, 2020

arvindth commented May 18, 2020 •

edited

Loading

bkabrda commented May 21, 2020

arvindth commented May 22, 2020 •

edited

Loading

arvindth commented Jun 16, 2020

jimschubert left a comment

jimschubert Aug 1, 2020

agilob Aug 1, 2020

arvindth Aug 5, 2020

jimschubert Aug 1, 2020

arvindth left a comment

arvindth Aug 5, 2020

jimschubert commented Aug 23, 2020

arvindth left a comment

arvindth Aug 23, 2020

arvindth commented Aug 31, 2020

jimschubert commented Aug 31, 2020

jimschubert left a comment

jimschubert commented Sep 1, 2020

jimschubert commented Sep 1, 2020

arvindth commented Sep 1, 2020

[Go] Interface definitions for api functions #5914

[Go] Interface definitions for api functions #5914

Conversation

arvindth commented Apr 13, 2020

PR checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindth commented Apr 19, 2020

bpicolo commented May 17, 2020

arvindth commented May 18, 2020 • edited Loading

bkabrda commented May 21, 2020

arvindth commented May 22, 2020 • edited Loading

arvindth commented Jun 16, 2020

jimschubert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimschubert commented Aug 23, 2020

arvindth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindth commented Aug 31, 2020

jimschubert commented Aug 31, 2020

jimschubert left a comment

Choose a reason for hiding this comment

jimschubert commented Sep 1, 2020

jimschubert commented Sep 1, 2020

arvindth commented Sep 1, 2020

arvindth commented May 18, 2020 •

edited

Loading

arvindth commented May 22, 2020 •

edited

Loading