Skip to content

Commit

Permalink
disable input validation when use_struct_references is enabled (#344)
Browse files Browse the repository at this point in the history
Fix for #342. As
`use_struct_references` is incompatible with `omitempty` and `pointer`
validation, the most simple change is to disable the validation when the
`use_struct_references` is enabled. Note this implementation disables
the validation even for types that are not influenced by
`use_struct_references`, but still better than disabling the validation
globally.

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
  • Loading branch information
Fazt01 authored Jul 3, 2024
1 parent 35fbf5b commit 2f265d2
Show file tree
Hide file tree
Showing 8 changed files with 237 additions and 13 deletions.
3 changes: 2 additions & 1 deletion docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ 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
- forbid `omitempty: false` (including implicit behaviour) when using pointer on non-null input field

### New features:

Expand All @@ -34,6 +34,7 @@ When releasing a new version:
- 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
- don't do `omitempty` and `pointer` input types validation when `use_struct_reference` is used, as the generated type is often not compatible with validation logic.

## v0.7.0

Expand Down
33 changes: 21 additions & 12 deletions generate/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,9 @@ func (g *generator) convertType(
def := g.schema.Types[typ.Name()]
goTyp, err := g.convertDefinition(
namePrefix, def, typ.Position, selectionSet, options, queryOptions)
if err != nil {
return nil, err
}

if g.getStructReference(def) {
if options.Pointer == nil || *options.Pointer {
Expand All @@ -274,7 +277,8 @@ func (g *generator) convertType(
Elem: goTyp,
}
}
return goTyp, err

return goTyp, nil
}

// getStructReference decides if a field should be of pointer type and have the omitempty flag set.
Expand Down Expand Up @@ -450,17 +454,22 @@ 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)
if !g.Config.StructReferences {
// Only do these validation when StructReferences are not used, as that can generate types that would not
// pass these validations. See https://github.com/Khan/genqlient/issues/342

// 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{
Expand Down
5 changes: 5 additions & 0 deletions generate/generate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,11 @@ func TestGenerateWithConfig(t *testing.T) {
Enums: map[string]CasingAlgorithm{"Role": CasingRaw},
},
}},
{
"UseStructReference", "", []string{"UseStructReference.graphql"}, &Config{
StructReferences: true,
},
},
}

sourceFilename := "SimpleQuery.graphql"
Expand Down
6 changes: 6 additions & 0 deletions generate/testdata/queries/UseStructReference.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#https://github.com/Khan/genqlient/issues/342
query UseStructReference(
$input: UseStructReferencesInput!
) {
useStructReferencesInput(input: $input)
}
13 changes: 13 additions & 0 deletions generate/testdata/queries/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ type Query {
# 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
useStructReferencesInput(input: UseStructReferencesInput!): Boolean
}

type Mutation {
Expand Down Expand Up @@ -226,3 +227,15 @@ input OmitemptyInput {
field: String!
nullableField: String
}

input StructInput {
field: String
}

input UseStructReferencesInput {
struct: StructInput!
nullableStruct: StructInput
list: [StructInput!]!
listOfNullable: [StructInput]!
nullableList: [StructInput!]
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"operations": [
{
"operationName": "UseStructReference",
"query": "\nquery UseStructReference ($input: UseStructReferencesInput!) {\n\tuseStructReferencesInput(input: $input)\n}\n",
"sourceLocation": "testdata/queries/UseStructReference.graphql"
}
]
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 2f265d2

Please sign in to comment.