From 3d8021f8707d359b7baf06bac872be2abcac8fa7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= <renee.kooi@apollographql.com>
Date: Fri, 4 Oct 2024 11:17:17 +0200
Subject: [PATCH] fix(federation): match JS condition order with multiple
 skip/include on the same selection

Rust was iterating in a consistent order, JS iterates in declaration
order.

Surprisingly this was the only place we use the consistent order. This
removes the `iter_sorted()` method from DirectiveList that is now no
longer used. Internally, DirectiveList still uses the consistent sort
order for hashing and equality.

I think it's somewhat nicer to use the consistent order here, so you do
not get different plans based on an innocuous input difference, but so
be it. We can consider using a consistent order everywhere we handle
directives in the future.
---
 .../src/operation/directive_list.rs           | 18 +-----
 .../src/query_plan/conditions.rs              |  2 +-
 .../condition_order_router799.graphql         | 58 +++++++++++++++++++
 3 files changed, 60 insertions(+), 18 deletions(-)
 create mode 100644 apollo-federation/tests/query_plan/supergraphs/condition_order_router799.graphql

diff --git a/apollo-federation/src/operation/directive_list.rs b/apollo-federation/src/operation/directive_list.rs
index df03995b53..ad716dd1b4 100644
--- a/apollo-federation/src/operation/directive_list.rs
+++ b/apollo-federation/src/operation/directive_list.rs
@@ -288,13 +288,6 @@ impl DirectiveList {
             .iter()
     }
 
-    /// Iterate the directives in a consistent sort order.
-    pub(crate) fn iter_sorted(&self) -> DirectiveIterSorted<'_> {
-        self.inner
-            .as_ref()
-            .map_or_else(DirectiveIterSorted::empty, |inner| inner.iter_sorted())
-    }
-
     /// Remove one directive application by name.
     ///
     /// To remove a repeatable directive, you may need to call this multiple times.
@@ -336,7 +329,7 @@ impl DirectiveList {
 }
 
 /// Iterate over a [`DirectiveList`] in a consistent sort order.
-pub(crate) struct DirectiveIterSorted<'a> {
+struct DirectiveIterSorted<'a> {
     directives: &'a [Node<executable::Directive>],
     inner: std::slice::Iter<'a, usize>,
 }
@@ -354,15 +347,6 @@ impl ExactSizeIterator for DirectiveIterSorted<'_> {
     }
 }
 
-impl DirectiveIterSorted<'_> {
-    fn empty() -> Self {
-        Self {
-            directives: &[],
-            inner: [].iter(),
-        }
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use std::collections::HashSet;
diff --git a/apollo-federation/src/query_plan/conditions.rs b/apollo-federation/src/query_plan/conditions.rs
index 7017c53a8a..c1bbab2e02 100644
--- a/apollo-federation/src/query_plan/conditions.rs
+++ b/apollo-federation/src/query_plan/conditions.rs
@@ -70,7 +70,7 @@ impl Conditions {
 
     pub(crate) fn from_directives(directives: &DirectiveList) -> Result<Self, FederationError> {
         let mut variables = IndexMap::default();
-        for directive in directives.iter_sorted() {
+        for directive in directives.iter() {
             let negated = match directive.name.as_str() {
                 "include" => false,
                 "skip" => true,
diff --git a/apollo-federation/tests/query_plan/supergraphs/condition_order_router799.graphql b/apollo-federation/tests/query_plan/supergraphs/condition_order_router799.graphql
new file mode 100644
index 0000000000..69ef1b485a
--- /dev/null
+++ b/apollo-federation/tests/query_plan/supergraphs/condition_order_router799.graphql
@@ -0,0 +1,58 @@
+# Composed from subgraphs with hash: 3ebe0e8c55ad21763879da6341617f14953e80d7
+schema
+  @link(url: "https://specs.apollo.dev/link/v1.0")
+  @link(url: "https://specs.apollo.dev/join/v0.4", for: EXECUTION)
+{
+  query: Query
+  mutation: Mutation
+}
+
+directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION
+
+directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
+
+directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
+
+directive @join__graph(name: String!, url: String!) on ENUM_VALUE
+
+directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE
+
+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
+
+directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
+
+directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
+
+scalar join__DirectiveArguments
+
+scalar join__FieldSet
+
+enum join__Graph {
+  BOOKS @join__graph(name: "books", url: "none")
+}
+
+scalar link__Import
+
+enum link__Purpose {
+  """
+  `SECURITY` features provide metadata necessary to securely resolve fields.
+  """
+  SECURITY
+
+  """
+  `EXECUTION` features provide metadata necessary for operation execution.
+  """
+  EXECUTION
+}
+
+type Mutation
+  @join__type(graph: BOOKS)
+{
+  bookName(name: String!): Int!
+}
+
+type Query
+  @join__type(graph: BOOKS)
+{
+  bookName: String!
+}