Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 129 additions & 0 deletions WIP.md
Original file line number Diff line number Diff line change
@@ -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 |
2 changes: 1 addition & 1 deletion readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
7 changes: 5 additions & 2 deletions validator/imported/export.js
Original file line number Diff line number Diff line change
Expand Up @@ -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));
},
});
Expand Down Expand Up @@ -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
Expand Down
7 changes: 4 additions & 3 deletions validator/imported/export.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion validator/imported/graphql-js-commit.log
Original file line number Diff line number Diff line change
@@ -1 +1 @@
48afd37a48d37f6d3342b959e8cb34e1ecbfeffb
123e958de1362eef098c30e917b51981c484729e
2 changes: 1 addition & 1 deletion validator/imported/readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
```

Expand Down
15 changes: 0 additions & 15 deletions validator/imported/spec/ValuesOfCorrectTypeRule.spec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 40 additions & 0 deletions validator/imported/spec/VariablesInAllowedPositionRule.spec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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}
16 changes: 2 additions & 14 deletions validator/rules/values_of_correct_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}()
}
Expand Down
29 changes: 29 additions & 0 deletions validator/rules/variables_in_allowed_position.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)
}
}
})
},
}