Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server cannot unmarshal array of polymorphic values #1637

Closed
ntoljic opened this issue Jul 26, 2018 · 11 comments
Closed

Server cannot unmarshal array of polymorphic values #1637

ntoljic opened this issue Jul 26, 2018 · 11 comments
Assignees
Labels
bug discriminator Related to polymorphic types model Related to swagger generate model command pending PR

Comments

@ntoljic
Copy link

ntoljic commented Jul 26, 2018

Problem statement

Not sure if this is related to #842, but when an endpoint expects an array of polymorphic values, the server fails with the following error message:

{"code":400,"message":"parsing contents body from \"\" failed, because json: cannot unmarshal object into Go value of type models.Value"}

Swagger specification

swagger: '2.0'
info:
  version: '0.0.1'
  title: Test
schemes:
  - http
basePath: /v1
produces:
  - application/json
consumes:
  - application/json
paths:
  /test:
    put:
      operationId: test
      parameters:
      - name: payload
        in: body
        required: true
        schema:
          type: array
          items:
            $ref: '#/definitions/Value'
      tags:
        - Test
      responses:
        default:
          description: "successful operation"
definitions:
  Value:
    type: object
    discriminator: ValueType
    required:
    - ValueType
    properties:
      ValueType:
        type: string
  StringValue:
    allOf:
    - $ref: '#/definitions/Value'
    - type: object
      properties:
        foobar:
          type: string

Steps to reproduce

  • Implement a dummy handler
  • Build and start the server
  • execute the following curl:
curl 'http://localhost:PORT/v1/test -X PUT -H 'Content-type: application/json' -d '[{"ValueType":"StringValue","Foobar":"hello"}]'

Environment

swagger version: 0.12.0
go version: go1.9.1 darwin/amd64
OS: MacOS but should be irrelevant

Workaround

When payload is to be an object that contains the array instead of just the array, it works.

- name: payload
    in: body
    required: true
    schema:
      type: object
      properties:
        values:
          type: array
          items:
            $ref: '#/definitions/Value'

Since the issue can be worked around it is not a blocking issue for me, I am submitting this issue more out of curiosity as to what provokes this behavior.

Edit: fix invalid sample spec

@gregmarr
Copy link
Contributor

You're using a really old version. Please try with the latest release, this should be fixed.

@ntoljic
Copy link
Author

ntoljic commented Jul 26, 2018

Just upgraded to 0.15.0.

Same result :/

@gregmarr
Copy link
Contributor

I guess it's different because of this: When payload is to be an object that contains the array instead of just the array, it works.

@fredbi fredbi added bug model Related to swagger generate model command discriminator Related to polymorphic types labels Jul 26, 2018
@fredbi
Copy link
Contributor

fredbi commented Jul 26, 2018

Could you please add here the BindRequest() method generated for your operation?

@ntoljic
Copy link
Author

ntoljic commented Jul 26, 2018

Sure thing. Here you go:

// BindRequest both binds and validates a request, it assumes that complex things implement a Validatable(strfmt.Registry) error interface
// for simple values it will use straight method calls.
//
// To ensure default values, the struct must have been initialized with NewTestParams() beforehand.
func (o *TestParams) BindRequest(r *http.Request, route *middleware.MatchedRoute) error {
	var res []error

	o.HTTPRequest = r

	if runtime.HasBody(r) {
		defer r.Body.Close()
		var body []models.Value
		if err := route.Consumer.Consume(r.Body, &body); err != nil {
			if err == io.EOF {
				res = append(res, errors.Required("payload", "body"))
			} else {
				res = append(res, errors.NewParseError("payload", "body", "", err))
			}
		} else {
			// validate array of body objects
			for i := range body {
				if err := body[i].Validate(route.Formats); err != nil {
					res = append(res, err)
					break
				}
			}

			if len(res) == 0 {
				o.Payload = body
			}
		}
	} else {
		res = append(res, errors.Required("payload", "body"))
	}
	if len(res) > 0 {
		return errors.CompositeValidationError(res...)
	}
	return nil
}

@fredbi
Copy link
Contributor

fredbi commented Jul 26, 2018

Yes I've seen already this when trying to fix some other stuff with polymorphism.
Problem is that the array type does not get its "IsExported" flag set.

@fredbi
Copy link
Contributor

fredbi commented Jul 26, 2018

I did not release this fix, because there are many other problems here.
I may fix this, but then how about [][]Value, or map[string]Value, no even thinking of [][]map[string][][]Value... They all fail. So that is a bit more complicated.

@ntoljic
Copy link
Author

ntoljic commented Jul 26, 2018

Well, if this cannot be fixed for all possible cases it should at least be documented as a limitation of go-swagger, don't you think? Not sure, how heavily used this feature is anyway.
Would be nice to have it working for the 'simple' case though ;)

@fredbi
Copy link
Contributor

fredbi commented Jul 26, 2018

i am not saying this cannot be fixed. I am saying it is not fixed yet because it is not a simplistic issue.
About documenting limitations, many things have been added recently. Did you read it?

@ntoljic
Copy link
Author

ntoljic commented Jul 26, 2018

You got me there. I definitely have to catch up on the docs.
In the meantime, I can work around this issue by wrapping the array in an object.
Happy to test any fixes, although you seem to have a much better understanding of the wider implications and possible edge cases.

@fredbi fredbi self-assigned this Jul 29, 2018
@fredbi
Copy link
Contributor

fredbi commented Jul 29, 2018

Found minimal patch to make this one work.

There are many other edge cases with polymorphism.
ad this story shall also be told.

fredbi added a commit to fredbi/go-swagger that referenced this issue Jul 29, 2018
…body parameter)

> NOTE: polymorphism still gets a number of uncovered edge cases.
> This provides minimal patching for this particular case.
fredbi added a commit that referenced this issue Aug 3, 2018
…eter) (#1641)

> NOTE: polymorphism still gets a number of uncovered edge cases.
> This provides minimal patching for this particular case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug discriminator Related to polymorphic types model Related to swagger generate model command pending PR
Projects
None yet
Development

No branches or pull requests

3 participants