diff --git a/federation-js/CHANGELOG.md b/federation-js/CHANGELOG.md index 4302516d4..576f6092c 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!_ +- This change is mostly a set of follow-up changes for PR #622. Most of these changes are internal (renaming, etc.). Some noteworthy changes worth mentioning are: a switch to graphql-js's `stripIgnoredCharacters` during field set printing, an update to the `join__Enum` generation algorithm, and some additional assertions. [PR #656](https://github.com/apollographql/federation/pull/656) ## v0.23.0 diff --git a/federation-js/src/__tests__/joinSpec.test.ts b/federation-js/src/__tests__/joinSpec.test.ts index 381fffa4b..1605e755d 100644 --- a/federation-js/src/__tests__/joinSpec.test.ts +++ b/federation-js/src/__tests__/joinSpec.test.ts @@ -1,5 +1,5 @@ import { fixtures } from 'apollo-federation-integration-testsuite'; -import { getJoins } from "../joinSpec"; +import { getJoinDefinitions } from "../joinSpec"; const questionableNamesRemap = { accounts: 'ServiceA', @@ -17,7 +17,7 @@ const fixturesWithQuestionableServiceNames = fixtures.map((service) => ({ describe('join__Graph enum', () => { it('correctly uniquifies and sanitizes service names', () => { - const { sanitizedServiceNames } = getJoins( + const { graphNameToEnumValueName } = getJoinDefinitions( fixturesWithQuestionableServiceNames, ); @@ -27,18 +27,18 @@ describe('join__Graph enum', () => { * 2. Numeric first characters are prefixed with _ (9product*!) * 3. Names ending in an underscore followed by numbers `_\d+` are suffixed with _ (reviews_9, servicea_2) * 4. Names are uppercased (all) - * 5. After transformations 1-4, duplicates are suffixed with _{n} where {n} is number of times we've seen the dupe (ServiceA + serviceA, servicea_2 + servicea_2_) + * 5. After transformations 1-5, duplicates are suffixed with _{n} where {n} is number of times we've seen the dupe (ServiceA + serviceA, servicea_2 + servicea_2_) * * Miscellany * (serviceA) tests the edge case of colliding with a name we generated * (servicea_2_) tests a collision against (documents) post-transformation */ - expect(sanitizedServiceNames).toMatchObject({ + expect(graphNameToEnumValueName).toMatchObject({ '9product*!': '_9PRODUCT__', - ServiceA: 'SERVICEA', + ServiceA: 'SERVICEA_2', reviews_9: 'REVIEWS_9_', - serviceA: 'SERVICEA_2', - servicea_2: 'SERVICEA_2_', + serviceA: 'SERVICEA_1', + servicea_2: 'SERVICEA_2__1', servicea_2_: 'SERVICEA_2__2', }); }) diff --git a/federation-js/src/composition/compose.ts b/federation-js/src/composition/compose.ts index 37f595e06..57d7e2c1e 100644 --- a/federation-js/src/composition/compose.ts +++ b/federation-js/src/composition/compose.ts @@ -27,7 +27,6 @@ import { mapFieldNamesToServiceName, stripExternalFieldsFromTypeDefs, typeNodesAreEquivalent, - mapValues, isFederationDirective, executableDirectiveLocations, stripTypeSystemDirectivesFromTypeDefs, @@ -46,6 +45,7 @@ import { import { validateSDL } from 'graphql/validation/validate'; import { compositionRules } from './rules'; import { printSupergraphSdl } from '../service/printSupergraphSdl'; +import { mapValues } from '../utilities'; const EmptyQueryDefinition = { kind: Kind.OBJECT_TYPE_DEFINITION, diff --git a/federation-js/src/composition/utils.ts b/federation-js/src/composition/utils.ts index 9f7909d94..9e6fd2239 100644 --- a/federation-js/src/composition/utils.ts +++ b/federation-js/src/composition/utils.ts @@ -42,6 +42,7 @@ import { FederationField, } from './types'; import federationDirectives from '../directives'; +import { assert, isNotNullOrUndefined } from '../utilities'; export function isStringValueNode(node: any): node is StringValueNode { return node.kind === Kind.STRING; @@ -147,9 +148,19 @@ function removeExternalFieldsFromExtensionVisitor< }; } -export function parseSelections(source: string) { - return (parse(`query { ${source} }`) - .definitions[0] as OperationDefinitionNode).selectionSet.selections; +/** + * For lack of a "home of federation utilities", this function is copy/pasted + * verbatim across the federation, gateway, and query-planner packages. Any changes + * made here should be reflected in the other two locations as well. + * + * @param source A string representing a FieldSet + * @returns A parsed FieldSet + */ +export function parseSelections(source: string): ReadonlyArray { + const parsed = parse(`{${source}}`); + assert(parsed.definitions.length === 1, `Unexpected } found in FieldSet`); + return (parsed.definitions[0] as OperationDefinitionNode).selectionSet + .selections; } export function hasMatchingFieldInDirectives({ @@ -556,26 +567,6 @@ export const defKindToExtKind: { [kind: string]: string } = { [Kind.INPUT_OBJECT_TYPE_DEFINITION]: Kind.INPUT_OBJECT_TYPE_EXTENSION, }; -// Transform an object's values via a callback function -export function mapValues( - object: Record, - callback: (value: T) => U, -): Record { - const result: Record = Object.create(null); - - for (const [key, value] of Object.entries(object)) { - result[key] = callback(value); - } - - return result; -} - -export function isNotNullOrUndefined( - value: T | null | undefined, -): value is T { - return value !== null && typeof value !== 'undefined'; -} - export const executableDirectiveLocations = [ 'QUERY', 'MUTATION', diff --git a/federation-js/src/composition/validate/postComposition/keysMatchBaseService.ts b/federation-js/src/composition/validate/postComposition/keysMatchBaseService.ts index ae0cf8adc..9a67db782 100644 --- a/federation-js/src/composition/validate/postComposition/keysMatchBaseService.ts +++ b/federation-js/src/composition/validate/postComposition/keysMatchBaseService.ts @@ -1,11 +1,16 @@ -import { isObjectType, GraphQLError, SelectionNode } from 'graphql'; +import { + isObjectType, + GraphQLError, + SelectionNode, + stripIgnoredCharacters, + print, +} from 'graphql'; import { logServiceAndType, errorWithCode, getFederationMetadata, } from '../../utils'; import { PostCompositionValidator } from '.'; -import { printWithReducedWhitespace } from '../../../service'; /** * 1. KEY_MISSING_ON_BASE - Originating types must specify at least 1 @key directive @@ -82,5 +87,7 @@ export const keysMatchBaseService: PostCompositionValidator = function ({ }; function printFieldSet(selections: readonly SelectionNode[]): string { - return selections.map(printWithReducedWhitespace).join(' '); + return selections + .map((selection) => stripIgnoredCharacters(print(selection))) + .join(' '); } diff --git a/federation-js/src/composition/validate/preComposition/keyFieldsMissingExternal.ts b/federation-js/src/composition/validate/preComposition/keyFieldsMissingExternal.ts index be0bffd76..febd1b9a6 100644 --- a/federation-js/src/composition/validate/preComposition/keyFieldsMissingExternal.ts +++ b/federation-js/src/composition/validate/preComposition/keyFieldsMissingExternal.ts @@ -15,8 +15,8 @@ import { isStringValueNode, logServiceAndType, errorWithCode, - isNotNullOrUndefined } from '../../utils'; +import { isNotNullOrUndefined } from '../../../utilities'; /** * For every @key directive, it must reference a field marked as @external diff --git a/federation-js/src/joinSpec.ts b/federation-js/src/joinSpec.ts index 6d5422e4f..1412999a5 100644 --- a/federation-js/src/joinSpec.ts +++ b/federation-js/src/joinSpec.ts @@ -7,6 +7,7 @@ import { GraphQLNonNull, } from 'graphql'; import { ServiceDefinition } from './composition'; +import { mapGetOrSet } from './utilities/mapGetOrSet'; const FieldSetScalar = new GraphQLScalarType({ name: 'join__FieldSet', @@ -27,57 +28,77 @@ const JoinGraphDirective = new GraphQLDirective({ /** * Expectations - * 1. Non-Alphanumeric characters are replaced with _ (alphaNumericUnderscoreOnly) - * 2. Numeric first characters are prefixed with _ (noNumericFirstChar) - * 3. Names ending in an underscore followed by numbers `_\d+` are suffixed with _ (noUnderscoreNumericEnding) - * 4. Names are uppercased (toUpper) - * 5. After transformations 1-4, duplicates are suffixed with _{n} where {n} is number of times we've seen the dupe + * 1. The input is first sorted using `String.localeCompare`, so the output is deterministic + * 2. Non-Alphanumeric characters are replaced with _ (alphaNumericUnderscoreOnly) + * 3. Numeric first characters are prefixed with _ (noNumericFirstChar) + * 4. Names ending in an underscore followed by numbers `_\d+` are suffixed with _ (noUnderscoreNumericEnding) + * 5. Names are uppercased (toUpper) + * 6. After transformations 1-5, duplicates are suffixed with _{n} where {n} is number of times we've seen the dupe * * Note: Collisions with name's we've generated are also accounted for */ function getJoinGraphEnum(serviceList: ServiceDefinition[]) { - // Track whether we've seen a name and how many times - const nameMap: Map = new Map(); - // Build a map of original service name to generated name - const sanitizedServiceNames: Record = Object.create(null); + const sortedServiceList = serviceList + .slice() + .sort((a, b) => a.name.localeCompare(b.name)); - function uniquifyAndSanitizeGraphQLName(name: string) { - // Transforms to ensure valid graphql `Name` - const alphaNumericUnderscoreOnly = name.replace(/[^_a-zA-Z0-9]/g, '_'); - const noNumericFirstChar = alphaNumericUnderscoreOnly.match(/^[0-9]/) + function sanitizeGraphQLName(name: string) { + // replace all non-word characters (\W). Word chars are _a-zA-Z0-9 + const alphaNumericUnderscoreOnly = name.replace(/[\W]/g, '_'); + // prefix a digit in the first position with an _ + const noNumericFirstChar = alphaNumericUnderscoreOnly.match(/^\d/) ? '_' + alphaNumericUnderscoreOnly : alphaNumericUnderscoreOnly; - const noUnderscoreNumericEnding = noNumericFirstChar.match(/_[0-9]+$/) + // suffix an underscore + digit in the last position with an _ + const noUnderscoreNumericEnding = noNumericFirstChar.match(/_\d+$/) ? noNumericFirstChar + '_' : noNumericFirstChar; // toUpper not really necessary but follows convention of enum values const toUpper = noUnderscoreNumericEnding.toLocaleUpperCase(); + return toUpper; + } + + // duplicate enum values can occur due to sanitization and must be accounted for + // collect the duplicates in an array so we can uniquify them in a second pass. + const sanitizedNameToServiceDefinitions: Map< + string, + ServiceDefinition[] + > = new Map(); + for (const service of sortedServiceList) { + const { name } = service; + const sanitized = sanitizeGraphQLName(name); + mapGetOrSet(sanitizedNameToServiceDefinitions, sanitized, []).push(service); + } - // Uniquifying post-transform - const nameCount = nameMap.get(toUpper); - if (nameCount) { - // Collision - bump counter by one - nameMap.set(toUpper, nameCount + 1); - const uniquified = `${toUpper}_${nameCount + 1}`; - // We also now need another entry for the name we just generated - nameMap.set(uniquified, 1); - sanitizedServiceNames[name] = uniquified; - return uniquified; + // if no duplicates for a given name, add it as is + // if duplicates exist, append _{n} (index-1) to each duplicate in the array + const enumValueNameToServiceDefinition: Record< + string, + ServiceDefinition + > = Object.create(null); + for (const [sanitizedName, services] of sanitizedNameToServiceDefinitions) { + if (services.length === 1) { + enumValueNameToServiceDefinition[sanitizedName] = services[0]; } else { - nameMap.set(toUpper, 1); - sanitizedServiceNames[name] = toUpper; - return toUpper; + for (const [index, service] of services.entries()) { + enumValueNameToServiceDefinition[ + `${sanitizedName}_${index + 1}` + ] = service; + } } } + const entries = Object.entries(enumValueNameToServiceDefinition); return { - sanitizedServiceNames, + graphNameToEnumValueName: Object.fromEntries( + entries.map(([enumValueName, service]) => [service.name, enumValueName]), + ), JoinGraphEnum: new GraphQLEnumType({ name: 'join__Graph', values: Object.fromEntries( - serviceList.map((service) => [ - uniquifyAndSanitizeGraphQLName(service.name), + entries.map(([enumValueName, service]) => [ + enumValueName, { value: service }, ]), ), @@ -115,8 +136,8 @@ function getJoinOwnerDirective(JoinGraphEnum: GraphQLEnumType) { }); } -export function getJoins(serviceList: ServiceDefinition[]) { - const { sanitizedServiceNames, JoinGraphEnum } = getJoinGraphEnum(serviceList); +export function getJoinDefinitions(serviceList: ServiceDefinition[]) { + const { graphNameToEnumValueName, JoinGraphEnum } = getJoinGraphEnum(serviceList); const JoinFieldDirective = getJoinFieldDirective(JoinGraphEnum); const JoinOwnerDirective = getJoinOwnerDirective(JoinGraphEnum); @@ -135,7 +156,7 @@ export function getJoins(serviceList: ServiceDefinition[]) { }); return { - sanitizedServiceNames, + graphNameToEnumValueName, FieldSetScalar, JoinTypeDirective, JoinFieldDirective, diff --git a/federation-js/src/service/__tests__/printSupergraphSdl.test.ts b/federation-js/src/service/__tests__/printSupergraphSdl.test.ts index 009c95bc1..9c5b86806 100644 --- a/federation-js/src/service/__tests__/printSupergraphSdl.test.ts +++ b/federation-js/src/service/__tests__/printSupergraphSdl.test.ts @@ -75,7 +75,7 @@ describe('printSupergraphSdl', () => { price: String @join__field(graph: PRODUCT) details: ProductDetailsBook @join__field(graph: PRODUCT) reviews: [Review] @join__field(graph: REVIEWS) - relatedReviews: [Review!]! @join__field(graph: REVIEWS, requires: \\"similarBooks { isbn }\\") + relatedReviews: [Review!]! @join__field(graph: REVIEWS, requires: \\"similarBooks{isbn}\\") } union Brand = Ikea | Amazon @@ -251,7 +251,7 @@ describe('printSupergraphSdl', () => { type User @join__owner(graph: ACCOUNTS) @join__type(graph: ACCOUNTS, key: \\"id\\") - @join__type(graph: ACCOUNTS, key: \\"username name { first last }\\") + @join__type(graph: ACCOUNTS, key: \\"username name{first last}\\") @join__type(graph: INVENTORY, key: \\"id\\") @join__type(graph: PRODUCT, key: \\"id\\") @join__type(graph: REVIEWS, key: \\"id\\") @@ -262,12 +262,12 @@ describe('printSupergraphSdl', () => { birthDate(locale: String): String @join__field(graph: ACCOUNTS) account: AccountType @join__field(graph: ACCOUNTS) metadata: [UserMetadata] @join__field(graph: ACCOUNTS) - goodDescription: Boolean @join__field(graph: INVENTORY, requires: \\"metadata { description }\\") + goodDescription: Boolean @join__field(graph: INVENTORY, requires: \\"metadata{description}\\") vehicle: Vehicle @join__field(graph: PRODUCT) thing: Thing @join__field(graph: PRODUCT) reviews: [Review] @join__field(graph: REVIEWS) numberOfReviews: Int! @join__field(graph: REVIEWS) - goodAddress: Boolean @join__field(graph: REVIEWS, requires: \\"metadata { address }\\") + goodAddress: Boolean @join__field(graph: REVIEWS, requires: \\"metadata{address}\\") } type UserMetadata { diff --git a/federation-js/src/service/printFederatedSchema.ts b/federation-js/src/service/printFederatedSchema.ts index 0d963d6c0..b96c46957 100644 --- a/federation-js/src/service/printFederatedSchema.ts +++ b/federation-js/src/service/printFederatedSchema.ts @@ -31,7 +31,6 @@ import { GraphQLEnumValue, GraphQLString, DEFAULT_DEPRECATION_REASON, - ASTNode, } from 'graphql'; import { Maybe } from '../composition'; import { isFederationType } from '../types'; @@ -305,12 +304,6 @@ function printFederationDirectives( return dedupedDirectives.length > 0 ? ' ' + dedupedDirectives.join(' ') : ''; } -export function printWithReducedWhitespace(ast: ASTNode): string { - return print(ast) - .replace(/\s+/g, ' ') - .trim(); -} - function printBlock(items: string[]) { return items.length !== 0 ? ' {\n' + items.join('\n') + '\n}' : ''; } diff --git a/federation-js/src/service/printSupergraphSdl.ts b/federation-js/src/service/printSupergraphSdl.ts index 9957907b3..7381772f2 100644 --- a/federation-js/src/service/printSupergraphSdl.ts +++ b/federation-js/src/service/printSupergraphSdl.ts @@ -25,12 +25,13 @@ import { GraphQLEnumValue, GraphQLString, DEFAULT_DEPRECATION_REASON, - ASTNode, SelectionNode, + stripIgnoredCharacters, } from 'graphql'; import { Maybe, FederationType, FederationField, ServiceDefinition } from '../composition'; +import { assert } from '../utilities'; import { CoreDirective } from '../coreSpec'; -import { getJoins } from '../joinSpec'; +import { getJoinDefinitions } from '../joinSpec'; type Options = { /** @@ -47,7 +48,7 @@ type Options = { interface PrintingContext { // Core addition: we need access to a map from serviceName to its corresponding // sanitized / uniquified enum value `Name` from the `join__Graph` enum - sanitizedServiceNames?: Record; + graphNameToEnumValueName?: Record; } /** @@ -72,8 +73,8 @@ export function printSupergraphSdl( JoinOwnerDirective, JoinGraphEnum, JoinGraphDirective, - sanitizedServiceNames, - } = getJoins(serviceList); + graphNameToEnumValueName, + } = getJoinDefinitions(serviceList); schema = new GraphQLSchema({ ...config, @@ -89,7 +90,7 @@ export function printSupergraphSdl( }); const context: PrintingContext = { - sanitizedServiceNames, + graphNameToEnumValueName, } return printFilteredSchema( @@ -246,10 +247,14 @@ function printTypeJoinDirectives( // We don't want to print an owner for interface types const shouldPrintOwner = isObjectType(type); + + const ownerGraphEnumValue = context.graphNameToEnumValueName?.[ownerService]; + assert( + ownerGraphEnumValue, + `Unexpected enum value missing for subgraph ${ownerService}`, + ); const joinOwnerString = shouldPrintOwner - ? `\n @join__owner(graph: ${ - context.sanitizedServiceNames?.[ownerService] ?? ownerService - })` + ? `\n @join__owner(graph: ${ownerGraphEnumValue})` : ''; return ( @@ -257,12 +262,17 @@ function printTypeJoinDirectives( [ownerEntry, ...restEntries] .map(([service, keys = []]) => keys - .map( - (selections) => - `\n @join__type(graph: ${ - context.sanitizedServiceNames?.[service] ?? service - }, key: ${printStringLiteral(printFieldSet(selections))})`, - ) + .map((selections) => { + const typeGraphEnumValue = + context.graphNameToEnumValueName?.[service]; + assert( + typeGraphEnumValue, + `Unexpected enum value missing for subgraph ${service}`, + ); + return `\n @join__type(graph: ${typeGraphEnumValue}, key: ${printStringLiteral( + printFieldSet(selections), + )})`; + }) .join(''), ) .join('') @@ -353,19 +363,15 @@ function printFields( return printBlock(fields, isEntity); } -export function printWithReducedWhitespace(ast: ASTNode): string { - return print(ast) - .replace(/\s+/g, ' ') - .trim(); -} - /** * Core change: print fieldsets for @join__field's @key, @requires, and @provides args * * @param selections */ function printFieldSet(selections: readonly SelectionNode[]): string { - return `${selections.map(printWithReducedWhitespace).join(' ')}`; + return selections + .map((selection) => stripIgnoredCharacters(print(selection))) + .join(' '); } /** @@ -391,18 +397,14 @@ function printJoinFieldDirectives( // Because we print `@join__type` directives based on the keys, but only used to // look at the owning service here, that meant we would print `@join__field` // without a corresponding `@join__type`, which is invalid according to the spec. - if ( - parentType.extensions?.federation?.serviceName && - parentType.extensions?.federation?.keys - ) { - return ( - printed + - `graph: ${ - context.sanitizedServiceNames?.[ - parentType.extensions?.federation.serviceName - ] ?? parentType.extensions?.federation.serviceName - })` + const { serviceName, keys } = parentType.extensions?.federation; + if (serviceName && keys) { + const enumValue = context.graphNameToEnumValueName?.[serviceName]; + assert( + enumValue, + `Unexpected enum value missing for subgraph ${serviceName}`, ); + return printed + `graph: ${enumValue})`; } return ''; } @@ -417,7 +419,7 @@ function printJoinFieldDirectives( if (serviceName && serviceName.length > 0) { directiveArgs.push( - `graph: ${context.sanitizedServiceNames?.[serviceName] ?? serviceName}`, + `graph: ${context.graphNameToEnumValueName?.[serviceName] ?? serviceName}`, ); } diff --git a/federation-js/src/utilities/assert.ts b/federation-js/src/utilities/assert.ts new file mode 100644 index 000000000..be1bbf0ca --- /dev/null +++ b/federation-js/src/utilities/assert.ts @@ -0,0 +1,14 @@ +/** + * For lack of a "home of federation utilities", this function is copy/pasted + * verbatim across the federation, gateway, and query-planner packages. Any changes + * made here should be reflected in the other two locations as well. + * + * @param condition + * @param message + * @throws + */ +export function assert(condition: any, message: string): asserts condition { + if (!condition) { + throw new Error(message); + } +} diff --git a/federation-js/src/utilities/index.ts b/federation-js/src/utilities/index.ts new file mode 100644 index 000000000..ce06261ed --- /dev/null +++ b/federation-js/src/utilities/index.ts @@ -0,0 +1,4 @@ +export { assert } from './assert'; +export { isNotNullOrUndefined } from './isNotNullOrUndefined'; +export { mapGetOrSet } from './mapGetOrSet'; +export { mapValues } from './mapValues'; diff --git a/federation-js/src/utilities/isNotNullOrUndefined.ts b/federation-js/src/utilities/isNotNullOrUndefined.ts new file mode 100644 index 000000000..6837314ae --- /dev/null +++ b/federation-js/src/utilities/isNotNullOrUndefined.ts @@ -0,0 +1,5 @@ +export function isNotNullOrUndefined( + value: T | null | undefined, +): value is T { + return value !== null && typeof value !== 'undefined'; +} diff --git a/federation-js/src/utilities/mapGetOrSet.ts b/federation-js/src/utilities/mapGetOrSet.ts new file mode 100644 index 000000000..afc835d4e --- /dev/null +++ b/federation-js/src/utilities/mapGetOrSet.ts @@ -0,0 +1,6 @@ +export function mapGetOrSet(map: Map, key: K, valueToSet: V): V { + if (!map.has(key)) { + map.set(key, valueToSet); + } + return map.get(key)!; +} diff --git a/federation-js/src/utilities/mapValues.ts b/federation-js/src/utilities/mapValues.ts new file mode 100644 index 000000000..f493eb6db --- /dev/null +++ b/federation-js/src/utilities/mapValues.ts @@ -0,0 +1,13 @@ +// Transform an object's values via a callback function +export function mapValues( + object: Record, + callback: (value: T) => U, +): Record { + const result: Record = Object.create(null); + + for (const [key, value] of Object.entries(object)) { + result[key] = callback(value); + } + + return result; +} diff --git a/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts b/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts index a524a7a59..40caa74de 100644 --- a/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts +++ b/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts @@ -74,7 +74,7 @@ describe('loadSupergraphSdlFromStorage', () => { price: String @join__field(graph: PRODUCT) details: ProductDetailsBook @join__field(graph: PRODUCT) reviews: [Review] @join__field(graph: REVIEWS) - relatedReviews: [Review!]! @join__field(graph: REVIEWS, requires: \\"similarBooks { isbn }\\") + relatedReviews: [Review!]! @join__field(graph: REVIEWS, requires: \\"similarBooks{isbn}\\") } union Brand = Ikea | Amazon @@ -250,7 +250,7 @@ describe('loadSupergraphSdlFromStorage', () => { type User @join__owner(graph: ACCOUNTS) @join__type(graph: ACCOUNTS, key: \\"id\\") - @join__type(graph: ACCOUNTS, key: \\"username name { first last }\\") + @join__type(graph: ACCOUNTS, key: \\"username name{first last}\\") @join__type(graph: INVENTORY, key: \\"id\\") @join__type(graph: PRODUCT, key: \\"id\\") @join__type(graph: REVIEWS, key: \\"id\\") @@ -261,12 +261,12 @@ describe('loadSupergraphSdlFromStorage', () => { birthDate(locale: String): String @join__field(graph: ACCOUNTS) account: AccountType @join__field(graph: ACCOUNTS) metadata: [UserMetadata] @join__field(graph: ACCOUNTS) - goodDescription: Boolean @join__field(graph: INVENTORY, requires: \\"metadata { description }\\") + goodDescription: Boolean @join__field(graph: INVENTORY, requires: \\"metadata{description}\\") vehicle: Vehicle @join__field(graph: PRODUCT) thing: Thing @join__field(graph: PRODUCT) reviews: [Review] @join__field(graph: REVIEWS) numberOfReviews: Int! @join__field(graph: REVIEWS) - goodAddress: Boolean @join__field(graph: REVIEWS, requires: \\"metadata { address }\\") + goodAddress: Boolean @join__field(graph: REVIEWS, requires: \\"metadata{address}\\") } type UserMetadata { diff --git a/gateway-js/src/index.ts b/gateway-js/src/index.ts index f00f2af78..e450b164c 100644 --- a/gateway-js/src/index.ts +++ b/gateway-js/src/index.ts @@ -695,12 +695,7 @@ export class ApolloGateway implements GraphQLService { throw Error(`Couldn't find graph map in composed schema`); } - const serviceList = Object.values(graphMap).map(graph => ({ - name: graph.name, - url: graph.url - })) - - return serviceList; + return Array.from(graphMap.values()); } private createSchemaFromSupergraphSdl(supergraphSdl: string) { diff --git a/gateway-js/src/utilities/assert.ts b/gateway-js/src/utilities/assert.ts new file mode 100644 index 000000000..a92dfc302 --- /dev/null +++ b/gateway-js/src/utilities/assert.ts @@ -0,0 +1,14 @@ +/** + * For lack of a "home of federation utilities", this function is copy/pasted + * verbatim across the federation, gateway, and query-planner packages. Any changes + * made here should be reflected in the other two locations as well. + * + * @param condition + * @param message + * @throws {@link Error} + */ +export function assert(condition: any, message: string): asserts condition { + if (!condition) { + throw new Error(message); + } +} diff --git a/gateway-js/src/utilities/graphql.ts b/gateway-js/src/utilities/graphql.ts index 44feedf81..592c02973 100644 --- a/gateway-js/src/utilities/graphql.ts +++ b/gateway-js/src/utilities/graphql.ts @@ -10,10 +10,10 @@ import { NamedTypeNode, OperationDefinitionNode, parse, - print, SelectionNode, TypeNode, } from 'graphql'; +import { assert } from './assert'; export function getResponseName(node: FieldNode): string { return node.alias ? node.alias.value : node.name.value; @@ -42,13 +42,20 @@ export function astFromType(type: GraphQLType): TypeNode { } } -export function printWithReducedWhitespace(ast: ASTNode): string { - return print(ast) - .replace(/\s+/g, ' ') - .trim(); -} - +/** + * For lack of a "home of federation utilities", this function is copy/pasted + * verbatim across the federation, gateway, and query-planner packages. Any changes + * made here should be reflected in the other two locations as well. + * + * @param source A string representing a FieldSet + * @returns A parsed FieldSet + */ export function parseSelections(source: string): ReadonlyArray { - return (parse(`query { ${source} }`) - .definitions[0] as OperationDefinitionNode).selectionSet.selections; + const parsed = parse(`{${source}}`); + assert( + parsed.definitions.length === 1, + `Invalid FieldSet provided: '${source}'. FieldSets may not contain operations within them.`, + ); + return (parsed.definitions[0] as OperationDefinitionNode).selectionSet + .selections; } diff --git a/harmonizer/src/snapshots/harmonizer__tests__it_works.snap b/harmonizer/src/snapshots/harmonizer__tests__it_works.snap index 294ddcb37..c910cef0a 100644 --- a/harmonizer/src/snapshots/harmonizer__tests__it_works.snap +++ b/harmonizer/src/snapshots/harmonizer__tests__it_works.snap @@ -23,8 +23,8 @@ directive @join__graph(name: String!, url: String!) on ENUM_VALUE scalar join__FieldSet enum join__Graph { - USERS @join__graph(name: "users" url: "undefined") MOVIES @join__graph(name: "movies" url: "undefined") + USERS @join__graph(name: "users" url: "undefined") } type Movie { diff --git a/query-planner-js/CHANGELOG.md b/query-planner-js/CHANGELOG.md index ff93e9c48..eda3a7361 100644 --- a/query-planner-js/CHANGELOG.md +++ b/query-planner-js/CHANGELOG.md @@ -4,6 +4,8 @@ > 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. +- This change is mostly a set of follow-up changes for PR #622. Most of these changes are internal (renaming, etc.). Some noteworthy changes worth mentioning are: the splitting of entity and value type metadata types and a conversion of GraphMap to an actual `Map` (which resulted in some additional assertions). [PR #656](https://github.com/apollographql/federation/pull/656) + # v0.1.1 - Remove unnecessary dependency on `@apollo/query-planner-wasm` diff --git a/query-planner-js/src/FieldSet.ts b/query-planner-js/src/FieldSet.ts index a508c6510..57b8645e1 100644 --- a/query-planner-js/src/FieldSet.ts +++ b/query-planner-js/src/FieldSet.ts @@ -200,6 +200,10 @@ function mergeFieldNodeSelectionSets( (node): node is FieldNode => node.kind === Kind.FIELD, ); + // Committed by @trevor-scheer but authored by @martijnwalraven + // XXX: This code has more problems and should be replaced by proper recursive + // selection set merging, but removing the unnecessary distinction between + // aliased fields and non-aliased fields at least fixes the test. const mergedFieldNodes = Array.from( groupBy((node: FieldNode) => node.alias?.value ?? node.name.value)( fieldNodes, diff --git a/query-planner-js/src/__tests__/features/multiple-keys/supergraphSdl.graphql b/query-planner-js/src/__tests__/features/multiple-keys/supergraphSdl.graphql index 785c190d9..51bb4ed06 100644 --- a/query-planner-js/src/__tests__/features/multiple-keys/supergraphSdl.graphql +++ b/query-planner-js/src/__tests__/features/multiple-keys/supergraphSdl.graphql @@ -23,9 +23,9 @@ type Group { scalar join__FieldSet enum join__Graph { - USERS @join__graph(name: "users" url: "undefined") - REVIEWS @join__graph(name: "reviews" url: "undefined") ACTUARY @join__graph(name: "actuary" url: "undefined") + REVIEWS @join__graph(name: "reviews" url: "undefined") + USERS @join__graph(name: "users" url: "undefined") } type Query { diff --git a/query-planner-js/src/buildQueryPlan.ts b/query-planner-js/src/buildQueryPlan.ts index 805b3ec45..e3a7af5f0 100644 --- a/query-planner-js/src/buildQueryPlan.ts +++ b/query-planner-js/src/buildQueryPlan.ts @@ -52,7 +52,11 @@ import { } from './QueryPlan'; import { getFieldDef, getResponseName } from './utilities/graphql'; import { MultiMap } from './utilities/MultiMap'; -import { getFederationMetadataForType, getFederationMetadataForField } from './composedSchema'; +import { + getFederationMetadataForType, + getFederationMetadataForField, + isEntityTypeMetadata, +} from './composedSchema'; import { DebugLogger } from './utilities/debug'; @@ -432,6 +436,12 @@ function splitSubfields( const { scope, fieldNode, fieldDef } = field; const { parentType } = scope; + // Committed by @trevor-scheer but authored by @martijnwalraven + // Treat abstract types as value types to replicate type explosion fix + // XXX: this replicates the behavior of the Rust query planner implementation, + // in order to get the tests passing before making further changes. But the + // type explosion fix this depends on is fundamentally flawed and needs to + // be replaced. const parentIsValueType = !isObjectType(parentType) || getFederationMetadataForType(parentType)?.isValueType; let baseService, owningService; @@ -842,6 +852,21 @@ function collectFields( break; } + // Committed by @trevor-scheer but authored by @martijnwalraven + // This replicates the behavior added to the Rust query planner in #178. + // Unfortunately, the logic in there is seriously flawed, and may lead to + // unexpected results. The assumption seems to be that fields with the + // same parent type are always nested in the same inline fragment. That + // is not necessarily true however, and because we take the directives + // from the scope of the first field with a particular parent type, + // those directives will be applied to all other fields that have the + // same parent type even if the directives aren't meant to apply to them + // because they were nested in a different inline fragment. (That also + // means that if the scope of the first field doesn't have directives, + // directives that would have applied to other fields will be lost.) + // Note that this also applies to `@skip` and `@include`, which could + // lead to invalid query plans that fail at runtime because expected + // fields are missing from a subgraph response. newScope.directives = selection.directives; collectFields( @@ -1136,7 +1161,8 @@ export class QueryPlanningContext { } getBaseService(parentType: GraphQLObjectType): string | undefined { - return getFederationMetadataForType(parentType)?.graphName; + const type = getFederationMetadataForType(parentType); + return (type && isEntityTypeMetadata(type)) ? type.graphName : undefined; } getOwningService( @@ -1170,7 +1196,11 @@ export class QueryPlanningContext { }); for (const possibleType of this.getPossibleTypes(parentType)) { - const keys = getFederationMetadataForType(possibleType)?.keys?.get(serviceName); + const type = getFederationMetadataForType(possibleType); + const keys = + type && isEntityTypeMetadata(type) + ? type.keys.get(serviceName) + : undefined; if (!(keys && keys.length > 0)) continue; diff --git a/query-planner-js/src/composedSchema/buildComposedSchema.ts b/query-planner-js/src/composedSchema/buildComposedSchema.ts index 9630c9b19..49fbcace6 100644 --- a/query-planner-js/src/composedSchema/buildComposedSchema.ts +++ b/query-planner-js/src/composedSchema/buildComposedSchema.ts @@ -14,15 +14,16 @@ import { assert } from '../utilities/assert'; import { getArgumentValuesForDirective, getArgumentValuesForRepeatableDirective, - isASTKind, - parseSelections, + parseFieldSet, } from '../utilities/graphql'; import { MultiMap } from '../utilities/MultiMap'; import { FederationFieldMetadata, FederationTypeMetadata, - FieldSet, + FederationEntityTypeMetadata, GraphMap, + isEntityTypeMetadata, + Graph, } from './metadata'; export function buildComposedSchema(document: DocumentNode): GraphQLSchema { @@ -82,7 +83,7 @@ export function buildComposedSchema(document: DocumentNode): GraphQLSchema { const graphEnumType = schema.getType(`${joinName}__Graph`); assert(isEnumType(graphEnumType), `${joinName}__Graph should be an enum`); - const graphMap: GraphMap = Object.create(null); + const graphMap: GraphMap = new Map(); schema.extensions = { ...schema.extensions, @@ -106,10 +107,10 @@ export function buildComposedSchema(document: DocumentNode): GraphQLSchema { const graphName: string = graphDirectiveArgs['name']; const url: string = graphDirectiveArgs['url']; - graphMap[name] = { + graphMap.set(name, { name: graphName, url, - }; + }); } for (const type of Object.values(schema.getTypeMap())) { @@ -128,15 +129,25 @@ export function buildComposedSchema(document: DocumentNode): GraphQLSchema { type.astNode, ); - const typeMetadata: FederationTypeMetadata = ownerDirectiveArgs - ? { - graphName: graphMap[ownerDirectiveArgs?.['graph']].name, - keys: new MultiMap(), - isValueType: false, - } - : { - isValueType: true, - }; + let typeMetadata: FederationTypeMetadata; + if (ownerDirectiveArgs) { + assert( + ownerDirectiveArgs.graph, + `@${ownerDirective.name} directive requires a \`graph\` argument`, + ); + const graph = graphMap.get(ownerDirectiveArgs.graph); + assertGraphFound(graph, ownerDirectiveArgs.graph, ownerDirective.name); + + typeMetadata = { + graphName: graph.name, + keys: new MultiMap(), + isValueType: false, + }; + } else { + typeMetadata = { + isValueType: true, + }; + } type.extensions = { ...type.extensions, @@ -148,18 +159,31 @@ export function buildComposedSchema(document: DocumentNode): GraphQLSchema { type.astNode, ); + // The assertion here guarantees the safety of the type cast below + // (typeMetadata as FederationEntityTypeMetadata). Adjustments to this assertion + // should account for this dependency. assert( - !(typeMetadata.isValueType && typeDirectivesArgs.length >= 1), + isEntityTypeMetadata(typeMetadata) || typeDirectivesArgs.length === 0, `GraphQL type "${type.name}" cannot have a @${typeDirective.name} \ directive without an @${ownerDirective.name} directive`, ); for (const typeDirectiveArgs of typeDirectivesArgs) { - const graphName = graphMap[typeDirectiveArgs['graph']].name; + assert( + typeDirectiveArgs.graph, + `GraphQL type "${type.name}" must provide a \`graph\` argument to the @${typeDirective.name} directive`, + ); + const graph = graphMap.get(typeDirectiveArgs.graph); + assertGraphFound(graph, typeDirectiveArgs.graph, typeDirective.name); const keyFields = parseFieldSet(typeDirectiveArgs['key']); - typeMetadata.keys?.add(graphName, keyFields); + // We know we won't actually be looping here in the case of a value type + // based on the assertion above, but TS is not able to infer that. + (typeMetadata as FederationEntityTypeMetadata).keys.add( + graph.name, + keyFields, + ); } for (const fieldDef of Object.values(type.getFields())) { @@ -175,9 +199,15 @@ directive without an @${ownerDirective.name} directive`, if (!fieldDirectiveArgs) continue; - const fieldMetadata: FederationFieldMetadata = { - graphName: graphMap[fieldDirectiveArgs?.['graph']]?.name, - }; + let fieldMetadata: FederationFieldMetadata; + if (fieldDirectiveArgs.graph) { + const graph = graphMap.get(fieldDirectiveArgs.graph); + // This should never happen, but the assertion guarantees the existence of `graph` + assertGraphFound(graph, fieldDirectiveArgs.graph, fieldDirective.name); + fieldMetadata = { graphName: graph.name }; + } else { + fieldMetadata = { graphName: undefined }; + } fieldDef.extensions = { ...fieldDef.extensions, @@ -226,15 +256,11 @@ directive without an @${ownerDirective.name} directive`, type NamedSchemaElement = GraphQLDirective | GraphQLNamedType; -function parseFieldSet(source: string): FieldSet { - const selections = parseSelections(source); - +// This should never happen, hence 'programming error', but this assertion +// guarantees the existence of `graph`. +function assertGraphFound(graph: Graph | undefined, graphName: string, directiveName: string): asserts graph { assert( - selections.every(isASTKind('Field', 'InlineFragment')), - `Field sets may not contain fragment spreads, but found: "${source}"`, + graph, + `Programming error: found unexpected \`graph\` argument value "${graphName}" in @${directiveName} directive`, ); - - assert(selections.length > 0, `Field sets may not be empty`); - - return selections; } diff --git a/query-planner-js/src/composedSchema/metadata.ts b/query-planner-js/src/composedSchema/metadata.ts index 45f5174bf..9549331f2 100644 --- a/query-planner-js/src/composedSchema/metadata.ts +++ b/query-planner-js/src/composedSchema/metadata.ts @@ -32,6 +32,11 @@ export function getFederationMetadataForField( } export type GraphName = string; + +// Without rewriting a number of AST types from graphql-js, this typing is +// technically too relaxed. Recursive selections are not excluded from containing +// FragmentSpreads, which is what this type is aiming to achieve (and accomplishes +// at the root level, but not recursively) export type FieldSet = readonly (FieldNode | InlineFragmentNode)[]; export interface Graph { @@ -39,14 +44,29 @@ export interface Graph { url: string; } -export type GraphMap = { [graphName: string]: Graph }; +export type GraphMap = Map; export interface FederationSchemaMetadata { graphs: GraphMap; } -export interface FederationTypeMetadata { - graphName?: GraphName; - keys?: MultiMap; - isValueType: boolean; + +export type FederationTypeMetadata = + | FederationEntityTypeMetadata + | FederationValueTypeMetadata; + +export interface FederationEntityTypeMetadata { + graphName: GraphName; + keys: MultiMap, + isValueType: false; +} + +interface FederationValueTypeMetadata { + isValueType: true; +} + +export function isEntityTypeMetadata( + metadata: FederationTypeMetadata, +): metadata is FederationEntityTypeMetadata { + return !metadata.isValueType; } export interface FederationFieldMetadata { diff --git a/query-planner-js/src/utilities/__tests__/graphql.test.ts b/query-planner-js/src/utilities/__tests__/graphql.test.ts new file mode 100644 index 000000000..633c6938e --- /dev/null +++ b/query-planner-js/src/utilities/__tests__/graphql.test.ts @@ -0,0 +1,57 @@ +import { FieldNode } from 'graphql'; +import { parseFieldSet, parseSelections } from '../graphql'; + +describe('graphql utility functions', () => { + describe('parseSelections', () => { + it('parses a valid FieldSet', () => { + const fieldSet = 'foo bar'; + const parsed = parseSelections(fieldSet); + expect(parsed).toHaveLength(2); + }); + + it('parses a nested FieldSet', () => { + const fieldSet = 'foo { bar }'; + const parsed = parseSelections(fieldSet); + expect(parsed).toHaveLength(1); + expect((parsed[0] as FieldNode).selectionSet?.selections).toHaveLength(1); + }); + + it('throws when injecting an extra operation', () => { + const invalidFieldSet = 'foo } query X { bar'; + expect(() => + parseSelections(invalidFieldSet), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid FieldSet provided: 'foo } query X { bar'. FieldSets may not contain operations within them."`, + ); + }); + }); + + describe('parseFieldSet', () => { + it('parses valid `FieldSet`s', () => { + const fieldSet = 'foo bar'; + const parsed = parseFieldSet(fieldSet); + expect(parsed).toHaveLength(2); + }); + + it('disallows empty `FieldSet`s', () => { + const invalid = ''; + expect(() => parseFieldSet(invalid)).toThrowErrorMatchingInlineSnapshot( + `"Syntax Error: Expected Name, found \\"}\\"."`, + ); + }); + + it('disallows `FragmentSpread`s', () => { + const invalid = 'foo ...Bar'; + expect(() => parseFieldSet(invalid)).toThrowErrorMatchingInlineSnapshot( + `"Field sets may not contain fragment spreads, but found: \\"foo ...Bar\\""`, + ); + }); + + it('disallows nested `FragmentSpread`s', () => { + const invalid = 'foo { ...Bar }'; + expect(() => parseFieldSet(invalid)).toThrowErrorMatchingInlineSnapshot( + `"Field sets may not contain fragment spreads, but found: \\"foo { ...Bar }\\""`, + ); + }); + }); +}); diff --git a/query-planner-js/src/utilities/assert.ts b/query-planner-js/src/utilities/assert.ts index e4c53793f..be1bbf0ca 100644 --- a/query-planner-js/src/utilities/assert.ts +++ b/query-planner-js/src/utilities/assert.ts @@ -1,3 +1,12 @@ +/** + * For lack of a "home of federation utilities", this function is copy/pasted + * verbatim across the federation, gateway, and query-planner packages. Any changes + * made here should be reflected in the other two locations as well. + * + * @param condition + * @param message + * @throws + */ export function assert(condition: any, message: string): asserts condition { if (!condition) { throw new Error(message); diff --git a/query-planner-js/src/utilities/graphql.ts b/query-planner-js/src/utilities/graphql.ts index a843ceb31..14cd44b2a 100644 --- a/query-planner-js/src/utilities/graphql.ts +++ b/query-planner-js/src/utilities/graphql.ts @@ -12,6 +12,7 @@ import { GraphQLSchema, GraphQLType, GraphQLUnionType, + InlineFragmentNode, isListType, isNonNullType, Kind, @@ -19,15 +20,15 @@ import { NamedTypeNode, OperationDefinitionNode, parse, - print, SchemaMetaFieldDef, SelectionNode, - SelectionSetNode, TypeMetaFieldDef, TypeNameMetaFieldDef, TypeNode, + visit, } from 'graphql'; import { getArgumentValues } from 'graphql/execution/values'; +import { FieldSet } from '../composedSchema'; import { assert } from './assert'; /** @@ -97,21 +98,50 @@ export function astFromType(type: GraphQLType): TypeNode { } } -export function printWithReducedWhitespace(ast: ASTNode): string { - return print(ast) - .replace(/\s+/g, ' ') - .trim(); +/** + * For lack of a "home of federation utilities", this function is copy/pasted + * verbatim across the federation, gateway, and query-planner packages. Any changes + * made here should be reflected in the other two locations as well. + * + * @param source A string representing a FieldSet + * @returns A parsed FieldSet + */ +export function parseSelections(source: string): ReadonlyArray { + const parsed = parse(`{${source}}`); + assert( + parsed.definitions.length === 1, + `Invalid FieldSet provided: '${source}'. FieldSets may not contain operations within them.`, + ); + return (parsed.definitions[0] as OperationDefinitionNode).selectionSet + .selections; } -export function parseSelectionSet(source: string): SelectionSetNode { - return (parse(`query ${source}`) - .definitions[0] as OperationDefinitionNode).selectionSet; +// TODO: should we be using this everywhere we're using `parseSelections`? +export function parseFieldSet(source: string): FieldSet { + const selections = parseSelections(source); + + const selectionSetNode = { + kind: Kind.SELECTION_SET, + selections, + }; + + visit(selectionSetNode, { + FragmentSpread() { + throw Error( + `Field sets may not contain fragment spreads, but found: "${source}"`, + ); + }, + }); + + // I'm not sure this case is possible - an empty string will first throw a + // graphql syntax error. Can you get 0 selections any other way? + assert(selections.length > 0, `Field sets may not be empty`); + + // This cast is asserted above by the visitor, ensuring that both `selections` + // and any recursive `selections` are not `FragmentSpreadNode`s + return selections as readonly (FieldNode | InlineFragmentNode)[]; } -export function parseSelections(source: string): ReadonlyArray { - return (parse(`query { ${source} }`) - .definitions[0] as OperationDefinitionNode).selectionSet.selections; -} // Using `getArgumentValues` from `graphql-js` ensures that arguments are of the right type, // and that required arguments are present.