fix: prevent GraphQL scalar/union from inheriting a later type's fields and line range#477
Open
JOhnsonKC201 wants to merge 1 commit into
Open
Conversation
…ds and line range scalar and union definitions have no brace-delimited body, but extractDefinitions unconditionally called extractFields() and searched for the next }. For a body-less definition that scanned forward into the next braced definition, so e.g. `scalar DateTime` declared above a `type` inherited that type's fields and closing-brace line range. Guard field/brace extraction behind a hasBody check (kind is not scalar or union). Body-less definitions now report no fields and a single-line range; braced definitions (type/input/interface/enum) are unchanged. Adds regression tests for a scalar and a union declared above a type.
Contributor
|
Reviewed for correctness of the scalar/union body-less guard. The fix is correct and well-scoped. Verification I ran
Correctness notes
Minor
Overall: correct fix for a real, high-impact parsing bug (custom scalars above object types are ubiquitous). Tests are meaningful and assert both the body-less definition and the unaffected following type. Good to merge. |
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.
Summary
GraphQLParser.extractDefinitionsmis-parsesscalaranduniondefinitions. Both are matched by the definition regex but have no{ ... }body, yet the parser unconditionally callsextractFields()and searches for the next}. For a body-less definition this scans forward into the next braced definition, so the scalar/union steals that type's fields and its closing-brace line range.This corrupts the knowledge graph for almost any real GraphQL schema, since a custom scalar above object types (
scalar DateTime,scalar JSON,scalar Upload, …) is ubiquitous.Steps to reproduce
Expected:
DateTime→{ kind: scalar, lineRange: [1, 1], fields: [] }Actual (before fix):
DateTime→{ kind: scalar, lineRange: [1, 6], fields: [id, name] }The
scalarwrongly spans lines 1–6 and claimsUser'sid/namefields. Auniondeclared above a braced type behaves identically.Fix
Guard field/brace extraction behind a
hasBodycheck (kindis notscalarorunion). Body-less definitions now report no fields and a single-line range. Braced definitions (type/input/interface/enum) are unchanged — only ~10 lines, scoped toextractDefinitions.Tests
Adds two regression tests (a
scalarand aunioneach declared above atype) asserting the body-less definition gets no fields and a single-line range, and that the following type keeps its own fields/range.pnpm --filter @understand-anything/core test→ 755 passed (was 753; +2 new). Both new tests fail onmainand pass with this change.pnpm lint→ clean.PR checklist
pnpm test)console.logor debug code left behindEnvironment
Found while auditing the parser layer. Repro is platform-independent; verified on Node v24 / pnpm 10.6.2.