fix(federation): correct order of @skip and @include conditions o…

…n the same selection

PR #6120 was the wrong solution. JS doesn't actually maintain the order
of the conditions, like I thought. Instead, it always puts `@skip` first
and `@include` second, the opposite of what Rust was doing.

The queries I was testing with just happened to pass in #6120.

This changes the implementation of `Conditions::from_directives` to
pick the directives out manually instead of iterating. Technically, this
does two iterations of the directive list...but, the code is a bit
simpler, I think.

apollo-federation/src/query_plan/conditions.rs

@@ -9,6 +9,7 @@ use indexmap::map::Entry;
 use serde::Serialize;
 
 use crate::error::FederationError;
+use crate::internal_error;
 use crate::operation::DirectiveList;
 use crate::operation::NamedFragments;
 use crate::operation::Selection;
@@ -70,42 +71,52 @@ impl Conditions {
         }
     }
 
+    /// Parse @skip and @include conditions from a directive list.
     pub(crate) fn from_directives(directives: &DirectiveList) -> Result<Self, FederationError> {
         let mut variables = IndexMap::default();
-        for directive in directives.iter() {
-            let negated = match directive.name.as_str() {
-                "include" => false,
-                "skip" => true,
-                _ => continue,
+
+        if let Some(skip) = directives.get("skip") {
+            let Some(value) = skip.specified_argument_by_name("if") else {
+                internal_error!("missing @skip(if:) argument");
             };
-            let value = directive.specified_argument_by_name("if").ok_or_else(|| {
-                FederationError::internal(format!(
-                    "missing if argument on @{}",
-                    if negated { "skip" } else { "include" },
-                ))
-            })?;
-            match &**value {
-                Value::Boolean(false) if !negated => return Ok(Self::Boolean(false)),
-                Value::Boolean(true) if negated => return Ok(Self::Boolean(false)),
+
+            match value.as_ref() {
+                // Constant @skip(if: true) can never match
+                Value::Boolean(true) => return Ok(Self::Boolean(false)),
+                // Constant @skip(if: false) always matches
                 Value::Boolean(_) => {}
-                Value::Variable(name) => match variables.entry(name.clone()) {
-                    Entry::Occupied(entry) => {
-                        let previous_negated = *entry.get();
-                        if previous_negated != negated {
-                            return Ok(Self::Boolean(false));
-                        }
-                    }
-                    Entry::Vacant(entry) => {
-                        entry.insert(negated);
+                Value::Variable(name) => {
+                    variables.insert(name.clone(), true);
+                }
+                _ => {
+                    internal_error!("expected boolean or variable `if` argument, got {value}");
+                }
+            }
+        }
+
+        if let Some(include) = directives.get("include") {
+            let Some(value) = include.specified_argument_by_name("if") else {
+                internal_error!("missing @include(if:) argument");
+            };
+
+            match value.as_ref() {
+                // Constant @include(if: false) can never match
+                Value::Boolean(false) => return Ok(Self::Boolean(false)),
+                // Constant @include(if: true) always matches
+                Value::Boolean(true) => {}
+                // If both @skip(if: $var) and @include(if: $var) exist, the condition can also
+                // never match
+                Value::Variable(name) => {
+                    if variables.insert(name.clone(), false) == Some(true) {
+                        return Ok(Self::Boolean(false));
                     }
-                },
+                }
                 _ => {
-                    return Err(FederationError::internal(format!(
-                        "expected boolean or variable `if` argument, got {value}",
-                    )))
+                    internal_error!("expected boolean or variable `if` argument, got {value}");
                 }
             }
         }
+
         Ok(Self::from_variables(variables))
     }
 

apollo-federation/tests/query_plan/build_query_plan_tests.rs

@@ -1333,4 +1333,31 @@ fn condition_order_router799() {
     }
     "###
     );
+
+    // Reordering @skip/@include should produce the same plan.
+    assert_plan!(
+        &planner,
+        r#"
+          mutation($var0: Boolean! = true, $var1: Boolean!) {
+            ... on Mutation @include(if: $var1) @skip(if: $var0) {
+              field0: __typename
+            }
+          }
+        "#,
+        @r###"
+    QueryPlan {
+      Include(if: $var1) {
+        Skip(if: $var0) {
+          Fetch(service: "books") {
+            {
+              ... on Mutation {
+                field0: __typename
+              }
+            }
+          },
+        },
+      },
+    }
+    "###
+    );
 }