omitempty validation on input fields with/without defaults (#338)

Fixes #290 and
#228 (comment)
(for latter, only the comment of mine, not the whole issue)

This is only changing where the `omitempty` is allowed and forbidden -
it does not change where is the `omitempty` actually generated or not in
generated code.

Separately each line of changelog:
- allow `omitempty` on non-nullable input field, if the field has a
default (pretty much
#228 (comment))
- added a `&& field.DefaultValue == nil` to the `"omitempty may only be
used on optional arguments"` error
- allow `omitempty: false` on an input field, even when it is
non-nullable (#290)
  - `fieldDir.Omitempty != nil` changed to `fieldOptions.GetOmitempty()`
- forbid `omitempty: false` (including implicit behaviour) when using
pointer on non-null, no-default input field
- as setting a correct combination of directives (and potentially some
options, which changes implicit pointer and/or omitempty), and this
library promises "Compile-time validation of GraphQL queries: never ship
an invalid GraphQL query again!", I found it fitting to guard against
the most simple case, that can be enforced in Go type system.
- this, however, is a breaking change, so not sure if I should include
it here. No previously present test failed after such change, but for
example
`generate/testdata/errors/DefaultInputsNoOmitPointerForDirective.graphql`
would previously generate following (below - which has a possibility to
send invalid graphql input), but now the generation fails.
 ```
        type InputWithDefaults struct {
        	Field         *string `json:"field"`
        	NullableField string  `json:"nullableField"`
        }
```
- - alternative would be to force omitempty tag in such cases (even if there is no omitempty directive/option) - so that generation would not fail. But I'm not sure if I can afford to do that. That would probably still be breaking change (different generated code for same query), but a bit better. Maybe just setting omitempty flag instead of returning error would be sufficient.
  
In general, I have also moved the omitempty check from directives to the time of creating Go types/tags of field. This seems more consistent, as not all possibilities were caught before (i.e. general `@genqlient(omitemtpy: true)` vs `@genqlient(for: ..., omitempty: true)`). When creating Go type/tags, all the options/directives are already merged, so the final result is being checked. There is minor difference in error message (instead of reference to directive, the error refers to the whole operation, but also includes type.field name)


I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable
- [x] Included a link to the issue fixed, if applicable
- [ ] Included documentation, for new features
- [x] Added an entry to the changelog

docs/CHANGELOG.md

@@ -22,12 +22,19 @@ When releasing a new version:
 
 ### Breaking changes:
 
+- omitempty validation:
+  - forbid `omitempty: false` (including implicit behaviour) when using pointer on non-null, no-default input field
+
 ### New features:
 
 - genqlient now supports double-star globs for schema and query files; see [`genqlient.yaml` docs](genqlient.yaml) for more.
 
 ### Bug fixes:
 
+- omitempty validation:
+  - allow `omitempty` on non-nullable input field, if the field has a default
+  - allow `omitempty: false` on an input field, even when it is non-nullable
+
 ## v0.7.0
 
 In addition to several new features and bugfixes, along with this release comes reorganized [documentation](.) for genqlient. Note that genqlient now requires Go 1.20 or higher, and is tested through Go 1.22.

generate/convert.go

@@ -450,6 +450,19 @@ func (g *generator) convertDefinition(
 				return nil, err
 			}
 
+			// Try to protect against generating field type that has possibility to send `null` to non-nullable graphQL
+			// type. This does not protect against lists/slices, as Go zero-slices are already serialized as `null`
+			// (which can therefore currently send invalid graphQL value - e.g. `null` for [String!]!)
+			// And does not protect against custom MarshalJSON.
+			_, isPointer := fieldGoType.(*goPointerType)
+			if field.Type.NonNull && isPointer && !fieldOptions.GetOmitempty() {
+				return nil, errorf(pos, "pointer on non-null input field can only be used together with omitempty: %s.%s", name, field.Name)
+			}
+
+			if fieldOptions.GetOmitempty() && field.Type.NonNull && field.DefaultValue == nil {
+				return nil, errorf(pos, "omitempty may only be used on optional arguments: %s.%s", name, field.Name)
+			}
+
 			goType.Fields[i] = &goStructField{
 				GoName:      goName,
 				GoType:      fieldGoType,

generate/genqlient_directive.go

@@ -213,10 +213,6 @@ func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) er
 				return errorf(fieldDir.pos, "struct and flatten can't be used via for")
 			}
 
-			if fieldDir.Omitempty != nil && field.Type.NonNull {
-				return errorf(fieldDir.pos, "omitempty may only be used on optional arguments")
-			}
-
 			if fieldDir.TypeName != "" && fieldDir.Bind != "" && fieldDir.Bind != "-" {
 				return errorf(fieldDir.pos, "typename and bind may not be used together")
 			}

generate/testdata/errors/DefaultInputsNoOmitPointer.graphql

@@ -0,0 +1,7 @@
+# very similar to DefaultInputsNoOmitPointerForDirective.graphql - same expected behaviour, but takes a different code path(?)
+# @genqlient(pointer: true)
+query DefaultInputs(
+  $input: InputWithDefaults!
+) {
+  default(input: $input)
+}

generate/testdata/errors/DefaultInputsNoOmitPointerForDirective.graphql

@@ -0,0 +1,8 @@
+# Non-nullable input field with default cannot be pointer without omitempty - as that
+# would send `null`, which is invalid value.
+# @genqlient(for: "InputWithDefaults.field", pointer: true)
+query DefaultInputs(
+  $input: InputWithDefaults!
+) {
+  default(input: $input)
+}

generate/testdata/errors/OmitemptyDirective.graphql

@@ -0,0 +1,7 @@
+# One of the input filed is non-nullable -> cannot omit it
+# @genqlient(omitempty: true)
+query OmitemptyDirective (
+  $input: OmitemptyInput
+) {
+  omitempty(input: $input)
+}

generate/testdata/errors/OmitemptyForDirective.graphql

@@ -0,0 +1,7 @@
+# The specific input field is non-nullable -> cannot omit it
+# @genqlient(for: "OmitemptyInput.field", omitempty: true)
+query OmitemptyDirective (
+  $input: OmitemptyInput
+) {
+  omitempty(input: $input)
+}

generate/testdata/errors/schema.graphql

@@ -1,6 +1,10 @@
 type Query {
   f: String
   user: User
+  # The top-level default is currently not used - there is no query that would not specify the input argument.
+  # But it is here for completeness - maybe it will be used in future or cause some other unexpected issues.
+  default(input: InputWithDefaults! = {field: "input omitted"}): Boolean
+  omitempty(input: OmitemptyInput): Boolean
 }
 
 type User {
@@ -9,3 +13,13 @@ type User {
 }
 
 scalar ValidScalar
+
+input InputWithDefaults {
+  field: String! = "input field omitted"
+  nullableField: String = "nullable input field omitted"
+}
+
+input OmitemptyInput {
+  field: String!
+  nullableField: String
+}

generate/testdata/queries/DefaultInputs.graphql

@@ -0,0 +1,7 @@
+# Without any extra directives or configuration, the defaults are never considered,
+# as the client sends at least zero-value (struct with empty string).
+query DefaultInputs(
+  $input: InputWithDefaults!
+) {
+  default(input: $input)
+}

generate/testdata/queries/DefaultInputsPointer.graphql

@@ -0,0 +1,9 @@
+# The `InputWithDefaults.field` cannot be `pointer: true`, together with implicit `omitempty: false`, as `null` is
+# not a valid value there. However, nullableField should still be ok
+# (this will send null, overwriting the server's default)
+# @genqlient(for: "InputWithDefaults.nullableField", pointer: true)
+query DefaultInputs(
+  $input: InputWithDefaults!
+) {
+  default(input: $input)
+}

generate/testdata/queries/DefaultInputsWithDirective.graphql

@@ -0,0 +1,7 @@
+# very similar to DefaultInputsWithForDirective.graphql - same expected behaviour, but takes a different code path(?)
+# @genqlient(omitempty: true)
+query DefaultInputs(
+  $input: InputWithDefaults!
+) {
+  default(input: $input)
+}

generate/testdata/queries/DefaultInputsWithForDirective.graphql

@@ -0,0 +1,7 @@
+# @genqlient(for: "InputWithDefaults.field", omitempty: true)
+# @genqlient(for: "InputWithDefaults.nullableField", omitempty: true)
+query DefaultInputs(
+  $input: InputWithDefaults!
+) {
+  default(input: $input)
+}

generate/testdata/queries/OmitemptyFalse.graphql

@@ -0,0 +1,7 @@
+# @genqlient(omitempty: true)
+# @genqlient(for: "OmitemptyInput.field",omitempty: false)
+query OmitemptyFalse(
+  $input: OmitemptyInput
+) {
+  omitempty(input: $input)
+}

generate/testdata/queries/schema.graphql

@@ -185,6 +185,10 @@ type Query {
   recur(input: RecursiveInput!): Recursive
   acceptsListOfListOfListsOfDates(datesss: [[[Date!]!]!]!): Boolean
   getPokemon(where: getPokemonBoolExp): [Pokemon!]!
+  # The top-level default is currently not used - there is no query that would not specify the input argument.
+  # But it is here for completeness - maybe it will be used in future or cause some other unexpected issues.
+  default(input: InputWithDefaults! = {field: "input omitted"}): Boolean
+  omitempty(input: OmitemptyInput): Boolean
 }
 
 type Mutation {
@@ -212,3 +216,13 @@ input IntComparisonExp {
   _neq: Int
   _nin: [Int!]
 }
+
+input InputWithDefaults {
+  field: String! = "input field omitted"
+  nullableField: String = "nullable input field omitted"
+}
+
+input OmitemptyInput {
+  field: String!
+  nullableField: String
+}

generate/testdata/snapshots/TestGenerate-DefaultInputs.graphql-DefaultInputs.graphql.json

@@ -0,0 +1,9 @@
+{
+  "operations": [
+    {
+      "operationName": "DefaultInputs",
+      "query": "\nquery DefaultInputs ($input: InputWithDefaults!) {\n\tdefault(input: $input)\n}\n",
+      "sourceLocation": "testdata/queries/DefaultInputs.graphql"
+    }
+  ]
+}

generate/testdata/snapshots/TestGenerate-DefaultInputsPointer.graphql-DefaultInputsPointer.graphql.json

@@ -0,0 +1,9 @@
+{
+  "operations": [
+    {
+      "operationName": "DefaultInputs",
+      "query": "\nquery DefaultInputs ($input: InputWithDefaults!) {\n\tdefault(input: $input)\n}\n",
+      "sourceLocation": "testdata/queries/DefaultInputsPointer.graphql"
+    }
+  ]
+}