[Go] Optional values should be pointers

[langston-barrett]

Description

If a field of a struct isn't specified as required, then it should be a pointer to that value so that one can differentiate between unset by the user or set to the "zero value". See the following Go playground: https://play.golang.org/p/85r_JbH5XVr

See also e.g. this Stackoverflow answer.

openapi-generator version

Docker container

OpenAPI declaration file content or url

---
openapi: 3.0.0
info:
  title: unset
  version: 0.1.0

paths:
  "/foo":
    get:
      summary: foo
      responses:
        '200':
          description: OK
          content:
            application/xml:
              schema:
                "$ref": "#/components/schemas/Bar"

components:
  schemas:
    Bar:
      properties:
        X:
          type: integer
        Y:
          type: integer
      required:
        - X

Related issues/PRs

Suggest a fix/enhancement

[langston-barrett]

Would this be as simple to fix as changing a single line of a template?

[grokify]

This would be a good update.

It may be as simple as changing the model template. If so, you can just update it, rebuild the CLI and run it against your spec to see if it works.

The template is here:

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/go/model.mustache

Here are some steps to do this. For the example, I've assumed you have a fork and want to send a PR ;)

$ git clone https://github.com/siddharthist/openapi-generator
$ cd openapi-generator
$ git checkout -b go/client/fix/optional-as-pointer
$ vi modules/openapi-generator/src/main/resources/go/model.mustache
$ mvn clean package
$ java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar help

Use the resulting openapi-generator-cli.jar to rebuild your client and see if it works as expected.

If so, then do the following to send a PR with the following:

$ bin/go-petstore.sh
$ mvn verify -f samples/client/petstore/go
$ mvn verify
$ git add
$ git commit -m 'use pointers for optional properties'
$ bin/util/ensure-up-to-date
$ git push origin go/client/fix/optional-as-pointer

If this is not in the petstore sample app, you can look into updating the sample Petshop Swagger 2.0 spec listed in bin/go-petstore.sh so your change is reflected in the generated go-petstore sample client.

If ensure-up-to-date creates new files or tells you to update your code, do it and run it again. Rinse and repeat until it says your working tree is clean.

Even if you edit only the model.mustache file, this may result in more files b/c you are rebuilding the Petstore sample and ensure-up-to-date may have you update other samples.

Then push to your fork and send a PR.

[langston-barrett]

@grokify Wow, thank you for the thorough instructions! I hope you have this saved as a "quick reply" 😄 I'd like to solve this, I'll report back here (within a week) if I'm able to.

[langston-barrett]

The fix is indeed one line. I have a branch here: https://github.com/siddharthist/openapi-generator/tree/go/client/fix/optional-as-pointer. I tested this with my project and it works fine.

Unfortunately, it looks like it will take more effort than I can justify to fix the tests: many of the fields are optional and now must be converted to pointers. Example conversion in my branch.

[grokify]

@siddharthist Thanks for looking into this.

There aren't actually that many tests so it should be reasonably easy to update. The tests are located here:

samples/client/petstore/go/auth_test.go
samples/client/petstore/go/fake_api_test.go
samples/client/petstore/go/pet_api_test.go
samples/client/petstore/go/store_api_test.go
samples/client/petstore/go/user_api_test.go

[grokify]

@siddharthist I just put together a contribution quickstart here:

https://medium.com/ringcentral-developers/openapi-generator-for-go-contribution-quickstart-8cc72bf37b53

Take a look. I'll also take a look at your change when I have time.

[antihax]

Was this not resolved with the optional package?
It may be better to extend that, since we could have NULL as a valid value in addition to NotSet?

[mattes]

I noticed the generated Go package uses github.com/antihax/optional for optional fields. This requires me to pass in a string with optional.NewString("foobar"). Example:

import "github.com/antihax/optional"

type UpdatePetWithFormOpts struct {
	Name optional.String
	Status optional.String
}

s := UpdatePetWithFormOpts{
	Name: optional.NewString("foobar"),
}

I found Google's approach more intuitive. They define a string as an interface{}, which allows something like this:

import "google.golang.org/cloud/internal/optional"

type UpdatePetWithFormOpts struct {
	Name optional.String
	Status optional.String
}

s := UpdatePetWithFormOpts{
	Name: "foobar",
	Status: nil,
}

[antihax]

You lose compile time type checking by using interface{}.

The Google library there will panic at runtime if a wrong type is passed, which is a bad way to handle this.

[bkabrda]

So I've just hit this issue and have a couple of observations:

• This problem was not solved by using the optional package, as it is only used for Opts, not for Models.
• Even if it was used for Models, I think it would need to provide means for json (un)marshalling - if I'm not mistaken, this is something that's not possible ATM.
• Even if we did use optional for Models and it had support for json (un)marshalling, we'd still need to have a mechanism for nested references, i.e. the case when structure contains attribute that references a different structure - AFAICS this wouldn't be supported by optional right now (basically I think we'd need to use pointers for struct types).

If the 3 above points seem correct, I'd be happy to work on them; CC @antihax.

[wing328]

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

[antihax]

Pointers for models could be a good short term solution, but i still think optional would be the way to implement.

Marshaling should be trivial to implement and should also work on nested references.
See: https://www.calhoun.io/how-to-determine-if-a-json-key-has-been-set-to-null-or-not-provided/

It should also be a cleaner way for the developer to know that they are dealing with a variable that may not be defined, since it will not compile if they directly reference.

I.e.

if someModel.someInt == 0 {

would not compile and clearly show it is optional and need to be changed to

if someModel.someInt.IsSet && someModel.someInt.Value == 0 {

If it was a pointer, it would cause a panic at runtime if it was nil if not checked.

@wing328 if we do decide to keep the optional package, can it be moved into the official repository?

[bkabrda]

Ok, thanks for the response @antihax. I'll try to send a PR to optional tomorrow that would implement json (un)marshalling to get this going.

[bkabrda]

So I've been trying to implement this and I'm having some difficulties. Perhaps someone can help me with this:

Basically the defintion of the type of the struct member depends on two variables - whether or not this member is required and whether or not its nullable. I tried to do a little list summarizing what we could use in each case:

• required = true, nullable = true
  • use Attribute optional.Type `json:"attribute"` for basic types
  • use Attribute *StructType `json:"attribute"` for struct types (=> we have to be able to handle pointers anyway)
• required = true, nullable = false
  • use Attribute basicType `json:"attribute"` for basic types
  • use Attribute StructType `json:"attribute"` for struct types
• required = false, nullable = true
  • use Attribute *basicType `json:"attribute,omitempty"` for basic types (I don't see how to make this work with optional)
  • use Attribute *StructType `json:"attribute,omitempty"` for struct types
• required = false, nullable = false
  • use Attribute ??? `json:"attribute,omitempty"` for basic types
  • use Attribute ??? `json:"attribute,omitempty"` for struct types

I'm not sure if I'm getting these right, but it seems to me that using optional is not really a general solution here. I think it'd be much better to make all the struct members private and generate getter/setter functions for them based on "required" and "nullable" values (because IIUC we'll need to use pointers anyway, so using optional would only add additional complexity that wouldn't really help). Would that sound good @antihax @wing328 ? If not, I'd appreciate some guidance on how to make this work without pointers and only with optional, because I really can't seem to figure it out. Thanks!

[antihax]

Maybe my understanding is incorrect. Basic types cannot be null, but a Schema can make a type which is.

We could have a schema that is

{
  "type": "string",
  "format": "email",
  "nullable": true,
  "required": false,
}

Maybe we need to generate the logic in the model struct to handle isNull, isSet and marshalling based on the same concept as the optional package. Which is similar to the idea of using Getter/Setter but should still allow it to be Marshalled again as the members are still exported.

Should be possible to wrap it all into one struct and embed that within the models to reduce duplicate code.

Are there any generators that are fully implementing this that we could lean on for an example?

[bkabrda]

I think we may be at the point of overengineering this :)

State of Art

I've been looking at some go api clients and have found that quite a few of them use a variation of a script called "gen-accessors.go" - I'm not sure where it originally came from, but it's used e.g. in the go-github client [1]. A simple github search shows that it's used quite widely [2]. (I think it was originally created as part of go-github, but I'm not sure).

Generally, it seems that most go API clients (that I've seen) just use pointers everywhere (except for arrays and maps) in combination with omitempty and then they use getters/setters generated by gen-accessors.go. TBH, I think it's better to use pointers everywhere, even for arrays and maps, since that makes it possible to distinguish between sending an empty array/map vs not sending the array/map at all (that's not possible if it's not a pointer AFAICS).

FTR, it seems that gen-accessors.go has since been transformed into a standalone project that offers some customization [3] - but I don't think this project is used much, judging by number of Github stars.

Proposal

Let's start with making all the struct fields pointers and generate getters/setters for them, such as the ones generated by gen-accessors.go. Eventually, we can add further validation to these getters/setters to handle nullable/required cases.

Pros

• Providing API similar to what a lot of other client API libraries provide.
• Easy to implement (I think)
• Proven to work

Cons

• Possible minor code duplication/slight confusion with optional package
• Not strictly mandating the nullable/required from OpenAPI spec (but to me that's pretty hard to do in any case and it doesn't seem to be a huge problem for other API clients that I've seen, so it's a reasonable con to accept IMO)

Does that sound reasonable?

[1] https://github.com/google/go-github/blob/master/github/gen-accessors.go
[2] https://github.com/search?p=1&q=gen-accessors.go&type=Code
[3] https://github.com/tors/getters

[antihax]

Reading through that it looks like it just makes it convenient to deference values and doesn't help differentiate from null or optional values. I.e. It creates these functions:

func (p *Pony) GetAge() int64 {
	if p == nil || p.Age == nil {
		return 0
	}
	return *p.Age
}

We would also need Go installed on the system running codegen.

Looking at some of the other generators, it appears they are wrapping the types. The JavaSpring generator uses: https://github.com/OpenAPITools/jackson-databind-nullable/blob/master/src/main/java/org/openapitools/jackson/nullable/JsonNullable.java

    private JsonNullable(T value, boolean isPresent) {
        this.value = value;
        this.isPresent = isPresent;
    }

This is very similar to how the sql package handles NULL in golang which looks like this:

// NullInt64 represents an int64 that may be null.
// NullInt64 implements the Scanner interface so
// it can be used as a scan destination, similar to NullString.
type NullInt64 struct {
	Int64 int64
	Valid bool // Valid is true if Int64 is not NULL
}

// Scan implements the Scanner interface.
func (n *NullInt64) Scan(value interface{}) error {
	if value == nil {
		n.Int64, n.Valid = 0, false
		return nil
	}
	n.Valid = true
	return convertAssign(&n.Int64, value)
}

// Value implements the driver Valuer interface.
func (n NullInt64) Value() (driver.Value, error) {
	if !n.Valid {
		return nil, nil
	}
	return n.Int64, nil
}

We also have to handle nullable structs/models which the sql package does not.

[bkabrda]

So I wasn't suggesting we use the gen-accessors.go, but rather generate the same output with our templates, so we wouldn't need go installed on the system running codegen.

And yeah, I know this is not ideal, but I'm just failing to find a good way to do all this. I'm going to keep searching; if you have any specific proposals on how to make this all work, I'd be very interested to hear them.

Edit: I think we could use the approach that I suggested (make everything a pointer and create getters/setters) and combine it with custom Marshal methods for all structures. The custom marshalling would return errors if some of the conditions specified by the OpenAPI spec (nullable/required/...) were not met. Would that make sense?

[bkabrda]

@antihax any thoughts on PR #3113 that I submitted as a draft to get this issue resolved? Thanks!

[bkabrda]

FTR, my PR that aims to fix this - #3371 - was merged to the go-experimental client. Feel free to give it a shot, I'd be happy to fix any issues that you might have with the implementation.

[EleanorRigby]

@bkabrda : Which version of openapi-gen, I have to use to get his go-experimental feature?
I am using 4.0.3 . Do I need to make a new binary from source?

[bkabrda]

@EleanorRigby 4.0.3 release happened before my PR for this was merged, so you'll either need to wait for a new release or do a testing build yourself.

[qmuntal]

@bkabrda, @wing328 as per v4.1.0 optional values of structs created to act as container for optional path and body parameters still uses the antihax/optional and not the pointer approach.
I think it would be great to implement your changes also in this situation, as we are currently loosing the type safety when the parameter is a optional.Interface.

Example:

Optional approach

type LoginUserOpts struct {
	Credentials optional.Interface
}

func (a *UsersApiService) LoginUser(ctx context.Context, localVarOptionals *LoginUserOpts) (*http.Response, error) {
...
}

Pointer approach

type LoginUserOpts struct {
	Credentials *Credentials
}

func (a *UsersApiService) LoginUser(ctx context.Context, localVarOptionals *LoginUserOpts) (*http.Response, error) {
...
}

[shaxbee]

@bkabrda PR that you referenced makes all fields pointer, that makes developer experience pretty bad as it requires wrapping every field with Ptr* func, shouldn't pointers be used only for non-pointer fields?

[bkabrda]

@qmuntal yeah, I guess this would make sense to do, good point.

@shaxbee so IIUC you're saying that pointers shouldn't be used for maps/slices? I think that's a fair point and worth looking into.

Unfortunately I don't have time to work on any of these features ATM, but I know that @zippolyte is going to be working with the go-experimental templates, so he might be able to help.

[bkabrda]

I believe that this issue can be closed now thanks to #4787 and #3989.