From 6500f18b9b80c5c9c473489e7e740d55dca74303 Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Wed, 14 Feb 2024 15:29:10 +0100 Subject: [PATCH] chore(): Prevent from soft deleting all entities (#6396) **What** The internal service abstraction should return idempotently if an empty array is given in order to prevent full soft deletion of the full data --- .changeset/spicy-scissors-lay.md | 5 ++ .../abstract-module-service-factory.spec.ts | 24 +++++----- .../internal-module-service-factory.spec.ts | 47 +++++++++++++------ .../internal-module-service-factory.ts | 6 ++- 4 files changed, 54 insertions(+), 28 deletions(-) create mode 100644 .changeset/spicy-scissors-lay.md diff --git a/.changeset/spicy-scissors-lay.md b/.changeset/spicy-scissors-lay.md new file mode 100644 index 0000000000..e101bf330e --- /dev/null +++ b/.changeset/spicy-scissors-lay.md @@ -0,0 +1,5 @@ +--- +"@medusajs/utils": patch +--- + +chore(): Prevent from soft deleting all entities diff --git a/packages/utils/src/modules-sdk/__tests__/abstract-module-service-factory.spec.ts b/packages/utils/src/modules-sdk/__tests__/abstract-module-service-factory.spec.ts index 35935e84ed..59717981ec 100644 --- a/packages/utils/src/modules-sdk/__tests__/abstract-module-service-factory.spec.ts +++ b/packages/utils/src/modules-sdk/__tests__/abstract-module-service-factory.spec.ts @@ -85,7 +85,7 @@ describe("Abstract Module Service Factory", () => { instance = new abstractModuleService(containerMock) }) - test("should have retrieve method", async () => { + it("should have retrieve method", async () => { const result = await instance.retrieve("1") expect(result).toEqual({ id: "1", name: "Item" }) expect(containerMock.mainModelMockService.retrieve).toHaveBeenCalledWith( @@ -95,7 +95,7 @@ describe("Abstract Module Service Factory", () => { ) }) - test("should have list method", async () => { + it("should have list method", async () => { const result = await instance.list() expect(result).toEqual([{ id: "1", name: "Item" }]) expect(containerMock.mainModelMockService.list).toHaveBeenCalledWith( @@ -105,7 +105,7 @@ describe("Abstract Module Service Factory", () => { ) }) - test("should have delete method", async () => { + it("should have delete method", async () => { await instance.delete("1") expect(containerMock.mainModelMockService.delete).toHaveBeenCalledWith( ["1"], @@ -113,7 +113,7 @@ describe("Abstract Module Service Factory", () => { ) }) - test("should have softDelete method", async () => { + it("should have softDelete method", async () => { const result = await instance.softDelete("1") expect(result).toEqual(undefined) expect( @@ -121,7 +121,7 @@ describe("Abstract Module Service Factory", () => { ).toHaveBeenCalledWith(["1"], defaultTransactionContext) }) - test("should have restore method", async () => { + it("should have restore method", async () => { const result = await instance.restore("1") expect(result).toEqual(undefined) expect(containerMock.mainModelMockService.restore).toHaveBeenCalledWith( @@ -130,7 +130,7 @@ describe("Abstract Module Service Factory", () => { ) }) - test("should have delete method with selector", async () => { + it("should have delete method with selector", async () => { await instance.delete({ selector: { id: "1" } }) expect(containerMock.mainModelMockService.delete).toHaveBeenCalledWith( [{ selector: { id: "1" } }], @@ -147,7 +147,7 @@ describe("Abstract Module Service Factory", () => { instance = new abstractModuleService(containerMock) }) - test("should have retrieve method for other models", async () => { + it("should have retrieve method for other models", async () => { const result = await instance.retrieveOtherModelMock1("1") expect(result).toEqual({ id: "1", name: "Item" }) expect( @@ -155,7 +155,7 @@ describe("Abstract Module Service Factory", () => { ).toHaveBeenCalledWith("1", undefined, defaultContext) }) - test("should have list method for other models", async () => { + it("should have list method for other models", async () => { const result = await instance.listOtherModelMock1s() expect(result).toEqual([{ id: "1", name: "Item" }]) expect(containerMock.otherModelMock1Service.list).toHaveBeenCalledWith( @@ -165,7 +165,7 @@ describe("Abstract Module Service Factory", () => { ) }) - test("should have delete method for other models", async () => { + it("should have delete method for other models", async () => { await instance.deleteOtherModelMock1s("1") expect(containerMock.otherModelMock1Service.delete).toHaveBeenCalledWith( ["1"], @@ -173,7 +173,7 @@ describe("Abstract Module Service Factory", () => { ) }) - test("should have softDelete method for other models", async () => { + it("should have softDelete method for other models", async () => { const result = await instance.softDeleteOtherModelMock1s("1") expect(result).toEqual(undefined) expect( @@ -181,7 +181,7 @@ describe("Abstract Module Service Factory", () => { ).toHaveBeenCalledWith(["1"], defaultTransactionContext) }) - test("should have restore method for other models", async () => { + it("should have restore method for other models", async () => { const result = await instance.restoreOtherModelMock1s("1") expect(result).toEqual(undefined) expect(containerMock.otherModelMock1Service.restore).toHaveBeenCalledWith( @@ -190,7 +190,7 @@ describe("Abstract Module Service Factory", () => { ) }) - test("should have delete method for other models with selector", async () => { + it("should have delete method for other models with selector", async () => { await instance.deleteOtherModelMock1s({ selector: { id: "1" } }) expect(containerMock.otherModelMock1Service.delete).toHaveBeenCalledWith( [{ selector: { id: "1" } }], diff --git a/packages/utils/src/modules-sdk/__tests__/internal-module-service-factory.spec.ts b/packages/utils/src/modules-sdk/__tests__/internal-module-service-factory.spec.ts index 2e4b9db5c4..b8825dd3e6 100644 --- a/packages/utils/src/modules-sdk/__tests__/internal-module-service-factory.spec.ts +++ b/packages/utils/src/modules-sdk/__tests__/internal-module-service-factory.spec.ts @@ -44,12 +44,12 @@ describe("Internal Module Service Factory", () => { instance = new internalModuleService(containerMock) }) - test("should throw model id undefined error on retrieve if id is not defined", async () => { + it("should throw model id undefined error on retrieve if id is not defined", async () => { const err = await instance.retrieve().catch((e) => e) expect(err.message).toBe("model - id must be defined") }) - test("should throw an error on retrieve if composite key values are not defined", async () => { + it("should throw an error on retrieve if composite key values are not defined", async () => { class CompositeModel { id: string name: string @@ -66,14 +66,14 @@ describe("Internal Module Service Factory", () => { expect(err.message).toBe("compositeModel - id, name must be defined") }) - test("should throw NOT_FOUND error on retrieve if entity not found", async () => { + it("should throw NOT_FOUND error on retrieve if entity not found", async () => { containerMock[modelRepositoryName].find.mockResolvedValueOnce([]) const err = await instance.retrieve("1").catch((e) => e) expect(err.message).toBe("Model with id: 1 was not found") }) - test("should retrieve entity successfully", async () => { + it("should retrieve entity successfully", async () => { const entity = { id: "1", name: "Item" } containerMock[modelRepositoryName].find.mockResolvedValueOnce([entity]) @@ -81,7 +81,7 @@ describe("Internal Module Service Factory", () => { expect(result).toEqual(entity) }) - test("should retrieve entity successfully with composite key", async () => { + it("should retrieve entity successfully with composite key", async () => { class CompositeModel { id: string name: string @@ -103,7 +103,7 @@ describe("Internal Module Service Factory", () => { expect(result).toEqual(entity) }) - test("should list entities successfully", async () => { + it("should list entities successfully", async () => { const entities = [ { id: "1", name: "Item" }, { id: "2", name: "Item2" }, @@ -114,7 +114,7 @@ describe("Internal Module Service Factory", () => { expect(result).toEqual(entities) }) - test("should list and count entities successfully", async () => { + it("should list and count entities successfully", async () => { const entities = [ { id: "1", name: "Item" }, { id: "2", name: "Item2" }, @@ -129,7 +129,7 @@ describe("Internal Module Service Factory", () => { expect(result).toEqual([entities, count]) }) - test("should create entity successfully", async () => { + it("should create entity successfully", async () => { const entity = { id: "1", name: "Item" } containerMock[modelRepositoryName].find.mockReturnValue([entity]) @@ -142,7 +142,7 @@ describe("Internal Module Service Factory", () => { expect(result).toEqual(entity) }) - test("should create entities successfully", async () => { + it("should create entities successfully", async () => { const entities = [ { id: "1", name: "Item" }, { id: "2", name: "Item2" }, @@ -156,7 +156,7 @@ describe("Internal Module Service Factory", () => { expect(result).toEqual(entities) }) - test("should update entity successfully", async () => { + it("should update entity successfully", async () => { const updateData = { id: "1", name: "UpdatedItem" } containerMock[modelRepositoryName].find.mockResolvedValueOnce([ @@ -171,7 +171,7 @@ describe("Internal Module Service Factory", () => { expect(result).toEqual([updateData]) }) - test("should update entities successfully", async () => { + it("should update entities successfully", async () => { const updateData = { id: "1", name: "UpdatedItem" } const entitiesToUpdate = [{ id: "1", name: "Item" }] @@ -189,7 +189,7 @@ describe("Internal Module Service Factory", () => { ]) }) - test("should delete entity successfully", async () => { + it("should delete entity successfully", async () => { await instance.delete("1") expect(containerMock[modelRepositoryName].delete).toHaveBeenCalledWith( { @@ -203,7 +203,7 @@ describe("Internal Module Service Factory", () => { ) }) - test("should delete entities successfully", async () => { + it("should delete entities successfully", async () => { const entitiesToDelete = [{ id: "1", name: "Item" }] containerMock[modelRepositoryName].find.mockResolvedValueOnce( entitiesToDelete @@ -222,14 +222,31 @@ describe("Internal Module Service Factory", () => { ) }) - test("should soft delete entity successfully", async () => { + it("should prevent from deleting all entities", async () => { + const entitiesToDelete = [{ id: "1", name: "Item" }] + containerMock[modelRepositoryName].find.mockResolvedValueOnce( + entitiesToDelete + ) + + await instance.delete([]) + expect(containerMock[modelRepositoryName].delete).not.toHaveBeenCalled() + }) + + it("should soft delete entity successfully", async () => { await instance.softDelete("1") expect( containerMock[modelRepositoryName].softDelete ).toHaveBeenCalledWith("1", defaultContext) }) - test("should restore entity successfully", async () => { + it("should prevent from soft deleting all data", async () => { + await instance.softDelete([]) + expect( + containerMock[modelRepositoryName].softDelete + ).not.toHaveBeenCalled() + }) + + it("should restore entity successfully", async () => { await instance.restore("1") expect(containerMock[modelRepositoryName].restore).toHaveBeenCalledWith( "1", diff --git a/packages/utils/src/modules-sdk/internal-module-service-factory.ts b/packages/utils/src/modules-sdk/internal-module-service-factory.ts index 62b2039a20..58af0431fc 100644 --- a/packages/utils/src/modules-sdk/internal-module-service-factory.ts +++ b/packages/utils/src/modules-sdk/internal-module-service-factory.ts @@ -312,7 +312,7 @@ export function internalModuleServiceFactory< ): Promise { if ( !isDefined(idOrSelector) || - (Array.isArray(idOrSelector) && idOrSelector.length === 0) + (Array.isArray(idOrSelector) && !idOrSelector.length) ) { return } @@ -402,6 +402,10 @@ export function internalModuleServiceFactory< idsOrFilter: string[] | InternalFilterQuery, @MedusaContext() sharedContext: Context = {} ): Promise<[TEntity[], Record]> { + if (Array.isArray(idsOrFilter) && !idsOrFilter.length) { + return [[], {}] + } + return await this[propertyRepositoryName].softDelete( idsOrFilter, sharedContext