Skip some supergraph validations in production by default #2657

pcmanus · 2023-07-05T13:50:40Z

When receiving a supergraph, we were always running all the graphQL validations on both the supergraph itslef, and on each of the subgraphs we extract from it.

This is useful for us when doing dev, as it allows to get decent error message when we break the supergraph generation process or the subgraph extraction process, but this is of very limited value otherwise: even if we don't run those full validations, the supergraph still has to be valid graphQL syntax to parse in the first place, and the subgraph extraction will still throw if the supergraph is not essentially a valid supergraph (and note that the code checking that the join spec version is valid, or that the supergraph does not use non-supported feature for SECURITY or EXECUTION is not disabled by this).

Besides, it's not like uplink will send basically-valid-but-slightly-corrupted supergraphs, and when using IntrospectAndCompose there isn't really room for the supergraph to be corrupted.

And those validations are actually not super cheap, especially on large supergraphs, so running them feels wasteful.

This commit thus disable those validation in production environment (when NODE_ENV === "production") by default. It keeps those validation during dev mainly for 2 reasons:

as mentioned above, so that those validate are run when we run our own unit tests, to help catch/identify any subtle mistake we would introduce in the supergraph generation and extaction codes.
when debugging something, it can happens that one start a gateway from a statically provided supergraph SDL, SDL that one may have manually edited for some reason (testing something, playing with it, ...). In that case, the validation can provide nicer errors if the manual edits are invalid.

The commit also introduce a validateReceivedSupergraphs in the gateway config to allows overriding the default behaviour described above.

netlify · 2023-07-05T13:50:43Z

‼️ Deploy request for apollo-federation-docs rejected.

Name	Link
🔨 Latest commit	2d543e90693c6066bebb2ea3b085d58f4742db2d

changeset-bot · 2023-07-05T13:50:43Z

🦋 Changeset detected

Latest commit: 387d2c5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@apollo/query-planner	Minor
@apollo/query-graphs	Minor
@apollo/composition	Minor
@apollo/federation-internals	Minor
@apollo/gateway	Minor
@apollo/subgraph	Minor
apollo-federation-integration-testsuite	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codesandbox-ci · 2023-07-05T13:51:34Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

pcmanus · 2023-07-05T13:52:04Z

Note that this PR is currently on top of #2655 because it uses the refactor done in that PR to be able to pass the argument to disable validation cleanly.

When receiving a supergraph, we were always running all the graphQL validations on both the supergraph itslef, and on each of the subgraphs we extract from it. This is useful for us when doing dev, as it allows to get decent error message when we break the supergraph generation process or the subgraph extraction process, but this is of very limited value otherwise: even if we don't run those full validations, the supergraph still has to be valid graphQL syntax to parse in the first place, and the subgraph extraction will still throw if the supergraph is not essentially a valid supergraph (and note that the code checking that the `join` spec version is valid, or that the supergraph does not use non-supported feature for `SECURITY` or `EXECUTION` is *not* disabled by this). Besides, it's not like uplink will send basically-valid-but-slightly-corrupted supergraphs, and when using `IntrospectAndCompose` there isn't really room for the supergraph to be corrupted. And those validations are actually not super cheap, especially on large supergraphs, so running them feels wasteful. This commit thus disable those validation in production environment (when `NODE_ENV === "production"`) by default. It keeps those validation during dev mainly for 2 reasons: 1. as mentioned above, so that those validate are run when we run our own unit tests, to help catch/identify any subtle mistake we would introduce in the supergraph generation and extaction codes. 2. when debugging something, it can happens that one start a gateway from a statically provided supergraph SDL, SDL that one may have manually edited for some reason (testing something, playing with it, ...). In that case, the validation can provide nicer errors if the manual edits are invalid. The commit also introduce a `validateReceivedSupergraphs` in the gateway config to allows overriding the default behaviour described above.

trevor-scheer

Changes lgtm. Maybe we should add some tests around the assumeValid behavior? I think it would be fine to follow up with those later, just to preserve the behavior we expect going forward.

.changeset/dull-lamps-invent.md

trevor-scheer · 2023-07-13T17:20:09Z

gateway-js/src/config.ts

+   * for development (mostly to provide better error messages when provided with an incorrect
+   * supergraph).
+   */
+  validateReceivedSupergraphs?: boolean;


Small naming nitpick, not a big deal though. validateSupergraph(s) maybe? "received" seems extraneous. I can see the argument to keep it plural but it doesn't seem necessary, fine either way.

gateway-js/src/config.ts

clenfest · 2023-07-13T19:00:51Z

internals-js/src/supergraphs.ts

@@ -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.assumeValid);


I don't love that we start with validateReceivedSupergraphs then go to assumeValid then back to validateExtractedSubgraphs. It would be nice to not keep negating the poor boolean.

clenfest · 2023-07-13T20:05:45Z

gateway-js/src/index.ts

@@ -634,8 +634,8 @@ export class ApolloGateway implements GatewayInterface {
  }

  private createSchemaFromSupergraphSdl(supergraphSdl: string) {
-    const assumeValid = !(this.config.validateReceivedSupergraphs ?? process.env.NODE_ENV !== 'production');
-    const supergraph = Supergraph.build(supergraphSdl, { assumeValid });
+    const validateSupergraph = this.config.validateSupergraph ?? process.env.NODE_ENV !== 'production';


I think this is wrong. If they explicitly set validateSupergraph to false, we should probably honor that, no?

false ?? true equates to false, unlike false || true, so I think this is doing what we want.

Yeah, of course. Clearly been doing too much rust lately. ship it.

pcmanus requested a review from a team as a code owner July 5, 2023 13:50

pcmanus force-pushed the disable-validation branch 2 times, most recently from 15ca882 to 2d543e9 Compare July 10, 2023 12:58

pcmanus requested a review from a team as a code owner July 10, 2023 12:58

pcmanus changed the base branch from main to next July 10, 2023 12:58

pcmanus force-pushed the disable-validation branch from 2d543e9 to 5b39d5b Compare July 12, 2023 13:06

trevor-scheer approved these changes Jul 13, 2023

View reviewed changes

clenfest reviewed Jul 13, 2023

View reviewed changes

clenfest and others added 4 commits July 13, 2023 14:03

Update gateway-js/src/config.ts

23b211e

Naming and boolean flip

14b1804

Update .changeset/dull-lamps-invent.md

23f9d1f

Merge branch 'next' into disable-validation

387d2c5

clenfest reviewed Jul 13, 2023

View reviewed changes

trevor-scheer merged commit fe1e3d7 into apollographql:next Jul 13, 2023

github-actions bot mentioned this pull request Jul 13, 2023

release: on branch next #2597

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip some supergraph validations in production by default #2657

Skip some supergraph validations in production by default #2657

pcmanus commented Jul 5, 2023

netlify bot commented Jul 5, 2023 •

edited

Loading

changeset-bot bot commented Jul 5, 2023 •

edited

Loading

codesandbox-ci bot commented Jul 5, 2023 •

edited

Loading

pcmanus commented Jul 5, 2023

trevor-scheer left a comment

trevor-scheer Jul 13, 2023

clenfest Jul 13, 2023

clenfest Jul 13, 2023

trevor-scheer Jul 13, 2023

clenfest Jul 13, 2023

Skip some supergraph validations in production by default #2657

Skip some supergraph validations in production by default #2657

Conversation

pcmanus commented Jul 5, 2023

netlify bot commented Jul 5, 2023 • edited Loading

‼️ Deploy request for apollo-federation-docs rejected.

changeset-bot bot commented Jul 5, 2023 • edited Loading

🦋 Changeset detected

codesandbox-ci bot commented Jul 5, 2023 • edited Loading

pcmanus commented Jul 5, 2023

trevor-scheer left a comment

Choose a reason for hiding this comment

trevor-scheer Jul 13, 2023

Choose a reason for hiding this comment

clenfest Jul 13, 2023

Choose a reason for hiding this comment

clenfest Jul 13, 2023

Choose a reason for hiding this comment

trevor-scheer Jul 13, 2023

Choose a reason for hiding this comment

clenfest Jul 13, 2023

Choose a reason for hiding this comment

netlify bot commented Jul 5, 2023 •

edited

Loading

changeset-bot bot commented Jul 5, 2023 •

edited

Loading

codesandbox-ci bot commented Jul 5, 2023 •

edited

Loading