Add omitempty for pointer input fields
#264
Conversation
When `optional: "pointer"` is configured, `omitempty` is needed for input
fields for the generated code to be semantically correct. Without `omitempty`
an empty pointer would generate an "empty value" which is incorrect.
Consider the following graphql schema generated by Hasura:
```
input task_insert_input {
id: Int
}
```
Without this fix, the following golang struct will be generated:
```
type Task_insert_input struct {
Id: *int `json:"id"`
}
```
When we populate the struct without `Id` being set, it'd serialize to
`{ id: 0 }` as oppose to `{}` (i.e. so that the id would be generated by Hasura)
dharijanto
requested review from
benjaminjkraft,
csilvers,
dnerdy and
StevenACoffman
as code owners
|
Created for this: #260 |
| @@ -439,6 +439,11 @@ func (g *generator) convertDefinition( | |||
| return nil, err | |||
| } | |||
|
|
|||
| if options.GetPointer() || (!field.Type.NonNull && g.Config.Optional == "pointer") { | |||
There was a problem hiding this comment.
I wonder if this should apply to non-input structures as well..?
There was a problem hiding this comment.
omitempty is meaningless for outputs (since it's meaningless on unmarshal)
There was a problem hiding this comment.
I think as-is this is in some sense a breaking change, although perhaps anyone who cares about this prefers the new behavior? It also seems confusing to do this only and automatically for optional: pointer, which right now is I think otherwise equivalent to # @genqlient(pointer: true) on every optional field. Maybe we add a new config optional: pointer_and_omitempty and handle both issues that way?
(Edit: Thinking more, doing a new config doesn't strictly handle the second issue -- you could want this for non-pointer: true fields. But I guess you can always annotate those individually, and probably you never want it for non-pointer fields globally (since that means false gets omitted). So I guess it's still maybe the best compromise.)
BTW, you'll need to update the tests, docs, changelog, etc. -- see the PR message for the checklist.
| oe := true | ||
| fieldOptions.Omitempty = &oe |
There was a problem hiding this comment.
There's no need to mutate fieldOptions -- just compute the correct value and then use it on line 453.
There was a problem hiding this comment.
A belated realization: actually the logic here is also wrong: users may need to override the global option locally, if they do need to pass an explicit null. (Sadly in Go, as bad as it is to represent "T or null", it's even worse to represent "T or null or unset", which to be fair is also true of almost any language that's not JavaScript.) That is, if you set this option, then you can do # @genqlient(omitempty: false) if you do want explicit null for some query.
So that means you'll need to actually look at the value of fieldOptions.Omitempty and then only use this logic if it's null, I think.
There was a problem hiding this comment.
@benjaminjkraft there doesn't seem to be much movement here on your feedback but I really really need this feature so I've created #287 with your suggestion, in order to get things forward. Feel free to take a look over there :)
| @@ -439,6 +439,11 @@ func (g *generator) convertDefinition( | |||
| return nil, err | |||
| } | |||
|
|
|||
| if options.GetPointer() || (!field.Type.NonNull && g.Config.Optional == "pointer") { | |||
There was a problem hiding this comment.
omitempty is meaningless for outputs (since it's meaningless on unmarshal)
|
This would be super awesome. The GraphQL server I'm interacting with has deeply nested inputs and without this feature it breaks. Seems like |
|
Closing in favor of #308 which uses a config option |
When
optional: "pointer"is configured,omitemptyis needed for input fields for the generated code to be semantically correct. Withoutomitemptyan empty pointer would generate an "empty value" which is incorrect.Consider the following graphql schema generated by Hasura:
Without this fix, the following golang struct will be generated:
When we populate the struct without
Idbeing set, it'd serialize to{ id: 0 }as oppose to{}(i.e. so that the id would be generated by Hasura)I have: