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

incorrect query plan generated when spreading interface fragment into multiple implementations #396

Closed
benjaminjkraft opened this issue May 4, 2020 · 4 comments · Fixed by #652

Comments

@benjaminjkraft
Copy link
Contributor

Consider the following query, against the federation demo service:

fragment Price on Product {
  price
}

query {
  topProducts {
    __typename
    ... on Book {
      ...Price
    }
    ... on Furniture {
      ...Price
    }
  }
}

Apollo gateway generates an incorrect query plan: it should simply pass on an equivalent query to the products service, but in fact it entirely omits asking for price on Furniture, and only asks for it on Book.

Test case to reproduce this on master
diff --git packages/apollo-gateway/src/__tests__/buildQueryPlan.test.ts packages/apollo-gateway/src/__tests__/buildQueryPlan.test.ts
index 156f7f93..4f210413 100644
--- packages/apollo-gateway/src/__tests__/buildQueryPlan.test.ts
+++ packages/apollo-gateway/src/__tests__/buildQueryPlan.test.ts
@@ -768,6 +768,46 @@ describe('buildQueryPlan', () => {
         }
       `);
     });
+
+    it(`should not get confused by a fragment spread multiple times`, () => {
+      const query = gql`
+        fragment Price on Product {
+          price
+        }
+
+        query {
+          topProducts {
+            __typename
+            ... on Book {
+              ...Price
+            }
+            ... on Furniture {
+              ...Price
+            }
+          }
+        }
+      `;
+
+      const queryPlan = buildQueryPlan(buildOperationContext(schema, query));
+
+      expect(queryPlan).toMatchInlineSnapshot(`
+                QueryPlan {
+                  Fetch(service: "product") {
+                    {
+                      topProducts {
+                        __typename
+                        ... on Book {
+                          price
+                        }
+                        ... on Furniture {
+                          price
+                        }
+                      }
+                    }
+                  },
+                }
+            `);
+    });
   });
 
   // GraphQLError: Cannot query field "isbn" on type "Book"

Test output excerpt:

  ● buildQueryPlan › for abstract types › should not get confused by a fragment spread multiple times

    expect(received).toMatchInlineSnapshot(snapshot)

    Snapshot name: `buildQueryPlan for abstract types should not get confused by a fragment spread multiple times 1`

    - Snapshot
    + Received

    @@ -4,12 +4,9 @@
            topProducts {
              __typename
              ... on Book {
                price
              }
    -         ... on Furniture {
    -           price
    -         }
            }
          }
        },
      }
@benjaminjkraft
Copy link
Contributor Author

It's possible this is related to #384, but there's not quite enough information there for me to know.

@abernix
Copy link
Member

abernix commented Jun 17, 2020

@benjaminjkraft Do you know if this is still happening on the latest version of @apollo/gateway? Do you mind opening a PR with this test to verify it is fixed (or not!)?

benjaminjkraft referenced this issue in benjaminjkraft/apollo-server Jun 17, 2020
Not intended to be merged as such, just to run the test per discussion
in the issue.
@benjaminjkraft
Copy link
Contributor Author

benjaminjkraft commented Jun 17, 2020

You got it: apollographql/apollo-server#4281. From a quick look, it looks like it is still happening.

(Edit: removed what I previously wrote here -- I had misread the diff.)

@abernix abernix transferred this issue from apollographql/apollo-server Jan 15, 2021
@abernix abernix changed the title gateway: incorrect query plan generated when spreading interface fragment into multiple implementations incorrect query plan generated when spreading interface fragment into multiple implementations Apr 1, 2021
pcmanus pushed a commit to pcmanus/federation that referenced this issue Apr 9, 2021
pcmanus pushed a commit to pcmanus/federation that referenced this issue Apr 9, 2021
The query planner associate to each occurrence of a field in a query a
"scope". That scope records a so-called "parent type" as well as all the
types the object could have when reaching that field.

For instance, with:
```graphql
{
  ... on X {
    ... on Z {
      f
    }
  }
  ... on Y {
    ... on Z {
      f
    }
  }
}
```
the planner associate `Z` as parent type of both occurrences of `f` (
because it's mot the "immediate" condition applied to it in both
cases), but also record that the 1st occurrence is necessary a `X`
while it is necessary a `Y` in the 2nd occurrence.

When handling fields a particular level, the planner groups fields by
field name and parent type. In the case above, it treats both
occurrences of `f` together, because they have the same parent type `Z`.

However, when treating that group, the planner incorrectly ignores the
fact that while the parent types are the same, the scope are not fully
similar, and it only handles the 1st occurrence of the field. As
a result, the generated plan ends up only fetching the first
occurrence, which is incorrect (concretely, the gateway never return `Y`
objects while it should).

This commit fixes the problem by changing the way fields are grouped.
Instead of being grouped just by parent type, they are grouped by
their scope. So, in the example above, both occurrences of `f` are
treated separately.

Fixes: apollographql#396
pcmanus pushed a commit to pcmanus/federation that referenced this issue Apr 29, 2021
The query planner associate to each occurrence of a field in a query a
"scope". That scope records a so-called "parent type" as well as all the
types the object could have when reaching that field.

For instance, with:
```graphql
{
  ... on X {
    ... on Z {
      f
    }
  }
  ... on Y {
    ... on Z {
      f
    }
  }
}
```
the planner associate `Z` as parent type of both occurrences of `f` (
because it's mot the "immediate" condition applied to it in both
cases), but also record that the 1st occurrence is necessary a `X`
while it is necessary a `Y` in the 2nd occurrence.

When handling fields a particular level, the planner groups fields by
field name and parent type. In the case above, it treats both
occurrences of `f` together, because they have the same parent type `Z`.

However, when treating that group, the planner incorrectly ignores the
fact that while the parent types are the same, the scope are not fully
similar, and it only handles the 1st occurrence of the field. As
a result, the generated plan ends up only fetching the first
occurrence, which is incorrect (concretely, the gateway never return `Y`
objects while it should).

This commit fixes the problem by changing the way fields are grouped.
Instead of being grouped just by parent type, they are grouped by
their scope. So, in the example above, both occurrences of `f` are
treated separately.

Fixes: apollographql#396
pcmanus pushed a commit that referenced this issue Apr 29, 2021
Fix missing fields due to loss of specific scope (fixes #396)
@abernix
Copy link
Member

abernix commented Apr 30, 2021

The fix for this was released in @apollo/gateway@0.27.1! 🎉

abernix pushed a commit that referenced this issue Nov 3, 2021
The 'should preserve type conditions for value types' test had been
added in the fix for #396: that fix was primarily ensure that we
did not elimiate type condition, even when they not strictly useful
to the rest of the selection, _if_ they have directives we need
to preserve. But that particular test was actually testing the
case where the type condition has no directives and is thus,
strictly speaking, useless, and the new query planner now optimize
that case "properly".

Note that the next test, 'should preserve directives on inline fragments
even if the fragment is otherwise useless', test the more interesting
where we do want to preserve a type condition due to a directive,
and this test is (still) properly handled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants