From b42f151be31161a5d7a73132ee6794b990403d76 Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Tue, 25 Feb 2025 19:59:57 +0100 Subject: [PATCH] chore(): Improve internal repository delete algo (#11601) * chore(): Improve internal repository delete algo * chore(): Improve internal repository delete algo * chore(): Improve internal repository delete algo * update tests * Create purple-donkeys-learn.md * update tests --- .changeset/purple-donkeys-learn.md | 9 +++ .../core/types/src/dal/repository-service.ts | 7 ++- .../modules-sdk/medusa-internal-service.ts | 10 ++-- .../src/dal/mikro-orm/mikro-orm-repository.ts | 37 ++++++++++-- .../__tests__/medusa-internal-service.ts | 7 +-- .../__tests__/medusa-service.spec.ts | 6 +- .../modules-sdk/medusa-internal-service.ts | 56 +++++-------------- .../utils/src/modules-sdk/medusa-service.ts | 8 +-- .../src/services/inventory-module.ts | 2 +- .../link-modules/src/repositories/link.ts | 5 +- .../product-module-service/products.spec.ts | 15 +++-- packages/modules/product/package.json | 2 +- .../src/repositories/product-category.ts | 3 +- .../product/src/services/product-category.ts | 4 +- 14 files changed, 89 insertions(+), 82 deletions(-) create mode 100644 .changeset/purple-donkeys-learn.md diff --git a/.changeset/purple-donkeys-learn.md b/.changeset/purple-donkeys-learn.md new file mode 100644 index 0000000000..9f7424f092 --- /dev/null +++ b/.changeset/purple-donkeys-learn.md @@ -0,0 +1,9 @@ +--- +"@medusajs/inventory": patch +"@medusajs/link-modules": patch +"@medusajs/product": patch +"@medusajs/types": patch +"@medusajs/utils": patch +--- + +chore(): Improve internal repository delete algo diff --git a/packages/core/types/src/dal/repository-service.ts b/packages/core/types/src/dal/repository-service.ts index 6aae45c990..086b4c442c 100644 --- a/packages/core/types/src/dal/repository-service.ts +++ b/packages/core/types/src/dal/repository-service.ts @@ -70,7 +70,10 @@ export interface RepositoryService extends BaseRepositoryService { context?: Context ): Promise[]> - delete(idsOrPKs: FindOptions["where"], context?: Context): Promise + delete( + idsOrPKs: FindOptions["where"], + context?: Context + ): Promise /** * Soft delete entities and cascade to related entities if configured. @@ -127,7 +130,7 @@ export interface TreeRepositoryService extends BaseRepositoryService { context?: Context ): Promise[]> - delete(ids: string[], context?: Context): Promise + delete(ids: string[], context?: Context): Promise } /** diff --git a/packages/core/types/src/modules-sdk/medusa-internal-service.ts b/packages/core/types/src/modules-sdk/medusa-internal-service.ts index 04cebe171e..2e5321ce57 100644 --- a/packages/core/types/src/modules-sdk/medusa-internal-service.ts +++ b/packages/core/types/src/modules-sdk/medusa-internal-service.ts @@ -64,16 +64,16 @@ export interface IMedusaInternalService< sharedContext?: Context ): Promise[]> - delete(idOrSelector: string, sharedContext?: Context): Promise - delete(idOrSelector: string[], sharedContext?: Context): Promise - delete(idOrSelector: object, sharedContext?: Context): Promise - delete(idOrSelector: object[], sharedContext?: Context): Promise + delete(idOrSelector: string, sharedContext?: Context): Promise + delete(idOrSelector: string[], sharedContext?: Context): Promise + delete(idOrSelector: object, sharedContext?: Context): Promise + delete(idOrSelector: object[], sharedContext?: Context): Promise delete( idOrSelector: { selector: FilterQuery | BaseFilterable> }, sharedContext?: Context - ): Promise + ): Promise softDelete( idsOrFilter: diff --git a/packages/core/utils/src/dal/mikro-orm/mikro-orm-repository.ts b/packages/core/utils/src/dal/mikro-orm/mikro-orm-repository.ts index 8506bb4217..f458be5d78 100644 --- a/packages/core/utils/src/dal/mikro-orm/mikro-orm-repository.ts +++ b/packages/core/utils/src/dal/mikro-orm/mikro-orm-repository.ts @@ -162,7 +162,10 @@ export class MikroOrmBaseRepository throw new Error("Method not implemented.") } - delete(idsOrPKs: FindOptions["where"], context?: Context): Promise { + delete( + idsOrPKs: FindOptions["where"], + context?: Context + ): Promise { throw new Error("Method not implemented.") } @@ -285,7 +288,7 @@ export class MikroOrmBaseTreeRepository< throw new Error("Method not implemented.") } - delete(ids: string[], context?: Context): Promise { + delete(ids: string[], context?: Context): Promise { throw new Error("Method not implemented.") } } @@ -301,6 +304,10 @@ export function mikroOrmBaseRepositoryFactory( class MikroOrmAbstractBaseRepository_ extends MikroOrmBaseRepository { entity = mikroOrmEntity + tableName = ( + (mikroOrmEntity as unknown as EntitySchema).meta ?? + (mikroOrmEntity as EntityClass).prototype.__meta + ).collection // @ts-ignore constructor(...args: any[]) { @@ -428,9 +435,29 @@ export function mikroOrmBaseRepositoryFactory( async delete( filters: FindOptions["where"], context?: Context - ): Promise { - const manager = this.getActiveManager(context) - await manager.nativeDelete(this.entity, filters) + ): Promise { + const manager = this.getActiveManager(context) + + const whereSqlInfo = manager + .createQueryBuilder(this.entity.name, this.tableName) + .where(filters) + .getKnexQuery() + .toSQL() + + const where = [ + whereSqlInfo.sql.split("where ")[1], + whereSqlInfo.bindings, + ] as [string, any[]] + + return await (manager.getTransactionContext() ?? manager.getKnex()) + .queryBuilder() + .from(this.tableName) + .delete() + .where(manager.getKnex().raw(...where)) + .returning("id") + .then((rows: { id: string }[]) => { + return rows.map((row: { id: string }) => row.id) + }) } async find( diff --git a/packages/core/utils/src/modules-sdk/__tests__/medusa-internal-service.ts b/packages/core/utils/src/modules-sdk/__tests__/medusa-internal-service.ts index 83e4137c4a..eb7eeef4e0 100644 --- a/packages/core/utils/src/modules-sdk/__tests__/medusa-internal-service.ts +++ b/packages/core/utils/src/modules-sdk/__tests__/medusa-internal-service.ts @@ -267,12 +267,7 @@ describe("Internal Module Service Factory", () => { }) it("should delete entities successfully", async () => { - const entitiesToDelete = [{ id: "1", name: "Item" }] - containerMock[modelRepositoryName].find.mockResolvedValueOnce( - entitiesToDelete - ) - - await instance.delete({ selector: {} }) + await instance.delete({ selector: { id: "1" } }) expect(containerMock[modelRepositoryName].delete).toHaveBeenCalledWith( { $or: [ diff --git a/packages/core/utils/src/modules-sdk/__tests__/medusa-service.spec.ts b/packages/core/utils/src/modules-sdk/__tests__/medusa-service.spec.ts index 708489b7f9..79deea214a 100644 --- a/packages/core/utils/src/modules-sdk/__tests__/medusa-service.spec.ts +++ b/packages/core/utils/src/modules-sdk/__tests__/medusa-service.spec.ts @@ -29,21 +29,21 @@ describe("Abstract Module Service Factory", () => { mainModelMockService: { retrieve: jest.fn().mockResolvedValue({ id: "1", name: "Item" }), list: jest.fn().mockResolvedValue([{ id: "1", name: "Item" }]), - delete: jest.fn().mockResolvedValue(undefined), + delete: jest.fn().mockResolvedValue([]), softDelete: jest.fn().mockResolvedValue([[], {}]), restore: jest.fn().mockResolvedValue([[], {}]), }, otherModelMock1Service: { retrieve: jest.fn().mockResolvedValue({ id: "1", name: "Item" }), list: jest.fn().mockResolvedValue([{ id: "1", name: "Item" }]), - delete: jest.fn().mockResolvedValue(undefined), + delete: jest.fn().mockResolvedValue([]), softDelete: jest.fn().mockResolvedValue([[], {}]), restore: jest.fn().mockResolvedValue([[], {}]), }, otherModelMock2Service: { retrieve: jest.fn().mockResolvedValue({ id: "1", name: "Item" }), list: jest.fn().mockResolvedValue([{ id: "1", name: "Item" }]), - delete: jest.fn().mockResolvedValue(undefined), + delete: jest.fn().mockResolvedValue([]), softDelete: jest.fn().mockResolvedValue([[], {}]), restore: jest.fn().mockResolvedValue([[], {}]), }, diff --git a/packages/core/utils/src/modules-sdk/medusa-internal-service.ts b/packages/core/utils/src/modules-sdk/medusa-internal-service.ts index 403626a81d..5809ddce1e 100644 --- a/packages/core/utils/src/modules-sdk/medusa-internal-service.ts +++ b/packages/core/utils/src/modules-sdk/medusa-internal-service.ts @@ -367,16 +367,16 @@ export function MedusaInternalService< ) } - delete(idOrSelector: string, sharedContext?: Context): Promise - delete(idOrSelector: string[], sharedContext?: Context): Promise - delete(idOrSelector: object, sharedContext?: Context): Promise - delete(idOrSelector: object[], sharedContext?: Context): Promise + delete(idOrSelector: string, sharedContext?: Context): Promise + delete(idOrSelector: string[], sharedContext?: Context): Promise + delete(idOrSelector: object, sharedContext?: Context): Promise + delete(idOrSelector: object[], sharedContext?: Context): Promise delete( idOrSelector: { selector: FilterQuery | BaseFilterable> }, sharedContext?: Context - ): Promise + ): Promise @InjectTransactionManager(propertyRepositoryName) async delete( @@ -389,12 +389,12 @@ export function MedusaInternalService< selector: FilterQuery | BaseFilterable> }, @MedusaContext() sharedContext: Context = {} - ): Promise { + ): Promise { if ( !isDefined(idOrSelector) || (Array.isArray(idOrSelector) && !idOrSelector.length) ) { - return + return [] } const primaryKeys = AbstractService_.retrievePrimaryKeys(model) @@ -420,21 +420,7 @@ export function MedusaInternalService< } if (isObject(idOrSelector) && "selector" in idOrSelector) { - const entitiesToDelete = await this.list( - idOrSelector.selector as FilterQuery, - { - select: primaryKeys, - }, - sharedContext - ) - - for (const entity of entitiesToDelete) { - const criteria = {} - primaryKeys.forEach((key) => { - criteria[key] = entity[key] - }) - deleteCriteria.$or.push(criteria) - } + deleteCriteria.$or.push(idOrSelector.selector) } else { const primaryKeysValues = Array.isArray(idOrSelector) ? idOrSelector @@ -451,34 +437,18 @@ export function MedusaInternalService< criteria[primaryKeys[0]] = primaryKeyValue } - // TODO: Revisit - /*primaryKeys.forEach((key) => { - /!*if ( - isObject(primaryKeyValue) && - !isDefined(primaryKeyValue[key]) && - // primaryKeys.length > 1 - ) { - throw new MedusaError( - MedusaError.Types.INVALID_DATA, - `Composite key must contain all primary key fields: ${primaryKeys.join( - ", " - )}. Found: ${Object.keys(primaryKeyValue)}` - ) - }*!/ - - criteria[key] = isObject(primaryKeyValue) - ? primaryKeyValue[key] - : primaryKeyValue - })*/ return criteria }) } if (!deleteCriteria.$or.length) { - return + return [] } - await this[propertyRepositoryName].delete(deleteCriteria, sharedContext) + return await this[propertyRepositoryName].delete( + deleteCriteria, + sharedContext + ) } @InjectTransactionManager(propertyRepositoryName) diff --git a/packages/core/utils/src/modules-sdk/medusa-service.ts b/packages/core/utils/src/modules-sdk/medusa-service.ts index 16cb23f586..ac3d7aa2d3 100644 --- a/packages/core/utils/src/modules-sdk/medusa-service.ts +++ b/packages/core/utils/src/modules-sdk/medusa-service.ts @@ -277,18 +277,16 @@ export function MedusaService< ? primaryKeyValues : [primaryKeyValues] - await this.__container__[serviceRegistrationName].delete( + const ids = await this.__container__[serviceRegistrationName].delete( primaryKeyValues_, sharedContext ) - primaryKeyValues_.map((primaryKeyValue) => + ids.map((id) => klassPrototype.aggregatedEvents.bind(this)({ action: CommonEvents.DELETED, object: camelToSnakeCase(modelName).toLowerCase(), - data: isString(primaryKeyValue) - ? { id: primaryKeyValue } - : primaryKeyValue, + data: isString(id) ? { id: id } : id, context: sharedContext, }) ) diff --git a/packages/modules/inventory/src/services/inventory-module.ts b/packages/modules/inventory/src/services/inventory-module.ts index 3d01ac3264..e298d313de 100644 --- a/packages/modules/inventory/src/services/inventory-module.ts +++ b/packages/modules/inventory/src/services/inventory-module.ts @@ -547,7 +547,7 @@ export default class InventoryModuleService return } - return await this.inventoryLevelService_.delete(inventoryLevel.id, context) + await this.inventoryLevelService_.delete(inventoryLevel.id, context) } // @ts-ignore diff --git a/packages/modules/link-modules/src/repositories/link.ts b/packages/modules/link-modules/src/repositories/link.ts index 116bf500e4..5064484755 100644 --- a/packages/modules/link-modules/src/repositories/link.ts +++ b/packages/modules/link-modules/src/repositories/link.ts @@ -17,7 +17,7 @@ export function getLinkRepository(model: EntitySchema) { this.joinerConfig_ = joinerConfig } - async delete(data: any, context: Context = {}): Promise { + async delete(data: any, context: Context = {}): Promise { const filter = {} for (const key in data) { filter[key] = { @@ -25,8 +25,7 @@ export function getLinkRepository(model: EntitySchema) { } } - const manager = this.getActiveManager(context) - await manager.nativeDelete(model, data, {}) + return await super.delete(filter, context) } async create(data: object[], context: Context = {}): Promise { diff --git a/packages/modules/product/integration-tests/__tests__/product-module-service/products.spec.ts b/packages/modules/product/integration-tests/__tests__/product-module-service/products.spec.ts index 241fad2a81..424959012a 100644 --- a/packages/modules/product/integration-tests/__tests__/product-module-service/products.spec.ts +++ b/packages/modules/product/integration-tests/__tests__/product-module-service/products.spec.ts @@ -1312,12 +1312,15 @@ moduleIntegrationTestRunner({ relations: ["images"], }) - const retrievedProductAgain = await service.retrieveProduct(product.id, { - relations: ["images"], - }) + const retrievedProductAgain = await service.retrieveProduct( + product.id, + { + relations: ["images"], + } + ) expect(retrievedProduct.images).toEqual(retrievedProductAgain.images) - + expect(retrievedProduct.images).toEqual( Array.from({ length: 1000 }, (_, i) => expect.objectContaining({ @@ -1332,7 +1335,9 @@ moduleIntegrationTestRunner({ // Explicitly verify sequential order retrievedProduct.images.forEach((img, idx) => { if (idx > 0) { - expect(img.rank).toBeGreaterThan(retrievedProduct.images[idx - 1].rank) + expect(img.rank).toBeGreaterThan( + retrievedProduct.images[idx - 1].rank + ) } }) }) diff --git a/packages/modules/product/package.json b/packages/modules/product/package.json index 090e572144..1e57055094 100644 --- a/packages/modules/product/package.json +++ b/packages/modules/product/package.json @@ -29,7 +29,7 @@ "resolve:aliases": "tsc --showConfig -p tsconfig.json > tsconfig.resolved.json && tsc-alias -p tsconfig.resolved.json && rimraf tsconfig.resolved.json", "build": "rimraf dist && tsc --build && npm run resolve:aliases", "test": "jest --runInBand --bail --forceExit -- src/**/__tests__/**/*.ts", - "test:integration": "jest --forceExit", + "test:integration": "jest --forceExit -- integration-tests/__tests__/**/*.ts", "migration:initial": " MIKRO_ORM_CLI_CONFIG=./mikro-orm.config.dev.ts medusa-mikro-orm migration:create --initial", "migration:create": " MIKRO_ORM_CLI_CONFIG=./mikro-orm.config.dev.ts medusa-mikro-orm migration:create", "migration:up": " MIKRO_ORM_CLI_CONFIG=./mikro-orm.config.dev.ts medusa-mikro-orm migration:up", diff --git a/packages/modules/product/src/repositories/product-category.ts b/packages/modules/product/src/repositories/product-category.ts index f395d806d0..b3ccfae6f0 100644 --- a/packages/modules/product/src/repositories/product-category.ts +++ b/packages/modules/product/src/repositories/product-category.ts @@ -272,10 +272,11 @@ export class ProductCategoryRepository extends DALUtils.MikroOrmBaseTreeReposito return [this.sortCategoriesByRank(categoriesTree), count] } - async delete(ids: string[], context: Context = {}): Promise { + async delete(ids: string[], context: Context = {}): Promise { const manager = super.getActiveManager(context) await this.baseDelete(ids, context) await manager.nativeDelete(ProductCategory.name, { id: ids }, {}) + return ids } async softDelete( diff --git a/packages/modules/product/src/services/product-category.ts b/packages/modules/product/src/services/product-category.ts index df27f4a4bb..3a2eef0109 100644 --- a/packages/modules/product/src/services/product-category.ts +++ b/packages/modules/product/src/services/product-category.ts @@ -163,8 +163,8 @@ export default class ProductCategoryService { async delete( ids: string[], @MedusaContext() sharedContext: Context = {} - ): Promise { - await this.productCategoryRepository_.delete(ids, sharedContext) + ): Promise { + return await this.productCategoryRepository_.delete(ids, sharedContext) } @InjectTransactionManager("productCategoryRepository_")