Skip to content

Commit c073d09

Browse files
kamilkisielakamilkisiela
authored
Cover last 2 failing test cases in the audit (#260)
1 parent c4827a2 commit c073d09

File tree

15 files changed

+680
-169
lines changed

15 files changed

+680
-169
lines changed

‎bin/dev-cli/src/main.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,8 @@ fn process_plan(supergraph_path: &str, operation_path: &str) -> QueryPlan {
141141
let graph = Graph::graph_from_supergraph_state(&supergraph).expect("failed to create graph");
142142
let operation = get_operation(operation_path, &supergraph);
143143
let override_context = PlannerOverrideContext::default();
144-
let best_paths_per_leaf = walk_operation(&graph, &override_context, &operation).unwrap();
144+
let best_paths_per_leaf =
145+
walk_operation(&graph, &supergraph, &override_context, &operation).unwrap();
145146
let query_tree = find_best_combination(&graph, best_paths_per_leaf).unwrap();
146147
let fetch_graph =
147148
build_fetch_graph_from_query_tree(&graph, &supergraph, &override_context, query_tree)
@@ -186,7 +187,8 @@ fn process_paths(
186187
let graph = Graph::graph_from_supergraph_state(&supergraph).expect("failed to create graph");
187188
let operation = get_operation(operation_path, &supergraph);
188189
let override_context = PlannerOverrideContext::default();
189-
let best_paths_per_leaf = walk_operation(&graph, &override_context, &operation).unwrap();
190+
let best_paths_per_leaf =
191+
walk_operation(&graph, &supergraph, &override_context, &operation).unwrap();
190192

191193
(graph, best_paths_per_leaf, operation, supergraph)
192194
}

‎lib/query-planner/benches/qp_benches.rs‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,13 @@ fn query_plan_pipeline(c: &mut Criterion) {
4848
let bb_supergraph_state = black_box(&supergraph_state);
4949
let bb_override_context = black_box(&override_context);
5050

51-
let best_paths_per_leaf = walk_operation(bb_graph, bb_override_context, bb_operation)
52-
.expect("walk_operation failed during benchmark");
51+
let best_paths_per_leaf = walk_operation(
52+
bb_graph,
53+
bb_supergraph_state,
54+
bb_override_context,
55+
bb_operation,
56+
)
57+
.expect("walk_operation failed during benchmark");
5358
let query_tree = find_best_combination(bb_graph, best_paths_per_leaf).unwrap();
5459
let fetch_graph = build_fetch_graph_from_query_tree(
5560
bb_graph,

‎lib/query-planner/fixture/tests/provides-on-interface.supergraph.graphql‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
schema
2+
@link(url: "https://specs.apollo.dev/link/v1.0")
3+
@link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) {
4+
query: Query
5+
}
6+
7+
directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
8+
9+
directive @join__field(
10+
graph: join__Graph
11+
requires: join__FieldSet
12+
provides: join__FieldSet
13+
type: String
14+
external: Boolean
15+
override: String
16+
usedOverridden: Boolean
17+
) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
18+
19+
directive @join__graph(name: String!, url: String!) on ENUM_VALUE
20+
21+
directive @join__implements(
22+
graph: join__Graph!
23+
interface: String!
24+
) repeatable on OBJECT | INTERFACE
25+
26+
directive @join__type(
27+
graph: join__Graph!
28+
key: join__FieldSet
29+
extension: Boolean! = false
30+
resolvable: Boolean! = true
31+
isInterfaceObject: Boolean! = false
32+
) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR
33+
34+
directive @join__unionMember(
35+
graph: join__Graph!
36+
member: String!
37+
) repeatable on UNION
38+
39+
directive @link(
40+
url: String
41+
as: String
42+
for: link__Purpose
43+
import: [link__Import]
44+
) repeatable on SCHEMA
45+
46+
interface Animal
47+
@join__type(graph: A)
48+
@join__type(graph: B)
49+
@join__type(graph: C) {
50+
id: ID!
51+
name: String @join__field(graph: B) @join__field(graph: C)
52+
}
53+
54+
type Book implements Media
55+
@join__implements(graph: A, interface: "Media")
56+
@join__implements(graph: B, interface: "Media")
57+
@join__implements(graph: C, interface: "Media")
58+
@join__type(graph: A, key: "id")
59+
@join__type(graph: B)
60+
@join__type(graph: C, key: "id") {
61+
id: ID!
62+
animals: [Animal]
63+
@join__field(graph: A)
64+
@join__field(graph: B, external: true)
65+
@join__field(graph: C)
66+
}
67+
68+
type Cat implements Animal
69+
@join__implements(graph: A, interface: "Animal")
70+
@join__implements(graph: B, interface: "Animal")
71+
@join__implements(graph: C, interface: "Animal")
72+
@join__type(graph: A, key: "id")
73+
@join__type(graph: B)
74+
@join__type(graph: C, key: "id") {
75+
id: ID!
76+
@join__field(graph: A, external: true)
77+
@join__field(graph: B, external: true)
78+
@join__field(graph: C)
79+
name: String @join__field(graph: B, external: true) @join__field(graph: C)
80+
age: Int @join__field(graph: C)
81+
}
82+
83+
type Dog implements Animal
84+
@join__implements(graph: A, interface: "Animal")
85+
@join__implements(graph: B, interface: "Animal")
86+
@join__implements(graph: C, interface: "Animal")
87+
@join__type(graph: A, key: "id")
88+
@join__type(graph: B)
89+
@join__type(graph: C, key: "id") {
90+
id: ID!
91+
@join__field(graph: A, external: true)
92+
@join__field(graph: B, external: true)
93+
@join__field(graph: C)
94+
name: String
95+
@join__field(graph: A, external: true)
96+
@join__field(graph: B, external: true)
97+
@join__field(graph: C)
98+
age: Int @join__field(graph: C)
99+
}
100+
101+
scalar join__FieldSet
102+
103+
enum join__Graph {
104+
A
105+
@join__graph(
106+
name: "a"
107+
url: "http://localhost:4200/provides-on-interface/a"
108+
)
109+
B
110+
@join__graph(
111+
name: "b"
112+
url: "http://localhost:4200/provides-on-interface/b"
113+
)
114+
C
115+
@join__graph(
116+
name: "c"
117+
url: "http://localhost:4200/provides-on-interface/c"
118+
)
119+
}
120+
121+
scalar link__Import
122+
123+
enum link__Purpose {
124+
"""
125+
`SECURITY` features provide metadata necessary to securely resolve fields.
126+
"""
127+
SECURITY
128+
129+
"""
130+
`EXECUTION` features provide metadata necessary for operation execution.
131+
"""
132+
EXECUTION
133+
}
134+
135+
interface Media
136+
@join__type(graph: A)
137+
@join__type(graph: B)
138+
@join__type(graph: C) {
139+
id: ID!
140+
animals: [Animal] @join__field(graph: B) @join__field(graph: C)
141+
}
142+
143+
type Query @join__type(graph: A) @join__type(graph: B) @join__type(graph: C) {
144+
media: Media
145+
@join__field(graph: A)
146+
@join__field(graph: B, provides: "animals { id name }")
147+
book: Book @join__field(graph: A, provides: "animals { ... on Dog { name } }")
148+
}

‎lib/query-planner/src/ast/normalization/mod.rs‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -598,11 +598,11 @@ mod tests {
598598
&supergraph,
599599
&parse_query(
600600
r#"
601-
query nodeid($id: ID!) {
602-
node(id: $id) {
603-
id
604-
}
601+
query nodeid($id: ID!) {
602+
node(id: $id) {
603+
id
605604
}
605+
}
606606
"#,
607607
)
608608
.expect("to parse"),
@@ -612,17 +612,17 @@ mod tests {
612612
.to_string()
613613
),
614614
@r"
615-
query nodeid($id: ID!) {
616-
node(id: $id) {
617-
... on Account {
618-
id
619-
}
620-
... on Chat {
621-
id
615+
query nodeid($id: ID!) {
616+
node(id: $id) {
617+
... on Account {
618+
id
619+
}
620+
... on Chat {
621+
id
622+
}
623+
}
622624
}
623-
}
624-
}
625-
",
625+
",
626626
);
627627

628628
insta::assert_snapshot!(

‎lib/query-planner/src/ast/normalization/pipeline/type_expand.rs‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,13 @@ fn handle_selection_set(
162162
continue;
163163
}
164164

165+
let field_def = type_def.fields().get(&field.name).ok_or_else(|| {
166+
NormalizationError::FieldNotFoundInType {
167+
field_name: field.name.clone(),
168+
type_name: type_def.name().to_string(),
169+
}
170+
})?;
171+
165172
if let Some(possible_object_types) = &possible_object_types {
166173
if handle_type_expansion_candidate(
167174
state,
@@ -176,15 +183,7 @@ fn handle_selection_set(
176183
}
177184

178185
if !field.selection_set.items.is_empty() {
179-
let inner_type_name = type_def
180-
.fields()
181-
.get(&field.name)
182-
.ok_or_else(|| NormalizationError::FieldNotFoundInType {
183-
field_name: field.name.clone(),
184-
type_name: type_def.name().to_string(),
185-
})?
186-
.field_type
187-
.inner_type();
186+
let inner_type_name = field_def.field_type.inner_type();
188187
let inner_type_def =
189188
state.definitions.get(inner_type_name).ok_or_else(|| {
190189
NormalizationError::SchemaTypeNotFound {

‎lib/query-planner/src/graph/mod.rs‎

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ impl Graph {
278278
#[instrument(level = "trace", skip(self, state))]
279279
fn build_entity_reference_edges(&mut self, state: &SupergraphState) -> Result<(), GraphError> {
280280
for (def_name, definition) in state.definitions.iter() {
281-
let is_interface = matches!(definition, SupergraphDefinition::Interface(_));
281+
let is_interface = definition.is_interface_type();
282282
for join_type1 in definition.join_types() {
283283
// Connects object and interface entities of the same name by @key
284284
for join_type2 in definition.join_types() {
@@ -910,6 +910,41 @@ impl Graph {
910910
head: NodeIndex,
911911
view_id: u64,
912912
) -> Result<(), GraphError> {
913+
for jt in parent_type_def
914+
.join_types()
915+
.iter()
916+
.filter(|jt| jt.resolvable && jt.key.is_some() && jt.graph_id != graph_id)
917+
{
918+
let tail = self.upsert_node(Node::new_node(
919+
parent_type_def.name(),
920+
state.resolve_graph_id(&jt.graph_id)?,
921+
jt.is_interface_object,
922+
));
923+
let key_selection = FederationRules::parse_key(
924+
state,
925+
&jt.graph_id,
926+
parent_type_def.name(),
927+
jt.key.as_ref().unwrap(),
928+
);
929+
trace!(
930+
"Creating entity move edge from '{}/{}' to '{}/{}' via key '{}'",
931+
parent_type_def.name(),
932+
graph_id,
933+
parent_type_def.name(),
934+
jt.graph_id,
935+
jt.key.as_ref().unwrap()
936+
);
937+
self.upsert_edge(
938+
head,
939+
tail,
940+
Edge::create_entity_move(
941+
jt.key.as_ref().unwrap(),
942+
key_selection,
943+
parent_type_def.is_interface_type(),
944+
),
945+
);
946+
}
947+
913948
for selection in selection_set.items.iter() {
914949
match selection {
915950
Selection::Field(field) => {
@@ -1030,6 +1065,10 @@ impl Graph {
10301065
for join_type in definition.join_types().iter() {
10311066
let mut view_id = 0;
10321067

1068+
// A map of provided types to graph ids that
1069+
// we need to create edges to their matching entity types.
1070+
let mut connection_to_build: HashMap<NodeIndex, String> = HashMap::new();
1071+
10331072
for (field_name, field_definition) in definition.fields().iter() {
10341073
for join_field in field_definition.join_field.iter() {
10351074
if join_field
@@ -1055,6 +1094,8 @@ impl Graph {
10551094
),
10561095
));
10571096

1097+
connection_to_build.insert(head, join_type.graph_id.clone());
1098+
10581099
let return_type_name = field_definition.field_type.inner_type();
10591100

10601101
let tail = self.upsert_node(Node::new_specialized_node(
@@ -1119,6 +1160,41 @@ impl Graph {
11191160
}
11201161
}
11211162
}
1163+
1164+
for (head, from_graph_id) in connection_to_build {
1165+
for jt in definition.join_types().iter().filter(|jt| {
1166+
jt.resolvable && jt.key.is_some() && jt.graph_id != from_graph_id
1167+
}) {
1168+
let tail = self.upsert_node(Node::new_node(
1169+
def_name,
1170+
state.resolve_graph_id(&jt.graph_id)?,
1171+
jt.is_interface_object,
1172+
));
1173+
let key_selection = FederationRules::parse_key(
1174+
state,
1175+
&jt.graph_id,
1176+
def_name,
1177+
jt.key.as_ref().unwrap(),
1178+
);
1179+
trace!(
1180+
"Creating entity move edge from '{}/{}' to '{}/{}' via key '{}'",
1181+
def_name,
1182+
from_graph_id,
1183+
def_name,
1184+
jt.graph_id,
1185+
jt.key.as_ref().unwrap()
1186+
);
1187+
self.upsert_edge(
1188+
head,
1189+
tail,
1190+
Edge::create_entity_move(
1191+
jt.key.as_ref().unwrap(),
1192+
key_selection,
1193+
definition.is_interface_type(),
1194+
),
1195+
);
1196+
}
1197+
}
11221198
}
11231199
}
11241200

‎lib/query-planner/src/graph/node.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,15 @@ impl Node {
6969
}
7070
}
7171

72+
pub fn name_str(&self) -> &str {
73+
match self {
74+
Node::QueryRoot(name) => name,
75+
Node::MutationRoot(name) => name,
76+
Node::SubscriptionRoot(name) => name,
77+
Node::SubgraphType(st) => &st.name,
78+
}
79+
}
80+
7281
pub fn is_using_provides(&self) -> bool {
7382
match self {
7483
Node::QueryRoot(_) => false,

0 commit comments

Comments
 (0)