Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix validation issue introduced by #1653 #1673

Merged
merged 2 commits into from
Apr 5, 2022

Conversation

pcmanus
Copy link
Contributor

@pcmanus pcmanus commented Apr 4, 2022

The "edge following other edges"-precomputation optimization introduced
in #1653 introduced a regression in the composition composition code.

The problem is due to the fact that the optimization was assuming that
the validation/query planning algorithm would always find optimal paths,
but the algorithm was a bit too aggressive at avoid some edges given
that new optimization is in place.

Let's use the example this patch uses as test to illustrate:

type Query {
  t: T
}

type T @key(fields: "k1") {
  k1: Int
}
type T @key(fields: "k2") @key(fields: "k1") {
  k1: Int
  k2: Int
}
type T @key(fields: "k2") {
  k2: Int
  v: Int
}

When the algorithm is on T in A and looks for v, it tries to find
where keys can lead it. In doing so, it:

  1. looked first at key k2 to B and checked if it was usable. And it is,
    provided we first use key k1 to get k1 from B. It's inefficient
    but possiblly (it's arguably dumb, but it's an algorithm, it has no
    shame).
  2. then looked at key k1 to B. Now, it actually ignored that edge
    because it already had a path to B (the one from the previous point).
    That's because the validation algorithm doesn't care one bit about
    efficiency, and that is not the issue. The issue is that the
    algorithm was also marking that edge "excluded" for remainder of
    the "current loop".
  3. it looked at k2 to C, which is actually edge we want to use. But
    unfortunately, to use it, we first need to get k2 from B using k1
    and as mentioned in the previous point, we had mistankenly excluded
    that edge, so that edge was considered a dead end when it shouldn't.
  4. at that point the algorithm was actually looking back at the path
    it found in point 1, which got it to B, and checked if it could get
    to C from there. That's where the algorithm "used to" make progress
    by, at this point, using the k2 edge from B to C. But the
    optimization from Optimise indirect path computations #1653 made use ignore that edge, on account that
    we never should have to use it.

Note that the optimization of #1653 is not really to blame: the true
problem is that in point 3 above, we're not able to use the k2 edge
from A to C due to an incorrect exclusion. It just happens that
before #1653, the algorithm was able to get back on its feet by using
an inefficient option it shouldn't have had to use.

Long story short, the patch fixes the exclusion issue.

The "edge following other edges"-precomputation optimization introduced
in apollographql#1653 introduced a regression in the composition composition code.

The problem is due to the fact that the optimization was assuming that
the validation/query planning algorithm would always find optimal paths,
but the algorithm was a bit too aggressive at avoid some edges given
that new optimization is in place.

Let's use the example this patch uses as test to illustrate:
```graphql
type Query {
  t: T
}

type T @key(fields: "k1") {
  k1: Int
}
```
```graphql
type T @key(fields: "k2") @key(fields: "k1") {
  k1: Int
  k2: Int
}
```
```graphql
type T @key(fields: "k2") {
  k2: Int
  v: Int
}
```
When the algorithm is on `T` in `A` and looks for `v`, it tries to find
where keys can lead it. In doing so, it:
1. looked first at key `k2` to B and checked if it was usable. And it is,
   provided we first use key `k1` to get `k1` from B. It's inefficient
   but possiblly (it's arguably dumb, but it's an algorithm, it has no
   shame).
2. then looked at key `k1` to B. Now, it actually ignored that edge
   because it already had a path to B (the one from the previous point).
   That's because the validation algorithm doesn't care one bit about
   efficiency, and that is not the issue. The issue is that the
   algorithm was also marking that edge "excluded" for remainder of
   the "current loop".
3. it looked at `k2` to C, which is actually edge we want to use. But
   unfortunately, to use it, we first need to get `k2` from B using `k1`
   and as mentioned in the previous point, we had mistankenly excluded
   that edge, so that edge was considered a dead end when it shouldn't.
4. at that point the algorithm was actually looking back at the path
   it found in point 1, which got it to B, and checked if it could get
   to C from there. That's where the algorithm "used to" make progress
   by, at this point, using the `k2` edge from B to C. But the
   optimization from apollographql#1653 made use ignore that edge, on account that
   we never should have to use it.

Note that the optimization of apollographql#1653 is not really to blame: the true
problem is that in point 3 above, we're not able to use the `k2` edge
from A to C due to an incorrect exclusion. It just happens that
before apollographql#1653, the algorithm was able to get back on its feet by using
an inefficient option it shouldn't have had to use.

Long story short, the patch fixes the exclusion issue.
@netlify
Copy link

netlify bot commented Apr 4, 2022

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2e0541c

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 4, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@cpeacock cpeacock requested a review from clenfest April 4, 2022 18:31
Copy link
Contributor

@benweatherman benweatherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐐

@pcmanus pcmanus merged commit d554780 into apollographql:main Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants