From 4ec2cfa1339d2155715970af348f79dd13d80edf Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Mon, 12 May 2025 20:43:08 -0500 Subject: [PATCH 1/5] Add new CompositionOption to bail if total number of subgraph path infos in the stack gets above a configured value. --- composition-js/src/compose.ts | 6 ++++-- composition-js/src/validate.ts | 28 +++++++++++++++++++++++++--- internals-js/src/error.ts | 7 +++++++ 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/composition-js/src/compose.ts b/composition-js/src/compose.ts index 3ec5711fc..014941c3c 100644 --- a/composition-js/src/compose.ts +++ b/composition-js/src/compose.ts @@ -39,6 +39,8 @@ export interface CompositionOptions { allowedFieldTypeMergingSubtypingRules?: SubtypingRule[]; /// Flag to toggle if satisfiability should be performed during composition runSatisfiability?: boolean; + /// Maximum allowable number of outstanding subgraph paths to validate + maxValidationSubgraphPaths?: number; } function validateCompositionOptions(options: CompositionOptions) { @@ -123,7 +125,7 @@ type SatisfiabilityArgs = { * @param args: SatisfiabilityArgs * @returns { errors? : GraphQLError[], hints? : CompositionHint[] } */ -export function validateSatisfiability({ supergraphSchema, supergraphSdl} : SatisfiabilityArgs) : { +export function validateSatisfiability({ supergraphSchema, supergraphSdl} : SatisfiabilityArgs, options: CompositionOptions = {}) : { errors? : GraphQLError[], hints? : CompositionHint[], } { @@ -133,7 +135,7 @@ export function validateSatisfiability({ supergraphSchema, supergraphSdl} : Sati const supergraph = supergraphSchema ? new Supergraph(supergraphSchema, null) : Supergraph.build(supergraphSdl, { supportedFeatures: null }); const supergraphQueryGraph = buildSupergraphAPIQueryGraph(supergraph); const federatedQueryGraph = buildFederatedQueryGraph(supergraph, false); - return validateGraphComposition(supergraph.schema, supergraph.subgraphNameToGraphEnumValue(), supergraphQueryGraph, federatedQueryGraph); + return validateGraphComposition(supergraph.schema, supergraph.subgraphNameToGraphEnumValue(), supergraphQueryGraph, federatedQueryGraph, options); } type ValidateSubgraphsAndMergeResult = MergeResult | { errors: GraphQLError[] }; diff --git a/composition-js/src/validate.ts b/composition-js/src/validate.ts index 1cb477e5a..2061bb3a1 100644 --- a/composition-js/src/validate.ts +++ b/composition-js/src/validate.ts @@ -62,6 +62,7 @@ import { } from "@apollo/query-graphs"; import { CompositionHint, HINTS } from "./hints"; import { ASTNode, GraphQLError, print } from "graphql"; +import { CompositionOptions } from './compose'; const debug = newDebugLogger('validation'); @@ -310,6 +311,7 @@ export function validateGraphComposition( subgraphNameToGraphEnumValue: Map, supergraphAPI: QueryGraph, federatedQueryGraph: QueryGraph, + compositionOptions: CompositionOptions = {}, ): { errors? : GraphQLError[], hints? : CompositionHint[], @@ -319,6 +321,7 @@ export function validateGraphComposition( subgraphNameToGraphEnumValue, supergraphAPI, federatedQueryGraph, + compositionOptions, ).validate(); return errors.length > 0 ? { errors, hints } : { hints }; } @@ -695,19 +698,21 @@ class ValidationTraversal { private readonly validationHints: CompositionHint[] = []; private readonly context: ValidationContext; + private totalValidationSubgraphPaths = 0; constructor( supergraphSchema: Schema, subgraphNameToGraphEnumValue: Map, supergraphAPI: QueryGraph, federatedQueryGraph: QueryGraph, + readonly compositionOptions: CompositionOptions, ) { this.conditionResolver = simpleValidationConditionResolver({ supergraph: supergraphSchema, queryGraph: federatedQueryGraph, withCaching: true, }); - supergraphAPI.rootKinds().forEach((kind) => this.stack.push(ValidationState.initial({ + supergraphAPI.rootKinds().forEach((kind) => this.pushStack(ValidationState.initial({ supergraphAPI, kind, federatedQueryGraph, @@ -720,13 +725,30 @@ class ValidationTraversal { subgraphNameToGraphEnumValue, ); } + + pushStack(state: ValidationState) { + this.totalValidationSubgraphPaths += state.subgraphPathInfos.length; + this.stack.push(state); + console.log(`totalValidationSubgraphPaths ${this.totalValidationSubgraphPaths}`); + if (this.compositionOptions.maxValidationSubgraphPaths && this.totalValidationSubgraphPaths > this.compositionOptions.maxValidationSubgraphPaths) { + throw ERRORS.MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED.err(`Maximum number of validation subgraph paths exceeded: ${this.totalValidationSubgraphPaths}`); + } + } + + popStack() { + const state = this.stack.pop(); + if (state) { + this.totalValidationSubgraphPaths -= state.subgraphPathInfos.length; + } + return state; + } validate(): { errors: GraphQLError[], hints: CompositionHint[], } { while (this.stack.length > 0) { - this.handleState(this.stack.pop()!); + this.handleState(this.popStack()!); } return { errors: this.validationErrors, hints: this.validationHints }; } @@ -799,7 +821,7 @@ class ValidationTraversal { // state to the stack this method, `handleState`, will do nothing later. But it's // worth checking it now and save some memory/cycles. if (newState && !newState.supergraphPath.isTerminal()) { - this.stack.push(newState); + this.pushStack(newState); debug.groupEnd(() => `Reached new state ${newState}`); } else { debug.groupEnd(`Reached terminal vertex/cycle`); diff --git a/internals-js/src/error.ts b/internals-js/src/error.ts index f658b2acc..3f96dccbc 100644 --- a/internals-js/src/error.ts +++ b/internals-js/src/error.ts @@ -627,6 +627,12 @@ const LIST_SIZE_INVALID_SIZED_FIELD = makeCodeDefinition( { addedIn: '2.9.2' }, ); +const MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED = makeCodeDefinition( + 'MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED', + 'The maximum number of validation subgraph paths has been exceeded.', + { addedIn: '2.8.0' }, +); + export const ERROR_CATEGORIES = { DIRECTIVE_FIELDS_MISSING_EXTERNAL, DIRECTIVE_UNSUPPORTED_ON_INTERFACE, @@ -727,6 +733,7 @@ export const ERRORS = { LIST_SIZE_INVALID_ASSUMED_SIZE, LIST_SIZE_INVALID_SIZED_FIELD, LIST_SIZE_INVALID_SLICING_ARGUMENT, + MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED, }; const codeDefByCode = Object.values(ERRORS).reduce((obj: {[code: string]: ErrorCodeDefinition}, codeDef: ErrorCodeDefinition) => { obj[codeDef.code] = codeDef; return obj; }, {}); From 01de2afabcfd876ddb24a9362f47a5d1f24a4c6e Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Wed, 14 May 2025 10:23:49 -0500 Subject: [PATCH 2/5] Adding changeset and setting a default value. --- .changeset/four-panthers-itch.md | 6 ++++++ composition-js/src/validate.ts | 12 ++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-) create mode 100644 .changeset/four-panthers-itch.md diff --git a/.changeset/four-panthers-itch.md b/.changeset/four-panthers-itch.md new file mode 100644 index 000000000..8fd1ee748 --- /dev/null +++ b/.changeset/four-panthers-itch.md @@ -0,0 +1,6 @@ +--- +"@apollo/composition": patch +"@apollo/federation-internals": patch +--- + +Adding new CompositionOption `maxValidationSubgraphPaths`. This value represents the maximium number of SubgraphPathInfo objects that may exist in a ValidationTraversal when checking for satisfiability. Setting this value can help composition error before running out of memory. Default is 1,000,000. diff --git a/composition-js/src/validate.ts b/composition-js/src/validate.ts index 2061bb3a1..d4f5967d5 100644 --- a/composition-js/src/validate.ts +++ b/composition-js/src/validate.ts @@ -699,14 +699,19 @@ class ValidationTraversal { private readonly context: ValidationContext; private totalValidationSubgraphPaths = 0; - + private maxValidationSubgraphPaths: number; + + private static DEFAULT_MAX_VALIDATION_SUBGRAPH_PATHS = 1000000; + constructor( supergraphSchema: Schema, subgraphNameToGraphEnumValue: Map, supergraphAPI: QueryGraph, federatedQueryGraph: QueryGraph, - readonly compositionOptions: CompositionOptions, + compositionOptions: CompositionOptions, ) { + this.maxValidationSubgraphPaths = compositionOptions.maxValidationSubgraphPaths ?? ValidationTraversal.DEFAULT_MAX_VALIDATION_SUBGRAPH_PATHS; + this.conditionResolver = simpleValidationConditionResolver({ supergraph: supergraphSchema, queryGraph: federatedQueryGraph, @@ -729,8 +734,7 @@ class ValidationTraversal { pushStack(state: ValidationState) { this.totalValidationSubgraphPaths += state.subgraphPathInfos.length; this.stack.push(state); - console.log(`totalValidationSubgraphPaths ${this.totalValidationSubgraphPaths}`); - if (this.compositionOptions.maxValidationSubgraphPaths && this.totalValidationSubgraphPaths > this.compositionOptions.maxValidationSubgraphPaths) { + if (this.totalValidationSubgraphPaths > this.maxValidationSubgraphPaths) { throw ERRORS.MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED.err(`Maximum number of validation subgraph paths exceeded: ${this.totalValidationSubgraphPaths}`); } } From 30f03097da9e7e2f207fc49e72f9abad74e3da2a Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Wed, 14 May 2025 10:34:49 -0500 Subject: [PATCH 3/5] spelling --- .changeset/four-panthers-itch.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/four-panthers-itch.md b/.changeset/four-panthers-itch.md index 8fd1ee748..7a88d78c7 100644 --- a/.changeset/four-panthers-itch.md +++ b/.changeset/four-panthers-itch.md @@ -3,4 +3,4 @@ "@apollo/federation-internals": patch --- -Adding new CompositionOption `maxValidationSubgraphPaths`. This value represents the maximium number of SubgraphPathInfo objects that may exist in a ValidationTraversal when checking for satisfiability. Setting this value can help composition error before running out of memory. Default is 1,000,000. +Adding new CompositionOption `maxValidationSubgraphPaths`. This value represents the maximum number of SubgraphPathInfo objects that may exist in a ValidationTraversal when checking for satisfiability. Setting this value can help composition error before running out of memory. Default is 1,000,000. From 0dc9e84e3dc2db00e53c2cec0412dbad56922726 Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Wed, 14 May 2025 11:53:14 -0500 Subject: [PATCH 4/5] Add test --- .../src/__tests__/validation_errors.test.ts | 77 +++++++++++++++++++ composition-js/src/compose.ts | 16 ++-- 2 files changed, 87 insertions(+), 6 deletions(-) diff --git a/composition-js/src/__tests__/validation_errors.test.ts b/composition-js/src/__tests__/validation_errors.test.ts index 43e78dcc2..1a69b413e 100644 --- a/composition-js/src/__tests__/validation_errors.test.ts +++ b/composition-js/src/__tests__/validation_errors.test.ts @@ -412,3 +412,80 @@ describe('when shared field has non-intersecting runtime types in different subg ]); }); }); + +describe('other validation errors', () => { + + it('errors when maxValidationSubgraphPaths is exceeded', () => { + const subgraphA = { + name: 'A', + typeDefs: gql` + type Query { + a: A + } + + type A @key(fields: "id") { + id: ID! + b: B + c: C + d: D + } + + type B @key(fields: "id") { + id: ID! + a: A @shareable + b: Int @shareable + c: C @shareable + d: D @shareable + } + + type C @key(fields: "id") { + id: ID! + a: A @shareable + b: B @shareable + c: Int @shareable + d: D @shareable + } + + type D @key(fields: "id") { + id: ID! + a: A @shareable + b: B @shareable + c: C @shareable + d: Int @shareable + } + ` + }; + const subgraphB = { + name: 'B', + typeDefs: gql` + type B @key(fields: "id") { + id: ID! + b: Int @shareable + c: C @shareable + d: D @shareable + } + + type C @key(fields: "id") { + id: ID! + b: B @shareable + c: Int @shareable + d: D @shareable + } + + type D @key(fields: "id") { + id: ID! + b: B @shareable + c: C @shareable + d: Int @shareable + } + ` + }; + const result = composeAsFed2Subgraphs([subgraphA, subgraphB], { maxValidationSubgraphPaths: 10 }); + expect(result.errors).toBeDefined(); + expect(errorMessages(result)).toMatchStringArray([ + ` + Maximum number of validation subgraph paths exceeded: 12 + ` + ]); + }); +}); diff --git a/composition-js/src/compose.ts b/composition-js/src/compose.ts index 014941c3c..97d9c3655 100644 --- a/composition-js/src/compose.ts +++ b/composition-js/src/compose.ts @@ -57,7 +57,7 @@ function validateCompositionOptions(options: CompositionOptions) { * @param options CompositionOptions */ export function compose(subgraphs: Subgraphs, options: CompositionOptions = {}): CompositionResult { - const { runSatisfiability = true, sdlPrintOptions } = options; + const { runSatisfiability = true, sdlPrintOptions, maxValidationSubgraphPaths } = options; validateCompositionOptions(options); @@ -68,11 +68,15 @@ export function compose(subgraphs: Subgraphs, options: CompositionOptions = {}): let satisfiabilityResult; if (runSatisfiability) { - satisfiabilityResult = validateSatisfiability({ - supergraphSchema: mergeResult.supergraph - }); - if (satisfiabilityResult.errors) { - return { errors: satisfiabilityResult.errors }; + try { + satisfiabilityResult = validateSatisfiability({ + supergraphSchema: mergeResult.supergraph, + }, { maxValidationSubgraphPaths }); + if (satisfiabilityResult.errors) { + return { errors: satisfiabilityResult.errors }; + } + } catch (err) { + return { errors: [err] }; } } From a657a82d851f8d3d66772eda088decea9eae5fc7 Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Thu, 22 May 2025 13:58:32 -0500 Subject: [PATCH 5/5] code review comments --- composition-js/src/compose.ts | 14 +++++--------- composition-js/src/validate.ts | 20 ++++++++++++++------ 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/composition-js/src/compose.ts b/composition-js/src/compose.ts index 97d9c3655..4216474ad 100644 --- a/composition-js/src/compose.ts +++ b/composition-js/src/compose.ts @@ -68,15 +68,11 @@ export function compose(subgraphs: Subgraphs, options: CompositionOptions = {}): let satisfiabilityResult; if (runSatisfiability) { - try { - satisfiabilityResult = validateSatisfiability({ - supergraphSchema: mergeResult.supergraph, - }, { maxValidationSubgraphPaths }); - if (satisfiabilityResult.errors) { - return { errors: satisfiabilityResult.errors }; - } - } catch (err) { - return { errors: [err] }; + satisfiabilityResult = validateSatisfiability({ + supergraphSchema: mergeResult.supergraph, + }, { maxValidationSubgraphPaths }); + if (satisfiabilityResult.errors) { + return { errors: satisfiabilityResult.errors }; } } diff --git a/composition-js/src/validate.ts b/composition-js/src/validate.ts index d4f5967d5..a813647ef 100644 --- a/composition-js/src/validate.ts +++ b/composition-js/src/validate.ts @@ -731,12 +731,13 @@ class ValidationTraversal { ); } - pushStack(state: ValidationState) { + pushStack(state: ValidationState): { error?: GraphQLError } { this.totalValidationSubgraphPaths += state.subgraphPathInfos.length; this.stack.push(state); if (this.totalValidationSubgraphPaths > this.maxValidationSubgraphPaths) { - throw ERRORS.MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED.err(`Maximum number of validation subgraph paths exceeded: ${this.totalValidationSubgraphPaths}`); + return { error: ERRORS.MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED.err(`Maximum number of validation subgraph paths exceeded: ${this.totalValidationSubgraphPaths}`) }; } + return {}; } popStack() { @@ -752,12 +753,15 @@ class ValidationTraversal { hints: CompositionHint[], } { while (this.stack.length > 0) { - this.handleState(this.popStack()!); + const { error } = this.handleState(this.popStack()!); + if (error) { + return { errors: [error], hints: this.validationHints }; + } } return { errors: this.validationErrors, hints: this.validationHints }; } - private handleState(state: ValidationState) { + private handleState(state: ValidationState): { error?: GraphQLError } { debug.group(() => `Validation: ${this.stack.length + 1} open states. Validating ${state}`); const vertex = state.supergraphPath.tail; @@ -774,7 +778,7 @@ class ValidationTraversal { // type, and have strictly more options regarding subgraphs. So whatever comes next, we can handle in the exact // same way we did previously, and there is thus no way to bother. debug.groupEnd(`Has already validated this vertex.`); - return; + return {}; } } // We're gonna have to validate, but we can save the new set of sources here to hopefully save work later. @@ -825,12 +829,16 @@ class ValidationTraversal { // state to the stack this method, `handleState`, will do nothing later. But it's // worth checking it now and save some memory/cycles. if (newState && !newState.supergraphPath.isTerminal()) { - this.pushStack(newState); + const { error } = this.pushStack(newState); + if (error) { + return { error }; + } debug.groupEnd(() => `Reached new state ${newState}`); } else { debug.groupEnd(`Reached terminal vertex/cycle`); } } debug.groupEnd(); + return {}; } }