merge next into main#3434
Merged
Merged
Conversation
Co-authored-by: Isaac M. Good <imgood@apollographql.com>
## Relax `@interfaceObject` validation for Fed 1 subgraphs ### What this PR does Previously, any use of `@interfaceObject` in a Fed 2 subgraph would cause an `INTERFACE_OBJECT_USAGE_ERROR` if **any** Fed 1 subgraph was present in the composition — regardless of whether the types actually conflicted. This was overly restrictive. The real problem is narrower: `@interfaceObject` on type `T` in a Fed 2 subgraph requires other subgraphs to be able to resolve `__typename` via an interface `@key` on `T`. Fed 1 subgraphs silently drop `@key` from interfaces during upgrade (the schema upgrader removes them), so they **cannot** fulfill that requirement for `T` — but they are perfectly fine for any other types. This PR replaces the blanket check with a **per-type** cross-check: - An error is only raised when a Fed 2 subgraph uses `@interfaceObject` on type `T` **and** a Fed 1 subgraph has `@key` on an interface also named `T`. - Fed 2 subgraphs using `@interfaceObject` on a type that has no corresponding interface `@key` in any Fed 1 subgraph compose successfully. ### Changes **`internals-js/src/schemaUpgrader.ts`** - `SchemaUpgrader`: added `interfaceKeyTypes: Set<string>` field to track interface types whose `@key` was removed during upgrade; the set is returned from `upgrade()` on success. - `upgradeSubgraphsIfNecessary`: replaced `subgraphsUsingInterfaceObject` with two `SetMultiMap<string, string>` instances (`fed2InterfaceObjectTypesToSubgraphs` and `fed1InterfaceKeyTypesToSubgraphs`) that are cross-checked per type name after all subgraphs are processed. **`internals-js/src/__tests__/schemaUpgrader.test.ts`** - Updated the existing `@interfaceObject` rejection test with the new, more precise error message. - Added a new test confirming that composition succeeds when the same type name does **not** have a `@key` on a Fed 1 interface. --------- Co-authored-by: Sachin D. Shinde <sachin@apollographql.com>
… instead of per-spec (#3391) ## Summary Fixes a bug in `ComposeDirectiveManager` where `@composeDirective` would fail with a false `DIRECTIVE_COMPOSITION_ERROR` when two subgraphs linked the same custom spec but imported different subsets of its directives. Closes FED-849 --- ## Problem `getLatestDirectiveDefinition()` resolved a directive's definition through a two-hop chain: ``` directiveName → spec identity → "latest" subgraph for that spec → schema.directive(...) ``` The "latest" subgraph was determined solely by its spec version (highest minor). This meant that if the latest-version subgraph had not imported the specific directive being resolved, the lookup would silently fail and produce a false error — even though another subgraph had a perfectly valid definition for it. **Failing scenarios:** ```graphql # SG1 — imports @foo AND @bar, composes both @link(url: "https://myorg.dev/myspec/v1.0", import: ["@foo", "@bar"]) @composeDirective(name: "@foo") @composeDirective(name: "@bar") # SG2 — imports only @foo from the same spec @link(url: "https://myorg.dev/myspec/v1.0", import: ["@foo"]) @composeDirective(name: "@foo") ``` If the iterator elected SG2 as "latest", resolving `@bar` would fail: ``` DIRECTIVE_COMPOSITION_ERROR: Core feature "https://myorg.dev/myspec" in subgraph "SG2" does not have a directive definition for "@bar" ``` The bug was order-dependent: it succeeded only when the elected subgraph happened to be a superset of all other subgraphs' imports. Completely disjoint imports (SG1 has `@foo`, SG2 has `@bar`) always failed regardless of order. --- ## Fix Added a `directiveDefinitionMap: Map<string, DirectiveDefinition>` built during `validate()`. For each composed directive, it finds the highest-minor-version subgraph that **actually imported** that specific directive, and stores its definition directly. `getLatestDirectiveDefinition()` now does a single lookup on this map instead of the indirect two-hop chain. **Before:** ``` directiveName → spec identity → latestSubgraph (may not have the directive!) → definition ``` **After:** ``` directiveName → definition (sourced from the subgraph that actually imported it) ``` --- ## Changes | File | Description | |---|---| | `composition-js/src/composeDirectiveManager.ts` | New `directiveDefinitionMap`, populated in `validate()`, `getLatestDirectiveDefinition()` simplified to a direct lookup | | `composition-js/src/__tests__/compose.composeDirective.test.ts` | Two new regression tests covering the subset and fully-disjoint import scenarios; one existing test updated that was asserting the previous (buggy) error behavior | --- ## Testing ```bash # Regression tests for FED-849 node_modules/.bin/jest --projects composition-js --no-coverage --testNamePattern="FED-849" # Full composition suite node_modules/.bin/jest --projects composition-js --no-coverage ``` All 15 test suites pass (400 tests, 0 failures). --------- Co-authored-by: Dariusz Kuc <9501705+dariuszkuc@users.noreply.github.com>
This PR: - Address post-merge feedback given for [#3391](#3391) (specifically, [this review](#3391 (review))). - Fixes an unrelated bug in `sourceFeature()`, where it would return the wrong name-in-spec sometimes. - Note this bug only occurs when both a `@composeDirective` spec (or the connect spec) is aliased and a directive with the same name as the spec is used (which should be rare).
…s connectors
<!--
First, 🌠 thank you 🌠 for taking the time to consider a contribution to
Apollo!
Here are some important details to follow:
* ⏰ Your time is important
To save your precious time, if the contribution you are making will
take more than an hour, please make sure it has been discussed in an
issue first. This is especially true for feature requests!
* 💡 Features
Feature requests can be created and discussed within a GitHub Issue.
Be sure to search for existing feature requests (and related issues!)
prior to opening a new request. If an existing issue covers the need,
please upvote that issue by using the 👍 emote, rather than opening a
new issue.
* 🕷 Bug fixes
These can be created and discussed in this repository. When fixing a
bug,
please _try_ to add a test which verifies the fix. If you cannot, you
should
still submit the PR but we may still ask you (and help you!) to create a
test.
* Federation versions
Please make sure you're targeting the federation version you're opening
the PR for. Federation 2 (alpha) is currently located on the `main`
branch and prior versions of Federation live on the `version-0.x`
branch.
* 📖 Contribution guidelines
Follow
https://github.com/apollographql/federation/blob/HEAD/CONTRIBUTING.md
when submitting a pull request. Make sure existing tests still pass, and
add
tests for all new behavior.
* ✏️ Explain your pull request
Describe the big picture of your changes here to communicate to what
your pull request is meant to accomplish. Provide 🔗 links 🔗 to
associated issues!
We hope you will find this to be a positive experience! Open source
contribution can be intimidating and we hope to alleviate that pain as
much
as possible. Without following these guidelines, you may be missing
context
that can help you succeed with your contribution, which is why we
encourage
discussion first. Ultimately, there is no guarantee that we will be able
to
merge your pull-request, but by following these guidelines we can try to
avoid disappointment.
-->
This PR adds `@link` validations to detect invalid alias names and alias/import conflicts. To maintain backwards compatibility, we do allow some exceptions, and we also auto-generate supergraph spec aliases when conflicts are detected.
In #3430, we try to enforce that spec aliases/name-in-schemas are valid GraphQL names. Unfortunately, there's some supergraph schemas where the aliases have `"-"` and `"."`, and that validation can cause supergraph schema loading to fail for those supergraph schemas. So instead, this PR: - Relaxes the validation to allow `-` and `.` after the first character (so almost a GraphQL name). - Does not change the logic that checks whether a spec alias is valid in the supergraph schema (this means if someone is currently using spec aliases with `"-"` and `"."`, new composition will generate an alias that's a valid GraphQL name, so in the future we should eventually drive down the number of supergraph schemas with such spec aliases). Note that `"-"` and `"."` in the spec alias means they can't use namespaced names for their spec elements (but in their cases, they were only using imports).
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 next, this PR will be updated. # Releases ## @apollo/composition@2.14.0 ### Minor Changes - Relax `@composeDirective` validation when definitions are absent in some subgraphs. ([#3422](#3422)) Previously, if some set of spec directives were being composed into the supergraph schema via `@composeDirective`, then subgraphs with the latest version of that spec would each have to declare all of those spec directive definitions. Not following this rule would often result in composition emitting a `DIRECTIVE_COMPOSITION_ERROR` error. This restriction has now been relaxed, and a definition needs to only be in at least one of those subgraphs with the latest version of that spec. As an example, the following `@composeDirective` usage could previously fail, but is now valid. ```graphql # subgraph A — composes and defines @foo and @bar extend schema # ... @link(url: "https://myorg.dev/myspec/v1.0", import: ["@foo", "@bar"]) @composeDirective(name: "@foo") @composeDirective(name: "@bar") # ... directive @foo on FIELD directive @bar on FIELD # subgraph B — composes and defines only @foo extend schema # ... @link(url: "https://myorg.dev/myspec/v1.0", import: ["@foo"]) @composeDirective(name: "@foo") # ... directive @foo on FIELD ``` ### Patch Changes - Fixed print logic when calculating the max number of elements to include in the message. Previously we were not passing ([#3424](#3424)) the current calculated length correctly leading to inclusion of additional elements in the error/hints message. - Updated dependencies \[[`21cf465d4c687daeed71635422718c3c7b7d2d0e`](21cf465), [`5b36fc6b5a494aa6983e0339713dc45a0bd031e3`](5b36fc6), [`a20279a0184d9dfbc01a806d849dc8ae22497298`](a20279a)]: - @apollo/federation-internals@2.14.0 - @apollo/query-graphs@2.14.0 ## @apollo/gateway@2.14.0 ### Minor Changes - Add opt-out anonymous deployment environment telemetry. To opt-out, set APOLLO_TELEMETRY_DISABLED=1 in your environment. ([#3379](#3379)) ### Patch Changes - Fixed print logic when calculating the max number of elements to include in the message. Previously we were not passing ([#3424](#3424)) the current calculated length correctly leading to inclusion of additional elements in the error/hints message. - Updated dependencies \[[`21cf465d4c687daeed71635422718c3c7b7d2d0e`](21cf465), [`e1fd4ac10f72bb09027995f0811ec6e0021bcd49`](e1fd4ac), [`5b36fc6b5a494aa6983e0339713dc45a0bd031e3`](5b36fc6), [`a20279a0184d9dfbc01a806d849dc8ae22497298`](a20279a)]: - @apollo/composition@2.14.0 - @apollo/federation-internals@2.14.0 - @apollo/query-planner@2.14.0 ## @apollo/federation-internals@2.14.0 ### Minor Changes - Relax `@interfaceObject` validation for Fed 1 subgraphs. ([#3392](#3392)) Previously, any use of `@interfaceObject` in a Fed 2 subgraph caused an `INTERFACE_OBJECT_USAGE_ERROR` if any Fed 1 subgraph was present in the composition, regardless of whether the types conflicted. The check is now per-type: an error is only raised when a Fed 2 subgraph uses `@interfaceObject` on type `T` **and** a Fed 1 subgraph has `@key` on an interface also named `T`. `@key` on an interface in a Fed 1 subgraph does not mean it can fulfill the `__typename`-resolution requirement that `@interfaceObject` depends on — but they are otherwise compatible with `@interfaceObject` usage on unrelated types. - Add validations for `@link` usages to prevent name conflicts. ([#3430](#3430)) This change helps to avoid ambiguity for downstream `@link`-consuming code, which previously may have found different results for a spec schema element depending on search order. If your composition fails after this change, please rename conflicting elements via `@link(import:)` and conflicting specs/features via `@link(as:)`. Note that if you were declaring `@link`s for the `https://specs.apollo.dev/tag` or `https://specs.apollo.dev/inaccessible` specs in your subgraph schema, you will need to instead import `@tag` and `@inaccessible` from the `https://specs.apollo.dev/federation` spec. This previous pattern only succeeded due to a now-fixed bug and is fragile/may lead to undesirable behavior. ### Patch Changes - Fixed print logic when calculating the max number of elements to include in the message. Previously we were not passing ([#3424](#3424)) the current calculated length correctly leading to inclusion of additional elements in the error/hints message. ## @apollo/query-graphs@2.14.0 ### Patch Changes - Updated dependencies \[[`21cf465d4c687daeed71635422718c3c7b7d2d0e`](21cf465), [`5b36fc6b5a494aa6983e0339713dc45a0bd031e3`](5b36fc6), [`a20279a0184d9dfbc01a806d849dc8ae22497298`](a20279a)]: - @apollo/federation-internals@2.14.0 ## @apollo/query-planner@2.14.0 ### Patch Changes - Updated dependencies \[[`21cf465d4c687daeed71635422718c3c7b7d2d0e`](21cf465), [`5b36fc6b5a494aa6983e0339713dc45a0bd031e3`](5b36fc6), [`a20279a0184d9dfbc01a806d849dc8ae22497298`](a20279a)]: - @apollo/federation-internals@2.14.0 - @apollo/query-graphs@2.14.0 ## @apollo/subgraph@2.14.0 ### Patch Changes - Updated dependencies \[[`21cf465d4c687daeed71635422718c3c7b7d2d0e`](21cf465), [`5b36fc6b5a494aa6983e0339713dc45a0bd031e3`](5b36fc6), [`a20279a0184d9dfbc01a806d849dc8ae22497298`](a20279a)]: - @apollo/federation-internals@2.14.0 ## apollo-federation-integration-testsuite@2.14.0 ### Minor Changes - Add opt-out anonymous deployment environment telemetry. To opt-out, set APOLLO_TELEMETRY_DISABLED=1 in your environment. ([#3379](#3379)) ### Patch Changes - Fixed print logic when calculating the max number of elements to include in the message. Previously we were not passing ([#3424](#3424)) the current calculated length correctly leading to inclusion of additional elements in the error/hints message. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.