use_struct_references not compatible with new omitempty validation logic

[klondikedragon]

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 in genqlient.yaml. I'm not specifying omitempty explicitly anywhere in my GraphQL queries. Since use_struct_references is set to true, it seems to be automatically setting pointer: true and omitempty: true on every struct field, but the new validation logic does not like omitempty: 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:

go run github.com/Khan/genqlient
.../internal/schema/itl-app.graphql:209: omitempty may only be used on optional arguments: Itl_agents_aggregate_bool_exp_count.predicate
exit status 1

Here is the relevant portion of the GraphQL schema:

input itl_agents_aggregate_bool_exp {
  count: itl_agents_aggregate_bool_exp_count
}

input itl_agents_aggregate_bool_exp_count {
  arguments: [itl_agents_select_column!]
  distinct: Boolean
  filter: itl_agents_bool_exp
  predicate: Int_comparison_exp!
}

Expected behavior
The new omitempty validation logic should be compatible with use_struct_references: true

genqlient version
Tip of main branch: 35fbf5b

[Fazt01]

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)

[benjaminjkraft]

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.

[Fazt01]

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):

• before change in omitempty validation on input fields with/without defaults #338:
  • omitempty: true, pointer: true; this means non-initialized / zero-value omits the field, which is not valid for non-nullable-type without default.
• after change in omitempty validation on input fields with/without defaults #338, the generated type is the same, but causes validation error
• after potential change in (2):
  • 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)
• after potential modified (2) you mention:
  • 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 #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

[benjaminjkraft]

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 :-) .

[Fazt01]

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 #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 #343 I tried to change the behaviour to only add omitempty if the struct was not wrapped in list. After that #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

[benjaminjkraft]

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.

[Fazt01]

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.

[klondikedragon]

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!

[klondikedragon]

@benjaminjkraft -- I can confirm this is working correctly with the latest HEAD. Thank you! There is an unrelated usability consideration with Hasura schemas that I'll add as a comment to #272