[Go][Server] Convert Optional Parameters to Pointers

[icubbon]

When a parameter is optional, make a best effort to make a parameter that wasn't provided nil versus the empty/0 value.
The concept of this change was to make all the parsing of parameters return a pointer to the value, that way a nil could be returned if the input wasn't provided.
Header Parameters and Path Parameters can still use more work as it is possible to get the underlying map of params to check if the value was provided.

Snapshot the existing Go-Server into a new generator called Go-Server-Deprecated

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh ./bin/configs/*.yaml
  ./bin/utils/export_docs_generators.sh
  

  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master (upcoming 7.1.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@wing328 @lwj5

[lwj5]

you can simply return nil

[lwj5]

It isn't a good idea to return pointers for all the parse* functions. This more likely than not results in heap allocation and slows it down considerably.

[icubbon]

A heap allocation will be required at some point for all optional query parameters. My thought process was to keep the mustache template as simple as I could keep it. I tried out not returning a pointer from here, but the changes in the controller-api.mustache became quite large.

I will argue that the heap allocation is negligible, especially for the primitive types being handled by these functions. I included two Benchmarks and their results from my local machine below. The summary is that the difference is a 1/1000th of a nanosecond.

I believe that this time penalty is not noticeable and should not be taken into account for how maintainable the code is.

// goos: linux
// goarch: amd64
// cpu: Intel(R) Core(TM) i9-9900 CPU @ 3.10GHz
// BenchmarkPointerReturn
// BenchmarkPointerReturn-16    	1000000000	         0.2164 ns/op
// PASS
func BenchmarkPointerReturn(b *testing.B) {
	for i := 0; i < b.N; i++ {
		p := pointerReturn()
		_ = p
	}
}

// goos: linux
// goarch: amd64
// cpu: Intel(R) Core(TM) i9-9900 CPU @ 3.10GHz
// BenchmarkValueReturn
// BenchmarkValueReturn-16    	1000000000	         0.2156 ns/op
// PASS
func BenchmarkValueReturn(b *testing.B) {
	for i := 0; i < b.N; i++ {
		v := valueReturn()
		_ = v
	}
}

func pointerReturn() *int {
	a := 1
	return &a
}

func valueReturn() int {
	a := 1
	return a
}```

[lwj5]

You need to print allocations.

Also, you need to disable inlining and prevent escaping. I suggest you run the benchmark on the parsers in the router.go if you want a better comparison.

[icubbon]

I just ran the benchmarks on the parsers in router.go and got similar results. The overhead of parsing the strings via strconv greatly outweighs any overhead an allocation on the heap that may occur for these primitive types.

//goos: linux
//goarch: amd64
//pkg: github.com/SpectraLogic/.../api/openapi
//cpu: Intel(R) Core(TM) i9-9900 CPU @ 3.10GHz
//BenchmarkPointerReturn
//BenchmarkPointerReturn-16    	1000000000	         0.9425 ns/op
//PASS

func BenchmarkPointerReturnBool(b *testing.B) {
	for i := 0; i < b.N; i++ {
		p, _ := parseBool("true")
		_ = p
	}
}

// goos: linux
// goarch: amd64
// pkg: github.com/SpectraLogic/.../api/openapi
// cpu: Intel(R) Core(TM) i9-9900 CPU @ 3.10GHz
// BenchmarkValReturn
// BenchmarkValReturn-16    	1000000000	         0.9367 ns/op
// PASS
func BenchmarkValReturnBool(b *testing.B) {
	for i := 0; i < b.N; i++ {
		p, _ := parseBoolVal("true")
		_ = p
	}
}

// goos: linux
// goarch: amd64
// pkg: github.com/SpectraLogic/.../api/openapi
// cpu: Intel(R) Core(TM) i9-9900 CPU @ 3.10GHz
// BenchmarkPointerReturnFloat64
// BenchmarkPointerReturnFloat64-16    	70159354	        17.99 ns/op
// PASS
func BenchmarkPointerReturnFloat64(b *testing.B) {
	for i := 0; i < b.N; i++ {
		p, _ := parseFloat64("1.0")
		_ = p
	}
}

// goos: linux
// goarch: amd64
// pkg: github.com/SpectraLogic/.../api/openapi
// cpu: Intel(R) Core(TM) i9-9900 CPU @ 3.10GHz
// BenchmarkValReturnFloat64
// BenchmarkValReturnFloat64-16    	59044398	        17.80 ns/op
// PASS
func BenchmarkValReturnFloat64(b *testing.B) {
	for i := 0; i < b.N; i++ {
		p, _ := parseFloat64Val("1.0")
		_ = p
	}
}

[lwj5]

BenchmarkNumNew-8 48563181 25.93 ns/op 4 B/op 1 allocs/op
BenchmarkBoolNew-8 100000000 11.35 ns/op 1 B/op 1 allocs/op

BenchmarkNumOld-8 71088042 16.85 ns/op 0 B/op 0 allocs/op
BenchmarkBoolOld-8 243797635 4.883 ns/op 0 B/op 0 allocs/op

1.5+x slower on Num (float32)
2+x slower on Bool

inlining turned off

package new

import (
	"testing"
)

var result float32
var result2 bool

func BenchmarkNumNew(b *testing.B) {
	b.ReportAllocs()
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		r, err := parseNumericParameter[float32](
			"5",
			WithParse[float32](parseFloat32),
			WithMinimum[float32](0),
		)
		if err != nil {
			b.Fatal(err)
		}

		result = *r
	}
}

func BenchmarkBoolNew(b *testing.B) {
	b.ReportAllocs()
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		r, err := parseBoolParameter(
			"true",
			WithParse[bool](parseBool),
		)
		if err != nil {
			b.Fatal(err)
		}

		result2 = *r
	}
}

[icubbon]

I do understand that it is slower, but it is on the order of a few nanoseconds slower per parameter.

I have tried a couple of different ways to write the controller-api.mustache file without the parsers returning pointers and all of the ways so far have been significantly less maintainable and uglier. Additionally, I haven't been able to smooth out all the wrinkles in the other attempts to get it to work even on my companies REST API Server.

[lwj5]

is this PR targeting query, path, or header?

I see changes to the path param. The last I saw in 3.0, the path param is required. https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#parameter-locations. And thus it should never be passed as a pointer

you may want to scope the PR to affect query first since you can clearly check if the query is provided by using query.Has

[icubbon]

is this PR targeting query, path, or header?

I see changes to the path param. The last I saw in 3.0, the path param is required. https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#parameter-locations. And thus it should never be passed as a pointer

you may want to scope the PR to affect query first since you can clearly check if the query is provided by using query.Has

The changes for the Path Parameters were made in a minimal fashion such that all parameters could be handled the same way at the bottom of the template:
result, err := c.service.{{nickname}}(r.Context(){{#allParams}}, {{^isArray}}{{^isFile}}{{#required}}*{{/required}}{{/isFile}}{{/isArray}}{{paramName}}Param{{/allParams}})

This kept the rest of the template much simpler instead of having to juggle what is a pointer and what is not a pointer.

[lwj5]

The only time pointers are used is when the param is optional and does not have defaults.

That means majority cases (each combination is case) are going to be non-pointers. Also. path param will never use pointers.

Why make the performance worst for majority of the cases when you can just target the minority case

[lwj5]

Like you said headers still need more work, and adding many parts can get complicated. You may want to create a PR to target optional query parameters with no defaults and convert that to a pointer first. In this case, you wouldn't need to change the Parse* functions. Small and concise PRs are appreciated.

[lwj5]

for optional query params with no default values you can change the code from

	booleanTestParam, err := parseBoolParameter(
		query.Get("boolean_test"),
		WithParse[bool](parseBool),
	)
	if err != nil {
		c.errorHandler(w, r, &ParsingError{Err: err}, nil)
		return
	}

	var booleanTestParam *bool
	if query.Has("boolean_test") {
		param, err := parseBoolParameter(
			query.Get("boolean_test"),
			WithParse[bool](parseBool),
		)
		if err != nil {
			c.errorHandler(w, r, &ParsingError{Err: err}, nil)
			return
		}
		booleanTestParam = &param
	}

That's the only change required. You can revert all the parse* functions in router.go

[lwj5]

so for every type there are 2 cases, where optional non-default is handled separately

	{{#isBoolean}}
// some condition must be here
	{{paramName}}Param, err := parseBoolParameter(
		query.Get("{{baseName}}"),{{#defaultValue}}
		WithDefaultOrParse[bool]({{defaultValue}}, parseBool),{{/defaultValue}}{{^defaultValue}}{{#required}}
		WithRequire[bool](parseBool),{{/required}}{{/defaultValue}}
	)
	if err != nil {
		c.errorHandler(w, r, &ParsingError{Err: err}, nil)
		return
	}
// end of some condition must be here
	{{^defaultValue}}{{^required}}
	var {{baseName}} *bool
	if query.Has("{{baseName}}") {
		param, err := parseBoolParameter(
			query.Get("{{baseName}}"),
			WithParse[bool](parseBool),
		)
		if err != nil {
			c.errorHandler(w, r, &ParsingError{Err: err}, nil)
			return
		}
		booleanTestParam = &param
	}
	{{/required}}{{/defaultValue}}
	{{/isBoolean}}

[icubbon]

so for every type there are 2 cases, where optional non-default is handled separately

	{{#isBoolean}}
// some condition must be here
	{{paramName}}Param, err := parseBoolParameter(
		query.Get("{{baseName}}"),{{#defaultValue}}
		WithDefaultOrParse[bool]({{defaultValue}}, parseBool),{{/defaultValue}}{{^defaultValue}}{{#required}}
		WithRequire[bool](parseBool),{{/required}}{{/defaultValue}}
	)
	if err != nil {
		c.errorHandler(w, r, &ParsingError{Err: err}, nil)
		return
	}
// end of some condition must be here
	{{^defaultValue}}{{^required}}
	var {{baseName}} *bool
	if query.Has("{{baseName}}") {
		param, err := parseBoolParameter(
			query.Get("{{baseName}}"),
			WithParse[bool](parseBool),
		)
		if err != nil {
			c.errorHandler(w, r, &ParsingError{Err: err}, nil)
			return
		}
		booleanTestParam = &param
	}
	{{/required}}{{/defaultValue}}
	{{/isBoolean}}

This one gets tricky for time values as the time import would be needed in the controller-api.mustache file but the Java code doesn't have a mechanism that I know of for this adding imports dynamically.
e.g. without some more work else where, we cannot have a time.Time conditionally referenced in controller-api.mustache.
I can add another helper in routers.mustache to get the time or nil as a workaround if we want to go this route of handling each type in two different cases.

[icubbon]

so for every type there are 2 cases, where optional non-default is handled separately

	{{#isBoolean}}
// some condition must be here
	{{paramName}}Param, err := parseBoolParameter(
		query.Get("{{baseName}}"),{{#defaultValue}}
		WithDefaultOrParse[bool]({{defaultValue}}, parseBool),{{/defaultValue}}{{^defaultValue}}{{#required}}
		WithRequire[bool](parseBool),{{/required}}{{/defaultValue}}
	)
	if err != nil {
		c.errorHandler(w, r, &ParsingError{Err: err}, nil)
		return
	}
// end of some condition must be here
	{{^defaultValue}}{{^required}}
	var {{baseName}} *bool
	if query.Has("{{baseName}}") {
		param, err := parseBoolParameter(
			query.Get("{{baseName}}"),
			WithParse[bool](parseBool),
		)
		if err != nil {
			c.errorHandler(w, r, &ParsingError{Err: err}, nil)
			return
		}
		booleanTestParam = &param
	}
	{{/required}}{{/defaultValue}}
	{{/isBoolean}}

This one gets tricky for time values as the time import would be needed in the controller-api.mustache file but the Java code doesn't have a mechanism that I know of for this adding imports dynamically. e.g. without some more work else where, we cannot have a time.Time conditionally referenced in controller-api.mustache. I can add another helper in routers.mustache to get the time or nil as a workaround if we want to go this route of handling each type in two different cases.

Also, that // some condition must be here is doing some heavy lifting as it's an OR condition which isn't the cleanest to implement in Mustache syntax without duplicating the code and typically requires adding a new JSON field that is the result of the OR that would be requiredOrHasDefaults.

[icubbon]

Without adding a new JSON field, each type would now have 3 cases for Query, Header, and FormParams. I know right now this would only be targeting Query params, but the other two also need to be addressed soon. Take a look at the below for what this looks like for the float32 case in the Query:

{{#isNumber}}
	{{#required}}{{#defaultValue}}
	{{paramName}}Param, err := parseNumericParameter[float32](
		query.Get("{{baseName}}"),{{#defaultValue}}
		WithDefaultOrParse[float32]({{defaultValue}}, parseFloat32),{{/defaultValue}}{{^defaultValue}}{{#required}}
		WithRequire[float32](parseFloat32),{{/required}}{{/defaultValue}}{{^defaultValue}}{{^required}}
		WithParse[float32](parseFloat32),{{/required}}{{/defaultValue}}{{#minimum}}
		WithMinimum[float32]({{minimum}}),{{/minimum}}{{#maximum}}
		WithMaximum[float32]({{maximum}}),{{/maximum}}
	)
	if err != nil {
		c.errorHandler(w, r, &ParsingError{Err: err}, nil)
		return
	}
	{{/defaultValue}}{{/required}}
	{{^required}}{{#defaultValue}}
	{{paramName}}Param, err := parseNumericParameter[float32](
		query.Get("{{baseName}}"),{{#defaultValue}}
		WithDefaultOrParse[float32]({{defaultValue}}, parseFloat32),{{/defaultValue}}{{^defaultValue}}{{#required}}
		WithRequire[float32](parseFloat32),{{/required}}{{/defaultValue}}{{^defaultValue}}{{^required}}
		WithParse[float32](parseFloat32),{{/required}}{{/defaultValue}}{{#minimum}}
		WithMinimum[float32]({{minimum}}),{{/minimum}}{{#maximum}}
		WithMaximum[float32]({{maximum}}),{{/maximum}}
	)
	if err != nil {
		c.errorHandler(w, r, &ParsingError{Err: err}, nil)
		return
	}
	{{/defaultValue}}{{/required}}
	{{^required}}{{^defaultValue}}
	var {{paramName}}Param *float32
	if query.Has("{{baseName}}" && query.Get("{{baseName}}") != "" {
		value, err := parseNumericParameter[float32](
			query.Get("{{baseName}}"),{{#defaultValue}}
			WithDefaultOrParse[float32]({{defaultValue}}, parseFloat32),{{/defaultValue}}{{^defaultValue}}{{#required}}
			WithRequire[float32](parseFloat32),{{/required}}{{/defaultValue}}{{^defaultValue}}{{^required}}
			WithParse[float32](parseFloat32),{{/required}}{{/defaultValue}}{{#minimum}}
			WithMinimum[float32]({{minimum}}),{{/minimum}}{{#maximum}}
			WithMaximum[float32]({{maximum}}),{{/maximum}}
		)
		if err != nil {
			c.errorHandler(w, r, &ParsingError{Err: err}, nil)
			return
		}
		{{paramName}}Param = &value
	} 
	{{/defaultValue}}{{/required}}
	{{/isNumber}}

This ends up being a lot of extra branching logic to not return pointers from the parsing functions. I really don't want to add 3x the code and branching into controller-api.mustache as it's already a complex file.

The other option I explored was using a NullableType to be returned from the Parser functions, but that change was much larger than what is currently in this Pull Request.

Another option I went with for awhile until I decided it just wasn't readable or maintainable enough as it is doing some harder to follow templating when compared to making the parsers return a pointer.

	{{#isFloat}}
	{{^required}}{{^defaultValue}}
	var {{paramName}}Param *float32
	if query.Has("{{baseName}}"){
	{{/defaultValue}}{{/required}}
	{{paramName}}Param{{^required}}{{^defaultValue}}Val{{/defaultValue}}{{/required}}, err := parseNumericParameter[float32](
		query.Get("{{baseName}}"),{{#defaultValue}}
		WithDefaultOrParse[float32]({{defaultValue}}, parseFloat32),{{/defaultValue}}{{^defaultValue}}{{#required}}
		WithRequire[float32](parseFloat32),{{/required}}{{/defaultValue}}{{^defaultValue}}{{^required}}
		WithParse[float32](parseFloat32),{{/required}}{{/defaultValue}}{{#minimum}}
		WithMinimum[float32]({{minimum}}),{{/minimum}}{{#maximum}}
		WithMaximum[float32]({{maximum}}),{{/maximum}}
	)
	if err != nil {
		c.errorHandler(w, r, &ParsingError{Err: err}, nil)
		return
	}
	{{^required}}{{^defaultValue}}
		{{paramName}}Param = &{{paramName}}ParamVal
	}
	{{/defaultValue}}{{/required}}
	{{/isFloat}}

I scrapped the above options before fully implementing them so I did not run the OpenAPI tests or my company's tests against them.

[lwj5]

Please see https://github.com/OpenAPITools/openapi-generator/pull/17051/files

Any review points are appreciated

[furiel]

@icubbon @lwj5 Thanks for working on this issue. I believe I face a similar problem in terms of root cause, but different manifestation.

I believe the problem is not really with the parsing logic, but a bit more fundamental: the generator generates invalid type for optional non-nullable parameters. I wanted to ask @icubbon if you happen to had explored this alternative.

Being a bit more specific:

example:

openapi: 3.0.0
info:
  version: 0.1.6
  title: example

paths:
  /example:
    get:
      responses:
        '200':
          description: example

components:
  schemas:
    Example:
      type: object
      properties:
        num:           # <--
          type: number

would incorrectly generate

type Example struct {
	Num float64 `json:"num,omitempty"`
}

instead of

type Example struct {
	Num *float64 `json:"num,omitempty"`
}

This causes problems both for request parameters and also for responses.

In case of request parameter, one cannot distinguish if num parameter is omitted, or it is sent with zero value, because both cases would be deserialized into Example{Num: 0.0}. If I understood your description @icubbon correctly, this is what you are facing.

In case of response, similar problem exists. There is no way to serialize zero value fo num. As soon as I set zero to the field, omitempty will remove it. omitempty check the default value of float64 instead of *float64 when it decides if the field needs to be included or removed during serialization: checking 0.0 instead of nil.

What do you think?

[lwj5]

Hi @furiel, just set the field as nullable and it will be a pointer

[furiel]

@lwj5 I see, but doesn't that mean a different protocol then? My understanding is optionality and nullability are orthogonal concepts.

Optionality means keys can be omitted. It does not set restrictions to the values.
Nullable means besides the original set of acceptable values nil can be sent too. It does not set restriction for the keys.

In use cases where nil is not an interpretable value for a domain, and users would prefer not sending nil for signalling optionality: optional + non-nullable is the intended combination, right?

[lwj5]

Yes, optional and nullable are orthogonal.

You are right, if you require ability to omit values, setting optional is the correct call. But setting this does not mean the that the value is able to take on a null or in the case of Go, nil value. It is supposed to make the value untouched.

I think openapi specs 3.1 does make it clearer. The nullable key is removed and instead you have to explicitly state the types, a null + a concrete type (e.g., null + string).

Yes and your last part paragraph is absolutely right.

[lwj5]

I do not have time this month to clear up the PR so probably only in Dec I can clean it up

[icubbon]

@furiel Great question! Yes, I've had to tackle this same exact problem for Models.
The "solution" that we went with at my company is to use the Models generated from the Go-Client and use the rest of the Go-Server code. The Go-Client models have much better support for anyOf, oneOf, and allOf type models whereas the Go-Server leaves a bit to be desired.
The Go-Client Models do correctly handle non-required parameters.

Here's an example of this where Partition is marked as "not required" and shows up as a pointer to a string in the generated Model.

// MoveRequestMedia An incoming request to perform a Media move. Supported moves are slot-to-slot, slot-to-drive, and drive-to-slot. It is **not** marked as `nullable` in the YAML.
type MoveRequestMedia struct {
	// Only required on multi-partition libraries.
	Partition *string `json:"partition,omitempty"`
	// MediaContainer Address from `/inventory`
	SourceAddress int32 `json:"sourceAddress"`
	// MediaContainer Address from `/inventory`
	DestAddress int32 `json:"destAddress"`
}

MoveRequestMedia:
allOf:
  - type: object
    required:
      - sourceAddress
      - destAddress
    properties:
      partition:
          type: string
      sourceAddress:
        type: integer
        format: int32
        minimum: 1
        maximum: 65535
      destAddress:
        type: integer
        format: int32
        minimum: 1
        maximum: 65535

I would like to eventually port the Go-Client Models into the Go-Server since they are more functional in my opinion.

[furiel]

Thank you both for your insights.