From beaa09e8e07beb9a224591f05d91e207797179db Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Fri, 9 Jul 2021 12:31:12 -0700 Subject: [PATCH 01/10] Demonstrate bug with test --- .../__tests__/composeAndValidate.test.ts | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/federation-js/src/composition/__tests__/composeAndValidate.test.ts b/federation-js/src/composition/__tests__/composeAndValidate.test.ts index 56e216341..928ff1f13 100644 --- a/federation-js/src/composition/__tests__/composeAndValidate.test.ts +++ b/federation-js/src/composition/__tests__/composeAndValidate.test.ts @@ -1038,3 +1038,72 @@ it('composition of full-SDL schemas without any errors', () => { const compositionResult = composeAndValidate([serviceA, serviceB]); expect(!compositionHasErrors(compositionResult)); }); + +it('tag directives are still included when usages are on an @external field', () => { + const inventory = { + name: 'inventory', + typeDefs: gql` + extend type Product @key(fields: "id") { + id: ID! @external @tag(name: "hi from inventory") + quantity: Int! + } + `, + }; + + const products = { + name: 'products', + typeDefs: gql` + extend type Query { + product(id: ID!): Product + } + + type Product @key(fields: "id") { + id: ID! + sku: String + } + `, + }; + + const compositionResult = composeAndValidate([inventory, products]); + assertCompositionSuccess(compositionResult); + expect(compositionResult.supergraphSdl).toMatchInlineSnapshot(` + "schema + @core(feature: \\"https://specs.apollo.dev/core/v0.1\\"), + @core(feature: \\"https://specs.apollo.dev/join/v0.1\\") + { + query: Query + } + + directive @core(feature: String!) repeatable on SCHEMA + + directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet) on FIELD_DEFINITION + + directive @join__type(graph: join__Graph!, key: join__FieldSet) repeatable on OBJECT | INTERFACE + + directive @join__owner(graph: join__Graph!) on OBJECT | INTERFACE + + directive @join__graph(name: String!, url: String!) on ENUM_VALUE + + scalar join__FieldSet + + enum join__Graph { + INVENTORY @join__graph(name: \\"inventory\\" url: \\"\\") + PRODUCTS @join__graph(name: \\"products\\" url: \\"\\") + } + + type Product + @join__owner(graph: PRODUCTS) + @join__type(graph: PRODUCTS, key: \\"id\\") + @join__type(graph: INVENTORY, key: \\"id\\") + { + id: ID! @join__field(graph: PRODUCTS) @tag(name: \\"hi from inventory\\") + quantity: Int! @join__field(graph: INVENTORY) + sku: String @join__field(graph: PRODUCTS) + } + + type Query { + product(id: ID!): Product @join__field(graph: PRODUCTS) + } + " + `); +}); From f32cf941bfb7bc77fa27018f96e931f68535b3d1 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Fri, 9 Jul 2021 12:43:38 -0700 Subject: [PATCH 02/10] Fix the bug and update test --- .../__tests__/composeAndValidate.test.ts | 5 +++- federation-js/src/composition/compose.ts | 29 +++++++++---------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/federation-js/src/composition/__tests__/composeAndValidate.test.ts b/federation-js/src/composition/__tests__/composeAndValidate.test.ts index 928ff1f13..a56fcbfe9 100644 --- a/federation-js/src/composition/__tests__/composeAndValidate.test.ts +++ b/federation-js/src/composition/__tests__/composeAndValidate.test.ts @@ -1069,7 +1069,8 @@ it('tag directives are still included when usages are on an @external field', () expect(compositionResult.supergraphSdl).toMatchInlineSnapshot(` "schema @core(feature: \\"https://specs.apollo.dev/core/v0.1\\"), - @core(feature: \\"https://specs.apollo.dev/join/v0.1\\") + @core(feature: \\"https://specs.apollo.dev/join/v0.1\\"), + @core(feature: \\"https://specs.apollo.dev/tag/v0.1\\") { query: Query } @@ -1084,6 +1085,8 @@ it('tag directives are still included when usages are on an @external field', () directive @join__graph(name: String!, url: String!) on ENUM_VALUE + directive @tag(name: String!) repeatable on FIELD_DEFINITION + scalar join__FieldSet enum join__Graph { diff --git a/federation-js/src/composition/compose.ts b/federation-js/src/composition/compose.ts index e2e82d579..0d9c4a83a 100644 --- a/federation-js/src/composition/compose.ts +++ b/federation-js/src/composition/compose.ts @@ -195,7 +195,6 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) { // Capture `@tag` and `@inaccessible` directive applications for (const field of definition.fields ?? []) { const fieldName = field.name.value; - const tagUsages = findDirectivesOnNode(field, 'tag'); const inaccessibleUsages = findDirectivesOnNode( field, @@ -355,25 +354,25 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) { // We need to capture applied directives from the external fields as well, // which are stripped and excluded from the main loop over the typeDefs for (const { parentTypeName, field } of externalFields) { - const tagDirectivesOnField = findDirectivesOnNode(field, 'tag'); - const inaccessibleDirectivesOnField = findDirectivesOnNode(field, 'inaccessible'); - - const appliedDirectivesOnField = [ - ...tagDirectivesOnField, - ...inaccessibleDirectivesOnField, - ]; - if (appliedDirectivesOnField.length > 0) { + const fieldName = field.name.value; + const tagUsages = findDirectivesOnNode(field, 'tag'); + const inaccessibleUsages = findDirectivesOnNode( + field, + 'inaccessible', + ); + + if (tagUsages.length > 0) appliedDirectiveUsages.add('tag'); + if (inaccessibleUsages.length > 0) + appliedDirectiveUsages.add('inaccessible'); + + if (tagUsages.length > 0 || inaccessibleUsages.length > 0) { const fieldToDirectivesMap = mapGetOrSet( typeNameToFieldDirectivesMap, parentTypeName, new Map(), ); - const directives = mapGetOrSet( - fieldToDirectivesMap, - field.name.value, - [], - ); - directives.push(...appliedDirectivesOnField); + const directives = mapGetOrSet(fieldToDirectivesMap, fieldName, []); + directives.push(...[...tagUsages, ...inaccessibleUsages]); } } From 83a343f1d58ec8438c43fcc5587722a463bdc84d Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Fri, 9 Jul 2021 13:55:40 -0700 Subject: [PATCH 03/10] DRY up logic for capturing known directive usages Also rename 'appliedDirectives' to 'otherKnownDirectives' --- federation-js/src/composition/compose.ts | 116 +++++++++--------- federation-js/src/composition/types.ts | 2 +- federation-js/src/directives.ts | 4 +- .../src/service/printFederatedSchema.ts | 12 +- .../src/service/printSupergraphSdl.ts | 24 ++-- .../src/composedSchema/metadata.ts | 2 +- 6 files changed, 82 insertions(+), 78 deletions(-) diff --git a/federation-js/src/composition/compose.ts b/federation-js/src/composition/compose.ts index 0d9c4a83a..74659ea60 100644 --- a/federation-js/src/composition/compose.ts +++ b/federation-js/src/composition/compose.ts @@ -21,7 +21,10 @@ import { DirectiveNode, } from 'graphql'; import { transformSchema } from 'apollo-graphql'; -import apolloTypeSystemDirectives, { appliedDirectives, federationDirectives } from '../directives'; +import apolloTypeSystemDirectives, { + otherKnownDirectives, + federationDirectives, +} from '../directives'; import { findDirectivesOnNode, isStringValueNode, @@ -130,9 +133,10 @@ type FieldDirectivesMap = Map; type TypeNameToFieldDirectivesMap = Map; /** - * A set of directive names that have been used at least once + * A set of directive names that have been used at least once - specifically + * non-federation, Apollo-specific directives */ -type AppliedDirectiveUsages = Set; +type OtherKnownDirectiveUsages = Set; /** * Loop over each service and process its typeDefs (`definitions`) @@ -148,7 +152,7 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) { const keyDirectivesMap: KeyDirectivesMap = Object.create(null); const valueTypes: ValueTypes = new Set(); const typeNameToFieldDirectivesMap: TypeNameToFieldDirectivesMap = new Map(); - const appliedDirectiveUsages: AppliedDirectiveUsages = new Set(); + const otherKnownDirectiveUsages: OtherKnownDirectiveUsages = new Set(); for (const { typeDefs, name: serviceName } of serviceList) { // Build a new SDL with @external fields removed, as well as information about @@ -192,28 +196,14 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) { } } - // Capture `@tag` and `@inaccessible` directive applications + // Capture `@tag` and `@inaccessible` directive usages for (const field of definition.fields ?? []) { - const fieldName = field.name.value; - const tagUsages = findDirectivesOnNode(field, 'tag'); - const inaccessibleUsages = findDirectivesOnNode( + captureOtherKnownDirectives( field, - 'inaccessible', + typeName, + typeNameToFieldDirectivesMap, + otherKnownDirectiveUsages, ); - - if (tagUsages.length > 0) appliedDirectiveUsages.add('tag'); - if (inaccessibleUsages.length > 0) - appliedDirectiveUsages.add('inaccessible'); - - if (tagUsages.length > 0 || inaccessibleUsages.length > 0) { - const fieldToDirectivesMap = mapGetOrSet( - typeNameToFieldDirectivesMap, - typeName, - new Map(), - ); - const directives = mapGetOrSet(fieldToDirectivesMap, fieldName, []); - directives.push(...[...tagUsages, ...inaccessibleUsages]); - } } } @@ -351,29 +341,16 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) { } } - // We need to capture applied directives from the external fields as well, - // which are stripped and excluded from the main loop over the typeDefs + // We need to capture other directive usages (`@tag` and `@inaccessible`) from + // the external fields as well, which are stripped and excluded from the main + // loop over the typeDefs for (const { parentTypeName, field } of externalFields) { - const fieldName = field.name.value; - const tagUsages = findDirectivesOnNode(field, 'tag'); - const inaccessibleUsages = findDirectivesOnNode( + captureOtherKnownDirectives( field, - 'inaccessible', + parentTypeName, + typeNameToFieldDirectivesMap, + otherKnownDirectiveUsages, ); - - if (tagUsages.length > 0) appliedDirectiveUsages.add('tag'); - if (inaccessibleUsages.length > 0) - appliedDirectiveUsages.add('inaccessible'); - - if (tagUsages.length > 0 || inaccessibleUsages.length > 0) { - const fieldToDirectivesMap = mapGetOrSet( - typeNameToFieldDirectivesMap, - parentTypeName, - new Map(), - ); - const directives = mapGetOrSet(fieldToDirectivesMap, fieldName, []); - directives.push(...[...tagUsages, ...inaccessibleUsages]); - } } // Since all Query/Mutation definitions in service schemas are treated as @@ -396,7 +373,7 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) { keyDirectivesMap, valueTypes, typeNameToFieldDirectivesMap, - appliedDirectiveUsages, + otherKnownDirectiveUsages, }; } @@ -404,19 +381,19 @@ export function buildSchemaFromDefinitionsAndExtensions({ typeDefinitionsMap, typeExtensionsMap, directiveDefinitionsMap, - appliedDirectiveUsages, + otherKnownDirectiveUsages, }: { typeDefinitionsMap: TypeDefinitionsMap; typeExtensionsMap: TypeExtensionsMap; directiveDefinitionsMap: DirectiveDefinitionsMap; - appliedDirectiveUsages: AppliedDirectiveUsages; + otherKnownDirectiveUsages: OtherKnownDirectiveUsages; }) { let errors: GraphQLError[] | undefined = undefined; - // We only want to include the definitions of applied directives (currently - // just @tag and @include) if there are usages. - const appliedDirectivesToInclude = appliedDirectives.filter((directive) => - appliedDirectiveUsages.has(directive.name), + // We only want to include the definitions of other known directives (currently + // just `@tag` and `@include`) if there are usages. + const otherKnownDirectivesToInclude = otherKnownDirectives.filter( + (directive) => otherKnownDirectiveUsages.has(directive.name), ); let schema = new GraphQLSchema({ @@ -424,7 +401,7 @@ export function buildSchemaFromDefinitionsAndExtensions({ directives: [ ...specifiedDirectives, ...federationDirectives, - ...appliedDirectivesToInclude, + ...otherKnownDirectivesToInclude, ], }); @@ -685,12 +662,12 @@ export function addFederationMetadataToSchemaNodes({ for (const [ fieldName, - appliedDirectives, + otherKnownDirectives, ] of fieldsToDirectivesMap.entries()) { const field = type.getFields()[fieldName]; const seenNonRepeatableDirectives: Record = {}; - const filteredDirectives = appliedDirectives.filter(directive => { + const filteredDirectives = otherKnownDirectives.filter(directive => { const name = directive.name.value; const matchingDirective = apolloTypeSystemDirectives.find(d => d.name === name); if (matchingDirective?.isRepeatable) return true; @@ -703,7 +680,7 @@ export function addFederationMetadataToSchemaNodes({ const existingMetadata = getFederationMetadata(field); const fieldFederationMetadata: FederationField = { ...existingMetadata, - appliedDirectives: filteredDirectives, + otherKnownDirectives: filteredDirectives, }; field.extensions = { @@ -725,14 +702,14 @@ export function composeServices(services: ServiceDefinition[]): CompositionResul keyDirectivesMap, valueTypes, typeNameToFieldDirectivesMap, - appliedDirectiveUsages, + otherKnownDirectiveUsages, } = buildMapsFromServiceList(services); let { schema, errors } = buildSchemaFromDefinitionsAndExtensions({ typeDefinitionsMap, typeExtensionsMap, directiveDefinitionsMap, - appliedDirectiveUsages, + otherKnownDirectiveUsages, }); // TODO: We should fix this to take non-default operation root types in @@ -785,3 +762,30 @@ export function composeServices(services: ServiceDefinition[]): CompositionResul }; } } + +// Capture usages of non-federation Apollo directives, currently +// `@tag` and `@inaccessible` +function captureOtherKnownDirectives( + field: FieldDefinitionNode, + parentTypeName: string, + typeNameToFieldDirectivesMap: TypeNameToFieldDirectivesMap, + otherKnownDirectiveUsages: OtherKnownDirectiveUsages, +) { + const fieldName = field.name.value; + const tagUsages = findDirectivesOnNode(field, 'tag'); + const inaccessibleUsages = findDirectivesOnNode(field, 'inaccessible'); + + if (tagUsages.length > 0) otherKnownDirectiveUsages.add('tag'); + if (inaccessibleUsages.length > 0) + otherKnownDirectiveUsages.add('inaccessible'); + + if (tagUsages.length > 0 || inaccessibleUsages.length > 0) { + const fieldToDirectivesMap = mapGetOrSet( + typeNameToFieldDirectivesMap, + parentTypeName, + new Map(), + ); + const directives = mapGetOrSet(fieldToDirectivesMap, fieldName, []); + directives.push(...[...tagUsages, ...inaccessibleUsages]); + } +} diff --git a/federation-js/src/composition/types.ts b/federation-js/src/composition/types.ts index 7e65bc0ed..ce93f7596 100644 --- a/federation-js/src/composition/types.ts +++ b/federation-js/src/composition/types.ts @@ -39,7 +39,7 @@ export interface FederationField { requires?: ReadonlyArray; provides?: ReadonlyArray; belongsToValueType?: boolean; - appliedDirectives?: DirectiveNode[] + otherKnownDirectives?: DirectiveNode[] } export interface FederationDirective { diff --git a/federation-js/src/directives.ts b/federation-js/src/directives.ts index bce0eb356..5f9ca07a6 100644 --- a/federation-js/src/directives.ts +++ b/federation-js/src/directives.ts @@ -80,11 +80,11 @@ export const federationDirectives = [ ProvidesDirective, ]; -export const appliedDirectives = [InaccessibleDirective, TagDirective]; +export const otherKnownDirectives = [InaccessibleDirective, TagDirective]; const apolloTypeSystemDirectives = [ ...federationDirectives, - ...appliedDirectives, + ...otherKnownDirectives, ]; export default apolloTypeSystemDirectives; diff --git a/federation-js/src/service/printFederatedSchema.ts b/federation-js/src/service/printFederatedSchema.ts index baff86c65..017592777 100644 --- a/federation-js/src/service/printFederatedSchema.ts +++ b/federation-js/src/service/printFederatedSchema.ts @@ -284,7 +284,7 @@ function printFields( String(f.type) + printDeprecated(f) + printFederationDirectives(f) + - printAppliedDirectives(f), + printOtherKnownDirectives(f), ); return printBlock(fields); } @@ -308,13 +308,13 @@ function printFederationDirectives( // Core addition: print `@tag` and `@inaccessible` directives found in subgraph // SDL into the supergraph SDL -function printAppliedDirectives(field: GraphQLField) { - const appliedDirectives = ( - field.extensions?.federation?.appliedDirectives ?? [] +function printOtherKnownDirectives(field: GraphQLField) { + const otherKnownDirectives = ( + field.extensions?.federation?.otherKnownDirectives ?? [] ) as DirectiveNode[]; - if (appliedDirectives.length < 1) return ''; - return ` ${appliedDirectives + if (otherKnownDirectives.length < 1) return ''; + return ` ${otherKnownDirectives .slice() .sort((a, b) => a.name.value.localeCompare(b.name.value)) .map(print) diff --git a/federation-js/src/service/printSupergraphSdl.ts b/federation-js/src/service/printSupergraphSdl.ts index 031ce838f..f3e6c9d40 100644 --- a/federation-js/src/service/printSupergraphSdl.ts +++ b/federation-js/src/service/printSupergraphSdl.ts @@ -33,7 +33,7 @@ import { assert } from '../utilities'; import { CoreDirective } from '../coreSpec'; import { getJoinDefinitions } from '../joinSpec'; import { printFieldSet } from '../composition/utils'; -import { appliedDirectives } from '../directives'; +import { otherKnownDirectives } from '../directives'; type Options = { /** @@ -172,19 +172,19 @@ function printSchemaDefinition(schema: GraphQLSchema): string { } function printCoreDirectives(schema: GraphQLSchema) { - const appliedDirectiveNames = appliedDirectives.map(({name}) => name); + const otherKnownDirectiveNames = otherKnownDirectives.map(({ name }) => name); const schemaDirectiveNames = schema.getDirectives().map(({ name }) => name); - const appliedDirectivesToInclude = schemaDirectiveNames.filter((name) => - appliedDirectiveNames.includes(name), + const otherKnownDirectivesToInclude = schemaDirectiveNames.filter((name) => + otherKnownDirectiveNames.includes(name), ); - const appliedDirectiveSpecUrls = appliedDirectivesToInclude.map( + const otherKnownDirectivesSpecUrls = otherKnownDirectivesToInclude.map( (name) => `https://specs.apollo.dev/${name}/v0.1`, ); return [ 'https://specs.apollo.dev/core/v0.1', 'https://specs.apollo.dev/join/v0.1', - ...appliedDirectiveSpecUrls, + ...otherKnownDirectivesSpecUrls, ].map((feature) => `\n @core(feature: ${printStringLiteral(feature)})`); } @@ -367,7 +367,7 @@ function printFields( // We don't want to print field owner directives on fields belonging to an interface type (isObjectType(type) ? printJoinFieldDirectives(f, type, context) + - printAppliedDirectives(f) + printOtherKnownDirectives(f) : ''), ); @@ -440,13 +440,13 @@ function printJoinFieldDirectives( // Core addition: print `@tag` and `@inaccessible` directives found in subgraph // SDL into the supergraph SDL -function printAppliedDirectives(field: GraphQLField) { - const appliedDirectives = ( - field.extensions?.federation?.appliedDirectives ?? [] +function printOtherKnownDirectives(field: GraphQLField) { + const otherKnownDirectives = ( + field.extensions?.federation?.otherKnownDirectives ?? [] ) as DirectiveNode[]; - if (appliedDirectives.length < 1) return ''; - return ` ${appliedDirectives + if (otherKnownDirectives.length < 1) return ''; + return ` ${otherKnownDirectives .slice() .sort((a, b) => a.name.value.localeCompare(b.name.value)) .map(print) diff --git a/query-planner-js/src/composedSchema/metadata.ts b/query-planner-js/src/composedSchema/metadata.ts index ed22c5aa4..6a84a3a01 100644 --- a/query-planner-js/src/composedSchema/metadata.ts +++ b/query-planner-js/src/composedSchema/metadata.ts @@ -73,5 +73,5 @@ export interface FederationFieldMetadata { graphName?: GraphName; requires?: FieldSet; provides?: FieldSet; - appliedDirectives?: DirectiveNode[]; + otherKnownDirectives?: DirectiveNode[]; } From 2421b3cf22f7de548a6e66a49bea2937d17b3e49 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Fri, 9 Jul 2021 13:57:52 -0700 Subject: [PATCH 04/10] Move test --- .../__tests__/composeAndValidate.test.ts | 144 +++++++++--------- 1 file changed, 72 insertions(+), 72 deletions(-) diff --git a/federation-js/src/composition/__tests__/composeAndValidate.test.ts b/federation-js/src/composition/__tests__/composeAndValidate.test.ts index a56fcbfe9..4b682146d 100644 --- a/federation-js/src/composition/__tests__/composeAndValidate.test.ts +++ b/federation-js/src/composition/__tests__/composeAndValidate.test.ts @@ -920,6 +920,78 @@ describe('composition of schemas with directives', () => { ); } }); + + it('includes @tag directives when usages are on an @external field', () => { + const inventory = { + name: 'inventory', + typeDefs: gql` + extend type Product @key(fields: "id") { + id: ID! @external @tag(name: "hi from inventory") + quantity: Int! + } + `, + }; + + const products = { + name: 'products', + typeDefs: gql` + extend type Query { + product(id: ID!): Product + } + + type Product @key(fields: "id") { + id: ID! + sku: String + } + `, + }; + + const compositionResult = composeAndValidate([inventory, products]); + assertCompositionSuccess(compositionResult); + expect(compositionResult.supergraphSdl).toMatchInlineSnapshot(` + "schema + @core(feature: \\"https://specs.apollo.dev/core/v0.1\\"), + @core(feature: \\"https://specs.apollo.dev/join/v0.1\\"), + @core(feature: \\"https://specs.apollo.dev/tag/v0.1\\") + { + query: Query + } + + directive @core(feature: String!) repeatable on SCHEMA + + directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet) on FIELD_DEFINITION + + directive @join__type(graph: join__Graph!, key: join__FieldSet) repeatable on OBJECT | INTERFACE + + directive @join__owner(graph: join__Graph!) on OBJECT | INTERFACE + + directive @join__graph(name: String!, url: String!) on ENUM_VALUE + + directive @tag(name: String!) repeatable on FIELD_DEFINITION + + scalar join__FieldSet + + enum join__Graph { + INVENTORY @join__graph(name: \\"inventory\\" url: \\"\\") + PRODUCTS @join__graph(name: \\"products\\" url: \\"\\") + } + + type Product + @join__owner(graph: PRODUCTS) + @join__type(graph: PRODUCTS, key: \\"id\\") + @join__type(graph: INVENTORY, key: \\"id\\") + { + id: ID! @join__field(graph: PRODUCTS) @tag(name: \\"hi from inventory\\") + quantity: Int! @join__field(graph: INVENTORY) + sku: String @join__field(graph: PRODUCTS) + } + + type Query { + product(id: ID!): Product @join__field(graph: PRODUCTS) + } + " + `); + }); }); it('composition of full-SDL schemas without any errors', () => { @@ -1038,75 +1110,3 @@ it('composition of full-SDL schemas without any errors', () => { const compositionResult = composeAndValidate([serviceA, serviceB]); expect(!compositionHasErrors(compositionResult)); }); - -it('tag directives are still included when usages are on an @external field', () => { - const inventory = { - name: 'inventory', - typeDefs: gql` - extend type Product @key(fields: "id") { - id: ID! @external @tag(name: "hi from inventory") - quantity: Int! - } - `, - }; - - const products = { - name: 'products', - typeDefs: gql` - extend type Query { - product(id: ID!): Product - } - - type Product @key(fields: "id") { - id: ID! - sku: String - } - `, - }; - - const compositionResult = composeAndValidate([inventory, products]); - assertCompositionSuccess(compositionResult); - expect(compositionResult.supergraphSdl).toMatchInlineSnapshot(` - "schema - @core(feature: \\"https://specs.apollo.dev/core/v0.1\\"), - @core(feature: \\"https://specs.apollo.dev/join/v0.1\\"), - @core(feature: \\"https://specs.apollo.dev/tag/v0.1\\") - { - query: Query - } - - directive @core(feature: String!) repeatable on SCHEMA - - directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet) on FIELD_DEFINITION - - directive @join__type(graph: join__Graph!, key: join__FieldSet) repeatable on OBJECT | INTERFACE - - directive @join__owner(graph: join__Graph!) on OBJECT | INTERFACE - - directive @join__graph(name: String!, url: String!) on ENUM_VALUE - - directive @tag(name: String!) repeatable on FIELD_DEFINITION - - scalar join__FieldSet - - enum join__Graph { - INVENTORY @join__graph(name: \\"inventory\\" url: \\"\\") - PRODUCTS @join__graph(name: \\"products\\" url: \\"\\") - } - - type Product - @join__owner(graph: PRODUCTS) - @join__type(graph: PRODUCTS, key: \\"id\\") - @join__type(graph: INVENTORY, key: \\"id\\") - { - id: ID! @join__field(graph: PRODUCTS) @tag(name: \\"hi from inventory\\") - quantity: Int! @join__field(graph: INVENTORY) - sku: String @join__field(graph: PRODUCTS) - } - - type Query { - product(id: ID!): Product @join__field(graph: PRODUCTS) - } - " - `); -}); From 243e731b3a6b0c6a59e3e5487b776b929830fc83 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Mon, 12 Jul 2021 13:23:51 -0700 Subject: [PATCH 05/10] Update approach - disallow @tag and @external together This reverts most of the previous implementation in favor of a new composition error which prevents users from confusingly using both @tag and @external together. For now, @tag is expected to have implications with schema contracts and in the context of an external field, it's not well-defined what that would actually mean. We may revisit and relax this in the future, but for now have decided to just prevent the possible confusion. --- docs/source/errors.md | 5 ++ .../__tests__/composeAndValidate.test.ts | 72 --------------- federation-js/src/composition/compose.ts | 62 ++++--------- .../__tests__/tagUsedWithExternal.test.ts | 89 +++++++++++++++++++ .../validate/preComposition/index.ts | 1 + .../preComposition/tagUsedWithExternal.ts | 59 ++++++++++++ 6 files changed, 171 insertions(+), 117 deletions(-) create mode 100644 federation-js/src/composition/validate/preComposition/__tests__/tagUsedWithExternal.test.ts create mode 100644 federation-js/src/composition/validate/preComposition/tagUsedWithExternal.ts diff --git a/docs/source/errors.md b/docs/source/errors.md index d4d67d77c..63e035cd6 100644 --- a/docs/source/errors.md +++ b/docs/source/errors.md @@ -53,6 +53,11 @@ If Apollo Gateway encounters an error, composition fails. This document lists co | `REQUIRES_FIELDS_MISSING_ON_BASE` | The `fields` argument of an entity field's `@requires` directive includes a field that is not defined in the entity's originating subgraph.`| | `REQUIRES_USED_ON_BASE` | An entity field is marked with `@requires` in the entity's originating subgraph, which is invalid. | +## `@tag` + +| Code | Description | +|---|---| +| `TAG_USED_WITH_EXTERNAL` | Fields marked as `@external` cannot also have `@tag` usages. | ## Custom directives | Code | Description | diff --git a/federation-js/src/composition/__tests__/composeAndValidate.test.ts b/federation-js/src/composition/__tests__/composeAndValidate.test.ts index 4b682146d..56e216341 100644 --- a/federation-js/src/composition/__tests__/composeAndValidate.test.ts +++ b/federation-js/src/composition/__tests__/composeAndValidate.test.ts @@ -920,78 +920,6 @@ describe('composition of schemas with directives', () => { ); } }); - - it('includes @tag directives when usages are on an @external field', () => { - const inventory = { - name: 'inventory', - typeDefs: gql` - extend type Product @key(fields: "id") { - id: ID! @external @tag(name: "hi from inventory") - quantity: Int! - } - `, - }; - - const products = { - name: 'products', - typeDefs: gql` - extend type Query { - product(id: ID!): Product - } - - type Product @key(fields: "id") { - id: ID! - sku: String - } - `, - }; - - const compositionResult = composeAndValidate([inventory, products]); - assertCompositionSuccess(compositionResult); - expect(compositionResult.supergraphSdl).toMatchInlineSnapshot(` - "schema - @core(feature: \\"https://specs.apollo.dev/core/v0.1\\"), - @core(feature: \\"https://specs.apollo.dev/join/v0.1\\"), - @core(feature: \\"https://specs.apollo.dev/tag/v0.1\\") - { - query: Query - } - - directive @core(feature: String!) repeatable on SCHEMA - - directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet) on FIELD_DEFINITION - - directive @join__type(graph: join__Graph!, key: join__FieldSet) repeatable on OBJECT | INTERFACE - - directive @join__owner(graph: join__Graph!) on OBJECT | INTERFACE - - directive @join__graph(name: String!, url: String!) on ENUM_VALUE - - directive @tag(name: String!) repeatable on FIELD_DEFINITION - - scalar join__FieldSet - - enum join__Graph { - INVENTORY @join__graph(name: \\"inventory\\" url: \\"\\") - PRODUCTS @join__graph(name: \\"products\\" url: \\"\\") - } - - type Product - @join__owner(graph: PRODUCTS) - @join__type(graph: PRODUCTS, key: \\"id\\") - @join__type(graph: INVENTORY, key: \\"id\\") - { - id: ID! @join__field(graph: PRODUCTS) @tag(name: \\"hi from inventory\\") - quantity: Int! @join__field(graph: INVENTORY) - sku: String @join__field(graph: PRODUCTS) - } - - type Query { - product(id: ID!): Product @join__field(graph: PRODUCTS) - } - " - `); - }); }); it('composition of full-SDL schemas without any errors', () => { diff --git a/federation-js/src/composition/compose.ts b/federation-js/src/composition/compose.ts index 74659ea60..1b6206fca 100644 --- a/federation-js/src/composition/compose.ts +++ b/federation-js/src/composition/compose.ts @@ -198,12 +198,23 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) { // Capture `@tag` and `@inaccessible` directive usages for (const field of definition.fields ?? []) { - captureOtherKnownDirectives( - field, - typeName, - typeNameToFieldDirectivesMap, - otherKnownDirectiveUsages, - ); + const fieldName = field.name.value; + const tagUsages = findDirectivesOnNode(field, 'tag'); + const inaccessibleUsages = findDirectivesOnNode(field, 'inaccessible'); + + if (tagUsages.length > 0) otherKnownDirectiveUsages.add('tag'); + if (inaccessibleUsages.length > 0) + otherKnownDirectiveUsages.add('inaccessible'); + + if (tagUsages.length > 0 || inaccessibleUsages.length > 0) { + const fieldToDirectivesMap = mapGetOrSet( + typeNameToFieldDirectivesMap, + typeName, + new Map(), + ); + const directives = mapGetOrSet(fieldToDirectivesMap, fieldName, []); + directives.push(...[...tagUsages, ...inaccessibleUsages]); + } } } @@ -341,18 +352,6 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) { } } - // We need to capture other directive usages (`@tag` and `@inaccessible`) from - // the external fields as well, which are stripped and excluded from the main - // loop over the typeDefs - for (const { parentTypeName, field } of externalFields) { - captureOtherKnownDirectives( - field, - parentTypeName, - typeNameToFieldDirectivesMap, - otherKnownDirectiveUsages, - ); - } - // Since all Query/Mutation definitions in service schemas are treated as // extensions, we don't have a Query or Mutation DEFINITION in the definitions // list. Without a Query/Mutation definition, we can't _extend_ the type. @@ -762,30 +761,3 @@ export function composeServices(services: ServiceDefinition[]): CompositionResul }; } } - -// Capture usages of non-federation Apollo directives, currently -// `@tag` and `@inaccessible` -function captureOtherKnownDirectives( - field: FieldDefinitionNode, - parentTypeName: string, - typeNameToFieldDirectivesMap: TypeNameToFieldDirectivesMap, - otherKnownDirectiveUsages: OtherKnownDirectiveUsages, -) { - const fieldName = field.name.value; - const tagUsages = findDirectivesOnNode(field, 'tag'); - const inaccessibleUsages = findDirectivesOnNode(field, 'inaccessible'); - - if (tagUsages.length > 0) otherKnownDirectiveUsages.add('tag'); - if (inaccessibleUsages.length > 0) - otherKnownDirectiveUsages.add('inaccessible'); - - if (tagUsages.length > 0 || inaccessibleUsages.length > 0) { - const fieldToDirectivesMap = mapGetOrSet( - typeNameToFieldDirectivesMap, - parentTypeName, - new Map(), - ); - const directives = mapGetOrSet(fieldToDirectivesMap, fieldName, []); - directives.push(...[...tagUsages, ...inaccessibleUsages]); - } -} diff --git a/federation-js/src/composition/validate/preComposition/__tests__/tagUsedWithExternal.test.ts b/federation-js/src/composition/validate/preComposition/__tests__/tagUsedWithExternal.test.ts new file mode 100644 index 000000000..4ff4e39c3 --- /dev/null +++ b/federation-js/src/composition/validate/preComposition/__tests__/tagUsedWithExternal.test.ts @@ -0,0 +1,89 @@ +import { tagUsedWithExternal } from '..'; +import { + gql, + graphqlErrorSerializer, +} from 'apollo-federation-integration-testsuite'; + +expect.addSnapshotSerializer(graphqlErrorSerializer); + +describe('tagUsedWithExternal', () => { + it('has no errors @external and @tag are used separately', () => { + const serviceA = { + typeDefs: gql` + type Query { + product: Product @tag(name: "prod") + } + + extend type Product @key(fields: "sku") { + sku: String @external + } + `, + name: 'serviceA', + }; + + const errors = tagUsedWithExternal(serviceA); + expect(errors).toHaveLength(0); + }); + + it('errors when `@external` and `@tag` are used on the same field of an object extension', () => { + const serviceA = { + typeDefs: gql` + type Query { + product: Product @tag(name: "prod") + } + + extend type Product @key(fields: "sku") { + sku: String @external @tag(name: "prod") + } + `, + name: 'serviceA', + }; + + const errors = tagUsedWithExternal(serviceA); + expect(errors).toMatchInlineSnapshot(` + Array [ + Object { + "code": "TAG_USED_WITH_EXTERNAL", + "locations": Array [ + Object { + "column": 25, + "line": 7, + }, + ], + "message": "[serviceA] Product.sku -> Found illegal use of @tag directive. @tag directives cannot currently be used in tandem with an @external directive.", + }, + ] + `); + }); + + it('errors when `@external` and `@tag` are used on the same field of an interface extension', () => { + const serviceA = { + typeDefs: gql` + type Query { + product: Product @tag(name: "prod") + } + + extend interface Product @key(fields: "sku") { + sku: String @external @tag(name: "prod") + } + `, + name: 'serviceA', + }; + + const errors = tagUsedWithExternal(serviceA); + expect(errors).toMatchInlineSnapshot(` + Array [ + Object { + "code": "TAG_USED_WITH_EXTERNAL", + "locations": Array [ + Object { + "column": 25, + "line": 7, + }, + ], + "message": "[serviceA] Product.sku -> Found illegal use of @tag directive. @tag directives cannot currently be used in tandem with an @external directive.", + }, + ] + `); + }); +}); diff --git a/federation-js/src/composition/validate/preComposition/index.ts b/federation-js/src/composition/validate/preComposition/index.ts index c2d78e45b..adb813bc7 100644 --- a/federation-js/src/composition/validate/preComposition/index.ts +++ b/federation-js/src/composition/validate/preComposition/index.ts @@ -4,3 +4,4 @@ export { keyFieldsMissingExternal } from './keyFieldsMissingExternal'; export { reservedFieldUsed } from './reservedFieldUsed'; export { duplicateEnumOrScalar } from './duplicateEnumOrScalar'; export { duplicateEnumValue } from './duplicateEnumValue'; +export { tagUsedWithExternal } from './tagUsedWithExternal'; diff --git a/federation-js/src/composition/validate/preComposition/tagUsedWithExternal.ts b/federation-js/src/composition/validate/preComposition/tagUsedWithExternal.ts new file mode 100644 index 000000000..35c2ce7ed --- /dev/null +++ b/federation-js/src/composition/validate/preComposition/tagUsedWithExternal.ts @@ -0,0 +1,59 @@ +import { + visit, + GraphQLError, + ObjectTypeExtensionNode, + InterfaceTypeExtensionNode, +} from 'graphql'; +import { ServiceDefinition } from '../../types'; + +import { + logServiceAndType, + errorWithCode, + findDirectivesOnNode, +} from '../../utils'; + +/** + * There are no fields with both @tag and @external. We're only concerned with + * the xTypeExtension nodes because @external is not allowed on base types. + */ +export const tagUsedWithExternal = ({ + name: serviceName, + typeDefs, +}: ServiceDefinition) => { + const errors: GraphQLError[] = []; + + visit(typeDefs, { + ObjectTypeExtension: fieldsVisitor, + InterfaceTypeExtension: fieldsVisitor, + }); + + function fieldsVisitor( + typeDefinition: + | ObjectTypeExtensionNode + | InterfaceTypeExtensionNode, + ) { + if (!typeDefinition.fields) return; + for (const fieldDefinition of typeDefinition.fields) { + const tagDirectives = findDirectivesOnNode(fieldDefinition, 'tag'); + const hasTagDirective = tagDirectives.length > 0; + const hasExternalDirective = + findDirectivesOnNode(fieldDefinition, 'external').length > 0; + if (hasTagDirective && hasExternalDirective) { + errors.push( + errorWithCode( + 'TAG_USED_WITH_EXTERNAL', + logServiceAndType( + serviceName, + typeDefinition.name.value, + fieldDefinition.name.value, + ) + + `Found illegal use of @tag directive. @tag directives cannot currently be used in tandem with an @external directive.`, + tagDirectives, + ), + ); + } + } + } + + return errors; +}; From be3f91974990efab1bc4279cb840d98970bbe76d Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Mon, 12 Jul 2021 13:26:27 -0700 Subject: [PATCH 06/10] Add missing newline --- docs/source/errors.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/source/errors.md b/docs/source/errors.md index 63e035cd6..e204e4cc3 100644 --- a/docs/source/errors.md +++ b/docs/source/errors.md @@ -58,6 +58,7 @@ If Apollo Gateway encounters an error, composition fails. This document lists co | Code | Description | |---|---| | `TAG_USED_WITH_EXTERNAL` | Fields marked as `@external` cannot also have `@tag` usages. | + ## Custom directives | Code | Description | From 884da2c2f5a023b89ba937b8a72c4490c3a017a4 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Mon, 12 Jul 2021 13:31:22 -0700 Subject: [PATCH 07/10] Update tests --- .../src/fixtures/reviews.ts | 2 +- .../src/composition/__tests__/composeAndValidate.test.ts | 8 ++++---- .../src/service/__tests__/printFederatedSchema.test.ts | 2 +- .../src/service/__tests__/printSupergraphSdl.test.ts | 2 +- .../src/__tests__/loadSupergraphSdlFromStorage.test.ts | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/federation-integration-testsuite-js/src/fixtures/reviews.ts b/federation-integration-testsuite-js/src/fixtures/reviews.ts index ccf3399d0..7660773bc 100644 --- a/federation-integration-testsuite-js/src/fixtures/reviews.ts +++ b/federation-integration-testsuite-js/src/fixtures/reviews.ts @@ -29,7 +29,7 @@ export const typeDefs = gql` } extend type User @key(fields: "id") { - id: ID! @external @tag(name: "reviews") @inaccessible + id: ID! @external username: String @external reviews: [Review] numberOfReviews: Int! diff --git a/federation-js/src/composition/__tests__/composeAndValidate.test.ts b/federation-js/src/composition/__tests__/composeAndValidate.test.ts index 56e216341..d907ba14b 100644 --- a/federation-js/src/composition/__tests__/composeAndValidate.test.ts +++ b/federation-js/src/composition/__tests__/composeAndValidate.test.ts @@ -185,14 +185,14 @@ describe('unknown types', () => { assertCompositionFailure(compositionResult); expect(compositionResult.errors[0]).toMatchInlineSnapshot(` Object { - "code": "EXTENSION_WITH_NO_BASE", + "code": "TAG_USED_WITH_EXTERNAL", "locations": Array [ Object { - "column": 1, - "line": 2, + "column": 21, + "line": 3, }, ], - "message": "[inventory] Product -> \`Product\` is an extension type, but \`Product\` is not defined in any service", + "message": "[inventory] Product.id -> Found illegal use of @tag directive. @tag directives cannot currently be used in tandem with an @external directive.", } `); }); diff --git a/federation-js/src/service/__tests__/printFederatedSchema.test.ts b/federation-js/src/service/__tests__/printFederatedSchema.test.ts index c5e5eb9af..3872c17f9 100644 --- a/federation-js/src/service/__tests__/printFederatedSchema.test.ts +++ b/federation-js/src/service/__tests__/printFederatedSchema.test.ts @@ -186,7 +186,7 @@ describe('printFederatedSchema', () => { birthDate(locale: String): String @tag(name: \\"admin\\") @tag(name: \\"dev\\") goodAddress: Boolean @requires(fields: \\"metadata { address }\\") goodDescription: Boolean @requires(fields: \\"metadata { description }\\") - id: ID! @inaccessible @tag(name: \\"accounts\\") @tag(name: \\"reviews\\") + id: ID! @inaccessible @tag(name: \\"accounts\\") metadata: [UserMetadata] name: Name numberOfReviews: Int! diff --git a/federation-js/src/service/__tests__/printSupergraphSdl.test.ts b/federation-js/src/service/__tests__/printSupergraphSdl.test.ts index 72e94a754..3c05e8c35 100644 --- a/federation-js/src/service/__tests__/printSupergraphSdl.test.ts +++ b/federation-js/src/service/__tests__/printSupergraphSdl.test.ts @@ -273,7 +273,7 @@ describe('printSupergraphSdl', () => { birthDate(locale: String): String @join__field(graph: ACCOUNTS) @tag(name: \\"admin\\") @tag(name: \\"dev\\") goodAddress: Boolean @join__field(graph: REVIEWS, requires: \\"metadata{address}\\") goodDescription: Boolean @join__field(graph: INVENTORY, requires: \\"metadata{description}\\") - id: ID! @join__field(graph: ACCOUNTS) @inaccessible @tag(name: \\"accounts\\") @tag(name: \\"reviews\\") + id: ID! @join__field(graph: ACCOUNTS) @inaccessible @tag(name: \\"accounts\\") metadata: [UserMetadata] @join__field(graph: ACCOUNTS) name: Name @join__field(graph: ACCOUNTS) numberOfReviews: Int! @join__field(graph: REVIEWS) diff --git a/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts b/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts index 9eb11ea13..bf31fc64e 100644 --- a/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts +++ b/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts @@ -280,7 +280,7 @@ describe('loadSupergraphSdlFromStorage', () => { birthDate(locale: String): String @join__field(graph: ACCOUNTS) @tag(name: \\"admin\\") @tag(name: \\"dev\\") goodAddress: Boolean @join__field(graph: REVIEWS, requires: \\"metadata{address}\\") goodDescription: Boolean @join__field(graph: INVENTORY, requires: \\"metadata{description}\\") - id: ID! @join__field(graph: ACCOUNTS) @inaccessible @tag(name: \\"accounts\\") @tag(name: \\"reviews\\") + id: ID! @join__field(graph: ACCOUNTS) @inaccessible @tag(name: \\"accounts\\") metadata: [UserMetadata] @join__field(graph: ACCOUNTS) name: Name @join__field(graph: ACCOUNTS) numberOfReviews: Int! @join__field(graph: REVIEWS) From 3d51e584ddb027cb3ea22f406db3f25d6f6f2663 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 13 Jul 2021 13:07:31 -0700 Subject: [PATCH 08/10] Also validate against improper @inaccessible usage --- docs/source/errors.md | 3 +- .../tagOrInaccessibleUsedWithExternal.test.ts | 173 ++++++++++++++++++ .../__tests__/tagUsedWithExternal.test.ts | 89 --------- .../validate/preComposition/index.ts | 2 +- ...s => tagOrInaccessibleUsedWithExternal.ts} | 31 +++- 5 files changed, 201 insertions(+), 97 deletions(-) create mode 100644 federation-js/src/composition/validate/preComposition/__tests__/tagOrInaccessibleUsedWithExternal.test.ts delete mode 100644 federation-js/src/composition/validate/preComposition/__tests__/tagUsedWithExternal.test.ts rename federation-js/src/composition/validate/preComposition/{tagUsedWithExternal.ts => tagOrInaccessibleUsedWithExternal.ts} (56%) diff --git a/docs/source/errors.md b/docs/source/errors.md index e204e4cc3..c7f40142c 100644 --- a/docs/source/errors.md +++ b/docs/source/errors.md @@ -53,11 +53,12 @@ If Apollo Gateway encounters an error, composition fails. This document lists co | `REQUIRES_FIELDS_MISSING_ON_BASE` | The `fields` argument of an entity field's `@requires` directive includes a field that is not defined in the entity's originating subgraph.`| | `REQUIRES_USED_ON_BASE` | An entity field is marked with `@requires` in the entity's originating subgraph, which is invalid. | -## `@tag` +## Non-federation directives | Code | Description | |---|---| | `TAG_USED_WITH_EXTERNAL` | Fields marked as `@external` cannot also have `@tag` usages. | +| `INACCESSIBLE_USED_WITH_EXTERNAL` | Fields marked as `@external` cannot also have `@inaccessible` usages. | ## Custom directives diff --git a/federation-js/src/composition/validate/preComposition/__tests__/tagOrInaccessibleUsedWithExternal.test.ts b/federation-js/src/composition/validate/preComposition/__tests__/tagOrInaccessibleUsedWithExternal.test.ts new file mode 100644 index 000000000..248e8a416 --- /dev/null +++ b/federation-js/src/composition/validate/preComposition/__tests__/tagOrInaccessibleUsedWithExternal.test.ts @@ -0,0 +1,173 @@ +import { tagOrInaccessibleUsedWithExternal } from '..'; +import { + gql, + graphqlErrorSerializer, +} from 'apollo-federation-integration-testsuite'; + +expect.addSnapshotSerializer(graphqlErrorSerializer); + +describe('tagOrInaccessibleUsedWithExternal', () => { + describe('no errors', () => { + it('has no errors @external and @tag are used separately', () => { + const serviceA = { + typeDefs: gql` + type Query { + product: Product @tag(name: "prod") + } + + extend type Product @key(fields: "sku") { + sku: String @external + } + `, + name: 'serviceA', + }; + + const errors = tagOrInaccessibleUsedWithExternal(serviceA); + expect(errors).toHaveLength(0); + }); + + it('has no errors @external and @inaccessible are used separately', () => { + const serviceA = { + typeDefs: gql` + type Query { + product: Product @inaccessible + } + + extend type Product @key(fields: "sku") { + sku: String @external + } + `, + name: 'serviceA', + }; + + const errors = tagOrInaccessibleUsedWithExternal(serviceA); + expect(errors).toHaveLength(0); + }); + }); + + describe('errors', () => { + it('errors when `@external` and `@tag` are used on the same field of an object extension', () => { + const serviceA = { + typeDefs: gql` + type Query { + product: Product @tag(name: "prod") + } + + extend type Product @key(fields: "sku") { + sku: String @external @tag(name: "prod") + } + `, + name: 'serviceA', + }; + + const errors = tagOrInaccessibleUsedWithExternal(serviceA); + expect(errors).toMatchInlineSnapshot(` + Array [ + Object { + "code": "TAG_USED_WITH_EXTERNAL", + "locations": Array [ + Object { + "column": 25, + "line": 7, + }, + ], + "message": "[serviceA] Product.sku -> Found illegal use of @tag directive. @tag directives cannot currently be used in tandem with an @external directive.", + }, + ] + `); + }); + + it('errors when `@external` and `@tag` are used on the same field of an interface extension', () => { + const serviceA = { + typeDefs: gql` + type Query { + product: Product @tag(name: "prod") + } + + extend interface Product @key(fields: "sku") { + sku: String @external @tag(name: "prod") + } + `, + name: 'serviceA', + }; + + const errors = tagOrInaccessibleUsedWithExternal(serviceA); + expect(errors).toMatchInlineSnapshot(` + Array [ + Object { + "code": "TAG_USED_WITH_EXTERNAL", + "locations": Array [ + Object { + "column": 25, + "line": 7, + }, + ], + "message": "[serviceA] Product.sku -> Found illegal use of @tag directive. @tag directives cannot currently be used in tandem with an @external directive.", + }, + ] + `); + }); + + it('errors when `@external` and `@inaccessible` are used on the same field of an object extension', () => { + const serviceA = { + typeDefs: gql` + type Query { + product: Product @tag(name: "prod") + } + + extend type Product @key(fields: "sku") { + sku: String @external @inaccessible + } + `, + name: 'serviceA', + }; + + const errors = tagOrInaccessibleUsedWithExternal(serviceA); + expect(errors).toMatchInlineSnapshot(` + Array [ + Object { + "code": "INACCESSIBLE_USED_WITH_EXTERNAL", + "locations": Array [ + Object { + "column": 25, + "line": 7, + }, + ], + "message": "[serviceA] Product.sku -> Found illegal use of @inaccessible directive. @inaccessible directives cannot currently be used in tandem with an @external directive.", + }, + ] + `); + }); + + it('errors when `@external` and `@inaccessible` are used on the same field of an interface extension', () => { + const serviceA = { + typeDefs: gql` + type Query { + product: Product @tag(name: "prod") + } + + extend interface Product @key(fields: "sku") { + sku: String @external @inaccessible + } + `, + name: 'serviceA', + }; + + const errors = tagOrInaccessibleUsedWithExternal(serviceA); + expect(errors).toMatchInlineSnapshot(` + Array [ + Object { + "code": "INACCESSIBLE_USED_WITH_EXTERNAL", + "locations": Array [ + Object { + "column": 25, + "line": 7, + }, + ], + "message": "[serviceA] Product.sku -> Found illegal use of @inaccessible directive. @inaccessible directives cannot currently be used in tandem with an @external directive.", + }, + ] + `); + }); + }); +}); diff --git a/federation-js/src/composition/validate/preComposition/__tests__/tagUsedWithExternal.test.ts b/federation-js/src/composition/validate/preComposition/__tests__/tagUsedWithExternal.test.ts deleted file mode 100644 index 4ff4e39c3..000000000 --- a/federation-js/src/composition/validate/preComposition/__tests__/tagUsedWithExternal.test.ts +++ /dev/null @@ -1,89 +0,0 @@ -import { tagUsedWithExternal } from '..'; -import { - gql, - graphqlErrorSerializer, -} from 'apollo-federation-integration-testsuite'; - -expect.addSnapshotSerializer(graphqlErrorSerializer); - -describe('tagUsedWithExternal', () => { - it('has no errors @external and @tag are used separately', () => { - const serviceA = { - typeDefs: gql` - type Query { - product: Product @tag(name: "prod") - } - - extend type Product @key(fields: "sku") { - sku: String @external - } - `, - name: 'serviceA', - }; - - const errors = tagUsedWithExternal(serviceA); - expect(errors).toHaveLength(0); - }); - - it('errors when `@external` and `@tag` are used on the same field of an object extension', () => { - const serviceA = { - typeDefs: gql` - type Query { - product: Product @tag(name: "prod") - } - - extend type Product @key(fields: "sku") { - sku: String @external @tag(name: "prod") - } - `, - name: 'serviceA', - }; - - const errors = tagUsedWithExternal(serviceA); - expect(errors).toMatchInlineSnapshot(` - Array [ - Object { - "code": "TAG_USED_WITH_EXTERNAL", - "locations": Array [ - Object { - "column": 25, - "line": 7, - }, - ], - "message": "[serviceA] Product.sku -> Found illegal use of @tag directive. @tag directives cannot currently be used in tandem with an @external directive.", - }, - ] - `); - }); - - it('errors when `@external` and `@tag` are used on the same field of an interface extension', () => { - const serviceA = { - typeDefs: gql` - type Query { - product: Product @tag(name: "prod") - } - - extend interface Product @key(fields: "sku") { - sku: String @external @tag(name: "prod") - } - `, - name: 'serviceA', - }; - - const errors = tagUsedWithExternal(serviceA); - expect(errors).toMatchInlineSnapshot(` - Array [ - Object { - "code": "TAG_USED_WITH_EXTERNAL", - "locations": Array [ - Object { - "column": 25, - "line": 7, - }, - ], - "message": "[serviceA] Product.sku -> Found illegal use of @tag directive. @tag directives cannot currently be used in tandem with an @external directive.", - }, - ] - `); - }); -}); diff --git a/federation-js/src/composition/validate/preComposition/index.ts b/federation-js/src/composition/validate/preComposition/index.ts index adb813bc7..6d0d1e4dc 100644 --- a/federation-js/src/composition/validate/preComposition/index.ts +++ b/federation-js/src/composition/validate/preComposition/index.ts @@ -4,4 +4,4 @@ export { keyFieldsMissingExternal } from './keyFieldsMissingExternal'; export { reservedFieldUsed } from './reservedFieldUsed'; export { duplicateEnumOrScalar } from './duplicateEnumOrScalar'; export { duplicateEnumValue } from './duplicateEnumValue'; -export { tagUsedWithExternal } from './tagUsedWithExternal'; +export { tagOrInaccessibleUsedWithExternal } from './tagOrInaccessibleUsedWithExternal'; diff --git a/federation-js/src/composition/validate/preComposition/tagUsedWithExternal.ts b/federation-js/src/composition/validate/preComposition/tagOrInaccessibleUsedWithExternal.ts similarity index 56% rename from federation-js/src/composition/validate/preComposition/tagUsedWithExternal.ts rename to federation-js/src/composition/validate/preComposition/tagOrInaccessibleUsedWithExternal.ts index 35c2ce7ed..815b39f59 100644 --- a/federation-js/src/composition/validate/preComposition/tagUsedWithExternal.ts +++ b/federation-js/src/composition/validate/preComposition/tagOrInaccessibleUsedWithExternal.ts @@ -13,10 +13,11 @@ import { } from '../../utils'; /** - * There are no fields with both @tag and @external. We're only concerned with - * the xTypeExtension nodes because @external is not allowed on base types. + * Ensure there are no fields with @external and _either_ @tag or @inaccessible. + * We're only concerned with the xTypeExtension nodes because @external is + * already disallowed on base types. */ -export const tagUsedWithExternal = ({ +export const tagOrInaccessibleUsedWithExternal = ({ name: serviceName, typeDefs, }: ServiceDefinition) => { @@ -28,14 +29,17 @@ export const tagUsedWithExternal = ({ }); function fieldsVisitor( - typeDefinition: - | ObjectTypeExtensionNode - | InterfaceTypeExtensionNode, + typeDefinition: ObjectTypeExtensionNode | InterfaceTypeExtensionNode, ) { if (!typeDefinition.fields) return; for (const fieldDefinition of typeDefinition.fields) { const tagDirectives = findDirectivesOnNode(fieldDefinition, 'tag'); const hasTagDirective = tagDirectives.length > 0; + const inaccessibleDirectives = findDirectivesOnNode( + fieldDefinition, + 'inaccessible', + ); + const hasInaccessibleDirective = inaccessibleDirectives.length > 0; const hasExternalDirective = findDirectivesOnNode(fieldDefinition, 'external').length > 0; if (hasTagDirective && hasExternalDirective) { @@ -52,6 +56,21 @@ export const tagUsedWithExternal = ({ ), ); } + + if (hasInaccessibleDirective && hasExternalDirective) { + errors.push( + errorWithCode( + 'INACCESSIBLE_USED_WITH_EXTERNAL', + logServiceAndType( + serviceName, + typeDefinition.name.value, + fieldDefinition.name.value, + ) + + `Found illegal use of @inaccessible directive. @inaccessible directives cannot currently be used in tandem with an @external directive.`, + inaccessibleDirectives, + ), + ); + } } } From 9922bb911929c693617d8790affbc5518ad7f673 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 13 Jul 2021 13:19:56 -0700 Subject: [PATCH 09/10] Naming: clarity between usages and definitions --- federation-js/src/composition/compose.ts | 35 +++++++++++-------- federation-js/src/composition/types.ts | 2 +- federation-js/src/directives.ts | 7 ++-- .../src/service/printFederatedSchema.ts | 20 +++++------ .../src/service/printSupergraphSdl.ts | 31 ++++++++-------- .../src/composedSchema/metadata.ts | 2 +- 6 files changed, 54 insertions(+), 43 deletions(-) diff --git a/federation-js/src/composition/compose.ts b/federation-js/src/composition/compose.ts index 1b6206fca..f609edf64 100644 --- a/federation-js/src/composition/compose.ts +++ b/federation-js/src/composition/compose.ts @@ -22,7 +22,7 @@ import { } from 'graphql'; import { transformSchema } from 'apollo-graphql'; import apolloTypeSystemDirectives, { - otherKnownDirectives, + otherKnownDirectiveDefinitions, federationDirectives, } from '../directives'; import { @@ -391,16 +391,17 @@ export function buildSchemaFromDefinitionsAndExtensions({ // We only want to include the definitions of other known directives (currently // just `@tag` and `@include`) if there are usages. - const otherKnownDirectivesToInclude = otherKnownDirectives.filter( - (directive) => otherKnownDirectiveUsages.has(directive.name), - ); + const otherKnownDirectiveDefinitionsToInclude = + otherKnownDirectiveDefinitions.filter((directive) => + otherKnownDirectiveUsages.has(directive.name), + ); let schema = new GraphQLSchema({ query: undefined, directives: [ ...specifiedDirectives, ...federationDirectives, - ...otherKnownDirectivesToInclude, + ...otherKnownDirectiveDefinitionsToInclude, ], }); @@ -661,25 +662,29 @@ export function addFederationMetadataToSchemaNodes({ for (const [ fieldName, - otherKnownDirectives, + otherKnownDirectiveUsages, ] of fieldsToDirectivesMap.entries()) { const field = type.getFields()[fieldName]; const seenNonRepeatableDirectives: Record = {}; - const filteredDirectives = otherKnownDirectives.filter(directive => { - const name = directive.name.value; - const matchingDirective = apolloTypeSystemDirectives.find(d => d.name === name); - if (matchingDirective?.isRepeatable) return true; - if (seenNonRepeatableDirectives[name]) return false; - seenNonRepeatableDirectives[name] = true; - return true; - }); + const filteredDirectives = otherKnownDirectiveUsages.filter( + (directive) => { + const name = directive.name.value; + const matchingDirective = apolloTypeSystemDirectives.find( + (d) => d.name === name, + ); + if (matchingDirective?.isRepeatable) return true; + if (seenNonRepeatableDirectives[name]) return false; + seenNonRepeatableDirectives[name] = true; + return true; + }, + ); // TODO: probably don't need to recreate these objects const existingMetadata = getFederationMetadata(field); const fieldFederationMetadata: FederationField = { ...existingMetadata, - otherKnownDirectives: filteredDirectives, + otherKnownDirectiveUsages: filteredDirectives, }; field.extensions = { diff --git a/federation-js/src/composition/types.ts b/federation-js/src/composition/types.ts index ce93f7596..cc39841f3 100644 --- a/federation-js/src/composition/types.ts +++ b/federation-js/src/composition/types.ts @@ -39,7 +39,7 @@ export interface FederationField { requires?: ReadonlyArray; provides?: ReadonlyArray; belongsToValueType?: boolean; - otherKnownDirectives?: DirectiveNode[] + otherKnownDirectiveUsages?: DirectiveNode[] } export interface FederationDirective { diff --git a/federation-js/src/directives.ts b/federation-js/src/directives.ts index 5f9ca07a6..5c6d110d1 100644 --- a/federation-js/src/directives.ts +++ b/federation-js/src/directives.ts @@ -80,11 +80,14 @@ export const federationDirectives = [ ProvidesDirective, ]; -export const otherKnownDirectives = [InaccessibleDirective, TagDirective]; +export const otherKnownDirectiveDefinitions = [ + InaccessibleDirective, + TagDirective, +]; const apolloTypeSystemDirectives = [ ...federationDirectives, - ...otherKnownDirectives, + ...otherKnownDirectiveDefinitions, ]; export default apolloTypeSystemDirectives; diff --git a/federation-js/src/service/printFederatedSchema.ts b/federation-js/src/service/printFederatedSchema.ts index 017592777..090c69f59 100644 --- a/federation-js/src/service/printFederatedSchema.ts +++ b/federation-js/src/service/printFederatedSchema.ts @@ -214,7 +214,7 @@ function printObject(type: GraphQLObjectType, options?: Options): string { (isExtension ? 'extend ' : '') + `type ${type.name}${implementedInterfaces}` + // Federation addition for printing @key usages - printFederationDirectives(type) + + printFederationDirectiveUsages(type) + printFields(options, type) ); } @@ -233,7 +233,7 @@ function printInterface(type: GraphQLInterfaceType, options?: Options): string { `interface ${type.name}` + // Federation change: graphql@14 doesn't support interfaces implementing interfaces // printImplementedInterfaces(type) + - printFederationDirectives(type) + + printFederationDirectiveUsages(type) + printFields(options, type) ); } @@ -283,14 +283,14 @@ function printFields( ': ' + String(f.type) + printDeprecated(f) + - printFederationDirectives(f) + - printOtherKnownDirectives(f), + printFederationDirectiveUsages(f) + + printOtherKnownDirectiveUsages(f), ); return printBlock(fields); } // Federation change: *do* print the usages of federation directives. -function printFederationDirectives( +function printFederationDirectiveUsages( typeOrField: GraphQLNamedType | GraphQLField, ): string { if (!typeOrField.astNode) return ''; @@ -308,13 +308,13 @@ function printFederationDirectives( // Core addition: print `@tag` and `@inaccessible` directives found in subgraph // SDL into the supergraph SDL -function printOtherKnownDirectives(field: GraphQLField) { - const otherKnownDirectives = ( - field.extensions?.federation?.otherKnownDirectives ?? [] +function printOtherKnownDirectiveUsages(field: GraphQLField) { + const otherKnownDirectiveUsages = ( + field.extensions?.federation?.otherKnownDirectiveUsages ?? [] ) as DirectiveNode[]; - if (otherKnownDirectives.length < 1) return ''; - return ` ${otherKnownDirectives + if (otherKnownDirectiveUsages.length < 1) return ''; + return ` ${otherKnownDirectiveUsages .slice() .sort((a, b) => a.name.value.localeCompare(b.name.value)) .map(print) diff --git a/federation-js/src/service/printSupergraphSdl.ts b/federation-js/src/service/printSupergraphSdl.ts index f3e6c9d40..e6eff7089 100644 --- a/federation-js/src/service/printSupergraphSdl.ts +++ b/federation-js/src/service/printSupergraphSdl.ts @@ -33,7 +33,7 @@ import { assert } from '../utilities'; import { CoreDirective } from '../coreSpec'; import { getJoinDefinitions } from '../joinSpec'; import { printFieldSet } from '../composition/utils'; -import { otherKnownDirectives } from '../directives'; +import { otherKnownDirectiveDefinitions } from '../directives'; type Options = { /** @@ -172,19 +172,22 @@ function printSchemaDefinition(schema: GraphQLSchema): string { } function printCoreDirectives(schema: GraphQLSchema) { - const otherKnownDirectiveNames = otherKnownDirectives.map(({ name }) => name); - const schemaDirectiveNames = schema.getDirectives().map(({ name }) => name); - const otherKnownDirectivesToInclude = schemaDirectiveNames.filter((name) => - otherKnownDirectiveNames.includes(name), + const otherKnownDirectiveNames = otherKnownDirectiveDefinitions.map( + ({ name }) => name, ); - const otherKnownDirectivesSpecUrls = otherKnownDirectivesToInclude.map( - (name) => `https://specs.apollo.dev/${name}/v0.1`, + const schemaDirectiveNames = schema.getDirectives().map(({ name }) => name); + const otherKnownDirectiveDefinitionsToInclude = schemaDirectiveNames.filter( + (name) => otherKnownDirectiveNames.includes(name), ); + const otherKnownDirectiveSpecUrls = + otherKnownDirectiveDefinitionsToInclude.map( + (name) => `https://specs.apollo.dev/${name}/v0.1`, + ); return [ 'https://specs.apollo.dev/core/v0.1', 'https://specs.apollo.dev/join/v0.1', - ...otherKnownDirectivesSpecUrls, + ...otherKnownDirectiveSpecUrls, ].map((feature) => `\n @core(feature: ${printStringLiteral(feature)})`); } @@ -367,7 +370,7 @@ function printFields( // We don't want to print field owner directives on fields belonging to an interface type (isObjectType(type) ? printJoinFieldDirectives(f, type, context) + - printOtherKnownDirectives(f) + printOtherKnownDirectiveUsages(f) : ''), ); @@ -440,13 +443,13 @@ function printJoinFieldDirectives( // Core addition: print `@tag` and `@inaccessible` directives found in subgraph // SDL into the supergraph SDL -function printOtherKnownDirectives(field: GraphQLField) { - const otherKnownDirectives = ( - field.extensions?.federation?.otherKnownDirectives ?? [] +function printOtherKnownDirectiveUsages(field: GraphQLField) { + const otherKnownDirectiveUsages = ( + field.extensions?.federation?.otherKnownDirectiveUsages ?? [] ) as DirectiveNode[]; - if (otherKnownDirectives.length < 1) return ''; - return ` ${otherKnownDirectives + if (otherKnownDirectiveUsages.length < 1) return ''; + return ` ${otherKnownDirectiveUsages .slice() .sort((a, b) => a.name.value.localeCompare(b.name.value)) .map(print) diff --git a/query-planner-js/src/composedSchema/metadata.ts b/query-planner-js/src/composedSchema/metadata.ts index 6a84a3a01..706a921b2 100644 --- a/query-planner-js/src/composedSchema/metadata.ts +++ b/query-planner-js/src/composedSchema/metadata.ts @@ -73,5 +73,5 @@ export interface FederationFieldMetadata { graphName?: GraphName; requires?: FieldSet; provides?: FieldSet; - otherKnownDirectives?: DirectiveNode[]; + otherKnownDirectiveUsages?: DirectiveNode[]; } From 3c674101a3a108b2e8f8f799830afa5f87f531d9 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 13 Jul 2021 13:21:26 -0700 Subject: [PATCH 10/10] Add changelog entry --- federation-js/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/federation-js/CHANGELOG.md b/federation-js/CHANGELOG.md index facc66ea9..5f2c8b538 100644 --- a/federation-js/CHANGELOG.md +++ b/federation-js/CHANGELOG.md @@ -5,6 +5,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. - Skip missing types while iterating over field directive usages. It's possible to capture directive usages on fields whose types don't actually exist in the schema (due to invalid composition). See PR for more details. [PR #868](https://github.com/apollographql/federation/pull/868) +- Prevent the usage of either @tag or @inaccessible on fields that are marked as @external. [PR #869](https://github.com/apollographql/federation/pull/869) ## v0.26.0