From 5e4decbc1c4cc25cb1adb1f63b2f8ea8669d352e Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Fri, 16 Dec 2022 20:13:53 +0100 Subject: [PATCH] fix(medusa): Batch job not saving the errors properly (#2812) --- .changeset/red-squids-compare.md | 5 ++ .../__tests__/batch-jobs/product/import.js | 29 ++++++----- .../src/interfaces/batch-job-strategy.ts | 3 +- packages/medusa/src/models/batch-job.ts | 17 ++----- packages/medusa/src/services/batch-job.ts | 3 +- packages/medusa/src/subscribers/batch-job.ts | 51 +++++++++---------- 6 files changed, 54 insertions(+), 54 deletions(-) create mode 100644 .changeset/red-squids-compare.md diff --git a/.changeset/red-squids-compare.md b/.changeset/red-squids-compare.md new file mode 100644 index 0000000000..8a3d54673d --- /dev/null +++ b/.changeset/red-squids-compare.md @@ -0,0 +1,5 @@ +--- +"@medusajs/medusa": patch +--- + +fix: Batch job not saving the errors properly diff --git a/integration-tests/api/__tests__/batch-jobs/product/import.js b/integration-tests/api/__tests__/batch-jobs/product/import.js index b82a7d656b..2acb346675 100644 --- a/integration-tests/api/__tests__/batch-jobs/product/import.js +++ b/integration-tests/api/__tests__/batch-jobs/product/import.js @@ -9,7 +9,9 @@ const adminSeeder = require("../../../helpers/admin-seeder") const batchJobSeeder = require("../../../helpers/batch-job-seeder") const userSeeder = require("../../../helpers/user-seeder") const { simpleProductFactory } = require("../../../factories") -const { simpleProductCollectionFactory } = require("../../../factories/simple-product-collection-factory"); +const { + simpleProductCollectionFactory, +} = require("../../../factories/simple-product-collection-factory") const adminReqConfig = { headers: { @@ -50,8 +52,8 @@ describe("Product import batch job", () => { let medusaProcess let dbConnection - let collectionHandle1 = "test-collection1" - let collectionHandle2 = "test-collection2" + const collectionHandle1 = "test-collection1" + const collectionHandle2 = "test-collection2" beforeAll(async () => { const cwd = path.resolve(path.join(__dirname, "..", "..", "..")) @@ -79,11 +81,14 @@ describe("Product import batch job", () => { await batchJobSeeder(dbConnection) await adminSeeder(dbConnection) await userSeeder(dbConnection) - await simpleProductCollectionFactory(dbConnection, [{ - handle: collectionHandle1 - }, { - handle: collectionHandle2 - }]) + await simpleProductCollectionFactory(dbConnection, [ + { + handle: collectionHandle1, + }, + { + handle: collectionHandle2, + }, + ]) }) afterEach(async () => { @@ -226,7 +231,7 @@ describe("Product import batch job", () => { ], collection: expect.objectContaining({ handle: collectionHandle1, - }) + }), }), expect.objectContaining({ title: "Test product", @@ -288,7 +293,7 @@ describe("Product import batch job", () => { tags: [], collection: expect.objectContaining({ handle: collectionHandle1, - }) + }), }), // UPDATED PRODUCT expect.objectContaining({ @@ -384,8 +389,8 @@ describe("Product import batch job", () => { }), ], collection: expect.objectContaining({ - handle: collectionHandle2 - }) + handle: collectionHandle2, + }), }), ]) ) diff --git a/packages/medusa/src/interfaces/batch-job-strategy.ts b/packages/medusa/src/interfaces/batch-job-strategy.ts index 80e0598b9e..924bcbcec2 100644 --- a/packages/medusa/src/interfaces/batch-job-strategy.ts +++ b/packages/medusa/src/interfaces/batch-job-strategy.ts @@ -69,6 +69,7 @@ export abstract class AbstractBatchJobStrategy err: unknown, result: T ): Promise { + // TODO just throw to be handled by the subscriber return await this.atomicPhase_(async (transactionManager) => { const batchJob = (await this.batchJobService_ .withTransaction(transactionManager) @@ -95,7 +96,7 @@ export abstract class AbstractBatchJobStrategy }, result: { ...result, - errors: [...existingErrors, resultError], + errors: [...existingErrors, resultError.message], }, }) } else { diff --git a/packages/medusa/src/models/batch-job.ts b/packages/medusa/src/models/batch-job.ts index 65110c1973..1c23a76fb0 100644 --- a/packages/medusa/src/models/batch-job.ts +++ b/packages/medusa/src/models/batch-job.ts @@ -1,16 +1,5 @@ -import { - AfterLoad, - BeforeInsert, - Column, - Entity, - JoinColumn, - ManyToOne, -} from "typeorm" -import { - BatchJobResultError, - BatchJobResultStatDescriptor, - BatchJobStatus, -} from "../types/batch-job" +import { AfterLoad, BeforeInsert, Column, Entity, JoinColumn, ManyToOne, } from "typeorm" +import { BatchJobResultError, BatchJobResultStatDescriptor, BatchJobStatus, } from "../types/batch-job" import { DbAwareColumn, resolveDbType } from "../utils/db-aware-column" import { SoftDeletableEntity } from "../interfaces/models/soft-deletable-entity" @@ -37,7 +26,7 @@ export class BatchJob extends SoftDeletableEntity { count?: number advancement_count?: number progress?: number - errors?: BatchJobResultError[] + errors?: (BatchJobResultError | string)[] stat_descriptors?: BatchJobResultStatDescriptor[] file_key?: string file_size?: number diff --git a/packages/medusa/src/services/batch-job.ts b/packages/medusa/src/services/batch-job.ts index 8f2b748c31..eb9de5d985 100644 --- a/packages/medusa/src/services/batch-job.ts +++ b/packages/medusa/src/services/batch-job.ts @@ -96,6 +96,7 @@ class BatchJobService extends TransactionBaseService { eventBusService, strategyResolverService, }: InjectedDependencies) { + // eslint-disable-next-line prefer-rest-params super(arguments[0]) this.manager_ = manager @@ -339,7 +340,7 @@ class BatchJobService extends TransactionBaseService { async setFailed( batchJobOrId: string | BatchJob, - error?: BatchJobResultError + error?: BatchJobResultError | string ): Promise { return await this.atomicPhase_(async () => { let batchJob = batchJobOrId as BatchJob diff --git a/packages/medusa/src/subscribers/batch-job.ts b/packages/medusa/src/subscribers/batch-job.ts index 002044c895..90f374474a 100644 --- a/packages/medusa/src/subscribers/batch-job.ts +++ b/packages/medusa/src/subscribers/batch-job.ts @@ -33,45 +33,44 @@ class BatchJobSubscriber { } preProcessBatchJob = async (data): Promise => { - await this.manager_.transaction(async (manager) => { - const batchJobServiceTx = this.batchJobService_.withTransaction(manager) - const batchJob = await batchJobServiceTx.retrieve(data.id) + try { + await this.manager_.transaction(async (manager) => { + const batchJobServiceTx = this.batchJobService_.withTransaction(manager) + const batchJob = await batchJobServiceTx.retrieve(data.id) - const batchJobStrategy = this.strategyResolver_.resolveBatchJobByType( - batchJob.type - ) + const batchJobStrategy = this.strategyResolver_.resolveBatchJobByType( + batchJob.type + ) - try { await batchJobStrategy .withTransaction(manager) .preProcessBatchJob(batchJob.id) await batchJobServiceTx.setPreProcessingDone(batchJob.id) - } catch (e) { - await this.batchJobService_.setFailed(batchJob.id, e.message) - throw e - } - }) + }) + } catch (e) { + await this.batchJobService_.setFailed(data.id, e.message) + throw e + } } processBatchJob = async (data): Promise => { - await this.manager_.transaction(async (manager) => { - const batchJobServiceTx = this.batchJobService_.withTransaction(manager) - const batchJob = await batchJobServiceTx.retrieve(data.id) + try { + await this.manager_.transaction(async (manager) => { + const batchJobServiceTx = this.batchJobService_.withTransaction(manager) + const batchJob = await batchJobServiceTx.retrieve(data.id) - const batchJobStrategy = this.strategyResolver_.resolveBatchJobByType( - batchJob.type - ) + const batchJobStrategy = this.strategyResolver_.resolveBatchJobByType( + batchJob.type + ) - await batchJobServiceTx.setProcessing(batchJob.id) - - try { + await batchJobServiceTx.setProcessing(batchJob.id) await batchJobStrategy.withTransaction(manager).processJob(batchJob.id) await batchJobServiceTx.complete(batchJob.id) - } catch (e) { - await this.batchJobService_.setFailed(batchJob.id, e.message) - throw e - } - }) + }) + } catch (e) { + await this.batchJobService_.setFailed(data.id, e.message) + throw e + } } }