swagger-codegen-generators
swagger-codegen-generators copied to clipboard
swagger-api
Can we have a discussion about the Go code generator?
So I saw that 3.0.0 will use this repository for codegen generators. Version 2.3.x introduced some major breaking changes to the produced code. While most of these changes made sense, some of them are - in my opinion - bad choices.
I would like to use the chance that releasing 3.0.0 is, to start a discussion on providing a better code generator for Golang.
I'd like to start with some things that I think are really well done in the new 2.3.x release:
- It's now possible to provide a custom
http.Client - It's now possible provide a
context.Contextin API calls
I think there are also some issues.
- Redirects do apparently no longer work, because all status codes higher than 299 result in an error:
if localVarHttpResponse.StatusCode >= 300 {
bodyBytes, _ := ioutil.ReadAll(localVarHttpResponse.Body)
return localVarHttpResponse, reportError("Status: %v, Body: %s", localVarHttpResponse.Status, bodyBytes)
}
- Adding request bodies now requires passing a
map[string]interface{}that needs, for example, abodykey.
The second limitation is especially painful from an API point of view as type assertions are necessary to decode the payload, and the API is not very developer friendly in the context of Go. In real-world use it looks something like this:
CreateResource(context.Context, map[string]interface{}{"body": &SomeStruct{}})
I propse that instead, a type is used to solve this, for example:
type CreateResourceParameters struct {
Body *SomeStruct
Path ...
Query ...
}
This would make the process of passing down request parameters much more usable and less error prone.
Let me know what you think.
One point I forgot is error payloads. In the spec, it's possbile to provide an error payload for e.g. 401 or 403. With the current swagger-codegen template for Go, the response body is actually not readable from outside as the body is closed within the generated code.
What I suggest is to have custom errors which implement the error.Error interface instead. These error structs are hydrated depending on the status code. The developer may use type assertion to get the full error message, including the original body.
Redirects do apparently no longer work, because all status codes higher than 299 result in an error:
This should be fixable.
Adding request bodies now requires passing a map[string]interface{} that needs, for example, a body key.
I agree this was a bad design choice, but there are limited options available to handle optional parameters.
What I suggest is to have custom errors which implement the error.Error interface instead. These error structs are hydrated depending on the status code.
I know the error models are generated, so just a matter of populating it. This may confuse some folks getting an interface{} back instead of error. Perhaps we extend the response instead with the struct?
I know the error models are generated, so just a matter of populating it. This may confuse some folks getting an interface{} back instead of error. Perhaps we extend the response instead with the struct?
You can type assert errors, no need for an interface.
I agree this was a bad design choice, but there are limited options available to handle optional parameters.
Optional parameters are solvable with structs, as no field in the struct is required!
If something isn't clear I'm happy to explain a bit more.
Optional parameters are solvable with structs, as no field in the struct is required!
An integer will default to 0, a boolean will default to false, both could be legitimate values. How do you determine if they are optional or not?
An integer will default to 0, a boolean will default to false both could be legitimate values. How do you determine if they are optional or not?
I see - where is this currently an issue though? As far as I understood it (I played around with the new code for ~ hour), we're still using structs for e.g. body payloads. There we can't define empty parts either, right? One possibility is, of course, to use pointers. Using pointers for simple values however is not that nice.
Indeed, pointers would require you to copy to a new variable before calling as a goroutine or you will race on the variable.
Unfortunately the API i am mostly working with is littered with such things.
In an update request this is optional. If you supply true or false, it will update the entry. So if it was true and Go defaults it to false, it will update it when you do not wish it to.
{
"name": "watched",
"in": "query",
"description": "Whether the new contact should be watched, note this is only effective on characters",
"required": false,
"type": "boolean",
},
Ok I see. I think there are multiple things we could consider here:
- Body payloads currently use JSON marshalling from structs. This implies that your use case would actually not work if that was in the body, right?
- Query parameters could be solved using
url.Values{}which is the stdlib variant for defining query parameters. I understand that this will not help us in replacing themap[string]interface{}with a struct, but it will at least use the stdlib which people are used to in this context!
The third part is, I think, the path. I'm not sure if all parameters in the path are always required or not. Currently, the path uses (if I understand it correctly) the method's arguments. So here, in the current version of the API, this wouldn't work either, right?
In conclusion I think that we could probably leverage url.Values{} to use the "standard way" for defining queries, and body structs for the request body. However, it won't solve this issue for your use case, if the payload is in the body and not in the query.
It is definitely worth exploring. Could you not convert url.Values{} to JSON/XML for the body?
It is definitely worth exploring. Could you not convert url.Values{} to JSON/XML for the body?
No, url.Values only supports strings: map[string][]string{}
The AWS SDK seems to be using helper functions to achieve potential nil values for primitives:
result, err := svc.GetObjectWithContext(ctx, &s3.GetObjectInput{
Bucket: aws.String("my-bucket"),
Key: aws.String("my-key"),
})
That looks like the way to go. We would get compile time type checking back.
type StringType struct{
val string
set bool
}
func String(v string) StringType {
return StringType{v, true}
}
func (s StringType) IsSet() bool {
return s.set
}
func (s StringType) Value() string {
return s.String()
}
func (s StringType) String() string {
if !s.set {
return ""
}
return s.val
}
So the question is, when a value is not required, if it should be nullable per default or not (as a Go developer I would say no, it's not nullable per default). Is there a way to express nullability in the swagger spec?
The segment Go SDK uses composable types:
client.Enqueue(analytics.Identify{
UserId: "019mr8mf4r",
Traits: analytics.NewTraits().
SetName("Michael Bolton").
SetEmail("[email protected]").
Set("plan", "Enterprise").
Set("friends", 42),
})
I don't really like this approach because doesn't allow marshal/unmarshal.
You can supply defaults:
{
"name": "label_id",
"in": "query",
"description": "Add a custom label to the new contact",
"required": false,
"type": "integer",
"format": "int64",
"default": 0
},
But from my understanding there is no way to express null in 2.0 or lower. Not sure about the newer 3.0.
Marshall seems to work quite well with nullable values, check this:
package main
import (
"encoding/json"
"fmt"
)
type Foo struct {
Foo *string `json:"foo"`
}
func main() {
out, err := json.Marshal(&Foo{})
if err != nil {
panic(err)
}
fmt.Printf("Hello, playground %s", out)
}
Hello, playground {"foo":null}
Program exited.
Ok, so personally I would be ok with the AWS way. I'll compose a few more examples from APIs I use, so we have a comprehensive overview of the different approaches.
Another idea would be to have something like this:
type MyRequestPayload struct {
Body SomeType
Query SomeType1
Path SomeType2
BodyCustom map[string]interface{}
QueryCustom map[string]interface{}
PathCustom map[string]interface{}
}
OAS2 indeed doesn't support null values directly.
OAS3 - does, using the nullable property.
This could actually be improved:
type MyRequestPayload struct {
*MyRequestPayloadNotNullable
*MyRequestPayloadNullable
}
type MyRequestPayloadNotNullable struct {
Body SomeType
Query SomeType1
Path SomeType2
}
type MyRequestPayloadNullable struct {
BodyCustom map[string]interface{}
QueryCustom map[string]interface{}
PathCustom map[string]interface{}
}
var payload MyRequestPayload = NewMyRequestPayload()
// or if it should be nullable
var payload MyRequestPayload = NewMyRequestPayloadNullable()
client.MyRequest(context, payload)
Ok that's quite unfortunate, because primitive null values are simply not standard for Go (see the sql.StringNull confusion/fiasko) :(
I checked out the google appengine SDK, but it doesn't seem to have nullable values.
It seems like the Google Cloud SDK supports nullable values, it has also custom types: https://godoc.org/cloud.google.com/go/bigquery#NullInt64
type NullInt64 struct {
Int64 int64
Valid bool // Valid is true if Int64 is not NULL.
}
func (n NullInt64) String() string { return nullstr(n.Valid, n.Int64) }
So if even Google does this in Go, it does seem like it's a good idea.
@arekkas - issue #7809 has been logged along the same lines as the first issue you discuss. Any comments on the proposed PR?
Thank you for pointing me to it, I commented and discourage merging it as it will introduce serious issues to anyone not paying attention.
My proposal for improving the API is as followed.
type NilString *string
// func (s NilString) String() string ...
// type NilFloat64 *float64
// ...
type SomeRequestBody struct {
Param1 NilString `json:"param1,omitempty"`
Param2 string `json:"param2"`
// ...
}
type SomeRequestPath struct {
Param3 NilString `json:"param3,omitempty"`
Param4 string `json:"param4"`
// ...
}
type SomeRequestQuery struct {
Param5 NilString `json:"param5,omitempty"`
Param6 string `json:"param6"`
// ...
}
type SomeRequest struct {
Query *SomeRequestQuery
Path *SomeRequestPath
Body *SomeRequestBody
}
// ...
request := NewSomeRequest().WithBody(body).WithPath(path).WithQuery(query)
response, err := client.SomeApiRequest(request)
The response is a composite type
type SomeResponse struct {
Response *http.Response
Body []byte
Payload *SomePayload // this is the default payload
404Payload *404Payload
400Payload *400Payload
// ... whatever other payload is defined
}
That way we have access to the original response (think headers), to the response body and can also return different structs for specific status codes. I'm not too sure currently on the different response types depending on the status code as it might add some complexity in deciding which payload is the default one and could potentially bloat the responses, depending on the swagger definition.
By the way, I can definitely help with the Go code (review, help with templating), but I'm not experienced with Java developer. As a reference, I write a ton of Go and maintain popular libraries (1 2 3 4). Not intended as a brag but as confirmation that I am experienced with writing/maintaining Go libraries :)
@arekkas many thanks for the comments on the proposed PR. I have to say I had some concerns about the PR being a breaking change and also the body leak. Let's try to work out a sensible solution to the problem.
I like the idea of unmarshaling the error into a model and returning it as an abstract of the error type. We could take that further and return a []byte of the body as well as the model for cases such as if a CDN throws an identical status to what an operation would.
I will need to read up a bit more on how 3.0 would change parameters, it would be nice to keep required parameters in the functions parameter list and optionals as an additional struct handling isSet and isNull.
IMO the library should handle if the parameter is body, query, or path if this is possible; not the developer. It would make life simpler than having to remember where each parameter goes.
IMO the library should handle if the parameter is body, query, or path if this is possible; not the developer. It would make life simpler than having to remember where each parameter goes.
I agree 100%. This can be solved with embedded structs:
type A struct {
A string
C string
}
type B struct {
B string
C string
}
type AB struct {
A
B
}
func main() {
s := new(AB)
s.A= "123"
s.B = "123"
// does not work: s.C = "123"
s.A.C = "123"
s.B.C = "123"
}
So here, the developer would only need to look this up if the names clash!