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

lib/graphql/client.rb

@@ -158,8 +158,17 @@ def parse(str, filename = nil, lineno = nil)
         match = Regexp.last_match
         const_name = match[1]
 
-        if str.match(/fragment\s*#{const_name}/)
-          # It's a fragment _definition_, not a fragment usage
+        if str.match(/fragment\s+#{Regexp.escape(const_name)}\s+on\b/)
+          # It's a named fragment _definition_ in this same document
+          # (`fragment Name on Type { ... }`), not a fragment spread that has
+          # to be resolved to a Ruby constant. Leave the spread untouched so
+          # graphql-ruby resolves it locally.
+          #
+          # The match is anchored to `fragment <Name> on` (mandatory whitespace
+          # on both sides, `on` word boundary) so that a spread whose name is a
+          # prefix of a definition's name — e.g. `...UserFields` alongside
+          # `fragment UserFieldsExtended on User` — is NOT mistaken for that
+          # definition and is still resolved as a constant spread.
           match[0]
         else
           # It's a fragment spread, so we should load the fragment

test/test_parse_fragment_anchoring.rb

@@ -0,0 +1,135 @@
+# frozen_string_literal: true
+require "graphql"
+require "graphql/client"
+require "minitest/autorun"
+
+# Regression tests for anchoring the fragment-_definition_ detection in `Client#parse`.
+#
+# When `#parse` walks `...Name` spreads it must decide, for each spread, whether `Name`
+# refers to a named fragment _defined in the same document_ (leave it untouched) or to a
+# Ruby constant that has to be resolved. That decision used to be made with the unanchored
+# `/fragment\s*#{const_name}/`, which matched too eagerly:
+#
+#   * `\s*` allowed zero whitespace (`fragmentName` would match), and
+#   * there was no boundary after the name, so `...UserFields` matched a
+#     `fragment UserFieldsExtended on User` definition.
+#
+# The match is now anchored to `fragment <Name> on` with mandatory surrounding whitespace.
+class TestParseFragmentAnchoring < Minitest::Test
+  class UserType < GraphQL::Schema::Object
+    field :id, ID, null: false
+    field :name, String, null: false
+  end
+
+  class QueryType < GraphQL::Schema::Object
+    field :user, UserType, null: true do
+      argument :id, ID, required: true
+    end
+  end
+
+  class Schema < GraphQL::Schema
+    query(QueryType)
+  end
+
+  module Temp
+  end
+
+  def setup
+    @client = GraphQL::Client.new(schema: Schema)
+  end
+
+  def teardown
+    Temp.constants.each { |sym| Temp.send(:remove_const, sym) }
+  end
+
+  # A spread whose name is a PREFIX of an inline fragment's name must NOT be mistaken for
+  # that fragment's definition. `...UserFields` has no `fragment UserFields on ...` and no
+  # matching constant, so it has to surface as an unresolved constant — not be silently
+  # left in place by colliding with `fragment UserFieldsExtended on User`.
+  def test_prefix_spread_is_not_swallowed_by_a_longer_fragment_definition
+    error = assert_raises(GraphQL::Client::ValidationError) do
+      @client.parse(<<-'GRAPHQL')
+        query Profile {
+          user(id: 4) {
+            ...UserFields
+            ...UserFieldsExtended
+          }
+        }
+
+        fragment UserFieldsExtended on User {
+          id
+          name
+        }
+      GRAPHQL
+    end
+
+    assert_match(/uninitialized constant UserFields/, error.message)
+  end
+
+  # The anchoring must not be too strict: when both a short and a longer (prefix-sharing)
+  # named fragment are defined inline and both are spread, both resolve locally with no
+  # error and both definitions are carried in the document.
+  def test_prefix_sharing_inline_fragments_both_resolve
+    Temp.const_set :Doc, @client.parse(<<-'GRAPHQL')
+      query Profile {
+        user(id: 4) {
+          ...UserFields
+          ...UserFieldsExtended
+        }
+      }
+
+      fragment UserFields on User {
+        id
+      }
+
+      fragment UserFieldsExtended on User {
+        id
+        name
+      }
+    GRAPHQL
+
+    query_string = Temp::Doc::Profile.document.to_query_string
+    assert_includes query_string, "fragment TestParseFragmentAnchoring__Temp__Doc__UserFields on User"
+    assert_includes query_string, "fragment TestParseFragmentAnchoring__Temp__Doc__UserFieldsExtended on User"
+    assert_includes query_string, "...TestParseFragmentAnchoring__Temp__Doc__UserFields"
+    assert_includes query_string, "...TestParseFragmentAnchoring__Temp__Doc__UserFieldsExtended"
+  end
+
+  # The realistic interop case: a pre-existing *constant* fragment spread coexisting in the
+  # SAME document with a newly-added *inline* named fragment whose name shares its prefix.
+  # This is exactly where the unanchored match silently broke — the constant spread
+  # `...ReuseFields` was mistaken for the inline `fragment ReuseFieldsFull on User`
+  # definition, so the constant was never resolved (and parsing later crashed). Both styles
+  # must now resolve side by side.
+  def test_constant_spread_and_prefix_sharing_inline_fragment_coexist
+    Object.const_set :ReuseFields, @client.parse(<<-'GRAPHQL')
+      fragment on User {
+        id
+      }
+    GRAPHQL
+
+    Temp.const_set :Doc, @client.parse(<<-'GRAPHQL')
+      query Profile {
+        user(id: 4) {
+          ...ReuseFields
+          ...ReuseFieldsFull
+        }
+      }
+
+      fragment ReuseFieldsFull on User {
+        id
+        name
+      }
+    GRAPHQL
+
+    query_string = Temp::Doc::Profile.document.to_query_string
+    # the constant spread resolved to the top-level constant's fragment (kept bare name)
+    assert_includes query_string, "fragment ReuseFields on User"
+    assert_match(/\.\.\.ReuseFields\b/, query_string)
+    # the prefix-sharing inline fragment resolved locally, renamed under the document path
+    assert_includes query_string, "fragment TestParseFragmentAnchoring__Temp__Doc__ReuseFieldsFull on User"
+    assert_includes query_string, "...TestParseFragmentAnchoring__Temp__Doc__ReuseFieldsFull"
+  ensure
+    Object.send(:remove_const, :ReuseFields) if Object.const_defined?(:ReuseFields)
+  end
+end

test/test_parse_fragment_characterization.rb

@@ -0,0 +1,139 @@
+# frozen_string_literal: true
+require "graphql"
+require "graphql/client"
+require "minitest/autorun"
+
+# Characterization tests pinning the CURRENT behavior of how Client#parse handles
+# *inline named fragments* (a `fragment Name on Type { ... }` definition that lives in
+# the same document as the operation that spreads `...Name`), as opposed to fragments
+# bound to Ruby constants and spread by their constant path.
+#
+# These behaviors are exercised today only incidentally (the existing
+# `test_client_parse_query_fragment_document` mixes a local spread with a constant
+# spread). They are isolated here so that a future refactor of the fragment-resolution
+# step in `Client#parse` cannot change them silently.
+class TestParseFragmentCharacterization < Minitest::Test
+  class UserType < GraphQL::Schema::Object
+    field :id, ID, null: false
+    field :name, String, null: false
+    field :login, String, null: false
+    field :friends, "[TestParseFragmentCharacterization::UserType]", null: false do
+      argument :first, Int, required: false
+    end
+  end
+
+  class QueryType < GraphQL::Schema::Object
+    field :viewer, UserType, null: false
+    field :user, UserType, null: true do
+      argument :id, ID, required: true
+    end
+  end
+
+  class Schema < GraphQL::Schema
+    query(QueryType)
+  end
+
+  module Temp
+  end
+
+  def setup
+    @client = GraphQL::Client.new(schema: Schema)
+    @client.document_tracking_enabled = true
+  end
+
+  def teardown
+    Temp.constants.each { |sym| Temp.send(:remove_const, sym) }
+  end
+
+  # A `...Name` spread that refers to a fragment defined in the SAME document — with no
+  # Ruby constant named `Name` anywhere — is resolved locally: the fragment definition is
+  # kept and renamed with the document's constant path, and the spread is rewritten to the
+  # renamed name. No ValidationError is raised and no constant lookup is required.
+  def test_local_named_fragment_spread_resolves_without_a_constant
+    Temp.const_set :Doc, @client.parse(<<-'GRAPHQL')
+      query Profile {
+        user(id: 4) {
+          ...UserFields
+        }
+      }
+
+      fragment UserFields on User {
+        id
+        name
+      }
+    GRAPHQL
+
+    op = Temp::Doc::Profile
+    refute_nil op
+    query_string = op.document.to_query_string
+
+    assert_includes query_string, "query TestParseFragmentCharacterization__Temp__Doc__Profile"
+    assert_includes query_string, "fragment TestParseFragmentCharacterization__Temp__Doc__UserFields on User"
+    # the spread points at the renamed local fragment, not a bare `...UserFields`
+    assert_includes query_string, "...TestParseFragmentCharacterization__Temp__Doc__UserFields"
+    refute_match(/\.\.\.UserFields\b/, query_string)
+  end
+
+  # Transitive local fragments (a local fragment that itself spreads another local
+  # fragment) are all resolved and renamed locally, and the operation's sliced document
+  # carries the full transitive dependency chain.
+  def test_transitive_local_named_fragments_resolve
+    Temp.const_set :Doc, @client.parse(<<-'GRAPHQL')
+      query Profile {
+        user(id: 4) {
+          ...Outer
+        }
+      }
+
+      fragment Outer on User {
+        id
+        ...Inner
+      }
+
+      fragment Inner on User {
+        name
+      }
+    GRAPHQL
+
+    document = Temp::Doc::Profile.document
+    # operation + both transitively-required fragments
+    assert_equal 3, document.definitions.size
+    query_string = document.to_query_string
+    assert_includes query_string, "...TestParseFragmentCharacterization__Temp__Doc__Outer"
+    assert_includes query_string, "...TestParseFragmentCharacterization__Temp__Doc__Inner"
+    assert_includes query_string, "fragment TestParseFragmentCharacterization__Temp__Doc__Inner on User"
+  end
+
+  # For a document with multiple independent operations, each operation's `document` is
+  # SLICED to just that operation plus its own fragment dependencies, while
+  # `source_document` retains every sibling definition from the original parse.
+  def test_source_document_retains_siblings_while_document_is_sliced
+    Temp.const_set :Doc, @client.parse(<<-'GRAPHQL')
+      query A {
+        user(id: 4) {
+          ...FieldsA
+        }
+      }
+
+      query B {
+        viewer {
+          ...FieldsB
+        }
+      }
+
+      fragment FieldsA on User {
+        id
+      }
+
+      fragment FieldsB on User {
+        name
+      }
+    GRAPHQL
+
+    a = Temp::Doc::A
+    # sliced: query A + fragment FieldsA only
+    assert_equal 2, a.document.definitions.size
+    # source: all four definitions from the original document
+    assert_equal 4, a.source_document.definitions.size
+  end
+end