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

CHANGELOG.md

@@ -7,6 +7,9 @@ The version headers in this history reflect the versions of Apollo Server itself
 
 ### v2.12.0
 
+- `apollo-server-core`: When operating in gateway mode using the `gateway` property of the Apollo Server constructor options, the failure to initialize a schema during initial start-up, e.g. connectivity problems, will no longer result in the federated executor from being assigned when the schema eventually becomes available.  This precludes a state where the gateway may never become available to serve federated requests, even when failure conditions are no longer present. [PR #3811](https://github.com/apollographql/apollo-server/pull/3811)
+- `apollo-server-core`: Prevent a condition which prefixed an error message on each request when the initial gateway initialization resulted in a Promise-rejection which was memoized and re-prepended with `Invalid options provided to ApolloServer:` on each request. [PR #3811](https://github.com/apollographql/apollo-server/pull/3811)
+
 ### v2.11.0
 
 - The range of accepted `peerDepedencies` versions for `graphql` has been widened to include `graphql@^15.0.0-rc.2` so as to accommodate the latest release-candidate of the `graphql@15` package, and an intention to support it when it is finally released on the `latest` npm tag.  While this change will subdue peer dependency warnings for Apollo Server packages, many dependencies from outside of this repository will continue to raise similar warnings until those packages own `peerDependencies` are updated.  It is unlikely that all of those packages will update their ranges prior to the final version of `graphql@15` being released, but if everything is working as expected, the warnings can be safely ignored. [PR #3825](https://github.com/apollographql/apollo-server/pull/3825)

docs/package.json

@@ -7,7 +7,7 @@
   },
   "dependencies": {
     "gatsby": "2.19.23",
-    "gatsby-theme-apollo-docs": "4.0.10",
+    "gatsby-theme-apollo-docs": "4.0.13",
     "react": "16.13.0",
     "react-dom": "16.13.0"
   }

packages/apollo-federation/CHANGELOG.md

@@ -1,6 +1,10 @@
 # CHANGELOG for `@apollo/federation`
 
-## 0.13.2 (pre-release; `@next` tag)
+## 0.14.0
+
+- Only changes in the similarly versioned `@apollo/gateway` package.
+
+## 0.13.2
 
 - Only changes in the similarly versioned `@apollo/gateway` package.
 

packages/apollo-gateway/CHANGELOG.md

@@ -1,6 +1,12 @@
-# CHANGELOG for `@apollo/gateway`
+# CHANGELOG for `@apollo/gatewae`
 
-## 0.13.2 (pre-release; `@next` tag)
+## 0.14.0 (pre-release; `@next` tag)
+
+- Several previously unhandled Promise rejection errors stemming from, e.g. connectivity, failures when communicating with Apollo Graph Manager within asynchronous code are now handled. [PR #3811](https://github.com/apollographql/apollo-server/pull/3811)
+- Provide a more helpful error message when encountering expected errors. [PR #3811](https://github.com/apollographql/apollo-server/pull/3811)
+- General improvements and clarity to error messages and logging. [PR #3811](https://github.com/apollographql/apollo-server/pull/3811)
+
+## 0.13.2
 
 - __BREAKING__: The behavior and signature of `RemoteGraphQLDataSource`'s `didReceiveResponse` method has been changed.  No changes are necessary _unless_ your implementation has overridden the default behavior of this method by either extending the class and overriding the method or by providing `didReceiveResponse` as a parameter to the `RemoteGraphQLDataSource`'s constructor options.  Implementations which have provided their own `didReceiveResponse` using either of these methods should view the PR linked here for details on what has changed.  [PR #3743](https://github.com/apollographql/apollo-server/pull/3743)
 - __NEW__: Setting the `apq` option to `true` on the `RemoteGraphQLDataSource` will enable the use of [automated persisted queries (APQ)](https://www.apollographql.com/docs/apollo-server/performance/apq/) when sending queries to downstream services.  Depending on the complexity of queries sent to downstream services, this technique can greatly reduce the size of the payloads being transmitted over the network.  Downstream implementing services must also support APQ functionality to participate in this feature (Apollo Server does by default unless it has been explicitly disabled).  As with normal APQ behavior, a downstream server must have received and registered a query once before it will be able to serve an APQ request. [#3744](https://github.com/apollographql/apollo-server/pull/3744)

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).toBe(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,57 @@ 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' &&
+        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 + ".  Ensure that the API key is " +
+            "configured properly and that a federated service has been " +
+            "pushed.  For details, see " +
+            "https://go.apollo.dev/g/resolve-access-denied.");
+      }
+
+      // 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 +117,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 is missing 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,14 @@ export interface GraphQLService {
     engine?: GraphQLServiceEngineConfig;
   }): Promise<GraphQLServiceConfig>;
   onSchemaChange(callback: SchemaChangeCallback): Unsubscriber;
+  // Note: The `TContext` typing here is not conclusively behaving as we expect:
+  // https://github.com/apollographql/apollo-server/pull/3811#discussion_r387381605
+  executor<TContext>(
+    requestContext: WithRequired<
+      GraphQLRequestContext<TContext>,
+      'document' | 'queryHash' | 'operationName' | 'operation'
+    >,
+  ): ValueOrPromise<GraphQLExecutionResult>;
 }
 
 // This configuration is shared between all integrations and should include