omitempty validation on input fields with/without defaults

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"
+    }
+  ]
+}