gateway: Small improvements to error messages and behavior during updates.

packages/apollo-gateway/src/__tests__/gateway/executor.test.ts

@@ -6,6 +6,7 @@ import * as books from '../__fixtures__/schemas/books';
 import * as inventory from '../__fixtures__/schemas/inventory';
 import * as product from '../__fixtures__/schemas/product';
 import * as reviews from '../__fixtures__/schemas/reviews';
+import { ApolloServer } from "apollo-server";
 
 describe('ApolloGateway executor', () => {
   it('validates requests prior to execution', async () => {
@@ -35,4 +36,24 @@ describe('ApolloGateway executor', () => {
       'Variable "$first" got invalid value "3"; Expected type Int.',
     );
   });
+
+  it('still sets the ApolloServer executor on load rejection', async () => {
+    jest.spyOn(console, 'error').mockImplementation();
+
+    const gateway = new ApolloGateway({
+      // Empty service list will throw, which is what we want.
+      serviceList: [],
+    });
+
+    const server = new ApolloServer({
+      gateway,
+      subscriptions: false,
+    });
+
+    // Ensure the throw happens to maintain the correctness of this test.
+    await expect(
+      server.executeOperation({ query: '{ __typename }' })).rejects.toThrow();
+
+    expect(server.requestOptions.executor).toStrictEqual(gateway.executor);
+  });
 });

packages/apollo-gateway/src/__tests__/gateway/lifecycle-hooks.test.ts

@@ -56,9 +56,7 @@ describe('lifecycle hooks', () => {
       experimental_didFailComposition,
     });
 
-    try {
-      await gateway.load();
-    } catch {}
+    await expect(gateway.load()).rejects.toThrowError();
 
     const callbackArgs = experimental_didFailComposition.mock.calls[0][0];
     expect(callbackArgs.serviceList).toHaveLength(1);

packages/apollo-gateway/src/index.ts

@@ -296,33 +296,33 @@ export class ApolloGateway implements GraphQLService {
       this.engineConfig = options.engine;
     }
 
-    const previousSchema = this.schema;
-    const previousServiceDefinitions = this.serviceDefinitions;
-    const previousCompositionMetadata = this.compositionMetadata;
-
     let result: Await<ReturnType<Experimental_UpdateServiceDefinitions>>;
-    this.logger.debug('Loading configuration for gateway');
+    this.logger.debug('Checking service definitions...');
     try {
       result = await this.updateServiceDefinitions(this.config);
     } catch (e) {
-      this.logger.warn(
-        'Error checking for schema updates. Falling back to existing schema.',
-        e,
+      this.logger.error(
+        "Error checking for changes to service definitions: " +
+         (e && e.message || e)
       );
-      return;
+      throw e;
     }
 
     if (
       !result.serviceDefinitions ||
       JSON.stringify(this.serviceDefinitions) ===
         JSON.stringify(result.serviceDefinitions)
     ) {
-      this.logger.debug('No change in service definitions since last check');
+      this.logger.debug('No change in service definitions since last check.');
       return;
     }
 
+    const previousSchema = this.schema;
+    const previousServiceDefinitions = this.serviceDefinitions;
+    const previousCompositionMetadata = this.compositionMetadata;
+
     if (previousSchema) {
-      this.logger.info('Gateway config has changed, updating schema');
+      this.logger.info("New service definitions were found.");
     }
 
     this.compositionMetadata = result.compositionMetadata;
@@ -335,9 +335,8 @@ export class ApolloGateway implements GraphQLService {
       this.onSchemaChangeListeners.forEach(listener => listener(this.schema!));
     } catch (e) {
       this.logger.error(
-        'Error notifying schema change listener of update to schema.',
-        e,
-      );
+        "An error was thrown from an 'onSchemaChange' listener. " +
+        "The schema will still update: ", e);
     }
 
     if (this.experimental_didUpdateComposition) {
@@ -415,8 +414,12 @@ export class ApolloGateway implements GraphQLService {
   private startPollingServices() {
     if (this.pollingTimer) clearInterval(this.pollingTimer);
 
-    this.pollingTimer = setInterval(() => {
-      this.updateComposition();
+    this.pollingTimer = setInterval(async () => {
+      try {
+        await this.updateComposition();
+      } catch (err) {
+        this.logger.error(err && err.message || err);
+      }
     }, this.experimental_pollInterval || 10000);
 
     // Prevent the Node.js event loop from remaining active (and preventing,

packages/apollo-gateway/src/loadServicesFromRemoteEndpoint.ts

@@ -29,7 +29,8 @@ export async function getServiceDefinitionsFromRemoteEndpoint({
   const serviceDefinitions: ServiceDefinition[] = (await Promise.all(
     serviceList.map(({ name, url, dataSource }) => {
       if (!url) {
-        throw new Error(`Tried to load schema from ${name} but no url found`);
+        throw new Error(
+          `Tried to load schema for '${name}' but no 'url' was specified.`);
       }
 
       const request: GraphQLRequest = {

packages/apollo-gateway/src/loadServicesFromStorage.ts

@@ -50,6 +50,55 @@ function getStorageSecretUrl(graphId: string, apiKeyHash: string): string {
   return `${urlStorageSecretBase}/${graphId}/storage-secret/${apiKeyHash}.json`;
 }
 
+function fetchApolloGcs(
+  fetcher: typeof fetch,
+  ...args: Parameters<typeof fetch>
+): ReturnType<typeof fetch> {
+  const [input, init] = args;
+
+  // Used in logging.
+  const url = typeof input === 'object' && input.url || input;
+
+  return fetcher(input, init)
+    .catch(fetchError => {
+      throw new Error(
+      "Cannot access Apollo Graph Manager storage: " + fetchError)
+    })
+    .then(async (response) => {
+      // If the fetcher has a cache and has implemented ETag validation, then
+      // a 304 response may be returned.  Either way, we will return the
+      // non-JSON-parsed version and let the caller decide if that's important
+      // to their needs.
+      if (response.ok || response.status === 304) {
+        return response;
+      }
+
+      // We won't make any assumptions that the body is anything but text, to
+      // avoid parsing errors in this unknown condition.
+      const body = await response.text();
+
+      // Google Cloud Storage returns an `application/xml` error under error
+      // conditions.  We'll special-case our known errors, and resort to
+      // printing the body for others.
+      if (response.headers.get('content-type') === 'application/xml') {
+        if (
+          response.status === 403 &&
+          body.includes("<Error><Code>AccessDenied</Code>") &&
+          body.includes("Anonymous caller does not have storage.objects.get")
+        ) {
+          throw new Error(
+            "Unable to authenticate with Apollo Graph Manager storage " +
+            "while fetching " + url);
+        }
+      }
+
+      // Normally, we'll try to keep the logs clean with errors we expect.
+      // If it's not a known error, reveal the full body for debugging.
+      throw new Error(
+        "Could not communicate with Apollo Graph Manager storage: " + body);
+    });
+};
+
 export async function getServiceDefinitionsFromStorage({
   graphId,
   apiKeyHash,
@@ -66,27 +115,29 @@ export async function getServiceDefinitionsFromStorage({
   // fetch the storage secret
   const storageSecretUrl = getStorageSecretUrl(graphId, apiKeyHash);
 
-  const secret: string = await fetcher(storageSecretUrl).then(response =>
-    response.json(),
-  );
+  // The storage secret is a JSON string (e.g. `"secret"`).
+  const secret: string =
+    await fetchApolloGcs(fetcher, storageSecretUrl).then(res => res.json());
 
   if (!graphVariant) {
     graphVariant = 'current';
   }
 
   const baseUrl = `${urlPartialSchemaBase}/${secret}/${graphVariant}/v${federationVersion}`;
 
-  const response = await fetcher(`${baseUrl}/composition-config-link`);
+  const compositionConfigResponse =
+    await fetchApolloGcs(fetcher, `${baseUrl}/composition-config-link`);
 
-  if (response.status === 304) {
+  if (compositionConfigResponse.status === 304) {
     return { isNewSchema: false };
   }
 
-  const linkFileResult: LinkFileResult = await response.json();
+  const linkFileResult: LinkFileResult = await compositionConfigResponse.json();
 
-  const compositionMetadata: CompositionMetadata = await fetcher(
+  const compositionMetadata: CompositionMetadata = await fetchApolloGcs(
+    fetcher,
     `${urlPartialSchemaBase}/${linkFileResult.configPath}`,
-  ).then(response => response.json());
+  ).then(res => res.json());
 
   // It's important to maintain the original order here
   const serviceDefinitions = await Promise.all(

packages/apollo-server-core/src/ApolloServer.ts

@@ -186,7 +186,6 @@ export class ApolloServerBase {
     } = config;
 
     if (gateway && (modules || schema || typeDefs || resolvers)) {
-      // TODO: this could be handled by adjusting the typings to keep gateway configs and non-gateway configs seprate.
       throw new Error(
         'Cannot define both `gateway` and any of: `modules`, `schema`, `typeDefs`, or `resolvers`',
       );
@@ -417,13 +416,27 @@ export class ApolloServerBase {
             }
           : undefined;
 
-      return gateway.load({ engine: engineConfig }).then(config => {
-        this.requestOptions.executor = config.executor;
-        return config.schema;
-      });
+      // Set the executor whether the gateway 'load' call succeeds or not.
+      // If the schema becomes available eventually (after a setInterval retry)
+      // this executor will still be necessary in order to be able to support
+      // a federated schema!
+      this.requestOptions.executor = gateway.executor;
+
+      return gateway.load({ engine: engineConfig })
+        .then(config => config.schema)
+        .catch(err => {
+          // We intentionally do not re-throw the exact error from the gateway
+          // configuration as it may contain implementation details and this
+          // error will propogate to the client. We will, however, log the error
+          // for observation in the logs.
+          const message = "This data graph lacks a valid configuration.";
+          console.error(message + " " + (err && err.message || err));
+          throw new Error(
+            message + " More details may be available in the server logs.");
+        });
     }
 
-    let constructedSchema;
+    let constructedSchema: GraphQLSchema;
     if (schema) {
       constructedSchema = schema;
     } else if (modules) {
@@ -560,7 +573,20 @@ export class ApolloServerBase {
   }
 
   protected async willStart() {
-    const { schema, schemaHash } = await this.schemaDerivedData;
+    try {
+      var { schema, schemaHash } = await this.schemaDerivedData;
+    } catch (err) {
+      // The `schemaDerivedData` can throw if the Promise it points to does not
+      // resolve with a `GraphQLSchema`. As errors from `willStart` are start-up
+      // errors, other Apollo middleware after us will not be called, including
+      // our health check, CORS, etc.
+      //
+      // Returning here allows the integration's other Apollo middleware to
+      // function properly in the event of a failure to obtain the data graph
+      // configuration from the gateway's `load` method during initialization.
+      return;
+    }
+
     const service: GraphQLServiceContext = {
       schema: schema,
       schemaHash: schemaHash,
@@ -789,14 +815,7 @@ export class ApolloServerBase {
   }
 
   public async executeOperation(request: GraphQLRequest) {
-    let options;
-
-    try {
-      options = await this.graphQLServerOptions();
-    } catch (e) {
-      e.message = `Invalid options provided to ApolloServer: ${e.message}`;
-      throw new Error(e);
-    }
+    const options = await this.graphQLServerOptions();
 
     if (typeof options.context === 'function') {
       options.context = (options.context as () => never)();

packages/apollo-server-core/src/runHttpQuery.ts

@@ -127,10 +127,6 @@ export async function runHttpQuery(
     // the normal options provided by the user, such as: formatError,
     // debug. Therefore, we need to do some unnatural things, such
     // as use NODE_ENV to determine the debug settings
-    e.message = `Invalid options provided to ApolloServer: ${e.message}`;
-    if (!debugDefault) {
-      e.warning = `To remove the stacktrace, set the NODE_ENV environment variable to production if the options creation can fail`;
-    }
     return throwHttpGraphQLError(500, [e], { debug: debugDefault });
   }
   if (options.debug === undefined) {

packages/apollo-server-core/src/types.ts

@@ -5,7 +5,13 @@ import {
   IMocks,
   GraphQLParseOptions,
 } from 'graphql-tools';
-import { ValueOrPromise, GraphQLExecutor } from 'apollo-server-types';
+import {
+  ValueOrPromise,
+  GraphQLExecutor,
+  GraphQLExecutionResult,
+  WithRequired,
+  GraphQLRequestContext,
+} from 'apollo-server-types';
 import { ConnectionContext } from 'subscriptions-transport-ws';
 // The types for `ws` use `export = WebSocket`, so we'll use the
 // matching `import =` to bring in its sole export.
@@ -87,6 +93,12 @@ export interface GraphQLService {
     engine?: GraphQLServiceEngineConfig;
   }): Promise<GraphQLServiceConfig>;
   onSchemaChange(callback: SchemaChangeCallback): Unsubscriber;
+  executor<TContext>(
+    requestContext: WithRequired<
+      GraphQLRequestContext<TContext>,
+      'document' | 'queryHash' | 'operationName' | 'operation'
+    >,
+  ): ValueOrPromise<GraphQLExecutionResult>;
 }
 
 // This configuration is shared between all integrations and should include