Skip to content

fix: anchor inline fragment-definition detection in Client#parse#82

Open
rellampec wants to merge 2 commits into
github-community-projects:masterfrom
rellampec:fix/anchor-fragment-definition-match
Open

fix: anchor inline fragment-definition detection in Client#parse#82
rellampec wants to merge 2 commits into
github-community-projects:masterfrom
rellampec:fix/anchor-fragment-definition-match

Conversation

@rellampec

Copy link
Copy Markdown

The bug

When Client#parse walks ...Name spreads, it decides for each whether Name is a named fragment defined in the same document (leave the spread alone) or a Ruby constant to resolve. That decision used an unanchored match:

str.match(/fragment\s*#{const_name}/)

which matches too eagerly:

  • \s* allows zero whitespace (fragmentName matches), and
  • there's no boundary after the name, so a ...UserFields spread matches a fragment UserFieldsExtended on User definition.

When that false positive happens, the spread is left unresolved and is never registered as a constant dependency.
Downstream this doesn't just produce a wrong query — it crashes in sliced_definitions with TypeError: no implicit conversion of nil into Array. An earlier attempt to tighten this matching was reverted long ago (b5a4612), so the eager form has been in place since.

The fix

Anchor the match to a real fragment definition:

str.match(/fragment\s+#{Regexp.escape(const_name)}\s+on\b/)

Tests

Adds test/test_parse_fragment_anchoring.rb:

  • ...UserFields alongside fragment UserFieldsExtended on User now surfaces a clean uninitialized constant UserFields ValidationError (previously: the TypeError crash above).
  • Two prefix-sharing inline fragments (UserFields / UserFieldsExtended), both spread, still resolve locally —
    anchoring isn't over-strict.

No behaviour change to anything pinned by the characterization tests (#81) — they remain green.

Relationship to other PRs

Pin current behavior of locally-defined named fragments (a
'fragment Name on Type' spread via '...Name' within the same document),
transitive local fragments, and the source_document vs sliced document
distinction. These were only exercised incidentally alongside constant-path
spreads; isolating them guards a future refactor of the fragment-resolution
step in parse.
The spread/definition disambiguation in #parse used an unanchored match,
str.match(/fragment\s*#{const_name}/), to decide whether a ...Name spread
referred to a named fragment defined in the same document. It matched too
eagerly: \s* allowed zero whitespace, and there was no boundary after the
name, so a ...UserFields spread was mistaken for a fragment UserFieldsExtended
on User definition and left unresolved (later crashing in sliced_definitions
with a TypeError, instead of resolving as a constant spread).

Anchor the match to /fragment\s+<Name>\s+on\b/ (mandatory surrounding
whitespace, escaped name, 'on' word boundary) so only a real
'fragment <Name> on Type' definition is treated as local; prefix-sharing
names no longer collide.

Adds test/test_parse_fragment_anchoring.rb (3 tests):
- a prefix spread is not swallowed by a longer fragment definition;
- prefix-sharing *inline* fragments both still resolve;
- a *constant* fragment spread coexists with a prefix-sharing inline
  fragment (the realistic migration/interop case).
Behaviour pinned by test_parse_fragment_characterization is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant