feat(core): allow for the number zero as a primary key (#426)

There are some cases where an entity will have a primary key of 0.
Our use case is a table of static statuses that are pervasive
throughout our application. We add the rows when we generate the
schema, not through the ORM. We only want to map relationships
to those statuses with the ORM. Some of these statuses start with
the id of 0 versus 1. Previous to this fix, there were generic
falsy checks that would take a primary key with a value of 0 as
if the primary key was not set. This fix makes those checks
specific for both null and undefined values.

Co-authored-by: Phillip Bohnenkamp <phil.bohnenkamp@youtyl.com>

lib/entity/EntityAssigner.ts

@@ -31,7 +31,7 @@ export class EntityAssigner {
         value = props[prop].customType.convertToJSValue(value, platform);
       }
 
-      if (props[prop] && [ReferenceType.MANY_TO_ONE, ReferenceType.ONE_TO_ONE].includes(props[prop].reference) && value && EntityAssigner.validateEM(em)) {
+      if (props[prop] && [ReferenceType.MANY_TO_ONE, ReferenceType.ONE_TO_ONE].includes(props[prop].reference) && Utils.isDefined(value, true) && EntityAssigner.validateEM(em)) {
         return EntityAssigner.assignReference<T>(entity, value, props[prop], em!);
       }
 

lib/entity/EntityFactory.ts

@@ -66,7 +66,7 @@ export class EntityFactory {
     const Entity = this.metadata.get<T>(meta.name).class;
     const pks = Utils.getOrderedPrimaryKeys<T>(data, meta);
 
-    if (meta.primaryKeys.some(pk => !data[pk as keyof T])) {
+    if (meta.primaryKeys.some(pk => !Utils.isDefined(data[pk as keyof T], true))) {
       const params = this.extractConstructorParams<T>(meta, data);
       meta.constructorParams.forEach(prop => delete data[prop]);
 
@@ -101,7 +101,7 @@ export class EntityFactory {
     const platform = this.driver.getPlatform();
     const pk = platform.getSerializedPrimaryKeyField(primaryKey);
 
-    if (data[pk] || data[primaryKey]) {
+    if (Utils.isDefined(data[pk], true) || Utils.isDefined(data[primaryKey], true)) {
       const id = platform.denormalizePrimaryKey(data[pk] || data[primaryKey]);
       delete data[pk];
       data[primaryKey as keyof T] = id as Primary<T> & T[keyof T];

lib/entity/EntityTransformer.ts

@@ -14,11 +14,11 @@ export class EntityTransformer {
     const ret = {} as EntityData<T>;
 
     meta.primaryKeys
-      .filter(pk => !entity[pk] || !meta.properties[pk].hidden)
+      .filter(pk => !Utils.isDefined(entity[pk], true) || !meta.properties[pk].hidden)
       .map(pk => [pk, Utils.getPrimaryKeyValue<T>(entity, [pk])] as [string, string])
       .forEach(([pk, value]) => ret[platform.getSerializedPrimaryKeyField(pk) as keyof T] = platform.normalizePrimaryKey(value));
 
-    if ((!wrapped.isInitialized() && wrapped.__primaryKey) || visited.includes(wrap(entity).__uuid)) {
+    if ((!wrapped.isInitialized() && Utils.isDefined(wrapped.__primaryKey, true)) || visited.includes(wrap(entity).__uuid)) {
       return ret;
     }
 

lib/entity/EntityValidator.ts

@@ -70,7 +70,7 @@ export class EntityValidator {
   }
 
   validatePrimaryKey<T extends AnyEntity<T>>(entity: EntityData<T>, meta: EntityMetadata): void {
-    const pkExists = meta.primaryKeys.every(pk => entity[pk as keyof T]) || entity[meta.serializedPrimaryKey];
+    const pkExists = meta.primaryKeys.every(pk => Utils.isDefined(entity[pk as keyof T], true)) || Utils.isDefined(entity[meta.serializedPrimaryKey], true);
 
     if (!entity || !pkExists) {
       throw ValidationError.fromMergeWithoutPK(meta);

lib/unit-of-work/ChangeSetComputer.ts

@@ -72,7 +72,7 @@ export class ChangeSetComputer {
     const pks = this.metadata.get(prop.type).primaryKeys;
     const entity = changeSet.entity[prop.name] as unknown as T;
 
-    if (pks.length === 1 && !entity[pks[0]]) {
+    if (pks.length === 1 && !Utils.isDefined(entity[pks[0]], true)) {
       changeSet.payload[prop.name] = this.identifierMap[wrap(entity).__uuid];
     }
   }

lib/unit-of-work/ChangeSetPersister.ts

@@ -32,14 +32,14 @@ export class ChangeSetPersister {
     } else if (changeSet.type === ChangeSetType.UPDATE) {
       res = await this.updateEntity(meta, changeSet, ctx);
       this.mapReturnedValues(changeSet.entity, res, meta);
-    } else if (changeSet.entity.__primaryKey) { // ChangeSetType.CREATE with primary key
+    } else if (Utils.isDefined(changeSet.entity.__primaryKey, true)) { // ChangeSetType.CREATE with primary key
       res = await this.driver.nativeInsert(changeSet.name, changeSet.payload, ctx);
       this.mapReturnedValues(changeSet.entity, res, meta);
       delete changeSet.entity.__initialized;
     } else { // ChangeSetType.CREATE without primary key
       res = await this.driver.nativeInsert(changeSet.name, changeSet.payload, ctx);
       this.mapReturnedValues(changeSet.entity, res, meta);
-      wrap(changeSet.entity).__primaryKey = changeSet.entity.__primaryKey || res.insertId as any;
+      wrap(changeSet.entity).__primaryKey = Utils.isDefined(changeSet.entity.__primaryKey, true) ? changeSet.entity.__primaryKey : res.insertId as any;
       this.identifierMap[changeSet.entity.__uuid].setValue(changeSet.entity.__primaryKey as IPrimaryKey);
       delete changeSet.entity.__initialized;
     }

lib/unit-of-work/UnitOfWork.ts

@@ -35,7 +35,7 @@ export class UnitOfWork {
     const wrapped = wrap(entity);
     Object.defineProperty(wrapped, '__em', { value: this.em, writable: true });
 
-    if (!wrapped.__primaryKey) {
+    if (!Utils.isDefined(wrapped.__primaryKey, true)) {
       return;
     }
 
@@ -81,7 +81,7 @@ export class UnitOfWork {
       return;
     }
 
-    if (!wrap(entity).__primaryKey) {
+    if (!Utils.isDefined(wrap(entity).__primaryKey, true)) {
       this.identifierMap[wrap(entity).__uuid] = new EntityIdentifier();
     }
 
@@ -191,7 +191,7 @@ export class UnitOfWork {
       return;
     }
 
-    if (!wrapped.__primaryKey && !this.identifierMap[wrapped.__uuid]) {
+    if (!Utils.isDefined(wrapped.__primaryKey, true) && !this.identifierMap[wrapped.__uuid]) {
       this.identifierMap[wrapped.__uuid] = new EntityIdentifier();
     }
 

lib/utils/Utils.ts

@@ -19,8 +19,8 @@ export class Utils {
   /**
    * Checks if the argument is not undefined
    */
-  static isDefined<T = object>(data: any): data is T {
-    return typeof data !== 'undefined';
+  static isDefined<T = object>(data: any, considerNullUndefined = false): data is T {
+    return typeof data !== 'undefined' && !(considerNullUndefined && data === null);
   }
 
   /**
@@ -155,7 +155,7 @@ export class Utils {
 
     const collection = entity[prop.name] as unknown instanceof ArrayCollection;
     const noPkRef = Utils.isEntity(entity[prop.name]) && !wrap(entity[prop.name]).__primaryKeys.every(pk => pk);
-    const noPkProp = prop.primary && !entity[prop.name];
+    const noPkProp = prop.primary && !Utils.isDefined(entity[prop.name], true);
     const inverse = prop.reference === ReferenceType.ONE_TO_ONE && !prop.owner;
 
     // bidirectional 1:1 and m:1 fields are defined as setters, we need to check for `undefined` explicitly

tests/EntityManager.mysql.test.ts

@@ -182,6 +182,40 @@ describe('EntityManagerMySql', () => {
     expect(wrap(a!.baz!.bar).isInitialized()).toBe(true);
   });
 
+  test('factory should support a primary key value of 0', async () => {
+    const factory = orm.em.getEntityFactory();
+    const p1 = new Publisher2(); // calls constructor, so uses default name
+    expect(p1.name).toBe('asd');
+    expect(p1).toBeInstanceOf(Publisher2);
+    expect(p1.books).toBeInstanceOf(Collection);
+    expect(p1.tests).toBeInstanceOf(Collection);
+    const p2 = factory.create(Publisher2, { id: 0 }); // shouldn't call constructor
+    expect(p2).toBeInstanceOf(Publisher2);
+    expect(p2.name).toBeUndefined();
+    expect(p2.books).toBeInstanceOf(Collection);
+    expect(p2.tests).toBeInstanceOf(Collection);
+  });
+
+  test(`1:1 relationships with an inverse side primary key of 0 should link`, async () => {
+    // Set up static data with id of 0
+    const driver = orm.em.getDriver();
+    const response = await driver.getConnection().execute('SET sql_mode = \'NO_AUTO_VALUE_ON_ZERO\';insert into foo_baz2 (id, name) values (?, ?)', [0, 'testBaz'], 'run');
+    expect(response[1]).toMatchObject({
+      affectedRows: 1,
+      insertId: 0,
+    });
+    const fooBazRef = orm.em.getReference<FooBaz2>(FooBaz2, 0);
+    const fooBar = FooBar2.create('testBar');
+    fooBar.baz = Reference.create(fooBazRef);
+    await orm.em.persistAndFlush(fooBar);
+    orm.em.clear();
+    const repo = orm.em.getRepository(FooBar2);
+    const a = await repo.findOne(fooBar.id, ['baz']);
+    expect(wrap(a!.baz).isInitialized()).toBe(true);
+    expect(wrap(a!.baz).id).toBe(0);
+    expect(wrap(a!.baz).name).toBe('testBaz');
+  });
+
   test('inverse side of 1:1 is ignored in change set', async () => {
     const bar = FooBar2.create('fb');
     bar.baz = new FooBaz2('fz 1');

tests/Utils.test.ts

@@ -13,6 +13,17 @@ describe('Utils', () => {
   beforeAll(async () => orm = await initORMMongo());
   beforeEach(async () => wipeDatabase(orm.em));
 
+  test('isDefined', () => {
+    let data;
+    expect(Utils.isDefined(data)).toBe(false);
+    data = null;
+    expect(Utils.isDefined(data)).toBe(true);
+    expect(Utils.isDefined(data, true)).toBe(false);
+    data = 0;
+    expect(Utils.isDefined(data)).toBe(true);
+    expect(Utils.isDefined(data, true)).toBe(true);
+  });
+
   test('isObject', () => {
     expect(Utils.isObject(undefined)).toBe(false);
     expect(Utils.isObject('a')).toBe(false);