Skip to content

Commit ccbf23d

Browse files
benjamnbenjamn
authored
Fix empty @join__directive(graphs: [], ...) bug (#8721)
1 parent 990111b commit ccbf23d

21 files changed

+288
-52
lines changed

‎apollo-federation/src/connectors/expand/carryover.rs‎

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ const JOIN_DIRECTIVE_DIRECTIVE_NAME: Name = name!("join__directive");
5050
pub(super) fn carryover_directives(
5151
from: &FederationSchema,
5252
to: &mut FederationSchema,
53-
specs: impl Iterator<Item = ConnectSpec>,
5453
subgraph_name_replacements: &MultiMap<&str, String>,
5554
connect_directive_names: HashMap<String, [Name; 2]>,
5655
) -> Result<(), FederationError> {
@@ -88,11 +87,10 @@ pub(super) fn carryover_directives(
8887
})
8988
.collect();
9089

91-
// @join__directive(graph: [], name: "link", args: { url: "https://specs.apollo.dev/connect/v0.1" })
92-
// this must exist for license key enforcement
93-
for spec in specs {
94-
SchemaDefinitionPosition.insert_directive(to, spec.join_directive_application().into())?;
95-
}
90+
// NOTE: We used to create @join__directive directives here for license enforcement,
91+
// but the merger already handles this correctly by collecting directives from all
92+
// subgraphs and creating the appropriate @join__directive applications.
93+
// Creating them here caused issues with invalid enum value references.
9694

9795
// @link for connect
9896
if let Some(link) = metadata.for_identity(&ConnectSpec::identity()) {
@@ -880,12 +878,48 @@ mod tests {
880878
use insta::assert_snapshot;
881879

882880
use super::carryover_directives;
883-
use crate::connectors::ConnectSpec;
884881
use crate::merge::merge_federation_subgraphs;
885882
use crate::schema::FederationSchema;
886883
use crate::schema::position::EnumTypeDefinitionPosition;
887884
use crate::supergraph::extract_subgraphs_from_supergraph;
888885

886+
#[test]
887+
fn test_carryover_fixes_empty_graphs() {
888+
// Test that we fix the buggy graphs: [] directive from old schemas
889+
let sdl = include_str!("tests/schemas/expand/buggy_graphs_empty.graphql");
890+
let schema = Schema::parse(sdl, "buggy.graphql").expect("parse failed");
891+
let supergraph_schema = FederationSchema::new(schema).expect("federation schema failed");
892+
893+
// Verify the input has the buggy directive
894+
let input_sdl = supergraph_schema.schema().serialize().to_string();
895+
assert!(
896+
input_sdl.contains("graphs: []"),
897+
"Input should have buggy graphs: []"
898+
);
899+
900+
let subgraphs = extract_subgraphs_from_supergraph(&supergraph_schema, None)
901+
.expect("extract subgraphs failed");
902+
let merged = merge_federation_subgraphs(subgraphs).expect("merge failed");
903+
let mut schema =
904+
FederationSchema::new(merged.schema.into_inner()).expect("federation schema failed");
905+
906+
carryover_directives(
907+
&supergraph_schema,
908+
&mut schema,
909+
&Default::default(),
910+
Default::default(),
911+
)
912+
.expect("carryover failed");
913+
914+
let output_sdl = schema.schema().serialize().to_string();
915+
// Verify the output does NOT have the buggy directive
916+
assert!(
917+
!output_sdl.contains("graphs: []"),
918+
"Output should NOT have buggy graphs: []"
919+
);
920+
assert_snapshot!("carryover_fixes_empty_graphs", output_sdl);
921+
}
922+
889923
#[test]
890924
fn test_carryover() {
891925
let sdl = include_str!("./tests/schemas/ignore/directives.graphql");
@@ -909,7 +943,6 @@ mod tests {
909943
carryover_directives(
910944
&supergraph_schema,
911945
&mut schema,
912-
[ConnectSpec::V0_1].into_iter(),
913946
&Default::default(),
914947
Default::default(),
915948
)

‎apollo-federation/src/connectors/expand/carryover/inputs.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,14 @@ pub(super) fn replace_join_directive_graphs_argument(
405405
}
406406
}
407407

408+
// If the graphs list is empty after processing, don't carry over the directive.
409+
// This filters out the buggy @join__directive(graphs: [], name: "link", ...)
410+
// that was created by the license enforcement code before our fix.
411+
// The correct directive with populated graphs will be created separately.
412+
if new_graph_values.is_empty() {
413+
return None;
414+
}
415+
408416
// Create a new directive with the updated graphs argument
409417
let mut new_directive = directive.clone();
410418
if let Some(graphs_arg) = new_directive

‎apollo-federation/src/connectors/expand/mod.rs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,9 @@ pub fn expand_connectors(
9090

9191
// Expand just the connector subgraphs
9292
let mut expanded_subgraphs = Vec::new();
93-
let mut spec_versions = HashSet::new();
9493

9594
for (link, sub) in connect_subgraphs {
9695
expanded_subgraphs.extend(split_subgraph(&link, sub)?);
97-
spec_versions.insert(link.spec);
9896
}
9997

10098
// Merge the subgraphs into one supergraph
@@ -120,7 +118,6 @@ pub fn expand_connectors(
120118
carryover_directives(
121119
&supergraph.schema,
122120
&mut new_supergraph,
123-
spec_versions.into_iter(),
124121
&subgraph_name_replacements,
125122
subgraph_connect_directive_names,
126123
)

‎apollo-federation/src/connectors/expand/snapshots/apollo_federation__connectors__expand__carryover__tests__carryover.snap‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
source: apollo-federation/src/connectors/expand/carryover.rs
33
expression: schema.schema().serialize().to_string()
44
---
5-
schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) @join__directive(graphs: [], name: "link", args: {url: "https://specs.apollo.dev/connect/v0.1"}) @link(url: "https://specs.apollo.dev/inaccessible/v0.2", for: SECURITY) @link(url: "https://specs.apollo.dev/tag/v0.3") @link(url: "https://specs.apollo.dev/authenticated/v0.1", for: SECURITY) @link(url: "https://specs.apollo.dev/requiresScopes/v0.1", for: SECURITY) @link(url: "https://specs.apollo.dev/policy/v0.1", for: SECURITY) @link(url: "https://specs.apollo.dev/cost/v0.1", import: ["@listSize"]) @link(url: "http://specs.example.org/custom/v0.1", import: ["@custom1", "@custom2", {name: "@originalName", as: "@custom3"}]) @link(url: "http://bugfix/weird/v1.0", import: ["@weird"]) @link(url: "https://specs.apollo.dev/context/v0.1", for: SECURITY) {
5+
schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) @link(url: "https://specs.apollo.dev/inaccessible/v0.2", for: SECURITY) @link(url: "https://specs.apollo.dev/tag/v0.3") @link(url: "https://specs.apollo.dev/authenticated/v0.1", for: SECURITY) @link(url: "https://specs.apollo.dev/requiresScopes/v0.1", for: SECURITY) @link(url: "https://specs.apollo.dev/policy/v0.1", for: SECURITY) @link(url: "https://specs.apollo.dev/cost/v0.1", import: ["@listSize"]) @link(url: "http://specs.example.org/custom/v0.1", import: ["@custom1", "@custom2", {name: "@originalName", as: "@custom3"}]) @link(url: "http://bugfix/weird/v1.0", import: ["@weird"]) @link(url: "https://specs.apollo.dev/context/v0.1", for: SECURITY) {
66
query: Query
77
}
88

‎apollo-federation/src/connectors/expand/snapshots/apollo_federation__connectors__expand__carryover__tests__carryover_fixes_empty_graphs.snap‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
---
2+
source: apollo-federation/src/connectors/expand/carryover.rs
3+
expression: output_sdl
4+
---
5+
schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) @link(url: "https://specs.apollo.dev/connect/v0.1", for: EXECUTION) @join__directive(graphs: [CONNECTOR], name: "link", args: {url: "https://specs.apollo.dev/connect/v0.1", import: ["@source", "@connect"]}) {
6+
query: Query
7+
}
8+
9+
directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
10+
11+
directive @join__graph(name: String!, url: String!) on ENUM_VALUE
12+
13+
directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on ENUM | INPUT_OBJECT | INTERFACE | OBJECT | SCALAR | UNION
14+
15+
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, overrideLabel: String, usedOverridden: Boolean, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
16+
17+
directive @join__implements(graph: join__Graph!, interface: String!) repeatable on INTERFACE | OBJECT
18+
19+
directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
20+
21+
directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
22+
23+
directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments!) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION
24+
25+
enum link__Purpose {
26+
"""
27+
SECURITY features provide metadata necessary to securely resolve fields.
28+
"""
29+
SECURITY
30+
"""EXECUTION features provide metadata necessary for operation execution."""
31+
EXECUTION
32+
}
33+
34+
scalar link__Import
35+
36+
scalar join__FieldSet
37+
38+
scalar join__FieldValue
39+
40+
input join__ContextArgument {
41+
name: String!
42+
type: String!
43+
context: String!
44+
selection: join__FieldValue!
45+
}
46+
47+
scalar join__DirectiveArguments
48+
49+
enum join__Graph {
50+
CONNECTOR @join__graph(name: "connector", url: "connector")
51+
}
52+
53+
type Query @join__type(graph: CONNECTOR) {
54+
user: User @join__field(graph: CONNECTOR, type: "User")
55+
}
56+
57+
type User @join__type(graph: CONNECTOR, key: "id") {
58+
id: ID! @join__field(graph: CONNECTOR, type: "ID!")
59+
name: String @join__field(graph: CONNECTOR, type: "String")
60+
}
61+
62+
scalar connect__JSONSelection @join__type(graph: CONNECTOR)
63+
64+
scalar connect__URLTemplate @join__type(graph: CONNECTOR)
65+
66+
input connect__HTTPHeaderMapping @join__type(graph: CONNECTOR) {
67+
name: String! @join__field(graph: CONNECTOR, type: "String!")
68+
from: String @join__field(graph: CONNECTOR, type: "String")
69+
value: [String!] @join__field(graph: CONNECTOR, type: "[String!]")
70+
}
71+
72+
input connect__ConnectHTTP @join__type(graph: CONNECTOR) {
73+
GET: connect__URLTemplate @join__field(graph: CONNECTOR, type: "connect__URLTemplate")
74+
POST: connect__URLTemplate @join__field(graph: CONNECTOR, type: "connect__URLTemplate")
75+
PUT: connect__URLTemplate @join__field(graph: CONNECTOR, type: "connect__URLTemplate")
76+
PATCH: connect__URLTemplate @join__field(graph: CONNECTOR, type: "connect__URLTemplate")
77+
DELETE: connect__URLTemplate @join__field(graph: CONNECTOR, type: "connect__URLTemplate")
78+
body: connect__JSONSelection @join__field(graph: CONNECTOR, type: "connect__JSONSelection")
79+
headers: [connect__HTTPHeaderMapping!] @join__field(graph: CONNECTOR, type: "[connect__HTTPHeaderMapping!]")
80+
path: connect__JSONSelection @join__field(graph: CONNECTOR, type: "connect__JSONSelection")
81+
queryParams: connect__JSONSelection @join__field(graph: CONNECTOR, type: "connect__JSONSelection")
82+
}
83+
84+
input connect__ConnectBatch @join__type(graph: CONNECTOR) {
85+
maxSize: Int @join__field(graph: CONNECTOR, type: "Int")
86+
}
87+
88+
input connect__ConnectorErrors @join__type(graph: CONNECTOR) {
89+
message: connect__JSONSelection @join__field(graph: CONNECTOR, type: "connect__JSONSelection")
90+
extensions: connect__JSONSelection @join__field(graph: CONNECTOR, type: "connect__JSONSelection")
91+
}
92+
93+
input connect__SourceHTTP @join__type(graph: CONNECTOR) {
94+
baseURL: String! @join__field(graph: CONNECTOR, type: "String!")
95+
headers: [connect__HTTPHeaderMapping!] @join__field(graph: CONNECTOR, type: "[connect__HTTPHeaderMapping!]")
96+
path: connect__JSONSelection @join__field(graph: CONNECTOR, type: "connect__JSONSelection")
97+
queryParams: connect__JSONSelection @join__field(graph: CONNECTOR, type: "connect__JSONSelection")
98+
}

‎apollo-federation/src/connectors/expand/tests/schemas/expand/buggy_graphs_empty.graphql‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
schema
2+
@link(url: "https://specs.apollo.dev/link/v1.0")
3+
@link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION)
4+
@join__directive(graphs: [], name: "link", args: {url: "https://specs.apollo.dev/connect/v0.1"})
5+
@link(url: "https://specs.apollo.dev/connect/v0.1", for: EXECUTION)
6+
@join__directive(graphs: [CONNECTOR], name: "link", args: {url: "https://specs.apollo.dev/connect/v0.1", import: ["@source", "@connect"]})
7+
{
8+
query: Query
9+
}
10+
11+
directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
12+
13+
directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION
14+
15+
directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
16+
17+
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
18+
19+
directive @join__graph(name: String!, url: String!) on ENUM_VALUE
20+
21+
directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE
22+
23+
directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR
24+
25+
directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
26+
27+
enum join__Graph {
28+
CONNECTOR @join__graph(name: "connector", url: "connector")
29+
}
30+
31+
scalar join__DirectiveArguments
32+
33+
scalar join__FieldSet
34+
35+
scalar join__FieldValue
36+
37+
input join__ContextArgument {
38+
name: String!
39+
type: String!
40+
context: String!
41+
selection: join__FieldValue!
42+
}
43+
44+
enum link__Purpose {
45+
EXECUTION
46+
SECURITY
47+
}
48+
49+
scalar link__Import
50+
51+
type Query
52+
@join__type(graph: CONNECTOR)
53+
{
54+
user: User @join__field(graph: CONNECTOR)
55+
}
56+
57+
type User
58+
@join__type(graph: CONNECTOR, key: "id")
59+
{
60+
id: ID! @join__field(graph: CONNECTOR)
61+
name: String @join__field(graph: CONNECTOR)
62+
}

‎apollo-federation/src/connectors/expand/tests/snapshots/api@buggy_graphs_empty.graphql.snap‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
source: apollo-federation/src/connectors/expand/tests/mod.rs
3+
expression: api_schema
4+
input_file: apollo-federation/src/connectors/expand/tests/schemas/expand/buggy_graphs_empty.graphql
5+
---
6+
directive @defer(label: String, if: Boolean! = true) on FRAGMENT_SPREAD | INLINE_FRAGMENT
7+
8+
type Query {
9+
user: User
10+
}
11+
12+
type User {
13+
id: ID!
14+
name: String
15+
}

‎apollo-federation/src/connectors/expand/tests/snapshots/connectors@buggy_graphs_empty.graphql.snap‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
source: apollo-federation/src/connectors/expand/tests/mod.rs
3+
expression: connectors.by_service_name
4+
input_file: apollo-federation/src/connectors/expand/tests/schemas/expand/buggy_graphs_empty.graphql
5+
---
6+
{}

‎apollo-federation/src/connectors/expand/tests/snapshots/[email protected]‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ source: apollo-federation/src/connectors/expand/tests/mod.rs
33
expression: raw_sdl
44
input_file: apollo-federation/src/connectors/expand/tests/schemas/expand/batch.graphql
55
---
6-
schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) @link(url: "https://specs.apollo.dev/inaccessible/v0.2", for: SECURITY) @join__directive(graphs: [], name: "link", args: {url: "https://specs.apollo.dev/connect/v0.2"}) @link(url: "https://specs.apollo.dev/connect/v0.2", for: EXECUTION) @join__directive(graphs: [ONE_QUERY_USERS_0, ONE_USER_0], name: "link", args: {url: "https://specs.apollo.dev/connect/v0.2", import: ["@source", "@connect"]}) {
6+
schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) @link(url: "https://specs.apollo.dev/inaccessible/v0.2", for: SECURITY) @link(url: "https://specs.apollo.dev/connect/v0.2", for: EXECUTION) @join__directive(graphs: [ONE_QUERY_USERS_0, ONE_USER_0], name: "link", args: {url: "https://specs.apollo.dev/connect/v0.2", import: ["@source", "@connect"]}) {
77
query: Query
88
}
99

‎apollo-federation/src/connectors/expand/tests/snapshots/supergraph@buggy_graphs_empty.graphql.snap‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
---
2+
source: apollo-federation/src/connectors/expand/tests/mod.rs
3+
expression: raw_sdl
4+
input_file: apollo-federation/src/connectors/expand/tests/schemas/expand/buggy_graphs_empty.graphql
5+
---
6+
directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
7+
8+
directive @join__graph(name: String!, url: String!) on ENUM_VALUE
9+
10+
directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on ENUM | INPUT_OBJECT | INTERFACE | OBJECT | SCALAR | UNION
11+
12+
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, overrideLabel: String, usedOverridden: Boolean, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
13+
14+
directive @join__implements(graph: join__Graph!, interface: String!) repeatable on INTERFACE | OBJECT
15+
16+
directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
17+
18+
directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
19+
20+
directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments!) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION
21+
22+
enum link__Purpose {
23+
"""
24+
SECURITY features provide metadata necessary to securely resolve fields.
25+
"""
26+
SECURITY
27+
"""EXECUTION features provide metadata necessary for operation execution."""
28+
EXECUTION
29+
}
30+
31+
scalar link__Import
32+
33+
scalar join__FieldSet
34+
35+
scalar join__FieldValue
36+
37+
input join__ContextArgument {
38+
name: String!
39+
type: String!
40+
context: String!
41+
selection: join__FieldValue!
42+
}
43+
44+
scalar join__DirectiveArguments
45+
46+
enum join__Graph

0 commit comments

Comments
 (0)