From 7ed1bbdd0089247361abecd35b40b22eeb7b0003 Mon Sep 17 00:00:00 2001 From: Steve Coffman Date: Mon, 24 Nov 2025 11:19:53 -0500 Subject: [PATCH 1/2] Initial import Signed-off-by: Steve Coffman --- validator/imported/graphql-js-commit.log | 2 +- .../spec/ValuesOfCorrectTypeRule.spec.yml | 15 ------- .../VariablesInAllowedPositionRule.spec.yml | 40 +++++++++++++++++++ 3 files changed, 41 insertions(+), 16 deletions(-) diff --git a/validator/imported/graphql-js-commit.log b/validator/imported/graphql-js-commit.log index 58d20686..ae97536e 100644 --- a/validator/imported/graphql-js-commit.log +++ b/validator/imported/graphql-js-commit.log @@ -1 +1 @@ -48afd37a48d37f6d3342b959e8cb34e1ecbfeffb +e2457b33e928f4ec8b22e96a6dc6cb2808c03dfa diff --git a/validator/imported/spec/ValuesOfCorrectTypeRule.spec.yml b/validator/imported/spec/ValuesOfCorrectTypeRule.spec.yml index 194f5824..0d3ef799 100644 --- a/validator/imported/spec/ValuesOfCorrectTypeRule.spec.yml +++ b/validator/imported/spec/ValuesOfCorrectTypeRule.spec.yml @@ -1015,21 +1015,6 @@ - message: Field "OneOfInput.stringField" must be non-null. locations: - {line: 4, column: 37} -- name: Invalid oneOf input object value/Exactly one nullable variable - rule: ValuesOfCorrectType - schema: 0 - query: |2- - - query ($string: String) { - complicatedArgs { - oneOfArgField(oneOfArg: { stringField: $string }) - } - } - - errors: - - message: Variable "string" must be non-nullable to be used for OneOf Input Object "OneOfInput". - locations: - - {line: 4, column: 37} - name: Invalid oneOf input object value/More than one field rule: ValuesOfCorrectType schema: 0 diff --git a/validator/imported/spec/VariablesInAllowedPositionRule.spec.yml b/validator/imported/spec/VariablesInAllowedPositionRule.spec.yml index e21f0e74..b58ceccc 100644 --- a/validator/imported/spec/VariablesInAllowedPositionRule.spec.yml +++ b/validator/imported/spec/VariablesInAllowedPositionRule.spec.yml @@ -346,3 +346,43 @@ dog @include(if: $boolVar) } errors: [] +- name: Allows exactly one non-nullable variable + rule: VariablesInAllowedPosition + schema: 0 + query: |2- + + query ($string: String!) { + complicatedArgs { + oneOfArgField(oneOfArg: { stringField: $string }) + } + } + + errors: [] +- name: Undefined variable in oneOf input object + rule: VariablesInAllowedPosition + schema: 0 + query: |2- + + { + complicatedArgs { + oneOfArgField(oneOfArg: { stringField: $undefinedVariable }) + } + } + + errors: [] +- name: Forbids one nullable variable + rule: VariablesInAllowedPosition + schema: 0 + query: |2- + + query ($string: String) { + complicatedArgs { + oneOfArgField(oneOfArg: { stringField: $string }) + } + } + + errors: + - message: Variable "$string" is of type "String" but must be non-nullable to be used for OneOf Input Object "OneOfInput". + locations: + - {line: 2, column: 14} + - {line: 4, column: 50} From 4b2d25d34dfe833bdcf9aebde266f619a1153438 Mon Sep 17 00:00:00 2001 From: Steve Coffman Date: Fri, 24 Apr 2026 16:03:47 -0400 Subject: [PATCH 2/2] Adjust export.sh script and commit to graphql-js v16.13.2 Signed-off-by: Steve Coffman # Conflicts: # validator/rules/values_of_correct_type.go --- WIP.md | 129 ++++++++++++++++++ readme.md | 2 +- validator/imported/export.js | 7 +- validator/imported/export.sh | 7 +- validator/imported/graphql-js-commit.log | 2 +- validator/imported/readme.md | 2 +- validator/rules/values_of_correct_type.go | 16 +-- .../rules/variables_in_allowed_position.go | 29 ++++ 8 files changed, 172 insertions(+), 22 deletions(-) create mode 100644 WIP.md diff --git a/WIP.md b/WIP.md new file mode 100644 index 00000000..6089e59a --- /dev/null +++ b/WIP.md @@ -0,0 +1,129 @@ +# SDL Validation Gaps: graphql-js vs gqlparser `ValidateSchemaDocument` + +## Architectural context + +Two facts apply throughout this analysis. + +**Fail-fast vs. accumulate.** gqlparser returns on the first error encountered; graphql-js collects and reports all errors in one pass. A schema with multiple violations will always surface only one error in gqlparser. + +**Isolated validation.** graphql-js SDL rules receive a `SDLValidationContext` carrying a pre-existing schema object, enabling checks like "this type already exists in the schema you're extending." gqlparser validates a single `SchemaDocument` in isolation. Checks that require pre-existing schema context are not gaps — they are an architectural difference. + +--- + +## Unintentional gaps + +### `UniqueEnumValueNames` — missing entirely + +gqlparser's `Enum` case in `validateDefinition` (`schema.go:285`) checks only that the value list is non-empty and that no value is named `true`, `false`, or `null`. There is no duplicate check. Enum values from extensions are appended unconditionally at line 58 (`def.EnumValues = append(def.EnumValues, ext.EnumValues...)`), and nothing checks for duplicates afterward. + +This schema loads silently: + +```graphql +enum Direction { NORTH SOUTH } +extend enum Direction { NORTH } +``` + +graphql-js rejects with: +> `Enum value "Direction.NORTH" already exists in the schema. It cannot also be defined in this type extension.` + +`schema_test.yml` has no case covering enum value uniqueness, confirming this is untested. + +--- + +### `UniqueArgumentDefinitionNames` — missing entirely + +`validateArgs` (`schema.go:337`) checks that argument names don't begin with `__`, that referenced types exist, and that argument directives are valid. It never checks for duplicate argument names within the same list. Both field arguments and directive arguments are unprotected: + +```graphql +type Query { + field(id: ID, id: String): Boolean +} +``` + +graphql-js rejects with: +> `Argument "Query.field(id:)" can only be defined once.` + +`schema_test.yml` has no case for this. + +--- + +### `UniqueOperationTypes` — silent overwrite in three distinct cases + +graphql-js rejects any attempt to specify the same operation type more than once. gqlparser silently overwrites with the last value in all three scenarios. + +**Case A — duplicate within one `schema {}` block:** + +```graphql +schema { query: A query: B } +``` + +The loop at `schema.go:110` processes both entries and assigns `schema.Query` twice. Last writer wins, no error. `schema_test.yml`'s "multiple schema entry points" test only covers two separate `schema {}` blocks, not two operations within one block. + +**Case B — two `extend schema` blocks both specifying the same operation:** + +```graphql +schema { query: Query } +extend schema { mutation: Mut } +extend schema { mutation: OtherMut } +``` + +The loop at `schema.go:130` overwrites `schema.Mutation` on the second extension. No error. + +**Case C — `extend schema` re-specifying an operation from the base `schema {}` block:** + +```graphql +schema { query: Query } +extend schema { query: Other } +``` + +`schema.Query` ends up pointing at `Other`. graphql-js rejects with: +> `Type for query already defined in the schema. It cannot be redefined.` + +--- + +## Intentional divergences + +### `PossibleTypeExtensions` — allows extensions of undefined types + +When an extension references a type that doesn't exist, gqlparser creates a synthetic `Definition` for it (`schema.go:40–47`) and continues. graphql-js rejects with: +> `Cannot extend type "X" because it is not defined.` + +This is **intentional**: `schema_test.yml` has an explicit test case "can extend non existant types" asserting no error. The practical use case is federation-style schemas where types are extended without a local base definition. + +The consequence worth noting: the resulting ghost type does pass through `validateDefinition`. If the extension body provides at least one field and all types referenced in it exist, the ghost type becomes a valid Object type in the compiled schema. A typo in an extension's type name therefore produces a new, unexpected type rather than an error. + +--- + +### `UniqueDirectiveNames` — builtin redeclaration silently accepted + +For non-builtin directives, gqlparser correctly returns an error (`schema.go:98`), tested by `schema_test.yml`'s "cannot redeclare directives" case. For the six builtins — `include`, `skip`, `deprecated`, `specifiedBy`, `defer`, `oneOf` — a redeclaration is silently accepted with the first definition kept. `schema_test.yml` has an explicit "can redeclare builtin directives" test asserting this. + +graphql-js rejects any directive redefinition, including builtins: +> `Directive "@skip" already exists in the schema. It cannot be redefined.` + +The rationale is documented at `schema.go:93`: servers may ship directive definitions from an older or divergent spec version, and validating definition equivalence is considered more work than it's worth. The practical consequence is that a schema with a conflicting `@deprecated` or `@skip` definition loads without error. + +--- + +## Confirmed covered + +**`UniqueTypeNames`** — the first-pass map insertion at `schema.go:29–34` catches any type defined more than once in the document, returning `"Cannot redeclare type X."` Tested by `schema_test.yml`. + +**`UniqueFieldDefinitionNames`** — field merging (`schema.go:56`) is followed by an O(n²) pair-scan at `schema.go:311–317`, catching duplicates within a definition, across definition + extension, and across multiple extensions. Tested by three cases in `schema_test.yml`. + +**`LoneSchemaDefinition`** — `len(sd.Schema) > 1` is checked at `schema.go:104`. The graphql-js check for "schema already defined in prior context" is an isolated-validation architectural difference, not a gap. + +--- + +## Summary + +| Rule | Status | Nature | +|---|---|---| +| `UniqueEnumValueNames` | Missing | Unintentional gap — no test, no check | +| `UniqueArgumentDefinitionNames` | Missing | Unintentional gap — no test, no check | +| `UniqueOperationTypes` | Missing (3 cases) | Unintentional gap — silent overwrite | +| `PossibleTypeExtensions` | Intentional divergence | Allows ghost types; federation use case | +| `UniqueDirectiveNames` (builtins) | Intentional divergence | Explicit test documents the choice | +| `UniqueTypeNames` | Covered | — | +| `UniqueFieldDefinitionNames` | Covered | — | +| `LoneSchemaDefinitionRule` | Covered / arch. difference | Within-doc check present | diff --git a/readme.md b/readme.md index ce756974..6cf1892a 100644 --- a/readme.md +++ b/readme.md @@ -3,7 +3,7 @@ gqlparser [![CircleCI](https://badgen.net/circleci/github/vektah/gqlparser/maste This is a parser for graphql, written to mirror the graphql-js reference implementation as closely while remaining idiomatic and easy to use. -spec target: [October 2021](https://spec.graphql.org/October2021/) and [select portions of the Draft](https://spec.graphql.org/draft/), based on the graphql-js reference implementation [graphql-js v16.10.0](https://github.com/graphql/graphql-js/releases/tag/v16.10.0). This includes Schema definition language, block strings as descriptions, error paths & extension, etc. If there is a spec update or [new release](https://github.com/graphql/graphql-spec/releases), please follow [this process to update](./validator/imported/readme.md) and submit a PR. +spec target: [September 2025](https://spec.graphql.org/October2021/) and [select portions of the Draft](https://spec.graphql.org/draft/), based on the graphql-js reference implementation [graphql-js v16.13.2](https://github.com/graphql/graphql-js/releases/tag/v16.13.2). This includes Schema definition language, block strings as descriptions, error paths & extension, etc. If there is a spec update or [new release](https://github.com/graphql/graphql-spec/releases), please follow [this process to update](./validator/imported/readme.md) and submit a PR. This parser is used by [gqlgen](https://github.com/99designs/gqlgen), and it should be reasonably stable. diff --git a/validator/imported/export.js b/validator/imported/export.js index 555f28cb..7db48aa6 100644 --- a/validator/imported/export.js +++ b/validator/imported/export.js @@ -131,7 +131,10 @@ function exportSpecDefinitions() { expectSDLValidationErrors(schema, rule, sdlStr) { return resultProxy("expectSDLValidationErrors", { toDeepEqual(expected) { - // ignore now... + // ignore now... pluggable named-rule system here is for + // query/operation validation only. + // SDL Validation happens imperatively in ValidateSchemaDocument in + // schema.go, at schema load time so error messages will diverge. // console.warn(rule.name, sdlStr, JSON.stringify(expected, null, 2)); }, }); @@ -209,7 +212,7 @@ function exportSpecDefinitions() { let schemaList = schemas.map((s) => printSchema(s)); schemaList[0] += ` -# injected becuase upstream spec is missing some types +# injected because upstream spec is missing some types extend type QueryRoot { field: T f1: Type diff --git a/validator/imported/export.sh b/validator/imported/export.sh index 40b80a52..fa5e5a69 100755 --- a/validator/imported/export.sh +++ b/validator/imported/export.sh @@ -4,8 +4,9 @@ REPO_DIR=./graphql-js EXPORTER_ROOT="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" cd "$EXPORTER_ROOT" || exit - -GIT_REF=origin/main +# graphql-js default branch is not main anymore +MAIN="16.x.x" +GIT_REF="origin/${MAIN}" if [[ -f "$EXPORTER_ROOT/graphql-js-commit.log" ]] ; then GIT_REF=$(cat "$EXPORTER_ROOT/graphql-js-commit.log") @@ -15,7 +16,7 @@ echo "$GIT_REF" if [[ -d "$REPO_DIR" ]] ; then echo "fetching graphql-js with ${GIT_REF}" cd "$REPO_DIR" || exit - git fetch origin main + git fetch origin "${MAIN}" git checkout "$GIT_REF" git reset --hard else diff --git a/validator/imported/graphql-js-commit.log b/validator/imported/graphql-js-commit.log index ae97536e..7d335dd6 100644 --- a/validator/imported/graphql-js-commit.log +++ b/validator/imported/graphql-js-commit.log @@ -1 +1 @@ -e2457b33e928f4ec8b22e96a6dc6cb2808c03dfa +123e958de1362eef098c30e917b51981c484729e diff --git a/validator/imported/readme.md b/validator/imported/readme.md index f7bdba32..a5597b55 100644 --- a/validator/imported/readme.md +++ b/validator/imported/readme.md @@ -8,7 +8,7 @@ Direct modifications should not be made to most of this directory, instead take # update to latest $ rm graphql-js-commit.log && ./export.sh -# re-generate with known revision +# re-generate with known revision set in graphql-js-commit.log $ ./export.sh ``` diff --git a/validator/rules/values_of_correct_type.go b/validator/rules/values_of_correct_type.go index 5126245e..95a06099 100644 --- a/validator/rules/values_of_correct_type.go +++ b/validator/rules/values_of_correct_type.go @@ -188,20 +188,8 @@ func ruleFuncValuesOfCorrectType(observers *Events, addError AddErrFunc, disable return } - isVariable := fieldValue.Kind == ast.Variable - if isVariable { - variableName := fieldValue.VariableDefinition.Variable - isNullableVariable := !fieldValue.VariableDefinition.Type.NonNull - if isNullableVariable { - addError( - Message( - `Variable "%s" must be non-nullable to be used for OneOf Input Object "%s".`, - variableName, - value.Definition.Name, - ), - At(fieldValue.Position), - ) - } + if fieldValue.Kind == ast.Variable { + return } }() } diff --git a/validator/rules/variables_in_allowed_position.go b/validator/rules/variables_in_allowed_position.go index d3d36a29..a10cb9ed 100644 --- a/validator/rules/variables_in_allowed_position.go +++ b/validator/rules/variables_in_allowed_position.go @@ -44,5 +44,34 @@ var VariablesInAllowedPositionRule = Rule{ ) } }) + + observers.OnValue(func(walker *Walker, value *ast.Value) { + if value.Kind != ast.ObjectValue || value.Definition == nil { + return + } + if value.Definition.Directives.ForName("oneOf") == nil { + return + } + + for _, child := range value.Children { + fieldValue := child.Value + if fieldValue == nil || fieldValue.Kind != ast.Variable || + fieldValue.VariableDefinition == nil { + continue + } + if !fieldValue.VariableDefinition.Type.NonNull { + addError( + Message( + `Variable "%s" is of type "%s" but must be non-nullable to be used for OneOf Input Object "%s".`, + fieldValue, + fieldValue.VariableDefinition.Type.String(), + value.Definition.Name, + ), + At(fieldValue.VariableDefinition.Position), + At(fieldValue.Position), + ) + } + } + }) }, }