Merge pull request apollographql/apollo-server#3811 from apollographq…

…l/abernix/gateway-minor-qol-improvements

gateway: Small improvements to error messages and behavior during updates.
Apollo-Orig-Commit-AS: apollographql/apollo-server@2094947

federation-js/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.
 

gateway-js/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)

gateway-js/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);
+  });
 });

gateway-js/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);

gateway-js/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,

gateway-js/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 = {

gateway-js/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(