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

Additional SourceSpecDefinition validations #2914

Merged
merged 13 commits into from
Jan 24, 2024
Merged

Additional SourceSpecDefinition validations #2914

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b714123
Enforce v2.7+ of federation is `@link`-imported when `@source*` used.
benjamn Jan 23, 2024
f13cd22
Allow hyphens in `@sourceAPI` name.
benjamn Jan 23, 2024
219e2c6
Better exception reporting for SourceSpecDefinition.validateSubgraphS…
benjamn Jan 23, 2024
c3d8b3c
Don't require header.as or header.value (header.name alone works).
benjamn Jan 23, 2024
e41715f
Keep validating after API name mismatch for `@source{Type,Field}`.
benjamn Jan 23, 2024
e81bdad
Validate header.as is valid HTTP header name.
benjamn Jan 23, 2024
3282924
Add more badSchema validation error tests.
benjamn Jan 23, 2024
264134e
Better federation version validation error message.
benjamn Jan 23, 2024
7ced763
Even more precise federation version validation message.
benjamn Jan 23, 2024
b9d3b3c
Add changeset
benjamn Jan 23, 2024
4079b9a
Fix rogue test in Node.js v14.
benjamn Jan 23, 2024
16a346b
Report only `nodes: application.sourceAST` for consistency.
benjamn Jan 24, 2024
066783f
Merge branch 'main' into benjamn/additional-SourceSpecDefinition-vali…
benjamn Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions composition-js/src/__tests__/compose.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4998,7 +4998,7 @@ describe('@source* directives', () => {

const badSchema = gql`
extend schema
@link(url: "https://specs.apollo.dev/federation/v2.7", import: ["@key"])
@link(url: "https://specs.apollo.dev/federation/v2.5", import: ["@key"])
@link(url: "https://specs.apollo.dev/source/v0.1", import: [
"@sourceAPI"
"@sourceType"
Expand Down Expand Up @@ -5046,7 +5046,11 @@ describe('@source* directives', () => {
const messages = result.errors!.map(e => e.message);

expect(messages).toContain(
'[bad] @sourceAPI(name: "A?!") must specify valid GraphQL name'
'[bad] @source{API,Type,Field} directives require @link-importing federation v2.7 or later'
);

expect(messages).toContain(
'[bad] @sourceAPI(name: "A?!") must specify name using only [a-zA-Z0-9-_] characters'
);

expect(messages).toContain(
Expand Down Expand Up @@ -5100,7 +5104,7 @@ describe('@source* directives', () => {
const messages = result.errors!.map(e => e.message);

expect(messages).toContain(
'[renamed] @api(name: "not an identifier") must specify valid GraphQL name'
'[renamed] @api(name: "not an identifier") must specify name using only [a-zA-Z0-9-_] characters'
);
});
});
Expand Down
1 change: 1 addition & 0 deletions docs/source/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ The following errors might be raised during composition:
| `SOURCE_API_HTTP_BASE_URL_INVALID` | The `@sourceAPI` directive must specify a valid http.baseURL | 2.7.0 | |
| `SOURCE_API_NAME_INVALID` | Each `@sourceAPI` directive must take a unique and valid name as an argument | 2.7.0 | |
| `SOURCE_API_PROTOCOL_INVALID` | Each `@sourceAPI` directive must specify exactly one of the known protocols | 2.7.0 | |
| `SOURCE_FEDERATION_VERSION_REQUIRED` | Schemas using `@source{API,Type,Field}` directives must @link-import v2.7 or later of federation | 2.7.1 | |
| `SOURCE_FIELD_API_ERROR` | The `api` argument of the `@sourceField` directive must match a valid `@sourceAPI` name | 2.7.0 | |
| `SOURCE_FIELD_HTTP_BODY_INVALID` | If `@sourceField` specifies http.body, it must be a valid `JSONSelection` matching available arguments and fields | 2.7.0 | |
| `SOURCE_FIELD_HTTP_METHOD_INVALID` | The `@sourceField` directive must specify at most one of `http.{GET,POST,PUT,PATCH,DELETE}` | 2.7.0 | |
Expand Down
7 changes: 7 additions & 0 deletions internals-js/src/error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,12 @@ const INTERFACE_KEY_MISSING_IMPLEMENTATION_TYPE = makeCodeDefinition(
{ addedIn: '2.3.0' },
)

const SOURCE_FEDERATION_VERSION_REQUIRED = makeCodeDefinition(
'SOURCE_FEDERATION_VERSION_REQUIRED',
'Schemas using `@source{API,Type,Field}` directives must @link-import v2.7 or later of federation',
benjamn marked this conversation as resolved.
Show resolved Hide resolved
{ addedIn: '2.7.1' },
);

const SOURCE_API_NAME_INVALID = makeCodeDefinition(
'SOURCE_API_NAME_INVALID',
'Each `@sourceAPI` directive must take a unique and valid name as an argument',
Expand Down Expand Up @@ -757,6 +763,7 @@ export const ERRORS = {
INTERFACE_KEY_NOT_ON_IMPLEMENTATION,
INTERFACE_KEY_MISSING_IMPLEMENTATION_TYPE,
// Errors related to @sourceAPI, @sourceType, and/or @sourceField
SOURCE_FEDERATION_VERSION_REQUIRED,
SOURCE_API_NAME_INVALID,
SOURCE_API_PROTOCOL_INVALID,
SOURCE_API_HTTP_BASE_URL_INVALID,
Expand Down
84 changes: 57 additions & 27 deletions internals-js/src/specs/sourceSpec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { DirectiveLocation, GraphQLError, assertName } from 'graphql';
import { DirectiveLocation, GraphQLError } from 'graphql';
import { FeatureDefinition, FeatureDefinitions, FeatureUrl, FeatureVersion, LinkDirectiveArgs } from "./coreSpec";
import {
Schema,
Expand All @@ -16,7 +16,7 @@
export const sourceIdentity = 'https://specs.apollo.dev/source';

export class SourceSpecDefinition extends FeatureDefinition {
constructor(version: FeatureVersion, minimumFederationVersion?: FeatureVersion) {
constructor(version: FeatureVersion, readonly minimumFederationVersion: FeatureVersion) {
super(new FeatureUrl(sourceIdentity, 'source', version), minimumFederationVersion);

this.registerDirective(createDirectiveSpecification({
Expand Down Expand Up @@ -147,17 +147,20 @@
return this.directive<SourceFieldDirectiveArgs>(schema, 'sourceField')!;
}

private getSourceDirectives(schema: Schema) {
private getSourceDirectives(schema: Schema, errors: GraphQLError[]) {
const result: {
sourceAPI?: DirectiveDefinition<SourceAPIDirectiveArgs>;
sourceType?: DirectiveDefinition<SourceTypeDirectiveArgs>;
sourceField?: DirectiveDefinition<SourceFieldDirectiveArgs>;
} = {};

let federationVersion: FeatureVersion | undefined;

schema.schemaDefinition.appliedDirectivesOf<LinkDirectiveArgs>('link')
.forEach(linkDirective => {
const { url, import: imports } = linkDirective.arguments();
if (imports && FeatureUrl.maybeParse(url)?.identity === sourceIdentity) {
const featureUrl = FeatureUrl.maybeParse(url);
if (imports && featureUrl && featureUrl.identity === sourceIdentity) {
imports.forEach(nameOrRename => {
const originalName = typeof nameOrRename === 'string' ? nameOrRename : nameOrRename.name;
const importedName = typeof nameOrRename === 'string' ? nameOrRename : nameOrRename.as || originalName;
Expand All @@ -172,17 +175,33 @@
}
});
}
if (featureUrl && featureUrl.name === 'federation') {
federationVersion = featureUrl.version;
}
});

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@lennyburdette lennyburdette Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

errors.push(ERRORS.SOURCE_FEDERATION_VERSION_REQUIRED.err(

Check warning on line 187 in internals-js/src/specs/sourceSpec.ts

View check run for this annotation

Codecov / codecov/patch

internals-js/src/specs/sourceSpec.ts#L187 

Added line #L187 was not covered by tests
`@source{API,Type,Field} directives require @link-importing federation ${
this.minimumFederationVersion
} or later`,
));
benjamn marked this conversation as resolved.
Show resolved Hide resolved
}
}

return result;
}

override validateSubgraphSchema(schema: Schema): GraphQLError[] {
const errors = super.validateSubgraphSchema(schema);
const {
sourceAPI,
sourceType,
sourceField,
} = this.getSourceDirectives(schema);
} = this.getSourceDirectives(schema, errors);

if (!(sourceAPI || sourceType || sourceField)) {
// If none of the @source* directives are present, nothing needs
Expand All @@ -191,7 +210,6 @@
}

const apiNameToProtocol = new Map<string, ProtocolName>();
const errors: GraphQLError[] = [];

if (sourceAPI) {
this.validateSourceAPI(sourceAPI, apiNameToProtocol, errors);
Expand All @@ -216,20 +234,18 @@
sourceAPI.applications().forEach(application => {
const { name, ...rest } = application.arguments();

if (apiNameToProtocol.has(name)) {
if (!isValidSourceAPIName(name)) {
errors.push(ERRORS.SOURCE_API_NAME_INVALID.err(
`${sourceAPI} must specify unique name`,
`${sourceAPI}(name: ${
JSON.stringify(name)
}) must specify name using only [a-zA-Z0-9-_] characters`,
{ nodes: application.sourceAST },
));
}

try {
assertName(name);
} catch (e) {
if (apiNameToProtocol.has(name)) {
errors.push(ERRORS.SOURCE_API_NAME_INVALID.err(
`${sourceAPI}(name: ${
JSON.stringify(name)
}) must specify valid GraphQL name`,
`${sourceAPI} must specify unique name (${JSON.stringify(name)} reused)`,
{ nodes: application.sourceAST },
));
}
Expand All @@ -239,7 +255,9 @@
if (rest[knownProtocol]) {
if (protocol) {
errors.push(ERRORS.SOURCE_API_PROTOCOL_INVALID.err(
`${sourceAPI} must specify only one of ${KNOWN_SOURCE_PROTOCOLS.join(', ')}`,
`${sourceAPI} must specify only one of ${
KNOWN_SOURCE_PROTOCOLS.join(', ')
} but specified both ${protocol} and ${knownProtocol}`,
{ nodes: application.sourceAST },
));
}
Expand All @@ -258,7 +276,7 @@
new URL(baseURL);
} catch (e) {
errors.push(ERRORS.SOURCE_API_HTTP_BASE_URL_INVALID.err(
`${sourceAPI} http.baseURL ${JSON.stringify(baseURL)} must be valid URL`,
`${sourceAPI} http.baseURL ${JSON.stringify(baseURL)} must be valid URL (error: ${e.message})`,
{ nodes: application.sourceAST },
));
}
Expand Down Expand Up @@ -314,7 +332,7 @@
parseURLPathTemplate(urlPathTemplate);
} catch (e) {
errors.push(ERRORS.SOURCE_TYPE_HTTP_PATH_INVALID.err(
`${sourceType} http.GET or http.POST must be valid URL path template`
`${sourceType} http.GET or http.POST must be valid URL path template (error: ${e.message})`
));
}
}
Expand All @@ -334,7 +352,7 @@
// TODO Validate body selection matches the available fields.
} catch (e) {
errors.push(ERRORS.SOURCE_TYPE_HTTP_BODY_INVALID.err(
`${sourceType} http.body not valid JSONSelection: ${e.message}`,
`${sourceType} http.body not valid JSONSelection (error: ${e.message})`,
{ nodes: application.sourceAST },
));
}
Expand All @@ -357,7 +375,7 @@
// TODO Validate selection is valid JSONSelection for type.
} catch (e) {
errors.push(ERRORS.SOURCE_TYPE_SELECTION_INVALID.err(
`${sourceType} selection not valid JSONSelection: ${e.message}`,
`${sourceType} selection not valid JSONSelection (error: ${e.message})`,
{ nodes: application.sourceAST },
));
}
Expand Down Expand Up @@ -406,7 +424,7 @@
parseURLPathTemplate(urlPathTemplate);
} catch (e) {
errors.push(ERRORS.SOURCE_FIELD_HTTP_PATH_INVALID.err(
`${sourceField} http.{GET,POST,PUT,PATCH,DELETE} must be valid URL path template`
`${sourceField} http.{GET,POST,PUT,PATCH,DELETE} must be valid URL path template (error: ${e.message})`
));
}
}
Expand All @@ -432,7 +450,7 @@
// parent type and/or argument names of the field.
} catch (e) {
errors.push(ERRORS.SOURCE_FIELD_HTTP_BODY_INVALID.err(
`${sourceField} http.body not valid JSONSelection: ${e.message}`,
`${sourceField} http.body not valid JSONSelection (error: ${e.message})`,
{ nodes: application.sourceAST },
));
}
Expand All @@ -447,7 +465,7 @@
// the parent type and/or argument names of the field.
} catch (e) {
errors.push(ERRORS.SOURCE_FIELD_SELECTION_INVALID.err(
`${sourceField} selection not valid JSONSelection: ${e.message}`,
`${sourceField} selection not valid JSONSelection (error: ${e.message})`,
{ nodes: application.sourceAST },
));
}
Expand All @@ -459,14 +477,18 @@
if (fieldParent.sourceAST?.kind !== "FieldDefinition") {
errors.push(ERRORS.SOURCE_FIELD_NOT_ON_ROOT_OR_ENTITY_FIELD.err(
`${sourceField} must be applied to field`,
{ nodes: application.sourceAST },
{ nodes: [application.sourceAST!, fieldParent.sourceAST!] },
));
} else {
const typeGrandparent = fieldParent.parent as SchemaElement<any, any>;
if (typeGrandparent.sourceAST?.kind !== "ObjectTypeDefinition") {
errors.push(ERRORS.SOURCE_FIELD_NOT_ON_ROOT_OR_ENTITY_FIELD.err(
`${sourceField} must be applied to field of object type`,
{ nodes: application.sourceAST },
{ nodes: [
benjamn marked this conversation as resolved.
Show resolved Hide resolved
application.sourceAST!,
fieldParent.sourceAST,
typeGrandparent.sourceAST!,
]},
));
} else {
const typeGrandparentName = typeGrandparent.sourceAST?.name.value;
Expand All @@ -477,7 +499,11 @@
) {
errors.push(ERRORS.SOURCE_FIELD_NOT_ON_ROOT_OR_ENTITY_FIELD.err(
`${sourceField} must be applied to root Query or Mutation field or field of entity type`,
{ nodes: application.sourceAST },
{ nodes: [
application.sourceAST!,
fieldParent.sourceAST,
typeGrandparent.sourceAST!,
]},
));
}
}
Expand All @@ -486,6 +512,10 @@
}
}

function isValidSourceAPIName(name: string): boolean {
return /^[a-z-_][a-z0-9-_]*$/i.test(name);

Check warning on line 516 in internals-js/src/specs/sourceSpec.ts

View check run for this annotation

Codecov / codecov/patch

internals-js/src/specs/sourceSpec.ts#L515-L516 

Added lines #L515 - L516 were not covered by tests
}

function isValidHTTPHeaderName(name: string): boolean {
// https://developers.cloudflare.com/rules/transform/request-header-modification/reference/header-format/
return /^[a-zA-Z0-9-_]+$/.test(name);
Expand All @@ -510,9 +540,9 @@
));
}

if (!as === !value) {
if (as && value) {
errors.push(ERRORS.SOURCE_HTTP_HEADERS_INVALID.err(
`${directiveName} headers[${i}] must specify exactly one of as or value`,
`${directiveName} header ${headers[i]} should specify at most one of 'as' or 'value'`,
));
}

Expand Down