Additional `SourceSpecDefinition` validations #2914

benjamn · 2024-01-23T16:58:53Z

Follow-up to PR #2910.

…chema.

codesandbox-ci · 2024-01-23T16:59:43Z

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.

internals-js/src/error.ts

internals-js/src/specs/sourceSpec.ts

#2914 (comment)

clenfest · 2024-01-23T19:01:24Z

internals-js/src/specs/sourceSpec.ts

+    if (result.sourceAPI || result.sourceType || result.sourceField) {
+      // Since this subgraph uses at least one of the @source{API,Type,Field}
+      // directives, it must also use v2.7 or later of federation.
+      if (!federationVersion || federationVersion.lt(this.minimumFederationVersion)) {


I don't think this error is necessary. I think it's fine to just not validate rather than throw an error. Not sure that this could ever happen.

Without this enforcement, there's a cryptic error later on because @join__directive isn't added to the supergraph schema unless federation v2.7+ is in use.

Specifically, joinDirective ends up undefined here:

federation/composition-js/src/merging/merge.ts

Lines 2747 to 2750 in 6ae4294

const joinDirective = this.joinSpec.directiveDirective(this.merged);

Object.keys(joinsByDirectiveName).forEach(directiveName => {

joinsByDirectiveName[directiveName].forEach(join => {

dest.applyDirective(joinDirective, {

when it wasn't added to the schema here:

federation/internals-js/src/specs/joinSpec.ts

Lines 137 to 143 in 6ae4294

if (this.version.gte(new FeatureVersion(0, 4))) {

const joinDirective = this.addDirective(schema, 'directive').addLocations(

DirectiveLocation.SCHEMA,

DirectiveLocation.OBJECT,

DirectiveLocation.INTERFACE,

DirectiveLocation.FIELD_DEFINITION,

);

I agree with @lennyburdette that not using federation v2.7 is an easy mistake to make, and this error should make the problem easier to diagnose.

All directives have an issue where if you don't specify the right version composition will say that it doesn't understand the directive. Not sure why this is a special case.

because the source directives are imported from their own link instead of from the federation link. it's a little weird, but i'm ok with leaning into it because enforcing a minimum version of 2.7 should minimize the number of unhappy paths:

@link federation < 2.7 @link federation >= 2.7

composition < 2.7 using source is silently ignored (🦶🏻 🔫 , but can't fix) composition fails due to link being ahead of composition

composition >= 2.7 this scenario which we're disallowing happy path

Ok, I got it. Can you make an issue to solve this for the general case? Might be a good starter project for our new hire.

clenfest

main has been updated, so you want to merge there rather than next (which is in theory 2.8 now).

internals-js/src/specs/sourceSpec.ts

#2914 (comment)

netlify · 2024-01-24T16:29:24Z

✅ Deploy Preview for apollo-federation-docs ready!

Name	Link
🔨 Latest commit	`066783f`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-federation-docs/deploys/65b13c1badc8ff0009cc36b1
😎 Deploy Preview	https://deploy-preview-2914--apollo-federation-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

…dations

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/[email protected] ### Patch Changes - Updated dependencies \[[`493f5acd16ad92adf99c963659cd40dc5eac1219`](493f5ac)]: - @apollo/[email protected] - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Updated dependencies \[[`493f5acd16ad92adf99c963659cd40dc5eac1219`](493f5ac)]: - @apollo/[email protected] - @apollo/[email protected] - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Additional `SourceSpecDefinition` validations and bug fixes ([#2914](#2914)) ## @apollo/[email protected] ### Patch Changes - Updated dependencies \[[`493f5acd16ad92adf99c963659cd40dc5eac1219`](493f5ac)]: - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Updated dependencies \[[`493f5acd16ad92adf99c963659cd40dc5eac1219`](493f5ac)]: - @apollo/[email protected] - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Updated dependencies \[[`493f5acd16ad92adf99c963659cd40dc5eac1219`](493f5ac)]: - @apollo/[email protected] ## [email protected] Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

… directive (#2929) # Overview The recent addition of `SourceSpecDefinition` surfaced a case where a subgraph with a `@link` to federation < 2.7 and a `@link` to the source spec (which requires federation >= 2.7) results in an unexpected error during composition. Validation for this case was introduced in #2914, raising a composition error when the versions are mismatched. Since the existing behavior will implicitly upgrade to a later minor version of federation when two subgraphs are mismatched, it makes sense to do the same with feature requirements. # Example Subgraph A has a `@link` to fed version 2.5 Subgraph B has a `@link` to fed version 2.7 Subgraph C has a `@link` to fed version 2.5 and a separate `@link` to the source spec Previous behavior: 1. Composing Subgraph A and Subgraph B silently upgrades to fed version 2.7 2. Composing Subgraph A and Subgraph C results in a composition error New behavior: 1. Composing Subgraph A and Subgraph B silently upgrades to fed version 2.7 2. Composing Subgraph A and Subgraph C upgrades to fed version 2.7 and raises a hint explaining why --------- Co-authored-by: Chris Lenfest <[email protected]>

benjamn added 4 commits January 23, 2024 11:16

Enforce v2.7+ of federation is @link-imported when @source* used.

b714123

Allow hyphens in @sourceAPI name.

f13cd22

Better exception reporting for SourceSpecDefinition.validateSubgraphS…

219e2c6

…chema.

Don't require header.as or header.value (header.name alone works).

c3d8b3c

benjamn self-assigned this Jan 23, 2024

This comment was marked as resolved.

Sign in to view

benjamn requested review from lennyburdette, o0Ignition0o and clenfest January 23, 2024 17:25

benjamn added 2 commits January 23, 2024 12:37

Keep validating after API name mismatch for @source{Type,Field}.

e41715f

Validate header.as is valid HTTP header name.

e81bdad

lennyburdette reviewed Jan 23, 2024

View reviewed changes

internals-js/src/error.ts Show resolved Hide resolved

o0Ignition0o reviewed Jan 23, 2024

View reviewed changes

internals-js/src/specs/sourceSpec.ts Outdated Show resolved Hide resolved

benjamn added 4 commits January 23, 2024 13:09

Add more badSchema validation error tests.

3282924

Better federation version validation error message.

264134e

#2914 (comment)

Even more precise federation version validation message.

7ced763

#2914 (comment)

Add changeset

b9d3b3c

benjamn marked this pull request as ready for review January 23, 2024 18:27

benjamn requested review from a team as code owners January 23, 2024 18:27

benjamn requested review from lennyburdette and o0Ignition0o January 23, 2024 18:31

o0Ignition0o approved these changes Jan 23, 2024

View reviewed changes

clenfest reviewed Jan 23, 2024

View reviewed changes

Fix rogue test in Node.js v14.

4079b9a

clenfest reviewed Jan 23, 2024

View reviewed changes

benjamn changed the base branch from next to main January 23, 2024 19:49

clenfest reviewed Jan 24, 2024

View reviewed changes

internals-js/src/specs/sourceSpec.ts Outdated Show resolved Hide resolved

Report only nodes: application.sourceAST for consistency.

16a346b

#2914 (comment)

clenfest approved these changes Jan 24, 2024

View reviewed changes

benjamn enabled auto-merge (squash) January 24, 2024 16:34

Merge branch 'main' into benjamn/additional-SourceSpecDefinition-vali…

066783f

…dations

benjamn merged commit 493f5ac into main Jan 24, 2024
17 checks passed

benjamn deleted the benjamn/additional-SourceSpecDefinition-validations branch January 24, 2024 16:39

github-actions bot mentioned this pull request Jan 24, 2024

release: on branch main #2913

Merged

tninesling mentioned this pull request Feb 2, 2024

Implicitly upgrade federation version to the max required by any used directive #2929

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional `SourceSpecDefinition` validations #2914

Additional `SourceSpecDefinition` validations #2914

benjamn commented Jan 23, 2024

This comment was marked as resolved.

codesandbox-ci bot commented Jan 23, 2024 •

edited

Loading

clenfest Jan 23, 2024

benjamn Jan 23, 2024

clenfest Jan 23, 2024

lennyburdette Jan 23, 2024 •

edited

Loading

clenfest Jan 23, 2024

clenfest left a comment

netlify bot commented Jan 24, 2024 •

edited

Loading

	const joinDirective = this.joinSpec.directiveDirective(this.merged);
	Object.keys(joinsByDirectiveName).forEach(directiveName => {
	joinsByDirectiveName[directiveName].forEach(join => {
	dest.applyDirective(joinDirective, {

	if (this.version.gte(new FeatureVersion(0, 4))) {
	const joinDirective = this.addDirective(schema, 'directive').addLocations(
	DirectiveLocation.SCHEMA,
	DirectiveLocation.OBJECT,
	DirectiveLocation.INTERFACE,
	DirectiveLocation.FIELD_DEFINITION,
	);

	`@link` federation < 2.7	`@link` federation >= 2.7
composition < 2.7	using source is silently ignored (🦶🏻 🔫 , but can't fix)	composition fails due to link being ahead of composition
composition >= 2.7	this scenario which we're disallowing	happy path

Additional SourceSpecDefinition validations #2914

Additional SourceSpecDefinition validations #2914

Conversation

benjamn commented Jan 23, 2024

This comment was marked as resolved.

codesandbox-ci bot commented Jan 23, 2024 • edited Loading

clenfest Jan 23, 2024

Choose a reason for hiding this comment

benjamn Jan 23, 2024

Choose a reason for hiding this comment

clenfest Jan 23, 2024

Choose a reason for hiding this comment

lennyburdette Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

clenfest Jan 23, 2024

Choose a reason for hiding this comment

clenfest left a comment

Choose a reason for hiding this comment

netlify bot commented Jan 24, 2024 • edited Loading

✅ Deploy Preview for apollo-federation-docs ready!

Additional `SourceSpecDefinition` validations #2914

Additional `SourceSpecDefinition` validations #2914

codesandbox-ci bot commented Jan 23, 2024 •

edited

Loading

lennyburdette Jan 23, 2024 •

edited

Loading

netlify bot commented Jan 24, 2024 •

edited

Loading