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

Don't treat entity interface __typename refinements as useless #2775

Merged
merged 5 commits into from
Sep 14, 2023

Conversation

trevor-scheer
Copy link
Member

@trevor-scheer trevor-scheer commented Sep 8, 2023

Fix specific case for requesting __typename on interface entity type

In certain cases, when resolving a __typename on an interface entity (due to it actual being requested in the operation), that fetch group could previously be trimmed / treated as useless. At a glance, it appears to be a redundant step, i.e.:

{ ... on Product { __typename id }} => { ... on Product { __typename} }

It's actually necessary to preserve this in the case that we're coming from an interface object to an (entity) interface so that we can resolve the concrete __typename correctly.

Ref: #2743
(this fixes one of the queries from the issue but not both)

@trevor-scheer trevor-scheer requested a review from a team as a code owner September 8, 2023 16:39
@changeset-bot
Copy link

changeset-bot bot commented Sep 8, 2023

🦋 Changeset detected

Latest commit: 4fd48ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@apollo/query-planner Patch
@apollo/subgraph Patch
@apollo/gateway Patch
@apollo/federation-internals Patch
@apollo/composition Patch
@apollo/query-graphs Patch
apollo-federation-integration-testsuite Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for apollo-federation-docs canceled.

Name Link
🔨 Latest commit 4fd48ba
🔍 Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/65038a0ec780da0008896d72

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 8, 2023

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.

if (parentType && isInterfaceType(parentType)) {
// Lastly, we just need to check that we're coming from a subgraph
// that has the type as an interface object in its schema.
return this.parents().some((p) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my own edification, probably just being dense, but why are there multiple parents here?

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding (and would appreciate @pcmanus to confirm) is that we'd rarely have more than one parent in practice, but it's possible in the case that we also happen to be in a diamond-shaped dependency situation.

@trevor-scheer trevor-scheer merged commit 66d7e4c into main Sep 14, 2023
@trevor-scheer trevor-scheer deleted the trevor/fix-2743 branch September 14, 2023 22:42
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.

2 participants