[go-experimental] export required fields without pointer #3989

shaxbee · 2019-09-29T18:53:26Z

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Required fields are exposed without pointer, as they cannot be set to nil. This reduces boilerplate and GC pressure.
Omitempty tag is only used for fields that are neither required or nullable. This removes need for private member that breaks struct conversions. This also removes need for custom MarshalJSON that creates intermediary map increasing GC pressure.

bkabrda · 2019-09-30T09:26:18Z

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

@@ -35,14 +35,14 @@ type {{classname}} struct {
 {{#description}}
 	// {{{description}}}
 {{/description}}
-	{{name}} *{{{dataType}}} `json:"{{baseName}},omitempty"{{#withXml}} xml:"{{baseName}}{{#isXmlAttribute}},attr{{/isXmlAttribute}}"{{/withXml}}{{#vendorExtensions.x-go-custom-tag}} {{{.}}}{{/vendorExtensions.x-go-custom-tag}}`
-{{#isNullable}}	isExplicitNull{{name}} bool `json:"-"{{#withXml}} xml:"-"{{/withXml}}`{{/isNullable}}
+	{{name}} {{^required}}*{{/required}}{{{dataType}}} `json:"{{baseName}}{{^required}}{{^isNullable}},omitempty{{/isNullable}}{{/required}}"{{#withXml}} xml:"{{baseName}}{{#isXmlAttribute}},attr{{/isXmlAttribute}}"{{/withXml}}{{#vendorExtensions.x-go-custom-tag}} {{{.}}}{{/vendorExtensions.x-go-custom-tag}}`


I see two cases where your implementation is problematic:

How would this work with a field that is required and nullable? AFAICS you'd have
Foo int ``json:"foo"`` , but there's no way to serialize the explicit null in this scenario AFAICS.

For scenario where you have a non-required nullable field, how do you distinguish when the field is just null because it was initialized that way versus when the user explicitly set it null?

Yes I totally agree, there is no differentiation between undefined and null in this case, but at least we don't introduce private members to what should be data only struct.

In case of required and nullable field since omitempty tag is not there nil value will be emitted as null.

To avoid breaking copying and pasting struct we could model this as interface{} and use helper methods or accept the fact that we cannot differentiate between undefined and null till generics land in Go.

As for the required and nullable field, you won't even be able to assign nil, because the type is not pointer - it's Foo int, not Foo *int.

but at least we don't introduce private members to what should be data only struct.

Why should it be a data only structure?

I feel like this is a sacrifice of feature completeness for getting a nicer code, which I'm not in favor of.

(Just to add to my previous point, I'd love to be able to simplify this and I'm all for someone doing that. I just don't think that this PR is sufficient, as it sacrifices feature completeness for a bit less boilerplate (which is generated anyway) and a bit of improvement for GC which I honestly don't think would be noticable. Maybe there's a way to modify this PR to make it cover all the cases (a way that I don't see) and I'd be more than happy to give it a thumbsup if you found it.)

You are correct, it should be pointer type, I will add it.

Regarding copying, if you copy subset of fields instead of whole object information about field being explicitly null is lost. I also think that gc pressure shouldn't be dismissed here, on every marshaling map is allocated and each value is converted into heap-referenced GC object.

Coming to think about it - we could simply generate Nullable types and Ptr methods for object schemas and this way support explicit null.

So right now you have

{{^required}}{{^isNullable}}*{{/isNullable}}{{/required}}{{{dataType}}}

but if you want to export all nullable fields as pointers (as your last commit message suggests), you should do

{{#isNullable}}*{{/isNullable}}{{{dataType}}}

Am I missing somethings?

Coming to think about it - we could simply generate Nullable types and Ptr methods for object schemas and this way support explicit null.

I honestly don't know what you mean by that. Could you provide an example of what this would look like?

You're absolutely right, required and nullable don't mix well in this case.

Added Nullable types to the templates. I need to probably override dataType in codegen for nullable types. I need to add Get/Set operations for nullable types and SetExplicitNull method.

Nullable types implement their own MarshalJSON/UnmarshalJSON avoiding need for intermediate map.
This approach guarantees that if we copy field value the information about explicit null is not lost, eg.

src := Foo{} src.SetBarExplicitNull(true) dst := Foo{ Bar: src.Bar }

shaxbee · 2019-09-30T17:52:23Z

This is currently broken due to #3988

etherealjoy · 2019-09-30T17:58:22Z

Apparently the master changed when I merged it.

shaxbee · 2019-09-30T18:07:32Z

@etherealjoy The build on branch failed and master is broken as well :-(

etherealjoy · 2019-09-30T18:12:26Z

I regenerated it again after rebase. #4000
The #3988 solved some build issue on the test files.

shaxbee · 2019-09-30T20:40:32Z

Thanks a lot! :-)

wing328 · 2019-10-03T01:55:39Z

Updates samples via 3c4f512. Let's see how it goes.

etherealjoy · 2019-10-03T09:32:08Z

a rebase is probably needed right?

wing328 · 2019-10-03T09:37:34Z

rspec ./spec/api_client_spec.rb:158 # Petstore::ApiClient#build_collection_param fails for invalid collection format

We got the same error in the master as well.

…pty if type is not required and not nullable

shaxbee · 2019-10-03T22:52:15Z

@wing328 after rebase i'm still getting type generated incorrectly, what command do you use to generate code?

EDIT: fixed itself after mvn package :-)

bkabrda · 2019-10-04T05:55:37Z

So I think this is starting to look very promising. It may still require couple of tweaks though; I'll try to do a detailed review during today.

bkabrda

I left a couple comments inline that I think should be taken care of, but otherwise this looks good. I'll try to test the generated code next, although that will probably take me some more time.

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

shaxbee · 2019-10-08T10:13:29Z

@bkabrda is there anything else left to address?

wing328 · 2019-10-14T17:20:04Z

@shaxbee sorry for the delay. Just merged it. Thanks for your contribution.

wing328 · 2019-10-31T09:28:52Z

@shaxbee thanks for the PR, which has been included in v4.2.0 release: https://twitter.com/oas_generator/status/1189824932345069569

bkabrda reviewed Sep 30, 2019

View reviewed changes

shaxbee force-pushed the go-exp-required-fields-nullable branch from 8971b82 to 5421593 Compare September 30, 2019 17:52

shaxbee force-pushed the go-exp-required-fields-nullable branch from 5421593 to 5c12de7 Compare September 30, 2019 20:54

wing328 added Client: Go Enhancement: General labels Oct 3, 2019

shaxbee and others added 4 commits October 4, 2019 00:44

[go-experimental] export required fields without pointer, only omitem…

04dc07a

…pty if type is not required and not nullable

fix: go-experimental sample code

c784089

fix: export nullable field as pointer

5cb4bfe

update samples

8f0b608

shaxbee force-pushed the go-exp-required-fields-nullable branch from 3c4f512 to e5f3229 Compare October 3, 2019 22:45

shaxbee force-pushed the go-exp-required-fields-nullable branch from e5f3229 to 73fa582 Compare October 3, 2019 23:16

feat: nullable types

ad5dafa

shaxbee force-pushed the go-exp-required-fields-nullable branch from 73fa582 to ad5dafa Compare October 3, 2019 23:17

bkabrda approved these changes Oct 4, 2019

View reviewed changes

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

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

add getter/setter for required fields, improve docstring

fe7ca62

shaxbee force-pushed the go-exp-required-fields-nullable branch from 4b05f17 to fe7ca62 Compare October 4, 2019 14:16

wing328 merged commit 2cab048 into OpenAPITools:master Oct 14, 2019

wing328 added this to the 4.2.0 milestone Oct 30, 2019

zippolyte mentioned this pull request Nov 7, 2019

[go-experimental] Remove duplicated line #4410

Merged

5 tasks

bkabrda mentioned this pull request Jan 28, 2020

[Go] Optional values should be pointers #522

Closed

shaxbee deleted the go-exp-required-fields-nullable branch February 10, 2020 06:56

bkabrda mentioned this pull request Feb 11, 2020

[BUG][go-experimental] Explicit nulls support is broken #5278

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[go-experimental] export required fields without pointer #3989

[go-experimental] export required fields without pointer #3989

shaxbee commented Sep 29, 2019

bkabrda Sep 30, 2019

shaxbee Sep 30, 2019 •

edited

Loading

bkabrda Sep 30, 2019

bkabrda Sep 30, 2019 •

edited

Loading

shaxbee Sep 30, 2019

shaxbee Sep 30, 2019

bkabrda Sep 30, 2019 •

edited

Loading

shaxbee Oct 3, 2019 •

edited

Loading

shaxbee commented Sep 30, 2019

etherealjoy commented Sep 30, 2019

shaxbee commented Sep 30, 2019

etherealjoy commented Sep 30, 2019

shaxbee commented Sep 30, 2019

wing328 commented Oct 3, 2019

etherealjoy commented Oct 3, 2019

wing328 commented Oct 3, 2019

shaxbee commented Oct 3, 2019 •

edited

Loading

bkabrda commented Oct 4, 2019

bkabrda left a comment

shaxbee commented Oct 8, 2019

wing328 commented Oct 14, 2019 •

edited

Loading

wing328 commented Oct 31, 2019

[go-experimental] export required fields without pointer #3989

[go-experimental] export required fields without pointer #3989

Conversation

shaxbee commented Sep 29, 2019

PR checklist

Description of the PR

bkabrda Sep 30, 2019

Choose a reason for hiding this comment

shaxbee Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

bkabrda Sep 30, 2019

Choose a reason for hiding this comment

bkabrda Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

shaxbee Sep 30, 2019

Choose a reason for hiding this comment

shaxbee Sep 30, 2019

Choose a reason for hiding this comment

bkabrda Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

shaxbee Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

shaxbee commented Sep 30, 2019

etherealjoy commented Sep 30, 2019

shaxbee commented Sep 30, 2019

etherealjoy commented Sep 30, 2019

shaxbee commented Sep 30, 2019

wing328 commented Oct 3, 2019

etherealjoy commented Oct 3, 2019

wing328 commented Oct 3, 2019

shaxbee commented Oct 3, 2019 • edited Loading

bkabrda commented Oct 4, 2019

bkabrda left a comment

Choose a reason for hiding this comment

shaxbee commented Oct 8, 2019

wing328 commented Oct 14, 2019 • edited Loading

wing328 commented Oct 31, 2019

shaxbee Sep 30, 2019 •

edited

Loading

bkabrda Sep 30, 2019 •

edited

Loading

bkabrda Sep 30, 2019 •

edited

Loading

shaxbee Oct 3, 2019 •

edited

Loading

shaxbee commented Oct 3, 2019 •

edited

Loading

wing328 commented Oct 14, 2019 •

edited

Loading