diff --git a/_SPIKE_.md b/_SPIKE_.md index 4e7f9983bbb6..3ff162f238c7 100644 --- a/_SPIKE_.md +++ b/_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. diff --git a/acceptance/repository-mongodb/src/__tests__/mongodb.datasource.ts b/acceptance/repository-mongodb/src/__tests__/mongodb.datasource.ts index 1f872af2a0d5..2948b03d827b 100644 --- a/acceptance/repository-mongodb/src/__tests__/mongodb.datasource.ts +++ b/acceptance/repository-mongodb/src/__tests__/mongodb.datasource.ts @@ -16,4 +16,8 @@ export const MONGODB_CONFIG: DataSourceOptions = { export const MONGODB_FEATURES: Partial = { 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). }; diff --git a/packages/repository-tests/src/crud/retrieve-including-relations.suite.ts b/packages/repository-tests/src/crud/retrieve-including-relations.suite.ts new file mode 100644 index 000000000000..215d5eea5dd0 --- /dev/null +++ b/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) { + 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) { + 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'); + }); + }, + ); +} diff --git a/packages/repository-tests/src/types.repository-tests.ts b/packages/repository-tests/src/types.repository-tests.ts index 189f4c3955c2..ee6555d88a31 100644 --- a/packages/repository-tests/src/types.repository-tests.ts +++ b/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; } /** diff --git a/packages/repository/src/__tests__/acceptance/has-many.relation.acceptance.ts b/packages/repository/src/__tests__/acceptance/has-many.relation.acceptance.ts index 233828221533..d39b9fb3c820 100644 --- a/packages/repository/src/__tests__/acceptance/has-many.relation.acceptance.ts +++ b/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', () => { diff --git a/packages/repository/src/relations/relation.helpers.ts b/packages/repository/src/relations/relation.helpers.ts index a4c8189bd147..5c4aada7384a 100644 --- a/packages/repository/src/relations/relation.helpers.ts +++ b/packages/repository/src/relations/relation.helpers.ts @@ -111,7 +111,7 @@ export function buildLookupMap( ): Map { const lookup = new Map(); 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(acc: T[] | undefined, it: T) { else acc = [it]; return acc; } + +function getKeyValue(model: T, keyName: StringKeyOf) { + 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; +} diff --git a/packages/repository/src/repositories/legacy-juggler-bridge.ts b/packages/repository/src/repositories/legacy-juggler-bridge.ts index 4a0cc61e5ff6..4403212ac1c0 100644 --- a/packages/repository/src/repositories/legacy-juggler-bridge.ts +++ b/packages/repository/src/repositories/legacy-juggler-bridge.ts @@ -102,7 +102,7 @@ export class DefaultCrudRepository< capabilities: ConnectorCapabilities; modelClass: juggler.PersistedModelClass; - protected inclusionResolvers: {[key: string]: InclusionResolver}; + public inclusionResolvers: {[key: string]: InclusionResolver}; /** * Constructor of DefaultCrudRepository @@ -411,13 +411,21 @@ export class DefaultCrudRepository< } async create(entity: DataObject, options?: Options): Promise { - const model = await ensurePromise(this.modelClass.create(entity, options)); + const model = await ensurePromise( + this.modelClass.create( + this.fromEntity(entity, {relations: 'throw'}), + options, + ), + ); return this.toEntity(model); } async createAll(entities: DataObject[], options?: Options): Promise { const models = await ensurePromise( - this.modelClass.create(entities, options), + this.modelClass.create( + entities.map(e => this.fromEntity(e, {relations: 'throw'})), + options, + ), ); return this.toEntities(models); } @@ -487,7 +495,7 @@ export class DefaultCrudRepository< ): Promise { where = where || {}; const result = await ensurePromise( - this.modelClass.updateAll(where, data, options), + this.modelClass.updateAll(where, this.fromEntity(data), options), ); return {count: result.count}; } @@ -512,7 +520,17 @@ export class DefaultCrudRepository< options?: Options, ): Promise { try { - await ensurePromise(this.modelClass.replaceById(id, data, options)); + const payload = this.fromEntity(data); + debug( + '%s replaceById', + this.modelClass.modelName, + typeof id, + id, + payload, + 'options', + options, + ); + await ensurePromise(this.modelClass.replaceById(id, payload, options)); } catch (err) { if (err.statusCode === 404) { throw new EntityNotFoundError(this.entityClass, id); @@ -560,6 +578,43 @@ export class DefaultCrudRepository< return models.map(m => this.toEntity(m)); } + // TODO(bajtos) add test coverage for all methods calling this helper + // and also test both variants (R.toJSON() and DataObject) + protected fromEntity( + entity: R | DataObject, + options: {relations?: true | false | 'throw'} = {}, + ): legacy.ModelData { + // FIXME(bajtos) Ideally, we should call toJSON() to convert R to data object + // Unfortunately that breaks replaceById for MongoDB connector, where we + // would call replaceId with id *argument* set to ObjectID value but + // id *property* set to string value. + /* + const data: AnyObject = + typeof entity.toJSON === 'function' ? entity.toJSON() : {...entity}; + */ + const data: AnyObject = new this.entityClass(entity); + + if (options.relations === true) return data; + + const def = this.entityClass.definition; + for (const r in def.relations) { + const relName = def.relations[r].name; + if (relName in data) { + if (options.relations === 'throw') { + throw new Error( + `Navigational properties are not allowed in model data (model "${ + this.entityClass.modelName + }" property "${relName}")`, + ); + } + + delete data[relName]; + } + } + + return data; + } + protected normalizeFilter(filter?: Filter): legacy.Filter | undefined { if (!filter) return undefined; return {...filter, include: undefined} as legacy.Filter; diff --git a/packages/repository/src/repositories/repository.ts b/packages/repository/src/repositories/repository.ts index 1db116c4d64f..8d6517213698 100644 --- a/packages/repository/src/repositories/repository.ts +++ b/packages/repository/src/repositories/repository.ts @@ -18,6 +18,7 @@ import {DataSource} from '../datasource'; import {EntityNotFoundError} from '../errors'; import {Entity, Model, ValueObject} from '../model'; import {Filter, Where} from '../query'; +import {InclusionResolver} from '../relations'; /* eslint-disable @typescript-eslint/no-unused-vars */ @@ -217,6 +218,18 @@ export interface EntityCrudRepository< exists(id: ID, options?: Options): Promise; } +// TODO(semver-major) move this property to CrudRepository interface +export interface WithInclusionResolvers { + inclusionResolvers: {[key: string]: InclusionResolver}; +} + +export function hasInclusionResolvers( + repo: Repository, +): repo is WithInclusionResolvers { + const resolvers = (repo as WithInclusionResolvers).inclusionResolvers; + return resolvers && typeof resolvers === 'object'; +} + /** * Repository implementation *