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.

Sign up for GitHub

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

Jump to bottom

Improve composition hints around types that are strictly @external #2951

Merged
merged 7 commits into from
Mar 19, 2024
Merged

Improve composition hints around types that are strictly @external #2951

merged 7 commits into from
Mar 19, 2024

Conversation

trevor-scheer
Copy link
Member

Note: 1st commit highlights the issue with a test case, 2nd commit proposes the change + test update. No existing tests are affected.

Composition currently gives unhelpful hints around inconsistent value types which are only defined for the sake of referencing (all fields or type is marked @external).

In the example test case, the Permissions type is only referenced in the account (2nd) subgraph due to the @requires, so a partial definition seems reasonable here. The hint suggests adding the missing field in order to make the type consistent across subgraphs.

However, if we take the actionable step recommended by that hint and add the missing field (and mark it @external, by necessity), we end up with a composition error instead:

[Subgraph2] Field "Permissions.canEdit" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).

@trevor-scheer trevor-scheer requested a review from a team as a code owner March 6, 2024 00:36
@netlify Netlify
Copy link

netlify bot commented Mar 6, 2024 •
edited

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit ff2a1cb

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Mar 6, 2024 •
edited

🦋 Changeset detected

Latest commit: ff2a1cb

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

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented Mar 6, 2024 •
edited

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.

sachindshinde
sachindshinde requested changes Mar 15, 2024
View reviewed changes
Copy link
Contributor

@sachindshinde sachindshinde left a comment

Choose a reason for hiding this comment

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

Just a few things below, but other than that this looks fine.

I think the logic you've stated in the PR description makes sense. The idea behind the value type hint is that you're more likely to run into satisfiability errors if you have partial value types in subgraphs. However, if all the fields are @external for a subgraph, then the type should never really be resolved in that subgraph, so you won't run into satisfiability errors from that (you can technically resolve them via @provides, but that's for providing more optimal paths to the query planner compared to existing ones, and it shouldn't have to resolve all value type fields for that)

composition-js/src/merging/merge.ts Outdated Show resolved Hide resolved
composition-js/src/merging/merge.ts Outdated Show resolved Hide resolved
@dariuszkuc dariuszkuc merged commit 33506be into apollographql:main Mar 19, 2024
12 of 13 checks passed
@github-actions github-actions bot mentioned this pull request Mar 19, 2024
Merged
@trevor-scheer trevor-scheer deleted the trevor/inconsistent-value-type-hints branch March 19, 2024 16:58
clenfest pushed a commit that referenced this pull request Mar 21, 2024
@github-actions
release: on branch main (#2930)
5e02367 
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.7.2

### Patch Changes

- When a linked directive requires a federation version higher than the
linked federation spec, upgrade to the implied version and issue a hint
([#2929](#2929))

- Stop emitting "inconsistent value type" hints against definitions
where the type is marked `@external` or all fields are marked
`@external`.
([#2951](#2951))

- Introduce a new composition hint pertaining specifically to
progressive `@override` usage (when a `label` argument is present).
([#2922](#2922))

- Updated dependencies
\[[`33b937b18d3c7ca6af14b904696b536399e597d1`](33b937b),
[`09cd3e55e810ee513127b7440f5b11af7540c9b0`](09cd3e5),
[`d7189a86c27891af408d3d0184db6133d3342967`](d7189a8)]:
    -   @apollo/federation-internals@2.7.2
    -   @apollo/query-graphs@2.7.2

## @apollo/gateway@2.7.2

### Patch Changes

- Remove out-of-band reporting in the gateway and provide a warning for
users who have the endpoint configured.
([#2946](#2946))

- Updated dependencies
\[[`33b937b18d3c7ca6af14b904696b536399e597d1`](33b937b),
[`09cd3e55e810ee513127b7440f5b11af7540c9b0`](09cd3e5),
[`d7189a86c27891af408d3d0184db6133d3342967`](d7189a8),
[`33506bef6d755c58400081824167711c1747ee40`](33506be),
[`1f72f2a361a83ebaaf15ae052f5ca9a93fc18bfc`](1f72f2a)]:
    -   @apollo/composition@2.7.2
    -   @apollo/federation-internals@2.7.2
    -   @apollo/query-planner@2.7.2

## @apollo/federation-internals@2.7.2

### Patch Changes

- When a linked directive requires a federation version higher than the
linked federation spec, upgrade to the implied version and issue a hint
([#2929](#2929))

- When auto-upgrading a subgraph (i.e. one that does not explicitly
@link the federation spec) do not go past v2.4. This is so that
subgraphs will not inadvertently require the latest join spec (which
cause the router or gateway not to start if running an older version).
([#2933](#2933))

- Add new `generateQueryFragments` option to query planner config
([#2958](#2958))

If enabled, the query planner will extract inline fragments into
fragment definitions before sending queries to subgraphs. This can
significantly reduce the size of the query sent to subgraphs, but may
increase the time it takes to plan the query.

## @apollo/query-graphs@2.7.2

### Patch Changes

- Updated dependencies
\[[`33b937b18d3c7ca6af14b904696b536399e597d1`](33b937b),
[`09cd3e55e810ee513127b7440f5b11af7540c9b0`](09cd3e5),
[`d7189a86c27891af408d3d0184db6133d3342967`](d7189a8)]:
    -   @apollo/federation-internals@2.7.2

## @apollo/query-planner@2.7.2

### Patch Changes

- When auto-upgrading a subgraph (i.e. one that does not explicitly
@link the federation spec) do not go past v2.4. This is so that
subgraphs will not inadvertently require the latest join spec (which
cause the router or gateway not to start if running an older version).
([#2933](#2933))

- Add new `generateQueryFragments` option to query planner config
([#2958](#2958))

If enabled, the query planner will extract inline fragments into
fragment definitions before sending queries to subgraphs. This can
significantly reduce the size of the query sent to subgraphs, but may
increase the time it takes to plan the query.

- Updated dependencies
\[[`33b937b18d3c7ca6af14b904696b536399e597d1`](33b937b),
[`09cd3e55e810ee513127b7440f5b11af7540c9b0`](09cd3e5),
[`d7189a86c27891af408d3d0184db6133d3342967`](d7189a8)]:
    -   @apollo/federation-internals@2.7.2
    -   @apollo/query-graphs@2.7.2

## @apollo/subgraph@2.7.2

### Patch Changes

- Updated dependencies
\[[`33b937b18d3c7ca6af14b904696b536399e597d1`](33b937b),
[`09cd3e55e810ee513127b7440f5b11af7540c9b0`](09cd3e5),
[`d7189a86c27891af408d3d0184db6133d3342967`](d7189a8)]:
    -   @apollo/federation-internals@2.7.2

## apollo-federation-integration-testsuite@2.7.2

### Patch Changes

- Add new `generateQueryFragments` option to query planner config
([#2958](#2958))

If enabled, the query planner will extract inline fragments into
fragment definitions before sending queries to subgraphs. This can
significantly reduce the size of the query sent to subgraphs, but may
increase the time it takes to plan the query.

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
Reviewers

@sachindshinde sachindshinde sachindshinde approved these changes

@dariuszkuc dariuszkuc dariuszkuc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@trevor-scheer @sachindshinde @dariuszkuc