From 1824990980ddc84a0fcb293a787221e906aae005 Mon Sep 17 00:00:00 2001 From: shaezard <47674815+shaezard@users.noreply.github.com> Date: Thu, 22 Jan 2026 13:32:48 -0500 Subject: [PATCH] fix: add teamID/userUid filter to updateMany queries, Fixed Row level locking to prevent deadlocks and achieve ~100x performance improvement (#5647) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: add teamID/userUid filter to updateMany queries Prevents cross-user/cross-team orderIndex corruption * fix: fix orderIndex of existing collections * feat(backend): add cascade delete for collections - Add onDelete: Cascade to TeamCollection parent relationship - Add onDelete: Cascade to UserRequest → UserCollection relationship - Remove manual recursive deleteCollectionData methods - Simplify deleteUserCollection and deleteTeamCollection services - Add Prisma migration for cascade delete foreign keys Resolves #5654 * refactor(team-collection): remove manual deleteCollectionData method Resolves #5654 * fix(backend): fixed locking mechanisms for collections and requests - User/Team Collection/Requests Resolves #5666 --------- Co-authored-by: Abdur Rahman Daanish --- .../migration.sql | 30 + .../migration.sql | 11 + .../hoppscotch-backend/prisma/schema.prisma | 4 +- .../src/prisma/prisma.service.ts | 85 ++- .../team-collection.service.spec.ts | 548 ++++++++++++++- .../team-collection.service.ts | 451 ++++++------- .../team-request/team-request.service.spec.ts | 4 +- .../src/team-request/team-request.service.ts | 147 ++-- .../user-collection.service.spec.ts | 632 +++++++++++++++++- .../user-collection.service.ts | 495 +++++++------- .../src/user-request/user-request.service.ts | 144 ++-- 11 files changed, 1827 insertions(+), 724 deletions(-) create mode 100644 packages/hoppscotch-backend/prisma/migrations/20251202154928_fix_order_index/migration.sql create mode 100644 packages/hoppscotch-backend/prisma/migrations/20251203202452_cascade_delete/migration.sql diff --git a/packages/hoppscotch-backend/prisma/migrations/20251202154928_fix_order_index/migration.sql b/packages/hoppscotch-backend/prisma/migrations/20251202154928_fix_order_index/migration.sql new file mode 100644 index 00000000..27e35d29 --- /dev/null +++ b/packages/hoppscotch-backend/prisma/migrations/20251202154928_fix_order_index/migration.sql @@ -0,0 +1,30 @@ +-- Recalculate orderIndex for UserCollection per (parentID) +WITH ordered AS ( + SELECT + id, + ROW_NUMBER() OVER ( + PARTITION BY "userUid", "parentID" + ORDER BY "orderIndex" ASC, "createdOn" ASC, id ASC + ) AS new_index + FROM "UserCollection" +) +UPDATE "UserCollection" uc +SET "orderIndex" = o.new_index +FROM ordered o +WHERE uc.id = o.id; + +-- Recalculate orderIndex for TeamCollection per (teamID, parentID) +WITH ordered AS ( + SELECT + id, + ROW_NUMBER() OVER ( + PARTITION BY "teamID", "parentID" + ORDER BY "orderIndex" ASC, "createdOn" ASC, id ASC + ) AS new_index + FROM "TeamCollection" +) +UPDATE "TeamCollection" tc +SET "orderIndex" = o.new_index +FROM ordered o +WHERE tc.id = o.id; + diff --git a/packages/hoppscotch-backend/prisma/migrations/20251203202452_cascade_delete/migration.sql b/packages/hoppscotch-backend/prisma/migrations/20251203202452_cascade_delete/migration.sql new file mode 100644 index 00000000..894af9e3 --- /dev/null +++ b/packages/hoppscotch-backend/prisma/migrations/20251203202452_cascade_delete/migration.sql @@ -0,0 +1,11 @@ +-- DropForeignKey +ALTER TABLE "TeamCollection" DROP CONSTRAINT "TeamCollection_parentID_fkey"; + +-- DropForeignKey +ALTER TABLE "UserRequest" DROP CONSTRAINT "UserRequest_collectionID_fkey"; + +-- AddForeignKey +ALTER TABLE "TeamCollection" ADD CONSTRAINT "TeamCollection_parentID_fkey" FOREIGN KEY ("parentID") REFERENCES "TeamCollection"("id") ON DELETE CASCADE ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "UserRequest" ADD CONSTRAINT "UserRequest_collectionID_fkey" FOREIGN KEY ("collectionID") REFERENCES "UserCollection"("id") ON DELETE CASCADE ON UPDATE CASCADE; diff --git a/packages/hoppscotch-backend/prisma/schema.prisma b/packages/hoppscotch-backend/prisma/schema.prisma index f77d6323..3fb074bb 100644 --- a/packages/hoppscotch-backend/prisma/schema.prisma +++ b/packages/hoppscotch-backend/prisma/schema.prisma @@ -48,7 +48,7 @@ model TeamCollection { createdOn DateTime @default(now()) @db.Timestamptz(3) updatedOn DateTime @updatedAt @db.Timestamptz(3) data Json? - parent TeamCollection? @relation("TeamCollectionChildParent", fields: [parentID], references: [id]) + parent TeamCollection? @relation("TeamCollectionChildParent", fields: [parentID], references: [id], onDelete: Cascade) children TeamCollection[] @relation("TeamCollectionChildParent") team Team @relation(fields: [teamID], references: [id], onDelete: Cascade) requests TeamRequest[] @@ -188,7 +188,7 @@ model UserRequest { orderIndex Int createdOn DateTime @default(now()) @db.Timestamptz(3) updatedOn DateTime @updatedAt @db.Timestamptz(3) - userCollection UserCollection @relation(fields: [collectionID], references: [id]) + userCollection UserCollection @relation(fields: [collectionID], references: [id], onDelete: Cascade) user User @relation(fields: [userUid], references: [uid], onDelete: Cascade) @@unique([userUid, collectionID, orderIndex]) diff --git a/packages/hoppscotch-backend/src/prisma/prisma.service.ts b/packages/hoppscotch-backend/src/prisma/prisma.service.ts index 58c50baf..fe871924 100644 --- a/packages/hoppscotch-backend/src/prisma/prisma.service.ts +++ b/packages/hoppscotch-backend/src/prisma/prisma.service.ts @@ -145,61 +145,54 @@ export class PrismaService } /** - * Centralized Lock Manager - * Locks UserCollections first, then UserRequests and same goes for TeamCollections and then TeamRequests - * - * For UserCollection -> userUid, parentID (nullable) - * For UserRequest -> collectionIDs - * For TeamCollection -> parentID (nullable) - * For TeamRequest -> collectionIDs + * Locks rows in TeamCollection for a specific teamId and parentID. */ - async acquireLocks( + async lockTeamCollectionByTeamAndParent( tx: Prisma.TransactionClient, - table: 'UserCollection' | 'UserRequest' | 'TeamCollection' | 'TeamRequest', - userUid: string, + teamId: string, parentID: string | null, - collectionIDs: string[] = [], ) { - if (table === 'UserCollection' && userUid) { - const lockQuery = parentID - ? Prisma.sql`SELECT "orderIndex" FROM "UserCollection" WHERE "userUid" = ${userUid} AND "parentID" = ${parentID} FOR UPDATE` - : Prisma.sql`SELECT "orderIndex" FROM "UserCollection" WHERE "userUid" = ${userUid} AND "parentID" IS NULL FOR UPDATE`; - return tx.$executeRaw(lockQuery); - } - - if (table === 'UserRequest' && collectionIDs.length > 0) { - collectionIDs = collectionIDs.filter(Boolean).sort(); - const lockQuery = Prisma.sql`SELECT "orderIndex" FROM "UserRequest" WHERE "collectionID" IN (${Prisma.join(collectionIDs)}) FOR UPDATE`; - return tx.$executeRaw(lockQuery); - } - - if (table === 'TeamCollection') { - const lockQuery = parentID - ? Prisma.sql`SELECT "orderIndex" FROM "TeamCollection" WHERE "parentID" = ${parentID} FOR UPDATE` - : Prisma.sql`SELECT "orderIndex" FROM "TeamCollection" WHERE "parentID" IS NULL FOR UPDATE`; - return tx.$executeRaw(lockQuery); - } - - if (table === 'TeamRequest' && collectionIDs.length > 0) { - collectionIDs = collectionIDs.filter(Boolean).sort(); - const lockQuery = Prisma.sql`SELECT "orderIndex" FROM "TeamRequest" WHERE "collectionID" IN (${Prisma.join(collectionIDs)}) FOR UPDATE`; - return tx.$executeRaw(lockQuery); - } + const lockQuery = parentID + ? Prisma.sql`SELECT "orderIndex" FROM "TeamCollection" WHERE "teamID" = ${teamId} AND "parentID" = ${parentID} FOR UPDATE` + : Prisma.sql`SELECT "orderIndex" FROM "TeamCollection" WHERE "teamID" = ${teamId} AND "parentID" IS NULL FOR UPDATE`; + return tx.$executeRaw(lockQuery); } /** - * Table-level lock + * Locks rows in TeamRequest for specific teamID and collectionIDs. */ - async lockTableExclusive( + async lockTeamRequestByCollections( tx: Prisma.TransactionClient, - tableName: - | 'UserCollection' - | 'UserRequest' - | 'TeamCollection' - | 'TeamRequest', + teamID: string, + collectionIDs: string[], ) { - return tx.$executeRaw( - Prisma.sql`LOCK TABLE ${Prisma.raw(`"${tableName}"`)} IN EXCLUSIVE MODE`, - ); + const lockQuery = Prisma.sql`SELECT "orderIndex" FROM "TeamRequest" WHERE "teamID" = ${teamID} AND "collectionID" IN (${Prisma.join(collectionIDs)}) FOR UPDATE`; + return tx.$executeRaw(lockQuery); + } + + /** + * Locks rows in UserCollection for a specific userUid and parentID. + */ + async lockUserCollectionByParent( + tx: Prisma.TransactionClient, + userUid: string, + parentID: string | null, + ) { + const lockQuery = parentID + ? Prisma.sql`SELECT "orderIndex" FROM "UserCollection" WHERE "userUid" = ${userUid} AND "parentID" = ${parentID} FOR UPDATE` + : Prisma.sql`SELECT "orderIndex" FROM "UserCollection" WHERE "userUid" = ${userUid} AND "parentID" IS NULL FOR UPDATE`; + return tx.$executeRaw(lockQuery); + } + + /** + * Locks rows in UserRequest for specific userUid and collectionIDs. + */ + async lockUserRequestByCollections( + tx: Prisma.TransactionClient, + userUid: string, + collectionIDs: string[] = [], + ) { + const lockQuery = Prisma.sql`SELECT "orderIndex" FROM "UserRequest" WHERE "userUid" = ${userUid} AND "collectionID" IN (${Prisma.join(collectionIDs)}) FOR UPDATE`; + return tx.$executeRaw(lockQuery); } } diff --git a/packages/hoppscotch-backend/src/team-collection/team-collection.service.spec.ts b/packages/hoppscotch-backend/src/team-collection/team-collection.service.spec.ts index fa2fd22f..92c6cf9a 100644 --- a/packages/hoppscotch-backend/src/team-collection/team-collection.service.spec.ts +++ b/packages/hoppscotch-backend/src/team-collection/team-collection.service.spec.ts @@ -888,12 +888,12 @@ describe('deleteCollection', () => { expect(result).toEqualLeft(TEAM_COLL_NOT_FOUND); }); - test('should throw TEAM_COLL_NOT_FOUND when collectionID is invalid when deleting TeamCollection from UserCollectionTable ', async () => { + test('should throw TEAM_COL_REORDERING_FAILED when deleteCollectionAndUpdateSiblingsOrderIndex fails', async () => { jest .spyOn(teamCollectionService, 'getCollection') .mockResolvedValueOnce(E.right(rootTeamCollection)); jest - .spyOn(teamCollectionService as any, 'deleteCollectionData') + .spyOn(teamCollectionService as any, 'deleteCollectionAndUpdateSiblingsOrderIndex') .mockResolvedValueOnce(E.left(TEAM_COL_REORDERING_FAILED)); const result = await teamCollectionService.deleteCollection( @@ -929,6 +929,8 @@ describe('deleteCollection', () => { describe('moveCollection', () => { test('should throw TEAM_COLL_NOT_FOUND if collectionID is invalid', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') .mockResolvedValueOnce(E.left(TEAM_COLL_NOT_FOUND)); @@ -938,6 +940,8 @@ describe('moveCollection', () => { }); test('should throw TEAM_COLL_DEST_SAME if collectionID and destCollectionID is the same', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') .mockResolvedValueOnce(E.right(rootTeamCollection)); @@ -950,11 +954,11 @@ describe('moveCollection', () => { }); test('should throw TEAM_COLL_NOT_FOUND if destCollectionID is invalid', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') - .mockResolvedValueOnce(E.right(rootTeamCollection)); - jest - .spyOn(teamCollectionService, 'getCollection') + .mockResolvedValueOnce(E.right(rootTeamCollection)) .mockResolvedValueOnce(E.left(TEAM_COLL_NOT_FOUND)); const result = await teamCollectionService.moveCollection( @@ -965,11 +969,11 @@ describe('moveCollection', () => { }); test('should throw TEAM_COLL_NOT_SAME_TEAM if collectionID and destCollectionID are not from the same team', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') - .mockResolvedValueOnce(E.right(rootTeamCollection)); - jest - .spyOn(teamCollectionService, 'getCollection') + .mockResolvedValueOnce(E.right(rootTeamCollection)) .mockResolvedValueOnce( E.right({ ...childTeamCollection_2, teamID: 'anotherTeamID' }), ); @@ -982,11 +986,11 @@ describe('moveCollection', () => { }); test('should throw TEAM_COLL_IS_PARENT_COLL if collectionID is parent of destCollectionID ', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') - .mockResolvedValueOnce(E.right(rootTeamCollection)); - jest - .spyOn(teamCollectionService, 'getCollection') + .mockResolvedValueOnce(E.right(rootTeamCollection)) .mockResolvedValueOnce(E.right(childTeamCollection)); const result = await teamCollectionService.moveCollection( @@ -997,6 +1001,8 @@ describe('moveCollection', () => { }); test('should throw TEAM_COL_ALREADY_ROOT when moving root TeamCollection to root', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') .mockResolvedValueOnce(E.right(rootTeamCollection)); @@ -1009,6 +1015,8 @@ describe('moveCollection', () => { }); test('should successfully move a child TeamCollection into root', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') .mockResolvedValueOnce(E.right(childTeamCollection)); @@ -1029,6 +1037,8 @@ describe('moveCollection', () => { }); test('should throw TEAM_COLL_NOT_FOUND when trying to change parent of collection with invalid collectionID', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') .mockResolvedValueOnce(E.right(childTeamCollection)); @@ -1044,6 +1054,8 @@ describe('moveCollection', () => { }); test('should send pubsub message to "team_coll//coll_moved" when a child TeamCollection is moved to root successfully', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') .mockResolvedValueOnce(E.right(childTeamCollection)); @@ -1064,11 +1076,11 @@ describe('moveCollection', () => { }); test('should successfully move a root TeamCollection into a child TeamCollection', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') - .mockResolvedValueOnce(E.right(rootTeamCollection)); - jest - .spyOn(teamCollectionService, 'getCollection') + .mockResolvedValueOnce(E.right(rootTeamCollection)) .mockResolvedValueOnce(E.right(childTeamCollection)); jest .spyOn(teamCollectionService as any, 'isParent') @@ -1093,11 +1105,11 @@ describe('moveCollection', () => { }); test('should send pubsub message to "team_coll//coll_moved" when root TeamCollection is moved into another child TeamCollection successfully', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') - .mockResolvedValueOnce(E.right(rootTeamCollection)); - jest - .spyOn(teamCollectionService, 'getCollection') + .mockResolvedValueOnce(E.right(rootTeamCollection)) .mockResolvedValueOnce(E.right(childTeamCollection)); jest .spyOn(teamCollectionService as any, 'isParent') @@ -1126,11 +1138,11 @@ describe('moveCollection', () => { }); test('should successfully move a child TeamCollection into another child TeamCollection', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') - .mockResolvedValueOnce(E.right(childTeamCollection)); - jest - .spyOn(teamCollectionService, 'getCollection') + .mockResolvedValueOnce(E.right(childTeamCollection)) .mockResolvedValueOnce(E.right(childTeamCollection_2)); jest .spyOn(teamCollectionService as any, 'isParent') @@ -1155,11 +1167,11 @@ describe('moveCollection', () => { }); test('should send pubsub message to "team_coll//coll_moved" when child TeamCollection is moved into another child TeamCollection successfully', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); jest .spyOn(teamCollectionService, 'getCollection') - .mockResolvedValueOnce(E.right(childTeamCollection)); - jest - .spyOn(teamCollectionService, 'getCollection') + .mockResolvedValueOnce(E.right(childTeamCollection)) .mockResolvedValueOnce(E.right(childTeamCollection_2)); jest .spyOn(teamCollectionService as any, 'isParent') @@ -1214,7 +1226,15 @@ describe('updateCollectionOrder', () => { mockPrisma.teamCollection.findUniqueOrThrow.mockResolvedValueOnce( childTeamCollectionList[4], ); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst.mockResolvedValueOnce( + childTeamCollectionList[4], + ); mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 10 }); + mockPrisma.teamCollection.count.mockResolvedValueOnce( + childTeamCollectionList.length, + ); mockPrisma.teamCollection.update.mockResolvedValueOnce({ ...childTeamCollectionList[4], orderIndex: childTeamCollectionList.length, @@ -1232,7 +1252,15 @@ describe('updateCollectionOrder', () => { mockPrisma.teamCollection.findUniqueOrThrow.mockResolvedValueOnce( rootTeamCollectionList[4], ); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst.mockResolvedValueOnce( + rootTeamCollectionList[4], + ); mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 10 }); + mockPrisma.teamCollection.count.mockResolvedValueOnce( + rootTeamCollectionList.length, + ); mockPrisma.teamCollection.update.mockResolvedValueOnce({ ...rootTeamCollectionList[4], orderIndex: rootTeamCollectionList.length, @@ -1264,7 +1292,15 @@ describe('updateCollectionOrder', () => { mockPrisma.teamCollection.findUniqueOrThrow.mockResolvedValueOnce( rootTeamCollectionList[4], ); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst.mockResolvedValueOnce( + rootTeamCollectionList[4], + ); mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 10 }); + mockPrisma.teamCollection.count.mockResolvedValueOnce( + rootTeamCollectionList.length, + ); mockPrisma.teamCollection.update.mockResolvedValueOnce({ ...rootTeamCollectionList[4], orderIndex: rootTeamCollectionList.length, @@ -1275,7 +1311,7 @@ describe('updateCollectionOrder', () => { null, ); expect(mockPubSub.publish).toHaveBeenCalledWith( - `team_coll/${childTeamCollectionList[4].teamID}/coll_order_updated`, + `team_coll/${rootTeamCollectionList[4].teamID}/coll_order_updated`, { collection: rootTeamCollectionListCasted[4], nextCollection: null, @@ -1304,6 +1340,11 @@ describe('updateCollectionOrder', () => { mockPrisma.teamCollection.findUniqueOrThrow .mockResolvedValueOnce(childTeamCollectionList[4]) .mockResolvedValueOnce(childTeamCollectionList[2]); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst + .mockResolvedValueOnce(childTeamCollectionList[4]) + .mockResolvedValueOnce(childTeamCollectionList[2]); mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 3 }); mockPrisma.teamCollection.update.mockResolvedValueOnce({ ...childTeamCollectionList[4], @@ -1322,6 +1363,16 @@ describe('updateCollectionOrder', () => { mockPrisma.teamCollection.findUniqueOrThrow .mockResolvedValueOnce(rootTeamCollectionList[4]) .mockResolvedValueOnce(rootTeamCollectionList[2]); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst + .mockResolvedValueOnce(rootTeamCollectionList[4]) + .mockResolvedValueOnce(rootTeamCollectionList[2]); + mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 3 }); + mockPrisma.teamCollection.update.mockResolvedValueOnce({ + ...rootTeamCollectionList[4], + orderIndex: 2, + }); const result = await teamCollectionService.updateCollectionOrder( rootTeamCollectionList[4].id, @@ -1330,7 +1381,7 @@ describe('updateCollectionOrder', () => { expect(result).toEqualRight(true); }); - test('should throw TEAM_COL_REORDERING_FAILED when re-ordering operation failed for child TeamCollection list', async () => { + test('should throw TEAM_COL_REORDERING_FAILED when re-ordering operation failed with nextCollection', async () => { // getCollection; mockPrisma.teamCollection.findUniqueOrThrow .mockResolvedValueOnce(childTeamCollectionList[4]) @@ -1350,13 +1401,23 @@ describe('updateCollectionOrder', () => { mockPrisma.teamCollection.findUniqueOrThrow .mockResolvedValueOnce(childTeamCollectionList[4]) .mockResolvedValueOnce(childTeamCollectionList[2]); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst + .mockResolvedValueOnce(childTeamCollectionList[4]) + .mockResolvedValueOnce(childTeamCollectionList[2]); + mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 3 }); + mockPrisma.teamCollection.update.mockResolvedValueOnce({ + ...childTeamCollectionList[4], + orderIndex: 2, + }); await teamCollectionService.updateCollectionOrder( childTeamCollectionList[4].id, childTeamCollectionList[2].id, ); expect(mockPubSub.publish).toHaveBeenCalledWith( - `team_coll/${childTeamCollectionList[2].teamID}/coll_order_updated`, + `team_coll/${childTeamCollectionList[4].teamID}/coll_order_updated`, { collection: childTeamCollectionListCasted[4], nextCollection: childTeamCollectionListCasted[2], @@ -1528,7 +1589,7 @@ describe('sortTeamCollections', () => { const teamID = team.id; mockPrisma.$transaction.mockImplementation(async (cb) => cb(mockPrisma)); - mockPrisma.acquireLocks.mockResolvedValue(undefined); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); mockPrisma.teamCollection.findMany.mockResolvedValueOnce( rootTeamCollectionList, ); @@ -1555,7 +1616,7 @@ describe('sortTeamCollections', () => { const teamID = team.id; mockPrisma.$transaction.mockImplementation(async (cb) => cb(mockPrisma)); - mockPrisma.acquireLocks.mockResolvedValue(undefined); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); mockPrisma.teamCollection.findMany.mockResolvedValueOnce( rootTeamCollectionList, ); @@ -1591,6 +1652,437 @@ describe('sortTeamCollections', () => { }); }); +describe('FIX: updateMany queries now include teamID filter for root collections', () => { + /** + * These tests verify that the bug has been fixed where updateMany queries with parentID: null + * now correctly filter by teamID, ensuring operations only affect the specific team's collections. + * + * The fix was applied to: + * - deleteCollectionAndUpdateSiblingsOrderIndex (line 565-572) + * - updateCollectionOrder (line 894-905, 976-985) + * - getCollectionCount (line 851-856) + */ + + beforeEach(() => { + mockReset(mockPrisma); + }); + + const team2: Team = { + id: 'team_2', + name: 'Team 2', + }; + + // Team 2's root collection - should NOT be affected by Team 1's operations + const team2RootCollection: DBTeamCollection = { + id: 'team2-root-coll', + orderIndex: 1, + parentID: null, + title: 'Team 2 Root Collection', + teamID: team2.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + test('FIX: deleteCollection - updateMany now correctly filters by teamID for root collections', async () => { + /** + * Scenario: Team 1 deletes a root collection + * The sibling orderIndex update should ONLY affect Team 1's root collections + * + * FIX: The query now includes teamID filter, ensuring isolation between teams + */ + + const team1RootToDelete: DBTeamCollection = { + id: 'team1-root-to-delete', + orderIndex: 2, + parentID: null, + title: 'Team 1 Root To Delete', + teamID: team.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + // getCollection + mockPrisma.teamCollection.findUniqueOrThrow.mockResolvedValueOnce( + team1RootToDelete, + ); + + // deleteCollectionAndUpdateSiblingsOrderIndex transaction + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.delete.mockResolvedValueOnce(team1RootToDelete); + mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 3 }); + + await teamCollectionService.deleteCollection(team1RootToDelete.id); + + // Get the updateMany call from the transaction + const updateManyCall = mockPrisma.teamCollection.updateMany.mock.calls[0][0]; + + // FIX VERIFICATION: The query now correctly includes teamID + // This ensures only Team 1's root collections are affected + expect(updateManyCall.where).toEqual({ + teamID: team.id, // FIX: teamID is now included! + parentID: null, + orderIndex: { gt: team1RootToDelete.orderIndex }, + }); + }); + + test('FIX: updateCollectionOrder (to end) - updateMany now correctly filters by teamID', async () => { + /** + * Scenario: Team 1 reorders a root collection to the end + * FIX: The query now includes teamID, ensuring only Team 1's collections are affected + */ + + const team1RootCollection: DBTeamCollection = { + id: 'team1-root-coll', + orderIndex: 2, + parentID: null, + title: 'Team 1 Root Collection', + teamID: team.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + // getCollection + mockPrisma.teamCollection.findUniqueOrThrow.mockResolvedValueOnce( + team1RootCollection, + ); + + // Mock the transaction + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst.mockResolvedValueOnce( + team1RootCollection, + ); + mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 3 }); + mockPrisma.teamCollection.count.mockResolvedValueOnce(5); + mockPrisma.teamCollection.update.mockResolvedValueOnce({ + ...team1RootCollection, + orderIndex: 5, + }); + + await teamCollectionService.updateCollectionOrder( + team1RootCollection.id, + null, // Move to end of list + ); + + // Get the actual updateMany call arguments + const updateManyCall = mockPrisma.teamCollection.updateMany.mock.calls[0][0]; + + // FIX VERIFICATION: The query now correctly includes teamID + expect(updateManyCall.where).toEqual({ + teamID: team.id, // FIX: teamID is now included! + parentID: null, + orderIndex: { gte: team1RootCollection.orderIndex + 1 }, + }); + }); + + test('FIX: updateCollectionOrder (with nextCollection) - updateMany now correctly filters by teamID', async () => { + /** + * Scenario: Team 1 reorders root collections + * FIX: The updateMany now correctly filters by teamID + */ + + const team1RootCollection1: DBTeamCollection = { + id: 'team1-root-1', + orderIndex: 1, + parentID: null, + title: 'Team 1 Root 1', + teamID: team.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + const team1RootCollection2: DBTeamCollection = { + id: 'team1-root-2', + orderIndex: 4, + parentID: null, + title: 'Team 1 Root 2', + teamID: team.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + // getCollection for both collections + mockPrisma.teamCollection.findUniqueOrThrow + .mockResolvedValueOnce(team1RootCollection1) + .mockResolvedValueOnce(team1RootCollection2); + + // Mock the transaction + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst + .mockResolvedValueOnce(team1RootCollection1) + .mockResolvedValueOnce(team1RootCollection2); + mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 2 }); + mockPrisma.teamCollection.update.mockResolvedValueOnce({ + ...team1RootCollection1, + orderIndex: 3, + }); + + await teamCollectionService.updateCollectionOrder( + team1RootCollection1.id, + team1RootCollection2.id, // Move before this collection + ); + + // Get the actual updateMany call arguments + const updateManyCall = mockPrisma.teamCollection.updateMany.mock.calls[0][0]; + + // FIX VERIFICATION: The query now correctly includes teamID + expect(updateManyCall.where).toEqual({ + teamID: team.id, // FIX: teamID is now included! + parentID: null, + orderIndex: { + gte: team1RootCollection1.orderIndex + 1, + lte: team1RootCollection2.orderIndex - 1, + }, + }); + }); + + test('FIX: getCollectionCount - now correctly filters by teamID for root collections', async () => { + /** + * Scenario: Getting count of root collections for a team + * FIX: The count query now requires and filters by teamID + */ + + mockPrisma.teamCollection.count.mockResolvedValueOnce(5); + + await teamCollectionService.getCollectionCount(null, team.id); + + // FIX VERIFICATION: The count query now filters by teamID + expect(mockPrisma.teamCollection.count).toHaveBeenCalledWith({ + where: { parentID: null, teamID: team.id }, // FIX: teamID is now included! + }); + }); +}); + +describe('SCENARIO: Two teams performing concurrent operations on root collections', () => { + /** + * Scenario tests to verify operations are correctly isolated between teams + */ + + beforeEach(() => { + mockReset(mockPrisma); + }); + + const team2: Team = { + id: 'team_2', + name: 'Team 2', + }; + + const team1RootCollection: DBTeamCollection = { + id: 'team1-root', + orderIndex: 2, + parentID: null, + title: 'Team 1 Root', + teamID: team.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + const team2RootCollection: DBTeamCollection = { + id: 'team2-root', + orderIndex: 2, + parentID: null, + title: 'Team 2 Root', + teamID: team2.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + test('SCENARIO: Team 1 deletes root collection - operations are now isolated from Team 2', async () => { + /** + * With the fix: + * - Team 1 deletes root collection (orderIndex 2) + * - Only Team 1's root collections with orderIndex > 2 are decremented + * - Team 2's collections are NOT affected (correct behavior) + */ + + // === Team 1 deletes their root collection === + mockPrisma.teamCollection.findUniqueOrThrow.mockResolvedValueOnce( + team1RootCollection, + ); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.delete.mockResolvedValueOnce(team1RootCollection); + mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 3 }); + + await teamCollectionService.deleteCollection(team1RootCollection.id); + + const deleteUpdateManyCall = + mockPrisma.teamCollection.updateMany.mock.calls[0][0]; + + // FIX VERIFICATION: The updateMany now correctly includes teamID + // Only Team 1's collections are affected + expect(deleteUpdateManyCall.where.teamID).toBe(team.id); // FIX: teamID is now included! + expect(deleteUpdateManyCall.where.parentID).toBe(null); + expect(deleteUpdateManyCall.where.orderIndex).toEqual({ + gt: team1RootCollection.orderIndex, + }); + }); + + test('SCENARIO: Team 2 reorders root collection - operations are isolated from Team 1', async () => { + /** + * With the fix: + * - Team 2 reorders a root collection to the end + * - Only Team 2's root collections are affected + * - Team 1's collections are NOT affected (correct behavior) + */ + + // === Team 2 reorders their root collection to the end === + mockPrisma.teamCollection.findUniqueOrThrow.mockResolvedValueOnce( + team2RootCollection, + ); + + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst.mockResolvedValueOnce( + team2RootCollection, + ); + mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 2 }); + mockPrisma.teamCollection.count.mockResolvedValueOnce(4); // Team 2 has 4 root collections + mockPrisma.teamCollection.update.mockResolvedValueOnce({ + ...team2RootCollection, + orderIndex: 4, + }); + + await teamCollectionService.updateCollectionOrder( + team2RootCollection.id, + null, // Move to end + ); + + const reorderUpdateManyCall = + mockPrisma.teamCollection.updateMany.mock.calls[0][0]; + + // FIX VERIFICATION: The updateMany now correctly includes teamID + // Only Team 2's collections are affected + expect(reorderUpdateManyCall.where.teamID).toBe(team2.id); // FIX: teamID is now included! + expect(reorderUpdateManyCall.where.parentID).toBe(null); + expect(reorderUpdateManyCall.where.orderIndex).toEqual({ + gte: team2RootCollection.orderIndex + 1, + }); + }); + + test('SCENARIO: Both teams reorder collections concurrently - each team isolated', async () => { + /** + * Simulates concurrent operations from two different teams + * Each team's reorder operation should only affect their own collections + */ + + const team1Root1: DBTeamCollection = { + id: 'team1-root-1', + orderIndex: 1, + parentID: null, + title: 'Team 1 Root 1', + teamID: team.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + const team1Root2: DBTeamCollection = { + id: 'team1-root-2', + orderIndex: 3, + parentID: null, + title: 'Team 1 Root 2', + teamID: team.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + // === Team 1 reorders collection === + mockPrisma.teamCollection.findUniqueOrThrow + .mockResolvedValueOnce(team1Root1) + .mockResolvedValueOnce(team1Root2); + + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst + .mockResolvedValueOnce(team1Root1) + .mockResolvedValueOnce(team1Root2); + mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 1 }); + mockPrisma.teamCollection.update.mockResolvedValueOnce({ + ...team1Root1, + orderIndex: 2, + }); + + await teamCollectionService.updateCollectionOrder( + team1Root1.id, + team1Root2.id, + ); + + const team1UpdateManyCall = + mockPrisma.teamCollection.updateMany.mock.calls[0][0]; + + // Verify Team 1's operation is correctly scoped + expect(team1UpdateManyCall.where.teamID).toBe(team.id); + expect(team1UpdateManyCall.where.parentID).toBe(null); + + // Reset mocks for Team 2's operation + mockReset(mockPrisma); + + const team2Root1: DBTeamCollection = { + id: 'team2-root-1', + orderIndex: 1, + parentID: null, + title: 'Team 2 Root 1', + teamID: team2.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + const team2Root2: DBTeamCollection = { + id: 'team2-root-2', + orderIndex: 5, + parentID: null, + title: 'Team 2 Root 2', + teamID: team2.id, + data: {}, + createdOn: currentTime, + updatedOn: currentTime, + }; + + // === Team 2 reorders collection === + mockPrisma.teamCollection.findUniqueOrThrow + .mockResolvedValueOnce(team2Root1) + .mockResolvedValueOnce(team2Root2); + + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockTeamCollectionByTeamAndParent.mockResolvedValue(undefined); + mockPrisma.teamCollection.findFirst + .mockResolvedValueOnce(team2Root1) + .mockResolvedValueOnce(team2Root2); + mockPrisma.teamCollection.updateMany.mockResolvedValueOnce({ count: 3 }); + mockPrisma.teamCollection.update.mockResolvedValueOnce({ + ...team2Root1, + orderIndex: 4, + }); + + await teamCollectionService.updateCollectionOrder( + team2Root1.id, + team2Root2.id, + ); + + const team2UpdateManyCall = + mockPrisma.teamCollection.updateMany.mock.calls[0][0]; + + // Verify Team 2's operation is correctly scoped + expect(team2UpdateManyCall.where.teamID).toBe(team2.id); + expect(team2UpdateManyCall.where.parentID).toBe(null); + + // Both operations are isolated - Team 1's operation only affects Team 1's collections, + // and Team 2's operation only affects Team 2's collections + }); +}); + //ToDo: write test cases for exportCollectionsToJSON describe('getCollectionForCLI', () => { diff --git a/packages/hoppscotch-backend/src/team-collection/team-collection.service.ts b/packages/hoppscotch-backend/src/team-collection/team-collection.service.ts index e7044258..b11b392e 100644 --- a/packages/hoppscotch-backend/src/team-collection/team-collection.service.ts +++ b/packages/hoppscotch-backend/src/team-collection/team-collection.service.ts @@ -219,7 +219,7 @@ export class TeamCollectionService { await this.prisma.$transaction(async (tx) => { try { // lock the rows - await this.prisma.lockTableExclusive(tx, 'TeamCollection'); + await this.prisma.lockTeamCollectionByTeamAndParent(tx, teamID, parentID); // Get the last order index const lastEntry = await tx.teamCollection.findFirst({ @@ -400,9 +400,9 @@ export class TeamCollectionService { * @param collectionID The collection ID * @returns An Either of the Collection details */ - async getCollection(collectionID: string) { + async getCollection(collectionID: string, tx: Prisma.TransactionClient | null = null) { try { - const teamCollection = await this.prisma.teamCollection.findUniqueOrThrow( + const teamCollection = await (tx || this.prisma).teamCollection.findUniqueOrThrow( { where: { id: collectionID, @@ -472,7 +472,7 @@ export class TeamCollectionService { teamCollection = await this.prisma.$transaction(async (tx) => { try { // lock the rows - await this.prisma.lockTableExclusive(tx, 'TeamCollection'); + await this.prisma.lockTeamCollectionByTeamAndParent(tx, teamID, parentID); // fetch last collection const lastCollection = await tx.teamCollection.findFirst({ @@ -555,20 +555,25 @@ export class TeamCollectionService { await this.prisma.$transaction(async (tx) => { try { // lock the rows - await this.prisma.lockTableExclusive(tx, 'TeamCollection'); + await this.prisma.lockTeamCollectionByTeamAndParent(tx, collection.teamID, collection.parentID); - await tx.teamCollection.delete({ + const deletedCollection = await tx.teamCollection.delete({ where: { id: collection.id }, }); - - // update siblings orderIndexes - await tx.teamCollection.updateMany({ - where: { - parentID: collection.parentID, - orderIndex: orderIndexCondition, - }, - data: { orderIndex: dataCondition }, - }); + + // if collection is deleted, update siblings orderIndexes + // if collection was deleted before the transaction started (race condition), do not update siblings orderIndexes + if (deletedCollection) { + // update siblings orderIndexes + await tx.teamCollection.updateMany({ + where: { + teamID: collection.teamID, + parentID: collection.parentID, + orderIndex: orderIndexCondition, + }, + data: { orderIndex: dataCondition }, + }); + } } catch (error) { throw new ConflictException(error); } @@ -597,48 +602,6 @@ export class TeamCollectionService { return E.right(true); } - /** - * Delete child collection and requests of a TeamCollection - * - * @param collectionID The Collection Id - * @returns A Boolean of deletion status - */ - private async deleteCollectionData(collection: DBTeamCollection) { - // Get all child collections in collectionID - const childCollectionList = await this.prisma.teamCollection.findMany({ - where: { - parentID: collection.id, - }, - }); - - // Delete child collections - await Promise.all( - childCollectionList.map((coll) => this.deleteCollection(coll.id)), - ); - - // Delete all requests in collectionID - await this.prisma.teamRequest.deleteMany({ - where: { - collectionID: collection.id, - }, - }); - - // Update orderIndexes in TeamCollection table for user - const isDeleted = await this.deleteCollectionAndUpdateSiblingsOrderIndex( - collection, - { gt: collection.orderIndex }, - { decrement: 1 }, - ); - if (E.isLeft(isDeleted)) return E.left(isDeleted.left); - - this.pubsub.publish( - `team_coll/${collection.teamID}/coll_removed`, - collection.id, - ); - - return E.right(collection); - } - /** * Delete a TeamCollection * @@ -650,11 +613,20 @@ export class TeamCollectionService { if (E.isLeft(collection)) return E.left(collection.left); // Delete all child collections and requests in the collection - const collectionData = await this.deleteCollectionData(collection.right); - if (E.isLeft(collectionData)) return E.left(collectionData.left); + const isDeleted = await this.deleteCollectionAndUpdateSiblingsOrderIndex( + collection.right, + { gt: collection.right.orderIndex }, + { decrement: 1 }, + ); + if (E.isLeft(isDeleted)) return E.left(isDeleted.left); + + this.pubsub.publish( + `team_coll/${collection.right.teamID}/coll_removed`, + collection.right.id, + ); return E.right(true); - } + } /** * Change parentID of TeamCollection's @@ -664,58 +636,43 @@ export class TeamCollectionService { * @returns If successful return an Either of collection or error message */ private async changeParentAndUpdateOrderIndex( + tx: Prisma.TransactionClient, collection: DBTeamCollection, newParentID: string | null, ) { - let updatedCollection: DBTeamCollection = null; - - try { - await this.prisma.$transaction(async (tx) => { - try { - // fetch last collection - const lastCollectionUnderNewParent = - await tx.teamCollection.findFirst({ - where: { teamID: collection.teamID, parentID: newParentID }, - orderBy: { orderIndex: 'desc' }, - }); - - // decrement orderIndex of all next sibling collections from original collection - await tx.teamCollection.updateMany({ - where: { - teamID: collection.teamID, - parentID: collection.parentID, - orderIndex: { gt: collection.orderIndex }, - }, - data: { - orderIndex: { decrement: 1 }, - }, - }); - - // update collection's parentID and orderIndex - updatedCollection = await tx.teamCollection.update({ - where: { id: collection.id }, - data: { - // if parentCollectionID == null, collection becomes root collection - // if parentCollectionID != null, collection becomes child collection - parentID: newParentID, - orderIndex: lastCollectionUnderNewParent - ? lastCollectionUnderNewParent.orderIndex + 1 - : 1, - }, - }); - } catch (error) { - throw new ConflictException(error); - } + // fetch last collection + const lastCollectionUnderNewParent = + await tx.teamCollection.findFirst({ + where: { teamID: collection.teamID, parentID: newParentID }, + orderBy: { orderIndex: 'desc' }, }); - return E.right(this.cast(updatedCollection)); - } catch (error) { - console.error( - 'Error from TeamCollectionService.changeParentAndUpdateOrderIndex', - error, - ); - return E.left(TEAM_COLL_NOT_FOUND); - } + // decrement orderIndex of all next sibling collections from original collection + await tx.teamCollection.updateMany({ + where: { + teamID: collection.teamID, + parentID: collection.parentID, + orderIndex: { gt: collection.orderIndex }, + }, + data: { + orderIndex: { decrement: 1 }, + }, + }); + + // update collection's parentID and orderIndex + const updatedCollection = await tx.teamCollection.update({ + where: { id: collection.id }, + data: { + // if parentCollectionID == null, collection becomes root collection + // if parentCollectionID != null, collection becomes child collection + parentID: newParentID, + orderIndex: lastCollectionUnderNewParent + ? lastCollectionUnderNewParent.orderIndex + 1 + : 1, + }, + }); + + return E.right(this.cast(updatedCollection)); } /** @@ -728,6 +685,7 @@ export class TeamCollectionService { private async isParent( collection: DBTeamCollection, destCollection: DBTeamCollection, + tx: Prisma.TransactionClient | null = null, ): Promise> { //* Recursively check if collection is a parent by going up the tree of child-parent collections until we reach a root collection i.e parentID === null //* Valid condition, isParent returns false @@ -755,12 +713,13 @@ export class TeamCollectionService { // Get collection details of collection one step above in the tree i.e the parent collection const parentCollection = await this.getCollection( destCollection.parentID, + tx, ); if (E.isLeft(parentCollection)) { return O.none; } // Call isParent again now with parent collection - return await this.isParent(collection, parentCollection.right); + return await this.isParent(collection, parentCollection.right, tx); } else { return O.some(true); } @@ -774,83 +733,102 @@ export class TeamCollectionService { * @returns An Either of the moved TeamCollection */ async moveCollection(collectionID: string, destCollectionID: string | null) { - // Get collection details of collectionID - const collection = await this.getCollection(collectionID); - if (E.isLeft(collection)) return E.left(collection.left); + try { + return await this.prisma.$transaction(async (tx) => { + // Get collection details of collectionID + const collection = await this.getCollection(collectionID, tx); + if (E.isLeft(collection)) return E.left(collection.left); + // lock the rows of the collection and its siblings + await this.prisma.lockTeamCollectionByTeamAndParent(tx, collection.right.teamID, collection.right.parentID); + // destCollectionID == null i.e move collection to root + if (!destCollectionID) { + if (!collection.right.parentID) { + // collection is a root collection + // Throw error if collection is already a root collection + return E.left(TEAM_COL_ALREADY_ROOT); + } + + // Change parent from child to root i.e child collection becomes a root collection + // Move child collection into root and update orderIndexes for root teamCollections + const updatedCollection = await this.changeParentAndUpdateOrderIndex( + tx, + collection.right, + null, + ); + if (E.isLeft(updatedCollection)) return E.left(updatedCollection.left); + + this.pubsub.publish( + `team_coll/${collection.right.teamID}/coll_moved`, + updatedCollection.right, + ); + + return E.right(updatedCollection.right); + } + + // destCollectionID != null i.e move into another collection + if (collectionID === destCollectionID) { + // Throw error if collectionID and destCollectionID are the same + return E.left(TEAM_COLL_DEST_SAME); + } + + // Get collection details of destCollectionID + const destCollection = await this.getCollection(destCollectionID, tx); + if (E.isLeft(destCollection)) return E.left(TEAM_COLL_NOT_FOUND); + + // Check if collection and destCollection belong to the same user account + if (collection.right.teamID !== destCollection.right.teamID) { + return E.left(TEAM_COLL_NOT_SAME_TEAM); + } + + // Check if collection is present on the parent tree for destCollection + const checkIfParent = await this.isParent( + collection.right, + destCollection.right, + tx, + ); + if (O.isNone(checkIfParent)) { + return E.left(TEAM_COLL_IS_PARENT_COLL); + } - // destCollectionID == null i.e move collection to root - if (!destCollectionID) { - if (!collection.right.parentID) { - // collection is a root collection - // Throw error if collection is already a root collection - return E.left(TEAM_COL_ALREADY_ROOT); - } + // lock the rows of the destination collection and its siblings + await this.prisma.lockTeamCollectionByTeamAndParent(tx, destCollection.right.teamID, destCollection.right.parentID); + + // Change parent from null to teamCollection i.e collection becomes a child collection + // Move root/child collection into another child collection and update orderIndexes of the previous parent + const updatedCollection = await this.changeParentAndUpdateOrderIndex( + tx, + collection.right, + destCollection.right.id, + ); + if (E.isLeft(updatedCollection)) return E.left(updatedCollection.left); - // Change parent from child to root i.e child collection becomes a root collection - // Move child collection into root and update orderIndexes for root teamCollections - const updatedCollection = await this.changeParentAndUpdateOrderIndex( - collection.right, - null, + this.pubsub.publish( + `team_coll/${collection.right.teamID}/coll_moved`, + updatedCollection.right, + ); + + return E.right(updatedCollection.right); + }); + } catch (error) { + + console.error( + 'Error from TeamCollectionService.moveCollection', + error, ); - if (E.isLeft(updatedCollection)) return E.left(updatedCollection.left); - - this.pubsub.publish( - `team_coll/${collection.right.teamID}/coll_moved`, - updatedCollection.right, - ); - - return E.right(updatedCollection.right); + return E.left(TEAM_COL_REORDERING_FAILED); } - - // destCollectionID != null i.e move into another collection - if (collectionID === destCollectionID) { - // Throw error if collectionID and destCollectionID are the same - return E.left(TEAM_COLL_DEST_SAME); - } - - // Get collection details of destCollectionID - const destCollection = await this.getCollection(destCollectionID); - if (E.isLeft(destCollection)) return E.left(TEAM_COLL_NOT_FOUND); - - // Check if collection and destCollection belong to the same user account - if (collection.right.teamID !== destCollection.right.teamID) { - return E.left(TEAM_COLL_NOT_SAME_TEAM); - } - - // Check if collection is present on the parent tree for destCollection - const checkIfParent = await this.isParent( - collection.right, - destCollection.right, - ); - if (O.isNone(checkIfParent)) { - return E.left(TEAM_COLL_IS_PARENT_COLL); - } - - // Change parent from null to teamCollection i.e collection becomes a child collection - // Move root/child collection into another child collection and update orderIndexes of the previous parent - const updatedCollection = await this.changeParentAndUpdateOrderIndex( - collection.right, - destCollection.right.id, - ); - if (E.isLeft(updatedCollection)) return E.left(updatedCollection.left); - - this.pubsub.publish( - `team_coll/${collection.right.teamID}/coll_moved`, - updatedCollection.right, - ); - - return E.right(updatedCollection.right); } /** * Find the number of child collections present in collectionID * * @param collectionID The Collection ID + * @param teamID The Team ID (required when collectionID is null for root collections) * @returns Number of collections */ - getCollectionCount(collectionID: string): Promise { - return this.prisma.teamCollection.count({ - where: { parentID: collectionID }, + getCollectionCount(collectionID: string, teamID: string, tx: Prisma.TransactionClient | null = null): Promise { + return (tx || this.prisma).teamCollection.count({ + where: { parentID: collectionID, teamID: teamID }, }); } @@ -879,39 +857,47 @@ export class TeamCollectionService { await this.prisma.$transaction(async (tx) => { try { // Step 0: lock the rows - await this.prisma.acquireLocks( + await this.prisma.lockTeamCollectionByTeamAndParent( tx, - 'TeamCollection', - null, - collectionID, + collection.right.teamID, + collection.right.parentID, ); - // Step 1: Decrement orderIndex of all items that come after collection.orderIndex till end of list of items const collectionInTx = await tx.teamCollection.findFirst({ where: { id: collection.right.id }, select: { orderIndex: true }, }); - await tx.teamCollection.updateMany({ - where: { - parentID: collection.right.parentID, - orderIndex: { - gte: collectionInTx.orderIndex + 1, - }, - }, - data: { - orderIndex: { decrement: 1 }, - }, - }); - // Step 2: Update orderIndex of collection to length of list - await tx.teamCollection.update({ - where: { id: collection.right.id }, - data: { - orderIndex: await this.getCollectionCount( - collection.right.parentID, - ), - }, - }); + // if collection is found, update orderIndexes of siblings + // if collection was deleted before the transaction started (race condition), do not update siblings orderIndexes + if(collectionInTx) { + // Step 1: Decrement orderIndex of all items that come after collection.orderIndex till end of list of items + await tx.teamCollection.updateMany({ + where: { + teamID: collection.right.teamID, + parentID: collection.right.parentID, + orderIndex: { + gte: collectionInTx.orderIndex + 1, + }, + }, + data: { + orderIndex: { decrement: 1 }, + }, + }); + + // Step 2: Update orderIndex of collection to length of list + await tx.teamCollection.update({ + where: { id: collection.right.id }, + data: { + orderIndex: await this.getCollectionCount( + collection.right.parentID, + collection.right.teamID, + tx, + ), + }, + }); + } + } catch (error) { throw new ConflictException(error); } @@ -944,14 +930,8 @@ export class TeamCollectionService { await this.prisma.$transaction(async (tx) => { try { // Step 0: lock the rows - await this.prisma.acquireLocks( - tx, - 'TeamCollection', - null, - collection.right.parentID, - ); + await this.prisma.lockTeamCollectionByTeamAndParent(tx, collection.right.teamID, collection.right.parentID); - // Step 1: Determine if we are moving collection up or down the list const collectionInTx = await tx.teamCollection.findFirst({ where: { id: collectionID }, select: { orderIndex: true }, @@ -960,37 +940,44 @@ export class TeamCollectionService { where: { id: nextCollectionID }, select: { orderIndex: true }, }); - const isMovingUp = + + // if collection and subsequentCollection are found, update orderIndexes of siblings + // if collection or subsequentCollection was deleted before the transaction started (race condition), do not update siblings orderIndexes + if(collectionInTx && subsequentCollectionInTx) { + // Step 1: Determine if we are moving collection up or down the list + const isMovingUp = subsequentCollectionInTx.orderIndex < collectionInTx.orderIndex; - // Step 2: Update OrderIndex of items in list depending on moving up or down - const updateFrom = isMovingUp - ? subsequentCollectionInTx.orderIndex - : collectionInTx.orderIndex + 1; + // Step 2: Update OrderIndex of items in list depending on moving up or down + const updateFrom = isMovingUp + ? subsequentCollectionInTx.orderIndex + : collectionInTx.orderIndex + 1; - const updateTo = isMovingUp - ? collectionInTx.orderIndex - 1 - : subsequentCollectionInTx.orderIndex - 1; + const updateTo = isMovingUp + ? collectionInTx.orderIndex - 1 + : subsequentCollectionInTx.orderIndex - 1; - await tx.teamCollection.updateMany({ - where: { - parentID: collection.right.parentID, - orderIndex: { gte: updateFrom, lte: updateTo }, - }, - data: { - orderIndex: isMovingUp ? { increment: 1 } : { decrement: 1 }, - }, - }); + await tx.teamCollection.updateMany({ + where: { + teamID: collection.right.teamID, + parentID: collection.right.parentID, + orderIndex: { gte: updateFrom, lte: updateTo }, + }, + data: { + orderIndex: isMovingUp ? { increment: 1 } : { decrement: 1 }, + }, + }); - // Step 3: Update OrderIndex of collection - await tx.teamCollection.update({ - where: { id: collection.right.id }, - data: { - orderIndex: isMovingUp - ? subsequentCollectionInTx.orderIndex - : subsequentCollectionInTx.orderIndex - 1, - }, - }); + // Step 3: Update OrderIndex of collection + await tx.teamCollection.update({ + where: { id: collection.right.id }, + data: { + orderIndex: isMovingUp + ? subsequentCollectionInTx.orderIndex + : subsequentCollectionInTx.orderIndex - 1, + }, + }); + } } catch (error) { throw new ConflictException(error); } @@ -1506,7 +1493,7 @@ export class TeamCollectionService { try { await this.prisma.$transaction(async (tx) => { - await this.prisma.acquireLocks(tx, 'TeamCollection', null, parentID); + await this.prisma.lockTeamCollectionByTeamAndParent(tx, teamID, parentID); const collections = await tx.teamCollection.findMany({ where: { teamID, parentID }, diff --git a/packages/hoppscotch-backend/src/team-request/team-request.service.spec.ts b/packages/hoppscotch-backend/src/team-request/team-request.service.spec.ts index 09b77fe9..c12782e1 100644 --- a/packages/hoppscotch-backend/src/team-request/team-request.service.spec.ts +++ b/packages/hoppscotch-backend/src/team-request/team-request.service.spec.ts @@ -762,7 +762,7 @@ describe('sortTeamRequests', () => { const collectionID = teamCollection.id; mockPrisma.$transaction.mockImplementation(async (cb) => cb(mockPrisma)); - mockPrisma.acquireLocks.mockResolvedValue(undefined); + mockPrisma.lockTeamRequestByCollections.mockResolvedValue(undefined); mockPrisma.teamRequest.findMany.mockResolvedValue(dbTeamRequests); const result = await teamRequestService.sortTeamRequests( @@ -788,7 +788,7 @@ describe('sortTeamRequests', () => { const collectionID = teamCollection.id; mockPrisma.$transaction.mockImplementation(async (cb) => cb(mockPrisma)); - mockPrisma.acquireLocks.mockResolvedValue(undefined); + mockPrisma.lockTeamRequestByCollections.mockResolvedValue(undefined); mockPrisma.teamRequest.findMany.mockResolvedValue(dbTeamRequests); const result = await teamRequestService.sortTeamRequests( diff --git a/packages/hoppscotch-backend/src/team-request/team-request.service.ts b/packages/hoppscotch-backend/src/team-request/team-request.service.ts index f42d72f0..941223c2 100644 --- a/packages/hoppscotch-backend/src/team-request/team-request.service.ts +++ b/packages/hoppscotch-backend/src/team-request/team-request.service.ts @@ -120,19 +120,23 @@ export class TeamRequestService { await this.prisma.$transaction(async (tx) => { try { // lock the rows - await this.prisma.lockTableExclusive(tx, 'TeamRequest'); + await this.prisma.lockTeamRequestByCollections(tx, dbTeamReq.teamID, [dbTeamReq.collectionID]); - await tx.teamRequest.updateMany({ - where: { - collectionID: dbTeamReq.collectionID, - orderIndex: { gte: dbTeamReq.orderIndex }, - }, - data: { orderIndex: { decrement: 1 } }, - }); - - await tx.teamRequest.delete({ + const deletedTeamRequest = await tx.teamRequest.delete({ where: { id: requestID }, }); + + // if request is deleted, update orderIndexes of siblings + // if request was deleted before the transaction started (race condition), do not update siblings orderIndexes + if(deletedTeamRequest) { + await tx.teamRequest.updateMany({ + where: { + collectionID: dbTeamReq.collectionID, + orderIndex: { gte: dbTeamReq.orderIndex }, + }, + data: { orderIndex: { decrement: 1 } }, + }); + } } catch (error) { throw new ConflictException(error); } @@ -177,7 +181,7 @@ export class TeamRequestService { dbTeamRequest = await this.prisma.$transaction(async (tx) => { try { // lock the rows - await this.prisma.lockTableExclusive(tx, 'TeamRequest'); + await this.prisma.lockTeamRequestByCollections(tx, teamID, [collectionID]); // fetch last team request const lastTeamRequest = await tx.teamRequest.findFirst({ @@ -381,8 +385,8 @@ export class TeamRequestService { * A helper function to get the number of requests in a collection * @param collectionID Collection ID to fetch */ - private async getRequestsCountInCollection(collectionID: string) { - return this.prisma.teamRequest.count({ + private async getRequestsCountInCollection(collectionID: string, tx: Prisma.TransactionClient | null = null) { + return (tx || this.prisma).teamRequest.count({ where: { collectionID }, }); } @@ -405,10 +409,7 @@ export class TeamRequestService { E.Left | E.Right >(async (tx) => { // lock the rows - await this.prisma.acquireLocks(tx, 'TeamRequest', null, null, [ - srcCollID, - destCollID, - ]); + await this.prisma.lockTeamRequestByCollections(tx, request.teamID, [srcCollID, destCollID]); request = await tx.teamRequest.findUnique({ where: { id: request.id }, @@ -419,64 +420,68 @@ export class TeamRequestService { }) : null; - const isSameCollection = srcCollID === destCollID; - const isMovingUp = nextRequest?.orderIndex < request.orderIndex; // false, if nextRequest is null - - const nextReqOrderIndex = nextRequest?.orderIndex; - const reqCountInDestColl = nextRequest - ? undefined - : await this.getRequestsCountInCollection(destCollID); - - // Updating order indexes of other requests in collection(s) - if (isSameCollection) { - const updateFrom = isMovingUp - ? nextReqOrderIndex - : request.orderIndex + 1; - const updateTo = isMovingUp ? request.orderIndex : nextReqOrderIndex; - - await tx.teamRequest.updateMany({ - where: { - collectionID: srcCollID, - orderIndex: { gte: updateFrom, lt: updateTo }, - }, - data: { - orderIndex: isMovingUp ? { increment: 1 } : { decrement: 1 }, - }, - }); - } else { - await tx.teamRequest.updateMany({ - where: { - collectionID: srcCollID, - orderIndex: { gte: request.orderIndex }, - }, - data: { orderIndex: { decrement: 1 } }, - }); - - if (nextRequest) { + // if request is found in transaction, update orderIndexes of siblings + // if request was deleted before the transaction started (race condition), do not update siblings orderIndexes + if(request) { + const isSameCollection = srcCollID === destCollID; + const isMovingUp = nextRequest?.orderIndex < request.orderIndex; // false, if nextRequest is null + + const nextReqOrderIndex = nextRequest?.orderIndex; + const reqCountInDestColl = nextRequest + ? undefined + : await this.getRequestsCountInCollection(destCollID, tx); + + // Updating order indexes of other requests in collection(s) + if (isSameCollection) { + const updateFrom = isMovingUp + ? nextReqOrderIndex + : request.orderIndex + 1; + const updateTo = isMovingUp ? request.orderIndex : nextReqOrderIndex; + await tx.teamRequest.updateMany({ where: { - collectionID: destCollID, - orderIndex: { gte: nextReqOrderIndex }, + collectionID: srcCollID, + orderIndex: { gte: updateFrom, lt: updateTo }, + }, + data: { + orderIndex: isMovingUp ? { increment: 1 } : { decrement: 1 }, }, - data: { orderIndex: { increment: 1 } }, }); + } else { + await tx.teamRequest.updateMany({ + where: { + collectionID: srcCollID, + orderIndex: { gte: request.orderIndex }, + }, + data: { orderIndex: { decrement: 1 } }, + }); + + if (nextRequest) { + await tx.teamRequest.updateMany({ + where: { + collectionID: destCollID, + orderIndex: { gte: nextReqOrderIndex }, + }, + data: { orderIndex: { increment: 1 } }, + }); + } } + + // Updating order index of the request + let adjust: number; + if (isSameCollection) adjust = nextRequest ? (isMovingUp ? 0 : -1) : 0; + else adjust = nextRequest ? 0 : 1; + + const newOrderIndex = + (nextReqOrderIndex ?? reqCountInDestColl) + adjust; + + const updatedRequest = await tx.teamRequest.update({ + where: { id: request.id }, + data: { orderIndex: newOrderIndex, collectionID: destCollID }, + }); + + return E.right(updatedRequest); } - - // Updating order index of the request - let adjust: number; - if (isSameCollection) adjust = nextRequest ? (isMovingUp ? 0 : -1) : 0; - else adjust = nextRequest ? 0 : 1; - - const newOrderIndex = - (nextReqOrderIndex ?? reqCountInDestColl) + adjust; - - const updatedRequest = await tx.teamRequest.update({ - where: { id: request.id }, - data: { orderIndex: newOrderIndex, collectionID: destCollID }, - }); - - return E.right(updatedRequest); }); } catch (err) { return E.left(TEAM_REQ_REORDERING_FAILED); @@ -534,9 +539,7 @@ export class TeamRequestService { try { await this.prisma.$transaction(async (tx) => { // lock the rows - await this.prisma.acquireLocks(tx, 'TeamRequest', null, null, [ - collectionID, - ]); + await this.prisma.lockTeamRequestByCollections(tx, teamID, [collectionID]); const teamRequests = await tx.teamRequest.findMany({ where: { teamID, collectionID }, orderBy, diff --git a/packages/hoppscotch-backend/src/user-collection/user-collection.service.spec.ts b/packages/hoppscotch-backend/src/user-collection/user-collection.service.spec.ts index fa98ee01..007d8b7e 100644 --- a/packages/hoppscotch-backend/src/user-collection/user-collection.service.spec.ts +++ b/packages/hoppscotch-backend/src/user-collection/user-collection.service.spec.ts @@ -12,6 +12,7 @@ import { USER_COLL_ALREADY_ROOT, USER_NOT_OWNER, USER_COLL_DATA_INVALID, + USER_COLLECTION_CREATION_FAILED, } from 'src/errors'; import * as E from 'fp-ts/Either'; import * as O from 'fp-ts/Option'; @@ -729,6 +730,7 @@ describe('createUserCollection', () => { }); test('should throw USER_NOT_OWNER when user is not the owner of the collection', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') .mockResolvedValueOnce( @@ -745,11 +747,12 @@ describe('createUserCollection', () => { rootRESTUserCollection.id, ReqType.REST, ); - expect(result).toEqualLeft(USER_NOT_OWNER); + expect(result).toEqualLeft(USER_COLLECTION_CREATION_FAILED); }); test('should successfully create a new root REST user-collection with valid inputs', async () => { mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); mockPrisma.userCollection.findFirst.mockResolvedValueOnce(null); mockPrisma.userCollection.create.mockResolvedValueOnce( rootRESTUserCollection, @@ -767,6 +770,7 @@ describe('createUserCollection', () => { test('should successfully create a new root GQL user-collection with valid inputs', async () => { mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); mockPrisma.userCollection.findFirst.mockResolvedValueOnce(null); mockPrisma.userCollection.create.mockResolvedValueOnce( rootGQLUserCollection, @@ -783,10 +787,11 @@ describe('createUserCollection', () => { }); test('should successfully create a new child REST user-collection with valid inputs', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') .mockResolvedValueOnce(E.right(rootRESTUserCollection)); - mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); mockPrisma.userCollection.findFirst.mockResolvedValueOnce(null); mockPrisma.userCollection.create.mockResolvedValueOnce( childRESTUserCollection, @@ -803,10 +808,11 @@ describe('createUserCollection', () => { }); test('should successfully create a new child GQL user-collection with valid inputs', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') .mockResolvedValueOnce(E.right(rootGQLUserCollection)); - mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); mockPrisma.userCollection.findFirst.mockResolvedValueOnce(null); mockPrisma.userCollection.create.mockResolvedValueOnce( childGQLUserCollection, @@ -823,10 +829,11 @@ describe('createUserCollection', () => { }); test('should send pubsub message to "user_coll//created" if child REST user-collection is created successfully', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') .mockResolvedValueOnce(E.right(rootRESTUserCollection)); - mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); mockPrisma.userCollection.findFirst.mockResolvedValueOnce(null); mockPrisma.userCollection.create.mockResolvedValueOnce( childRESTUserCollection, @@ -846,10 +853,11 @@ describe('createUserCollection', () => { }); test('should send pubsub message to "user_coll//created" if child GQL user-collection is created successfully', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') .mockResolvedValueOnce(E.right(rootGQLUserCollection)); - mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); mockPrisma.userCollection.findFirst.mockResolvedValueOnce(null); mockPrisma.userCollection.create.mockResolvedValueOnce( childGQLUserCollection, @@ -871,6 +879,7 @@ describe('createUserCollection', () => { test('should send pubsub message to "user_coll//created" if REST root user-collection is created successfully', async () => { mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); mockPrisma.userCollection.findFirst.mockResolvedValueOnce(null); mockPrisma.userCollection.create.mockResolvedValueOnce( rootRESTUserCollection, @@ -891,6 +900,7 @@ describe('createUserCollection', () => { test('should send pubsub message to "user_coll//created" if GQL root user-collection is created successfully', async () => { mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); mockPrisma.userCollection.findFirst.mockResolvedValueOnce(null); mockPrisma.userCollection.create.mockResolvedValueOnce( rootGQLUserCollection, @@ -1042,12 +1052,12 @@ describe('deleteUserCollection', () => { ); expect(result).toEqualLeft(USER_NOT_OWNER); }); - test('should throw USER_COLL_NOT_FOUND when collectionID is invalid when deleting user-collection from UserCollectionTable ', async () => { + test('should throw USER_COLL_REORDERING_FAILED when removeCollectionAndUpdateSiblingsOrderIndex fails', async () => { jest .spyOn(userCollectionService, 'getUserCollection') .mockResolvedValueOnce(E.right(rootRESTUserCollection)); jest - .spyOn(userCollectionService as any, 'deleteCollectionData') + .spyOn(userCollectionService as any, 'removeCollectionAndUpdateSiblingsOrderIndex') .mockResolvedValueOnce(E.left(USER_COLL_REORDERING_FAILED)); const result = await userCollectionService.deleteUserCollection( @@ -1091,6 +1101,7 @@ describe('deleteUserCollection', () => { describe('moveUserCollection', () => { test('should throw USER_COLL_NOT_FOUND if userCollectionID is invalid', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); // getUserCollection mockPrisma.userCollection.findUniqueOrThrow.mockRejectedValueOnce( 'NotFoundError', @@ -1105,6 +1116,7 @@ describe('moveUserCollection', () => { }); test('should throw USER_NOT_OWNER if user is not owner of collection', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); // getUserCollection mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( rootRESTUserCollection, @@ -1119,6 +1131,7 @@ describe('moveUserCollection', () => { }); test('should throw USER_COLL_DEST_SAME if userCollectionID and destCollectionID is the same', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); // getUserCollection mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( rootRESTUserCollection, @@ -1133,6 +1146,7 @@ describe('moveUserCollection', () => { }); test('should throw USER_COLL_NOT_FOUND if destCollectionID is invalid', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); // getUserCollection mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( rootRESTUserCollection, @@ -1151,6 +1165,7 @@ describe('moveUserCollection', () => { }); test('should throw USER_COLL_NOT_SAME_TYPE if userCollectionID and destCollectionID are not the same collection type', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); // getUserCollection mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( rootRESTUserCollection, @@ -1169,6 +1184,7 @@ describe('moveUserCollection', () => { }); test('should throw USER_COLL_NOT_SAME_USER if userCollectionID and destCollectionID are not from the same user', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); // getUserCollection mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( rootRESTUserCollection, @@ -1188,6 +1204,7 @@ describe('moveUserCollection', () => { }); test('should throw USER_COLL_IS_PARENT_COLL if userCollectionID is parent of destCollectionID ', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); // getUserCollection mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( rootRESTUserCollection, @@ -1206,6 +1223,7 @@ describe('moveUserCollection', () => { }); test('should throw USER_COL_ALREADY_ROOT when moving root user-collection to root', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); // getUserCollection mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( rootRESTUserCollection, @@ -1220,6 +1238,7 @@ describe('moveUserCollection', () => { }); test('should successfully move a child user-collection into root', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') .mockResolvedValueOnce(E.right(childRESTUserCollection)); @@ -1241,6 +1260,7 @@ describe('moveUserCollection', () => { }); test('should throw USER_COLL_NOT_FOUND when trying to change parent of collection with invalid collectionID', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') .mockResolvedValueOnce(E.right(childRESTUserCollection)); @@ -1257,6 +1277,7 @@ describe('moveUserCollection', () => { }); test('should send pubsub message to "user_coll//moved" when user-collection is moved to root successfully', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') .mockResolvedValueOnce(E.right(childRESTUserCollection)); @@ -1281,11 +1302,10 @@ describe('moveUserCollection', () => { }); test('should successfully move a root user-collection into a child user-collection', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') - .mockResolvedValueOnce(E.right(rootRESTUserCollection)); - jest - .spyOn(userCollectionService, 'getUserCollection') + .mockResolvedValueOnce(E.right(rootRESTUserCollection)) .mockResolvedValueOnce(E.right(childRESTUserCollection_2)); jest .spyOn(userCollectionService as any, 'isParent') @@ -1311,11 +1331,10 @@ describe('moveUserCollection', () => { }); test('should successfully move a child user-collection into another child user-collection', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') - .mockResolvedValueOnce(E.right(rootRESTUserCollection)); - jest - .spyOn(userCollectionService, 'getUserCollection') + .mockResolvedValueOnce(E.right(rootRESTUserCollection)) .mockResolvedValueOnce(E.right(childRESTUserCollection_2)); jest .spyOn(userCollectionService as any, 'isParent') @@ -1341,11 +1360,10 @@ describe('moveUserCollection', () => { }); test('should send pubsub message to "user_coll//moved" when user-collection is moved into another child user-collection successfully', async () => { + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); jest .spyOn(userCollectionService, 'getUserCollection') - .mockResolvedValueOnce(E.right(rootRESTUserCollection)); - jest - .spyOn(userCollectionService, 'getUserCollection') + .mockResolvedValueOnce(E.right(rootRESTUserCollection)) .mockResolvedValueOnce(E.right(childRESTUserCollection_2)); jest .spyOn(userCollectionService as any, 'isParent') @@ -1417,7 +1435,15 @@ describe('updateUserCollectionOrder', () => { mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( childRESTUserCollectionList[4], ); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); + mockPrisma.userCollection.findFirst.mockResolvedValueOnce( + childRESTUserCollectionList[4], + ); mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 4 }); + mockPrisma.userCollection.count.mockResolvedValueOnce( + childRESTUserCollectionList.length, + ); mockPrisma.userCollection.update.mockResolvedValueOnce({ ...childRESTUserCollectionList[4], orderIndex: childRESTUserCollectionList.length, @@ -1436,7 +1462,15 @@ describe('updateUserCollectionOrder', () => { mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( rootRESTUserCollectionList[4], ); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); + mockPrisma.userCollection.findFirst.mockResolvedValueOnce( + rootRESTUserCollectionList[4], + ); mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 4 }); + mockPrisma.userCollection.count.mockResolvedValueOnce( + rootRESTUserCollectionList.length, + ); mockPrisma.userCollection.update.mockResolvedValueOnce({ ...rootRESTUserCollectionList[4], orderIndex: rootRESTUserCollectionList.length, @@ -1470,7 +1504,15 @@ describe('updateUserCollectionOrder', () => { mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( childRESTUserCollectionList[4], ); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); + mockPrisma.userCollection.findFirst.mockResolvedValueOnce( + childRESTUserCollectionList[4], + ); mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 4 }); + mockPrisma.userCollection.count.mockResolvedValueOnce( + childRESTUserCollectionList.length, + ); mockPrisma.userCollection.update.mockResolvedValueOnce({ ...childRESTUserCollectionList[4], orderIndex: childRESTUserCollectionList.length, @@ -1532,6 +1574,16 @@ describe('updateUserCollectionOrder', () => { mockPrisma.userCollection.findUniqueOrThrow .mockResolvedValueOnce(childRESTUserCollectionList[4]) .mockResolvedValueOnce(childRESTUserCollectionList[2]); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); + mockPrisma.userCollection.findFirst + .mockResolvedValueOnce(childRESTUserCollectionList[4]) + .mockResolvedValueOnce(childRESTUserCollectionList[2]); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 2 }); + mockPrisma.userCollection.update.mockResolvedValueOnce({ + ...childRESTUserCollectionList[4], + orderIndex: 2, + }); const result = await userCollectionService.updateUserCollectionOrder( childRESTUserCollectionList[4].id, @@ -1562,6 +1614,16 @@ describe('updateUserCollectionOrder', () => { mockPrisma.userCollection.findUniqueOrThrow .mockResolvedValueOnce(childRESTUserCollectionList[4]) .mockResolvedValueOnce(childRESTUserCollectionList[2]); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); + mockPrisma.userCollection.findFirst + .mockResolvedValueOnce(childRESTUserCollectionList[4]) + .mockResolvedValueOnce(childRESTUserCollectionList[2]); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 2 }); + mockPrisma.userCollection.update.mockResolvedValueOnce({ + ...childRESTUserCollectionList[4], + orderIndex: 2, + }); await userCollectionService.updateUserCollectionOrder( childRESTUserCollectionList[4].id, @@ -1584,6 +1646,544 @@ describe('updateUserCollectionOrder', () => { }); }); +describe('FIX: updateMany queries now include userUid filter for root collections', () => { + /** + * These tests verify the fix for the bug where updateMany queries with parentID: null + * were not filtering by userUid, potentially affecting other users' root collections. + * + * The fix adds userUid filter to: + * - changeParentAndUpdateOrderIndex + * - removeCollectionAndUpdateSiblingsOrderIndex + * - updateUserCollectionOrder (both cases) + * - getCollectionCount + */ + + beforeEach(() => { + mockReset(mockPrisma); + }); + + describe('SCENARIO: Two users performing concurrent operations on root collections', () => { + /** + * This scenario test simulates two users (Alice and Bob) each having multiple + * root collections and performing various operations. It verifies that: + * 1. All updateMany calls include the correct userUid filter + * 2. Alice's operations never affect Bob's collections and vice versa + */ + + const alice: AuthUser = { + uid: 'alice-uid', + email: 'alice@example.com', + displayName: 'Alice', + photoURL: null, + isAdmin: false, + refreshToken: 'alice-token', + lastLoggedOn: currentTime, + lastActiveOn: currentTime, + createdOn: currentTime, + currentGQLSession: {}, + currentRESTSession: {}, + }; + + const bob: AuthUser = { + uid: 'bob-uid', + email: 'bob@example.com', + displayName: 'Bob', + photoURL: null, + isAdmin: false, + refreshToken: 'bob-token', + lastLoggedOn: currentTime, + lastActiveOn: currentTime, + createdOn: currentTime, + currentGQLSession: {}, + currentRESTSession: {}, + }; + + // Alice's root collections (orderIndex: 1, 2, 3) + const aliceCollection1: DBUserCollection = { + id: 'alice-coll-1', + orderIndex: 1, + parentID: null, + title: 'Alice Collection 1', + userUid: alice.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + const aliceCollection2: DBUserCollection = { + id: 'alice-coll-2', + orderIndex: 2, + parentID: null, + title: 'Alice Collection 2', + userUid: alice.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + const aliceCollection3: DBUserCollection = { + id: 'alice-coll-3', + orderIndex: 3, + parentID: null, + title: 'Alice Collection 3', + userUid: alice.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + // Bob's root collections (orderIndex: 1, 2, 3) + const bobCollection1: DBUserCollection = { + id: 'bob-coll-1', + orderIndex: 1, + parentID: null, + title: 'Bob Collection 1', + userUid: bob.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + const bobCollection2: DBUserCollection = { + id: 'bob-coll-2', + orderIndex: 2, + parentID: null, + title: 'Bob Collection 2', + userUid: bob.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + const bobCollection3: DBUserCollection = { + id: 'bob-coll-3', + orderIndex: 3, + parentID: null, + title: 'Bob Collection 3', + userUid: bob.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + test('SCENARIO: Alice deletes collection, Bob reorders - operations are isolated', async () => { + /** + * Scenario: + * 1. Alice deletes her collection #2 (orderIndex: 2) + * - Expected: Alice's collection #3 becomes orderIndex: 2 + * - Bob's collections should be UNCHANGED + * + * 2. Bob reorders his collection #1 to the end + * - Expected: Bob's collections become: #2->1, #3->2, #1->3 + * - Alice's collections should be UNCHANGED + * + * Without the fix, both operations would affect ALL root collections + * because parentID: null matches everyone's root collections. + */ + + // === STEP 1: Alice deletes her collection #2 === + mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( + aliceCollection2, + ); + mockPrisma.userCollection.findMany.mockResolvedValueOnce([]); // No children + mockPrisma.userRequest.deleteMany.mockResolvedValueOnce({ count: 0 }); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.userCollection.delete.mockResolvedValueOnce(aliceCollection2); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 1 }); + + await userCollectionService.deleteUserCollection( + aliceCollection2.id, + alice.uid, + ); + + // Verify Alice's delete only affected Alice's collections + const aliceDeleteCall = mockPrisma.userCollection.updateMany.mock.calls[0][0]; + expect(aliceDeleteCall.where.userUid).toBe(alice.uid); + expect(aliceDeleteCall.where.parentID).toBe(null); + expect(aliceDeleteCall.where.orderIndex).toEqual({ gt: aliceCollection2.orderIndex }); + + // Reset mocks for Bob's operation + mockReset(mockPrisma); + + // === STEP 2: Bob reorders his collection #1 to the end === + mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( + bobCollection1, + ); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.userCollection.findFirst.mockResolvedValueOnce(bobCollection1); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 2 }); + mockPrisma.userCollection.count.mockResolvedValueOnce(3); + mockPrisma.userCollection.update.mockResolvedValueOnce({ + ...bobCollection1, + orderIndex: 3, + }); + + await userCollectionService.updateUserCollectionOrder( + bobCollection1.id, + null, + bob.uid, + ); + + // Verify Bob's reorder only affected Bob's collections + const bobReorderCall = mockPrisma.userCollection.updateMany.mock.calls[0][0]; + expect(bobReorderCall.where.userUid).toBe(bob.uid); + expect(bobReorderCall.where.parentID).toBe(null); + }); + + test('SCENARIO: Both users reorder collections simultaneously - no cross-contamination', async () => { + /** + * Scenario: Both Alice and Bob reorder their collections at the "same time" + * + * Alice: Moves collection #3 before collection #1 (to position 1) + * Before: [#1, #2, #3] -> After: [#3, #1, #2] + * + * Bob: Moves collection #1 before collection #3 (to position 3) + * Before: [#1, #2, #3] -> After: [#2, #3, #1] + * + * Without the fix: All 6 root collections (3 Alice + 3 Bob) would be + * affected by each operation, causing data corruption. + */ + + // === Alice moves collection #3 to position 1 (before #1) === + mockPrisma.userCollection.findUniqueOrThrow + .mockResolvedValueOnce(aliceCollection3) + .mockResolvedValueOnce(aliceCollection1); + + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); + mockPrisma.userCollection.findFirst + .mockResolvedValueOnce(aliceCollection3) + .mockResolvedValueOnce(aliceCollection1); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 2 }); + mockPrisma.userCollection.update.mockResolvedValueOnce({ + ...aliceCollection3, + orderIndex: 1, + }); + + await userCollectionService.updateUserCollectionOrder( + aliceCollection3.id, + aliceCollection1.id, + alice.uid, + ); + + // Verify Alice's operation is scoped to Alice + const aliceReorderCall = mockPrisma.userCollection.updateMany.mock.calls[0][0]; + expect(aliceReorderCall.where.userUid).toBe(alice.uid); + expect(aliceReorderCall.where.parentID).toBe(null); + + // Reset for Bob + mockReset(mockPrisma); + + // === Bob moves collection #1 to position 3 (before #3) === + mockPrisma.userCollection.findUniqueOrThrow + .mockResolvedValueOnce(bobCollection1) + .mockResolvedValueOnce(bobCollection3); + + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); + mockPrisma.userCollection.findFirst + .mockResolvedValueOnce(bobCollection1) + .mockResolvedValueOnce(bobCollection3); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 1 }); + mockPrisma.userCollection.update.mockResolvedValueOnce({ + ...bobCollection1, + orderIndex: 2, + }); + + await userCollectionService.updateUserCollectionOrder( + bobCollection1.id, + bobCollection3.id, + bob.uid, + ); + + // Verify Bob's operation is scoped to Bob + const bobReorderCall = mockPrisma.userCollection.updateMany.mock.calls[0][0]; + expect(bobReorderCall.where.userUid).toBe(bob.uid); + expect(bobReorderCall.where.parentID).toBe(null); + }); + + test('SCENARIO: Alice moves child to root while Bob deletes root - isolated operations', async () => { + /** + * Complex scenario: + * 1. Alice has a child collection under aliceCollection1 + * 2. Alice moves that child to root (becomes a new root collection) + * 3. Bob deletes his bobCollection2 + * + * Both operations update orderIndex of root collections (parentID: null) + * Without the fix, they would interfere with each other. + */ + + const aliceChildCollection: DBUserCollection = { + id: 'alice-child', + orderIndex: 1, + parentID: aliceCollection1.id, + title: 'Alice Child Collection', + userUid: alice.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + // === Alice moves child collection to root === + jest + .spyOn(userCollectionService, 'getUserCollection') + .mockResolvedValueOnce(E.right(aliceChildCollection)); + + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.userCollection.findFirst.mockResolvedValueOnce(aliceCollection3); // Last root + mockPrisma.userCollection.update.mockResolvedValueOnce({ + ...aliceChildCollection, + parentID: null, + orderIndex: 4, + }); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 0 }); + + await userCollectionService.moveUserCollection( + aliceChildCollection.id, + null, // Move to root + alice.uid, + ); + + // Verify Alice's move-to-root only affects Alice's collections + const aliceMoveCall = mockPrisma.userCollection.updateMany.mock.calls[0][0]; + expect(aliceMoveCall.where.userUid).toBe(alice.uid); + expect(aliceMoveCall.where.parentID).toBe(aliceChildCollection.parentID); + + // Reset mocks + mockReset(mockPrisma); + jest.restoreAllMocks(); + + // === Bob deletes his collection #2 === + mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( + bobCollection2, + ); + mockPrisma.userCollection.findMany.mockResolvedValueOnce([]); + mockPrisma.userRequest.deleteMany.mockResolvedValueOnce({ count: 0 }); + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.userCollection.delete.mockResolvedValueOnce(bobCollection2); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 1 }); + + await userCollectionService.deleteUserCollection( + bobCollection2.id, + bob.uid, + ); + + // Verify Bob's delete only affects Bob's collections + const bobDeleteCall = mockPrisma.userCollection.updateMany.mock.calls[0][0]; + expect(bobDeleteCall.where.userUid).toBe(bob.uid); + expect(bobDeleteCall.where.parentID).toBe(null); + expect(bobDeleteCall.where.orderIndex).toEqual({ gt: bobCollection2.orderIndex }); + }); + }); + + test('FIXED: moveUserCollection (child to root) - updateMany now filters by userUid', async () => { + const user1ChildCollection: DBUserCollection = { + id: 'user1-child-coll', + orderIndex: 2, + parentID: 'user1-parent-id', + title: 'User 1 Child Collection', + userUid: user.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + jest + .spyOn(userCollectionService, 'getUserCollection') + .mockResolvedValueOnce(E.right(user1ChildCollection)); + + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.userCollection.findFirst.mockResolvedValueOnce(null); + mockPrisma.userCollection.update.mockResolvedValueOnce({ + ...user1ChildCollection, + parentID: null, + orderIndex: 1, + }); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 1 }); + + await userCollectionService.moveUserCollection( + user1ChildCollection.id, + null, + user.uid, + ); + + expect(mockPrisma.userCollection.updateMany).toHaveBeenCalled(); + const updateManyCall = mockPrisma.userCollection.updateMany.mock.calls[0][0]; + + // FIXED: The where clause now includes userUid to prevent cross-user data corruption + expect(updateManyCall.where).toEqual({ + userUid: user1ChildCollection.userUid, + parentID: user1ChildCollection.parentID, + orderIndex: { gt: user1ChildCollection.orderIndex }, + }); + }); + + test('FIXED: updateUserCollectionOrder (to end) - updateMany now filters by userUid', async () => { + const user1RootCollection: DBUserCollection = { + id: 'user1-root-coll', + orderIndex: 2, + parentID: null, + title: 'User 1 Root Collection', + userUid: user.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( + user1RootCollection, + ); + + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); + mockPrisma.userCollection.findFirst.mockResolvedValueOnce( + user1RootCollection, + ); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 3 }); + mockPrisma.userCollection.count.mockResolvedValueOnce(5); + mockPrisma.userCollection.update.mockResolvedValueOnce({ + ...user1RootCollection, + orderIndex: 5, + }); + + await userCollectionService.updateUserCollectionOrder( + user1RootCollection.id, + null, + user.uid, + ); + + const updateManyCall = mockPrisma.userCollection.updateMany.mock.calls[0][0]; + + // FIXED: Now includes userUid - only affects current user's root collections + expect(updateManyCall.where).toEqual({ + userUid: user1RootCollection.userUid, + parentID: null, + orderIndex: { gte: user1RootCollection.orderIndex + 1 }, + }); + }); + + test('FIXED: updateUserCollectionOrder (with nextCollection) - updateMany now filters by userUid', async () => { + const user1RootCollection1: DBUserCollection = { + id: 'user1-root-1', + orderIndex: 1, + parentID: null, + title: 'User 1 Root 1', + userUid: user.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + const user1RootCollection2: DBUserCollection = { + id: 'user1-root-2', + orderIndex: 4, + parentID: null, + title: 'User 1 Root 2', + userUid: user.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + mockPrisma.userCollection.findUniqueOrThrow + .mockResolvedValueOnce(user1RootCollection1) + .mockResolvedValueOnce(user1RootCollection2); + + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.lockUserCollectionByParent.mockResolvedValue(undefined); + mockPrisma.userCollection.findFirst + .mockResolvedValueOnce(user1RootCollection1) + .mockResolvedValueOnce(user1RootCollection2); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 2 }); + mockPrisma.userCollection.update.mockResolvedValueOnce({ + ...user1RootCollection1, + orderIndex: 3, + }); + + await userCollectionService.updateUserCollectionOrder( + user1RootCollection1.id, + user1RootCollection2.id, + user.uid, + ); + + const updateManyCall = mockPrisma.userCollection.updateMany.mock.calls[0][0]; + + // FIXED: Now includes userUid - only affects current user's root collections + expect(updateManyCall.where).toEqual({ + userUid: user1RootCollection1.userUid, + parentID: null, + orderIndex: { + gte: user1RootCollection1.orderIndex + 1, + lte: user1RootCollection2.orderIndex - 1, + }, + }); + }); + + test('FIXED: deleteUserCollection - removeCollectionAndUpdateSiblingsOrderIndex now filters by userUid', async () => { + const user1RootToDelete: DBUserCollection = { + id: 'user1-root-to-delete', + orderIndex: 2, + parentID: null, + title: 'User 1 Root To Delete', + userUid: user.uid, + type: ReqType.REST, + createdOn: currentTime, + updatedOn: currentTime, + data: {}, + }; + + mockPrisma.userCollection.findUniqueOrThrow.mockResolvedValueOnce( + user1RootToDelete, + ); + + mockPrisma.userCollection.findMany.mockResolvedValueOnce([]); + mockPrisma.userRequest.deleteMany.mockResolvedValueOnce({ count: 0 }); + + mockPrisma.$transaction.mockImplementation(async (fn) => fn(mockPrisma)); + mockPrisma.userCollection.delete.mockResolvedValueOnce(user1RootToDelete); + mockPrisma.userCollection.updateMany.mockResolvedValueOnce({ count: 3 }); + + await userCollectionService.deleteUserCollection( + user1RootToDelete.id, + user.uid, + ); + + const updateManyCall = mockPrisma.userCollection.updateMany.mock.calls[0][0]; + + // FIXED: Now includes userUid - only affects current user's root collections + expect(updateManyCall.where).toEqual({ + userUid: user1RootToDelete.userUid, + parentID: null, + orderIndex: { gt: user1RootToDelete.orderIndex }, + }); + }); + + test('FIXED: getCollectionCount - now requires userUid parameter and filters by it', async () => { + mockPrisma.userCollection.count.mockResolvedValueOnce(10); + + await userCollectionService.getCollectionCount(null, user.uid); + + // FIXED: Now filters by userUid - only counts current user's collections + expect(mockPrisma.userCollection.count).toHaveBeenCalledWith({ + where: { + parentID: null, + userUid: user.uid, + }, + }); + }); +}); + describe('updateUserCollection', () => { test('should throw USER_COLL_DATA_INVALID is collection data is invalid', async () => { const result = await userCollectionService.updateUserCollection( diff --git a/packages/hoppscotch-backend/src/user-collection/user-collection.service.ts b/packages/hoppscotch-backend/src/user-collection/user-collection.service.ts index 270c441b..66dc179d 100644 --- a/packages/hoppscotch-backend/src/user-collection/user-collection.service.ts +++ b/packages/hoppscotch-backend/src/user-collection/user-collection.service.ts @@ -175,9 +175,9 @@ export class UserCollectionService { * @param collectionID The collection ID * @returns An Either of the Collection details */ - async getUserCollection(collectionID: string) { + async getUserCollection(collectionID: string, tx: Prisma.TransactionClient | null = null) { try { - const userCollection = await this.prisma.userCollection.findUniqueOrThrow( + const userCollection = await (tx || this.prisma).userCollection.findUniqueOrThrow( { where: { id: collectionID } }, ); return E.right(userCollection); @@ -212,26 +212,28 @@ export class UserCollectionService { data = jsonReq.right; } - // If creating a child collection - if (parentID !== null) { - const parentCollection = await this.getUserCollection(parentID); - if (E.isLeft(parentCollection)) return E.left(parentCollection.left); - - // Check to see if parentUserCollectionID belongs to this User - if (parentCollection.right.userUid !== user.uid) - return E.left(USER_NOT_OWNER); - - // Check to see if parent collection is of the same type of new collection being created - if (parentCollection.right.type !== type) - return E.left(USER_COLL_NOT_SAME_TYPE); - } + let userCollection: UserCollection = null; try { userCollection = await this.prisma.$transaction(async (tx) => { try { + // If creating a child collection + if (parentID !== null) { + const parentCollection = await this.getUserCollection(parentID, tx); + if (E.isLeft(parentCollection)) throw Error(parentCollection.left); + + // Check to see if parentUserCollectionID belongs to this User + if (parentCollection.right.userUid !== user.uid) + throw Error(USER_NOT_OWNER); + + // Check to see if parent collection is of the same type of new collection being created + if (parentCollection.right.type !== type) + throw Error(USER_COLL_NOT_SAME_TYPE); + } // lock the rows - await this.prisma.lockTableExclusive(tx, 'UserCollection'); + await this.prisma.lockUserCollectionByParent(tx, user.uid, parentID); + // fetch last user collection const lastUserCollection = await tx.userCollection.findFirst({ @@ -380,50 +382,6 @@ export class UserCollectionService { } } - /** - * Delete child collection and requests of a UserCollection - * - * @param collectionID The Collection Id - * @returns A Boolean of deletion status - */ - private async deleteCollectionData(collection: UserCollection) { - // Get all child collections in collectionID - const childCollectionList = await this.prisma.userCollection.findMany({ - where: { - parentID: collection.id, - }, - }); - - // Delete child collections - await Promise.all( - childCollectionList.map((coll) => - this.deleteUserCollection(coll.id, coll.userUid), - ), - ); - - // Delete all requests in collectionID - await this.prisma.userRequest.deleteMany({ - where: { - collectionID: collection.id, - }, - }); - - // Update orderIndexes in userCollection table for user - const isDeleted = await this.removeCollectionAndUpdateSiblingsOrderIndex( - collection, - { gt: collection.orderIndex }, - { decrement: 1 }, - ); - if (E.isLeft(isDeleted)) return E.left(isDeleted.left); - - this.pubsub.publish(`user_coll/${collection.userUid}/deleted`, { - id: collection.id, - type: ReqType[collection.type], - }); - - return E.right(true); - } - /** * Delete a UserCollection * @@ -440,8 +398,18 @@ export class UserCollectionService { if (collection.right.userUid !== userID) return E.left(USER_NOT_OWNER); // Delete all child collections and requests in the collection - const collectionData = await this.deleteCollectionData(collection.right); - if (E.isLeft(collectionData)) return E.left(collectionData.left); + const isDeleted = await this.removeCollectionAndUpdateSiblingsOrderIndex( + collection.right, + { gt: collection.right.orderIndex }, + { decrement: 1 }, + userID, + ); + if (E.isLeft(isDeleted)) return E.left(isDeleted.left); + + this.pubsub.publish(`user_coll/${collection.right.userUid}/deleted`, { + id: collection.right.id, + type: ReqType[collection.right.type], + }); return E.right(true); } @@ -454,55 +422,41 @@ export class UserCollectionService { * @returns If successful return an Either of collection or error message */ private async changeParentAndUpdateOrderIndex( + tx: Prisma.TransactionClient, collection: UserCollection, newParentID: string | null, ) { - let updatedCollection: UserCollection = null; - - try { - await this.prisma.$transaction(async (tx) => { - try { - // fetch last collection - const lastCollectionUnderNewParent = - await tx.userCollection.findFirst({ - where: { userUid: collection.userUid, parentID: newParentID }, - orderBy: { orderIndex: 'desc' }, - }); - - // update collection's parentID and orderIndex - updatedCollection = await tx.userCollection.update({ - where: { id: collection.id }, - data: { - // if parentCollectionID == null, collection becomes root collection - // if parentCollectionID != null, collection becomes child collection - parentID: newParentID, - orderIndex: lastCollectionUnderNewParent - ? lastCollectionUnderNewParent.orderIndex + 1 - : 1, - }, - }); - - // decrement orderIndex of all next sibling collections from original collection - await tx.userCollection.updateMany({ - where: { - parentID: collection.parentID, - orderIndex: { gt: collection.orderIndex }, - }, - data: { orderIndex: { decrement: 1 } }, - }); - } catch (error) { - throw new ConflictException(error); - } + // fetch last collection + const lastCollectionUnderNewParent = + await tx.userCollection.findFirst({ + where: { userUid: collection.userUid, parentID: newParentID }, + orderBy: { orderIndex: 'desc' }, }); - return E.right(updatedCollection); - } catch (error) { - console.error( - 'Error from UserCollectionService.changeParentAndUpdateOrderIndex:', - error, - ); - return E.left(USER_COLL_REORDERING_FAILED); - } + // update collection's parentID and orderIndex + const updatedCollection = await tx.userCollection.update({ + where: { id: collection.id }, + data: { + // if parentCollectionID == null, collection becomes root collection + // if parentCollectionID != null, collection becomes child collection + parentID: newParentID, + orderIndex: lastCollectionUnderNewParent + ? lastCollectionUnderNewParent.orderIndex + 1 + : 1, + }, + }); + + // decrement orderIndex of all next sibling collections from original collection + await tx.userCollection.updateMany({ + where: { + userUid: collection.userUid, + parentID: collection.parentID, + orderIndex: { gt: collection.orderIndex }, + }, + data: { orderIndex: { decrement: 1 } }, + }); + + return E.right(updatedCollection); } /** @@ -515,6 +469,7 @@ export class UserCollectionService { private async isParent( collection: UserCollection, destCollection: UserCollection, + tx: Prisma.TransactionClient | null = null, ): Promise> { // Check if collection and destCollection are same if (collection === destCollection) { @@ -528,12 +483,13 @@ export class UserCollectionService { // Get collection details of collection one step above in the tree i.e the parent collection const parentCollection = await this.getUserCollection( destCollection.parentID, + tx, ); if (E.isLeft(parentCollection)) { return O.none; } // Call isParent again now with parent collection - return await this.isParent(collection, parentCollection.right); + return await this.isParent(collection, parentCollection.right, tx); } else { return O.some(true); } @@ -550,6 +506,7 @@ export class UserCollectionService { collection: UserCollection, orderIndexCondition: Prisma.IntFilter, dataCondition: Prisma.IntFieldUpdateOperationsInput, + userID: string, ) { let retryCount = 0; while (retryCount < this.MAX_RETRIES) { @@ -557,20 +514,25 @@ export class UserCollectionService { await this.prisma.$transaction(async (tx) => { try { // lock the rows - await this.prisma.lockTableExclusive(tx, 'UserCollection'); + await this.prisma.lockUserCollectionByParent(tx, userID, collection.parentID); - await tx.userCollection.delete({ + const deletedCollection = await tx.userCollection.delete({ where: { id: collection.id }, }); - // update orderIndexes - await tx.userCollection.updateMany({ - where: { - parentID: collection.parentID, - orderIndex: orderIndexCondition, - }, - data: { orderIndex: dataCondition }, - }); + // if collection is deleted, update siblings orderIndexes + // if collection was deleted before the transaction started (race condition), do not update siblings orderIndexes + if (deletedCollection) { + // update orderIndexes + await tx.userCollection.updateMany({ + where: { + userUid: collection.userUid, + parentID: collection.parentID, + orderIndex: orderIndexCondition, + }, + data: { orderIndex: dataCondition }, + }); + } } catch (error) { throw new ConflictException(error); } @@ -612,91 +574,109 @@ export class UserCollectionService { destCollectionID: string | null, userID: string, ) { - // Get collection details of collectionID - const collection = await this.getUserCollection(userCollectionID); - if (E.isLeft(collection)) return E.left(USER_COLL_NOT_FOUND); + try { + return await this.prisma.$transaction(async (tx) => { + // Get collection details of collectionID + const collection = await this.getUserCollection(userCollectionID, tx); + if (E.isLeft(collection)) return E.left(USER_COLL_NOT_FOUND); + + // Check to see is the collection belongs to the user + if (collection.right.userUid !== userID) return E.left(USER_NOT_OWNER); + + // destCollectionID == null i.e move collection to root + if (!destCollectionID) { + if (!collection.right.parentID) { + // collection is a root collection + // Throw error if collection is already a root collection + return E.left(USER_COLL_ALREADY_ROOT); + } + + // Change parent from child to root i.e child collection becomes a root collection + // Move child collection into root and update orderIndexes for child userCollections + const updatedCollection = await this.changeParentAndUpdateOrderIndex( + tx, + collection.right, + null, + ); + if (E.isLeft(updatedCollection)) return E.left(updatedCollection.left); + + this.pubsub.publish( + `user_coll/${collection.right.userUid}/moved`, + this.cast(updatedCollection.right), + ); + + return E.right(this.cast(updatedCollection.right)); + } + + // destCollectionID != null i.e move into another collection + if (userCollectionID === destCollectionID) { + // Throw error if collectionID and destCollectionID are the same + return E.left(USER_COLL_DEST_SAME); + } + + // Get collection details of destCollectionID + const destCollection = await this.getUserCollection(destCollectionID, tx); + if (E.isLeft(destCollection)) return E.left(USER_COLL_NOT_FOUND); + + // Check if collection and destCollection belong to the same collection type + if (collection.right.type !== destCollection.right.type) { + return E.left(USER_COLL_NOT_SAME_TYPE); + } + + // Check if collection and destCollection belong to the same user account + if (collection.right.userUid !== destCollection.right.userUid) { + return E.left(USER_COLL_NOT_SAME_USER); + } + + // Check if collection is present on the parent tree for destCollection + const checkIfParent = await this.isParent( + collection.right, + destCollection.right, + tx, + ); + if (O.isNone(checkIfParent)) { + return E.left(USER_COLL_IS_PARENT_COLL); + } + + // Change parent from null to teamCollection i.e collection becomes a child collection + // Move root/child collection into another child collection and update orderIndexes of the previous parent + const updatedCollection = await this.changeParentAndUpdateOrderIndex( + tx, + collection.right, + destCollection.right.id, + ); + if (E.isLeft(updatedCollection)) return E.left(updatedCollection.left); + + this.pubsub.publish( + `user_coll/${collection.right.userUid}/moved`, + this.cast(updatedCollection.right), + ); + + return E.right(this.cast(updatedCollection.right)); + }); + } catch (error) { - // Check to see is the collection belongs to the user - if (collection.right.userUid !== userID) return E.left(USER_NOT_OWNER); - - // destCollectionID == null i.e move collection to root - if (!destCollectionID) { - if (!collection.right.parentID) { - // collection is a root collection - // Throw error if collection is already a root collection - return E.left(USER_COLL_ALREADY_ROOT); - } - - // Change parent from child to root i.e child collection becomes a root collection - // Move child collection into root and update orderIndexes for child userCollections - const updatedCollection = await this.changeParentAndUpdateOrderIndex( - collection.right, - null, + console.error( + 'Error from UserCollectionService.moveUserCollection', + error, ); - if (E.isLeft(updatedCollection)) return E.left(updatedCollection.left); - - this.pubsub.publish( - `user_coll/${collection.right.userUid}/moved`, - this.cast(updatedCollection.right), - ); - - return E.right(this.cast(updatedCollection.right)); + return E.left(USER_COLL_REORDERING_FAILED); } - - // destCollectionID != null i.e move into another collection - if (userCollectionID === destCollectionID) { - // Throw error if collectionID and destCollectionID are the same - return E.left(USER_COLL_DEST_SAME); - } - - // Get collection details of destCollectionID - const destCollection = await this.getUserCollection(destCollectionID); - if (E.isLeft(destCollection)) return E.left(USER_COLL_NOT_FOUND); - - // Check if collection and destCollection belong to the same collection type - if (collection.right.type !== destCollection.right.type) { - return E.left(USER_COLL_NOT_SAME_TYPE); - } - - // Check if collection and destCollection belong to the same user account - if (collection.right.userUid !== destCollection.right.userUid) { - return E.left(USER_COLL_NOT_SAME_USER); - } - - // Check if collection is present on the parent tree for destCollection - const checkIfParent = await this.isParent( - collection.right, - destCollection.right, - ); - if (O.isNone(checkIfParent)) { - return E.left(USER_COLL_IS_PARENT_COLL); - } - - // Change parent from null to teamCollection i.e collection becomes a child collection - // Move root/child collection into another child collection and update orderIndexes of the previous parent - const updatedCollection = await this.changeParentAndUpdateOrderIndex( - collection.right, - destCollection.right.id, - ); - if (E.isLeft(updatedCollection)) return E.left(updatedCollection.left); - - this.pubsub.publish( - `user_coll/${collection.right.userUid}/moved`, - this.cast(updatedCollection.right), - ); - - return E.right(this.cast(updatedCollection.right)); } /** - * Find the number of child collections present in collectionID + * Find the number of child collections present in collectionID for a specific user * - * @param collectionID The Collection ID + * @param collectionID The Collection ID (parent collection ID, or null for root) + * @param userUid The User UID * @returns Number of collections */ - getCollectionCount(collectionID: string): Promise { - return this.prisma.userCollection.count({ - where: { parentID: collectionID }, + getCollectionCount(collectionID: string, userUid: string, tx: Prisma.TransactionClient | null = null): Promise { + return (tx || this.prisma).userCollection.count({ + where: { + parentID: collectionID, + userUid: userUid, + }, }); } @@ -730,35 +710,39 @@ export class UserCollectionService { await this.prisma.$transaction(async (tx) => { try { // Step 0: lock the rows - await this.prisma.acquireLocks( - tx, - 'UserCollection', - userID, - collection.right.parentID, - ); + await this.prisma.lockUserCollectionByParent(tx, userID, collection.right.parentID); - // Step 1: Decrement orderIndex of all items that come after collection.orderIndex till end of list of items const collectionInTx = await tx.userCollection.findFirst({ where: { id: collectionID }, select: { orderIndex: true }, }); - await tx.userCollection.updateMany({ - where: { - parentID: collection.right.parentID, - orderIndex: { gte: collectionInTx.orderIndex + 1 }, - }, - data: { orderIndex: { decrement: 1 } }, - }); - // Step 2: Update orderIndex of collection to length of list - await tx.userCollection.update({ - where: { id: collection.right.id }, - data: { - orderIndex: await this.getCollectionCount( - collection.right.parentID, - ), - }, - }); + // if collection is found, update orderIndexes of siblings + // if collection was deleted before the transaction started (race condition), do not update siblings orderIndexes + if(collectionInTx) { + // Step 1: Decrement orderIndex of all items that come after collection.orderIndex till end of list of items + await tx.userCollection.updateMany({ + where: { + userUid: collection.right.userUid, + parentID: collection.right.parentID, + orderIndex: { gte: collectionInTx.orderIndex + 1 }, + }, + data: { orderIndex: { decrement: 1 } }, + }); + + // Step 2: Update orderIndex of collection to length of list + await tx.userCollection.update({ + where: { id: collection.right.id }, + data: { + orderIndex: await this.getCollectionCount( + collection.right.parentID, + collection.right.userUid, + tx, + ), + }, + }); + } + } catch (error) { throw new ConflictException(error); } @@ -795,12 +779,7 @@ export class UserCollectionService { await this.prisma.$transaction(async (tx) => { try { // Step 0: lock the rows - await this.prisma.acquireLocks( - tx, - 'UserCollection', - userID, - subsequentCollection.right.parentID, - ); + await this.prisma.lockUserCollectionByParent(tx, userID, subsequentCollection.right.parentID); // subsequentCollectionInTx and subsequentCollection are same, just to make sure, orderIndex value is concrete const collectionInTx = await tx.userCollection.findFirst({ @@ -812,38 +791,44 @@ export class UserCollectionService { select: { orderIndex: true }, }); - // Step 1: Determine if we are moving collection up or down the list - const isMovingUp = - subsequentCollectionInTx.orderIndex < collectionInTx.orderIndex; + // if collection and subsequentCollection are found, update orderIndexes of siblings + // if collection or subsequentCollection was deleted before the transaction started (race condition), do not update siblings orderIndexes + if(collectionInTx && subsequentCollectionInTx) { + // Step 1: Determine if we are moving collection up or down the list + const isMovingUp = + subsequentCollectionInTx.orderIndex < collectionInTx.orderIndex; + + // Step 2: Update OrderIndex of items in list depending on moving up or down + const updateFrom = isMovingUp + ? subsequentCollectionInTx.orderIndex + : collectionInTx.orderIndex + 1; + + const updateTo = isMovingUp + ? collectionInTx.orderIndex - 1 + : subsequentCollectionInTx.orderIndex - 1; + + await tx.userCollection.updateMany({ + where: { + userUid: collection.right.userUid, + parentID: collection.right.parentID, + orderIndex: { gte: updateFrom, lte: updateTo }, + }, + data: { + orderIndex: isMovingUp ? { increment: 1 } : { decrement: 1 }, + }, + }); + + // Step 3: Update OrderIndex of collection + await tx.userCollection.update({ + where: { id: collection.right.id }, + data: { + orderIndex: isMovingUp + ? subsequentCollectionInTx.orderIndex + : subsequentCollectionInTx.orderIndex - 1, + }, + }); + } - // Step 2: Update OrderIndex of items in list depending on moving up or down - const updateFrom = isMovingUp - ? subsequentCollectionInTx.orderIndex - : collectionInTx.orderIndex + 1; - - const updateTo = isMovingUp - ? collectionInTx.orderIndex - 1 - : subsequentCollectionInTx.orderIndex - 1; - - await tx.userCollection.updateMany({ - where: { - parentID: collection.right.parentID, - orderIndex: { gte: updateFrom, lte: updateTo }, - }, - data: { - orderIndex: isMovingUp ? { increment: 1 } : { decrement: 1 }, - }, - }); - - // Step 3: Update OrderIndex of collection - await tx.userCollection.update({ - where: { id: collection.right.id }, - data: { - orderIndex: isMovingUp - ? subsequentCollectionInTx.orderIndex - : subsequentCollectionInTx.orderIndex - 1, - }, - }); } catch (error) { throw new ConflictException(error); } @@ -1119,7 +1104,7 @@ export class UserCollectionService { await this.prisma.$transaction(async (tx) => { try { // lock the rows - await this.prisma.lockTableExclusive(tx, 'UserCollection'); + await this.prisma.lockUserCollectionByParent(tx, userID, destCollectionID); // Get the last order index const lastCollection = await tx.userCollection.findFirst({ @@ -1362,7 +1347,7 @@ export class UserCollectionService { try { await this.prisma.$transaction(async (tx) => { - await this.prisma.acquireLocks(tx, 'UserCollection', userID, parentID); + await this.prisma.lockUserCollectionByParent(tx, userID, parentID); const collections = await tx.userCollection.findMany({ where: { userUid: userID, parentID }, diff --git a/packages/hoppscotch-backend/src/user-request/user-request.service.ts b/packages/hoppscotch-backend/src/user-request/user-request.service.ts index 05da9b56..2f45c60a 100644 --- a/packages/hoppscotch-backend/src/user-request/user-request.service.ts +++ b/packages/hoppscotch-backend/src/user-request/user-request.service.ts @@ -99,8 +99,8 @@ export class UserRequestService { * @param user User who owns the collection * @returns Number of requests in the collection */ - getRequestsCountInCollection(collectionID: string): Promise { - return this.prisma.userRequest.count({ + getRequestsCountInCollection(collectionID: string, tx: Prisma.TransactionClient | null = null): Promise { + return (tx || this.prisma).userRequest.count({ where: { collectionID }, }); } @@ -139,7 +139,7 @@ export class UserRequestService { newRequest = await this.prisma.$transaction(async (tx) => { try { // lock the rows - await this.prisma.lockTableExclusive(tx, 'UserRequest'); + await this.prisma.lockUserRequestByCollections(tx, user.uid, [collectionID]); // fetch last user request const lastUserRequest = await tx.userRequest.findFirst({ @@ -235,17 +235,21 @@ export class UserRequestService { await this.prisma.$transaction(async (tx) => { try { // lock the rows - await this.prisma.lockTableExclusive(tx, 'UserRequest'); + await this.prisma.lockUserRequestByCollections(tx, user.uid, [request.collectionID]); - await tx.userRequest.updateMany({ - where: { - collectionID: request.collectionID, - orderIndex: { gt: request.orderIndex }, - }, - data: { orderIndex: { decrement: 1 } }, - }); + const deletedRequest = await tx.userRequest.delete({ where: { id } }); - await tx.userRequest.delete({ where: { id } }); + // if request is found, update orderIndexes of siblings + // if request was deleted before the transaction started (race condition), do not update siblings orderIndexes + if(deletedRequest) { + await tx.userRequest.updateMany({ + where: { + collectionID: request.collectionID, + orderIndex: { gt: request.orderIndex }, + }, + data: { orderIndex: { decrement: 1 } }, + }); + } } catch (error) { throw new ConflictException(error); } @@ -412,10 +416,7 @@ export class UserRequestService { E.Left | E.Right >(async (tx) => { // lock the rows - await this.prisma.acquireLocks(tx, 'UserRequest', null, null, [ - request.id, - nextRequest?.id, - ]); + await this.prisma.lockUserRequestByCollections(tx, request.userUid, [srcCollID, destCollID]); request = await tx.userRequest.findUnique({ where: { id: request.id }, @@ -426,64 +427,67 @@ export class UserRequestService { }) : null; - const isSameCollection = srcCollID === destCollID; - const isMovingUp = nextRequest?.orderIndex < request.orderIndex; // false, if nextRequest is null - - const nextReqOrderIndex = nextRequest?.orderIndex; - const reqCountInDestColl = nextRequest - ? undefined - : await this.getRequestsCountInCollection(destCollID); - - // Updating order indexes of other requests in collection(s) - if (isSameCollection) { - const updateFrom = isMovingUp - ? nextReqOrderIndex - : request.orderIndex + 1; - const updateTo = isMovingUp ? request.orderIndex : nextReqOrderIndex; - - await tx.userRequest.updateMany({ - where: { - collectionID: srcCollID, - orderIndex: { gte: updateFrom, lt: updateTo }, - }, - data: { - orderIndex: isMovingUp ? { increment: 1 } : { decrement: 1 }, - }, - }); - } else { - await tx.userRequest.updateMany({ - where: { - collectionID: srcCollID, - orderIndex: { gte: request.orderIndex }, - }, - data: { orderIndex: { decrement: 1 } }, - }); - - if (nextRequest) { + // Check again if request is found in transaction, update orderIndexes of siblings + // if request was deleted before the transaction started (race condition), do not update siblings orderIndexes + if(request) { + const isSameCollection = srcCollID === destCollID; + const isMovingUp = nextRequest?.orderIndex < request.orderIndex; // false, if nextRequest is null + + const nextReqOrderIndex = nextRequest?.orderIndex; + const reqCountInDestColl = nextRequest + ? undefined + : await this.getRequestsCountInCollection(destCollID); + + // Updating order indexes of other requests in collection(s) + if (isSameCollection) { + const updateFrom = isMovingUp + ? nextReqOrderIndex + : request.orderIndex + 1; + const updateTo = isMovingUp ? request.orderIndex : nextReqOrderIndex; + await tx.userRequest.updateMany({ where: { - collectionID: destCollID, - orderIndex: { gte: nextReqOrderIndex }, + collectionID: srcCollID, + orderIndex: { gte: updateFrom, lt: updateTo }, + }, + data: { + orderIndex: isMovingUp ? { increment: 1 } : { decrement: 1 }, }, - data: { orderIndex: { increment: 1 } }, }); + } else { + await tx.userRequest.updateMany({ + where: { + collectionID: srcCollID, + orderIndex: { gte: request.orderIndex }, + }, + data: { orderIndex: { decrement: 1 } }, + }); + + if (nextRequest) { + await tx.userRequest.updateMany({ + where: { + collectionID: destCollID, + orderIndex: { gte: nextReqOrderIndex }, + }, + data: { orderIndex: { increment: 1 } }, + }); + } } + + // Updating order index of the request + let adjust: number; + if (isSameCollection) adjust = nextRequest ? (isMovingUp ? 0 : -1) : 0; + else adjust = nextRequest ? 0 : 1; + + const newOrderIndex = + (nextReqOrderIndex ?? reqCountInDestColl) + adjust; + + const updatedRequest = await tx.userRequest.update({ + where: { id: request.id }, + data: { orderIndex: newOrderIndex, collectionID: destCollID }, + }); + return E.right(updatedRequest); } - - // Updating order index of the request - let adjust: number; - if (isSameCollection) adjust = nextRequest ? (isMovingUp ? 0 : -1) : 0; - else adjust = nextRequest ? 0 : 1; - - const newOrderIndex = - (nextReqOrderIndex ?? reqCountInDestColl) + adjust; - - const updatedRequest = await tx.userRequest.update({ - where: { id: request.id }, - data: { orderIndex: newOrderIndex, collectionID: destCollID }, - }); - - return E.right(updatedRequest); }); } catch (error) { console.error('Error from UserRequestService.reorderRequests', error); @@ -516,9 +520,7 @@ export class UserRequestService { try { await this.prisma.$transaction(async (tx) => { - await this.prisma.acquireLocks(tx, 'UserRequest', userUid, null, [ - collectionID, - ]); + await this.prisma.lockUserRequestByCollections(tx, userUid, [collectionID]); const userRequests = await tx.userRequest.findMany({ where: { userUid, collectionID },