-
Notifications
You must be signed in to change notification settings - Fork 249
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
Handle edge cases with subgraph extraction logic #3136
Conversation
🦋 Changeset detectedLatest commit: 9626d3d The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
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 |
✅ Deploy Preview for apollo-federation-docs canceled.
|
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. |
@@ -95,7 +103,27 @@ export function validateSupergraph(supergraph: Schema): [CoreFeatures, JoinSpecD | |||
throw ERRORS.INVALID_FEDERATION_SUPERGRAPH.err( | |||
`Invalid supergraph: uses unsupported join spec version ${joinFeature.url.version} (supported versions: ${JOIN_VERSIONS.versions().join(', ')})`); | |||
} | |||
return [coreFeatures, joinSpec]; | |||
|
|||
const contextFeature = coreFeatures.getByIdentity(ContextSpecDefinition.identity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function seemed like the most natural place to fetch the context and core specs (if present), as it's also where we fetch the join spec (it's also the place we error if we do not know our own specs' versions).
* when a custom directive's name conflicts with that of a default one. | ||
*/ | ||
function getApolloDirectiveNames(supergraph: Schema): Record<string, string> { | ||
const originalDirectiveNames: Record<string, string> = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic in this function is already performed by FeatureDefinition.directive()
, and there's a pre-existing pattern of adding helper methods to the spec's file to call FeatureDefinition.directive()
. In this PR, we accordingly add costDirective()
and listSizeDirective()
to costSpec.ts
.
Also, note the logic in this function:
- Is checking identity via
identity.includes("specs.apollo.dev")
instead of strict equality. - Does not check for spec renaming via
as
(which is separate from importas
).
Shifting to FeatureDefinition.directive()
will address these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tninesling @sachindshinde to triple check, this fix was also mirrored in rust, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
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.9.1 ### Patch Changes - Fix bugs in composition when merging nulls in directive applications and when handling renames. ([#3134](#3134)) - Updated dependencies \[[`b8e4ab5352a4dfd262af49493fdd42e86e5e3d99`](b8e4ab5), [`e6c05b6c96023aa3dec79889431f8217fcb3806d`](e6c05b6)]: - @apollo/federation-internals@2.9.1 - @apollo/query-graphs@2.9.1 ## @apollo/gateway@2.9.1 ### Patch Changes - Fix bugs in composition when merging nulls in directive applications and when handling renames. ([#3134](#3134)) - Updated dependencies \[[`b8e4ab5352a4dfd262af49493fdd42e86e5e3d99`](b8e4ab5), [`e6c05b6c96023aa3dec79889431f8217fcb3806d`](e6c05b6)]: - @apollo/federation-internals@2.9.1 - @apollo/composition@2.9.1 - @apollo/query-planner@2.9.1 ## @apollo/federation-internals@2.9.1 ### Patch Changes - Fix edge cases for subgraph extraction logic when using spec renaming or specs URLs that look similar to `specs.apollo.dev`. ([#3136](#3136)) - Fix bugs in composition when merging nulls in directive applications and when handling renames. ([#3134](#3134)) ## @apollo/query-graphs@2.9.1 ### Patch Changes - Updated dependencies \[[`b8e4ab5352a4dfd262af49493fdd42e86e5e3d99`](b8e4ab5), [`e6c05b6c96023aa3dec79889431f8217fcb3806d`](e6c05b6)]: - @apollo/federation-internals@2.9.1 ## @apollo/query-planner@2.9.1 ### Patch Changes - Updated dependencies \[[`b8e4ab5352a4dfd262af49493fdd42e86e5e3d99`](b8e4ab5), [`e6c05b6c96023aa3dec79889431f8217fcb3806d`](e6c05b6)]: - @apollo/federation-internals@2.9.1 - @apollo/query-graphs@2.9.1 ## @apollo/subgraph@2.9.1 ### Patch Changes - Updated dependencies \[[`b8e4ab5352a4dfd262af49493fdd42e86e5e3d99`](b8e4ab5), [`e6c05b6c96023aa3dec79889431f8217fcb3806d`](e6c05b6)]: - @apollo/federation-internals@2.9.1 ## apollo-federation-integration-testsuite@2.9.1 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
While reviewing recent code changes that released in
2.9.0
, I noticed a few bugs in the code. This PR contains fixes for subgraph extraction bugs.Specifically, this PR updates subgraph extraction to use pre-existing functions/patterns instead of repeating logic, to avoid copy/pasting and bugs/divergence (the copied logic wasn't looking at spec renaming, for example).