Skip to content

Commit

Permalink
fix(federation): match JS condition order with multiple skip/include …
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
goto-bus-stop committed Oct 4, 2024
1 parent 0dd8322 commit 3d8021f
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 18 deletions.
18 changes: 1 addition & 17 deletions apollo-federation/src/operation/directive_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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>,
}
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion apollo-federation/src/query_plan/conditions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -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!
}

0 comments on commit 3d8021f

Please sign in to comment.