Resolve several issues in generated Go code #8491

aeneasr · 2021-01-20T21:24:39Z

This PR resolves several generated Go-client code issues which resolve compilation errors, improve return and field types (pointer vs value) to better match idiomatic Go-style.

Closes #8489
See #8489
See #8485

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master, 5.1.x, 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@antihax @grokify @kemokemo @jirikuncar

aeneasr · 2021-01-23T12:39:25Z

I don't understand why this fails. The go tests run locally fine. Any ideas?

aeneasr · 2021-01-25T09:40:44Z

@antihax @grokify @kemokemo @jirikuncar this patch includes some major improvements to the Go generated client code. It resolves several bugs and improves readability of the consumed go code by using best practice, idiomatic, Go patterns. I would appreciate a review and potentially some help around resolving the CI issue as I can not reproduce it locally!

samples/client/petstore/go/go-petstore/README.md

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

aeneasr · 2021-03-11T10:00:39Z

I know that reviewing large PRs is very straining. Unfortunately, the Go generator is simply broken at the moment and this PR addresses so many issues in the generated code. I would be really happy to get some pointers what I need to do next in order to get this PR through.

pmcgleenon · 2021-03-22T21:20:21Z

I'm also getting 405/404 errors when running the integration tests. When I checked it out, the tests were running against the server "petstore.swagger.io".

Looking at what happens during the CI script, it adds this entry to /etc/hosts
127.0.0.1 petstore.swagger.io

Then it starts a docker container "swaggerapi/petstore" which I guess is the local server that we need, so the tests are communicating with the server inside the docker image

Here's the CI script
https://github.com/cognitedata/openapi-generator/blob/fix/cogniteTypes/circle.yml

@aeneasr is it worth a try with a similar setup to see if something obvious is going causing the test failure?

wing328 · 2021-03-23T02:27:31Z

@aeneasr Thanks again for the PR. Let me review shortly. We were busy with the v5.1.0 release.

wing328 · 2021-03-23T02:34:47Z

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

-	_ioutil "io/ioutil"
-	_nethttp "net/http"
-	_neturl "net/url"
+	"context"


What if one of the parameter names in the operation is context? Will that result in issues after this change?

iirc all methods used are upper-case as they would not be exported otherwise, meaning that context can not conflict with one of those. However, I might have missed something - do you have a specific place in mind that could conflict here?

Ok, so I looked into this. The thing is, context is only used in a few places - primarily in structs, variable declarations, and function signatures:

type AdminApiApiCreateIdentityRequest struct { ctx context.Context ApiService *AdminApiService createIdentity *CreateIdentity } var ( _ context.Context ) func (a *AdminApiService) CreateIdentity(ctx context.Context) AdminApiApiCreateIdentityRequest { return AdminApiApiCreateIdentityRequest{ ApiService: a, ctx: ctx, } }

In all of those cases there can never be an overshadowing variable clash. So yes, this is safe!

wing328 · 2021-03-23T02:40:09Z

modules/openapi-generator/src/main/resources/go/client.mustache

@@ -447,6 +447,13 @@ func reportError(format string, a ...interface{}) error {
 	return fmt.Errorf(format, a...)
 }

+// Prevent trying to import "bytes"


Not sure what you mean by this. Any issues using bytes?

Right, this is a bit confusing. The problem is that in another file bytes might or might not be needed as an import. By defining the function here, as bytes is always required then, we avoid unused import / undefined import. This is the same as fmt.Errorf but the comment can probably be improved:

Suggested change

// Prevent trying to import "bytes"

// A wrapper for strict JSON decoding

wing328 · 2021-03-23T02:41:19Z

modules/openapi-generator/src/main/resources/go/model_oneof.mustache

@@ -1,14 +1,16 @@
 // {{classname}} - {{#description}}{{{description}}}{{/description}}{{^description}}struct for {{{classname}}}{{/description}}
 type {{classname}} struct {
 	{{#oneOf}}
-	{{{.}}} *{{{.}}}
+	{{#lambda.titlecase}}{{{.}}}{{/lambda.titlecase}} *{{{.}}}


Do you have a mininal spec to show the issue related to this fix?

We may better fix it in the Go client generator (java class) instead.

Yes, so the problem is if we have simple types in the oneOf (bool, float, string) as we generate

type UiNodeInputAttributesValue struct { bool *bool float32 *float32 string *string }

from

"uiNodeInputAttributesValue": { "oneOf": [ { "type": "string" }, { "type": "number" }, { "type": "boolean" } ] },

Using the upper case variant here fixes that:

type UiNodeInputAttributesValue struct { Bool *bool Float32 *float32 String *string }

aeneasr · 2021-03-25T07:37:00Z

Thank you for your help! I've addressed everything, and tests now pass also. From my side this is good to go :)

We will be switching to this generator for all of https://github.com/ory/ / https://www.ory.sh and given that we have large and complex api specs I'll very likely come back with more fixes and improvements :)

See OpenAPITools#8489

Improves the way pass-by-value and pass-by-reference variables are used. Closes OpenAPITools#8489

If polymorphism without discriminator was used, the previous code was unable to properly decode the vaules. By using a strict decoder, which rejects unknown fields, type guessing now works.

aeneasr · 2021-11-01T17:59:25Z

Looks like this just won't get addressed @wing328 , so I'll just go ahead and close this!

tbalthazar · 2021-11-02T11:27:55Z

This PR is a lot of work and has lots of nice improvements, thanks @aeneasr.
I'm grateful for all the work that has already been put on the project by the maintainers and contributors and I'd love to understand why this hasn't been merged. Has support for the Go generator been discontinued? Or is everybody just super busy? It would help clarify the current status of the Go generator. Cc: @wing328 @jirikuncar

aeneasr · 2022-01-02T12:43:32Z

Thank you @tbalthazar ! I have been asked by several people to reopen the PR in hopes it gets merged at some point. So reopening!

@wing328 I know how difficult it is to maintain such a large project. �If there's anything I can do to help and get things resolved please let me know. I'll do my best :)

wing328 · 2022-01-02T13:18:51Z

@aeneasr first of all thanks again for the PR. I left some feedback before but didn't have time to review your replies.

Can you please merge the latest master into your branch and I'll try to take another look this week? Thanks again for your work.

aeneasr · 2022-01-02T13:20:32Z

No problem, I'll get on it immediately!

# Conflicts: # modules/openapi-generator/src/main/resources/go/api.mustache # modules/openapi-generator/src/main/resources/go/model_simple.mustache # samples/client/petstore/go/go-petstore/api_another_fake.go # samples/client/petstore/go/go-petstore/api_fake.go # samples/client/petstore/go/go-petstore/api_fake_classname_tags123.go # samples/client/petstore/go/go-petstore/api_pet.go # samples/client/petstore/go/go-petstore/api_store.go # samples/client/petstore/go/go-petstore/api_user.go # samples/openapi3/client/extensions/x-auth-id-alias/go-experimental/api_usage.go # samples/openapi3/client/petstore/go/go-petstore/api_another_fake.go # samples/openapi3/client/petstore/go/go-petstore/api_default.go # samples/openapi3/client/petstore/go/go-petstore/api_fake.go # samples/openapi3/client/petstore/go/go-petstore/api_fake_classname_tags123.go # samples/openapi3/client/petstore/go/go-petstore/api_pet.go # samples/openapi3/client/petstore/go/go-petstore/api_store.go # samples/openapi3/client/petstore/go/go-petstore/api_user.go # samples/openapi3/client/petstore/go/go-petstore/model_nullable_class.go

aeneasr · 2022-01-03T08:46:38Z

@wing328 issues resolved, CI passes! It's good to go! :)

modules/openapi-generator/src/main/resources/go/model_simple.mustache

samples/openapi3/client/petstore/go/go-petstore/model_pet.go

jirikuncar

nit: can you please replace spaces with a tab?

Co-authored-by: Jiri Kuncar <[email protected]>

aeneasr · 2022-01-03T11:32:55Z

Done :)

aeneasr · 2022-01-03T14:13:07Z

CI passed again, everything green, good to go! :)

wing328 · 2022-01-11T09:23:06Z

@jirikuncar thanks for reviewing the change.

@aeneasr thanks again for your work. Can you please PM me via Slack for a few quick questions when you've time? https://join.slack.com/t/openapi-generator/shared_invite/enQtNzAyNDMyOTU0OTE1LTY5ZDBiNDI5NzI5ZjQ1Y2E5OWVjMjZkYzY1ZGM2MWQ4YWFjMzcyNDY5MGI4NjQxNDBiMTlmZTc5NjY2ZTQ5MGM

My timezone is +0800 (Hong Kong).

wing328 · 2022-01-21T08:54:04Z

Tested locally and the result is good. Thanks for the PR and sorry again for the delay in reviewing the change

aeneasr · 2022-01-21T09:09:46Z

@wing328 thank you so much for reviewing and merging this and reaching out on Slack!!

aeneasr force-pushed the fix-go branch 2 times, most recently from 9369c07 to ec11480 Compare January 23, 2021 11:58

jirikuncar reviewed Feb 2, 2021

View reviewed changes

samples/client/petstore/go/go-petstore/README.md Show resolved Hide resolved

jirikuncar reviewed Feb 2, 2021

View reviewed changes

modules/openapi-generator/src/main/resources/go/api.mustache Show resolved Hide resolved

wing328 added Client: Go Issue: Bug labels Mar 23, 2021

wing328 added this to the 5.1.1 milestone Mar 23, 2021

wing328 reviewed Mar 23, 2021

View reviewed changes

aeneasr force-pushed the fix-go branch 2 times, most recently from b30cbed to 29ecad8 Compare March 24, 2021 22:37

aeneasr added 12 commits April 6, 2021 17:14

[go] use regular stdlib import names

f9f0b59

[go] support primitive oneOf types

d07fd80

See OpenAPITools#8489

[go] improve pbv/pbr handling

0697625

Improves the way pass-by-value and pass-by-reference variables are used. Closes OpenAPITools#8489

[go] improve generated documentation

14593ea

[go] adopt pointer changes in interface

b22ed9a

[go] regenerate sample

002071e

[go] resolve pointer issues

0c5e5ae

[go] regenerate clients and avoid pointers on primitive return values

883605d

[go] improve Exec() return value handling

4d06e02

[go] regernate files

f71f125

[go] use go modules

f377d35

[go] properly handle polymorph decode

31555e5

If polymorphism without discriminator was used, the previous code was unable to properly decode the vaules. By using a strict decoder, which rejects unknown fields, type guessing now works.

wing328 added this to the 5.3.1 milestone Oct 25, 2021

aeneasr closed this Nov 1, 2021

aeneasr reopened this Jan 2, 2022

aeneasr added 2 commits January 2, 2022 15:42

[go] resolve merge conflicts

e039e30

jirikuncar reviewed Jan 3, 2022

View reviewed changes

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

jirikuncar reviewed Jan 3, 2022

View reviewed changes

samples/openapi3/client/petstore/go/go-petstore/model_pet.go Outdated Show resolved Hide resolved

jirikuncar suggested changes Jan 3, 2022

View reviewed changes

aeneasr and others added 2 commits January 3, 2022 13:32

[go] Replace spaces with tabs

e6e3f6d

Co-authored-by: Jiri Kuncar <[email protected]>

[go] Replace spaces with tabs

3139bb5

Co-authored-by: Jiri Kuncar <[email protected]>

jirikuncar approved these changes Jan 3, 2022

View reviewed changes

wing328 merged commit 93488f4 into OpenAPITools:master Jan 21, 2022

Benoit12345 mentioned this pull request Jan 21, 2022

[BUG] [GO] Unknown newStrictDecoder function with model only #11374

Open

rhafer mentioned this pull request Jan 27, 2022

Support for Group Management via Graph owncloud/ocis#2996

Merged

15 tasks

the-akhil-nair mentioned this pull request Jan 31, 2022

[GO] Fix for Pass by value/Pass by Reference changes which is breaking the go client #11466

Merged

5 tasks

meghana-rajashekar mentioned this pull request Jan 31, 2022

[BUG][GO] Pass by value/Pass by Reference changes breaking Go client #11467

Closed

6 tasks

maros7 mentioned this pull request Mar 10, 2022

[BUG][GO] Bug generating Get<name>Ok method when using $ref #11848

Open

6 tasks

icubbon mentioned this pull request Jul 22, 2022

[BUG] [GO] Issues when downloading files #12810

Open

5 tasks

danielgtaylor mentioned this pull request Aug 23, 2022

feat!: update OpenAPI Generator, regenerate latest SDK istreamlabs/go-sdk#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve several issues in generated Go code #8491

Resolve several issues in generated Go code #8491

aeneasr commented Jan 20, 2021

aeneasr commented Jan 23, 2021

aeneasr commented Jan 25, 2021

aeneasr commented Mar 11, 2021

pmcgleenon commented Mar 22, 2021

wing328 commented Mar 23, 2021

wing328 Mar 23, 2021

aeneasr Mar 23, 2021

aeneasr Mar 24, 2021

wing328 Mar 23, 2021

aeneasr Mar 23, 2021

wing328 Mar 23, 2021

aeneasr Mar 24, 2021

aeneasr commented Mar 25, 2021

aeneasr commented Nov 1, 2021

tbalthazar commented Nov 2, 2021

aeneasr commented Jan 2, 2022

wing328 commented Jan 2, 2022 •

edited

Loading

aeneasr commented Jan 2, 2022

aeneasr commented Jan 3, 2022

jirikuncar left a comment •

edited

Loading

aeneasr commented Jan 3, 2022

aeneasr commented Jan 3, 2022

wing328 commented Jan 11, 2022

wing328 commented Jan 21, 2022

aeneasr commented Jan 21, 2022

	// Prevent trying to import "bytes"
	// A wrapper for strict JSON decoding

Resolve several issues in generated Go code #8491

Resolve several issues in generated Go code #8491

Conversation

aeneasr commented Jan 20, 2021

PR checklist

aeneasr commented Jan 23, 2021

aeneasr commented Jan 25, 2021

aeneasr commented Mar 11, 2021

pmcgleenon commented Mar 22, 2021

wing328 commented Mar 23, 2021

wing328 Mar 23, 2021

Choose a reason for hiding this comment

aeneasr Mar 23, 2021

Choose a reason for hiding this comment

aeneasr Mar 24, 2021

Choose a reason for hiding this comment

wing328 Mar 23, 2021

Choose a reason for hiding this comment

aeneasr Mar 23, 2021

Choose a reason for hiding this comment

wing328 Mar 23, 2021

Choose a reason for hiding this comment

aeneasr Mar 24, 2021

Choose a reason for hiding this comment

aeneasr commented Mar 25, 2021

aeneasr commented Nov 1, 2021

tbalthazar commented Nov 2, 2021

aeneasr commented Jan 2, 2022

wing328 commented Jan 2, 2022 • edited Loading

aeneasr commented Jan 2, 2022

aeneasr commented Jan 3, 2022

jirikuncar left a comment • edited Loading

Choose a reason for hiding this comment

aeneasr commented Jan 3, 2022

aeneasr commented Jan 3, 2022

wing328 commented Jan 11, 2022

wing328 commented Jan 21, 2022

aeneasr commented Jan 21, 2022

wing328 commented Jan 2, 2022 •

edited

Loading

jirikuncar left a comment •

edited

Loading