From 82d369bc518f85818cc678699d89b36a8a9cfb9c Mon Sep 17 00:00:00 2001 From: Stevche Radevski Date: Wed, 3 Apr 2024 17:05:59 +0200 Subject: [PATCH] feat: Normalize known DB errors to a MedusaError when possible --- .changeset/cold-jeans-shave.md | 5 + .../api/__tests__/admin/product.js | 9 +- .../integration-tests/__tests__/index.spec.ts | 8 +- .../tax/src/services/tax-module-service.ts | 74 +-------- .../src/dal/mikro-orm/db-error-mapper.ts | 82 ++++++++++ .../__tests__/mikro-orm-repository.spec.ts | 144 ++++++++++++++---- .../src/dal/mikro-orm/mikro-orm-repository.ts | 31 +++- packages/utils/src/dal/utils.ts | 12 +- 8 files changed, 240 insertions(+), 125 deletions(-) create mode 100644 .changeset/cold-jeans-shave.md create mode 100644 packages/utils/src/dal/mikro-orm/db-error-mapper.ts diff --git a/.changeset/cold-jeans-shave.md b/.changeset/cold-jeans-shave.md new file mode 100644 index 0000000000000..0c3b1fe52bb63 --- /dev/null +++ b/.changeset/cold-jeans-shave.md @@ -0,0 +1,5 @@ +--- +"@medusajs/utils": minor +--- + +Return normalized DB errors from the mikro ORM repository diff --git a/integration-tests/api/__tests__/admin/product.js b/integration-tests/api/__tests__/admin/product.js index 7f068a03165fb..e62d4e1259039 100644 --- a/integration-tests/api/__tests__/admin/product.js +++ b/integration-tests/api/__tests__/admin/product.js @@ -2630,8 +2630,7 @@ medusaIntegrationTestRunner({ ) }) - // TODO: This needs to be fixed - it.skip("successfully creates product with soft-deleted product handle and deletes it again", async () => { + it("successfully creates product with soft-deleted product handle and deletes it again", async () => { // First we soft-delete the product const response = await api .delete(`/admin/products/${baseProduct.id}`, adminHeaders) @@ -2708,8 +2707,7 @@ medusaIntegrationTestRunner({ expect(response.data.id).toEqual(baseCollection.id) }) - // TODO: This needs to be fixed, it returns 422 now. - it.skip("successfully creates soft-deleted product collection", async () => { + it("successfully creates soft-deleted product collection", async () => { const response = await api.delete( `/admin/collections/${baseCollection.id}`, adminHeaders @@ -2749,8 +2747,7 @@ medusaIntegrationTestRunner({ } }) - // TODO: This needs to be fixed - it.skip("successfully creates soft-deleted product variant", async () => { + it("successfully creates soft-deleted product variant", async () => { const variant = baseProduct.variants[0] const response = await api.delete( `/admin/products/${baseProduct.id}/variants/${variant.id}`, diff --git a/packages/tax/integration-tests/__tests__/index.spec.ts b/packages/tax/integration-tests/__tests__/index.spec.ts index 197296d4a0282..40e6cc23477b6 100644 --- a/packages/tax/integration-tests/__tests__/index.spec.ts +++ b/packages/tax/integration-tests/__tests__/index.spec.ts @@ -711,7 +711,7 @@ moduleIntegrationTestRunner({ ], }) ).rejects.toThrowError( - /You are trying to create a Tax Rate Rule for a reference that already exists. Tax Rate id: .*?, reference id: product_id_1./ + /Tax rate rule with tax_rate_id: .*?, reference_id: product_id_1 already exists./ ) const rate = await service.create({ @@ -729,7 +729,7 @@ moduleIntegrationTestRunner({ reference_id: "product_id_1", }) ).rejects.toThrowError( - /You are trying to create a Tax Rate Rule for a reference that already exists. Tax Rate id: .*?, reference id: product_id_1./ + /Tax rate rule with tax_rate_id: .*?, reference_id: product_id_1 already exists./ ) }) @@ -764,7 +764,7 @@ moduleIntegrationTestRunner({ province_code: "QC", }) ).rejects.toThrowError( - "You are trying to create a Tax Region for (country_code: ca, province_code: qc) but one already exists." + "Tax region with country_code: ca, province_code: qc already exists." ) }) @@ -797,7 +797,7 @@ moduleIntegrationTestRunner({ is_default: true, }) ).rejects.toThrowError( - /You are trying to create a default tax rate for region: .*? which already has a default tax rate. Unset the current default rate and try again./ + /Tax rate with tax_region_id: .*? already exists./ ) }) diff --git a/packages/tax/src/services/tax-module-service.ts b/packages/tax/src/services/tax-module-service.ts index aad7eb11c6d45..24b31f487e0ca 100644 --- a/packages/tax/src/services/tax-module-service.ts +++ b/packages/tax/src/services/tax-module-service.ts @@ -21,9 +21,6 @@ import { } from "@medusajs/utils" import { TaxProvider, TaxRate, TaxRateRule, TaxRegion } from "@models" import { entityNameToLinkableKeysMap, joinerConfig } from "../joiner-config" -import { singleDefaultRegionIndexName } from "../models/tax-rate" -import { uniqueRateReferenceIndexName } from "../models/tax-rate-rule" -import { countryCodeProvinceIndexName } from "../models/tax-region" type InjectedDependencies = { baseRepository: DAL.RepositoryService @@ -106,11 +103,7 @@ export default class TaxModuleService< @MedusaContext() sharedContext: Context = {} ): Promise { const input = Array.isArray(data) ? data : [data] - const rates = await this.create_(input, sharedContext).catch((err) => { - this.handleCreateError(err) - this.handleCreateRulesError(err) - throw err - }) + const rates = await this.create_(input, sharedContext) return Array.isArray(data) ? rates : rates[0] } @@ -182,13 +175,7 @@ export default class TaxModuleService< data: TaxTypes.UpdateTaxRateDTO, @MedusaContext() sharedContext: Context = {} ): Promise { - const rates = await this.update_(selector, data, sharedContext).catch( - (err) => { - this.handleCreateError(err) - this.handleCreateRulesError(err) - throw err - } - ) + const rates = await this.update_(selector, data, sharedContext) const serialized = await this.baseRepository_.serialize< TaxTypes.TaxRateDTO[] >(rates, { populate: true }) @@ -322,12 +309,7 @@ export default class TaxModuleService< @MedusaContext() sharedContext: Context = {} ) { const input = Array.isArray(data) ? data : [data] - const result = await this.createTaxRegions_(input, sharedContext).catch( - (err) => { - this.handleCreateRegionsError(err) - throw err - } - ) + const result = await this.createTaxRegions_(input, sharedContext) return Array.isArray(data) ? result : result[0] } @@ -382,12 +364,7 @@ export default class TaxModuleService< @MedusaContext() sharedContext: Context = {} ) { const input = Array.isArray(data) ? data : [data] - const result = await this.createTaxRateRules_(input, sharedContext).catch( - (err) => { - this.handleCreateRulesError(err) - throw err - } - ) + const result = await this.createTaxRateRules_(input, sharedContext) return Array.isArray(data) ? result : result[0] } @@ -748,49 +725,6 @@ export default class TaxModuleService< return code.toLowerCase() } - private handleCreateRegionsError(err: any) { - if (err.constraint === countryCodeProvinceIndexName) { - const [countryCode, provinceCode] = err.detail - .split("=")[1] - .match(/\(([^)]+)\)/)[1] - .split(",") - - throw new MedusaError( - MedusaError.Types.INVALID_DATA, - `You are trying to create a Tax Region for (country_code: ${countryCode.trim()}, province_code: ${provinceCode.trim()}) but one already exists.` - ) - } - } - - private handleCreateError(err: any) { - if (err.constraint === singleDefaultRegionIndexName) { - // err.detail = Key (tax_region_id)=(txreg_01HQX5E8GEH36ZHJWFYDAFY67P) already exists. - const regionId = err.detail.split("=")[1].match(/\(([^)]+)\)/)[1] - - throw new MedusaError( - MedusaError.Types.INVALID_DATA, - `You are trying to create a default tax rate for region: ${regionId} which already has a default tax rate. Unset the current default rate and try again.` - ) - } - } - - private handleCreateRulesError(err: any) { - if (err.constraint === uniqueRateReferenceIndexName) { - // err.detail == "Key (tax_rate_id, reference_id)=(txr_01HQWRXTC0JK0F02D977WRR45T, product_id_1) already exists." - // We want to extract the ids from the detail string - // i.e. txr_01HQWRXTC0JK0F02D977WRR45T and product_id_1 - const [taxRateId, referenceId] = err.detail - .split("=")[1] - .match(/\(([^)]+)\)/)[1] - .split(",") - - throw new MedusaError( - MedusaError.Types.INVALID_DATA, - `You are trying to create a Tax Rate Rule for a reference that already exists. Tax Rate id: ${taxRateId.trim()}, reference id: ${referenceId.trim()}.` - ) - } - } - // @InjectTransactionManager("baseRepository_") // async createProvidersOnLoad(@MedusaContext() sharedContext: Context = {}) { // const providersToLoad = this.container_["tax_providers"] as ITaxProvider[] diff --git a/packages/utils/src/dal/mikro-orm/db-error-mapper.ts b/packages/utils/src/dal/mikro-orm/db-error-mapper.ts new file mode 100644 index 0000000000000..e81bfdc39a5e1 --- /dev/null +++ b/packages/utils/src/dal/mikro-orm/db-error-mapper.ts @@ -0,0 +1,82 @@ +import { + ForeignKeyConstraintViolationException, + InvalidFieldNameException, + NotFoundError, + NotNullConstraintViolationException, + UniqueConstraintViolationException, +} from "@mikro-orm/core" +import { MedusaError, upperCaseFirst } from "../../common" + +export const dbErrorMapper = (err: Error) => { + if (err instanceof NotFoundError) { + throw new MedusaError(MedusaError.Types.NOT_FOUND, err.message) + } + + if (err instanceof UniqueConstraintViolationException) { + const info = getConstraintInfo(err) + if (!info) { + throw err + } + + throw new MedusaError( + MedusaError.Types.INVALID_DATA, + `${upperCaseFirst(info.table)} with ${info.keys + .map((key, i) => `${key}: ${info.values[i]}`) + .join(", ")} already exists.` + ) + } + + if (err instanceof NotNullConstraintViolationException) { + throw new MedusaError( + MedusaError.Types.INVALID_DATA, + `Cannot set field '${(err as any).column}' of ${upperCaseFirst( + (err as any).table + )} to null` + ) + } + + if (err instanceof InvalidFieldNameException) { + const userFriendlyMessage = err.message.match(/(column.*)/)?.[0] + throw new MedusaError( + MedusaError.Types.INVALID_DATA, + userFriendlyMessage ?? err.message + ) + } + + if (err instanceof ForeignKeyConstraintViolationException) { + const info = getConstraintInfo(err) + throw new MedusaError( + MedusaError.Types.INVALID_DATA, + `You tried to set relationship ${info?.keys.map( + (key, i) => `${key}: ${info.values[i]}` + )}, but such entity does not exist` + ) + } + + throw err +} + +const getConstraintInfo = (err: any) => { + const detail = err.detail as string + if (!detail) { + return null + } + + const [keys, values] = detail.match(/\([^\(.]*\)/g) || [] + + if (!keys || !values) { + return null + } + + return { + table: err.table.split("_").join(" "), + keys: keys + .substring(1, keys.length - 1) + .split(",") + .map((k) => k.trim()), + values: values + .substring(1, values.length - 1) + .split(",") + .map((v) => v.trim()), + } +} diff --git a/packages/utils/src/dal/mikro-orm/integration-tests/__tests__/mikro-orm-repository.spec.ts b/packages/utils/src/dal/mikro-orm/integration-tests/__tests__/mikro-orm-repository.spec.ts index b6462a6a6f372..b53595f59fc68 100644 --- a/packages/utils/src/dal/mikro-orm/integration-tests/__tests__/mikro-orm-repository.spec.ts +++ b/packages/utils/src/dal/mikro-orm/integration-tests/__tests__/mikro-orm-repository.spec.ts @@ -10,6 +10,7 @@ import { OnInit, PrimaryKey, Property, + Unique, wrap, } from "@mikro-orm/core" import { mikroOrmBaseRepositoryFactory } from "../../mikro-orm-repository" @@ -101,6 +102,7 @@ class Entity3 { @PrimaryKey() id: string + @Unique() @Property() title: string @@ -124,44 +126,44 @@ const Entity2Repository = mikroOrmBaseRepositoryFactory(Entity2) const Entity3Repository = mikroOrmBaseRepositoryFactory(Entity3) describe("mikroOrmRepository", () => { - describe("upsert with replace", () => { - let orm!: MikroORM - let manager!: EntityManager - const manager1 = () => { - return new Entity1Repository({ manager: manager.fork() }) - } - const manager2 = () => { - return new Entity2Repository({ manager: manager.fork() }) - } - const manager3 = () => { - return new Entity3Repository({ manager: manager.fork() }) - } + let orm!: MikroORM + let manager!: EntityManager + const manager1 = () => { + return new Entity1Repository({ manager: manager.fork() }) + } + const manager2 = () => { + return new Entity2Repository({ manager: manager.fork() }) + } + const manager3 = () => { + return new Entity3Repository({ manager: manager.fork() }) + } - beforeEach(async () => { - await dropDatabase( - { databaseName: DB_NAME, errorIfNonExist: false }, - pgGodCredentials - ) + beforeEach(async () => { + await dropDatabase( + { databaseName: DB_NAME, errorIfNonExist: false }, + pgGodCredentials + ) - orm = await MikroORM.init({ - entities: [Entity1, Entity2], - clientUrl: getDatabaseURL(), - type: "postgresql", - }) + orm = await MikroORM.init({ + entities: [Entity1, Entity2], + clientUrl: getDatabaseURL(), + type: "postgresql", + }) - const generator = orm.getSchemaGenerator() - await generator.ensureDatabase() - await generator.createSchema() + const generator = orm.getSchemaGenerator() + await generator.ensureDatabase() + await generator.createSchema() - manager = orm.em.fork() - }) + manager = orm.em.fork() + }) - afterEach(async () => { - const generator = orm.getSchemaGenerator() - await generator.dropSchema() - await orm.close(true) - }) + afterEach(async () => { + const generator = orm.getSchemaGenerator() + await generator.dropSchema() + await orm.close(true) + }) + describe("upsert with replace", () => { it("should successfully create a flat entity", async () => { const entity1 = { id: "1", title: "en1" } @@ -585,7 +587,7 @@ describe("mikroOrmRepository", () => { ) }) - it("should successfully update, create, and delete subentities an entity with a many-to-many relation", async () => { + it("should successfully create subentities and delete pivot relationships on a many-to-many relation", async () => { const entity1 = { id: "1", title: "en1", @@ -598,8 +600,11 @@ describe("mikroOrmRepository", () => { let resp = await manager1().upsertWithReplace([entity1], { relations: ["entity3"], }) + entity1.title = "newen1" entity1.entity3 = [{ id: "4", title: "newen3-1" }, { title: "en3-4" }] + + // We don't do many-to-many updates, so id: 4 entity should remain unchanged resp = await manager1().upsertWithReplace([entity1], { relations: ["entity3"], }) @@ -619,6 +624,7 @@ describe("mikroOrmRepository", () => { expect(listedEntities[0].entity3.getItems()).toEqual( expect.arrayContaining([ expect.objectContaining({ + // title: "en3-1", title: "newen3-1", }), expect.objectContaining({ @@ -673,8 +679,11 @@ describe("mikroOrmRepository", () => { const mainEntity = await manager1().upsertWithReplace([entity1], { relations: ["entity3"], }) + entity1.title = "newen1" entity1.entity3 = [{ id: "4", title: "newen3-1" }, { title: "en3-4" }] + + // We don't do many-to-many updates, so id: 4 entity should remain unchanged await manager1().upsertWithReplace([entity1], { relations: ["entity3"], }) @@ -703,6 +712,7 @@ describe("mikroOrmRepository", () => { expect(listedEntities[0].entity3.getItems()).toEqual( expect.arrayContaining([ expect.objectContaining({ + // title: "en3-1", title: "newen3-1", }), expect.objectContaining({ @@ -774,5 +784,73 @@ describe("mikroOrmRepository", () => { ]) ) }) + + // it("should correctly handle many-to-many upserts with a uniqueness constriant on a non-primary key", async () => { + // const entity1 = { + // id: "1", + // title: "en1", + // entity3: [{ title: "en3-1" }, { title: "en3-2" }] as any, + // } + + // await manager1().upsertWithReplace([entity1], { + // relations: ["entity3"], + // }) + + // await manager1().upsertWithReplace([{ ...entity1, id: "2" }], { + // relations: ["entity3"], + // }) + + // const listedEntities = await manager1().find({ + // where: {}, + // options: { populate: ["entity3"] }, + // }) + + // expect(listedEntities).toHaveLength(2) + // expect(listedEntities[0].entity3.getItems()).toEqual( + // listedEntities[1].entity3.getItems() + // ) + // }) + }) + + describe.only("error mapping", () => { + it("should map UniqueConstraintViolationException to MedusaError on upsertWithReplace", async () => { + const entity3 = { title: "en3" } + await manager3().upsertWithReplace([entity3]) + const err = await manager3() + .upsertWithReplace([entity3]) + .catch((e) => e.message) + + expect(err).toEqual("Entity3 with title: en3 already exists.") + }) + + it("should map NotNullConstraintViolationException MedusaError on upsertWithReplace", async () => { + const entity3 = { title: null } + const err = await manager3() + .upsertWithReplace([entity3]) + .catch((e) => e.message) + + expect(err).toEqual("Cannot set field 'title' of Entity3 to null") + }) + + it("should map InvalidFieldNameException MedusaError on upsertWithReplace", async () => { + const entity3 = { othertitle: "en3" } + const err = await manager3() + .upsertWithReplace([entity3]) + .catch((e) => e.message) + + expect(err).toEqual( + 'column "othertitle" of relation "entity3" does not exist' + ) + }) + it("should map ForeignKeyConstraintViolationException MedusaError on upsertWithReplace", async () => { + const entity2 = { title: "en2", entity1: { id: "1" } } + const err = await manager2() + .upsertWithReplace([entity2]) + .catch((e) => e.message) + + expect(err).toEqual( + "You tried to set relationship entity1_id: 1, but such entity does not exist" + ) + }) }) }) diff --git a/packages/utils/src/dal/mikro-orm/mikro-orm-repository.ts b/packages/utils/src/dal/mikro-orm/mikro-orm-repository.ts index 2aae41be9cfb2..a46d6ed982d2c 100644 --- a/packages/utils/src/dal/mikro-orm/mikro-orm-repository.ts +++ b/packages/utils/src/dal/mikro-orm/mikro-orm-repository.ts @@ -27,7 +27,6 @@ import { SqlEntityManager } from "@mikro-orm/postgresql" import { MedusaError, arrayDifference, - deepCopy, isString, promiseAll, } from "../../common" @@ -38,6 +37,7 @@ import { } from "../utils" import { mikroOrmUpdateDeletedAtRecursively } from "./utils" import { mikroOrmSerializer } from "./mikro-orm-serializer" +import { dbErrorMapper } from "./db-error-mapper" export class MikroOrmBase { readonly manager_: any @@ -67,8 +67,13 @@ export class MikroOrmBase { transaction?: TManager } = {} ): Promise { - // @ts-ignore - return await transactionWrapper.bind(this)(task, options) + const freshManager = this.getFreshManager + ? this.getFreshManager() + : this.manager_ + + return await transactionWrapper(freshManager, task, options).catch( + dbErrorMapper + ) } async serialize( @@ -262,6 +267,23 @@ export function mikroOrmBaseRepositoryFactory( constructor(...args: any[]) { // @ts-ignore super(...arguments) + + return new Proxy(this, { + get: (target, prop) => { + if (typeof target[prop] === "function") { + return (...args) => { + const res = target[prop].bind(target)(...args) + if (res instanceof Promise) { + return res.catch(dbErrorMapper) + } + + return res + } + } + + return target[prop] + }, + }) } static buildUniqueCompositeKeyValue(keys: string[], data: object) { @@ -474,7 +496,8 @@ export function mikroOrmBaseRepositoryFactory( ) if (nonexistentRelations.length) { - throw new Error( + throw new MedusaError( + MedusaError.Types.INVALID_DATA, `Nonexistent relations were passed during upsert: ${nonexistentRelations}` ) } diff --git a/packages/utils/src/dal/utils.ts b/packages/utils/src/dal/utils.ts index de3e0aa8ad239..dd4f8c8a604d9 100644 --- a/packages/utils/src/dal/utils.ts +++ b/packages/utils/src/dal/utils.ts @@ -1,8 +1,8 @@ import { isObject } from "../common" export async function transactionWrapper( - this: any, - task: (transactionManager: unknown) => Promise, + manager: any, + task: (transactionManager: any) => Promise, { transaction, isolationLevel, @@ -28,12 +28,8 @@ export async function transactionWrapper( Object.assign(options, { isolationLevel }) } - const freshManager = this.getFreshManager - ? this.getFreshManager() - : this.manager_ - const transactionMethod = - freshManager.transaction ?? freshManager.transactional - return await transactionMethod.bind(freshManager)(task, options) + const transactionMethod = manager.transaction ?? manager.transactional + return await transactionMethod.bind(manager)(task, options) } /**