diff --git a/lib/graphql/client.rb b/lib/graphql/client.rb index dcc4b0c..862c8e6 100644 --- a/lib/graphql/client.rb +++ b/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 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 diff --git a/test/test_parse_fragment_anchoring.rb b/test/test_parse_fragment_anchoring.rb new file mode 100644 index 0000000..079fd5c --- /dev/null +++ b/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 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 diff --git a/test/test_parse_fragment_characterization.rb b/test/test_parse_fragment_characterization.rb new file mode 100644 index 0000000..78d4ab8 --- /dev/null +++ b/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