Closed klondikedragon closed 2 days ago
Thanks for the report.
The issue is that after moving the validation to later stage (from parsing directives to generating Go types) (in my attempt to make the validation more consistent with various directives and global options), and the fact that use_struct_references
currently sets both pointer
and omitempty
flag (same as a directive would).
The use_struct_references
really means that predicate
(in example) would have omittable type where omitted value is not valid (and the validation tries to prevent that).
Solutions:
(1) Remove the validation - allowing any combination of pointer
and omitempty
, and potentially failing at runtime :(
(2) have use_struct_references
force omitempty
and pointer
only where valid - that is - input object (not regular/output objects), not nested in list, non-nullable (!
), without default value - would have a different generated type. In this example, generate Predicate Int_comparison_exp 'json:"predicate"
instead of Predicate *Int_comparison_exp 'json:"predicate,omitempty"
. This is also breaking change in a sense that generated type would change to non-pointer (forcing user of generated code to always provide a value, or rely on zero-initialized struct)
(3) selectively validate only cases (inconsistent pointer/omitempty) caused by directives, and not by use_struct_references
- don't know how to do this easily
(4) ? something else?
After trying for a while to do a change of (2) - unsuccessfully (the conditions just keep growing) - and also considering the necessity to change the generated type in some cases, I'm currently leaning towards (1) just removing these validations and will try to do a PR if I (or someone else) won't come up with better solution.
Temporary workaround (if necessary):
As the use_struct_references
only forces these flags only when not previously set, # @genqlient(for: "itl_agents_aggregate_bool_exp_count.predicate", omitempty: false, pointer: false)
, should override it, causing the behavior described in (2)
Great analysis. Can we do something like (2) but apply it only to omitempty (not pointer)? Off the top of my head that seems like it would preserve behavior at least in the obvious cases.
Great analysis. Can we do something like (2) but apply it only to omitempty (not pointer)? Off the top of my head that seems like it would preserve behavior at least in the obvious cases.
The omitempty and pointer are quite related: removing just omitempty would still leave the type "not valid", in a sense that zero value would send a null
, which is not valid for non-nullable type.
(all related to use_struct_references: true
):
omitempty: true, pointer: true
; this means non-initialized / zero-value omits the field, which is not valid for non-nullable-type without default.omitempty: false, pointer: false
: this changes the generated pointer field type to non-pointer. It is the only valid combination that prevents possibility of sending invalid field value (at least for simple types, does not include lists or some fancy custom marshalers)omitempty: false, pointer: true
: this changes what is sent, from omited field to null
, but that is still not valid for non-nullable type.Just to be clear, "not valid" above means the type can be generated and used, just that there is a possibility of having invalid value (zero value) in the type that causes error during sending of the query.
In the https://github.com/Khan/genqlient/pull/338 , I added this pointer validation (together with moving the omitempty validation from directives to type generation): https://github.com/Khan/genqlient/blob/main/generate/convert.go#L453
Hmm, I see what you mean. But on reading slightly more carefully, removing the pointers seems like too big a compat break; I suspect it will break many of the Hasura users using this option, at least, and in a "rewrite most of your code that uses genqlient types" kind of way. (See #149 for examples of the schemas we're dealing with here. Unfortunately it seems we don't have any tests with required inputs with this option, which we should.)
Another possible least-bad option: what if we disable the validation only if use_struct_references
is set? That's kind of an easy variation of your option (3), with the option that we could improve it to what you describe if we figure out how. (I've been hoping some day to completely rewrite the directive and option handling to have more of a separate resolution step instead of doing that all inline, which would probably make it much easier.)
Optionally we could also add use_struct_references: v2
(or something) which would be either the current (post-#338) behavior, or probably better, (2). It's not backwards compatible but we could encourage that for new users of the option. I don't love adding more versions of use_struct_references
-- but I suspect that may be where #272 ends up one day anyway (whether it's called use_struct_references: v3
, or hasura: true
, or what). (And again if we rewrite the option handling it would make the code for that more manageable.)
By the way, I forgot to say: @klondikedragon thanks for testing the latest version! This is how we keep this stuff out of the tagged releases :-) .
Another possible least-bad option: what if we disable the validation only if use_struct_references is set? That's kind of an easy variation of your option (3), with the option that we could improve it to what you describe if we figure out how. (I've been hoping some day to completely rewrite the directive and option handling to have more of a separate resolution step instead of doing that all inline, which would probably make it much easier.)
That does seem like the most reasonbale middleground. I requested a PR.
I also want to mention a weird behaviour that I encountered when attempting (2) in https://github.com/Khan/genqlient/pull/343. When the input field type is a list, the omitempty (from use_struct_references) is propagated from the struct item to the list itself. E.g:
listField: [SomeStructInput]
-> ListField []*SomeStructInput 'json:"listField,omitempty"
but
listField2 [String]
-> ListField2 []string 'json:"listField2"'
The omitempty is applied to the list/slice, not the inner type (omitempty on inner type doesn't really even make sense). But it seemed surprising to me that an inner type influenced how the list is sent / omited. In https://github.com/Khan/genqlient/pull/343 I tried to change the behaviour to only add omitempty
if the struct was not wrapped in list. After that https://github.com/Khan/genqlient/pull/343 (that will not be merged), it would look like this
Probably nothing to change now, but maybe could be considered if there was some larger refactor or new option like use_struct_references v2
Yeah, we generally never do *[]T
, only []*T
and []T
, since slices already have a nil
value. It could be something to revisit in the future though! omitempty
can also make sense on a slice, if (and only if) the list itself is what's nullable in GraphQL (i.e. [T!]
, not [T]!
). Not sure if that's what we currently do though.
my point is that for use_struct_references
(which adds both pointer and omitempty unconditionaly) e.g. graphql [T]!
, the Go type will currently be []*T
. The pointer is correct, but omitempty is also added (from struct_references) and that applies to the slice, not pointer. Which is weird, as use_struct_references
adds pointer and omitempty to different things - pointer to innermost struct, and omitempty to outermost slice. But again, I guess this probably cannot be changed, as it was like this for a while and different behaviour could (breaking) change existing use_struct_references
usage.
Thanks for all the careful consideration with this use case!
Just for reference, I landed up also adding the following to my genqlient.yaml, as I was fighting some non-deterministic behavior trying to mark only certain fields in responses as using pointers and as optional. (I think it had to do with the order of resolving certain shared fragment types used in mutation inputs for different queries where one query had a field in the fragment type marked as optional, and another query that referenced the same fragment type as mandatory.
use_struct_references: true
optional: pointer
I haven't tried this configuration yet with the latest code after #344, I imagine it would work as expected, I'll report back if there is an issue. Thanks!
Describe the bug In trying out the latest unreleased version of the repo (hoping it would fix a different bug with nondeterministic failures during codegen where it's sometimes (but not always) ignoring a "pointer: true" option for a deeply nested input field), I found that the new omitempty validation logic in #338 is incompatible with
use_struct_references: true
ingenqlient.yaml
. I'm not specifyingomitempty
explicitly anywhere in my GraphQL queries. Since use_struct_references is set to true, it seems to be automatically settingpointer: true
andomitempty: true
on every struct field, but the new validation logic does not likeomitempty: true
being set if the struct field is not optional.To Reproduce I'm using a fairly complex GraphQL schema (auto-generated by Hasura). Here is an example failure message:
Here is the relevant portion of the GraphQL schema:
Expected behavior The new omitempty validation logic should be compatible with
use_struct_references: true
genqlient version Tip of main branch: 35fbf5b