feat: update after retrieving data with related models

Signed-off-by: Miroslav Bajtoš <[email protected]>

_SPIKE_.md

@@ -75,3 +75,74 @@ Additional things not covered by the initial implementation
 - HasAndBelongsToMany - search juggler's test/include.test.js
 - Inclusion for polymorphic relations, see jugglers' test/relations.test.js
 - hasOne with scope (??), see jugglers' test/relations.test.js
+
+
+---
+
+Ideally, I'd like LB4 to define MongoDB PK and FKs as follows:
+- `{type: 'string', mongodb: {dataType: 'ObjectID'}}`
+Even better, `dataType: 'ObjectID'` should be automatically applied by the
+connector for PK and FKs referencing ObjectID PKs.
+
+For example:
+
+```ts
+@model()
+class Product {
+  @property({
+    type: 'string',
+    generated: true,
+    // ^^ implies dataType: 'ObjectID'
+  })
+  id: string;
+
+  @property({
+    type: 'string',
+    references: {
+      model: () => Category,
+      property: 'id',
+    },
+    // ^^ implies dataType: 'ObjectID' when Category is attached to MongoDB
+  })
+  categoryId: string;
+}
+```
+
+For v1, I suppose we can ask developers to provide dataType manually.
+
+```ts
+@model()
+class Product {
+  @property({
+    type: 'string',
+    generated: true,
+    mongodb: {dataType: 'ObjectID'},
+  })
+  id: string;
+
+  @property({
+    type: 'string',
+    mongodb: {dataType: 'ObjectID'},
+  })
+  categoryId: string;
+}
+```
+
+With this setup in place, `id` and `categoryId` properties should be always
+returned as strings from DAO and connector methods.
+
+This is tricky to achieve for the PK property, because juggler overrides
+user-supplied type with connector's default type when the PK is generated
+by the database. See
+[`DataSource.prototype.setupDataAccess()`](https://github.com/strongloop/loopback-datasource-juggler/blob/0c2bb81dace3592ecde8b9eccbd70d589da44d7d/lib/datasource.js#L713-L719)
+
+Can we rework MongoDB connector to hide ObjectID complexity as an internal
+implementation detail and always use string values in public APIs? Accept
+ids as strings and internally convert them to ObjectID. Convert values returned
+by the database from ObjectID to strings.
+
+Downside: this is not going to work for free-form properties that don't have
+any type definition and where the connector does not know that they should
+be converted from string to ObjectID. But then keep in mind that JSON cannot
+carry type information, therefore REST API clients are not able to set
+free-form properties to ObjectID values even today.

acceptance/repository-mongodb/src/__tests__/mongodb.datasource.ts

@@ -16,4 +16,8 @@ export const MONGODB_CONFIG: DataSourceOptions = {
 
 export const MONGODB_FEATURES: Partial<CrudFeatures> = {
   idType: 'string',
+
+  // TODO: we should run the test suite against two connector configurations:
+  // - one with "strictObjectID" set to true,
+  // - the other with "strictObjectID" turned off (the default).
 };

packages/repository-tests/src/crud/retrieve-including-relations.suite.ts

@@ -0,0 +1,153 @@
+// Copyright IBM Corp. 2019. All Rights Reserved.
+// Node module: @loopback/repository-tests
+// This file is licensed under the MIT License.
+// License text available at https://opensource.org/licenses/MIT
+
+import {
+  Entity,
+  EntityCrudRepository,
+  hasInclusionResolvers,
+  hasMany,
+  HasManyDefinition,
+  HasManyInclusionResolver,
+  model,
+  property,
+} from '@loopback/repository';
+import {expect, skipIf} from '@loopback/testlab';
+import {Suite} from 'mocha';
+import {withCrudCtx} from '../helpers.repository-tests';
+import {
+  CrudFeatures,
+  CrudRepositoryCtor,
+  CrudTestContext,
+  DataSourceOptions,
+} from '../types.repository-tests';
+
+// Core scenarios around retrieving model instances with related model includes
+// Please keep this file short, put any advanced scenarios to other files
+export function retrieveIncludingRelationsSuite(
+  dataSourceOptions: DataSourceOptions,
+  repositoryClass: CrudRepositoryCtor,
+  features: CrudFeatures,
+) {
+  @model()
+  class Category extends Entity {
+    @property({
+      type: features.idType,
+      id: true,
+      generated: true,
+    })
+    id: number | string;
+
+    @property({type: 'string', required: true})
+    name: string;
+
+    @hasMany(() => Item)
+    items?: Item[];
+    constructor(data?: Partial<Category>) {
+      super(data);
+    }
+  }
+
+  interface CategoryRelations {
+    items?: Item[];
+  }
+
+  @model()
+  class Item extends Entity {
+    @property({
+      type: features.idType,
+      id: true,
+      generated: true,
+    })
+    id: number | string;
+
+    @property({type: 'string', required: true})
+    name: string;
+
+    @property({
+      type: features.idType,
+      required: true,
+      // hacky workaround, we need a more generic solution that will allow
+      // any connector to contribute additional metadata for foreign keys
+      // ideally, we should use database-agnostic "references" field
+      // as proposed in https://github.com/strongloop/loopback-next/issues/2766
+      mongodb: {
+        dataType: 'ObjectID',
+      },
+    })
+    categoryId: number | string;
+
+    constructor(data?: Partial<Item>) {
+      super(data);
+    }
+  }
+
+  interface ItemRelations {
+    category?: Category;
+  }
+
+  skipIf<[(this: Suite) => void], void>(
+    !features.inclusionResolvers,
+    describe,
+    'retrieve models including relations',
+    () => {
+      let categoryRepo: EntityCrudRepository<
+        Category,
+        typeof Category.prototype.id,
+        CategoryRelations
+      >;
+      let itemRepo: EntityCrudRepository<
+        Item,
+        typeof Item.prototype.id,
+        ItemRelations
+      >;
+      before(
+        withCrudCtx(async function setupRepository(ctx: CrudTestContext) {
+          categoryRepo = new repositoryClass(Category, ctx.dataSource);
+          itemRepo = new repositoryClass(Item, ctx.dataSource);
+
+          if (!hasInclusionResolvers(categoryRepo)) {
+            throw new Error(
+              `Repository class "${
+                categoryRepo.constructor.name
+              }" must provide a public "inclusionResolvers" property`,
+            );
+          }
+
+          const itemsMeta = Category.definition.relations.items;
+          const itemsResolver = new HasManyInclusionResolver(
+            itemsMeta as HasManyDefinition,
+            async () => itemRepo,
+          );
+          categoryRepo.inclusionResolvers['items'] = itemsResolver;
+
+          await ctx.dataSource.automigrate([Category.name, Item.name]);
+        }),
+      );
+
+      it('ignores navigational properties when updating model instance', async () => {
+        const created = await categoryRepo.create({name: 'Stationery'});
+        const categoryId = created.id;
+
+        await itemRepo.create({
+          name: 'Pen',
+          categoryId,
+        });
+
+        const found = await categoryRepo.findById(categoryId, {
+          include: [{relation: 'items'}],
+        });
+        expect(found.items).to.have.lengthOf(1);
+
+        found.name = 'updated name';
+        const saved = await categoryRepo.save(found);
+        expect(saved.name).to.equal('updated name');
+
+        const loaded = await categoryRepo.findById(categoryId);
+        expect(loaded.name).to.equal('updated name');
+        expect(loaded).to.not.have.property('items');
+      });
+    },
+  );
+}

packages/repository-tests/src/types.repository-tests.ts

@@ -38,6 +38,14 @@ export interface CrudFeatures {
    * Default: `true`
    */
   freeFormProperties: boolean;
+
+  /**
+   * Does the repository provide `inclusionResolvers` object where resolvers
+   * can be registered?
+   *
+   * Default: `true`
+   */
+  inclusionResolvers: boolean;
 }
 
 /**

packages/repository/src/__tests__/acceptance/has-many.relation.acceptance.ts

@@ -149,7 +149,7 @@ describe('HasMany relation', () => {
           },
         ],
       }),
-    ).to.be.rejectedWith(/`orders` is not defined/);
+    ).to.be.rejectedWith(/Navigational properties are not allowed.*"orders"/);
   });
 
   context('when targeting the source model', () => {

packages/repository/src/relations/relation.helpers.ts

@@ -111,7 +111,7 @@ export function buildLookupMap<Key, Entry, T extends Model>(
 ): Map<Key, Entry> {
   const lookup = new Map<Key, Entry>();
   for (const entity of list) {
-    const key = (entity as AnyObject)[keyName] as Key;
+    const key = getKeyValue(entity, keyName) as Key;
     const original = lookup.get(key);
     const reduced = reducer(original, entity);
     lookup.set(key, reduced);
@@ -137,7 +137,8 @@ export function assignTargetsOfOneToOneRelation<
   );
 
   for (const src of sourceEntites) {
-    const target = lookup.get(src[sourceKey]);
+    const key = getKeyValue(src, sourceKey);
+    const target = lookup.get(key);
     if (!target) continue;
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     src[linkName] = target as any;
@@ -166,7 +167,8 @@ export function assignTargetsOfOneToManyRelation<
   );
 
   for (const src of sourceEntites) {
-    const target = lookup.get(src[sourceKey]);
+    const key = getKeyValue(src, sourceKey);
+    const target = lookup.get(key);
     if (!target) continue;
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     src[linkName] = target as any;
@@ -178,3 +180,12 @@ function reduceAsArray<T>(acc: T[] | undefined, it: T) {
   else acc = [it];
   return acc;
 }
+
+function getKeyValue<T>(model: T, keyName: StringKeyOf<T>) {
+  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;
+}