diff --git a/README.md b/README.md index 12acaac3..2888dd5a 100644 --- a/README.md +++ b/README.md @@ -153,7 +153,7 @@ type Query { * The `@stitch` directive is applied to a root query where the merged type may be accessed. The merged type identity is inferred from the field return. * The `key: "id"` parameter indicates that an `{ id }` must be selected from prior locations so it may be submitted as an argument to this query. The query argument used to send the key is inferred when possible ([more on arguments](#multiple-query-arguments) later). -Each location that provides a unique variant of a type must provide at least one resolver query for the type. The exception to this requirement are [foreign key types](./docs/mechanics.md##modeling-foreign-keys-for-stitching) that contain only a single key field: +Each location that provides a unique variant of a type must provide at least one resolver query for the type. The exception to this requirement are [outbound-only types](./docs/mechanics.md#outbound-only-merged-types) and/or [foreign key types](./docs/mechanics.md##modeling-foreign-keys-for-stitching) that contain no exclusive data: ```graphql type Product { @@ -161,7 +161,7 @@ type Product { } ``` -The above representation of a `Product` type provides no unique data beyond a key that is available in other locations. Thus, this representation will never require an inbound request to fetch it, and its resolver query may be omitted. +The above representation of a `Product` type contains nothing but a key that is available in other locations. Therefore, this representation will never require an inbound request to fetch it, and its resolver query may be omitted. #### List queries diff --git a/docs/mechanics.md b/docs/mechanics.md index 35db9ef5..76420e17 100644 --- a/docs/mechanics.md +++ b/docs/mechanics.md @@ -303,3 +303,45 @@ And produces this result: ``` Location B is allowed to return `null` here because its one unique field, `rating`, is nullable (the `id` field can be provided by Location A). If `rating` were non-null, then null bubbling would invalidate the response data. + +### Outbound-only merged types + +Merged types do not always require a resolver query. For example: + +```graphql +# -- Location A + +type Widget { + id: ID! + name: String +} + +type Query { + widgetA(id: ID!): Widget @stitch(key: "id") +} + +# -- Location B + +type Widget { + id: ID! + size: Float +} + +type Query { + widgetB(id: ID!): Widget @stitch(key: "id") +} + +# -- Location C + +type Widget { + id: ID! + name: String + size: Float +} + +type Query { + featuredWidget: Widget +} +``` + +In this graph, `Widget` is a merged type without a resolver query in location C. This works because all of its fields are resolvable in other locations; that means location C can provide outbound representations of this type without ever needing to resolve inbound requests for it. Outbound types do still require a key field (such as `id` above) that allow them to join with data in other resolver locations. diff --git a/lib/graphql/stitching/composer.rb b/lib/graphql/stitching/composer.rb index ae2db7ae..d16f5788 100644 --- a/lib/graphql/stitching/composer.rb +++ b/lib/graphql/stitching/composer.rb @@ -68,6 +68,7 @@ def initialize( @boundary_map = nil @mapped_type_names = nil @candidate_directives_by_name_and_location = nil + @candidate_types_by_name_and_location = nil @schema_directives = nil end diff --git a/lib/graphql/stitching/composer/validate_boundaries.rb b/lib/graphql/stitching/composer/validate_boundaries.rb index b4eaa332..8feb379c 100644 --- a/lib/graphql/stitching/composer/validate_boundaries.rb +++ b/lib/graphql/stitching/composer/validate_boundaries.rb @@ -4,29 +4,29 @@ module GraphQL::Stitching class Composer class ValidateBoundaries < BaseValidator - def perform(ctx, composer) - ctx.schema.types.each do |type_name, type| + def perform(supergraph, composer) + supergraph.schema.types.each do |type_name, type| # objects and interfaces that are not the root operation types next unless type.kind.object? || type.kind.interface? - next if ctx.schema.query == type || ctx.schema.mutation == type + next if supergraph.schema.query == type || supergraph.schema.mutation == type next if type.graphql_name.start_with?("__") # multiple subschemas implement the type candidate_types_by_location = composer.candidate_types_by_name_and_location[type_name] next unless candidate_types_by_location.length > 1 - boundaries = ctx.boundaries[type_name] + boundaries = supergraph.boundaries[type_name] if boundaries&.any? - validate_as_boundary(ctx, type, candidate_types_by_location, boundaries) + validate_as_boundary(supergraph, type, candidate_types_by_location, boundaries) elsif type.kind.object? - validate_as_shared(ctx, type, candidate_types_by_location) + validate_as_shared(supergraph, type, candidate_types_by_location) end end end private - def validate_as_boundary(ctx, type, candidate_types_by_location, boundaries) + def validate_as_boundary(supergraph, type, candidate_types_by_location, boundaries) # abstract boundaries are expanded with their concrete implementations, which each get validated. Ignore the abstract itself. return if type.kind.abstract? @@ -41,25 +41,30 @@ def validate_as_boundary(ctx, type, candidate_types_by_location, boundaries) memo[boundary.location][boundary.key] = boundary end - boundary_keys = boundaries.map { _1.key }.uniq - key_only_types_by_location = candidate_types_by_location.select do |location, subschema_type| - subschema_type.fields.keys.length == 1 && boundary_keys.include?(subschema_type.fields.keys.first) - end + boundary_keys = boundaries.map(&:key).to_set + + # All non-key fields must be resolvable in at least one boundary location + supergraph.locations_by_type_and_field[type.graphql_name].each do |field_name, locations| + next if boundary_keys.include?(field_name) - # all locations have a boundary, or else are key-only - candidate_types_by_location.each do |location, subschema_type| - unless boundaries_by_location_and_key[location] || key_only_types_by_location[location] - raise Composer::ValidationError, "A boundary query is required for `#{type.graphql_name}` in #{location} because it provides unique fields." + if locations.none? { boundaries_by_location_and_key[_1] } + where = locations.length > 1 ? "one of #{locations.join(", ")} locations" : locations.first + raise Composer::ValidationError, "A boundary query is required for `#{type.graphql_name}` in #{where} to resolve field `#{field_name}`." end end - outbound_access_locations = key_only_types_by_location.keys - bidirectional_access_locations = candidate_types_by_location.keys - outbound_access_locations + # All locations of a boundary type must include at least one key field + supergraph.fields_by_type_and_location[type.graphql_name].each do |location, field_names| + if field_names.none? { boundary_keys.include?(_1) } + raise Composer::ValidationError, "A boundary key is required for `#{type.graphql_name}` in #{location} to join with other locations." + end + end # verify that all outbound locations can access all inbound locations - (outbound_access_locations + bidirectional_access_locations).each do |location| - remote_locations = bidirectional_access_locations.reject { _1 == location } - paths = ctx.route_type_to_locations(type.graphql_name, location, remote_locations) + resolver_locations = boundaries_by_location_and_key.keys + candidate_types_by_location.each_key do |location| + remote_locations = resolver_locations.reject { _1 == location } + paths = supergraph.route_type_to_locations(type.graphql_name, location, remote_locations) if paths.length != remote_locations.length || paths.any? { |_loc, path| path.nil? } raise Composer::ValidationError, "Cannot route `#{type.graphql_name}` boundaries in #{location} to all other locations. "\ "All locations must provide a boundary accessor that uses a conjoining key." @@ -67,7 +72,7 @@ def validate_as_boundary(ctx, type, candidate_types_by_location, boundaries) end end - def validate_as_shared(ctx, type, candidate_types_by_location) + def validate_as_shared(supergraph, type, candidate_types_by_location) expected_fields = begin type.fields.keys.sort rescue StandardError => e @@ -79,8 +84,8 @@ def validate_as_shared(ctx, type, candidate_types_by_location) end end - candidate_types_by_location.each do |location, subschema_type| - if subschema_type.fields.keys.sort != expected_fields + candidate_types_by_location.each do |location, candidate_type| + if candidate_type.fields.keys.sort != expected_fields raise Composer::ValidationError, "Shared type `#{type.graphql_name}` must have consistent fields across locations, "\ "or else define boundary queries so that its unique fields may be accessed remotely." end diff --git a/lib/graphql/stitching/version.rb b/lib/graphql/stitching/version.rb index a2a3c95f..0119e55a 100644 --- a/lib/graphql/stitching/version.rb +++ b/lib/graphql/stitching/version.rb @@ -2,6 +2,6 @@ module GraphQL module Stitching - VERSION = "1.2.4" + VERSION = "1.2.5" end end diff --git a/test/graphql/stitching/composer/validate_boundaries_test.rb b/test/graphql/stitching/composer/validate_boundaries_test.rb index 5ee663ef..837f30b3 100644 --- a/test/graphql/stitching/composer/validate_boundaries_test.rb +++ b/test/graphql/stitching/composer/validate_boundaries_test.rb @@ -40,7 +40,7 @@ def test_permits_multiple_boundary_query_keys_per_type_location assert compose_definitions({ "a" => a, "b" => b, "c" => c }) end - def test_validates_at_least_one_boundary_per_type_location + def test_validates_boundary_present_when_providing_unique_fields a = %{type T { id:ID! name:String } type Query { a(id: ID!):T @stitch(key: "id") }} b = %{type T { id:ID! size:Float } type Query { b:T }} @@ -49,6 +49,24 @@ def test_validates_at_least_one_boundary_per_type_location end end + def test_validates_boundary_present_in_multiple_locations_when_providing_unique_fields + a = %{type T { id:ID! name:String } type Query { a(id: ID!):T @stitch(key: "id") }} + b = %{type T { id:ID! size:Float } type Query { b:T }} + c = %{type T { id:ID! size:Float } type Query { c:T }} + + assert_error("A boundary query is required for `T` in one of b, c locations", ValidationError) do + compose_definitions({ "a" => a, "b" => b, "c" => c }) + end + end + + def test_permits_no_boundary_query_for_types_that_can_be_fully_resolved_elsewhere + a = %{type T { id:ID! name:String } type Query { a(id: ID!):T @stitch(key: "id") }} + b = %{type T { id:ID! size:Float } type Query { b(id: ID!):T @stitch(key: "id") }} + c = %{type T { id:ID! size:Float name:String } type Query { c:T }} + + assert compose_definitions({ "a" => a, "b" => b, "c" => c }) + end + def test_permits_no_boundary_query_for_key_only_types a = %{type T { id:ID! name:String } type Query { a(id: ID!):T @stitch(key: "id") }} b = %{type T { id:ID! } type Query { b:T }} @@ -56,6 +74,15 @@ def test_permits_no_boundary_query_for_key_only_types assert compose_definitions({ "a" => a, "b" => b }) end + def test_validates_subset_types_have_a_key + a = %{type T { id:ID! name:String } type Query { a(id: ID!):T @stitch(key: "id") }} + b = %{type T { name:String } type Query { b:T }} + + assert_error("A boundary key is required for `T` in b", ValidationError) do + compose_definitions({ "a" => a, "b" => b }) + end + end + def test_validates_bidirection_types_are_mutually_accessible a = %{ type T { upc:ID! name:String } @@ -75,6 +102,25 @@ def test_validates_bidirection_types_are_mutually_accessible end end + def test_validates_key_only_types_are_mutually_accessible + a = %| + type T { upc:ID! } + type Query { a(upc:ID!):T @stitch(key: "upc") } + | + b = %| + type T { id:ID! } + type Query { b(id:ID!):T @stitch(key: "id") } + | + c = %| + type T { id:ID! } + type Query { c(id:ID!):T @stitch(key: "id") } + | + + assert_error("Cannot route `T` boundaries in a", ValidationError) do + compose_definitions({ "a" => a, "b" => b, "c" => c }) + end + end + def test_validates_outbound_types_can_access_all_bidirection_types a = %{ type T { upc:ID! } diff --git a/test/graphql/stitching/supergraph_test.rb b/test/graphql/stitching/supergraph_test.rb index 72a05ceb..b25cd7f6 100644 --- a/test/graphql/stitching/supergraph_test.rb +++ b/test/graphql/stitching/supergraph_test.rb @@ -290,27 +290,6 @@ def test_route_type_to_locations_favors_longer_paths_through_necessary_locations assert routes.none? { |_key, path| path.any? { _1["location"] == "e" } } end - def test_route_type_to_locations_returns_nil_for_unreachable_locations - a = %| - type T { upc:ID! } - type Query { a(upc:ID!):T @stitch(key: "upc") } - | - b = %| - type T { id:ID! } - type Query { b(id:ID!):T @stitch(key: "id") } - | - c = %| - type T { id:ID! } - type Query { c(id:ID!):T @stitch(key: "id") } - | - - supergraph = compose_definitions({ "a" => a, "b" => b, "c" => c }) - - routes = supergraph.route_type_to_locations("T", "b", ["a", "c"]) - assert_equal ["c"], routes["c"].map { _1["location"] } - assert_nil routes["a"] - end - describe "#to_definition / #from_definition" do def setup alpha = %|