fix: fixed filter_on_schema (#5459)

This PR fixes a small logic bug in filter_on_schema. 4 of the requires.rs test failures are gone.
apollographql · Jun 15, 2024 · b4326a3 · b4326a3
1 parent bbe1f35
commit b4326a3
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 13 deletions.
diff --git a/apollo-federation/src/query_graph/graph_path.rs b/apollo-federation/src/query_graph/graph_path.rs
@@ -3562,12 +3562,18 @@ impl OpPath {
             match element.as_ref() {
                 OpPathElement::InlineFragment(fragment) => {
                     if let Some(type_condition) = &fragment.data().type_condition_position {
-                        if schema.get_type(type_condition.type_name().clone()).is_ok() {
-                            let updated_fragment = fragment.with_updated_type_condition(None);
-                            filtered
-                                .push(Arc::new(OpPathElement::InlineFragment(updated_fragment)));
+                        if schema.get_type(type_condition.type_name().clone()).is_err() {
+                            if element.directives().is_empty() {
+                                continue; // skip this element
+                            } else {
+                                // Replace this element with an unconditioned inline fragment
+                                let updated_fragment = fragment.with_updated_type_condition(None);
+                                filtered.push(Arc::new(OpPathElement::InlineFragment(
+                                    updated_fragment,
+                                )));
+                            }
                         } else {
-                            continue;
+                            filtered.push(element.clone());
                         }
                     } else {
                         filtered.push(element.clone());

diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/requires.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/requires.rs
@@ -293,8 +293,6 @@ fn handles_multiple_requires_involving_different_nestedness() {
 
 /// require that depends on another require
 #[test]
-#[should_panic(expected = "snapshot assertion")]
-// TODO: investigate this failure
 fn it_handles_simple_require_chain() {
     let planner = planner!(
         Subgraph1: r#"
@@ -447,8 +445,6 @@ fn it_handles_simple_require_chain() {
 }
 
 #[test]
-#[should_panic(expected = "snapshot assertion")]
-// TODO: investigate this failure
 fn it_handles_require_chain_not_ending_in_original_group() {
     // This is somewhat simiar to the 'simple require chain' case, but the chain does not
     // end in the group in which the query start
@@ -634,8 +630,6 @@ fn it_handles_require_chain_not_ending_in_original_group() {
 
 /// a chain of 10 requires
 #[test]
-#[should_panic(expected = "snapshot assertion")]
-// TODO: investigate this failure
 fn it_handles_longer_require_chain() {
     let planner = planner!(
         Subgraph1: r#"
@@ -1371,8 +1365,6 @@ fn it_can_require_at_inaccessible_fields() {
 }
 
 #[test]
-#[should_panic(expected = "snapshot assertion")]
-// TODO: investigate this failure
 fn it_require_of_multiple_field_when_one_is_also_a_key_to_reach_another() {
     // The specificity of this example is that we `T.v` requires 2 fields `req1`
     // and `req2`, but `req1` is also a key to get `req2`. This dependency was