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

Allow @interfaceObject to stand in for all runtime types #3087

Merged
merged 5 commits into from
Jul 19, 2024

Conversation

clenfest
Copy link
Contributor

Rather than relying on the fact that one filteredPaths is reached, allow @interfaceObject types to intersect with all runtime types

clenfest added 2 commits July 16, 2024 19:54
We sometimes can get multiple paths generated for the same location (cause tbd). When this happens, it breaks the logic when detecting concrete type intersection as one type could potentially be an interfaceObject (and the check should not be run). To be safe, if the type we see does not exist in the list of possible implementation types, skip it from intersection testing.
Copy link

changeset-bot bot commented Jul 17, 2024

🦋 Changeset detected

Latest commit: 5a4863a

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

This PR includes changesets to release 7 packages
Name Type
@apollo/composition Patch
@apollo/gateway Patch
@apollo/federation-internals Patch
@apollo/query-planner Patch
@apollo/query-graphs Patch
@apollo/subgraph 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

Copy link

netlify bot commented Jul 17, 2024

Deploy Preview for apollo-federation-docs ready!

Name Link
🔨 Latest commit 5a4863a
🔍 Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/669963b4b7c3bc000870dc8a
😎 Deploy Preview https://deploy-preview-3087--apollo-federation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codesandbox-ci bot commented Jul 17, 2024

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.

@clenfest clenfest marked this pull request as ready for review July 17, 2024 14:18
@clenfest clenfest requested a review from a team as a code owner July 17, 2024 14:18
@duckki
Copy link
Contributor

duckki commented Jul 17, 2024

Wouldn't it make sense to deduplicate while computing the filteredPaths here?

    const filteredPaths = newSubgraphPathInfos.map((p) => p.path.path).filter((p) => isAbstractType(p.tail.type));

Also, I'm concerned about having duplicate paths in newSubgraphPathInfos, which means we do duplicate work as well.

@clenfest
Copy link
Contributor Author

Wouldn't it make sense to deduplicate while computing the filteredPaths here?

    const filteredPaths = newSubgraphPathInfos.map((p) => p.path.path).filter((p) => isAbstractType(p.tail.type));

Also, I'm concerned about having duplicate paths in newSubgraphPathInfos, which means we do duplicate work as well.

I'm also concerned about why the duplicate paths exist, and I'm going to keep investigating that. I think that this is an ok compromise as it should unblock the customer and we can deliver a more comprehensive fix shortly. I agree that there is something else wrong, but I think this should be safe.

Copy link
Contributor

@duckki duckki left a comment

Choose a reason for hiding this comment

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

Looks good! I could reproduce the issue with the included test. And the fix makes sense.
Also, even without duplicate paths, interface object + 2 other paths with abstract types would cause this problem, it seems. We can track the duplicate path issue separately.

@clenfest clenfest merged commit 4d9e0f6 into main Jul 19, 2024
19 checks passed
@clenfest clenfest deleted the clenfest/fed-354 branch July 19, 2024 13:35
clenfest pushed a commit that referenced this pull request Jul 25, 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/composition@2.8.4

### Patch Changes

- When doing interface type intersection detection, allow
@interfaceObject to stand in for any type
([#3087](#3087))

- Updated dependencies
\[[`5f4bb160d024678d6facd471c43c8ec61c86e701`](5f4bb16),
[`672aca7cbeb0a6a38586357a4e154f2dd91caa0c`](672aca7)]:
    -   @apollo/federation-internals@2.8.4
    -   @apollo/query-graphs@2.8.4

## @apollo/gateway@2.8.4

### Patch Changes

- Updated dependencies
\[[`4d9e0f6390c5114132d205ab73b6aa1b9ffa8cd8`](4d9e0f6),
[`5f4bb160d024678d6facd471c43c8ec61c86e701`](5f4bb16),
[`672aca7cbeb0a6a38586357a4e154f2dd91caa0c`](672aca7)]:
    -   @apollo/composition@2.8.4
    -   @apollo/federation-internals@2.8.4
    -   @apollo/query-planner@2.8.4

## @apollo/federation-internals@2.8.4

### Patch Changes

- When auto-upgrading schemas from fed1, never add @Shareable on
subscription fields.
([#3094](#3094))

- Save time in SchemaUpgrader by pre-computing which subgraphs contain
each type
([#3057](#3057))

## @apollo/query-graphs@2.8.4

### Patch Changes

- Updated dependencies
\[[`5f4bb160d024678d6facd471c43c8ec61c86e701`](5f4bb16),
[`672aca7cbeb0a6a38586357a4e154f2dd91caa0c`](672aca7)]:
    -   @apollo/federation-internals@2.8.4

## @apollo/query-planner@2.8.4

### Patch Changes

- Updated dependencies
\[[`5f4bb160d024678d6facd471c43c8ec61c86e701`](5f4bb16),
[`672aca7cbeb0a6a38586357a4e154f2dd91caa0c`](672aca7)]:
    -   @apollo/federation-internals@2.8.4
    -   @apollo/query-graphs@2.8.4

## @apollo/subgraph@2.8.4

### Patch Changes

- Add descriptions for federation directives
([#3095](#3095))

- Updated dependencies
\[[`5f4bb160d024678d6facd471c43c8ec61c86e701`](5f4bb16),
[`672aca7cbeb0a6a38586357a4e154f2dd91caa0c`](672aca7)]:
    -   @apollo/federation-internals@2.8.4

## apollo-federation-integration-testsuite@2.8.4

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 this pull request may close these issues.

2 participants