Swagger-codegen: [Golang] Optional parameters are not optional.

[antihax]

Description

Currently optional parameters generate code with no option to exclude them, making the parameters required.

Swagger-codegen version

master HEAD

Suggestions

1. Use interface{} for optional parameters, except string, with run-time type checking.
2. Use pointers (dangerous due to race conditions: i.e. go func in for loop).
3. Use a map to pass parameters (similar to the Perl version).

I am leaning towards implementing 1 unless there are other suggestions?

[wing328]

@antihax thanks for the feedback. We did have a discussion on that before. From what I recall, it's a bit tricky to implement optional parameters in Go as it does not support optional parameter natively.

Here is the approach taken by Github GO API client (provided by Google):
https://github.com/google/go-github/blob/master/github/activity_star.go#L49-L61
which defines the optional parameters in a struct.

I'm not sure which approach is the best. cc'ing the experts:

cc @guohuang @neilotoole

[antihax]

@wing328 That is a very interesting API. It is using another library (https://github.com/google/go-querystring) to handle the url encoding which states that the "optional parameters" are determined by the following:

The empty values are false, 0, any nil pointer or interface value, any array slice, map, or string of length zero, and any time.Time that returns true for IsZero().

Would these cases be true for Swagger 2.0 optionals or would applying the same logic break from spec? Assuming an integer of 0 is empty sounds a bit scary.

[wing328]

You're right in saying that the Swagger spec does not use the same logic.

Here is another discussion related to Go optional parameters: http://stackoverflow.com/a/13603885/677735

[antihax]

This unfortunately is the same as the google method. Go is like C, an int cannot be nil or null; so in their example:

type Params struct {
  a, b, c int
}
p := &Params{a: 1, c: 9}

Results in a structure of: { a: 1, b: 0, c: 9 }

Thus I am still leaning towards the best option being the use of interface{} which would allow any type and then perform type checking at runtime. The pro is that the API would remain the same, the cons are that developers can trip over a runtime error due to passing the wrong argument type, which would normally be caught at compile time.

Something like this is what I am thinking: https://play.golang.org/p/3Kjab4mUQn

[antihax]

See PR #4306

[NfNitLoop]

What about accepting pointers for optional values? That way, you can pass a nil, or a pointer to a value, to specify whether that value should be specified in the request. Ex:

~
func foo(optional *int) {
if optional == nil {
fmt.Println("No value specified.")
} else {
fmt.Println("Got value: ", *optional)
}
}
~

This has the advantage that you don't throw away compile-time type checking. And you keep the types in the Go code, if you're reading that to figure out what type you need to pass in to a generated function.

[antihax]

You lose the ability to pass anonymous variables in the function parameters, which is very inconvenient.

func takesAString(s *string) {
    fmt.Printf("%s\n", *s)
}

takesAString("some other string")

results in cannot use "some other string" (type string) as type *string in argument to takesAString

Also easily introduce race conditions. https://play.golang.org/p/43YfkHb_tO

[NfNitLoop]

That's a fair criticism. But it's also easy to work around.

~
value := "some other string"
takesAString(&value);
~

A little less convenient, but worth it for the compile-time type checking, IMO. (I don't do a lot of development in Go, though, so maybe doing runtime reflect-y stuff is more idiomatic than I imagine?)

[antihax]

I agree it is also easy to work around.

My reasoning is that with interface{}

1. The function can be called as is, with no changes to arguments or variables.
2. Races are not introduced as arguments are passed by value.
3. An incorrect type should be caught fast during development or through testing; just inconveniently.
4. The only downside I see, is the loss of compile time type checking and visibility of type during development.

I do see some use of checking type with reflect, usually for structs and functions.

I am quite new to Go, so there may be things I am overlooking however.

[wing328]

@antihax @NfNitLoop A PR (https://github.com/swagger-api/swagger-codegen/issues/4308) has been submitted by @wy-z to support optional parameters. Please kindly review.

[antihax]

Looking at the PR. It would be using params map[string]interface{}.

I could go either way, but #4269 would still offer compile time checking on required parameters where #4308 would not.
Perhaps the two methods could be merged?
Typed params for required and map[string]interface{} for optional to keep the call parameters cleaner?

Unfortunately I forked off the go template and we made some major changes to functionality in order to make a client for an API quickly; so we can move on with development.

[NfNitLoop]

I like @antihax's suggestion.

1) You can keep compile-time type checking for required parameters.
2) By moving optional parameters into a map, there could be potentially lots of optional parameters defined for a method, but it won't make the method call signature explode, and if you only specify a subset of them, it reduces the burden for calling it.

[wy-z]

typed params for required and map[string]interface{} for optional
I have tried this solution, but it is unsatisfactory...golang function does not accept key-value params and the order of api params is logic-less, so that it is poignant to call a function with many structureless params
@antihax

[wy-z]

A:

GetFoo("host description", 6, "host_name", "127.0.0.1")

B:

GetFoo(map[string]interface{}{
        "name": "host_name",
        "host_id": 6,
        "description": "host description",
        "gateway_ip": "127.0.0.1",
})

In my opinion, B is more comfort and humanization.

[antihax]

There may be a way to merge the two, I would have to take a deeper look later.

This is what i was thinking. https://play.golang.org/p/cx5eTTKCcr

It would need good documentation in the comments above the function so developers know what the optionals are.

[wing328]

This is what i was thinking. https://play.golang.org/p/cx5eTTKCcr

This looks good to me

I also agree about the need for good documentation and we'll need to update the code samples in the auto-generated documentation to show how the function can be called with optional parameters.

[wy-z]

It is ok, but there may be a small problem...

API: POST /foo1s/{foo1_id}/foo2s/{foo2_id}/foo3s/{foo3_id}

You may get

XXFoo(foo3ID int64, foo1ID int64, foo2ID int64)

@antihax

[wing328]

XXFoo(foo3ID int64, foo1ID int64, foo2ID int64)

Sorry I probably missed it. What's the issue?

Also @antihax suggestion requires less change by the developers (If the spec does not contain any optional parameters, the developers using the upgraded SDK do not need to make any change at all)

[wy-z]

Also @antihax suggestion requires less change by the developers (If the spec does not contain any optional parameters, the developers using the upgraded SDK do not need to make any change at all)

This is fine~

[wy-z]

XXFoo(foo3ID int64, foo1ID int64, foo2ID int64)

Ideally, it should be XXFoo(foo1ID int64, foo2ID int64, foo3ID int64)

[wing328]

@wy-z I see but the order of the parameter can be controlled/set by the spec owner, right?

Also is sorting the parameters alphabetically based on name a best practice in Golang?

I also want to point out that other languages such as Ruby, Python also uses the approach suggested by @antihax with required parameter first and then hash/map/interface for optional parameters so Go developers who have used Ruby, Python SDK generated by Swagger Codegen may find the Go function call more "natural". (this is not to say Go developers need to follow Ruby/Python, etc coding style but a more consistent developer experience across SDKs in different languages may make the developer's life easier.)

[wy-z]

@wy-z I see but the order of the parameter can be controlled/set by the spec owner, right? +1

[wy-z]

@wing328
Thanks for your reminder~

[wy-z]

You are right, look forward to your implementation~ @antihax

[antihax]

See #4415

[wing328]

@antihax @wy-z @NfNitLoop PR by @antihax has been merged into 2.3.0. Please pull the latest to give it a try.

Kudos to contributions by @antihax