[golang] Default to pointers for non-primitive types

[treeder]

It generates code like this currently:

type JobWrapper struct {
    Job  Job  `json:"job,omitempty"`
}

Same with slices and what not.

It would be nice (and probably what most people would want) to have it use pointers, eg:

type JobWrapper struct {
    Job *Job  `json:"job,omitempty"`
}

[wing328]

@treeder thanks for the feedback. I think we need to provide an option here or use vendor extension to determine using pointer for model property or not.

When I created the Go API client generator, I looked to Stripe, Twilio GO API client (and a couple more) and seems like it really depends on use case. For example, in https://github.com/stripe/stripe-go/blob/master/order.go, you will find some properties refer to the actual model while the others use model pointers.

[treeder]

Interesting. Seems pretty random there, although I'm sure it's not. They may have wanted the ability to check for nil's on certain fields rather than the zero value for them seeing as how they even have pointers on primitives. I might argue that pointers for everything (including primitives) is best for an API so you can check for nil's on every field. Without pointers, you don't know whether the user passed in an empty string or nothing for string a field, or if they passed in a 0 or nothing for a number field, etc.

[neilotoole]

I recently ran into this issue with a PR I created for go-github client. I initially implemented the datatypes without pointers, but there's a valid reason the Google guys are using pointers: sometimes you want to send the zero value (e.g. an empty string) back to the API, and sometimes you want to omit a field entirely. Pointers allow you to do this.

However, and this is a big design issue, I don't see any mechanism (I'm still a Go n00b though!) wherein swagger-codegen can support polymorphic return types if the generated code is returning a plain struct (pointers or not). I think we'll have to return an interface type, which can then be cast (by the caller) to the desired sub-type based upon the discriminator field (or using the Go type interrogation mechanisms). If we decided to go with interfaces, we could also provide getter/setter/isSet functions which would allow the user to determine if a field is really present vs just being at the zero value, and also allow the generated client code to determine whether or not a field should be sent or omitted when communicating with the endpoint.

This would likely break any existing code that relies upon the beta Go code generator. But I guess that's why it's a beta...

[wing328]

@neilotoole thanks for the feedback on this.

I'd a look at https://github.com/google/go-github/ and found the following:

1. model properties use pointer for primitive types and models (structs) (e.g. https://github.com/google/go-github/blob/master/github/issues.go#L22)

2. method arguments use pointer for models (structs) only whereas primitive types do not use pointer in method arguments, e.g. https://github.com/google/go-github/blob/master/github/gists.go#L199

I suggest we follow the same and I'm ok with this breaking change (non-backward compatible) as long as we clearly describe it in the PR to make our developers informed.

[neilotoole]

@wing328 @treeder There's some really good background reading on the go-github design choice here.

This is a significant decision, and I suggest we consider it very carefully. As you can see from the go-github conversation, they really thought hard about it as well, and were reluctant to go the pointer route, because, frankly, it makes use of the API ugly as hell. And by ugly, check out this code (from my own use of go-github):

    authReq := &github.AuthorizationRequest{
        Note:        github.String("Here's a note"),
        NoteURL:     github.String(urlString),
        Scopes:      []github.Scope{github.ScopePublicRepo, github.ScopeRepo, github.ScopeWriteRepoHook, github.ScopeUserEmail},
        Fingerprint: github.String(fingerprint),
    }

What is this github.String() function, I hear you asking?! Well, take a look at this part of their codebase:

// Bool is a helper routine that allocates a new bool value
// to store v and returns a pointer to it.
func Bool(v bool) *bool {
    p := new(bool)
    *p = v
    return p
}

// Int is a helper routine that allocates a new int32 value
// to store v and returns a pointer to it, but unlike Int32
// its argument value is an int.
func Int(v int) *int {
    p := new(int)
    *p = v
    return p
}

// String is a helper routine that allocates a new string value
// to store v and returns a pointer to it.
func String(v string) *string {
    p := new(string)
    *p = v
    return p
}

Yep, it's really that bad. That's how ugly the API gets when we resort to pointers. Again, from the go-github discussion linked above, you can see they were quite anguished about this choice, and rightly so. I do not know for a fact that there's a better way of handling this issue (and bear in mind, they also use pointers for primitive types, which we would need to do also, if we're really going to address this issue fully), but I really like to believe (hope springs eternal!) that there's a better way of doing this.

Aside: just to complicate matters further, if a parameter is required, there's no need for it to be a pointer, so one could end up with an API that takes, say, myRequiredParam string, but also myNotRequiredParam *string.

Aside 2: For a lot of APIs, in practical terms it doesn't really matter whether or not the empty value is sent or not, the API usually handles it correctly. So we would be forcing this pointer nastiness on all users, just so we can handle what is kinda an edge case in practice.

Also, since this would be a major breaking change, and it would be nice to only force one breaking change on the users, I think we should roll into this clusterfark the design decision about how to support polymorphism. I haven't given it a huge amount of thought - and I'm still learning this Go thang - but my suspicion is that the only way to handle polymorphism is via returning interface types. Which might provide a mechanism for avoiding this pointer ugliness. I'm not sure. Deep thinking required!

[treeder]

Interesting read @neilotoole , thanks for sharing. It sounds like there is no other option really, you have to use pointers. I suppose the title of this issue should just be "Default to pointers for all types".

Aside 1: If it's required, that's true, doesn't need to be a pointer, but the more I think about it, the more you might as well just go all pointers, in case you change the api at some point. Also makes it more consistent. That Stripe API looks like a mess because they are using pointers on some things and not others.

Aside 2: I don't think this is entirely true. The guy in that thread gave a good example with bool values, you could disable and fark up a lot of stuff. And the API has no possible way of knowing what it should do.

I think we can sort of blame this on the Go language itself, kind of unfortunate really. Somebody ought to make a library that just has those helper functions that any client lib can use.

[neilotoole]

@treeder I suspect there's another option that the now infamous Issue #19 didn't consider. Don't pull the trigger on going all pointers just yet. I'll try to get a generator available this week for you guys to look at, but I believe it'll solve this issue elegantly.

[ojongerius]

@neilotoole how did you go with your test? What is the point that issue #19 did not consider?

[antihax]

Is there a spec .json that was generating circular references I could play with to see if the new client in #4440 works correctly? This may be resolved.

[wy-z]

https://play.golang.org/p/NtQrL2RPnw

package main

import (
	"encoding/json"
	"fmt"
	"time"
)

type FooMap map[string]interface{}

func (m FooMap) TypeFoo() {}

type Foo struct {
	Name   string    `json:"name"`
	Age    int       `json:"age"`
	Foo    *Foo      `json:"foo"`
	Addrs  []string  `json:"addrs"`
	Create time.Time `json:"create"`
}

func (foo Foo) TypeFoo() {}

func (foo Foo) Map() (m FooMap, err error) {
	m = make(FooMap)
	bytes, err := json.Marshal(foo)
	if err != nil {
		return
	}
	err = json.Unmarshal(bytes, &m)
	if err != nil {
		return
	}
	return
}

type FooInterface interface {
	TypeFoo()
}

func CreateFoo(foo FooInterface) (err error) {
	bytes, err := json.Marshal(foo)
	if err != nil {
		return
	}
	fmt.Println(string(bytes))
	return
}

func main() {
	// use `Foo` in most cases
	foo := Foo{
		Name:   "name",
		Age:    12,
		Create: time.Now(),
	}
	CreateFoo(foo)

	// convert `Foo` into `FooMap` and customize it if necessary
	fooMap, _ := foo.Map()
	delete(fooMap, "name")
	CreateFoo(fooMap)

	// use `FooMap` for full customization
	fooMap = FooMap{
		"name":   "new-name",
		"age":    15,
		"create": time.Now(),
	}
	CreateFoo(fooMap)
}

How about this? @wing328 @treeder @neilotoole

[wy-z]

Here is a test copy 🎁 @antihax
swagger.json.zip

[wing328]

@wy-z thanks. You can consider using https://gist.github.com to share the swagger spec.

[wing328]

The circular reference issue should be fixed by #5478 (by @wy-z) if I'm not mistaken.

[zhaomoloco]

Wonder if this discussion is concluded? If I could, I would vote for pointers for all types to differentiate between setting a field to zero value vs. merely omitted to indicate it's not touched.