Treat unsupplied query variables as an empty variables object#867
Merged
Conversation
phdoerfler
marked this pull request as ready for review
July 14, 2026 10:26
milessabin
requested changes
Jul 17, 2026
milessabin
left a comment
Member
There was a problem hiding this comment.
I think this is along the right lines, but I don't like the way the tests have been handled. Instead of repurposing test("variable not defined (2)") that test should have been fixed by marking the type as not null, so that the test doesn't fail as a result of the null default being applied. A new test should be added to cover the defaulted case, and probably some others to cover variations on defaults.
Fixes typelevel#743. compileVars short-circuited to an empty Vars map when no variables JSON was supplied, so declared variables never received entries and any reference to one - at a field argument or inside an input object literal - failed elaboration with a spurious "Variable 'x' is undefined". Supplying an empty variables object took the coercion path and worked correctly, an inconsistency: per the GraphQL spec (CoerceVariableValues) an absent variable values map is equivalent to an empty one. The None case now coerces each declared variable against an absent value exactly as the Some path does for variables missing from the supplied object: defaults apply, nullable variables coerce to AbsentValue and are treated as not provided downstream, and non-nullable variables without defaults are errors. Two existing expectations changed as a consequence, both spec-aligned: a declared nullable variable used in a nullable argument position now compiles successfully with the argument absent rather than failing, and using a declared-but-unprovided variable in @Skip(if:) now fails in directive argument validation (a Boolean! position with no value) rather than with the misleading 'undefined' message.
Review follow-up: the fix newly makes variable-level defaults reachable when no variables JSON is supplied (previously the empty Vars map ignored them), which was uncovered. Also drop reportUnused = false from the issue 743 reproducer so it verifies the variable is seen as used, not merely that the spurious undefined error is gone.
phdoerfler
force-pushed
the
fix/issue-743
branch
from
July 17, 2026 13:25
7ca45a6 to
e91159e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: The current check failure is due to the usual
Your tlBaseVersion 0.28 is behind the latest tag 0.29.0.As a bit of an experiment, I put Claude to the task of figuring this one out. I have reviewed what I could and I have instructed Claude to adhere to proper test first etiquette and do a critical review of its own code but ultimately I am a bit out of my element here. This fix could be wrong. I am not deep enough into Grackle's compiler internals to deem this a correct fix or not. This being said: To me it looks fine. Also, the fix is very small, there is a reproducible test and the bug report is very recent.
And now without further ado, here is the PR text Claude proposed:
Fixes #743.
Symptom
Compiling a query that references a declared variable — at a field argument or inside an input object literal — fails with a spurious
Variable 'x' is undefinedwhen no variables JSON is supplied:The same query compiles fine with
untypedVars = Some(json"{}"), which is the tell: per the spec (CoerceVariableValues), an absent variable values map and an empty one must behave identically.Root cause
compileVarsshort-circuits toMap.emptywhenuntypedVarsisNone, so declared variables never receiveVarsentries. Any subsequent reference then fails the lookup inValue.elaborateValuewith the misleading "undefined" error. TheSomepath instead coerces every declared variable viacheckVarValue, which already handles absence correctly (defaults apply, nullable variables coerce toAbsentValue, non-nullable variables without defaults are errors).Note this is not specific to input objects, despite how the issue was encountered — a plain
profilePic(size: $x)with no variables supplied failed the same way. (The input-object variable collection bug in this area was fixed separately in #845; this reproducer still failed after it.)Fix
The
Nonecase now runs the same coercion as theSomepath with every variable absent.Behaviour changes (all spec-aligned, review attention welcome)
Two existing test expectations codified the old behaviour and had to change:
variable not defined (2)(now "variable declared but not provided is treated as absent"): a declared nullable variable used in a nullable argument position now compiles successfully with the argument absent, instead of failing. This is the same mechanism as unused var error is triggered when var appears in an input value #743 — the issue could not be fixed while keeping this expectation.variable not defined (5):@skip(if: $skipName)with$skipNamedeclared but unprovided still fails (aBoolean!position with no value), but now in directive argument validation (Expected Boolean found 'null' for 'if' in directive skip) rather than with the misleadingundefinedmessage.Additionally:
Value of type Int required for 'x' in variable values), even if unreferenced — matching both the spec and the existing behaviour when a variables object is supplied without that key.Varsmap ignored them entirely).Tests
issue/743branch (thanks @tpolecat), watched fail with the reported error before the fix, and run with unused-variable reporting enabled so it verifies the variable is seen as used.Noneconsistent with it).