diff --git a/federation-js/CHANGELOG.md b/federation-js/CHANGELOG.md index 9ee88e39c..855e7b70e 100644 --- a/federation-js/CHANGELOG.md +++ b/federation-js/CHANGELOG.md @@ -4,7 +4,7 @@ > The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section. -- _Nothing yet! Stay tuned!_ +- Add flexibility for @tag directive definition validation in subgraphs. @tag definitions are now permitted to be a subset of the spec's definition. This means that within the definition, `repeatable` is optional as are each of the directive locations. [PR #1022](https://github.com/apollographql/federation/pull/1022) ## v0.32.0 diff --git a/federation-js/src/composition/utils.ts b/federation-js/src/composition/utils.ts index cf0c2b452..58385566e 100644 --- a/federation-js/src/composition/utils.ts +++ b/federation-js/src/composition/utils.ts @@ -32,6 +32,8 @@ import { isDirective, isNamedType, stripIgnoredCharacters, + NonNullTypeNode, + NamedTypeNode, } from 'graphql'; import { ExternalFieldDefinition, @@ -56,6 +58,14 @@ export function isDirectiveDefinitionNode(node: any): node is DirectiveDefinitio return node.kind === Kind.DIRECTIVE_DEFINITION; } +export function isNonNullTypeNode(node: any): node is NonNullTypeNode { + return node.kind === Kind.NON_NULL_TYPE; +} + +export function isNamedTypeNode(node: any): node is NamedTypeNode { + return node.kind === Kind.NAMED_TYPE; +} + // Create a map of { fieldName: serviceName } for each field. export function mapFieldNamesToServiceName( fields: ReadonlyArray, @@ -139,14 +149,6 @@ export function stripExternalFieldsFromTypeDefs( return { typeDefsWithoutExternalFields, strippedFields }; } -export function stripDescriptions(astNode: ASTNode) { - return visit(astNode, { - enter(node) { - return 'description' in node ? { ...node, description: undefined } : node; - }, - }); -} - export function stripTypeSystemDirectivesFromTypeDefs(typeDefs: DocumentNode) { const typeDefsWithoutTypeSystemDirectives = visit(typeDefs, { Directive(node) { diff --git a/federation-js/src/composition/validate/preNormalization/__tests__/tagDirective.test.ts b/federation-js/src/composition/validate/preNormalization/__tests__/tagDirective.test.ts index b836b39fe..f31f6f1b4 100644 --- a/federation-js/src/composition/validate/preNormalization/__tests__/tagDirective.test.ts +++ b/federation-js/src/composition/validate/preNormalization/__tests__/tagDirective.test.ts @@ -65,6 +65,21 @@ describe('tagDirective', () => { const errors = tagDirective(serviceA); expect(errors).toHaveLength(0); }); + + it('permits alternative, compatible @tag definitions', () => { + const serviceA = { + typeDefs: gql` + directive @tag(name: String!) on FIELD_DEFINITION | INTERFACE + type Query { + hello: String @tag(name: "hello") + } + `, + name: 'serviceA', + }; + + const errors = tagDirective(serviceA); + expect(errors).toHaveLength(0); + }); }); describe('reports errors', () => { @@ -99,35 +114,130 @@ describe('tagDirective', () => { `); }); - it('when @tag usage and definition exist, but definition is incorrect', () => { - const serviceA = { - typeDefs: gql` - directive @tag(name: String!) on FIELD_DEFINITION + describe('incompatible definition', () => { + it('locations incompatible', () => { + const serviceA = { + typeDefs: gql` + directive @tag(name: String!) on FIELD_DEFINITION | SCHEMA - type Query { - hello: String @tag(name: "hello") - } - `, - name: 'serviceA', - }; + type Query { + hello: String @tag(name: "hello") + } + `, + name: 'serviceA', + }; - const errors = tagDirective(serviceA); + const errors = tagDirective(serviceA); - expect(errors).toMatchInlineSnapshot(` - Array [ - Object { - "code": "TAG_DIRECTIVE_DEFINITION_INVALID", - "locations": Array [ - Object { - "column": 1, - "line": 2, - }, - ], - "message": "[@tag] -> Found @tag definition in service serviceA, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions matches the following: - directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION", - }, - ] - `); + expect(errors).toMatchInlineSnapshot(` + Array [ + Object { + "code": "TAG_DIRECTIVE_DEFINITION_INVALID", + "locations": Array [ + Object { + "column": 1, + "line": 2, + }, + ], + "message": "[@tag] -> Found @tag definition in service serviceA, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions is compatible with the following: + directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION", + }, + ] + `); + }); + + it('name arg missing', () => { + const serviceA = { + typeDefs: gql` + directive @tag on FIELD_DEFINITION + + type Query { + hello: String @tag + } + `, + name: 'serviceA', + }; + + const errors = tagDirective(serviceA); + + expect(errors).toMatchInlineSnapshot(` + Array [ + Object { + "code": "TAG_DIRECTIVE_DEFINITION_INVALID", + "locations": Array [ + Object { + "column": 1, + "line": 2, + }, + ], + "message": "[@tag] -> Found @tag definition in service serviceA, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions is compatible with the following: + directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION", + }, + ] + `); + }); + + it('name arg incompatible', () => { + const serviceA = { + typeDefs: gql` + directive @tag(name: String) on FIELD_DEFINITION + + type Query { + hello: String @tag(name: "hello") + } + `, + name: 'serviceA', + }; + + const errors = tagDirective(serviceA); + + expect(errors).toMatchInlineSnapshot(` + Array [ + Object { + "code": "TAG_DIRECTIVE_DEFINITION_INVALID", + "locations": Array [ + Object { + "column": 1, + "line": 2, + }, + ], + "message": "[@tag] -> Found @tag definition in service serviceA, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions is compatible with the following: + directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION", + }, + ] + `); + }); + + it('additional args', () => { + const serviceA = { + typeDefs: gql` + directive @tag(name: String!, additional: String) on FIELD_DEFINITION + + type Query { + hello: String @tag(name: "hello") + } + `, + name: 'serviceA', + }; + + const errors = tagDirective(serviceA); + + expect(errors).toMatchInlineSnapshot(` + Array [ + Object { + "code": "TAG_DIRECTIVE_DEFINITION_INVALID", + "locations": Array [ + Object { + "column": 1, + "line": 2, + }, + ], + "message": "[@tag] -> Found @tag definition in service serviceA, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions is compatible with the following: + directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION", + }, + ] + `); + }); }); it('when @tag usage is missing args', () => { diff --git a/federation-js/src/composition/validate/preNormalization/tagDirective.ts b/federation-js/src/composition/validate/preNormalization/tagDirective.ts index 5eb3fcefd..5dddd5487 100644 --- a/federation-js/src/composition/validate/preNormalization/tagDirective.ts +++ b/federation-js/src/composition/validate/preNormalization/tagDirective.ts @@ -1,17 +1,19 @@ -import { federationDirectives } from '../../../directives'; +import { + directiveDefinitionsAreCompatible, + federationDirectives, +} from '../../../directives'; import { DirectiveDefinitionNode, KnownDirectivesRule, visit, BREAK, - print, - NameNode, + parse, } from 'graphql'; import { KnownArgumentNamesOnDirectivesRule } from 'graphql/validation/rules/KnownArgumentNamesRule'; import { ProvidedRequiredArgumentsOnDirectivesRule } from 'graphql/validation/rules/ProvidedRequiredArgumentsRule'; import { validateSDL } from 'graphql/validation/validate'; import { ServiceDefinition } from '../../types'; -import { errorWithCode, logDirective, stripDescriptions } from '../../utils'; +import { errorWithCode, logDirective } from '../../utils'; // Likely brittle but also will be very obvious if this breaks. Based on the // content of the error message itself to remove expected errors related to @@ -47,24 +49,26 @@ export const tagDirective = ({ }, }); - // Ensure the tag directive definition is correct - if (tagDirectiveDefinition) { - const printedTagDefinition = - 'directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION'; + const printedTagDefinition = + 'directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION'; + const parsedTagDefinition = parse(printedTagDefinition) + .definitions[0] as DirectiveDefinitionNode; - if ( - print(normalizeDirectiveDefinitionNode(tagDirectiveDefinition)) !== - printedTagDefinition - ) { - errors.push( - errorWithCode( - 'TAG_DIRECTIVE_DEFINITION_INVALID', - logDirective('tag') + - `Found @tag definition in service ${serviceName}, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions matches the following:\n\t${printedTagDefinition}`, - tagDirectiveDefinition, - ), - ); - } + if ( + tagDirectiveDefinition && + !directiveDefinitionsAreCompatible( + parsedTagDefinition, + tagDirectiveDefinition, + ) + ) { + errors.push( + errorWithCode( + 'TAG_DIRECTIVE_DEFINITION_INVALID', + logDirective('tag') + + `Found @tag definition in service ${serviceName}, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions is compatible with the following:\n\t${printedTagDefinition}`, + tagDirectiveDefinition, + ), + ); } return errors.filter( @@ -72,11 +76,3 @@ export const tagDirective = ({ !errorsMessagesToFilter.some((keyWord) => message === keyWord), ); }; - -function normalizeDirectiveDefinitionNode(node: DirectiveDefinitionNode) { - // Remove descriptions from the AST - node = stripDescriptions(node); - // Sort locations alphabetically - (node.locations as NameNode[]).sort((a, b) => a.value.localeCompare(b.value)); - return node; -} diff --git a/federation-js/src/directives.ts b/federation-js/src/directives.ts index 89530f878..04d4664da 100644 --- a/federation-js/src/directives.ts +++ b/federation-js/src/directives.ts @@ -14,6 +14,10 @@ import { TypeSystemExtensionNode, TypeDefinitionNode, ExecutableDefinitionNode, + DirectiveDefinitionNode, + print, + ASTNode, + visit, } from 'graphql'; export const KeyDirective = new GraphQLDirective({ @@ -135,5 +139,48 @@ export function typeIncludesDirective( ): boolean { if (isInputObjectType(type)) return false; const directives = gatherDirectives(type as GraphQLNamedTypeWithDirectives); - return directives.some(directive => directive.name.value === directiveName); + return directives.some((directive) => directive.name.value === directiveName); +} + +export function directiveDefinitionsAreCompatible( + baseDefinition: DirectiveDefinitionNode, + toCompare: DirectiveDefinitionNode, +) { + if (baseDefinition.name.value !== toCompare.name.value) return false; + // arguments must be equal in length + if (baseDefinition.arguments?.length !== toCompare.arguments?.length) { + return false; + } + // arguments must be equal in type + for (const arg of baseDefinition.arguments ?? []) { + const toCompareArg = toCompare.arguments?.find( + (a) => a.name.value === arg.name.value, + ); + if (!toCompareArg) return false; + if ( + print(stripDescriptions(arg)) !== print(stripDescriptions(toCompareArg)) + ) { + return false; + } + } + // toCompare's locations must exist in baseDefinition's locations + if ( + toCompare.locations.some( + (location) => + !baseDefinition.locations.find( + (baseLocation) => baseLocation.value === location.value, + ), + ) + ) { + return false; + } + return true; +} + +function stripDescriptions(astNode: ASTNode) { + return visit(astNode, { + enter(node) { + return 'description' in node ? { ...node, description: undefined } : node; + }, + }); } diff --git a/harmonizer/src/lib.rs b/harmonizer/src/lib.rs index 07929c9c6..3a9d6338b 100644 --- a/harmonizer/src/lib.rs +++ b/harmonizer/src/lib.rs @@ -31,8 +31,8 @@ composition implementation while we work toward something else. #![warn(missing_docs, future_incompatible, unreachable_pub, rust_2018_idioms)] use deno_core::{op_sync, JsRuntime}; use serde::{Deserialize, Serialize}; +use std::fmt::Display; use std::sync::mpsc::channel; -use std::{fmt::Display, io::Write}; use thiserror::Error; /// The `ServiceDefinition` represents everything we need to know about a @@ -163,15 +163,10 @@ pub fn harmonize(service_list: ServiceList) -> Result, zero_copy| { - let mut out = std::io::stdout(); - - // Write the contents of every buffer to stdout - if let Some(buf) = zero_copy { - out.write_all(&buf) - .expect("failure writing buffered output"); + op_sync(|_state, maybe_msg: Option, _zero_copy| { + if let Some(msg) = maybe_msg { + println!("{}", msg); } - Ok(()) // No meaningful result }), );