Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(gateway): Simplify startup code paths #440

Merged
merged 19 commits into from
Jan 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions gateway-js/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.

- Simplify startup code paths. This is technically only intended to be an internal restructure, but it's substantial enough to warrant a changelog entry for observability in case of any unexpected behavioral changes. [PR #440](https://github.com/apollographql/federation/pull/440)

## v0.22.0

- Include original error during creation of `GraphQLError` in `downstreamServiceError()`. [PR #309](https://github.com/apollographql/federation/pull/309)
Expand Down
17 changes: 7 additions & 10 deletions gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import gql from 'graphql-tag';
import { ApolloGateway } from '../..';
import {
ApolloGateway,
GatewayConfig,
Experimental_DidResolveQueryPlanCallback,
Experimental_UpdateServiceDefinitions,
} from '../../index';
} from '../../config';
import {
product,
reviews,
Expand Down Expand Up @@ -49,7 +48,7 @@ beforeEach(() => {
describe('lifecycle hooks', () => {
it('uses updateServiceDefinitions override', async () => {
const experimental_updateServiceDefinitions: Experimental_UpdateServiceDefinitions = jest.fn(
async (_config: GatewayConfig) => {
async () => {
return { serviceDefinitions, isNewSchema: true };
},
);
Expand All @@ -71,7 +70,7 @@ describe('lifecycle hooks', () => {
const experimental_didFailComposition = jest.fn();

const gateway = new ApolloGateway({
async experimental_updateServiceDefinitions(_config: GatewayConfig) {
async experimental_updateServiceDefinitions() {
return {
serviceDefinitions: [serviceDefinitions[0]],
compositionMetadata: {
Expand Down Expand Up @@ -107,9 +106,7 @@ describe('lifecycle hooks', () => {
schemaHash: 'hash1',
};

const update: Experimental_UpdateServiceDefinitions = async (
_config: GatewayConfig,
) => ({
const update: Experimental_UpdateServiceDefinitions = async () => ({
serviceDefinitions,
isNewSchema: true,
compositionMetadata: {
Expand All @@ -124,7 +121,7 @@ describe('lifecycle hooks', () => {

// We want to return a different composition across two ticks, so we mock it
// slightly differenty
mockUpdate.mockImplementationOnce(async (_config: GatewayConfig) => {
mockUpdate.mockImplementationOnce(async () => {
const services = serviceDefinitions.filter(s => s.name !== 'books');
return {
serviceDefinitions: [
Expand Down Expand Up @@ -215,7 +212,7 @@ describe('lifecycle hooks', () => {

it('registers schema change callbacks when experimental_pollInterval is set for unmanaged configs', async () => {
const experimental_updateServiceDefinitions: Experimental_UpdateServiceDefinitions = jest.fn(
async (_config: GatewayConfig) => {
async (_config) => {
return { serviceDefinitions, isNewSchema: true };
},
);
Expand Down
169 changes: 169 additions & 0 deletions gateway-js/src/__tests__/integration/configuration.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
import { Logger } from 'apollo-server-types';
import { ApolloGateway } from '../..';
import {
mockSDLQuerySuccess,
mockStorageSecretSuccess,
mockCompositionConfigLinkSuccess,
mockCompositionConfigsSuccess,
mockImplementingServicesSuccess,
mockRawPartialSchemaSuccess,
apiKeyHash,
graphId,
} from './nockMocks';
import { getTestingCsdl } from '../execution-utils';
import { MockService } from './networkRequests.test';
import { parse } from 'graphql';

let logger: Logger;

const service: MockService = {
gcsDefinitionPath: 'service-definition.json',
partialSchemaPath: 'accounts-partial-schema.json',
url: 'http://localhost:4001',
sdl: `#graphql
extend type Query {
me: User
everyone: [User]
}

"This is my User"
type User @key(fields: "id") {
id: ID!
name: String
username: String
}
`,
};

beforeEach(() => {
const warn = jest.fn();
const debug = jest.fn();
const error = jest.fn();
const info = jest.fn();

logger = {
warn,
debug,
error,
info,
};
});

describe('gateway configuration warnings', () => {
it('warns when both csdl and studio configuration are provided', async () => {
const gateway = new ApolloGateway({
csdl: getTestingCsdl(),
logger,
});

await gateway.load({
apollo: { keyHash: apiKeyHash, graphId, graphVariant: 'current' },
});

expect(logger.warn).toHaveBeenCalledWith(
'A local gateway configuration is overriding a managed federation configuration.' +
' To use the managed configuration, do not specify a service list or csdl locally.',
);
});

it('conflicting configurations are warned about when present', async () => {
mockSDLQuerySuccess(service);

const gateway = new ApolloGateway({
serviceList: [{ name: 'accounts', url: service.url }],
logger,
});

await gateway.load({
apollo: { keyHash: apiKeyHash, graphId, graphVariant: 'current' },
});

expect(logger.warn).toHaveBeenCalledWith(
expect.stringMatching(
/A local gateway configuration is overriding a managed federation configuration/,
),
);
});

it('conflicting configurations are not warned about when absent', async () => {
mockStorageSecretSuccess();
mockCompositionConfigLinkSuccess();
mockCompositionConfigsSuccess([service]);
mockImplementingServicesSuccess(service);
mockRawPartialSchemaSuccess(service);

const gateway = new ApolloGateway({
logger,
});

await gateway.load({
apollo: { keyHash: apiKeyHash, graphId, graphVariant: 'current' },
});

expect(logger.warn).not.toHaveBeenCalledWith(
expect.stringMatching(
/A local gateway configuration is overriding a managed federation configuration/,
),
);
});

it('throws when no configuration is provided', async () => {
const gateway = new ApolloGateway({
logger,
});

expect(gateway.load()).rejects.toThrowErrorMatchingInlineSnapshot(
`"When a manual configuration is not provided, gateway requires an Apollo configuration. See https://www.apollographql.com/docs/apollo-server/federation/managed-federation/ for more information. Manual configuration options include: \`serviceList\`, \`csdl\`, and \`experimental_updateServiceDefinitions\`."`,
);
});
});

describe('gateway startup errors', () => {
it("throws when static config can't be composed", async () => {
const uncomposableSdl = parse(`#graphql
type Query {
me: User
everyone: [User]
account(id: String): Account
}

type User @key(fields: "id") {
name: String
username: String
}

type Account @key(fields: "id") {
name: String
username: String
}
`);

const gateway = new ApolloGateway({
localServiceList: [
{ name: 'accounts', url: service.url, typeDefs: uncomposableSdl },
],
logger,
});

// This is the ideal, but our version of Jest has a bug with printing error snapshots.
// See: https://github.com/facebook/jest/pull/10217 (fixed in v26.2.0)
// expect(gateway.load()).rejects.toThrowErrorMatchingInlineSnapshot(`
// "A valid schema couldn't be composed. The following composition errors were found:
// [accounts] User -> A @key selects id, but User.id could not be found
// [accounts] Account -> A @key selects id, but Account.id could not be found"
// `);
// Instead we'll just use the regular snapshot matcher...
let err: any;
try {
await gateway.load();
} catch (e) {
err = e;
}

expect(err.message).toMatchInlineSnapshot(`
"A valid schema couldn't be composed. The following composition errors were found:
[accounts] User -> A @key selects id, but User.id could not be found
[accounts] Account -> A @key selects id, but Account.id could not be found"
`);
});
});
104 changes: 2 additions & 102 deletions gateway-js/src/__tests__/integration/networkRequests.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,6 @@ import {
apiKeyHash,
graphId,
} from './nockMocks';
import loadServicesFromStorage = require('../../loadServicesFromStorage');
import { getTestingCsdl } from '../execution-utils';

// This is a nice DX hack for GraphQL code highlighting and formatting within the file.
// Anything wrapped within the gql tag within this file is just a string, not an AST.
const gql = String.raw;

export interface MockService {
gcsDefinitionPath: string;
Expand All @@ -37,7 +31,7 @@ const service: MockService = {
gcsDefinitionPath: 'service-definition.json',
partialSchemaPath: 'accounts-partial-schema.json',
url: 'http://localhost:4001',
sdl: gql`
sdl: `#graphql
extend type Query {
me: User
everyone: [User]
Expand All @@ -56,7 +50,7 @@ const updatedService: MockService = {
gcsDefinitionPath: 'updated-service-definition.json',
partialSchemaPath: 'updated-accounts-partial-schema.json',
url: 'http://localhost:4002',
sdl: gql`
sdl: `#graphql
extend type Query {
me: User
everyone: [User]
Expand Down Expand Up @@ -130,100 +124,6 @@ it('Extracts service definitions from remote storage', async () => {
expect(gateway.schema!.getType('User')!.description).toBe('This is my User');
});

it.each([
['warned', 'present'],
['not warned', 'absent'],
])('conflicting configurations are %s about when %s', async (_word, mode) => {
const isConflict = mode === 'present';
let blockerResolve: () => void;
const blocker = new Promise((resolve) => (blockerResolve = resolve));
const original = loadServicesFromStorage.getServiceDefinitionsFromStorage;
const spyGetServiceDefinitionsFromStorage = jest
.spyOn(loadServicesFromStorage, 'getServiceDefinitionsFromStorage')
.mockImplementationOnce(async (...args) => {
try {
return await original(...args);
} catch (e) {
throw e;
} finally {
setImmediate(blockerResolve);
}
});

mockStorageSecretSuccess();
if (isConflict) {
mockCompositionConfigLinkSuccess();
mockCompositionConfigsSuccess([service]);
mockImplementingServicesSuccess(service);
mockRawPartialSchemaSuccess(service);
} else {
mockCompositionConfigLink().reply(403);
}

mockSDLQuerySuccess(service);

const gateway = new ApolloGateway({
serviceList: [{ name: 'accounts', url: service.url }],
logger,
});

await gateway.load({
apollo: { keyHash: apiKeyHash, graphId, graphVariant: 'current' },
});
await blocker; // Wait for the definitions to be "fetched".

(isConflict
? expect(logger.warn)
: expect(logger.warn).not
).toHaveBeenCalledWith(
expect.stringMatching(
/A local gateway configuration is overriding a managed federation configuration/,
),
);
spyGetServiceDefinitionsFromStorage.mockRestore();
});

it('warns when both csdl and studio configuration are provided', async () => {
mockStorageSecretSuccess();
mockCompositionConfigLinkSuccess();
mockCompositionConfigsSuccess([service]);
mockImplementingServicesSuccess(service);
mockRawPartialSchemaSuccess(service);

let blockerResolve: () => void;
const blocker = new Promise((resolve) => (blockerResolve = resolve));
const original = loadServicesFromStorage.getServiceDefinitionsFromStorage;
const spyGetServiceDefinitionsFromStorage = jest
.spyOn(loadServicesFromStorage, 'getServiceDefinitionsFromStorage')
.mockImplementationOnce(async (...args) => {
try {
return await original(...args);
} catch (e) {
throw e;
} finally {
setImmediate(blockerResolve);
}
});

const gateway = new ApolloGateway({
csdl: getTestingCsdl(),
logger,
});

await gateway.load({
apollo: { keyHash: apiKeyHash, graphId, graphVariant: 'current' },
});

await blocker;

expect(logger.warn).toHaveBeenCalledWith(
'A local gateway configuration is overriding a managed federation configuration.' +
' To use the managed configuration, do not specify a service list or csdl locally.',
);

spyGetServiceDefinitionsFromStorage.mockRestore();
});

// This test has been flaky for a long time, and fails consistently after changes
// introduced by https://github.com/apollographql/apollo-server/pull/4277.
// I've decided to skip this test for now with hopes that we can one day
Expand Down
Loading