Skip to content

Commit

Permalink
Allows passing the list of supported features when building a supergr…
Browse files Browse the repository at this point in the history
…aph (#2656)

When we parse/build a supergraph, one validation run checks that any
feature in the supergraph marked for `EXECUTION` or `SECURITY` is
actually supported. But the list of what was supported was currently
completely hard-coded, and that means for instance that external
use of the query planner (like in the Apollo router typically) could
not modify this list of supported features. This commit allows that
list to be provided (and default to the hard-coded one if not).

Now, passing that list down to the validation that use it was not
very convenient in the existing code. Taking the query planner as
an example, it was taking the supergraph as argument as a `Schema`
but the validation was then done somewhere within the code
of the planner. Passing the list of supported options, so it can
then be passed down to the relevant code, was technically possible,
but it would have been a bit ugly, because 1) the query planner
is one example, but supergraphs could be use elsewhere and 2) that
list of supported feature isn't really a property of the planner
(which mostly doesn't care), so passing it as a planner option is
not too clean.

So most of this commit is really a small refactor to introduce
a `Supergraph` class, to essentially warp a schema that is known
to be a supergraph schema. Places that expect a supergraph as argument
now use `Supergraph` instead of just `Schema`. I'd argue that this
refactor make sense regardless of the final goal of this issue for
at least 2 reasons:
1. this provided nice symmetry with the `Subgraph` class we have had for
   a long time for very similar reasons.
2. it's a better use of the type system. Now that the query planner
  take a `Supergraph` instance, it's extra clear what the argument is,
  and if multiple methods take that instance, they know for a fact that
  the underlying schema is a valid supergraph and they don't have to
  defensively check it again.
  • Loading branch information
Sylvain Lebresne authored Jul 10, 2023
1 parent 2b5796a commit db90ba1
Show file tree
Hide file tree
Showing 17 changed files with 143 additions and 111 deletions.
8 changes: 4 additions & 4 deletions composition-js/src/__tests__/composeFed1Subgraphs.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { buildSchema, extractSubgraphsFromSupergraph, FEDERATION2_LINK_WITH_FULL_IMPORTS, ObjectType, printSchema, Schema, SubgraphASTNode, Subgraphs } from '@apollo/federation-internals';
import { FEDERATION2_LINK_WITH_FULL_IMPORTS, ObjectType, printSchema, Schema, SubgraphASTNode, Subgraphs, Supergraph } from '@apollo/federation-internals';
import { CompositionResult, composeServices, CompositionSuccess } from '../compose';
import gql from 'graphql-tag';
import './matchers';
Expand All @@ -16,9 +16,9 @@ function errors(r: CompositionResult): [string, string][] {
// Returns [the supergraph schema, its api schema, the extracted subgraphs]
function schemas(result: CompositionSuccess): [Schema, Schema, Subgraphs] {
// Note that we could user `result.schema`, but re-parsing to ensure we don't lose anything with printing/parsing.
const schema = buildSchema(result.supergraphSdl);
expect(schema.isCoreSchema()).toBeTruthy();
return [schema, schema.toAPISchema(), extractSubgraphsFromSupergraph(schema)];
const supergraph = Supergraph.build(result.supergraphSdl);
expect(supergraph.schema.isCoreSchema()).toBeTruthy();
return [supergraph.schema, supergraph.apiSchema(), supergraph.subgraphs()];
}

describe('basic type extensions', () => {
Expand Down
4 changes: 2 additions & 2 deletions composition-js/src/__tests__/supergraph_reversibility.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { assertCompositionSuccess, composeAsFed2Subgraphs } from "./testHelper";
import gql from 'graphql-tag';
import { asFed2SubgraphDocument, buildSubgraph, buildSupergraphSchema, extractSubgraphsFromSupergraph, ServiceDefinition } from "@apollo/federation-internals";
import { asFed2SubgraphDocument, buildSubgraph, ServiceDefinition, Supergraph } from "@apollo/federation-internals";
import './matchers';

function composeAndTestReversibility(subgraphs: ServiceDefinition[]) {
const result = composeAsFed2Subgraphs(subgraphs);
assertCompositionSuccess(result);

const extracted = extractSubgraphsFromSupergraph(buildSupergraphSchema(result.supergraphSdl)[0]);
const extracted = Supergraph.build(result.supergraphSdl).subgraphs();
for (const expectedSubgraph of subgraphs) {
const actual = extracted.get(expectedSubgraph.name)!;
// Note: the subgraph extracted from the supergraph are created with their `@link` on the schema definition, not as an extension (no
Expand Down
11 changes: 5 additions & 6 deletions composition-js/src/__tests__/testHelper.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import {
asFed2SubgraphDocument,
buildSchema,
extractSubgraphsFromSupergraph,
Schema,
ServiceDefinition,
Subgraphs
Subgraphs,
Supergraph
} from '@apollo/federation-internals';
import { CompositionResult, composeServices, CompositionSuccess, CompositionOptions } from '../compose';

Expand All @@ -21,9 +20,9 @@ export function errors(r: CompositionResult): [string, string][] {
// Returns [the supergraph schema, its api schema, the extracted subgraphs]
export function schemas(result: CompositionSuccess): [Schema, Schema, Subgraphs] {
// Note that we could user `result.schema`, but reparsing to ensure we don't lose anything with printing/parsing.
const schema = buildSchema(result.supergraphSdl);
expect(schema.isCoreSchema()).toBeTruthy();
return [schema, schema.toAPISchema(), extractSubgraphsFromSupergraph(schema)];
const supergraph = Supergraph.build(result.supergraphSdl);
expect(supergraph.schema.isCoreSchema()).toBeTruthy();
return [supergraph.schema, supergraph.apiSchema(), supergraph.subgraphs()];
}

// Note that tests for composition involving fed1 subgraph are in `composeFed1Subgraphs.test.ts` so all the test of this
Expand Down
13 changes: 7 additions & 6 deletions composition-js/src/compose.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
upgradeSubgraphsIfNecessary,
SubtypingRule,
assert,
Supergraph,
} from "@apollo/federation-internals";
import { GraphQLError } from "graphql";
import { buildFederatedQueryGraph, buildSupergraphAPIQueryGraph } from "@apollo/query-graphs";
Expand Down Expand Up @@ -65,10 +66,10 @@ export function compose(subgraphs: Subgraphs, options: CompositionOptions = {}):
return { errors: mergeResult.errors };
}

const supergraphSchema = mergeResult.supergraph;
const supergraphQueryGraph = buildSupergraphAPIQueryGraph(supergraphSchema);
const federatedQueryGraph = buildFederatedQueryGraph(supergraphSchema, false);
const { errors, hints } = validateGraphComposition(supergraphSchema, supergraphQueryGraph, federatedQueryGraph);
const supergraph = new Supergraph(mergeResult.supergraph);
const supergraphQueryGraph = buildSupergraphAPIQueryGraph(supergraph);
const federatedQueryGraph = buildFederatedQueryGraph(supergraph, false);
const { errors, hints } = validateGraphComposition(supergraph.schema, supergraphQueryGraph, federatedQueryGraph);
if (errors) {
return { errors };
}
Expand All @@ -77,15 +78,15 @@ export function compose(subgraphs: Subgraphs, options: CompositionOptions = {}):
let supergraphSdl;
try {
supergraphSdl = printSchema(
supergraphSchema,
supergraph.schema,
options.sdlPrintOptions ?? shallowOrderPrintedDefinitions(defaultPrintOptions),
);
} catch (err) {
return { errors: [err] };
}

return {
schema: supergraphSchema,
schema: supergraph.schema,
supergraphSdl,
hints: mergeResult.hints.concat(hints ?? []),
};
Expand Down
17 changes: 10 additions & 7 deletions gateway-js/src/__tests__/executeQueryPlan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { QueryPlan, QueryPlanner } from '@apollo/query-planner';
import { ApolloGateway } from '..';
import { ApolloServer } from '@apollo/server';
import { getFederatedTestingSchema } from './execution-utils';
import { Schema, Operation, parseOperation, buildSchemaFromAST, arrayEquals } from '@apollo/federation-internals';
import { Schema, Operation, parseOperation, arrayEquals, Supergraph } from '@apollo/federation-internals';
import {
addResolversToSchema,
GraphQLResolverMap,
Expand Down Expand Up @@ -1270,10 +1270,11 @@ describe('executeQueryPlan', () => {

describe('@inaccessible', () => {
it(`should not include @inaccessible fields in introspection`, async () => {
schema = buildSchemaFromAST(superGraphWithInaccessible);
const supergraph = Supergraph.build(superGraphWithInaccessible);
schema = supergraph.schema;

const operation = parseOp(`${getIntrospectionQuery()}`, schema);
queryPlanner = new QueryPlanner(schema);
queryPlanner = new QueryPlanner(supergraph);
const queryPlan = queryPlanner.buildQueryPlan(operation);
const response = await executePlan(queryPlan, operation, undefined, schema);

Expand Down Expand Up @@ -1303,9 +1304,10 @@ describe('executeQueryPlan', () => {

const operation = parseOp(operationString);

schema = buildSchemaFromAST(superGraphWithInaccessible);
const supergraph = Supergraph.build(superGraphWithInaccessible);
schema = supergraph.schema;

queryPlanner = new QueryPlanner(schema);
queryPlanner = new QueryPlanner(supergraph);
const queryPlan = queryPlanner.buildQueryPlan(operation);

const response = await executePlan(queryPlan, operation, undefined, schema);
Expand Down Expand Up @@ -1382,9 +1384,10 @@ describe('executeQueryPlan', () => {

// Vehicle ID #1 is a "Car" type.
// This supergraph marks the "Car" type as inaccessible.
schema = buildSchemaFromAST(superGraphWithInaccessible);
const supergraph = Supergraph.build(superGraphWithInaccessible);
schema = supergraph.schema;

queryPlanner = new QueryPlanner(schema);
queryPlanner = new QueryPlanner(supergraph);
const queryPlan = queryPlanner.buildQueryPlan(operation);

const response = await executePlan(queryPlan, operation, undefined, schema);
Expand Down
4 changes: 2 additions & 2 deletions gateway-js/src/__tests__/execution-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { queryPlanSerializer, astSerializer } from 'apollo-federation-integratio
import gql from 'graphql-tag';
import { fixtures } from 'apollo-federation-integration-testsuite';
import { composeServices } from '@apollo/composition';
import { buildSchema, Operation, operationFromDocument, ServiceDefinition } from '@apollo/federation-internals';
import { buildSchema, Operation, operationFromDocument, ServiceDefinition, Supergraph } from '@apollo/federation-internals';
import { GatewayExecutionResult, GatewayGraphQLRequest } from '@apollo/server-gateway-interface';

const prettyFormat = require('pretty-format');
Expand Down Expand Up @@ -93,7 +93,7 @@ export function getFederatedTestingSchema(services: ServiceDefinitionModule[] =
throw new GraphQLSchemaValidationError(compositionResult.errors);
}

const queryPlanner = new QueryPlanner(compositionResult.schema);
const queryPlanner = new QueryPlanner(new Supergraph(compositionResult.schema));
const schema = buildSchema(compositionResult.supergraphSdl);

const serviceMap = Object.fromEntries(
Expand Down
30 changes: 15 additions & 15 deletions gateway-js/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ import {
LocalCompose,
} from './supergraphManagers';
import {
buildSupergraphSchema,
operationFromDocument,
Schema,
ServiceDefinition,
Supergraph,
} from '@apollo/federation-internals';
import { getDefaultLogger } from './logger';
import {GatewayInterface, GatewayUnsubscriber, GatewayGraphQLRequestContext, GatewayExecutionResult} from '@apollo/server-gateway-interface';
Expand Down Expand Up @@ -507,7 +507,7 @@ export class ApolloGateway implements GatewayInterface {
// In the case that it throws, the gateway will:
// * on initial load, throw the error
// * on update, log the error and don't update
const { supergraphSchema, supergraphSdl } = this.createSchemaFromSupergraphSdl(
const { supergraph, supergraphSdl } = this.createSchemaFromSupergraphSdl(
result.supergraphSdl,
);

Expand All @@ -521,14 +521,14 @@ export class ApolloGateway implements GatewayInterface {

this.compositionId = result.id;
this.supergraphSdl = supergraphSdl;
this.supergraphSchema = supergraphSchema.toGraphQLJSSchema();
this.supergraphSchema = supergraph.schema.toGraphQLJSSchema();

if (!supergraphSdl) {
this.logger.error(
"A valid schema couldn't be composed. Falling back to previous schema.",
);
} else {
this.updateWithSchemaAndNotify(supergraphSchema, supergraphSdl);
this.updateWithSchemaAndNotify(supergraph, supergraphSdl);

if (this.experimental_didUpdateSupergraph) {
this.experimental_didUpdateSupergraph(
Expand All @@ -553,15 +553,15 @@ export class ApolloGateway implements GatewayInterface {
// ensure we do not forget to update some of that state, and to avoid scenarios where
// concurrently executing code sees partially-updated state.
private updateWithSchemaAndNotify(
coreSchema: Schema,
coreSupergraphSdl: string,
supergraph: Supergraph,
supergraphSdl: string,
// Once we remove the deprecated onSchemaChange() method, we can remove this.
legacyDontNotifyOnSchemaChangeListeners: boolean = false,
): void {
this.queryPlanStore.clear();
this.apiSchema = coreSchema.toAPISchema();
this.apiSchema = supergraph.apiSchema();
this.schema = addExtensions(this.apiSchema.toGraphQLJSSchema());
this.queryPlanner = new QueryPlanner(coreSchema, this.config.queryPlannerConfig);
this.queryPlanner = new QueryPlanner(supergraph, this.config.queryPlannerConfig);

// Notify onSchemaChange listeners of the updated schema
if (!legacyDontNotifyOnSchemaChangeListeners) {
Expand All @@ -583,7 +583,7 @@ export class ApolloGateway implements GatewayInterface {
try {
listener({
apiSchema: this.schema!,
coreSupergraphSdl,
coreSupergraphSdl: supergraphSdl,
});
} catch (e) {
this.logger.error(
Expand Down Expand Up @@ -629,16 +629,16 @@ export class ApolloGateway implements GatewayInterface {

private serviceListFromSupergraphSdl(
supergraphSdl: string,
): Omit<ServiceDefinition, 'typeDefs'>[] {
return buildSupergraphSchema(supergraphSdl)[1];
): readonly Omit<ServiceDefinition, 'typeDefs'>[] {
return Supergraph.build(supergraphSdl).subgraphsMetadata();
}

private createSchemaFromSupergraphSdl(supergraphSdl: string) {
const [schema, serviceList] = buildSupergraphSchema(supergraphSdl);
this.createServices(serviceList);
const supergraph = Supergraph.build(supergraphSdl);
this.createServices(supergraph.subgraphsMetadata());

return {
supergraphSchema: schema,
supergraph,
supergraphSdl,
};
}
Expand Down Expand Up @@ -704,7 +704,7 @@ export class ApolloGateway implements GatewayInterface {
});
}

private createServices(services: ServiceEndpointDefinition[]) {
private createServices(services: readonly ServiceEndpointDefinition[]) {
for (const serviceDef of services) {
this.getOrCreateDataSource(serviceDef);
}
Expand Down
26 changes: 9 additions & 17 deletions internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { buildSupergraphSchema, extractSubgraphsFromSupergraph, InputObjectType } from "..";
import { Supergraph, InputObjectType } from "..";


test('handles types having no fields referenced by other objects in a subgraph correctly', () => {
Expand Down Expand Up @@ -95,8 +95,7 @@ test('handles types having no fields referenced by other objects in a subgraph c
}
`;

const schema = buildSupergraphSchema(supergraph)[0];
const subgraphs = extractSubgraphsFromSupergraph(schema);
const subgraphs = Supergraph.build(supergraph).subgraphs();
expect(subgraphs.size()).toBe(3);

const [a, b, c] = subgraphs.values().map((s) => s.schema);
Expand Down Expand Up @@ -202,8 +201,7 @@ test('handles types having no fields referenced by other interfaces in a subgrap
}
`;

const schema = buildSupergraphSchema(supergraph)[0];
const subgraphs = extractSubgraphsFromSupergraph(schema);
const subgraphs = Supergraph.build(supergraph).subgraphs();
expect(subgraphs.size()).toBe(3);

const [a, b, c] = subgraphs.values().map((s) => s.schema);
Expand Down Expand Up @@ -303,8 +301,7 @@ test('handles types having no fields referenced by other unions in a subgraph co
}
`;

const schema = buildSupergraphSchema(supergraph)[0];
const subgraphs = extractSubgraphsFromSupergraph(schema);
const subgraphs = Supergraph.build(supergraph).subgraphs();
expect(subgraphs.size()).toBe(2);

const [a, b] = subgraphs.values().map((s) => s.schema);
Expand Down Expand Up @@ -412,8 +409,7 @@ test('handles types having only some of their fields removed in a subgraph corre
}
`;

const schema = buildSupergraphSchema(supergraph)[0];
const subgraphs = extractSubgraphsFromSupergraph(schema);
const subgraphs = Supergraph.build(supergraph).subgraphs();
expect(subgraphs.size()).toBe(3);

const [a, b, c] = subgraphs.values().map((s) => s.schema);
Expand Down Expand Up @@ -518,8 +514,7 @@ test('handles unions types having no members in a subgraph correctly', () => {
}
`;

const schema = buildSupergraphSchema(supergraph)[0];
const subgraphs = extractSubgraphsFromSupergraph(schema);
const subgraphs = Supergraph.build(supergraph).subgraphs();
expect(subgraphs.size()).toBe(2);

const [a, b] = subgraphs.values().map((s) => s.schema);
Expand Down Expand Up @@ -586,8 +581,7 @@ test('preserves default values of input object fields', () => {
}
`;

const schema = buildSupergraphSchema(supergraph)[0];
const subgraphs = extractSubgraphsFromSupergraph(schema);
const subgraphs = Supergraph.build(supergraph).subgraphs();

const subgraph = subgraphs.get('service')
const inputType = subgraph?.schema.type('Input') as InputObjectType | undefined
Expand Down Expand Up @@ -650,8 +644,7 @@ test('throw meaningful error for invalid federation directive fieldSet', () => {
}
`;

const schema = buildSupergraphSchema(supergraph)[0];
expect(() => extractSubgraphsFromSupergraph(schema)).toThrow(
expect(() => Supergraph.build(supergraph).subgraphs()).toThrow(
'Error extracting subgraph "serviceB" from the supergraph: this might be due to errors in subgraphs that were mistakenly ignored by federation 0.x versions but are rejected by federation 2.\n'
+ 'Please try composing your subgraphs with federation 2: this should help precisely pinpoint the problems and, once fixed, generate a correct federation 2 supergraph.\n'
+ '\n'
Expand Down Expand Up @@ -737,8 +730,7 @@ test('throw meaningful error for type erased from supergraph due to extending an
}
`;

const schema = buildSupergraphSchema(supergraph)[0];
expect(() => extractSubgraphsFromSupergraph(schema)).toThrow(
expect(() => Supergraph.build(supergraph).subgraphs()).toThrow(
'Error extracting subgraphs from the supergraph: this might be due to errors in subgraphs that were mistakenly ignored by federation 0.x versions but are rejected by federation 2.\n'
+ 'Please try composing your subgraphs with federation 2: this should help precisely pinpoint the problems and, once fixed, generate a correct federation 2 supergraph.\n'
+ '\n'
Expand Down
1 change: 0 additions & 1 deletion internals-js/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ export * from './tagSpec';
export * from './inaccessibleSpec';
export * from './federationSpec';
export * from './supergraphs';
export * from './extractSubgraphsFromSupergraph';
export * from './error';
export * from './schemaUpgrader';
export * from './suggestions';
Expand Down
Loading

0 comments on commit db90ba1

Please sign in to comment.