Fail to compose when a service's federated SDL cannot be retrie… (#3867)

Previously, when attempting to compose a schema from a downstream service in
unmanaged mode, the unavailability of a service would not cause composition
to fail.

Given a condition when the remaining downstream services are still
composable (e.g. they do not depend on the unavailable service and it does
not depend on them), this could still render a valid, but unintentionally
partial schema.

While a partial schema is in many ways fine, it will cause any client's
queries against that now-missing part of the graph to suddenly become
queries which will no longer validate, despite the fact that they may have
previously been designed to fail gracefully during degradation of the service.

Rather than simply logging errors with `console.error` in those conditions,
we will now `throw` the errors.  Thanks to changes in the upstream invokers'
error handling (e.g.#3811),
this `throw`-ing will now prevent unintentionally serving an incomplete graph.

packages/apollo-gateway/CHANGELOG.md

@@ -6,6 +6,10 @@
 - 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.14.0 (pre-release; `@next` tag)
+
+- During composition, the unavailability of a downstream service in unmanaged federation mode will no longer result in a partially composed schema which merely lacks the types provided by the downed service.  This prevents unexpected validation errors for clients querying a graph which lacks types which were merely unavailable during the initial composition but were intended to be part of the graph. [PR #3867](https://github.com/apollographql/apollo-server/pull/3867)
+
 ## 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)

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

@@ -0,0 +1,35 @@
+import { getServiceDefinitionsFromRemoteEndpoint } from '../loadServicesFromRemoteEndpoint';
+import { mockLocalhostSDLQuery } from './integration/nockMocks';
+import { RemoteGraphQLDataSource } from '../datasources';
+import nock = require('nock');
+
+describe('getServiceDefinitionsFromRemoteEndpoint', () => {
+  it('errors when no URL was specified', async () => {
+    const serviceSdlCache = new Map<string, string>();
+    const dataSource = new RemoteGraphQLDataSource({ url: '' });
+    const serviceList = [{ name: 'test', dataSource }];
+    await expect(
+      getServiceDefinitionsFromRemoteEndpoint({
+        serviceList,
+        serviceSdlCache,
+      }),
+    ).rejects.toThrowError(
+      "Tried to load schema for 'test' but no 'url' was specified.",
+    );
+  });
+
+  it('throws when the downstream service returns errors', async () => {
+    const serviceSdlCache = new Map<string, string>();
+    const host = 'http://host-which-better-not-resolve';
+    const url = host + '/graphql';
+
+    const dataSource = new RemoteGraphQLDataSource({ url });
+    const serviceList = [{ name: 'test', url, dataSource }];
+    await expect(
+      getServiceDefinitionsFromRemoteEndpoint({
+        serviceList,
+        serviceSdlCache,
+      }),
+    ).rejects.toThrowError(/^Couldn't load service definitions for "test" at http:\/\/host-which-better-not-resolve\/graphql: request to http:\/\/host-which-better-not-resolve\/graphql failed, reason: getaddrinfo ENOTFOUND/);
+  });
+});

packages/apollo-gateway/src/loadServicesFromRemoteEndpoint.ts

@@ -26,58 +26,51 @@ export async function getServiceDefinitionsFromRemoteEndpoint({
 
   let isNewSchema = false;
   // for each service, fetch its introspection schema
-  const serviceDefinitions: ServiceDefinition[] = (await Promise.all(
-    serviceList.map(({ name, url, dataSource }) => {
-      if (!url) {
-        throw new Error(
-          `Tried to load schema for '${name}' but no 'url' was specified.`);
-      }
+  const promiseOfServiceList = serviceList.map(({ name, url, dataSource }) => {
+    if (!url) {
+      throw new Error(
+        `Tried to load schema for '${name}' but no 'url' was specified.`);
+    }
 
-      const request: GraphQLRequest = {
-        query: 'query GetServiceDefinition { _service { sdl } }',
-        http: {
-          url,
-          method: 'POST',
-          headers: new Headers(headers),
-        },
-      };
+    const request: GraphQLRequest = {
+      query: 'query GetServiceDefinition { _service { sdl } }',
+      http: {
+        url,
+        method: 'POST',
+        headers: new Headers(headers),
+      },
+    };
 
-      return dataSource
-        .process({ request, context: {} })
-        .then(({ data, errors }) => {
-          if (data && !errors) {
-            const typeDefs = data._service.sdl as string;
-            const previousDefinition = serviceSdlCache.get(name);
-            // this lets us know if any downstream service has changed
-            // and we need to recalculate the schema
-            if (previousDefinition !== typeDefs) {
-              isNewSchema = true;
-            }
-            serviceSdlCache.set(name, typeDefs);
-            return {
-              name,
-              url,
-              typeDefs: parse(typeDefs),
-            };
+    return dataSource
+      .process({ request, context: {} })
+      .then(({ data, errors }): ServiceDefinition => {
+        if (data && !errors) {
+          const typeDefs = data._service.sdl as string;
+          const previousDefinition = serviceSdlCache.get(name);
+          // this lets us know if any downstream service has changed
+          // and we need to recalculate the schema
+          if (previousDefinition !== typeDefs) {
+            isNewSchema = true;
           }
+          serviceSdlCache.set(name, typeDefs);
+          return {
+            name,
+            url,
+            typeDefs: parse(typeDefs),
+          };
+        }
 
-          // XXX handle local errors better for local development
-          if (errors) {
-            errors.forEach(console.error);
-          }
+        throw new Error(errors?.map(e => e.message).join("\n"));
+      })
+      .catch(err => {
+        const errorMessage =
+          `Couldn't load service definitions for "${name}" at ${url}` +
+          (err && err.message ? ": " + err.message || err : "");
 
-          return false;
-        })
-        .catch(error => {
-          console.warn(
-            `Encountered error when loading ${name} at ${url}: ${error.message}`,
-          );
-          return false;
-        });
-    }),
-  ).then(serviceDefinitions =>
-    serviceDefinitions.filter(Boolean),
-  )) as ServiceDefinition[];
+        throw new Error(errorMessage);
+      });
+  });
 
+  const serviceDefinitions = await Promise.all(promiseOfServiceList);
   return { serviceDefinitions, isNewSchema }
 }