fix: handle foreign key toward non primary key (#585)

GuillaumeCisco · web-flow · commit 64c78c7545bc · 2021-01-21T09:34:24.000+01:00
diff --git a/src/services/belongs-to-updater.js b/src/services/belongs-to-updater.js
@@ -2,19 +2,6 @@ const _ = require('lodash');
 const orm = require('../utils/orm');
 const associationRecord = require('../utils/association-record');
 
-// WORKAROUND: Make the hasOne associations update work while waiting
-//             for the Sequelize 4 release with the fix of the following
-//             issue: https://github.com/sequelize/sequelize/issues/6069
-const getTargetKey = async (pk, association) => {
-  let targetKey = pk;
-
-  if (association.associationType === 'HasOne') {
-    targetKey = await associationRecord.get(association.target, pk);
-  }
-
-  return targetKey;
-};
-
 class BelongsToUpdater {
   constructor(model, assoc, opts, params, data) {
     this.model = model;
@@ -24,15 +11,34 @@ class BelongsToUpdater {
     this.data = data;
   }
 
+  // WORKAROUND: Make the hasOne associations update work while waiting
+  //             for the Sequelize 4 release with the fix of the following
+  //             issue: https://github.com/sequelize/sequelize/issues/6069
+  async _getTargetKey(association) {
+    const pk = this.data.data.id;
+    let targetKey = pk;
+
+    if (association.associationType === 'HasOne' || association.targetKey !== 'id') {
+      const record = await associationRecord.get(association.target, pk);
+      if (association.associationType === 'HasOne') {
+        targetKey = record;
+      } else if (association.targetKey !== 'id') {
+        // NOTICE: special use case with foreign key non pointing to a primary key
+        targetKey = record[association.targetKey];
+      }
+    }
+
+    return targetKey;
+  }
+
   async perform() {
     const { associationName, recordId } = this.params;
     const record = await orm.findRecord(this.model, recordId);
     const association = Object.values(this.model.associations)
       .find((a) => a.associationAccessor === associationName);
 
     if (association && this.data.data) {
-      const pk = this.data.data.id;
-      const targetKey = await getTargetKey(pk, association);
+      const targetKey = await this._getTargetKey(association);
 
       // NOTICE: Enable model hooks to change fields values during an association update.
       const options = { fields: null };
diff --git a/src/services/resource-creator.js b/src/services/resource-creator.js
@@ -4,6 +4,7 @@ const Interface = require('forest-express');
 const { ErrorHTTP422 } = require('./errors');
 const ResourceGetter = require('./resource-getter');
 const CompositeKeysManager = require('./composite-keys-manager');
+const associationRecord = require('../utils/association-record');
 
 class ResourceCreator {
   constructor(model, params) {
@@ -12,11 +13,23 @@ class ResourceCreator {
     this.schema = Interface.Schemas.schemas[model.name];
   }
 
+  async _getTargetKey(name, association) {
+    const pk = this.params[name];
+
+    let targetKey = pk;
+    if (association.targetKey !== 'id') {
+      const record = await associationRecord.get(association.target, pk);
+      targetKey = record[association.targetKey];
+    }
+    return targetKey;
+  }
+
   _makePromisesBeforeSave(record) {
-    return (promises, [name, association]) => {
+    return async (promises, [name, association]) => {
       if (association.associationType === 'BelongsTo') {
         const setterName = `set${_.upperFirst(name)}`;
-        const promise = record[setterName](this.params[name], { save: false });
+        const targetKey = await this._getTargetKey(name, association);
+        const promise = record[setterName](targetKey, { save: false });
         promises.push(promise);
       }
       return promises;
diff --git a/test/databases.test.js b/test/databases.test.js
@@ -114,6 +114,16 @@ const HasManyDissociator = require('../src/services/has-many-dissociator');
       fieldBad: { type: Sequelize.REAL }, // NOTICE: not supported yet.
     });
 
+    models.owner = sequelize.define('owner', {
+      name: { type: Sequelize.STRING },
+      ownerId: { type: Sequelize.INTEGER, allowNull: false, unique: true },
+    });
+
+    models.project = sequelize.define('project', {
+      name: { type: Sequelize.STRING },
+      ownerId: { type: Sequelize.INTEGER },
+    });
+
     models.address.belongsTo(models.user);
     models.addressWithUserAlias.belongsTo(models.user, { as: 'userAlias' });
     models.user.hasMany(models.address);
@@ -123,6 +133,20 @@ const HasManyDissociator = require('../src/services/has-many-dissociator');
     models.member.hasOne(models.membership);
     models.member.hasMany(models.friend);
     models.friend.belongsTo(models.member);
+    models.project.belongsTo(models.owner, {
+      foreignKey: {
+        name: 'ownerIdKey',
+        field: 'owner_id',
+      },
+      targetKey: 'ownerId',
+    });
+    models.owner.hasMany(models.project, {
+      foreignKey: {
+        name: 'ownerIdKey',
+        field: 'owner_id',
+      },
+      sourceKey: 'ownerId',
+    });
 
     Interface.Schemas = {
       schemas: {
@@ -270,6 +294,28 @@ const HasManyDissociator = require('../src/services/has-many-dissociator');
             { field: 'member', type: 'Number', reference: 'member.id' },
           ],
         },
+        owner: {
+          name: 'owner',
+          idField: 'id',
+          primaryKeys: ['id'],
+          isCompositePrimary: false,
+          fields: [
+            { field: 'id', type: 'Number' },
+            { field: 'name', type: 'STRING' },
+            { field: 'ownerId', type: 'Number' },
+          ],
+        },
+        project: {
+          name: 'project',
+          idField: 'id',
+          primaryKeys: ['id'],
+          isCompositePrimary: false,
+          fields: [
+            { field: 'id', type: 'Number' },
+            { field: 'name', type: 'STRING' },
+            { field: 'ownerId', type: 'Number', reference: 'owner.ownerId' },
+          ],
+        },
       },
     };
 
@@ -533,6 +579,55 @@ const HasManyDissociator = require('../src/services/has-many-dissociator');
         });
       });
 
+      describe('create a record on a collection with a foreign key non pointing to a primary key', () => {
+        it('should create a record', async () => {
+          expect.assertions(6);
+          const { models } = initializeSequelize();
+          try {
+            await new ResourceCreator(models.owner, {
+              id: 1,
+              name: 'foo',
+              ownerId: 3,
+            }).perform();
+            const result = await new ResourceCreator(models.project, {
+              id: 1,
+              name: 'bar',
+              owner: 1,
+            }).perform();
+
+            expect(result.id).toStrictEqual(1);
+            expect(result.name).toStrictEqual('bar');
+            expect(result.ownerIdKey).toStrictEqual(3);
+
+            const project = await models.project.findOne({ where: { name: 'bar' }, include: { model: models.owner, as: 'owner' } });
+            expect(project).not.toBeNull();
+            expect(project.owner.id).toStrictEqual(1);
+            expect(project.owner.ownerId).toStrictEqual(3);
+          } finally {
+            connectionManager.closeConnection();
+          }
+        });
+
+        it('should not create a record', async () => {
+          expect.assertions(1);
+          const { models } = initializeSequelize();
+          try {
+            await new ResourceCreator(models.owner, {
+              id: 2,
+              name: 'foo',
+              ownerId: 4,
+            }).perform();
+            await expect(new ResourceCreator(models.project, {
+              id: 1,
+              name: 'bar',
+              owner: 4,
+            }).perform()).rejects.toThrow(Error('related owner with pk 4 does not exist.'));
+          } finally {
+            connectionManager.closeConnection();
+          }
+        });
+      });
+
       describe('create a record on a collection with a composite primary key', () => {
         it('should create a record', async () => {
           expect.assertions(3);
@@ -658,6 +753,56 @@ const HasManyDissociator = require('../src/services/has-many-dissociator');
           }
         });
       });
+      describe('update a record on a collection with a foreign key non pointing to a primary key', () => {
+        it('should update a record', async () => {
+          expect.assertions(2);
+          const { models } = initializeSequelize();
+          try {
+            await new ResourceCreator(models.owner, {
+              id: 3,
+              name: 'foo3',
+              ownerId: 5,
+            }).perform();
+            const result = await new BelongsToUpdater(models.project, null, null, {
+              recordId: '1',
+              associationName: 'owner',
+            }, {
+              data: {
+                id: '3',
+                type: 'owner',
+              },
+            }).perform();
+
+            expect(result.id).toStrictEqual(1);
+            expect(result.ownerIdKey).toStrictEqual(5);
+          } finally {
+            connectionManager.closeConnection();
+          }
+        });
+
+        it('should not update a record', async () => {
+          expect.assertions(1);
+          const { models } = initializeSequelize();
+          try {
+            await new ResourceCreator(models.owner, {
+              id: 4,
+              name: 'foo4',
+              ownerId: 6,
+            }).perform();
+            await expect(new BelongsToUpdater(models.project, null, null, {
+              recordId: '1',
+              associationName: 'owner',
+            }, {
+              data: {
+                id: '6',
+                type: 'owner',
+              },
+            }).perform()).rejects.toThrow(Error('related owner with pk 6 does not exist.'));
+          } finally {
+            connectionManager.closeConnection();
+          }
+        });
+      });
     });
 
     describe('resources > resources getter', () => {
