oapi-codegen icon indicating copy to clipboard operation
oapi-codegen copied to clipboard

BindQueryParameter can't adapt to "x-go-type-skip-optional-pointer: true"

Open ysuliu opened this issue 2 years ago • 4 comments

For slice parameters for example []string, we prefer to use it as a flat array, instead of *[]string. So we can add the property x-go-type-skip-optional-pointer: true in the spec:

        - name: tags
          required: false
          in: query
          description: tags
          example: space1,high_prio,flow_A
          schema:
            type: array
            items:
              type: string
          style: form
          explode: false
          x-go-type-skip-optional-pointer: true

The struct will be generated like:

type TestParams struct {
	Tags []string `form:"tags,omitempty" json:"tags,omitempty"`
}

But with it, when we send a request including a parameter like ?tags=taga,tagb,tagc BindQueryParameter will raise error: Invalid format for parameter tags: multiple values for single value parameter 'tags'

The problem is that the BindQueryParameter is doing 2 times reflect.Indirect for the non-required parameter, no matter x-go-type-skip-optional-pointer: true exists or not.

		// For optional parameters, we have an extra indirect. An optional
		// parameter of type "int" will be *int on the struct. We pass that
		// in by pointer, and have **int.

		// If the destination, is a nil pointer, we need to allocate it.
		if v.IsNil() {
			t := v.Type()
			newValue := reflect.New(t.Elem())
			// for now, hang onto the output buffer separately from destination,
			// as we don't want to write anything to destination until we can
			// unmarshal successfully, and check whether a field is required.
			output = newValue.Interface()
		} else {
			// If the destination isn't nil, just use that.
			output = v.Interface()
		}

		// Get rid of that extra indirect as compared to the required case,
		// so the code below doesn't have to care.
		v = reflect.Indirect(reflect.ValueOf(output))

In another word, BindQueryParameter seems not adapt to x-go-type-skip-optional-pointer: true yet.

@jamietanna Could you please look at this problem? Thanks a lot!

ysuliu avatar Oct 31 '23 12:10 ysuliu

Hey, please try to avoid pinging me or the other maintainers unless it's an issue that's stalled or may have missed, or it's urgent.

Marcin and I are maintaining this for the most part in our free time, and are currently juggling a number of things, including https://github.com/deepmap/oapi-codegen/discussions/1309

If you'd like to expedite me looking at this issue, I'm on GitHub Sponsors :wink: Otherwise I'll try and get to triage it at some point soon, but can't promise anything.

jamietanna avatar Oct 31 '23 15:10 jamietanna

This behavior is also relevant for header parameters

miramir avatar Nov 05 '23 14:11 miramir

A related breakage happens with explode: true: The field is generated as []string, but bad code is generated for the client, akin to:

		if params.Foo != nil {

			if queryFrag, err := runtime.StyleParamWithLocation("form", true, "foo", runtime.ParamLocationQuery, *params.Foo); err != nil {

Which doesn't compile because you can't * dereference a slice

mgabeler-lee-6rs avatar Jan 05 '24 22:01 mgabeler-lee-6rs

I ran into this too. The generated code does not compile when using x-go-type-skip-optional-pointer with an integer query parameter. Using the echo server.

# OpenAPI 3 spec
        - name: timeout
          in: query
          description: The timeout period in seconds
          required: false
          schema:
            type: integer
            example: 60
            x-go-type-skip-optional-pointer: true
// Generated code
		if params.Timeout != nil {

			if queryFrag, err := runtime.StyleParamWithLocation("form", true, "timeout", runtime.ParamLocationQuery, *params.Timeout); err != nil {
				return nil, err
# Compiler error
invalid operation: params.Timeout != nil (mismatched types int and untyped nil)

armsnyder avatar Jan 23 '24 07:01 armsnyder