diff --git a/apollo-federation/src/sources/connect/expand/mod.rs b/apollo-federation/src/sources/connect/expand/mod.rs index d36a9f438d..ef998c6028 100644 --- a/apollo-federation/src/sources/connect/expand/mod.rs +++ b/apollo-federation/src/sources/connect/expand/mod.rs @@ -186,6 +186,7 @@ mod helpers { use apollo_compiler::schema::ScalarType; use apollo_compiler::Name; use apollo_compiler::Node; + use apollo_compiler::Schema; use indexmap::IndexMap; use indexmap::IndexSet; use itertools::Itertools; @@ -199,9 +200,11 @@ mod helpers { use crate::error::FederationError; use crate::link::spec::Identity; use crate::link::Link; + use crate::schema::position::InterfaceFieldDefinitionPosition; use crate::schema::position::ObjectFieldDefinitionPosition; use crate::schema::position::ObjectOrInterfaceTypeDefinitionPosition; use crate::schema::position::ObjectTypeDefinitionPosition; + use crate::schema::position::PositionLookupError; use crate::schema::position::SchemaRootDefinitionKind; use crate::schema::position::SchemaRootDefinitionPosition; use crate::schema::position::TypeDefinitionPosition; @@ -453,119 +456,116 @@ mod helpers { // The actual selection might make use of the $this variable, so we grab them too let selection_parameters = extract_params_from_selection(&connector.selection); + let (key_for_type, var_filter) = + if matches!(connector.entity_resolver, Some(EntityResolver::Explicit)) { + // `entity: true` connectors only exist on Query. + // We don't generate keys for `Query`, these are keys for the output type of the field. + // Therefore, we're also only considering the `$args` fields as keys, which should + // map 1-1 with output type fields. + (output_type, VariableType::Args) + } else { + // We're extending an entity by adding a new field dependent on some other fields + // (identified by `$this`). + (parent_type, VariableType::This) + }; + // We'll need to collect all synthesized keys for the output type, adding a federation // `@key` directive once completed. let mut keys = Vec::new(); - for Variable { var_type, path, .. } in body_parameters + for Variable { path, .. } in body_parameters .into_iter() .chain(url_parameters) .chain(selection_parameters) .unique() + .filter(|var| var.var_type == var_filter) { - match var_type { - // Arguments should be added to the synthesized key, since they are mandatory - // to resolving the output type. The synthesized key should only include the portions - // of the inputs actually used throughout the selections of the transport. - // - // Note that this only applies to connectors marked as an entity resolver, since only - // those should be allowed to fully resolve a type given the required arguments / - // synthesized keys. - VariableType::Args => { - // Synthesize the key based on the argument. Note that this is only relevant in the - // argument case when the connector is marked as being an entity resolved. - if matches!(connector.entity_resolver, Some(EntityResolver::Explicit)) { - let (field, selection) = path_to_selection(&path); - keys.push(format!("{field}{selection}")); - } + // Arguments should be added to the synthesized key, since they are mandatory + // to resolving the output type. The synthesized key should only include the portions + // of the inputs actually used throughout the selections of the transport. + // + // Note that this only applies to connectors marked as an entity resolver, since only + // those should be allowed to fully resolve a type given the required arguments / + // synthesized keys. + // + // All sibling fields marked by $this in a transport must be carried over to the output type + // regardless of its use in the output selection. + let (field_name_str, selection) = path_to_selection(&path); + let field_name = Name::new(&field_name_str)?; + let field: Box = match &key_for_type { + TypeDefinitionPosition::Object(o) => Box::new(o.field(field_name)), + TypeDefinitionPosition::Interface(i) => Box::new(i.field(field_name)), + TypeDefinitionPosition::Union(_) | TypeDefinitionPosition::InputObject(_)=> { + return Err(FederationError::internal( + "siblings of type interface, input object, or union are not yet handled", + )) + } + other => { + return Err(FederationError::internal(format!( + "cannot select a sibling on a leaf type: {}", + other.type_name() + ))) } + }; - VariableType::Config => {} // Expansion doesn't care about config - - // All sibling fields marked by $this in a transport must be carried over to the output type - // regardless of its use in the output selection. - VariableType::This => { - match parent_type { - TypeDefinitionPosition::Object(ref o) => { - let (field, selection) = path_to_selection(&path); - let field_name = Name::new(&field)?; - let field = o.field(field_name.clone()); - let field_def = field.get(self.original_schema.schema())?; - - // Mark it as a required key for the output type - if !selection.is_empty() { - // We'll also need to carry over the output type of this sibling if there is a sub - // selection. - let field_output = field_def.ty.inner_named_type(); - if to_schema.try_get_type(field_output.clone()).is_none() { - // We use a fake JSONSelection here so that we can reuse the visitor - // when generating the output types and their required members. - let visitor = SchemaVisitor::new( - self.original_schema, - to_schema, - &self.directive_deny_list, - ); - let (_, parsed) = - JSONSelection::parse(&selection).map_err(|e| { - FederationError::internal(format!("could not parse fake selection for sibling field: {e}")) - })?; - - let output_type = match self - .original_schema - .get_type(field_output.clone())? - { - TypeDefinitionPosition::Object(object) => object, - - other => { - return Err(FederationError::internal(format!("connector output types currently only support object types: found {}", other.type_name()))) - } - }; - - visitor.walk(( - output_type, - parsed.next_subselection().cloned().ok_or( - FederationError::internal( - "empty selections are not allowed", - ), - )?, - ))?; - } - } - - keys.push(format!("{field_name}{selection}")); - - // Add the field if not already present in the output schema - if field.try_get(to_schema.schema()).is_none() { - field.insert( - to_schema, - Component::new(FieldDefinition { - description: field_def.description.clone(), - name: field_def.name.clone(), - arguments: field_def.arguments.clone(), - ty: field_def.ty.clone(), - directives: filter_directives( - &self.directive_deny_list, - &field_def.directives, - ), - }), - )?; - } - } - TypeDefinitionPosition::Interface(_) - | TypeDefinitionPosition::Union(_) | TypeDefinitionPosition::InputObject(_)=> { - return Err(FederationError::internal( - "siblings of type interface, input object, or union are not yet handled", - )) - } + let field_def = field.get(self.original_schema.schema())?; + + // Mark it as a required key for the output type + if !selection.is_empty() { + // We'll also need to carry over the output type of this sibling if there is a sub + // selection. + let field_output = field_def.ty.inner_named_type(); + if to_schema.try_get_type(field_output.clone()).is_none() { + // We use a fake JSONSelection here so that we can reuse the visitor + // when generating the output types and their required members. + let visitor = SchemaVisitor::new( + self.original_schema, + to_schema, + &self.directive_deny_list, + ); + let (_, parsed) = JSONSelection::parse(&selection).map_err(|e| { + FederationError::internal(format!( + "could not parse fake selection for sibling field: {e}" + )) + })?; + + let output_type = match self + .original_schema + .get_type(field_output.clone())? + { + TypeDefinitionPosition::Object(object) => object, other => { - return Err(FederationError::internal(format!( - "cannot select a sibling on a leaf type: {}", - other.type_name() - ))) + return Err(FederationError::internal(format!("connector output types currently only support object types: found {}", other.type_name()))) } }; + + visitor.walk(( + output_type, + parsed.next_subselection().cloned().ok_or( + FederationError::internal("empty selections are not allowed"), + )?, + ))?; } } + + keys.push(format!("{field_name_str}{selection}")); + + // Add the field if not already present in the output schema + if field.get(to_schema.schema()).is_err() { + field.insert( + to_schema, + Component::new(FieldDefinition { + description: field_def.description.clone(), + name: field_def.name.clone(), + arguments: field_def.arguments.clone(), + ty: field_def.ty.clone(), + directives: filter_directives( + &self.directive_deny_list, + &field_def.directives, + ), + }), + )?; + } } // If we have marked keys as being necessary for this output type, add them as an `@key` @@ -579,12 +579,6 @@ mod helpers { })], }; - let key_for_type = - if matches!(connector.entity_resolver, Some(EntityResolver::Explicit)) { - output_type - } else { - parent_type - }; match key_for_type { TypeDefinitionPosition::Object(o) => { o.insert_directive(to_schema, Component::new(key_directive)) @@ -789,6 +783,54 @@ mod helpers { }) }) } + + // TODO: contribute some code to `position.rs` to make those types more flexible rather than adding it here + trait Field { + fn get<'schema>( + &self, + schema: &'schema Schema, + ) -> Result<&'schema Component, PositionLookupError>; + + fn insert( + &self, + schema: &mut FederationSchema, + field: Component, + ) -> Result<(), FederationError>; + } + + impl Field for ObjectFieldDefinitionPosition { + fn get<'schema>( + &self, + schema: &'schema Schema, + ) -> Result<&'schema Component, PositionLookupError> { + self.get(schema) + } + + fn insert( + &self, + schema: &mut FederationSchema, + field: Component, + ) -> Result<(), FederationError> { + self.insert(schema, field) + } + } + + impl Field for InterfaceFieldDefinitionPosition { + fn get<'schema>( + &self, + schema: &'schema Schema, + ) -> Result<&'schema Component, PositionLookupError> { + self.get(schema) + } + + fn insert( + &self, + schema: &mut FederationSchema, + field: Component, + ) -> Result<(), FederationError> { + self.insert(schema, field) + } + } } /// Turn a path like a.b.c into a selection like (a, { b { c } }). Join together to get a key. diff --git a/apollo-federation/src/sources/connect/expand/tests/schemas/expand/keys.graphql b/apollo-federation/src/sources/connect/expand/tests/schemas/expand/keys.graphql index 477d52282b..c4928c19cb 100644 --- a/apollo-federation/src/sources/connect/expand/tests/schemas/expand/keys.graphql +++ b/apollo-federation/src/sources/connect/expand/tests/schemas/expand/keys.graphql @@ -57,8 +57,11 @@ enum link__Purpose { type Query @join__type(graph: ONE) { - t(id: ID!): T @join__directive(graphs: [ONE], name: "connect", args: {http: {GET: "http://localhost/ts/{$args.id}"}, selection: "id id2", entity: true}) - t2(id: ID!, id2: ID!): T @join__directive(graphs: [ONE], name: "connect", args: {http: {GET: "http://localhost/ts/{$args.id}?id2={$args.id2}"}, selection: "id id2", entity: true}) + t(id: ID!): T @join__directive(graphs: [ONE], name: "connect", args: {http: {GET: "http://localhost/ts/{$args.id}"}, selection: "id id2 unselected", entity: true}) + t2(id: ID!, id2: ID!): T @join__directive(graphs: [ONE], name: "connect", args: {http: {GET: "http://localhost/ts/{$args.id}?id2={$args.id2}"}, selection: "id id2 unselected", entity: true}) + + """ Uses the `unselected` field as a key, but doesn't select it """ + unselected(unselected: ID!): T @join__directive(graphs: [ONE], name: "connect", args: {http: {GET: "http://localhost/ts/{$args.unselected}"}, selection: "id id2 accessibleByUnselected", entity: true}) } type R @@ -71,9 +74,12 @@ type R type T @join__type(graph: ONE, key: "id") @join__type(graph: ONE, key: "id id2") + @join__type(graph: ONE, key: "unselected") { id: ID! id2: ID! + unselected: ID! + accessibleByUnselected: ID! r1: R @join__directive(graphs: [ONE], name: "connect", args: {http: {GET: "http://localhost/rs/{$this.id}"}, selection: "id id2"}) r2: R @join__directive(graphs: [ONE], name: "connect", args: {http: {GET: "http://localhost/rs/{$this.id}?id2={$this.id2}"}, selection: "id id2"}) r3: R @join__directive(graphs: [ONE], name: "connect", args: {http: {GET: "http://localhost/rs/{$this.id}"}, selection: "id id2: $this.id2"}) diff --git a/apollo-federation/src/sources/connect/expand/tests/schemas/expand/keys.yaml b/apollo-federation/src/sources/connect/expand/tests/schemas/expand/keys.yaml index b9c643b4d4..aa80b407e4 100644 --- a/apollo-federation/src/sources/connect/expand/tests/schemas/expand/keys.yaml +++ b/apollo-federation/src/sources/connect/expand/tests/schemas/expand/keys.yaml @@ -4,25 +4,34 @@ subgraphs: schema: sdl: | extend schema - @link(url: "https://specs.apollo.dev/federation/v2.8", import: ["@key"]) + @link(url: "https://specs.apollo.dev/federation/v2.10", import: ["@key"]) @link(url: "https://specs.apollo.dev/connect/v0.1", import: ["@connect"]) type Query { t(id: ID!): T @connect( # expect `key: "id"` http: { GET: "http://localhost/ts/{$$args.id}" } - selection: "id id2" + selection: "id id2 unselected" entity: true ) - t2(id: ID! id2: ID!): T + t2(id: ID!, id2: ID!): T @connect( # expect `key: "id id2"` http: { GET: "http://localhost/ts/{$$args.id}?id2={$$args.id2}" } - selection: "id id2" + selection: "id id2 unselected" + entity: true + ) + """ Uses the `unselected` field as a key, but doesn't select it """ + unselected(unselected: ID!): T + @connect( + http: { GET: "http://localhost/ts/{$$args.unselected}" } + selection: "id id2 accessibleByUnselected" entity: true ) } - type T @key(fields: "id") @key(fields: "id id2") { + type T @key(fields: "id") @key(fields: "id id2") @key(fields: "unselected") { id: ID! id2: ID! + unselected: ID! + accessibleByUnselected: ID! r1: R @connect(http: { GET: "http://localhost/rs/{$$this.id}" }, selection: "id id2") # expect `key: "id"`` r2: R @connect(http: { GET: "http://localhost/rs/{$$this.id}?id2={$$this.id2}" }, selection: "id id2") # expect `key: "id id2"` r3: R @connect(http: { GET: "http://localhost/rs/{$$this.id}" }, selection: "id id2: $$this.id2") # expect `key: "id id2"` diff --git a/apollo-federation/src/sources/connect/expand/tests/schemas/regenerate.sh b/apollo-federation/src/sources/connect/expand/tests/schemas/regenerate.sh old mode 100644 new mode 100755 index 4c87cb9613..01bc3aca0b --- a/apollo-federation/src/sources/connect/expand/tests/schemas/regenerate.sh +++ b/apollo-federation/src/sources/connect/expand/tests/schemas/regenerate.sh @@ -1,8 +1,26 @@ +# Composes a single supergraph config file passed as an argument or all `.yaml` files in any subdirectories. +# For each supergraph config, outputs a `.graphql` file in the same directory. +# Optionally, you can set `FEDERATION_VERSION` to override the supergraph binary used set -euo pipefail -for supergraph_config in */*.yaml; do +if [ -z "${FEDERATION_VERSION:-}" ]; then + FEDERATION_VERSION="2.10.0-preview.0" +fi + +regenerate_graphql() { + local supergraph_config=$1 + local test_name test_name=$(basename "$supergraph_config" .yaml) + local dir_name dir_name=$(dirname "$supergraph_config") echo "Regenerating $dir_name/$test_name.graphql" - rover supergraph compose --federation-version "=2.10.0-preview.0" --config "$supergraph_config" > "$dir_name/$test_name.graphql" -done \ No newline at end of file + rover supergraph compose --federation-version "=$FEDERATION_VERSION" --config "$supergraph_config" > "$dir_name/$test_name.graphql" +} + +if [ -z "${1:-}" ]; then + for supergraph_config in */*.yaml; do + regenerate_graphql "$supergraph_config" + done +else + regenerate_graphql "$1" +fi \ No newline at end of file diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-2.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-2.snap index 66fa8d11b2..052acb9960 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-2.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-2.snap @@ -88,9 +88,21 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke }, None, ), + Field( + None, + WithRange { + node: Field( + "unselected", + ), + range: Some( + 7..17, + ), + }, + None, + ), ], range: Some( - 0..6, + 0..17, ), }, ), @@ -202,9 +214,129 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke }, None, ), + Field( + None, + WithRange { + node: Field( + "unselected", + ), + range: Some( + 7..17, + ), + }, + None, + ), ], range: Some( - 0..6, + 0..17, + ), + }, + ), + config: None, + max_requests: None, + entity_resolver: Some( + Explicit, + ), + }, + "one_Query_unselected_0": Connector { + id: ConnectId { + label: "one. http: GET http://localhost/ts/{$args.unselected}", + subgraph_name: "one", + source_name: None, + directive: ObjectOrInterfaceFieldDirectivePosition { + field: Object(Query.unselected), + directive_name: "connect", + directive_index: 0, + }, + }, + transport: HttpJsonTransport { + source_url: None, + connect_template: URLTemplate { + base: Some( + Url { + scheme: "http", + cannot_be_a_base: false, + username: "", + password: None, + host: Some( + Domain( + "localhost", + ), + ), + port: None, + path: "/", + query: None, + fragment: None, + }, + ), + path: [ + Component { + parts: [ + Text( + "ts", + ), + ], + }, + Component { + parts: [ + Var( + Variable { + var_type: Args, + path: "unselected", + location: 21..37, + }, + ), + ], + }, + ], + query: {}, + }, + method: Get, + headers: {}, + body: None, + }, + selection: Named( + SubSelection { + selections: [ + Field( + None, + WithRange { + node: Field( + "id", + ), + range: Some( + 0..2, + ), + }, + None, + ), + Field( + None, + WithRange { + node: Field( + "id2", + ), + range: Some( + 3..6, + ), + }, + None, + ), + Field( + None, + WithRange { + node: Field( + "accessibleByUnselected", + ), + range: Some( + 7..29, + ), + }, + None, + ), + ], + range: Some( + 0..29, ), }, ), diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-3.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-3.snap index 36e2f3f336..b791d51dba 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-3.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-3.snap @@ -43,6 +43,7 @@ scalar join__DirectiveArguments enum join__Graph { ONE_QUERY_T2_0 @join__graph(name: "one_Query_t2_0", url: "none") ONE_QUERY_T_0 @join__graph(name: "one_Query_t_0", url: "none") + ONE_QUERY_UNSELECTED_0 @join__graph(name: "one_Query_unselected_0", url: "none") ONE_T_R1_0 @join__graph(name: "one_T_r1_0", url: "none") ONE_T_R2_0 @join__graph(name: "one_T_r2_0", url: "none") ONE_T_R3_0 @join__graph(name: "one_T_r3_0", url: "none") @@ -50,9 +51,11 @@ enum join__Graph { ONE_T_R5_0 @join__graph(name: "one_T_r5_0", url: "none") } -type T @join__type(graph: ONE_QUERY_T2_0, key: "id id2") @join__type(graph: ONE_QUERY_T_0, key: "id") @join__type(graph: ONE_T_R1_0, key: "id") @join__type(graph: ONE_T_R2_0, key: "id id2") @join__type(graph: ONE_T_R3_0, key: "id id2") @join__type(graph: ONE_T_R4_0, key: "id") @join__type(graph: ONE_T_R5_0, key: "id id2") { - id: ID! @join__field(graph: ONE_QUERY_T2_0) @join__field(graph: ONE_QUERY_T_0) @join__field(graph: ONE_T_R1_0) @join__field(graph: ONE_T_R2_0) @join__field(graph: ONE_T_R3_0) @join__field(graph: ONE_T_R4_0) @join__field(graph: ONE_T_R5_0) - id2: ID! @join__field(graph: ONE_QUERY_T2_0) @join__field(graph: ONE_QUERY_T_0) @join__field(graph: ONE_T_R2_0) @join__field(graph: ONE_T_R3_0) @join__field(graph: ONE_T_R5_0) +type T @join__type(graph: ONE_QUERY_T2_0, key: "id id2") @join__type(graph: ONE_QUERY_T_0, key: "id") @join__type(graph: ONE_QUERY_UNSELECTED_0, key: "unselected") @join__type(graph: ONE_T_R1_0, key: "id") @join__type(graph: ONE_T_R2_0, key: "id id2") @join__type(graph: ONE_T_R3_0, key: "id id2") @join__type(graph: ONE_T_R4_0, key: "id") @join__type(graph: ONE_T_R5_0, key: "id id2") { + id: ID! @join__field(graph: ONE_QUERY_T2_0) @join__field(graph: ONE_QUERY_T_0) @join__field(graph: ONE_QUERY_UNSELECTED_0) @join__field(graph: ONE_T_R1_0) @join__field(graph: ONE_T_R2_0) @join__field(graph: ONE_T_R3_0) @join__field(graph: ONE_T_R4_0) @join__field(graph: ONE_T_R5_0) + id2: ID! @join__field(graph: ONE_QUERY_T2_0) @join__field(graph: ONE_QUERY_T_0) @join__field(graph: ONE_QUERY_UNSELECTED_0) @join__field(graph: ONE_T_R2_0) @join__field(graph: ONE_T_R3_0) @join__field(graph: ONE_T_R5_0) + unselected: ID! @join__field(graph: ONE_QUERY_T2_0) @join__field(graph: ONE_QUERY_T_0) @join__field(graph: ONE_QUERY_UNSELECTED_0) + accessibleByUnselected: ID! @join__field(graph: ONE_QUERY_UNSELECTED_0) r1: R @join__field(graph: ONE_T_R1_0) r2: R @join__field(graph: ONE_T_R2_0) r3: R @join__field(graph: ONE_T_R3_0) @@ -60,9 +63,10 @@ type T @join__type(graph: ONE_QUERY_T2_0, key: "id id2") @join__type(graph: ONE_ r5: R @join__field(graph: ONE_T_R5_0) } -type Query @join__type(graph: ONE_QUERY_T2_0) @join__type(graph: ONE_QUERY_T_0) @join__type(graph: ONE_T_R1_0) @join__type(graph: ONE_T_R2_0) @join__type(graph: ONE_T_R3_0) @join__type(graph: ONE_T_R4_0) @join__type(graph: ONE_T_R5_0) { +type Query @join__type(graph: ONE_QUERY_T2_0) @join__type(graph: ONE_QUERY_T_0) @join__type(graph: ONE_QUERY_UNSELECTED_0) @join__type(graph: ONE_T_R1_0) @join__type(graph: ONE_T_R2_0) @join__type(graph: ONE_T_R3_0) @join__type(graph: ONE_T_R4_0) @join__type(graph: ONE_T_R5_0) { t2(id: ID!, id2: ID!): T @join__field(graph: ONE_QUERY_T2_0) t(id: ID!): T @join__field(graph: ONE_QUERY_T_0) + unselected(unselected: ID!): T @join__field(graph: ONE_QUERY_UNSELECTED_0) _: ID @inaccessible @join__field(graph: ONE_T_R1_0) @join__field(graph: ONE_T_R2_0) @join__field(graph: ONE_T_R3_0) @join__field(graph: ONE_T_R4_0) @join__field(graph: ONE_T_R5_0) } diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql.snap index 5ee3a2d753..67300fc0d2 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql.snap @@ -8,6 +8,8 @@ directive @defer(label: String, if: Boolean! = true) on FRAGMENT_SPREAD | INLINE type Query { t(id: ID!): T t2(id: ID!, id2: ID!): T + " Uses the `unselected` field as a key, but doesn't select it " + unselected(unselected: ID!): T } type R { @@ -18,6 +20,8 @@ type R { type T { id: ID! id2: ID! + unselected: ID! + accessibleByUnselected: ID! r1: R r2: R r3: R