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.

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

Fix @composeDirective corner case #2046

Merged
merged 4 commits into from
Aug 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,6 @@ describe('composing custom core directives', () => {
typeDefs: gql`
extend schema
@link(url: "https://specs.apollo.dev/federation/v2.1", import: ["@key", "@composeDirective"])
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/foo/v1.0", import: [{ name: "@foo", as: "@inaccessible" }])
@composeDirective(name: "@inaccessible")

Expand Down
2 changes: 1 addition & 1 deletion composition-js/src/__tests__/compose.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2467,7 +2467,7 @@ describe('composition', () => {
expect(userType?.field('name')?.appliedDirectivesOf('inaccessible').pop()).toBeDefined();
});

describe('rejects @inaccessible and @external together', () => {
it('rejects @inaccessible and @external together', () => {
const subgraphA = {
typeDefs: gql`
type Query {
Expand Down
24 changes: 19 additions & 5 deletions internals-js/src/buildSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export function buildSchemaFromAST(
// is that:
// 1. we can (enum values are self-contained and cannot reference anything that may need to be imported first; this
// is also why we skip directive applications at that point, as those _may_ reference something that hasn't been imported yet)
// 2. this allows the code to handle better the case where the `link__Purpose` enum is provided in the AST despite the `@link`
// 2. this allows the code to handle better the case where the `link__Purpose` enum is provided in the AST despite the `@link`
// _definition_ not being provided. And the reason that is true is that as we later _add_ the `@link` definition, we
// will need to check if `link_Purpose` needs to be added or not, but when it is already present, we check it's definition
// is the expected, but that check will unexpected fail if we haven't finished "building" said type definition.
Expand Down Expand Up @@ -215,7 +215,7 @@ function buildNamedTypeAndDirectivesShallow(documentNode: DocumentNode, schema:
let type = schema.type(definitionNode.name.value);
// Note that the type may already exists due to an extension having been processed first, but we know we
// have seen 2 definitions (which is invalid) if the definition has `preserverEmptyDefnition` already set
// since it's only set for definitions, not extensions.
// since it's only set for definitions, not extensions.
// Also note that we allow to redefine built-ins.
if (!type || type.isBuiltIn) {
type = schema.addType(newNamedType(withoutTrailingDefinition(definitionNode.kind), definitionNode.name.value));
Expand Down Expand Up @@ -332,9 +332,23 @@ function buildAppliedDirectives(
for (const directive of elementNode.directives ?? []) {
withNodeAttachedToError(
() => {
const d = element.applyDirective(directive.name.value, buildArgs(directive));
d.setOfExtension(extension);
d.sourceAST = directive;
/**
* If we are at the schemaDefinition level of a federation schema, it's possible that some directives
* will not be added until after the federation calls completeSchema. In that case, we want to wait
* until after completeSchema is called before we try to apply those directives.
*/
if (element !== element.schema().schemaDefinition || directive.name.value === 'link' || !element.schema().blueprint.applyDirectivesAfterParsing()) {
const d = element.applyDirective(directive.name.value, buildArgs(directive));
d.setOfExtension(extension);
d.sourceAST = directive;
} else {
element.addUnappliedDirective({
extension,
directive,
args: buildArgs(directive),
nameOrDef: directive.name.value,
});
}
},
directive,
errors,
Expand Down
33 changes: 32 additions & 1 deletion internals-js/src/definitions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ import {
SchemaDefinitionNode,
TypeDefinitionNode,
DefinitionNode,
DirectiveDefinitionNode
DirectiveDefinitionNode,
DirectiveNode,
} from "graphql";
import {
CoreImport,
Expand Down Expand Up @@ -522,11 +523,37 @@ export class Extension<TElement extends ExtendableElement> {
}
}

type UnappliedDirective = {
nameOrDef: DirectiveDefinition<Record<string, any>> | string,
args: Record<string, any>,
extension?: Extension<any>,
directive: DirectiveNode,
};

// TODO: ideally, we should hide the ctor of this class as we rely in places on the fact the no-one external defines new implementations.
export abstract class SchemaElement<TOwnType extends SchemaElement<any, TParent>, TParent extends SchemaElement<any, any> | Schema> extends Element<TParent> {
protected readonly _appliedDirectives: Directive<TOwnType>[] = [];
protected _unappliedDirectives: UnappliedDirective[] = [];
description?: string;

addUnappliedDirective({ nameOrDef, args, extension, directive }: UnappliedDirective) {
this._unappliedDirectives.push({
nameOrDef,
args: args ?? {},
extension,
directive,
});
}

processUnappliedDirectives() {
for (const { nameOrDef, args, extension, directive } of this._unappliedDirectives) {
const d = this.applyDirective(nameOrDef, args);
d.setOfExtension(extension);
d.sourceAST = directive;
}
this._unappliedDirectives = [];
}

get appliedDirectives(): readonly Directive<TOwnType>[] {
return this._appliedDirectives;
}
Expand Down Expand Up @@ -943,6 +970,10 @@ export class SchemaBlueprint {
onUnknownDirectiveValidationError(_schema: Schema, _unknownDirectiveName: string, error: GraphQLError): GraphQLError {
return error;
}

applyDirectivesAfterParsing() {
return false;
}
}

export const defaultSchemaBlueprint = new SchemaBlueprint();
Expand Down
8 changes: 7 additions & 1 deletion internals-js/src/federation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,9 @@ export class FederationBlueprint extends SchemaBlueprint {
}

onDirectiveDefinitionAndSchemaParsed(schema: Schema): GraphQLError[] {
return completeSubgraphSchema(schema);
const errors = completeSubgraphSchema(schema);
schema.schemaDefinition.processUnappliedDirectives();
return errors;
}

onInvalidation(schema: Schema) {
Expand Down Expand Up @@ -878,6 +880,10 @@ export class FederationBlueprint extends SchemaBlueprint {
}
return error;
}

applyDirectivesAfterParsing() {
return true;
}
}

function findUnusedNamedForLinkDirective(schema: Schema): string | undefined {
Expand Down