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

null parameters cause crashes #346

Closed
venoms opened this Issue Sep 18, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@venoms

venoms commented Sep 18, 2018

I'm dealing with a swath of issues around gqlgen's implementation of defaults and nullable types. They seem bizarre, so I thought the issue was on my end, but i have been unable to find a sold root cause within my minimal code.

Expected Behaviour

When a null is passed as a parameter, it should be replaced with its default parameter.

Actual Behavior

Generated code attempts to parse <nil>, and then rejects the request.

Minimal graphql.schema and models to reproduce

type Query {
   history(start: Time = "anything", end: Time = "anything", limit: Int = 10)
}

image
image

@vektah

This comment has been minimized.

Show comment
Hide comment
@vektah

vektah Sep 18, 2018

Collaborator

When a null is passed as a parameter, it should be replaced with its default parameter.

I don't think that's quite true, the spec distinguishes between null and undefined (see hasValue) in
http://facebook.github.io/graphql/June2018/#sec-Coercing-Variable-Values
http://facebook.github.io/graphql/June2018/#CoerceArgumentValues()

from your example, History should be called as History(null,null,null, []). If you want the defaults, don't send the variables (undefined works too if using js).

The nil is not an int is definitely a bug though, it should be setting those nullable fields to null.

Collaborator

vektah commented Sep 18, 2018

When a null is passed as a parameter, it should be replaced with its default parameter.

I don't think that's quite true, the spec distinguishes between null and undefined (see hasValue) in
http://facebook.github.io/graphql/June2018/#sec-Coercing-Variable-Values
http://facebook.github.io/graphql/June2018/#CoerceArgumentValues()

from your example, History should be called as History(null,null,null, []). If you want the defaults, don't send the variables (undefined works too if using js).

The nil is not an int is definitely a bug though, it should be setting those nullable fields to null.

@vektah vektah added the bug label Sep 18, 2018

@venoms

This comment has been minimized.

Show comment
Hide comment
@venoms

venoms Sep 20, 2018

Thanks! Learned something about gql today :)

venoms commented Sep 20, 2018

Thanks! Learned something about gql today :)

@venoms

This comment has been minimized.

Show comment
Hide comment
@venoms

venoms Sep 20, 2018

I was actually looking at the code you use around marshalling / unmarshalling -- why choose map[string]interface{} over map[string]json.RawMessage? I was surprised to see interface{} involved in decoding in such an elegant library. You could, I think easily use the encoding interfaces like BinaryMarshaler, or "encoding/json".Marshaller if the transport is going to be JSON anyway.

I think also the number of pointer types that are generated is a bit excessive for most scenarios where "" vs undefined makes little difference. I was thinking perhaps gqlgen could support pointer types in resolvers but accept direct values, returning the zero value instead of nil.

venoms commented Sep 20, 2018

I was actually looking at the code you use around marshalling / unmarshalling -- why choose map[string]interface{} over map[string]json.RawMessage? I was surprised to see interface{} involved in decoding in such an elegant library. You could, I think easily use the encoding interfaces like BinaryMarshaler, or "encoding/json".Marshaller if the transport is going to be JSON anyway.

I think also the number of pointer types that are generated is a bit excessive for most scenarios where "" vs undefined makes little difference. I was thinking perhaps gqlgen could support pointer types in resolvers but accept direct values, returning the zero value instead of nil.

@vektah

This comment has been minimized.

Show comment
Hide comment
@vektah

vektah Sep 20, 2018

Collaborator

if the transport is going to be JSON anyway.

GraphQL doesn't specify the transport or encoding at all, the defacto standard is http + json, but it can be anything. gqlgen is written with this in mind, even though it targets the common use case out of the box.

I think also the number of pointer types that are generated is a bit excessive

If you write your own models the binding logic should let you remove most of them. There are some upcoming changes to make generated models more customizable, maybe a @zeroisnull will help generate these.

Collaborator

vektah commented Sep 20, 2018

if the transport is going to be JSON anyway.

GraphQL doesn't specify the transport or encoding at all, the defacto standard is http + json, but it can be anything. gqlgen is written with this in mind, even though it targets the common use case out of the box.

I think also the number of pointer types that are generated is a bit excessive

If you write your own models the binding logic should let you remove most of them. There are some upcoming changes to make generated models more customizable, maybe a @zeroisnull will help generate these.

@venoms

This comment has been minimized.

Show comment
Hide comment
@venoms

venoms Sep 20, 2018

Ah, I see so because the marshallers consume types they can't implement the standard encoding interfaces, which all consume bytes

venoms commented Sep 20, 2018

Ah, I see so because the marshallers consume types they can't implement the standard encoding interfaces, which all consume bytes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment