Skip some supergraph validations in production by default (#2657)

Stop running the full suite of graphQL validations on supergraphs and
their extracted subgraphs by default in production environment.

Running those validations on every updates of the schema takes a
non-negligible amount of time (especially on large schema) and
mainly only serves in catching bugs early in the supergraph handling
code, and in some limited cases, provide slightly better messages
when a corrupted supergraph is received, neither of which is worth
the cost in production environment.

A new `validateSupergraph` option is also introduced in the gateway
configuration to force this behaviour.

Co-authored-by: Chris Lenfest <[email protected]>
Co-authored-by: Trevor Scheer <[email protected]>

Author: 3 people

.changeset/dull-lamps-invent.md

@@ -0,0 +1,17 @@
+---
+"@apollo/query-planner": minor
+"@apollo/query-graphs": minor
+"@apollo/composition": minor
+"@apollo/federation-internals": minor
+"@apollo/gateway": minor
+---
+
+Do not run the full suite of graphQL validations on supergraphs and their extracted subgraphs by default in production environment.
+
+Running those validations on every updates of the schema takes a non-negligible amount of time (especially on large
+schema) and mainly only serves in catching bugs early in the supergraph handling code, and in some limited cases,
+provide slightly better messages when a corrupted supergraph is received, neither of which is worth the cost in
+production environment.
+
+A new `validateSupergraph` option is also introduced in the gateway configuration to force this behaviour.
+  

gateway-js/src/config.ts

@@ -129,6 +129,24 @@ interface GatewayConfigBase {
   serviceHealthCheck?: boolean;
 
   queryPlannerConfig?: QueryPlannerConfig;
+
+  /**
+   * Whether to validate the supergraphs received from either the static configuration or the
+   * configured supergraph manager.
+   *
+   * When enables, this run validations to make sure the supergraph SDL is full valid graphQL
+   * and it equally validates the subgraphs extracted from that supergraph. Note that even when
+   * this is disabled, the supergraph SDL still needs to be valid graphQL syntax and essentially
+   * be a valid supergraph for the gateway to be able to use it, and so this option is not
+   * necessary to protected against corrupted/invalid supergraphs, but it may produce more legible
+   * errors when facing such invalid supergraph.
+   *
+   * By default, this depends on the value of `NODE_ENV`: for production, validation is disabled
+   * (as it is somewhat expensive and not that valuable as mentioned above), but it is enabled
+   * for development (mostly to provide better error messages when provided with an incorrect
+   * supergraph).
+   */
+  validateSupergraph?: boolean;
 }
 
 // TODO(trevor:removeServiceList)

gateway-js/src/index.ts

@@ -634,7 +634,8 @@ export class ApolloGateway implements GatewayInterface {
   }
 
   private createSchemaFromSupergraphSdl(supergraphSdl: string) {
-    const supergraph = Supergraph.build(supergraphSdl);
+    const validateSupergraph = this.config.validateSupergraph ?? process.env.NODE_ENV !== 'production';
+    const supergraph = Supergraph.build(supergraphSdl, { validateSupergraph });
     this.createServices(supergraph.subgraphsMetadata());
 
     return {

internals-js/src/definitions.ts

@@ -1576,6 +1576,22 @@ export class Schema {
     this.isValidated = false;
   }
 
+  /**
+   * Marks the schema as validated _without running actual validation_.
+   * Should obviously only be called when we know the built schema must be valid.
+   *
+   * Note that if `validate` is called after this, then it will exit immediately without validation as
+   * the schema will have been marked as validated. However, if this schema is further modified, then
+   * `invalidate` will be called, after which `validate` would run validation again.
+   */
+  assumeValid() {
+    this.runWithBuiltInModificationAllowed(() => {
+      addIntrospectionFields(this);
+    });
+
+    this.isValidated = true;
+  }
+
   validate() {
     if (this.isValidated) {
       return;
@@ -1609,9 +1625,7 @@ export class Schema {
     const cloned = new Schema(builtIns ?? this.blueprint);
     copy(this, cloned);
     if (this.isValidated) {
-      // TODO: when we do actual validation, no point in redoing it, but we should
-      // at least call builtIns.onValidation() and set the proper isConstructed/isValidated flags.
-      cloned.validate();
+      cloned.assumeValid();
     }
     return cloned;
   }

internals-js/src/extractSubgraphsFromSupergraph.ts

@@ -193,7 +193,7 @@ function typesUsedInFederationDirective(fieldSet: string | undefined, parentType
   return usedTypes;
 }
 
-export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs {
+export function extractSubgraphsFromSupergraph(supergraph: Schema, validateExtractedSubgraphs: boolean = true): Subgraphs {
   const [coreFeatures, joinSpec] = validateSupergraph(supergraph);
   const isFed1 = joinSpec.version.equals(new FeatureVersion(0, 1));
   try {
@@ -229,11 +229,15 @@ export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs {
     // We're done with the subgraphs, so call validate (which, amongst other things, sets up the _entities query field, which ensures
     // all entities in all subgraphs are reachable from a query and so are properly included in the "query graph" later).
     for (const subgraph of subgraphs) {
-      try {
-        subgraph.validate();
-      } catch (e) {
-        // This is going to be caught directly by the enclosing try-catch, but this is so we indicate the subgraph having the issue.
-        throw new SubgraphExtractionError(e, subgraph);
+      if (validateExtractedSubgraphs) {
+        try {
+          subgraph.validate();
+        } catch (e) {
+          // This is going to be caught directly by the enclosing try-catch, but this is so we indicate the subgraph having the issue.
+          throw new SubgraphExtractionError(e, subgraph);
+        }
+      } else {
+        subgraph.assumeValid();
       }
     }
 

internals-js/src/federation.ts

@@ -1690,6 +1690,15 @@ export class Subgraph {
     }
   }
 
+  /**
+   * Same as `Schema.assumeValid`. Use carefully.
+   */
+  assumeValid(): Subgraph {
+    this.addFederationOperations();
+    this.schema.assumeValid();
+    return this;
+  }
+
   validate(): Subgraph {
     try {
       this.addFederationOperations();

internals-js/src/supergraphs.ts

@@ -87,20 +87,25 @@ export class Supergraph {
   constructor(
     readonly schema: Schema,
     supportedFeatures: Set<string> = DEFAULT_SUPPORTED_SUPERGRAPH_FEATURES,
+    private readonly shouldValidate: boolean = true,
   ) {
     const [coreFeatures] = validateSupergraph(schema);
     checkFeatureSupport(coreFeatures, supportedFeatures);
-    schema.validate();
+    if (shouldValidate) {
+      schema.validate();
+    } else {
+      schema.assumeValid();
+    }
     this.containedSubgraphs = extractSubgraphsNamesAndUrlsFromSupergraph(schema);
   }
 
-  static build(supergraphSdl: string | DocumentNode, options?: { supportedFeatures?: Set<string> }) {
+  static build(supergraphSdl: string | DocumentNode, options?: { supportedFeatures?: Set<string>, validateSupergraph?: boolean }) {
     // We delay validation because `checkFeatureSupport` in the constructor gives slightly more useful errors if, say, 'for' is used with core v0.1.
     const schema = typeof supergraphSdl === 'string'
       ? buildSchema(supergraphSdl, { validate: false })
       : buildSchemaFromAST(supergraphSdl, { validate: false });
 
-    return new Supergraph(schema, options?.supportedFeatures);
+    return new Supergraph(schema, options?.supportedFeatures, options?.validateSupergraph);
   }
 
   /**
@@ -118,7 +123,7 @@ export class Supergraph {
       // Note that `extractSubgraphsFromSupergraph` redo a little bit of work we're already one, like validating
       // the supergraph. We could refactor things to avoid it, but it's completely negligible in practice so we
       // can leave that to "some day, maybe".
-      this._subgraphs = extractSubgraphsFromSupergraph(this.schema);
+      this._subgraphs = extractSubgraphsFromSupergraph(this.schema, this.shouldValidate);
     }
     return this._subgraphs;
   }