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