fix(repository): revert hasOne target FK as PK implementation

Remove attempt at guaranteeing the foreign key of a has one relation to be unique by using it as the primary key of the target model. This allow users to set up their own unique contstraints.
loopbackio · Dec 12, 2018 · caa876b · caa876b
1 parent 40973a8
commit caa876b
Show file tree

Hide file tree

Showing 10 changed files with 24 additions and 163 deletions.
diff --git a/examples/todo-list/src/models/todo-list-image.model.ts b/examples/todo-list/src/models/todo-list-image.model.ts
@@ -1,9 +1,15 @@
-import {Entity, model, property, belongsToUniquely} from '@loopback/repository';
+import {Entity, model, property, belongsTo} from '@loopback/repository';
 import {TodoList} from './todo-list.model';
 
 @model()
 export class TodoListImage extends Entity {
-  @belongsToUniquely(() => TodoList)
+  @property({
+    type: 'string',
+    id: true,
+  })
+  id: string;
+
+  @belongsTo(() => TodoList)
   todoListId?: number;
 
   @property({

diff --git a/examples/todo-list/src/repositories/todo-list-image.repository.ts b/examples/todo-list/src/repositories/todo-list-image.repository.ts
@@ -10,11 +10,11 @@ import {TodoListRepository} from './todo-list.repository';
 
 export class TodoListImageRepository extends DefaultCrudRepository<
   TodoListImage,
-  typeof TodoListImage.prototype.todoListId
+  typeof TodoListImage.prototype.id
 > {
   public readonly todoList: BelongsToAccessor<
     TodoList,
-    typeof TodoListImage.prototype.todoListId
+    typeof TodoListImage.prototype.id
   >;
   constructor(
     @inject('datasources.db') dataSource: DbDataSource,

diff --git a/packages/repository/src/relations/belongs-to/belongs-to.decorator.ts b/packages/repository/src/relations/belongs-to/belongs-to.decorator.ts
@@ -14,14 +14,11 @@ import {BelongsToDefinition, RelationType} from '../relation.types';
  * @param targetResolver A resolver function that returns the target model for
  * a belongsTo relation
  * @param definition Optional metadata for setting up a belongsTo relation
- * @param propertyMeta Optional property metadata to call the proprety decorator
- * with
  * @returns {(target: Object, key:string)}
  */
 export function belongsTo<T extends Entity>(
   targetResolver: EntityResolver<T>,
   definition?: Partial<BelongsToDefinition>,
-  propertyMeta?: Partial<PropertyDefinition>,
 ) {
   return function(decoratedTarget: Entity, decoratedKey: string) {
     const propMeta: PropertyDefinition = {
@@ -33,7 +30,6 @@ export function belongsTo<T extends Entity>(
       // allows controller methods to exclude required properties
       // required: true,
     };
-    Object.assign(propMeta, propertyMeta);
     property(propMeta)(decoratedTarget, decoratedKey);
 
     // @belongsTo() is typically decorating the foreign key property,
@@ -58,22 +54,3 @@ export function belongsTo<T extends Entity>(
     relation(meta)(decoratedTarget, decoratedKey);
   };
 }
-
-/**
- * Sugar syntax for belongsTo decorator for hasOne relation. Calls belongsTo
- * with property definition defaults for non-generated id field
- * @param targetResolver A resolver function that returns the target model for
- * a belongsTo relation
- * @param definition Optional metadata for setting up a belongsTo relation
- * @returns {(target: Object, key:string)}
- */
-export function belongsToUniquely<T extends Entity>(
-  targetResolver: EntityResolver<T>,
-  definition?: Partial<BelongsToDefinition>,
-) {
-  const propertyMetaDefaults: Partial<PropertyDefinition> = {
-    id: true,
-    generated: false,
-  };
-  return belongsTo(targetResolver, definition, propertyMetaDefaults);
-}
diff --git a/packages/repository/src/relations/has-one/has-one-repository.factory.ts b/packages/repository/src/relations/has-one/has-one-repository.factory.ts
@@ -99,11 +99,5 @@ function resolveHasOneMetadata(
     throw new InvalidRelationError(reason, relationMeta);
   }
 
-  const defaultFkProp = targetModel.definition.properties[defaultFkName];
-  if (!defaultFkProp.id || defaultFkProp.generated) {
-    const reason = `foreign key ${defaultFkName} must be an id property that is not auto generated`;
-    throw new InvalidRelationError(reason, relationMeta);
-  }
-
   return Object.assign(relationMeta, {keyTo: defaultFkName});
 }
diff --git a/packages/repository/src/relations/has-one/has-one.repository.ts b/packages/repository/src/relations/has-one/has-one.repository.ts
@@ -47,7 +47,7 @@ export class DefaultHasOneRepository<
   TargetRepository extends EntityCrudRepository<TargetEntity, TargetID>
 > implements HasOneRepository<TargetEntity> {
   /**
-   * Constructor of DefaultHasManyEntityCrudRepository
+   * Constructor of DefaultHasOneEntityCrudRepository
    * @param getTargetRepository the getter of the related target model repository instance
    * @param constraint the key value pair representing foreign key name to constrain
    * the target repository instance

diff --git a/packages/repository/test/acceptance/has-one.relation.acceptance.ts b/packages/repository/test/acceptance/has-one.relation.acceptance.ts
@@ -44,11 +44,14 @@ describe('hasOne relation', () => {
       street: '123 test avenue',
     });
 
-    const persisted = await addressRepo.findById(address.customerId);
+    const persisted = await addressRepo.findById(address.zipcode);
     expect(persisted.toObject()).to.deepEqual(address.toObject());
   });
 
-  it('refuses to create related model instance twice', async () => {
+  // We do not enforce referential integrity at the moment. It is up to
+  // our users to set up unique constraint(s) between related models at the
+  // database level
+  it.skip('refuses to create related model instance twice', async () => {
     const address = await controller.createCustomerAddress(existingCustomerId, {
       street: '123 test avenue',
     });
@@ -62,7 +65,7 @@ describe('hasOne relation', () => {
       street: '123 test avenue',
     });
 
-    const persisted = await addressRepo.findById(address.customerId);
+    const persisted = await addressRepo.findById(address.zipcode);
     expect(persisted.toObject()).to.deepEqual(address.toObject());
   });
 
@@ -106,7 +109,7 @@ describe('hasOne relation', () => {
     const address = await controller.createCustomerAddress(existingCustomerId, {
       street: '123 test avenue',
     });
-    await addressRepo.deleteById(address.customerId);
+    await addressRepo.deleteById(address.zipcode);
 
     await expect(
       controller.findCustomerAddress(existingCustomerId),

diff --git a/packages/repository/test/fixtures/models/address.model.ts b/packages/repository/test/fixtures/models/address.model.ts
@@ -3,7 +3,7 @@
 // This file is licensed under the MIT License.
 // License text available at https://opensource.org/licenses/MIT
 
-import {Entity, model, property, belongsToUniquely} from '../../..';
+import {Entity, model, property, belongsTo} from '../../..';
 import {Customer} from './customer.model';
 
 @model()
@@ -14,6 +14,7 @@ export class Address extends Entity {
   street: String;
   @property({
     type: 'string',
+    id: true,
   })
   zipcode: String;
   @property({
@@ -25,6 +26,6 @@ export class Address extends Entity {
   })
   province: String;
 
-  @belongsToUniquely(() => Customer)
+  @belongsTo(() => Customer)
   customerId: number;
 }
diff --git a/packages/repository/test/fixtures/repositories/address.repository.ts b/packages/repository/test/fixtures/repositories/address.repository.ts
@@ -15,11 +15,11 @@ import {CustomerRepository} from '../repositories';
 
 export class AddressRepository extends DefaultCrudRepository<
   Address,
-  typeof Address.prototype.customerId
+  typeof Address.prototype.zipcode
 > {
   public readonly customer: BelongsToAccessor<
     Customer,
-    typeof Address.prototype.customerId
+    typeof Address.prototype.zipcode
   >;
 
   constructor(

diff --git a/packages/repository/test/unit/decorator/model-and-relation.decorator.unit.ts b/packages/repository/test/unit/decorator/model-and-relation.decorator.unit.ts
@@ -8,7 +8,6 @@ import {expect} from '@loopback/testlab';
 import {RelationMetadata} from '../../..';
 import {
   belongsTo,
-  belongsToUniquely,
   embedsMany,
   embedsOne,
   Entity,
@@ -94,30 +93,14 @@ describe('model decorator', () => {
     @property({type: 'string', id: true, generated: true})
     id: string;
 
-    @belongsTo(
-      () => Customer,
-      {},
-      {
-        id: true,
-        generated: false,
-      },
-    )
+    @belongsTo(() => Customer)
     customerId: string;
 
     // Validates that property no longer requires a parameter
     @property()
     isShipped: boolean;
   }
 
-  @model()
-  class RegistrationDate extends Entity {
-    @belongsToUniquely(() => Customer)
-    customerId: number;
-
-    @property()
-    registeredOn: Date;
-  }
-
   @model()
   class Customer extends Entity {
     @property({type: 'string', id: true, generated: true})
@@ -303,32 +286,6 @@ describe('model decorator', () => {
     expect(relationDef.target()).to.be.exactly(Customer);
   });
 
-  it('passes property metadata from belongsToUniquely', () => {
-    const propMeta =
-      MetadataInspector.getAllPropertyMetadata(
-        MODEL_PROPERTIES_KEY,
-        RegistrationDate.prototype,
-      ) || /* istanbul ignore next */ {};
-
-    expect(propMeta.customerId).to.containEql({
-      id: true,
-      generated: false,
-    });
-  });
-
-  it('passes property metadata from belongsTo', () => {
-    const propMeta =
-      MetadataInspector.getAllPropertyMetadata(
-        MODEL_PROPERTIES_KEY,
-        Order.prototype,
-      ) || /* istanbul ignore next */ {};
-
-    expect(propMeta.customerId).to.containEql({
-      id: true,
-      generated: false,
-    });
-  });
-
   it('adds hasOne metadata', () => {
     const meta =
       MetadataInspector.getAllPropertyMetadata(

diff --git a/packages/repository/test/unit/repositories/has-one-repository-factory.unit.ts b/packages/repository/test/unit/repositories/has-one-repository-factory.unit.ts
@@ -78,38 +78,6 @@ describe('createHasOneRepositoryFactory', () => {
     ).to.throw(/target model Address is missing.*foreign key customerId/);
   });
 
-  it('rejects relations with keyTo that is not an id', () => {
-    const relationMeta = givenHasOneDefinition({
-      name: 'order',
-      target: () => ModelWithoutIndexFK,
-    });
-
-    expect(() =>
-      createHasOneRepositoryFactory(
-        relationMeta,
-        Getter.fromValue(customerRepo),
-      ),
-    ).to.throw(
-      /foreign key customerId must be an id property that is not auto generated/,
-    );
-  });
-
-  it('rejects relations with keyTo that is a generated id property', () => {
-    const relationMeta = givenHasOneDefinition({
-      name: 'profile',
-      target: () => ModelWithGeneratedFK,
-    });
-
-    expect(() =>
-      createHasOneRepositoryFactory(
-        relationMeta,
-        Getter.fromValue(customerRepo),
-      ),
-    ).to.throw(
-      /foreign key customerId must be an id property that is not auto generated/,
-    );
-  });
-
   /*------------- HELPERS ---------------*/
 
   class Address extends Entity {
@@ -131,51 +99,6 @@ describe('createHasOneRepositoryFactory', () => {
     city: String;
     province: String;
   }
-
-  // added an id property because the model itself extends from Entity, but the
-  // purpose here is the decorated relational property is not part of the
-  // composite index, thus the name ModelWithoutIndexFK.
-  class ModelWithoutIndexFK extends Entity {
-    static definition = new ModelDefinition('ModelWithoutIndexFK')
-      .addProperty('customerId', {
-        type: 'string',
-        id: false,
-      })
-      .addProperty('description', {
-        type: 'string',
-        required: true,
-      })
-      .addProperty('id', {
-        type: 'number',
-        id: true,
-      })
-
-      .addProperty('isShipped', {
-        type: 'boolean',
-        required: false,
-      });
-    id: number;
-    customerId: string;
-    description: string;
-    isShipped: boolean;
-  }
-
-  class ModelWithGeneratedFK extends Entity {
-    static definition = new ModelDefinition('ModelWithGeneratedFK')
-      .addProperty('customerId', {
-        type: 'string',
-        id: true,
-        generated: true,
-      })
-      .addProperty('description', {
-        type: 'string',
-        required: true,
-      });
-
-    customerId: string;
-    description: string;
-  }
-
   class Customer extends Entity {
     static definition = new ModelDefinition('Customer').addProperty('id', {
       type: Number,