Merge branch 'master' into trevor/gateway-logging

packages/apollo-gateway/CHANGELOG.md

@@ -8,6 +8,7 @@
 * Gateway schema change listener bug + refactor [#3411](https://github.com/apollographql/apollo-server/pull/3411) introduces a change to the `experimental_didUpdateComposition` hook and `experimental_pollInterval` configuration behavior.
   1. Previously, the `experimental_didUpdateComposition` hook wouldn't be reliably called unless the `experimental_pollInterval` was set. If it _was_ called, it was sporadic and didn't necessarily mark the timing of an actual composition update. After this change, the hook is called on a successful composition update.
   2. The `experimental_pollInterval` configuration option now affects both the GCS polling interval when gateway is configured for managed federation, as well as the polling interval of services. The former being newly introduced behavior.
+* Gateway cached DataSource bug [#3412](https://github.com/apollographql/apollo-server/pull/3412) introduces a fix for managed federation users where `DataSource`s wouldn't update correctly if a service's url changed. This bug was introduced with heavier DataSource caching in [#3388](https://github.com/apollographql/apollo-server/pull/3388). By inspecting the `url` as well, `DataSource`s will now update correctly when a composition update occurs.
 
 # v.0.10.7
 

packages/apollo-gateway/src/index.ts

@@ -76,6 +76,10 @@ export type GatewayConfig =
   | LocalGatewayConfig
   | ManagedGatewayConfig;
 
+type DataSourceCache = {
+  [serviceName: string]: { url?: string; dataSource: GraphQLDataSource };
+};
+
 function isLocalConfig(config: GatewayConfig): config is LocalGatewayConfig {
   return 'localServiceList' in config;
 }
@@ -121,16 +125,19 @@ export type Experimental_DidUpdateCompositionCallback = (
   previousConfig?: Experimental_CompositionInfo,
 ) => void;
 
+/**
+ * **Note:** It's possible for a schema to be the same (`isNewSchema: false`) when
+ * `serviceDefinitions` have changed. For example, during type migration, the
+ * composed schema may be identical but the `serviceDefinitions` would differ
+ * since a type has moved from one service to another.
+ */
 export type Experimental_UpdateServiceDefinitions = (
   config: GatewayConfig,
-) => Promise<
-  | {
-      serviceDefinitions: ServiceDefinition[];
-      compositionMetadata?: CompositionMetadata;
-      isNewSchema: true;
-    }
-  | { isNewSchema: false }
->;
+) => Promise<{
+  serviceDefinitions?: ServiceDefinition[];
+  compositionMetadata?: CompositionMetadata;
+  isNewSchema: boolean;
+}>;
 
 type Await<T> = T extends Promise<infer U> ? U : T;
 
@@ -141,7 +148,7 @@ type RequestContext<TContext> = WithRequired<
 
 export class ApolloGateway implements GraphQLService {
   public schema?: GraphQLSchema;
-  protected serviceMap: ServiceMap = Object.create(null);
+  protected serviceMap: DataSourceCache = Object.create(null);
   protected config: GatewayConfig;
   protected logger: Logger;
   protected queryPlanStore?: InMemoryLRUCache<QueryPlan>;
@@ -281,7 +288,7 @@ export class ApolloGateway implements GraphQLService {
     }
 
     if (
-      !('serviceDefinitions' in result) ||
+      !result.serviceDefinitions ||
       JSON.stringify(this.serviceDefinitions) ===
         JSON.stringify(result.serviceDefinitions)
     ) {
@@ -397,11 +404,14 @@ export class ApolloGateway implements GraphQLService {
     serviceDef: ServiceEndpointDefinition,
   ): GraphQLDataSource {
     // If the DataSource has already been created, early return
-    if (this.serviceMap[serviceDef.name])
-      return this.serviceMap[serviceDef.name];
+    if (
+      this.serviceMap[serviceDef.name] &&
+      serviceDef.url === this.serviceMap[serviceDef.name].url
+    )
+      return this.serviceMap[serviceDef.name].dataSource;
 
     if (!serviceDef.url && !isLocalConfig(this.config)) {
-      throw new Error(
+      this.logger.error(
         `Service definition for service ${serviceDef.name} is missing a url`,
       );
     }
@@ -413,7 +423,7 @@ export class ApolloGateway implements GraphQLService {
         });
 
     // Cache the created DataSource
-    this.serviceMap[serviceDef.name] = dataSource;
+    this.serviceMap[serviceDef.name] = { url: serviceDef.url, dataSource };
 
     return dataSource;
   }
@@ -513,17 +523,25 @@ export class ApolloGateway implements GraphQLService {
       }
     }
 
+    const serviceMap: ServiceMap = Object.entries(this.serviceMap).reduce(
+      (serviceDataSources, [serviceName, { dataSource }]) => {
+        serviceDataSources[serviceName] = dataSource;
+        return serviceDataSources;
+      },
+      Object.create(null) as ServiceMap,
+    );
+
     if (this.experimental_didResolveQueryPlan) {
       this.experimental_didResolveQueryPlan({
         queryPlan,
-        serviceMap: this.serviceMap,
+        serviceMap,
         operationContext,
       });
     }
 
     const response = await executeQueryPlan<TContext>(
       queryPlan,
-      this.serviceMap,
+      serviceMap,
       requestContext,
       operationContext,
     );

packages/apollo-gateway/src/loadServicesFromRemoteEndpoint.ts

@@ -78,8 +78,5 @@ export async function getServiceDefinitionsFromRemoteEndpoint({
     serviceDefinitions.filter(Boolean),
   )) as ServiceDefinition[];
 
-  // XXX TS can't seem to infer that isNewSchema could be true
-  return (isNewSchema as true | false)
-    ? { serviceDefinitions, isNewSchema: true }
-    : { isNewSchema: false };
+  return { serviceDefinitions, isNewSchema }
 }