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

Client cannot unmarshal struct #1729

Closed
alexz0000 opened this issue Sep 28, 2018 · 7 comments · Fixed by #1788
Closed

Client cannot unmarshal struct #1729

alexz0000 opened this issue Sep 28, 2018 · 7 comments · Fixed by #1788
Labels
discriminator Related to polymorphic types enhancement model Related to swagger generate model command

Comments

@alexz0000
Copy link

alexz0000 commented Sep 28, 2018

Problem statement

Let's say, I have a struct called Pet

type struct Pet{
    Type string
    Name string
}

I also have a struct called Person

type struct Person{
    Name string
    Pet Pet
} 

I get the Person via API from server, the json looks like

{
"name":"Alex"
"pet":null
}

Then I use the client of go-swagger to unmarshal the json, I will get runtime panic.
Since the field 'Type' of Pet is required in my definition. When it unmarshal the value of 'pet', it will validate if the value of 'Type' is exist, if it's not exist, runtime panic happens. Actually, the value is null, so the client cannot get a value for the field 'Type'.

Do you think my scenario is reasonable?

Swagger specification

Swagger 2.0

Steps to reproduce

Environment

swagger version: 0.15.0
go version: 1.10.3
OS: Mac OS

@casualjim
Copy link
Member

I would need to see the spec and so on. For structs we generate pointers, so in the concrete scenario you described here, it would just marshal. We don't validate responses afaik.

@alexz0000
Copy link
Author

alexz0000 commented Sep 29, 2018

@casualjim More details for the generated client

func unmarshalPet(data []byte, consumer runtime.Consumer) (Pet, error) {
	buf := bytes.NewBuffer(data)
	buf2 := bytes.NewBuffer(data)

	// the first time this is read is to fetch the value of the type property.
	var getType struct {
		Type string `json:"type"`
	}
	if err := consumer.Consume(buf, &getType); err != nil {
		return nil, err
	}

	if err := validate.RequiredString("type", "body", getType.Type); err != nil {
		return nil, err
	}
        //Something else
}

Will get error from validate.RequiredString if data is 'null'
BTW, the Pet is a polymorphic class in java

@casualjim
Copy link
Member

yes it should be provided because that's how discriminators work. I don't know if that changed in 3.0

But polymorphic types all need to have their discriminator field filled out. The reason for this is that they need to be able to be located through reflection. This will be required in java, go, C#, ...

One might be able to make a case that we should provide an implementation of the base interface too and use that as value when there is no discriminator present, although it's more likely to be a bug.

@fredbi fredbi added question model Related to swagger generate model command discriminator Related to polymorphic types labels Oct 1, 2018
@woz5999
Copy link

woz5999 commented Oct 24, 2018

I think it would be reasonable in the case of structs that have optional properties whose type is polymorphic, to do a nil check, and if nil, skip unmarshalling of that property. This respects the optional nature of the struct property without changing the discriminator behavior in the unmarshal template.

We'd be happy to put in a PR to make this change if you agree that this is reasonable and desired.

@alexz0000
Copy link
Author

@casualjim Could we change it as @woz5999 said?

@casualjim
Copy link
Member

feel free to send a PR. This fix should be fairly trivial to make
It should be made somewhere here: https://github.com/go-swagger/go-swagger/blob/master/generator/templates/schema.gotmpl#L91

@alexz0000
Copy link
Author

I create a PR #1788

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discriminator Related to polymorphic types enhancement model Related to swagger generate model command
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants