refactor: simplify InclusionResolver to return an array of targets

Move the logic assigning targets to source properties into Repository code. This makes it easier to use InclusionResolvers to implement GraphQL resolvers. Signed-off-by: Miroslav Bajtoš <[email protected]>
loopbackio · Jul 19, 2019 · 2f69523 · 2f69523
1 parent a7260ff
commit 2f69523
Show file tree

Hide file tree

Showing 7 changed files with 88 additions and 80 deletions.
diff --git a/_SPIKE_.md b/_SPIKE_.md
@@ -38,7 +38,11 @@ resolver for each relation type.
 
 ```ts
 /**
- * @returns Promise of void. The source models are updated in-place.
+ * @returns An array of resolved values, the items must be ordered in the same
+ * way as `sourceEntities`. The resolved value can be one of:
+ * - `undefined` when no target model(s) were found
+ * - `Entity` for relations targeting a single model
+ * - `Entity[]` for relations targeting multiple models
  */
 export type InclusionResolver = (
   /**
@@ -53,11 +57,21 @@ export type InclusionResolver = (
    * Generic options object, e.g. carrying the Transaction object.
    */
   options?: Options,
-) => Promise<void>;
+) => Promise<(Entity | undefined)[] | (Entity[] | undefined)[]>;
 ```
 
-With a bit of luck, we will be able to use these resolvers for GraphQL too.
-However, such use-case is out of scope of this spike.
+This signature is loosely aligned with GraphQL resolvers as described e.g. in
+[Apollo docs](https://www.apollographql.com/docs/graphql-tools/resolvers/).
+While it should be possible to use these resolvers to directly implement GraphQL
+resolvers, such implementation would be prone to
+[`SELECT N+1` problem](https://stackoverflow.com/q/97197/69868). I did a quick
+search and it looks like the recommended solution is to leverage
+[DataLoader](https://github.com/graphql/dataloader/) to batch multiple queries
+into a single one. DataLoader's example showing integration with GraphQL is not
+trivial: https://github.com/graphql/dataloader#using-with-graphql.
+
+As I see it, implementing inclusion resolvers for GraphQL requires further
+research that's out of scope of this spike.
 
 ### 2. Base repository classes handle inclusions via resolvers
 
@@ -109,11 +123,17 @@ export class DefaultCrudRepository<T, Relations> {
     const result = entities as (T & Relations)[];
 
     // process relations in parallel
-    const resolveTasks = filter.include.map(i => {
+    const resolveTasks = filter.include.map(async i => {
       const relationName = i.relation;
       const resolver = this.inclusionResolvers.get(relationName);
-      return resolve(entities, i, options);
+      const targets = await resolver(entities, i, options);
+
+      for (const ix in result) {
+        const src = result[ix];
+        (src as AnyObject)[relationName] = targets[ix];
+      }
     });
+
     await Promise.all(resolveTasks);
 
     return result;

diff --git a/packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts b/packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts
@@ -8,8 +8,8 @@ import {Entity} from '../../model';
 import {Filter, Inclusion} from '../../query';
 import {EntityCrudRepository} from '../../repositories/repository';
 import {
-  assignTargetsOfOneToOneRelation,
   findByForeignKeys,
+  flattenTargetsOfOneToOneRelation,
   StringKeyOf,
   uniq,
 } from '../relation.helpers';
@@ -36,8 +36,8 @@ export function createBelongsToInclusionResolver<
     entities: Entity[],
     inclusion: Inclusion,
     options?: Options,
-  ): Promise<void> {
-    if (!entities.length) return;
+  ): Promise<((Target & TargetRelations) | undefined)[]> {
+    if (!entities.length) return [];
 
     const sourceKey = relationMeta.keyFrom;
     const sourceIds = entities.map(e => (e as AnyObject)[sourceKey]);
@@ -52,14 +52,6 @@ export function createBelongsToInclusionResolver<
       options,
     );
 
-    const linkName = relationMeta.name;
-
-    assignTargetsOfOneToOneRelation(
-      entities,
-      sourceKey,
-      linkName,
-      targetsFound,
-      targetKey,
-    );
+    return flattenTargetsOfOneToOneRelation(sourceIds, targetsFound, targetKey);
   };
 }
diff --git a/packages/repository/src/relations/has-many/has-many.inclusion-resolver.ts b/packages/repository/src/relations/has-many/has-many.inclusion-resolver.ts
@@ -8,8 +8,8 @@ import {Entity} from '../../model';
 import {Filter, Inclusion} from '../../query';
 import {EntityCrudRepository} from '../../repositories/repository';
 import {
-  assignTargetsOfOneToManyRelation,
   findByForeignKeys,
+  flattenTargetsOfOneToManyRelation,
   StringKeyOf,
 } from '../relation.helpers';
 import {Getter, HasManyDefinition, InclusionResolver} from '../relation.types';
@@ -31,8 +31,8 @@ export function createHasManyInclusionResolver<
     entities: Entity[],
     inclusion: Inclusion<Entity>,
     options?: Options,
-  ): Promise<void> {
-    if (!entities.length) return;
+  ): Promise<((Target & TargetRelations)[] | undefined)[]> {
+    if (!entities.length) return [];
 
     const sourceKey = relationMeta.keyFrom;
     const sourceIds = entities.map(e => (e as AnyObject)[sourceKey]);
@@ -47,12 +47,8 @@ export function createHasManyInclusionResolver<
       options,
     );
 
-    const linkName = relationMeta.name;
-
-    assignTargetsOfOneToManyRelation(
-      entities,
-      sourceKey,
-      linkName,
+    return flattenTargetsOfOneToManyRelation(
+      sourceIds,
       targetsFound,
       targetKey,
     );

diff --git a/packages/repository/src/relations/has-one/has-one.inclusion-resolver.ts b/packages/repository/src/relations/has-one/has-one.inclusion-resolver.ts
@@ -8,8 +8,8 @@ import {Entity} from '../../model';
 import {Filter, Inclusion} from '../../query';
 import {EntityCrudRepository} from '../../repositories/repository';
 import {
-  assignTargetsOfOneToOneRelation,
   findByForeignKeys,
+  flattenTargetsOfOneToOneRelation,
   StringKeyOf,
 } from '../relation.helpers';
 import {Getter, HasOneDefinition, InclusionResolver} from '../relation.types';
@@ -31,30 +31,22 @@ export function createHasOneInclusionResolver<
     entities: Entity[],
     inclusion: Inclusion<Entity>,
     options?: Options,
-  ): Promise<void> {
-    if (!entities.length) return;
+  ): Promise<((Target & TargetRelations) | undefined)[]> {
+    if (!entities.length) return [];
 
     const sourceKey = relationMeta.keyFrom;
     const sourceIds = entities.map(e => (e as AnyObject)[sourceKey]);
     const targetKey = relationMeta.keyTo as StringKeyOf<Target>;
 
     const targetRepo = await getTargetRepo();
-    const relatedTargets = await findByForeignKeys(
+    const targetsFound = await findByForeignKeys(
       targetRepo,
       targetKey,
       sourceIds,
       inclusion.scope as Filter<Target>,
       options,
     );
 
-    const linkName = relationMeta.name;
-
-    assignTargetsOfOneToOneRelation(
-      entities,
-      sourceKey,
-      linkName,
-      relatedTargets,
-      targetKey,
-    );
+    return flattenTargetsOfOneToOneRelation(sourceIds, targetsFound, targetKey);
   };
 }
diff --git a/packages/repository/src/relations/relation.helpers.ts b/packages/repository/src/relations/relation.helpers.ts
@@ -6,7 +6,7 @@
 import * as assert from 'assert';
 import {AnyObject} from 'loopback-datasource-juggler';
 import {Options} from '../common-types';
-import {Entity, Model} from '../model';
+import {Entity} from '../model';
 import {Filter, Where} from '../query';
 import {EntityCrudRepository, getRepositoryCapabilities} from '../repositories';
 
@@ -104,12 +104,12 @@ function splitByPageSize<T>(items: T[], pageSize: number): T[][] {
 export type StringKeyOf<T> = Extract<keyof T, string>;
 
 // TODO(bajtos) add test coverage
-export function buildLookupMap<Key, Entry, T extends Model>(
-  list: T[],
-  keyName: StringKeyOf<T>,
-  reducer: (accumulator: Entry | undefined, current: T) => Entry,
-): Map<Key, Entry> {
-  const lookup = new Map<Key, Entry>();
+export function buildLookupMap<Key, InType, OutType = InType>(
+  list: InType[],
+  keyName: StringKeyOf<InType>,
+  reducer: (accumulator: OutType | undefined, current: InType) => OutType,
+): Map<Key, OutType> {
+  const lookup = new Map<Key, OutType>();
   for (const entity of list) {
     const key = getKeyValue(entity, keyName) as Key;
     const original = lookup.get(key);
@@ -120,70 +120,68 @@ export function buildLookupMap<Key, Entry, T extends Model>(
 }
 
 // TODO(bajtos) add test coverage
-export function assignTargetsOfOneToOneRelation<
+export function flattenTargetsOfOneToOneRelation<
   SourceWithRelations extends Entity,
   Target extends Entity
 >(
-  sourceEntites: SourceWithRelations[],
-  sourceKey: string,
-  linkName: string,
+  sourceIds: unknown[],
   targetEntities: Target[],
   targetKey: StringKeyOf<Target>,
-) {
+): (Target | undefined)[] {
   const lookup = buildLookupMap<unknown, Target, Target>(
     targetEntities,
     targetKey,
     reduceAsSingleItem,
   );
 
-  for (const src of sourceEntites) {
-    const key = getKeyValue(src, sourceKey);
-    const target = lookup.get(key);
-    if (!target) continue;
-    (src as AnyObject)[linkName] = target;
-  }
+  return flattenMapByKeys(sourceIds, lookup);
 }
 
-function reduceAsSingleItem<T>(_acc: T | undefined, it: T) {
+export function reduceAsSingleItem<T>(_acc: T | undefined, it: T) {
   return it;
 }
 
 // TODO(bajtos) add test coverage
-export function assignTargetsOfOneToManyRelation<
-  SourceWithRelations extends Entity,
-  Target extends Entity
->(
-  sourceEntites: SourceWithRelations[],
-  sourceKey: string,
-  linkName: string,
+export function flattenTargetsOfOneToManyRelation<Target extends Entity>(
+  sourceIds: unknown[],
   targetEntities: Target[],
   targetKey: StringKeyOf<Target>,
-) {
-  const lookup = buildLookupMap<unknown, Target[], Target>(
+): (Target[] | undefined)[] {
+  const lookup = buildLookupMap<unknown, Target, Target[]>(
     targetEntities,
     targetKey,
     reduceAsArray,
   );
 
-  for (const src of sourceEntites) {
-    const key = getKeyValue(src, sourceKey);
-    const target = lookup.get(key);
-    if (!target) continue;
-    (src as AnyObject)[linkName] = target;
-  }
+  return flattenMapByKeys(sourceIds, lookup);
 }
 
-function reduceAsArray<T>(acc: T[] | undefined, it: T) {
+export function reduceAsArray<T>(acc: T[] | undefined, it: T) {
   if (acc) acc.push(it);
   else acc = [it];
   return acc;
 }
 
-function getKeyValue<T>(model: T, keyName: string) {
+export function getKeyValue<T>(model: T, keyName: string) {
   const rawKey = (model as AnyObject)[keyName];
   // Hacky workaround for MongoDB, see _SPIKE_.md for details
   if (typeof rawKey === 'object' && rawKey.constructor.name === 'ObjectID') {
     return rawKey.toString();
   }
   return rawKey;
 }
+
+export function flattenMapByKeys<T>(
+  sourceIds: unknown[],
+  targetMap: Map<unknown, T>,
+): (T | undefined)[] {
+  const result: (T | undefined)[] = new Array(sourceIds.length);
+
+  for (const ix in sourceIds) {
+    const key = sourceIds[ix];
+    const target = targetMap.get(key);
+    result[ix] = target;
+  }
+
+  return result;
+}
diff --git a/packages/repository/src/relations/relation.types.ts b/packages/repository/src/relations/relation.types.ts
@@ -112,7 +112,11 @@ export type RelationMetadata =
 export {Getter} from '@loopback/context';
 
 /**
- * @returns Promise of void. The source models are updated in-place.
+ * @returns An array of resolved values, the items must be ordered in the same
+ * way as `sourceEntities`. The resolved value can be one of:
+ * - `undefined` when no target model(s) were found
+ * - `Entity` for relations targeting a single model
+ * - `Entity[]` for relations targeting multiple models
  */
 export type InclusionResolver = (
   /**
@@ -127,4 +131,4 @@ export type InclusionResolver = (
    * Generic options object, e.g. carrying the Transaction object.
    */
   options?: Options,
-) => Promise<void>;
+) => Promise<(Entity | undefined)[] | (Entity[] | undefined)[]>;
diff --git a/packages/repository/src/repositories/legacy-juggler-bridge.ts b/packages/repository/src/repositories/legacy-juggler-bridge.ts
@@ -627,6 +627,7 @@ export class DefaultCrudRepository<
     return {...filter, include: undefined} as legacy.Filter;
   }
 
+  // TODO(bajtos) Extract a public helper that other repositories can use too
   protected async includeRelatedModels(
     entities: T[],
     filter?: Filter<T>,
@@ -649,10 +650,15 @@ export class DefaultCrudRepository<
       throw err;
     }
 
-    const resolveTasks = include.map(i => {
+    const resolveTasks = include.map(async i => {
       const relationName = i.relation!;
       const resolver = this.inclusionResolvers.get(relationName)!;
-      return resolver(entities, i, options);
+      const targets = await resolver(entities, i, options);
+
+      for (const ix in result) {
+        const src = result[ix];
+        (src as AnyObject)[relationName] = targets[ix];
+      }
     });
 
     await Promise.all(resolveTasks);