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

Query planner bug with queries fetching fields nested in multiple unions #2938

Closed
tinnou opened this issue Feb 15, 2024 · 1 comment · Fixed by #2949
Closed

Query planner bug with queries fetching fields nested in multiple unions #2938

tinnou opened this issue Feb 15, 2024 · 1 comment · Fixed by #2949
Assignees

Comments

@tinnou
Copy link
Contributor

tinnou commented Feb 15, 2024

Issue Description

When querying a field that is in a path of 2 or more unions, the query planner is not able to handle different selections and will aggressively collapse selections in fetches yielding an incorrect plan.

Steps to Reproduce

I built a minimum viable example unit test that reproduces building an incorrect plan.

Subgraph searchSubgraph schema:

type Query {
  search: [SearchResult]
}

union SearchResult = MovieResult | ArticleResult
union Section = EntityCollectionSection | GallerySection

type MovieResult @key(fields: "id") {
  id: ID!
  sections: [Section]
}

type ArticleResult @key(fields: "id") {
  id: ID!
  sections: [Section]
}

type EntityCollectionSection @key(fields: "id") {
  id: ID!
}

type GallerySection @key(fields: "id") {
  id: ID!
}

Subgraph artworkSubgraph schema:

type Query {
  me: String
}

type EntityCollectionSection @key(fields: "id") {
  id: ID!
  title: String
  artwork(params: String): String
}

type GallerySection @key(fields: "id") {
  id: ID!
  artwork(params: String): String
}

Executing the following query:

query Search($movieParams: String, $articleParams: String) {
  search {
    __typename
    ... on MovieResult {
      id
      sections {
        ... on EntityCollectionSection {
          id
          artwork(params: $movieParams)
        }
      }
    }
    ... on ArticleResult {
      id
      sections {
        ... on EntityCollectionSection {
          id
          artwork(params: $articleParams)
          title
        }
      }
    }
  }
}

The plan that is currently generated is as follow:

QueryPlan {
  Sequence {
    Fetch(service: "searchSubgraph") {
      {
        search {
          __typename
          ... on MovieResult {
            id
            sections {
              __typename
              ... on EntityCollectionSection {
                __typename
                id
              }
            }
          }
          ... on ArticleResult {
            id
            sections {
              __typename
              ... on EntityCollectionSection {
                __typename
                id
              }
            }
          }
        }
      }
    },
    Flatten(path: "[email protected].@") {
      Fetch(service: "artworkSubgraph") {
        {
          ... on EntityCollectionSection {
            __typename
            id
          }
        } =>
        {
          ... on EntityCollectionSection {
            artwork(params: $movieParams)
            title
          }
        }
      },
    },
  },
}

The problem here is the second fetch:

Fetch(service: "artworkSubgraph") {
        {
          ... on EntityCollectionSection {
            __typename
            id
          }
        } =>
        {
          ... on EntityCollectionSection {
            artwork(params: $movieParams)
            title
          }
        }
      },
    },
  },

You can see that the query plan merge the fields from the two selections on EntityCollectionSection from the original query. This is incorrect for two reasons:

  1. the artwork field here accept different inputs based on the whether it's on a ArticleResult or a MovieResult
  2. the title field is actually only requested in case of ArticleResult

Impact

Since this is a query planner bug I believe this will affect both v1 and v2 gateways.

Potential Fixes

We addressed this issue in our internal planner by having two separate parallel fetches one for each branch. Additionally, we made the flatten path support polymorphic types using a new syntax such that the execution can flatten the results back to the appropriate fragment.

Here is a snippet of the output of our query planner, (the full query plan is available in the unit test), note the | delimiter to hint on the concrete type, search.@|MovieResult:

 Parallel {
            Flatten(path: "search.@|MovieResult.sections.@") {
              Fetch(service: "artworkSubgraph") {
                {
                  ... on EntityCollectionSection {
                    __typename
                    id
                  }
                } =>
                {
                  ... on EntityCollectionSection {
                    artwork(params: $movieParams)
                  }
                }
              }
            }
            Flatten(path: "search.@|ArticleResult.sections.@") {
              Fetch(service: "artworkSubgraph") {
                {
                  ... on EntityCollectionSection {
                    __typename
                    id
                  }
                } =>
                {
                  ... on EntityCollectionSection {
                    title
                    artwork(params: $articleParams)
                  }
                }
              }
            }
          }

Link to Reproduction

https://github.com/apollographql/federation/compare/main...tinnou:union_planner_bug?expand=1

Reproduction Steps

No response

@o0Ignition0o o0Ignition0o self-assigned this Mar 4, 2024
@samuelAndalon
Copy link
Contributor

#2952 (comment)

o0Ignition0o added a commit that referenced this issue Apr 12, 2024
Type conditioned fetching

Fixes #2938
When querying a field that is in a path of 2 or more unions, the query
planner was not able to handle different selections and would
aggressively collapse selections in fetches yielding an incorrect plan.

This change introduces new syntax to express type conditions in (key and
flatten) paths. Type conditioned fetching can be enabled through a flag,
and execution is supported in the router only.
o0Ignition0o pushed a commit that referenced this issue Apr 16, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @apollo/[email protected]

### Patch Changes

- Fix a query planning bug where invalid subgraph queries are generated
with `reuseQueryFragments` set true.
([#2952](#2952))
([#2963](#2963))

- Updated dependencies
\[[`ec04c50b4fb832bfd281ecf9c0c2dd7656431b96`](ec04c50),
[`a494631918156f0431ceace74281c076cf1d5d51`](a494631)]:
    -   @apollo/[email protected]
    -   @apollo/[email protected]

## @apollo/[email protected]

### Patch Changes

- Updated dependencies
\[[`ec04c50b4fb832bfd281ecf9c0c2dd7656431b96`](ec04c50),
[`3e2c845c74407a136b9e0066e44c1ad1467d3013`](3e2c845),
[`a494631918156f0431ceace74281c076cf1d5d51`](a494631)]:
    -   @apollo/[email protected]
    -   @apollo/[email protected]
    -   @apollo/[email protected]

## @apollo/[email protected]

### Patch Changes

- Fix a query planning bug where invalid subgraph queries are generated
with `reuseQueryFragments` set true.
([#2952](#2952))
([#2963](#2963))

- Fixed query planner to pass the directives from original query to
subgraph operations (#2961)
([#2967](#2967))

## @apollo/[email protected]

### Patch Changes

- Updated dependencies
\[[`ec04c50b4fb832bfd281ecf9c0c2dd7656431b96`](ec04c50),
[`a494631918156f0431ceace74281c076cf1d5d51`](a494631)]:
    -   @apollo/[email protected]

## @apollo/[email protected]

### Patch Changes

- Fix a query planning bug where invalid subgraph queries are generated
with `reuseQueryFragments` set true.
([#2952](#2952))
([#2963](#2963))

- Type conditioned fetching
([#2949](#2949))

When querying a field that is in a path of 2 or more unions, the query
planner was not able to handle different selections and would
aggressively collapse selections in fetches yielding an incorrect plan.

This change introduces new syntax to express type conditions in (key and
flatten) paths. Type conditioned fetching can be enabled through a flag,
and execution is supported in the router only. (#2938)

- Fixed query planner to pass the directives from original query to
subgraph operations (#2961)
([#2967](#2967))

- Updated dependencies
\[[`ec04c50b4fb832bfd281ecf9c0c2dd7656431b96`](ec04c50),
[`a494631918156f0431ceace74281c076cf1d5d51`](a494631)]:
    -   @apollo/[email protected]
    -   @apollo/[email protected]

## @apollo/[email protected]

### Patch Changes

- Updated dependencies
\[[`ec04c50b4fb832bfd281ecf9c0c2dd7656431b96`](ec04c50),
[`a494631918156f0431ceace74281c076cf1d5d51`](a494631)]:
    -   @apollo/[email protected]

## [email protected]

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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 a pull request may close this issue.

3 participants