fix(manyToMany): attach method now updates pivot rows when attributes…

… are defined

src/Orm/Relations/ManyToMany/QueryBuilder.ts

@@ -14,7 +14,7 @@ import { ModelContract, ManyToManyQueryBuilderContract } from '@ioc:Adonis/Lucid
 import { QueryClientContract, TransactionClientContract } from '@ioc:Adonis/Lucid/Database'
 
 import { ManyToMany } from './index'
-import { unique, difference } from '../../../utils'
+import { unique, syncDiff } from '../../../utils'
 import { BaseRelationQueryBuilder } from '../Base/QueryBuilder'
 
 /**
@@ -374,8 +374,16 @@ export class ManyToManyQueryBuilder
     ids: (string | number)[] | { [key: string]: any },
     checkExisting: boolean,
   ) {
-    let idsList = unique(Array.isArray(ids) ? ids : Object.keys(ids))
+    const pivotFKValue = this.$getRelatedValue(parent, this._relation.localKey, 'attach')
+
     const hasAttributes = !Array.isArray(ids)
+    const idsList = hasAttributes ? Object.keys(ids) : ids as string[]
+
+    /**
+     * Initial diff has all the ids under the insert array. If `checkExisting = true`
+     * then we will re-compute the diff from the existing database rows.
+     */
+    let diff: { update: any[], insert: any[] } = { update: [], insert: idsList }
 
     /**
      * Pull existing pivot rows when `checkExisting = true` and persist only
@@ -385,38 +393,51 @@ export class ManyToManyQueryBuilder
       const existingRows = await client
         .query()
         .from(this._relation.pivotTable)
-        .select(this._relation.pivotRelatedForeignKey)
         .whereIn(this._relation.pivotRelatedForeignKey, idsList)
-        .where(
-          this._relation.pivotForeignKey,
-          this.$getRelatedValue(parent, this._relation.localKey, 'attach'),
-        )
+        .where(this._relation.pivotForeignKey, pivotFKValue)
 
-      const existingIds = existingRows.map((row) => row[this._relation.pivotRelatedForeignKey])
-      idsList = difference(idsList, existingIds)
+      /**
+       * Computing the diff using the existing database rows
+       */
+      diff = syncDiff(existingRows, ids, (rows, forId) => {
+        /* eslint eqeqeq: "off" */
+        return rows.find((row) => row[this._relation.pivotRelatedForeignKey] == forId)
+      })
     }
 
     /**
-     * Ignore when there is nothing to insert
+     * Update rows where attributes have changed. The query is exactly the
+     * same as the above fetch query with two changes.
+     *
+     * 1. Performing an updating, instead of select
+     * 2. Instead of whereIn on the `pivotRelatedForeignKey`, we are using `where`
+     *    cause of the nature of the update action.
      */
-    if (!idsList.length) {
-      return
+    if (diff.update.length) {
+      await Promise.all(unique(diff.update).map((id) => {
+        return client
+          .query()
+          .from(this._relation.pivotTable)
+          .where(this._relation.pivotForeignKey, pivotFKValue)
+          .where(this._relation.pivotRelatedForeignKey, id)
+          .update(ids[id])
+      }))
     }
 
     /**
      * Perform multiple inserts in one go
      */
-    await client
-      .insertQuery()
-      .table(this._relation.pivotTable)
-      .multiInsert(idsList.map((id) => {
-        const payload = {
-          [this._relation.pivotForeignKey]: this.$getRelatedValue(parent, this._relation.localKey),
-          [this._relation.pivotRelatedForeignKey]: id,
-        }
-
-        return hasAttributes ? Object.assign(payload, ids[id]) : payload
-      }))
+    if (diff.insert.length) {
+      await client
+        .insertQuery()
+        .table(this._relation.pivotTable)
+        .multiInsert(unique(diff.insert).map((id) => {
+          return Object.assign({}, hasAttributes ? ids[id] : {}, {
+            [this._relation.pivotForeignKey]: pivotFKValue,
+            [this._relation.pivotRelatedForeignKey]: id,
+          })
+        }))
+    }
   }
 
   /**

src/utils/index.ts

@@ -49,17 +49,27 @@ export function getValue (
   return value
 }
 
+/**
+ * Helper to find if value is a valid Object or
+ * not
+ */
 export function isObject (value: any): boolean {
   return value !== null && typeof (value) === 'object' && !Array.isArray(value)
 }
 
+/**
+ * Drops duplicate values from an array
+ */
 export function unique (value: any[]) {
   if (!Array.isArray(value)) {
     return []
   }
   return [...new Set(value)]
 }
 
+/**
+ * Finds the diff between 2 arrays
+ */
 export function difference (main: any[], other: []) {
   return [main, other].reduce((a, b) => {
     return a.filter(c => {
@@ -69,6 +79,67 @@ export function difference (main: any[], other: []) {
   })
 }
 
+/**
+ * A helper to know file ends with a script file
+ * extension or not
+ */
 export function isJavaScriptFile (file: string) {
   return ['.js', '.ts'].includes(extname(file))
 }
+
+/**
+ * Returns a diff of rows to be updated or inserted when performing
+ * a many to many `attach`
+ */
+export function syncDiff (
+  dbRows: any[],
+  attributesToSync: any[] | { [key: string]: any },
+  rowIdResolver: (rows: any, forId: string) => any,
+) {
+  /**
+   * When attributes to sync are not defined as an array. Then we expect it
+   * to be an object
+   */
+  const hasExtraAttributes = !Array.isArray(attributesToSync)
+
+  /**
+   * An array of ids we want to sync
+   */
+  const idsToSync = (hasExtraAttributes ? Object.keys(attributesToSync) : attributesToSync) as string[]
+
+  return idsToSync.reduce((result: { insert: any[], update: any[] }, id) => {
+    /**
+     * Find the matching row for the given id
+     */
+    const matchingRow = rowIdResolver(dbRows, id)
+
+    /**
+     * When there isn't any matching row, we need to insert
+     * the id
+     */
+    if (!matchingRow) {
+      result.insert.push(id)
+      return result
+    }
+
+    /**
+     * When there aren't any extra attributes to check, we skip the
+     * given id, since it already exists.
+     */
+    if (!hasExtraAttributes) {
+      return result
+    }
+
+    /**
+     * When one or more attributes inside the update payload are different
+     * from the actual row, then we perform an update
+     */
+    const attributes = attributesToSync[id]
+    /* eslint eqeqeq: "off" */
+    if (Object.keys(attributes).find((key) => matchingRow[key] != attributes[key])) {
+      result.update.push(id)
+    }
+
+    return result
+  }, { insert: [], update: [] })
+}

test/orm/model-many-to-many.spec.ts

@@ -3184,7 +3184,7 @@ test.group('Model | ManyToMany | sync', (group) => {
     assert.equal(skillUsersAfterSync[0].skill_id, 2)
   })
 
-  test('insert new ids metioned in sync', async (assert) => {
+  test('insert new ids mentioned in sync', async (assert) => {
     class Skill extends BaseModel {
       @column({ primary: true })
       public id: number
@@ -3264,10 +3264,61 @@ test.group('Model | ManyToMany | sync', (group) => {
     assert.equal(skillUsers[0].id, skillUsersAfterSync[0].id)
     assert.equal(skillUsersAfterSync[0].user_id, user.id)
     assert.equal(skillUsersAfterSync[0].skill_id, 1)
-    assert.isNull(skillUsersAfterSync[0].proficiency)
+    assert.equal(skillUsersAfterSync[0].proficiency, 'master')
 
     assert.equal(skillUsersAfterSync[1].user_id, user.id)
     assert.equal(skillUsersAfterSync[1].skill_id, 2)
     assert.equal(skillUsersAfterSync[1].proficiency, 'beginner')
   })
+
+  test('sync update extra properties when rows are same', async (assert) => {
+    class Skill extends BaseModel {
+      @column({ primary: true })
+      public id: number
+
+      @column()
+      public name: string
+    }
+
+    class User extends BaseModel {
+      @column({ primary: true })
+      public id: number
+
+      @column()
+      public username: string
+
+      @manyToMany(() => Skill)
+      public skills: Skill[]
+    }
+
+    const user = new User()
+    user.username = 'virk'
+    await user.save()
+
+    await user.related<'manyToMany', 'skills'>('skills').attach([1])
+    const skillUsers = await db.query().from('skill_user')
+
+    await user.related<'manyToMany', 'skills'>('skills').sync({
+      1: { proficiency: 'master' },
+      2: { proficiency: 'beginner' },
+    })
+
+    await user.related<'manyToMany', 'skills'>('skills').sync({
+      1: { proficiency: 'master' },
+      2: { proficiency: 'intermediate' },
+    })
+    const skillUsersAfterSync = await db.query().from('skill_user')
+
+    assert.lengthOf(skillUsers, 1)
+    assert.lengthOf(skillUsersAfterSync, 2)
+
+    assert.equal(skillUsers[0].id, skillUsersAfterSync[0].id)
+    assert.equal(skillUsersAfterSync[0].user_id, user.id)
+    assert.equal(skillUsersAfterSync[0].skill_id, 1)
+    assert.equal(skillUsersAfterSync[0].proficiency, 'master')
+
+    assert.equal(skillUsersAfterSync[1].user_id, user.id)
+    assert.equal(skillUsersAfterSync[1].skill_id, 2)
+    assert.equal(skillUsersAfterSync[1].proficiency, 'intermediate')
+  })
 })

test/utils.spec.ts

@@ -0,0 +1,95 @@
+/*
+* @adonisjs/lucid
+*
+* (c) Harminder Virk <[email protected]>
+*
+* For the full copyright and license information, please view the LICENSE
+* file that was distributed with this source code.
+*/
+
+/// <reference path="../adonis-typings/index.ts" />
+
+import test from 'japa'
+import { syncDiff } from '../src/utils'
+
+test.group('Utils | attachDiff', () => {
+  test('return ids to be inserted', (assert) => {
+    const dbRows = [{
+      id: '1',
+      user_id: '1',
+      skill_id: '1',
+      score: 1,
+    }]
+
+    const idsToSync = ['1', '2', '3']
+
+    const diff = syncDiff(dbRows, idsToSync, (rows, id) => rows.find(({ skill_id }) => skill_id === id))
+    assert.deepEqual(diff, { insert: ['2', '3'], update: [] })
+  })
+
+  test('return ids when ids to sync are represented as an object', (assert) => {
+    const dbRows = [{
+      id: '1',
+      user_id: '1',
+      skill_id: '1',
+      score: 1,
+    }]
+
+    const idsToSync = {
+      '1': {},
+      '2': {},
+      '3': {},
+    }
+
+    const diff = syncDiff(dbRows, idsToSync, (rows, id) => rows.find(({ skill_id }) => skill_id === id))
+    assert.deepEqual(diff, { insert: ['2', '3'], update: [] })
+  })
+
+  test('return ids to be updated when attributes are different', (assert) => {
+    const dbRows = [{
+      id: '1',
+      user_id: '1',
+      skill_id: '1',
+      score: 1,
+    }]
+
+    const idsToSync = {
+      '1': {
+        score: 4,
+      },
+      '2': {
+        score: 2,
+      },
+      '3': {
+        score: 1,
+      },
+    }
+
+    const diff = syncDiff(dbRows, idsToSync, (rows, id) => rows.find(({ skill_id }) => skill_id === id))
+    assert.deepEqual(diff, { insert: ['2', '3'], update: ['1'] })
+  })
+
+  test('ignore rows whose attributes value is same', (assert) => {
+    const dbRows = [{
+      id: '1',
+      user_id: '1',
+      skill_id: '1',
+      score: 1,
+    }]
+
+    const idsToSync = {
+      '1': {
+        score: 1,
+      },
+      '2': {
+        score: 2,
+      },
+      '3': {
+        score: 1,
+      },
+    }
+
+    const diff = syncDiff(dbRows, idsToSync, (rows, id) => rows.find(({ skill_id }) => skill_id === id))
+    assert.deepEqual(diff, { insert: ['2', '3'], update: [] })
+  })
+})