From a593ac8c229180ba66d2921f8623e29eb8c25476 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Fri, 26 Aug 2022 10:23:13 +0200 Subject: [PATCH] Fix high memory usage when extracting subgraphs for some fed1 supergraphs (#2089) Fed1 supergraph lacks information on value types regarding which subgraphs defines them. We use to brute-force add all value types to all extract subgraphs, on the idea that if some extracted subgraphs have a few unused types, it's useless but has no functional impact. Unfortunately, in some special cases (lots of subgraphs and value types), those useless types can lead to a significant increase in memory consumptions. This patch instead look at type reachability within subgraphs to avoid including those useless value types, and thus lower the memory consumptions. Note that fed2 supergraphs are not affected by this problem has they have all the information needed to only extract types in the proper subgraphs. Also note that this patch also include a few small improvements to reduce memory consumption in general, and opportunistically remove the (essentially unused) dependency on the `core-schema-js` library. Fixes #2085 --- docs/source/errors.md | 1 + gateway-js/CHANGELOG.md | 2 + gateway-js/package.json | 1 - internals-js/package.json | 1 - internals-js/src/__tests__/coreSpec.test.ts | 2 +- .../extractSubgraphsFromSupergraph.test.ts | 14 +- .../removeInaccessibleElements.test.ts | 4 +- .../src/__tests__/schemaUpgrader.test.ts | 1 - .../src/__tests__/subgraphValidation.test.ts | 3 +- internals-js/src/buildSchema.ts | 3 +- internals-js/src/coreSpec.ts | 9 +- internals-js/src/definitions.ts | 303 ++++++++++-------- internals-js/src/error.ts | 62 ++++ .../src/extractSubgraphsFromSupergraph.ts | 185 ++++++++++- internals-js/src/federation.ts | 7 +- internals-js/src/print.ts | 4 +- internals-js/src/schemaUpgrader.ts | 9 +- internals-js/src/supergraphs.ts | 41 +-- internals-js/src/utils.ts | 15 + package-lock.json | 33 -- query-graphs-js/CHANGELOG.md | 1 + 21 files changed, 464 insertions(+), 237 deletions(-) diff --git a/docs/source/errors.md b/docs/source/errors.md index b87e0c200..82d91481e 100644 --- a/docs/source/errors.md +++ b/docs/source/errors.md @@ -91,6 +91,7 @@ The following errors might be raised during composition: | `UNKNOWN_FEDERATION_LINK_VERSION` | The version of federation in a @link directive on the schema is unknown. | 2.0.0 | | | `UNKNOWN_LINK_VERSION` | The version of @link set on the schema is unknown. | 2.1.0 | | | `UNSUPPORTED_FEATURE` | Indicates an error due to feature currently unsupported by federation. | 2.1.0 | | +| `UNSUPPORTED_LINKED_FEATURE` | Indicates that a feature used in a @link is either unsupported or is used with unsupported options. | 2.0.0 | | diff --git a/gateway-js/CHANGELOG.md b/gateway-js/CHANGELOG.md index 6bfc9f333..4bb850876 100644 --- a/gateway-js/CHANGELOG.md +++ b/gateway-js/CHANGELOG.md @@ -4,6 +4,8 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The ## vNext +- Fix abnormally high memory usage when extracting subgraphs for some fed1 supergraphs (and small other memory footprint improvements) [PR #2089](https://github.com/apollographql/federation/pull/2089). +- Fix issue with fragment reusing code something mistakenly re-expanding fragments [PR #2098](https://github.com/apollographql/federation/pull/2098). - Fix issue when type is only reachable through a @provides [PR #2083](https://github.com/apollographql/federation/pull/2083). - Fix case where some key field necessary to a `@require` fetch were not previously fetched [PR #2075](https://github.com/apollographql/federation/pull/2075). - Add type definitions to schema extensions [PR #2081](https://github.com/apollographql/federation/pull/2081) diff --git a/gateway-js/package.json b/gateway-js/package.json index 4746116bf..82306ed9d 100644 --- a/gateway-js/package.json +++ b/gateway-js/package.json @@ -26,7 +26,6 @@ }, "dependencies": { "@apollo/composition": "file:../composition-js", - "@apollo/core-schema": "~0.3.0", "@apollo/federation-internals": "file:../internals-js", "@apollo/query-planner": "file:../query-planner-js", "@apollo/server-gateway-interface": "^1.0.2", diff --git a/internals-js/package.json b/internals-js/package.json index b3ec3030e..bc7d1f11e 100644 --- a/internals-js/package.json +++ b/internals-js/package.json @@ -23,7 +23,6 @@ "node": ">=12.13.0" }, "dependencies": { - "@apollo/core-schema": "~0.3.0", "chalk": "^4.1.0", "js-levenshtein": "^1.1.6" }, diff --git a/internals-js/src/__tests__/coreSpec.test.ts b/internals-js/src/__tests__/coreSpec.test.ts index b5957c2a4..9107b0a78 100644 --- a/internals-js/src/__tests__/coreSpec.test.ts +++ b/internals-js/src/__tests__/coreSpec.test.ts @@ -1,10 +1,10 @@ import { DocumentNode, GraphQLError } from "graphql"; import gql from "graphql-tag"; import { buildSubgraph } from "../federation"; -import { errorCauses } from "../definitions"; import { assert } from "../utils"; import { buildSchemaFromAST } from "../buildSchema"; import { removeAllCoreFeatures } from "../coreSpec"; +import { errorCauses } from "../error"; function expectErrors( subgraphDefs: DocumentNode, diff --git a/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts b/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts index 9806ec428..eb4693a79 100644 --- a/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts +++ b/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts @@ -598,7 +598,6 @@ test('throw meaningful error for invalid federation directive fieldSet', () => { ); }) - test('throw meaningful error for type erased from supergraph due to extending an entity without a key', () => { // Supergraph generated by fed1 composition from: // ServiceA: @@ -616,13 +615,14 @@ test('throw meaningful error for type erased from supergraph due to extending an // x: Int! @external // } // - // type Other { + // type Other @key(fields: "id") { + // id: ID! // f: T @provides(fields: "x") // } // // The issue of that schema is that `T` is referenced in `ServiceB`, but because it extends an entity type - // without a key and has noly external field, ther is no remaining traces of it's definition in `ServiceB` - // in the supergraph. As extract cannot make up the original definition out of thin air, it ends up erroring + // without a key and has only external fields, there is no remaining traces of its definition in `ServiceB` + // in the supergraph. As extraction cannot make up the original definition out of thin air, it ends up erroring // when extracting `Other.t` due to not knowing that type. const supergraph = ` schema @@ -642,7 +642,11 @@ test('throw meaningful error for type erased from supergraph due to extending an directive @join__type(graph: join__Graph!, key: join__FieldSet) repeatable on INTERFACE | OBJECT - type Other { + type Other + @join__owner(graph: SERVICEB) + @join__type(graph: SERVICEB, key: "id") + { + id: ID! @join__field(graph: SERVICEB) f: T @join__field(graph: SERVICEB, provides: "x") } diff --git a/internals-js/src/__tests__/removeInaccessibleElements.test.ts b/internals-js/src/__tests__/removeInaccessibleElements.test.ts index a5ff8255a..313e168c9 100644 --- a/internals-js/src/__tests__/removeInaccessibleElements.test.ts +++ b/internals-js/src/__tests__/removeInaccessibleElements.test.ts @@ -1,6 +1,5 @@ import { ArgumentDefinition, - errorCauses, FieldDefinition, InterfaceType, ObjectType, @@ -8,8 +7,8 @@ import { } from "../definitions"; import { buildSchema } from "../buildSchema"; import { removeInaccessibleElements } from "../inaccessibleSpec"; -import { GraphQLErrorExt } from "@apollo/core-schema/dist/error"; import { GraphQLError } from "graphql"; +import { errorCauses } from "../error"; describe("removeInaccessibleElements", () => { const INACCESSIBLE_V02_HEADER = ` @@ -31,7 +30,6 @@ describe("removeInaccessibleElements", () => { `; function getCauses(e: unknown): GraphQLError[] { - expect(e instanceof GraphQLErrorExt).toBeTruthy(); const causes = errorCauses(e as Error); expect(causes).toBeDefined(); expect(Array.isArray(causes)).toBeTruthy(); diff --git a/internals-js/src/__tests__/schemaUpgrader.test.ts b/internals-js/src/__tests__/schemaUpgrader.test.ts index 4cc293a01..7547eb054 100644 --- a/internals-js/src/__tests__/schemaUpgrader.test.ts +++ b/internals-js/src/__tests__/schemaUpgrader.test.ts @@ -167,7 +167,6 @@ test('update federation directive non-string arguments', () => { `); }) - test('remove tag on external field if found on definition', () => { const s1 = ` type Query { diff --git a/internals-js/src/__tests__/subgraphValidation.test.ts b/internals-js/src/__tests__/subgraphValidation.test.ts index 784ee1515..577823290 100644 --- a/internals-js/src/__tests__/subgraphValidation.test.ts +++ b/internals-js/src/__tests__/subgraphValidation.test.ts @@ -1,7 +1,6 @@ import { DocumentNode } from 'graphql'; import gql from 'graphql-tag'; -import { Subgraph } from '..'; -import { errorCauses } from '../definitions'; +import { Subgraph, errorCauses } from '..'; import { asFed2SubgraphDocument, buildSubgraph } from "../federation" import { defaultPrintOptions, printSchema } from '../print'; import './matchers'; diff --git a/internals-js/src/buildSchema.ts b/internals-js/src/buildSchema.ts index b20f37977..e0cfd793d 100644 --- a/internals-js/src/buildSchema.ts +++ b/internals-js/src/buildSchema.ts @@ -52,10 +52,9 @@ import { EnumType, Extension, ErrGraphQLValidationFailed, - errorCauses, NamedSchemaElement, } from "./definitions"; -import { ERRORS, withModifiedErrorNodes } from "./error"; +import { ERRORS, errorCauses, withModifiedErrorNodes } from "./error"; function buildValue(value?: ValueNode): any { return value ? valueFromASTUntyped(value) : undefined; diff --git a/internals-js/src/coreSpec.ts b/internals-js/src/coreSpec.ts index 917a37948..e5d58c042 100644 --- a/internals-js/src/coreSpec.ts +++ b/internals-js/src/coreSpec.ts @@ -2,9 +2,8 @@ import { ASTNode, DirectiveLocation, GraphQLError, StringValueNode } from "graph import { URL } from "url"; import { CoreFeature, Directive, DirectiveDefinition, EnumType, ErrGraphQLAPISchemaValidationFailed, ErrGraphQLValidationFailed, InputType, ListType, NamedType, NonNullType, ScalarType, Schema, SchemaDefinition, SchemaElement, sourceASTs } from "./definitions"; import { sameType } from "./types"; -import { err } from '@apollo/core-schema'; import { assert, firstOf } from './utils'; -import { ERRORS } from "./error"; +import { aggregateError, ERRORS } from "./error"; import { valueToString } from "./values"; import { coreFeatureDefinitionIfKnown, registerKnownFeature } from "./knownCoreFeatures"; import { didYouMean, suggestionList } from "./suggestions"; @@ -15,11 +14,7 @@ export const linkIdentity = 'https://specs.apollo.dev/link'; export const linkDirectiveDefaultName = 'link'; -export const ErrCoreCheckFailed = (causes: Error[]) => - err('CheckFailed', { - message: 'one or more checks failed', - causes - }) +export const ErrCoreCheckFailed = (causes: GraphQLError[]) => aggregateError('CheckFailed', 'one or more checks failed', causes); function buildError(message: string): Error { // Maybe not the right error for this? diff --git a/internals-js/src/definitions.ts b/internals-js/src/definitions.ts index a042331dd..c22f15b35 100644 --- a/internals-js/src/definitions.ts +++ b/internals-js/src/definitions.ts @@ -31,67 +31,30 @@ import { isCoreSpecDirectiveApplication, removeAllCoreFeatures, } from "./coreSpec"; -import { assert, mapValues, MapWithCachedArrays, setValues } from "./utils"; +import { assert, mapValues, MapWithCachedArrays, removeArrayElement } from "./utils"; import { withDefaultValues, valueEquals, valueToString, valueToAST, variablesInValue, valueFromAST, valueNodeToConstValueNode, argumentsEquals } from "./values"; import { removeInaccessibleElements } from "./inaccessibleSpec"; import { printDirectiveDefinition, printSchema } from './print'; import { sameType } from './types'; import { addIntrospectionFields, introspectionFieldNames, isIntrospectionName } from "./introspection"; -import { err } from '@apollo/core-schema'; -import { GraphQLErrorExt } from "@apollo/core-schema/dist/error"; import { validateSDL } from "graphql/validation/validate"; import { SDLValidationRule } from "graphql/validation/ValidationContext"; import { specifiedSDLRules } from "graphql/validation/specifiedRules"; import { validateSchema } from "./validate"; import { createDirectiveSpecification, createScalarTypeSpecification, DirectiveSpecification, TypeSpecification } from "./directiveAndTypeSpecification"; import { didYouMean, suggestionList } from "./suggestions"; -import { ERRORS, withModifiedErrorMessage } from "./error"; +import { aggregateError, ERRORS, withModifiedErrorMessage } from "./error"; const validationErrorCode = 'GraphQLValidationFailed'; const DEFAULT_VALIDATION_ERROR_MESSAGE = 'The schema is not a valid GraphQL schema.'; export const ErrGraphQLValidationFailed = (causes: GraphQLError[], message: string = DEFAULT_VALIDATION_ERROR_MESSAGE) => - err(validationErrorCode, { - message: message + '. Caused by:\n' + causes.map((c) => c.toString()).join('\n\n'), - causes - }); + aggregateError(validationErrorCode, message, causes); const apiSchemaValidationErrorCode = 'GraphQLAPISchemaValidationFailed'; export const ErrGraphQLAPISchemaValidationFailed = (causes: GraphQLError[]) => - err(apiSchemaValidationErrorCode, { - message: 'The supergraph schema failed to produce a valid API schema', - causes - }); - -/** - * Given an error that may have been thrown during schema validation, extract the causes of validation failure. - * If the error is not a graphQL error, undefined is returned. - */ -export function errorCauses(e: Error): GraphQLError[] | undefined { - if (e instanceof GraphQLErrorExt) { - if (e.code === validationErrorCode || e.code === apiSchemaValidationErrorCode) { - return ((e as any).causes) as GraphQLError[]; - } - return [e]; - } - if (e instanceof GraphQLError) { - return [e]; - } - return undefined; -} - -export function printGraphQLErrorsOrRethrow(e: Error): string { - const causes = errorCauses(e); - if (!causes) { - throw e; - } - return causes.map(e => e.toString()).join('\n\n'); -} - -export function printErrors(errors: GraphQLError[]): string { - return errors.map(e => e.toString()).join('\n\n'); -} + aggregateError(apiSchemaValidationErrorCode, 'The supergraph schema failed to produce a valid API schema', causes); export const typenameFieldName = '__typename'; @@ -372,7 +335,7 @@ export interface Named { export type ExtendableElement = SchemaDefinition | NamedType; export class DirectiveTargetElement> { - public readonly appliedDirectives: Directive[] = []; + private _appliedDirectives: Directive[] | undefined; constructor(private readonly _schema: Schema) {} @@ -387,6 +350,10 @@ export class DirectiveTargetElement> { return this.appliedDirectives.filter(d => d.name == directiveName); } + get appliedDirectives(): readonly Directive[] { + return this._appliedDirectives ?? []; + } + hasAppliedDirective(nameOrDefinition: string | DirectiveDefinition): boolean { const directiveName = typeof nameOrDefinition === 'string' ? nameOrDefinition : nameOrDefinition.name; return this.appliedDirectives.some(d => d.name == directiveName); @@ -410,7 +377,11 @@ export class DirectiveTargetElement> { } Element.prototype['setParent'].call(toAdd, this); // TODO: we should typecheck arguments or our TApplicationArgs business is just a lie. - this.appliedDirectives.push(toAdd); + if (this._appliedDirectives) { + this._appliedDirectives.push(toAdd); + } else { + this._appliedDirectives = [ toAdd ]; + } return toAdd; } @@ -532,35 +503,40 @@ type UnappliedDirective = { // TODO: ideally, we should hide the ctor of this class as we rely in places on the fact the no-one external defines new implementations. export abstract class SchemaElement, TParent extends SchemaElement | Schema> extends Element { - protected readonly _appliedDirectives: Directive[] = []; - protected _unappliedDirectives: UnappliedDirective[] = []; + protected _appliedDirectives: Directive[] | undefined; + protected _unappliedDirectives: UnappliedDirective[] | undefined; description?: string; addUnappliedDirective({ nameOrDef, args, extension, directive }: UnappliedDirective) { - this._unappliedDirectives.push({ + const toAdd = { nameOrDef, args: args ?? {}, extension, directive, - }); + }; + if (this._unappliedDirectives) { + this._unappliedDirectives.push(toAdd); + } else { + this._unappliedDirectives = [toAdd]; + } } processUnappliedDirectives() { - for (const { nameOrDef, args, extension, directive } of this._unappliedDirectives) { + for (const { nameOrDef, args, extension, directive } of this._unappliedDirectives ?? []) { const d = this.applyDirective(nameOrDef, args); d.setOfExtension(extension); d.sourceAST = directive; } - this._unappliedDirectives = []; + this._unappliedDirectives = undefined; } get appliedDirectives(): readonly Directive[] { - return this._appliedDirectives; + return this._appliedDirectives ?? []; } appliedDirectivesOf(nameOrDefinition: string | DirectiveDefinition): Directive[] { const directiveName = typeof nameOrDefinition === 'string' ? nameOrDefinition : nameOrDefinition.name; - return this._appliedDirectives.filter(d => d.name == directiveName) as Directive[]; + return this.appliedDirectives.filter(d => d.name == directiveName) as Directive[]; } hasAppliedDirective(nameOrDefinition: string | DirectiveDefinition): boolean { @@ -600,10 +576,14 @@ export abstract class SchemaElement const toAdd = new Directive(name, args ?? Object.create(null)); Element.prototype['setParent'].call(toAdd, this); // TODO: we should typecheck arguments or our TApplicationArgs business is just a lie. - if (asFirstDirective) { - this._appliedDirectives.unshift(toAdd); + if (this._appliedDirectives) { + if (asFirstDirective) { + this._appliedDirectives.unshift(toAdd); + } else { + this._appliedDirectives.push(toAdd); + } } else { - this._appliedDirectives.push(toAdd); + this._appliedDirectives = [toAdd]; } DirectiveDefinition.prototype['addReferencer'].call(toAdd.definition!, toAdd); this.onModification(); @@ -612,6 +592,9 @@ export abstract class SchemaElement protected removeAppliedDirectives() { // We copy the array because this._appliedDirectives is modified in-place by `directive.remove()` + if (!this._appliedDirectives) { + return; + } const applied = this._appliedDirectives.concat(); applied.forEach(d => d.remove()); } @@ -685,8 +668,8 @@ export abstract class NamedSchemaElement> extends NamedSchemaElement { - protected readonly _referencers: Set = new Set(); - protected readonly _extensions: Set> = new Set(); + protected _referencers?: TReferencer[]; + protected _extensions?: Extension[]; public preserveEmptyDefinition: boolean = false; constructor(name: string, readonly isBuiltIn: boolean = false) { @@ -694,11 +677,19 @@ abstract class BaseNamedType> { - return this._extensions; + extensions(): readonly Extension[] { + return this._extensions ?? []; + } + + hasExtension(extension: Extension): boolean { + return this._extensions?.includes(extension) ?? false; } newExtension(): Extension { @@ -720,23 +715,27 @@ abstract class BaseNamedType): Extension { this.checkUpdate(); // Let's be nice and not complaint if we add an extension already added. - if (this._extensions.has(extension)) { + if (this.hasExtension(extension)) { return extension; } assert(!extension.extendedElement, () => `Cannot add extension to type ${this}: it is already added to another type`); - this._extensions.add(extension); + if (this._extensions) { + this._extensions.push(extension); + } else { + this._extensions = [ extension ]; + } Extension.prototype['setExtendedElement'].call(extension, this); this.onModification(); return extension; } removeExtensions() { - if (this._extensions.size === 0) { + if (!this._extensions) { return; } - this._extensions.clear(); - for (const directive of this._appliedDirectives) { + this._extensions = undefined; + for (const directive of this.appliedDirectives) { directive.removeOfExtension(); } this.removeInnerElementsExtensions(); @@ -747,12 +746,12 @@ abstract class BaseNamedType 0; + return !!this._extensions; } hasNonExtensionElements(): boolean { return this.preserveEmptyDefinition - || this._appliedDirectives.some(d => d.ofExtension() === undefined) + || this.appliedDirectives.some(d => d.ofExtension() === undefined) || this.hasNonExtensionInnerElements(); } @@ -798,11 +797,11 @@ abstract class BaseNamedType { + const toReturn = this._referencers?.map(r => { SchemaElement.prototype['removeTypeReferenceInternal'].call(r, this); return r; - }); - this._referencers.clear(); + }) ?? []; + this._referencers = undefined; // Remove this type from its parent schema. Schema.prototype['removeTypeInternal'].call(this._parent, this); this._parent = undefined; @@ -830,11 +829,11 @@ abstract class BaseNamedType 0; + return !!this._referencers; } protected abstract removeInnerElements(): void; @@ -887,8 +886,7 @@ abstract class BaseExtensionMember extends setOfExtension(extension: Extension | undefined) { this.checkUpdate(); - // See similar comment on FieldDefinition.setOfExtension for why we have to cast. - assert(!extension || this._parent?.extensions().has(extension as any), () => `Cannot set object as part of the provided extension: it is not an extension of parent ${this.parent}`); + assert(!extension || this._parent?.hasExtension(extension), () => `Cannot set object as part of the provided extension: it is not an extension of parent ${this.parent}`); this._extension = extension; } @@ -1178,6 +1176,10 @@ export type StreamDirectiveArgs = { // A coordinate is up to 3 "graphQL name" ([_A-Za-z][_0-9A-Za-z]*). const coordinateRegexp = /^@?[_A-Za-z][_0-9A-Za-z]*(\.[_A-Za-z][_0-9A-Za-z]*)?(\([_A-Za-z][_0-9A-Za-z]*:\))?$/; +export type SchemaConfig = { + cacheAST?: boolean, +} + export class Schema { private _schemaDefinition: SchemaDefinition; private readonly _builtInTypes = new MapWithCachedArrays(); @@ -1191,7 +1193,10 @@ export class Schema { private cachedDocument?: DocumentNode; private apiSchema?: Schema; - constructor(readonly blueprint: SchemaBlueprint = defaultSchemaBlueprint) { + constructor( + readonly blueprint: SchemaBlueprint = defaultSchemaBlueprint, + readonly config: SchemaConfig = {}, + ) { this._schemaDefinition = new SchemaDefinition(); Element.prototype['setParent'].call(this._schemaDefinition, this); graphQLBuiltInTypesSpecifications.forEach((spec) => spec.checkOrAdd(this, undefined, true)); @@ -1241,10 +1246,6 @@ export class Schema { } } - private forceSetCachedDocument(document: DocumentNode) { - this.cachedDocument = document; - } - isCoreSchema(): boolean { return this.coreFeatures !== undefined; } @@ -1256,7 +1257,12 @@ export class Schema { toAST(): DocumentNode { if (!this.cachedDocument) { // As we're not building the document from a file, having locations info might be more confusing that not. - this.forceSetCachedDocument(parse(printSchema(this), { noLocation: true })); + const ast = parse(printSchema(this), { noLocation: true }); + const shouldCache = this.config.cacheAST ?? false; + if (!shouldCache) { + return ast; + } + this.cachedDocument = ast; } return this.cachedDocument!; } @@ -1537,8 +1543,10 @@ export class Schema { } invalidate() { + if (this.isValidated) { + this.blueprint.onInvalidation(this); + } this.isValidated = false; - this.blueprint.onInvalidation(this); } validate() { @@ -1682,7 +1690,7 @@ export class RootType extends BaseExtensionMember { export class SchemaDefinition extends SchemaElement { readonly kind = 'SchemaDefinition' as const; protected readonly _roots = new MapWithCachedArrays(); - protected readonly _extensions = new Set>(); + protected _extensions: Extension[] | undefined; public preserveEmptyDefinition: boolean = false; roots(): readonly RootType[] { @@ -1752,8 +1760,12 @@ export class SchemaDefinition extends SchemaElement { return toSet; } - extensions(): ReadonlySet> { - return this._extensions; + extensions(): Extension[] { + return this._extensions ?? []; + } + + hasExtension(extension: Extension): boolean { + return this._extensions?.includes(extension) ?? false; } newExtension(): Extension { @@ -1763,23 +1775,27 @@ export class SchemaDefinition extends SchemaElement { addExtension(extension: Extension): Extension { this.checkUpdate(); // Let's be nice and not complaint if we add an extension already added. - if (this._extensions.has(extension)) { + if (this.hasExtension(extension)) { return extension; } assert(!extension.extendedElement, 'Cannot add extension to this schema: extension is already added to another schema'); - this._extensions.add(extension); + if (this._extensions) { + this._extensions.push(extension); + } else { + this._extensions = [extension]; + } Extension.prototype['setExtendedElement'].call(extension, this); this.onModification(); return extension; } hasExtensionElements(): boolean { - return this._extensions.size > 0; + return !!this._extensions; } hasNonExtensionElements(): boolean { return this.preserveEmptyDefinition - || this._appliedDirectives.some((d) => d.ofExtension() === undefined) + || this.appliedDirectives.some((d) => d.ofExtension() === undefined) || this.roots().some((r) => r.ofExtension() === undefined); } @@ -1853,7 +1869,7 @@ abstract class FieldBasedType> = new MapWithCachedArrays(); + private _interfaceImplementations: MapWithCachedArrays> | undefined; private readonly _fields: MapWithCachedArrays> = new MapWithCachedArrays(); private _cachedNonBuiltInFields?: readonly FieldDefinition[]; @@ -1872,11 +1888,11 @@ abstract class FieldBasedType[] { - return this._interfaceImplementations.values(); + return this._interfaceImplementations?.values() ?? []; } interfaceImplementation(type: string | InterfaceType): InterfaceImplementation | undefined { - return this._interfaceImplementations.get(typeof type === 'string' ? type : type.name); + return this._interfaceImplementations ? this._interfaceImplementations.get(typeof type === 'string' ? type : type.name) : undefined; } interfaces(): readonly InterfaceType[] { @@ -1884,7 +1900,7 @@ abstract class FieldBasedType | InterfaceType | string): InterfaceImplementation { @@ -1908,8 +1924,11 @@ abstract class FieldBasedType(itf); } - const existing = this._interfaceImplementations.get(toAdd.interface.name); + const existing = this._interfaceImplementations?.get(toAdd.interface.name); if (!existing) { + if (!this._interfaceImplementations) { + this._interfaceImplementations = new MapWithCachedArrays(); + } this._interfaceImplementations.set(toAdd.interface.name, toAdd); addReferenceToType(this, toAdd.interface); Element.prototype['setParent'].call(toAdd, this); @@ -1996,12 +2015,12 @@ abstract class FieldBasedType ref.kind === 'ObjectType' || ref.kind === 'InterfaceType') as (ObjectType | InterfaceType)[]; + return this.referencers().filter(ref => ref.kind === 'ObjectType' || ref.kind === 'InterfaceType') as (ObjectType | InterfaceType)[]; } possibleRuntimeTypes(): readonly ObjectType[] { @@ -2271,15 +2290,12 @@ export class EnumType extends BaseNamedType { } private removeValueInternal(value: EnumValue) { - const index = this._values.indexOf(value); - if (index >= 0) { - this._values.splice(index, 1); - } + removeArrayElement(value, this._values); } protected removeInnerElements(): void { - // Make a copy, since EnumValue.remove() will modify this._values. - const values = Array.from(this._values); + // Make a copy (indirectly), since EnumValue.remove() will modify this._values. + const values = this.values; for (const value of values) { value.remove(); } @@ -2434,7 +2450,7 @@ export class NonNullType extends BaseWrapperType { export class FieldDefinition extends NamedSchemaElementWithType, TParent, never> { readonly kind = 'FieldDefinition' as const; - private readonly _args: MapWithCachedArrays>> = new MapWithCachedArrays(); + private _args: MapWithCachedArrays>> | undefined; private _extension?: Extension; constructor(name: string, readonly isBuiltIn: boolean = false) { @@ -2451,15 +2467,15 @@ export class FieldDefinition extends NamedSchemaE } hasArguments(): boolean { - return this._args.size > 0; + return !!this._args && this._args.size > 0; } arguments(): readonly ArgumentDefinition>[] { - return this._args.values(); + return this._args?.values() ?? []; } argument(name: string): ArgumentDefinition> | undefined { - return this._args.get(name); + return this._args?.get(name); } addArgument(arg: ArgumentDefinition>): ArgumentDefinition>; @@ -2489,6 +2505,9 @@ export class FieldDefinition extends NamedSchemaE if (type && !isInputType(type)) { throw ERRORS.INVALID_GRAPHQL.err(`Invalid output type ${type} for argument ${toAdd.name} of ${this}: arguments should be input types.`); } + if (!this._args) { + this._args = new MapWithCachedArrays(); + } this._args.set(toAdd.name, toAdd); Element.prototype['setParent'].call(toAdd, this); if (typeof nameOrArg === 'string') { @@ -2508,10 +2527,8 @@ export class FieldDefinition extends NamedSchemaE setOfExtension(extension: Extension | undefined) { this.checkUpdate(); - // It seems typescript "expand" `TParent` below into `ObjectType | Interface`, so it essentially lose the context that - // the `TParent` in `Extension` will always match. Hence the `as any`. assert( - !extension || this._parent?.extensions().has(extension as any), + !extension || this._parent?.hasExtension(extension), () => `Cannot mark field ${this.name} as part of the provided extension: it is not an extension of field parent type ${this.parent}` ); this._extension = extension; @@ -2527,7 +2544,9 @@ export class FieldDefinition extends NamedSchemaE } private removeArgumentInternal(name: string) { - this._args.delete(name); + if (this._args) { + this._args.delete(name); + } } // Only called through the prototype from FieldBasedType.removeInnerElements because we don't want to expose it. @@ -2589,9 +2608,9 @@ export class FieldDefinition extends NamedSchemaE } toString(): string { - const args = this._args.size == 0 - ? "" - : '(' + this.arguments().map(arg => arg.toString()).join(', ') + ')'; + const args = this.hasArguments() + ? '(' + this.arguments().map(arg => arg.toString()).join(', ') + ')' + : ""; return `${this.name}${args}: ${this.type}`; } } @@ -2620,10 +2639,8 @@ export class InputFieldDefinition extends NamedSchemaElementWithType | undefined) { this.checkUpdate(); - // It seems typescript "expand" `TParent` below into `ObjectType | Interface`, so it essentially lose the context that - // the `TParent` in `Extension` will always match. Hence the `as any`. assert( - !extension || this._parent?.extensions().has(extension as any), + !extension || this._parent?.hasExtension(extension), () => `Cannot mark field ${this.name} as part of the provided extension: it is not an extension of field parent type ${this.parent}`, ); this._extension = extension; @@ -2773,7 +2790,7 @@ export class EnumValue extends NamedSchemaElement { setOfExtension(extension: Extension | undefined) { this.checkUpdate(); assert( - !extension || this._parent?.extensions().has(extension as any), + !extension || this._parent?.hasExtension(extension), () => `Cannot mark field ${this.name} as part of the provided extension: it is not an extension of enum value parent type ${this.parent}`, ); this._extension = extension; @@ -2830,10 +2847,10 @@ export class EnumValue extends NamedSchemaElement { export class DirectiveDefinition extends NamedSchemaElement, Schema, Directive> { readonly kind = 'DirectiveDefinition' as const; - private readonly _args: MapWithCachedArrays> = new MapWithCachedArrays(); + private _args?: MapWithCachedArrays>; repeatable: boolean = false; private readonly _locations: DirectiveLocation[] = []; - private readonly _referencers: Set, TApplicationArgs>> = new Set(); + private _referencers?: Directive, TApplicationArgs>[]; constructor(name: string, readonly isBuiltIn: boolean = false) { super(name); @@ -2844,11 +2861,11 @@ export class DirectiveDefinition[] { - return this._args.values(); + return this._args?.values() ?? []; } argument(name: string): ArgumentDefinition | undefined { - return this._args.get(name); + return this._args?.get(name); } addArgument(arg: ArgumentDefinition): ArgumentDefinition; @@ -2866,6 +2883,9 @@ export class DirectiveDefinition= 0) { - this._locations.splice(index, 1); - modified = true; - } + modified ||= removeArrayElement(location, this._locations); } if (modified) { this.onModification(); @@ -2939,16 +2955,24 @@ export class DirectiveDefinition, TApplicationArgs>[] { - return setValues(this._referencers); + return this._referencers ?? []; } private addReferencer(referencer: Directive, TApplicationArgs>) { assert(referencer, 'Referencer should exists'); - this._referencers.add(referencer); + if (this._referencers) { + if (!this._referencers.includes(referencer)) { + this._referencers.push(referencer); + } + } else { + this._referencers = [ referencer ]; + } } private removeReferencer(referencer: Directive, TApplicationArgs>) { - this._referencers.delete(referencer); + if (this._referencers) { + removeArrayElement(referencer, this._referencers); + } } protected removeTypeReference(type: NamedType) { @@ -2969,7 +2993,7 @@ export class DirectiveDefinition `Cannot mark directive ${this.name} as part of the provided extension: it is not an extension of parent ${parent}`); + assert(parent.hasExtension(extension), () => `Cannot mark directive ${this.name} as part of the provided extension: it is not an extension of parent ${parent}`); } this._extension = extension; this.onModification(); @@ -3157,9 +3181,8 @@ export class Directive< } // Remove this directive application from its parent schema element. const parentDirectives = this._parent.appliedDirectives as Directive[]; - const index = parentDirectives.indexOf(this); - assert(index >= 0, () => `Directive ${this} lists ${this._parent} as parent, but that parent doesn't list it as applied directive`); - parentDirectives.splice(index, 1); + const removed = removeArrayElement(this, parentDirectives); + assert(removed, () => `Directive ${this} lists ${this._parent} as parent, but that parent doesn't list it as applied directive`); this._parent = undefined; this._extension = undefined; return true; @@ -3179,7 +3202,7 @@ export function sameDirectiveApplication(application1: Directive, appl /** * Checks whether the 2 provided "set" of directive applications are the same (same applications, regardless or order). */ -export function sameDirectiveApplications(applications1: Directive[], applications2: Directive[]): boolean { +export function sameDirectiveApplications(applications1: readonly Directive[], applications2: readonly Directive[]): boolean { if (applications1.length !== applications2.length) { return false; } @@ -3197,7 +3220,7 @@ export function sameDirectiveApplications(applications1: Directive[], * * Sub-set here means that all of the applications in `maybeSubset` appears in `applications`. */ -export function isDirectiveApplicationsSubset(applications: Directive[], maybeSubset: Directive[]): boolean { +export function isDirectiveApplicationsSubset(applications: readonly Directive[], maybeSubset: readonly Directive[]): boolean { if (maybeSubset.length > applications.length) { return false; } @@ -3213,7 +3236,7 @@ export function isDirectiveApplicationsSubset(applications: Directive[ /** * Computes the difference between the set of directives applications `baseApplications` and the `toRemove` one. */ -export function directiveApplicationsSubstraction(baseApplications: Directive[], toRemove: Directive[]): Directive[] { +export function directiveApplicationsSubstraction(baseApplications: readonly Directive[], toRemove: readonly Directive[]): Directive[] { return baseApplications.filter((application) => !toRemove.some((other) => sameDirectiveApplication(application, other))); } diff --git a/internals-js/src/error.ts b/internals-js/src/error.ts index 36800e5d3..21d66c065 100644 --- a/internals-js/src/error.ts +++ b/internals-js/src/error.ts @@ -56,6 +56,62 @@ export function extractGraphQLErrorOptions(e: GraphQLError): GraphQLErrorOptions }; } +class AggregateGraphQLError extends GraphQLError { + constructor( + code: String, + message: string, + readonly causes: GraphQLError[], + options?: GraphQLErrorOptions, + ) { + super( + message + '. Caused by:\n' + causes.map((c) => c.toString()).join('\n\n'), + { + ...options, + extensions: { code }, + } + ); + } + + toString() { + let output = `[${this.extensions.code}] ${super.toString()}` + output += "\ncaused by:"; + for (const cause of this.causes) { + output += "\n\n - "; + output += cause.toString().split("\n").join("\n "); + } + return output; + } +} + +export function aggregateError(code: String, message: string, causes: GraphQLError[]): GraphQLError { + return new AggregateGraphQLError(code, message, causes); +} + +/** + * Given an error, check if it is a graphQL error and potentially extract its causes if is aggregate. + * If the error is not a graphQL error, undefined is returned. + */ +export function errorCauses(e: Error): GraphQLError[] | undefined { + if (e instanceof AggregateGraphQLError) { + return e.causes; + } + if (e instanceof GraphQLError) { + return [e]; + } + return undefined; +} + +export function printGraphQLErrorsOrRethrow(e: Error): string { + const causes = errorCauses(e); + if (!causes) { + throw e; + } + return causes.map(e => e.toString()).join('\n\n'); +} + +export function printErrors(errors: GraphQLError[]): string { + return errors.map(e => e.toString()).join('\n\n'); +} /* * Most codes currently originate from the initial fed 2 release so we use this for convenience. * This can be changed later, inline versions everywhere, if that becomes irrelevant. @@ -144,6 +200,11 @@ const TYPE_DEFINITION_INVALID = makeCodeDefinition( 'A built-in or federation type has an invalid definition in the schema.', ); +const UNSUPPORTED_LINKED_FEATURE = makeCodeDefinition( + 'UNSUPPORTED_LINKED_FEATURE', + 'Indicates that a feature used in a @link is either unsupported or is used with unsupported options.', +); + const UNKNOWN_FEDERATION_LINK_VERSION = makeCodeDefinition( 'UNKNOWN_FEDERATION_LINK_VERSION', 'The version of federation in a @link directive on the schema is unknown.', @@ -479,6 +540,7 @@ export const ERRORS = { INVALID_GRAPHQL, DIRECTIVE_DEFINITION_INVALID, TYPE_DEFINITION_INVALID, + UNSUPPORTED_LINKED_FEATURE, UNKNOWN_FEDERATION_LINK_VERSION, UNKNOWN_LINK_VERSION, KEY_FIELDS_HAS_ARGS, diff --git a/internals-js/src/extractSubgraphsFromSupergraph.ts b/internals-js/src/extractSubgraphsFromSupergraph.ts index d3fbd57c0..83614cfb6 100644 --- a/internals-js/src/extractSubgraphsFromSupergraph.ts +++ b/internals-js/src/extractSubgraphsFromSupergraph.ts @@ -34,6 +34,7 @@ import { validateSupergraph } from "./supergraphs"; import { builtTypeReference } from "./buildSchema"; import { isSubtype } from "./types"; import { printSchema } from "./print"; +import { parseSelectionSet } from "./operations"; import fs from 'fs'; import path from 'path'; import { validateStringContainsBoolean } from "./utils"; @@ -82,6 +83,114 @@ class SubgraphExtractionError { } } +function collectFieldReachableTypesForSubgraph( + supergraph: Schema, + subgraphName: string, + addReachableType: (t: NamedType) => void, + fieldInfoInSubgraph: (f: FieldDefinition | InputFieldDefinition, subgraphName: string) => { isInSubgraph: boolean, typesInFederationDirectives: NamedType[] }, + typeInfoInSubgraph: (t: NamedType, subgraphName: string) => { isEntityWithKeyInSubgraph: boolean, typesInFederationDirectives: NamedType[] }, +): void { + const seenTypes = new Set(); + // The types reachable at "top-level" are both the root types, plus any entity type with a key in this subgraph. + const stack = supergraph.schemaDefinition.roots().map((root) => root.type as NamedType) + for (const type of supergraph.types()) { + const { isEntityWithKeyInSubgraph, typesInFederationDirectives } = typeInfoInSubgraph(type, subgraphName); + if (isEntityWithKeyInSubgraph) { + stack.push(type); + } + typesInFederationDirectives.forEach((t) => stack.push(t)); + } + while (stack.length > 0) { + const type = stack.pop()!; + addReachableType(type); + if (seenTypes.has(type.name)) { + continue; + } + seenTypes.add(type.name); + switch (type.kind) { + // @ts-expect-error: we fall-through to ObjectType for fields and implemented interfaces. + case 'InterfaceType': + // If an interface if reachable, then all of its implementation are too (a field returning the interface could return any of the + // implementation at runtime typically). + type.allImplementations().forEach((t) => stack.push(t)); + case 'ObjectType': + type.interfaces().forEach((t) => stack.push(t)); + for (const field of type.fields()) { + const { isInSubgraph, typesInFederationDirectives } = fieldInfoInSubgraph(field, subgraphName); + if (isInSubgraph) { + field.arguments().forEach((arg) => stack.push(baseType(arg.type!))); + stack.push(baseType(field.type!)); + typesInFederationDirectives.forEach((t) => stack.push(t)); + } + } + break; + case 'InputObjectType': + for (const field of type.fields()) { + const { isInSubgraph, typesInFederationDirectives } = fieldInfoInSubgraph(field, subgraphName); + if (isInSubgraph) { + stack.push(baseType(field.type!)); + typesInFederationDirectives.forEach((t) => stack.push(t)); + } + } + break; + case 'UnionType': + type.members().forEach((m) => stack.push(m.type)); + break; + } + } + + for (const directive of supergraph.directives()) { + // In fed1 supergraphs, which is the only place this is called, only executable directive from subgraph only ever made + // it to the supergraph. Skipping anything else saves us from worrying about supergraph-specific directives too. + if (!directive.hasExecutableLocations()) { + continue; + } + directive.arguments().forEach((arg) => stack.push(baseType(arg.type!))); + } +} + +function collectFieldReachableTypesForAllSubgraphs( + supergraph: Schema, + allSubgraphs: readonly string[], + fieldInfoInSubgraph: (f: FieldDefinition | InputFieldDefinition, subgraphName: string) => { isInSubgraph: boolean, typesInFederationDirectives: NamedType[] }, + typeInfoInSubgraph: (t: NamedType, subgraphName: string) => { isEntityWithKeyInSubgraph: boolean, typesInFederationDirectives: NamedType[] }, +): Map> { + const reachableTypesBySubgraphs = new Map>(); + for (const subgraphName of allSubgraphs) { + const reachableTypes = new Set(); + collectFieldReachableTypesForSubgraph( + supergraph, + subgraphName, + (t) => reachableTypes.add(t.name), + fieldInfoInSubgraph, + typeInfoInSubgraph, + ); + reachableTypesBySubgraphs.set(subgraphName, reachableTypes); + } + return reachableTypesBySubgraphs; +} + +function typesUsedInFederationDirective(fieldSet: string | undefined, parentType: CompositeType): NamedType[] { + if (!fieldSet) { + return []; + } + + const usedTypes: NamedType[] = []; + parseSelectionSet({ + parentType, + source: fieldSet, + fieldAccessor: (type, fieldName) => { + const field = type.field(fieldName); + if (field) { + usedTypes.push(baseType(field.type!)); + } + return field; + }, + validate: false, + }); + return usedTypes; +} + export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs { const [coreFeatures, joinSpec] = validateSupergraph(supergraph); const isFed1 = joinSpec.version.equals(new FeatureVersion(0, 1)); @@ -90,6 +199,61 @@ export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs { const [subgraphs, graphEnumNameToSubgraphName] = collectEmptySubgraphs(supergraph, joinSpec); const typeDirective = joinSpec.typeDirective(supergraph); const implementsDirective = joinSpec.implementsDirective(supergraph); + const ownerDirective = joinSpec.ownerDirective(supergraph); + const fieldDirective = joinSpec.fieldDirective(supergraph); + + const getSubgraph = (application: Directive) => graphEnumNameToSubgraphName.get(application.arguments().graph); + + /* + * Fed2 supergraph have "provenance" information for all types and fields, so we can faithfully extract subgraph relatively easily. + * For fed1 supergraph however, only entity types are marked with `@join__type` and `@join__field`. Which mean that for value types, + * we cannot directly know in which subgraphs they were initially defined. One strategy consists in "extracting" value types into + * all subgraphs blindly: functionally, having some unused types in an extracted subgraph schema does not matter much. However, adding + * those useless types increases memory usage, and we've seen some case with lots of subgraphs and lots of value types where those + * unused types balloon up memory usage (from 100MB to 1GB in one example; obviously, this is made worst by the fact that javascript + * is pretty memory heavy in the first place). So to avoid that problem, for fed1 supergraph, we do a first pass where we collect + * for all the subgraphs the set of types that are actually reachable in that subgraph. As we extract do the actual type extraction, + * we use this to ignore non-reachable types for any given subgraph. + */ + let includeTypeInSubgraph: (t: NamedType, name: string) => boolean = () => true; + if (isFed1) { + const reachableTypesBySubgraph = collectFieldReachableTypesForAllSubgraphs( + supergraph, + subgraphs.names(), + (f, name) => { + const fieldApplications: Directive[] = f.appliedDirectivesOf(fieldDirective); + if (fieldApplications.length) { + const application = fieldApplications.find((application) => getSubgraph(application) === name); + if (application) { + const args = application.arguments(); + const typesInFederationDirectives = + typesUsedInFederationDirective(args.provides, baseType(f.type!) as CompositeType) + .concat(typesUsedInFederationDirective(args.requires, f.parent)); + return { isInSubgraph: true, typesInFederationDirectives }; + } else { + return { isInSubgraph: false, typesInFederationDirectives: [] }; + } + } else { + // No field application depends on the "owner" directive on the type. If we have no owner, then the + // field is in all subgraph and we return true. Otherwise, the field is only in the owner subgraph. + // In any case, the field cannot have a requires or provides + const ownerApplications = ownerDirective ? f.parent.appliedDirectivesOf(ownerDirective) : []; + return { isInSubgraph: !ownerApplications.length || getSubgraph(ownerApplications[0]) == name, typesInFederationDirectives: [] }; + } + }, + (t, name) => { + const typeApplications: Directive[] = t.appliedDirectivesOf(typeDirective); + const application = typeApplications.find((application) => (application.arguments().key && (getSubgraph(application) === name))); + if (application) { + const typesInFederationDirectives = typesUsedInFederationDirective(application.arguments().key, t as CompositeType); + return { isEntityWithKeyInSubgraph: true, typesInFederationDirectives }; + } else { + return { isEntityWithKeyInSubgraph: false, typesInFederationDirectives: [] }; + } + }, + ); + includeTypeInSubgraph = (t, name) => reachableTypesBySubgraph.get(name)?.has(t.name) ?? false; + } // Next, we iterate on all types and add it to the proper subgraphs (along with any @key). // Note that we first add all types empty and populate the types next. This avoids having to care about the iteration @@ -97,13 +261,15 @@ export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs { for (const type of filteredTypes(supergraph, joinSpec, coreFeatures.coreDefinition)) { const typeApplications = type.appliedDirectivesOf(typeDirective); if (!typeApplications.length) { - // Imply the type is in all subgraphs (technically, some subgraphs may not have had this type, but adding it - // in that case is harmless because it will be unreachable anyway). - subgraphs.values().map(sg => sg.schema).forEach(schema => schema.addType(newNamedType(type.kind, type.name))); + // Imply we don't know in which subgraph the type is, so we had it in all subgraph in which the type is reachable. + subgraphs + .values() + .filter((sg) => includeTypeInSubgraph(type, sg.name)) + .map(sg => sg.schema).forEach(schema => schema.addType(newNamedType(type.kind, type.name))); } else { for (const application of typeApplications) { const args = application.arguments(); - const subgraphName = graphEnumNameToSubgraphName.get(args.graph)!; + const subgraphName = getSubgraph(application)!; const schema = subgraphs.get(subgraphName)!.schema; // We can have more than one type directive for a given subgraph let subgraphType = schema.type(type.name); @@ -121,8 +287,6 @@ export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs { } } - const ownerDirective = joinSpec.ownerDirective(supergraph); - const fieldDirective = joinSpec.fieldDirective(supergraph); // We can now populate all those types (with relevant @provides and @requires on fields). for (const type of filteredTypes(supergraph, joinSpec, coreFeatures.coreDefinition)) { switch (type.kind) { @@ -181,7 +345,14 @@ export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs { const args = application.arguments(); const subgraph = subgraphs.get(graphEnumNameToSubgraphName.get(args.graph)!)!; const subgraphField = addSubgraphField(field, subgraph, args.type); - assert(subgraphField, () => `Found join__field directive for graph ${subgraph.name} on field ${field.coordinate} but no corresponding join__type on ${type}`); + if (!subgraphField) { + // It's unlikely but possible that a fed1 supergraph has a `@provides` on a field of a value type, + // and that value type is actually unreachable. Because we trim unreachable types for fed1 supergraph + // (see comment on `includeTypeInSubgraph` above), it would mean we get `undefined` here. It's fine + // however: the type is unreachable in this subgraph, so ignoring that field application is fine too. + assert(!includeTypeInSubgraph(type, subgraph.name), () => `Found join__field directive for graph ${subgraph.name} on field ${field.coordinate} but no corresponding join__type on ${type}`); + continue; + } if (args.requires) { subgraphField.applyDirective(subgraph.metadata().requiresDirective(), {'fields': args.requires}); } diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index b247ab5b4..ddd730029 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -7,7 +7,6 @@ import { Directive, DirectiveDefinition, ErrGraphQLValidationFailed, - errorCauses, FieldDefinition, InputFieldDefinition, InterfaceType, @@ -22,6 +21,7 @@ import { ScalarType, Schema, SchemaBlueprint, + SchemaConfig, SchemaDefinition, SchemaElement, sourceASTs, @@ -54,6 +54,7 @@ import { ERRORS, withModifiedErrorMessage, extractGraphQLErrorOptions, + errorCauses, } from "./error"; import { computeShareables } from "./precompute"; import { @@ -1024,8 +1025,8 @@ export function buildSubgraph( return subgraph.validate(); } -export function newEmptyFederation2Schema(): Schema { - const schema = new Schema(new FederationBlueprint(true)); +export function newEmptyFederation2Schema(config?: SchemaConfig): Schema { + const schema = new Schema(new FederationBlueprint(true), config); setSchemaAsFed2Subgraph(schema); return schema; } diff --git a/internals-js/src/print.ts b/internals-js/src/print.ts index a18edafd6..b175b0a31 100644 --- a/internals-js/src/print.ts +++ b/internals-js/src/print.ts @@ -94,7 +94,7 @@ export function printSchema(schema: Schema, options: PrintOptions = defaultPrint return definitions.flat().join('\n\n'); } -function definitionAndExtensions(element: {extensions(): ReadonlySet>}, options: PrintOptions): (Extension | null | undefined)[] { +function definitionAndExtensions(element: {extensions(): readonly Extension[]}, options: PrintOptions): (Extension | null | undefined)[] { return options.mergeTypesAndExtensions ? [undefined] : [null, ...element.extensions()]; } @@ -102,7 +102,7 @@ function printSchemaDefinitionAndExtensions(schemaDefinition: SchemaDefinition, return printDefinitionAndExtensions(schemaDefinition, options, printSchemaDefinitionOrExtension); } -function printDefinitionAndExtensions>}>( +function printDefinitionAndExtensions[]}>( t: T, options: PrintOptions, printer: (t: T, options: PrintOptions, extension?: Extension | null) => string | undefined diff --git a/internals-js/src/schemaUpgrader.ts b/internals-js/src/schemaUpgrader.ts index f01afdb20..9db0c4be1 100644 --- a/internals-js/src/schemaUpgrader.ts +++ b/internals-js/src/schemaUpgrader.ts @@ -4,12 +4,11 @@ import { Kind, print as printAST, } from "graphql"; -import { ERRORS } from "./error"; +import { errorCauses, ERRORS } from "./error"; import { baseType, CompositeType, Directive, - errorCauses, Extension, FieldDefinition, isCompositeType, @@ -432,7 +431,8 @@ class SchemaUpgrader { private fixFederationDirectivesArguments() { for (const directive of [this.metadata.keyDirective(), this.metadata.requiresDirective(), this.metadata.providesDirective()]) { - for (const application of directive.applications()) { + // Note that we may remove (to replace) some of the application we iterate on, so we need to copy the list we iterate on first. + for (const application of Array.from(directive.applications())) { const fields = application.arguments().fields; if (typeof fields !== 'string') { // The one case we have seen in practice is user passing an array of string, so we handle that. If it's something else, @@ -697,7 +697,8 @@ class SchemaUpgrader { return; } - for (const application of tagDirective.applications()) { + // Copying the list we iterate on as we remove in the loop. + for (const application of Array.from(tagDirective.applications())) { const element = application.parent; if (!(element instanceof FieldDefinition)) { continue; diff --git a/internals-js/src/supergraphs.ts b/internals-js/src/supergraphs.ts index 5951f10f4..022dde929 100644 --- a/internals-js/src/supergraphs.ts +++ b/internals-js/src/supergraphs.ts @@ -1,7 +1,6 @@ -import { ASTNode, DocumentNode } from "graphql"; -import { err } from '@apollo/core-schema'; +import { DocumentNode, GraphQLError } from "graphql"; import { ErrCoreCheckFailed, FeatureUrl, FeatureVersion } from "./coreSpec"; -import { CoreFeature, CoreFeatures, Schema } from "./definitions"; +import { CoreFeatures, Schema, sourceASTs } from "./definitions"; import { joinIdentity, JoinSpecDefinition, JOIN_VERSIONS } from "./joinSpec"; import { buildSchema, buildSchemaFromAST } from "./buildSchema"; import { extractSubgraphsNamesAndUrlsFromSupergraph } from "./extractSubgraphsFromSupergraph"; @@ -18,24 +17,6 @@ const SUPPORTED_FEATURES = new Set([ 'https://specs.apollo.dev/inaccessible/v0.2', ]); -export function ErrUnsupportedFeature(feature: CoreFeature): Error { - return err('UnsupportedFeature', { - message: `feature ${feature.url} is for: ${feature.purpose} but is unsupported`, - feature, - nodes: feature.directive.sourceAST, - }); -} - -export function ErrForUnsupported(core: CoreFeature, ...features: readonly CoreFeature[]): Error { - return err('ForUnsupported', { - message: - `the \`for:\` argument is unsupported by version ${core.url.version} ` + - `of the core spec. Please upgrade to at least @core v0.2 (https://specs.apollo.dev/core/v0.2).`, - features, - nodes: [core.directive.sourceAST, ...features.map(f => f.directive.sourceAST)].filter(n => !!n) as ASTNode[] - }); -} - const coreVersionZeroDotOneUrl = FeatureUrl.parse('https://specs.apollo.dev/core/v0.1'); export function buildSupergraphSchema(supergraphSdl: string | DocumentNode): [Schema, {name: string, url: string}[]] { @@ -56,18 +37,28 @@ export function buildSupergraphSchema(supergraphSdl: string | DocumentNode): [Sc * Throws if that is not true. */ function checkFeatureSupport(coreFeatures: CoreFeatures) { - const errors = []; - if (coreFeatures.coreItself.url.equals(coreVersionZeroDotOneUrl)) { + const errors: GraphQLError[] = []; + const coreItself = coreFeatures.coreItself; + if (coreItself.url.equals(coreVersionZeroDotOneUrl)) { const purposefulFeatures = [...coreFeatures.allFeatures()].filter(f => f.purpose) if (purposefulFeatures.length > 0) { - errors.push(ErrForUnsupported(coreFeatures.coreItself, ...purposefulFeatures)); + errors.push(ERRORS.UNSUPPORTED_LINKED_FEATURE.err( + `the \`for:\` argument is unsupported by version ${coreItself.url.version} ` + + `of the core spec. Please upgrade to at least @core v0.2 (https://specs.apollo.dev/core/v0.2).`, + { + nodes: sourceASTs(coreItself.directive, ...purposefulFeatures.map(f => f.directive)) + } + )); } } for (const feature of coreFeatures.allFeatures()) { if (feature.url.equals(coreVersionZeroDotOneUrl) || feature.purpose === 'EXECUTION' || feature.purpose === 'SECURITY') { if (!SUPPORTED_FEATURES.has(feature.url.base.toString())) { - errors.push(ErrUnsupportedFeature(feature)); + errors.push(ERRORS.UNSUPPORTED_LINKED_FEATURE.err( + `feature ${feature.url} is for: ${feature.purpose} but is unsupported`, + { nodes: feature.directive.sourceAST }, + )); } } } diff --git a/internals-js/src/utils.ts b/internals-js/src/utils.ts index 7a5bd53dd..6beb59bf1 100644 --- a/internals-js/src/utils.ts +++ b/internals-js/src/utils.ts @@ -394,3 +394,18 @@ export type Concrete = { // const x = [1,2,undefined]; // const y: number[] = x.filter(isDefined); export const isDefined = (t: T | undefined): t is T => t === undefined ? false : true; + +/** + * Removes the first occurrence of the provided element in the provided array, if said array contains said elements. + * + * @return whether the element was removed. + */ +export function removeArrayElement(element: T, array: T[]): boolean { + const index = array.indexOf(element); + if (index >= 0) { + array.splice(index, 1); + return true; + } else { + return false; + } +} diff --git a/package-lock.json b/package-lock.json index 80ddb1144..ae7fbdbf0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -96,7 +96,6 @@ "license": "SEE LICENSE IN ./LICENSE", "dependencies": { "@apollo/composition": "file:../composition-js", - "@apollo/core-schema": "~0.3.0", "@apollo/federation-internals": "file:../internals-js", "@apollo/query-planner": "file:../query-planner-js", "@apollo/server-gateway-interface": "^1.0.2", @@ -286,7 +285,6 @@ "version": "2.1.0-alpha.4", "license": "SEE LICENSE IN ./LICENSE", "dependencies": { - "@apollo/core-schema": "~0.3.0", "chalk": "^4.1.0", "js-levenshtein": "^1.1.6" }, @@ -345,19 +343,6 @@ "resolved": "composition-js", "link": true }, - "node_modules/@apollo/core-schema": { - "version": "0.3.0", - "license": "MIT", - "dependencies": { - "@protoplasm/recall": "^0.2" - }, - "engines": { - "node": ">=12.13.0" - }, - "peerDependencies": { - "graphql": "^15 || ^16" - } - }, "node_modules/@apollo/federation-internals": { "resolved": "internals-js", "link": true @@ -4504,13 +4489,6 @@ "version": "1.1.0", "license": "BSD-3-Clause" }, - "node_modules/@protoplasm/recall": { - "version": "0.2.2", - "license": "MIT", - "engines": { - "node": ">=12.13.0" - } - }, "node_modules/@sinclair/typebox": { "version": "0.24.19", "license": "MIT" @@ -18770,16 +18748,9 @@ "@apollo/query-graphs": "file:../query-graphs-js" } }, - "@apollo/core-schema": { - "version": "0.3.0", - "requires": { - "@protoplasm/recall": "^0.2" - } - }, "@apollo/federation-internals": { "version": "file:internals-js", "requires": { - "@apollo/core-schema": "~0.3.0", "chalk": "^4.1.0", "js-levenshtein": "^1.1.6" } @@ -18788,7 +18759,6 @@ "version": "file:gateway-js", "requires": { "@apollo/composition": "file:../composition-js", - "@apollo/core-schema": "~0.3.0", "@apollo/federation-internals": "file:../internals-js", "@apollo/query-planner": "file:../query-planner-js", "@apollo/server-gateway-interface": "^1.0.2", @@ -21884,9 +21854,6 @@ "@protobufjs/utf8": { "version": "1.1.0" }, - "@protoplasm/recall": { - "version": "0.2.2" - }, "@sinclair/typebox": { "version": "0.24.19" }, diff --git a/query-graphs-js/CHANGELOG.md b/query-graphs-js/CHANGELOG.md index 7912eccd1..6a635a742 100644 --- a/query-graphs-js/CHANGELOG.md +++ b/query-graphs-js/CHANGELOG.md @@ -1,5 +1,6 @@ # CHANGELOG for `@apollo/query-graphs` +- Fix abnormally high memory usage when extracting subgraphs for some fed1 supergraphs (and small other memory footprint improvements) [PR #2089](https://github.com/apollographql/federation/pull/2089). - Fix issue when type is only reachable through a @provides [PR #2083](https://github.com/apollographql/federation/pull/2083). ## 2.1.0-alpha.4