apollographql / router

A configurable, high-performance routing runtime for Apollo Federation 🚀
https://www.apollographql.com/docs/router/
Other
813 stars 271 forks source link

fix(federation): correct order of @skip and @include conditions on the same selection #6137

Closed goto-bus-stop closed 1 month ago

goto-bus-stop commented 1 month ago

PR https://github.com/apollographql/router/pull/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.

https://github.com/apollographql/federation/blob/cc4573471696ef78d04fa00c4cf8e5c50314ba9f/query-graphs-js/src/pathContext.ts#L13-L33

The queries I was testing with just happened to pass in https://github.com/apollographql/router/pull/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.

In the second commit I do a small refactor to replace the is_negated boolean by a ConditionKind enum. I found it quite tricky to constantly translate the skip/include concepts to negation or not while reading the code, so I hope this simplifies it a bit.

svc-apollo-docs commented 1 month ago

✅ Docs Preview Ready

No new or changed pages found.

github-actions[bot] commented 1 month ago

@goto-bus-stop, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

router-perf[bot] commented 1 month ago

CI performance tests