improvement: improve sync method to not select all pivot rows

src/Orm/Relations/ManyToMany/QueryClient.ts

@@ -155,7 +155,7 @@ export class ManyToManyQueryClient implements ManyToManyClientContract<ManyToMan
    * Create and persist an instance of related model. Also makes the pivot table
    * entry to create the relationship
    */
-  public async create (values: ModelObject, checkExisting?: boolean): Promise<LucidRow> {
+  public async create (values: ModelObject, checkExisting: boolean = true): Promise<LucidRow> {
     return managedTransaction(this.parent.$trx || this.client, async (trx) => {
       this.parent.$trx = trx
       await this.parent.save()
@@ -183,7 +183,7 @@ export class ManyToManyQueryClient implements ManyToManyClientContract<ManyToMan
    * Create and persist multiple of instances of related model. Also makes
    * the pivot table entries to create the relationship.
    */
-  public async createMany (values: ModelObject[], checkExisting?: boolean): Promise<LucidRow[]> {
+  public async createMany (values: ModelObject[], checkExisting: boolean = true): Promise<LucidRow[]> {
     return managedTransaction(this.parent.$trx || this.client, async (trx) => {
       this.parent.$trx = trx
       await this.parent.save()
@@ -294,40 +294,31 @@ export class ManyToManyQueryClient implements ManyToManyClientContract<ManyToMan
         return result
       }, {}) : ids
 
-      /**
-       * We must scope the select query to related foreign key when ids
-       * is an array and not on object. Otherwise we select *.
-       */
       const query = this.pivotQuery().useTransaction(transaction)
 
       /**
        * We must scope the select query to related foreign key when ids
-       * is an array and not on object. This will help in performance
+       * is an array and not an object. This will help in performance
        * when their are indexes defined on this key
        */
       if (!hasAttributes) {
         query.select(this.relation.pivotRelatedForeignKey)
       }
 
-      /**
-       * Scope query to passed ids, when don't want to detach the missing one's
-       * in the current payload.
-       */
       const pivotRelatedForeignKeys = Object.keys(pivotRows)
-      if (!this.detach && pivotRelatedForeignKeys.length) {
-        query.whereIn(this.relation.pivotRelatedForeignKey, pivotRelatedForeignKeys)
-      }
 
       /**
        * Fetch existing pivot rows for the relationship
        */
-      const existingPivotRows = await query.exec()
+      const existingPivotRows = await query
+        .whereIn(this.relation.pivotRelatedForeignKey, pivotRelatedForeignKeys)
+        .exec()
 
       /**
        * Find a diff of rows being removed, added or updated in comparison
        * to the existing pivot rows.
        */
-      const { added, removed, updated } = syncDiff(existingPivotRows.reduce((result, row) => {
+      const { added, updated } = syncDiff(existingPivotRows.reduce((result, row) => {
         result[row[this.relation.pivotRelatedForeignKey]] = row
         return result
       }, {}), pivotRows)
@@ -361,9 +352,13 @@ export class ManyToManyQueryClient implements ManyToManyClientContract<ManyToMan
       }
 
       /**
-       * Detach the removed one's
+       * Detach everything except the synced ids
        */
-      await this.detach(Object.keys(removed), transaction)
+      await this
+        .pivotQuery()
+        .useTransaction(transaction)
+        .whereNotInPivot(this.relation.pivotRelatedForeignKey, pivotRelatedForeignKeys)
+        .del()
     })
   }
 }

src/utils/index.ts

@@ -97,7 +97,7 @@ export function unique (value: any[]) {
 export function syncDiff (original: ModelObject, incoming: ModelObject) {
   const diff = Object
     .keys(incoming)
-    .reduce<{ added: ModelObject, updated: ModelObject, removed: ModelObject }>((
+    .reduce<{ added: ModelObject, updated: ModelObject }>((
       result,
       incomingRowId,
     ) => {
@@ -119,17 +119,7 @@ export function syncDiff (original: ModelObject, incoming: ModelObject) {
       }
 
       return result
-    }, { added: {}, updated: {}, removed: {} })
-
-  /**
-   * Deleted rows
-   */
-  diff.removed = Object.keys(original).reduce((result, originalRowId) => {
-    if (!incoming[originalRowId]) {
-      result[originalRowId] = {}
-    }
-    return result
-  }, {})
+    }, { added: {}, updated: {} })
 
   return diff
 }

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

@@ -3093,6 +3093,7 @@ test.group('Model | ManyToMany | sync', (group) => {
 
     assert.equal(totalUsers[0].total, 1)
     assert.equal(totalSkills[0].total, 0)
+    assert.lengthOf(skillUsers, 2)
 
     assert.equal(skillUsers[0].id, 1)
     assert.equal(skillUsers[0].user_id, user.id)
@@ -3103,6 +3104,67 @@ test.group('Model | ManyToMany | sync', (group) => {
     assert.equal(skillUsers[1].skill_id, 1)
   })
 
+  test('keep duplicates of the id under sync', async (assert) => {
+    class Skill extends BaseModel {
+      @column({ isPrimary: true })
+      public id: number
+
+      @column()
+      public name: string
+    }
+
+    class User extends BaseModel {
+      @column({ isPrimary: true })
+      public id: number
+
+      @column()
+      public username: string
+
+      @manyToMany(() => Skill)
+      public skills: ManyToMany<typeof Skill>
+    }
+
+    const user = new User()
+    user.username = 'virk'
+    await user.save()
+
+    await db.insertQuery().table('skill_user').multiInsert([
+      {
+        user_id: user.id,
+        skill_id: 1,
+        proficiency: 'Beginner',
+      },
+      {
+        user_id: user.id,
+        skill_id: 2,
+        proficiency: 'Master',
+      },
+      {
+        user_id: user.id,
+        skill_id: 1,
+        proficiency: 'Master',
+      },
+    ])
+
+    await user.related('skills').sync([1])
+
+    const totalUsers = await db.query().from('users').count('*', 'total')
+    const totalSkills = await db.query().from('skills').count('*', 'total')
+    const skillUsers = await db.query().from('skill_user')
+
+    assert.equal(totalUsers[0].total, 1)
+    assert.equal(totalSkills[0].total, 0)
+    assert.lengthOf(skillUsers, 2)
+
+    assert.equal(skillUsers[0].id, 1)
+    assert.equal(skillUsers[0].user_id, user.id)
+    assert.equal(skillUsers[0].skill_id, 1)
+
+    assert.equal(skillUsers[1].id, 3)
+    assert.equal(skillUsers[1].user_id, user.id)
+    assert.equal(skillUsers[1].skill_id, 1)
+  })
+
   test('update pivot rows when additional properties are changed', async (assert) => {
     class Skill extends BaseModel {
       @column({ isPrimary: true })
@@ -3157,6 +3219,7 @@ test.group('Model | ManyToMany | sync', (group) => {
 
     assert.equal(totalUsers[0].total, 1)
     assert.equal(totalSkills[0].total, 0)
+    assert.lengthOf(skillUsers, 2)
 
     assert.equal(skillUsers[0].id, 1)
     assert.equal(skillUsers[0].user_id, user.id)
@@ -3166,6 +3229,7 @@ test.group('Model | ManyToMany | sync', (group) => {
     assert.equal(skillUsers[1].id, 3)
     assert.equal(skillUsers[1].user_id, 2)
     assert.equal(skillUsers[1].skill_id, 1)
+    assert.equal(skillUsers[1].proficiency, 'Master')
   })
 
   test('do not update pivot row when no extra properties are defined', async (assert) => {
@@ -3218,6 +3282,7 @@ test.group('Model | ManyToMany | sync', (group) => {
 
     assert.equal(totalUsers[0].total, 1)
     assert.equal(totalSkills[0].total, 0)
+    assert.lengthOf(skillUsers, 2)
 
     assert.equal(skillUsers[0].id, 1)
     assert.equal(skillUsers[0].user_id, user.id)
@@ -3227,6 +3292,7 @@ test.group('Model | ManyToMany | sync', (group) => {
     assert.equal(skillUsers[1].id, 3)
     assert.equal(skillUsers[1].user_id, 2)
     assert.equal(skillUsers[1].skill_id, 1)
+    assert.equal(skillUsers[1].proficiency, 'Master')
   })
 
   test('do not remove rows when detach = false', async (assert) => {
@@ -3297,6 +3363,74 @@ test.group('Model | ManyToMany | sync', (group) => {
     assert.equal(skillUsers[2].proficiency, 'Master')
   })
 
+  test('do not remove rows when nothing has changed', async (assert) => {
+    class Skill extends BaseModel {
+      @column({ isPrimary: true })
+      public id: number
+
+      @column()
+      public name: string
+    }
+
+    class User extends BaseModel {
+      @column({ isPrimary: true })
+      public id: number
+
+      @column()
+      public username: string
+
+      @manyToMany(() => Skill)
+      public skills: ManyToMany<typeof Skill>
+    }
+
+    const user = new User()
+    user.username = 'virk'
+    await user.save()
+
+    await db.insertQuery().table('skill_user').multiInsert([
+      {
+        user_id: user.id,
+        skill_id: 1,
+        proficiency: 'Beginner',
+      },
+      {
+        user_id: user.id,
+        skill_id: 2,
+        proficiency: 'Master',
+      },
+      {
+        user_id: 2,
+        skill_id: 1,
+        proficiency: 'Master',
+      },
+    ])
+
+    await user.related('skills').sync([1, 2])
+
+    const totalUsers = await db.query().from('users').count('*', 'total')
+    const totalSkills = await db.query().from('skills').count('*', 'total')
+    const skillUsers = await db.query().from('skill_user')
+
+    assert.equal(totalUsers[0].total, 1)
+    assert.equal(totalSkills[0].total, 0)
+    assert.lengthOf(skillUsers, 3)
+
+    assert.equal(skillUsers[0].id, 1)
+    assert.equal(skillUsers[0].user_id, user.id)
+    assert.equal(skillUsers[0].skill_id, 1)
+    assert.equal(skillUsers[0].proficiency, 'Beginner')
+
+    assert.equal(skillUsers[1].id, 2)
+    assert.equal(skillUsers[1].user_id, user.id)
+    assert.equal(skillUsers[1].skill_id, 2)
+    assert.equal(skillUsers[1].proficiency, 'Master')
+
+    assert.equal(skillUsers[2].id, 3)
+    assert.equal(skillUsers[2].user_id, 2)
+    assert.equal(skillUsers[2].skill_id, 1)
+    assert.equal(skillUsers[2].proficiency, 'Master')
+  })
+
   test('use custom transaction', async (assert) => {
     class Skill extends BaseModel {
       @column({ isPrimary: true })

test/utils.spec.ts

@@ -33,7 +33,6 @@ test.group('Utils | syncDiff', () => {
     assert.deepEqual(diff, {
       added: { 2: {}, 3: {} },
       updated: {},
-      removed: {},
     })
   })
 
@@ -61,7 +60,6 @@ test.group('Utils | syncDiff', () => {
       updated: {
         1: { score: 4 },
       },
-      removed: {},
     })
   })
 
@@ -94,43 +92,6 @@ test.group('Utils | syncDiff', () => {
         3: { score: 4 },
       },
       updated: {},
-      removed: {},
-    })
-  })
-
-  test('return rows to be removed', (assert) => {
-    const dbRows = {
-      1: {
-        id: '1',
-        user_id: '1',
-        skill_id: '1',
-        score: 1,
-      },
-      2: {
-        id: '2',
-        user_id: '1',
-        skill_id: '2',
-        score: 1,
-      },
-    }
-
-    const idsToSync = {
-      1: {
-        score: 1,
-      },
-      3: {
-        score: 4,
-      },
-    }
-
-    const diff = syncDiff(dbRows, idsToSync)
-    assert.deepEqual(diff, {
-      added: {
-        3: { score: 4 },
-      },
-      updated: {
-      },
-      removed: { 2: {} },
     })
   })
 })