From a77780671aadc311c0e8a541104cbff1ea769ac7 Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Fri, 18 Nov 2022 12:15:53 +0100 Subject: [PATCH] fix(medusa): Transaction lock issues on create/update cart items (#2612) * fix(medusa): Transaction lock issues on create/update cart items * fix add missing trans * cleanup * cleanup * Create perfect-bears-invent.md * cleanup * revert draft order to no take it in that pr * cleanup handler * cleanup steps * fix reference issue * cleanup + fix tests and mock * cleanup type * rename file * cleanup * fix missing transaction * wip * Address pr feedback * cleanup and fix unit tests * fix handler Co-authored-by: Oliver Windall Juhl <59018053+olivermrbl@users.noreply.github.com> --- .changeset/perfect-bears-invent.md | 5 + .../store/carts/__tests__/create-line-item.js | 2 +- .../index.ts} | 92 +++++++++++------- .../create-line-item/utils/handler-steps.ts | 67 +++++++++++++ .../src/services/__mocks__/idempotency-key.js | 28 +++--- .../medusa/src/services/__tests__/cart.js | 34 ++++++- packages/medusa/src/services/cart.ts | 97 +++++++++---------- .../medusa/src/services/idempotency-key.ts | 16 ++- packages/medusa/src/services/line-item.ts | 4 +- packages/medusa/src/types/idempotency-key.ts | 6 ++ .../medusa/src/utils/exception-formatter.ts | 8 ++ .../medusa/src/utils/idempotency/index.ts | 2 + .../initialize-idempotency-request.ts | 26 +++++ .../utils/idempotency/run-idempotency-step.ts | 40 ++++++++ 14 files changed, 310 insertions(+), 117 deletions(-) create mode 100644 .changeset/perfect-bears-invent.md rename packages/medusa/src/api/routes/store/carts/{create-line-item.ts => create-line-item/index.ts} (58%) create mode 100644 packages/medusa/src/api/routes/store/carts/create-line-item/utils/handler-steps.ts create mode 100644 packages/medusa/src/utils/idempotency/index.ts create mode 100644 packages/medusa/src/utils/idempotency/initialize-idempotency-request.ts create mode 100644 packages/medusa/src/utils/idempotency/run-idempotency-step.ts diff --git a/.changeset/perfect-bears-invent.md b/.changeset/perfect-bears-invent.md new file mode 100644 index 0000000000..4f31668407 --- /dev/null +++ b/.changeset/perfect-bears-invent.md @@ -0,0 +1,5 @@ +--- +"@medusajs/medusa": patch +--- + +fix(medusa): Transaction lock issues on create/update cart items diff --git a/packages/medusa/src/api/routes/store/carts/__tests__/create-line-item.js b/packages/medusa/src/api/routes/store/carts/__tests__/create-line-item.js index b06931d146..fefd5d86c3 100644 --- a/packages/medusa/src/api/routes/store/carts/__tests__/create-line-item.js +++ b/packages/medusa/src/api/routes/store/carts/__tests__/create-line-item.js @@ -25,7 +25,7 @@ describe("POST /store/carts/:id", () => { }) it("calls CartService retrieve", () => { - expect(CartServiceMock.retrieve).toHaveBeenCalledTimes(2) + expect(CartServiceMock.retrieve).toHaveBeenCalledTimes(1) expect(CartServiceMock.retrieveWithTotals).toHaveBeenCalledTimes(1) }) diff --git a/packages/medusa/src/api/routes/store/carts/create-line-item.ts b/packages/medusa/src/api/routes/store/carts/create-line-item/index.ts similarity index 58% rename from packages/medusa/src/api/routes/store/carts/create-line-item.ts rename to packages/medusa/src/api/routes/store/carts/create-line-item/index.ts index 4d9d1729ad..b743ba8122 100644 --- a/packages/medusa/src/api/routes/store/carts/create-line-item.ts +++ b/packages/medusa/src/api/routes/store/carts/create-line-item/index.ts @@ -1,9 +1,16 @@ import { IsInt, IsOptional, IsString } from "class-validator" import { EntityManager } from "typeorm" -import { defaultStoreCartFields, defaultStoreCartRelations } from "." -import { CartService, LineItemService } from "../../../../services" -import { validator } from "../../../../utils/validator" -import { FlagRouter } from "../../../../utils/flag-router" +import { validator } from "../../../../../utils/validator" +import { + CreateLineItemSteps, + handleAddOrUpdateLineItem, +} from "./utils/handler-steps" +import { IdempotencyKey } from "../../../../../models" +import { + initializeIdempotencyRequest, + runIdempotencyStep, + RunIdempotencyStepOptions, +} from "../../../../../utils/idempotency" /** * @oas [post] /carts/{id}/line-items @@ -63,46 +70,65 @@ import { FlagRouter } from "../../../../utils/flag-router" export default async (req, res) => { const { id } = req.params - const customerId = req.user?.customer_id + const customerId: string | undefined = req.user?.customer_id const validated = await validator(StorePostCartsCartLineItemsReq, req.body) - const lineItemService: LineItemService = req.scope.resolve("lineItemService") - const cartService: CartService = req.scope.resolve("cartService") - const manager: EntityManager = req.scope.resolve("manager") - const featureFlagRouter: FlagRouter = req.scope.resolve("featureFlagRouter") - await manager.transaction(async (m) => { - const txCartService = cartService.withTransaction(m) - const cart = await txCartService.retrieve(id) + let idempotencyKey!: IdempotencyKey + try { + idempotencyKey = await initializeIdempotencyRequest(req, res) + } catch { + res.status(409).send("Failed to create idempotency key") + return + } - const line = await lineItemService - .withTransaction(m) - .generate(validated.variant_id, cart.region_id, validated.quantity, { - customer_id: customerId || cart.customer_id, - metadata: validated.metadata, - }) + let inProgress = true + let err: unknown = false - await txCartService.addLineItem(id, line, { - validateSalesChannels: - featureFlagRouter.isFeatureEnabled("sales_channels"), - }) + const stepOptions: RunIdempotencyStepOptions = { + manager, + idempotencyKey, + container: req.scope, + isolationLevel: "SERIALIZABLE", + } - const updated = await txCartService.retrieve(id, { - relations: ["payment_sessions"], - }) + while (inProgress) { + switch (idempotencyKey.recovery_point) { + case CreateLineItemSteps.STARTED: { + await runIdempotencyStep(async ({ manager }) => { + return await handleAddOrUpdateLineItem( + id, + { + customer_id: customerId, + metadata: validated.metadata, + quantity: validated.quantity, + variant_id: validated.variant_id, + }, + { + manager, + container: req.scope, + } + ) + }, stepOptions).catch((e) => { + inProgress = false + err = e + }) + break + } - if (updated.payment_sessions?.length) { - await txCartService.setPaymentSessions(id) + case CreateLineItemSteps.FINISHED: { + inProgress = false + break + } } - }) + } - const data = await cartService.retrieveWithTotals(id, { - select: defaultStoreCartFields, - relations: defaultStoreCartRelations, - }) + if (err) { + throw err + } - res.status(200).json({ cart: data }) + res.status(idempotencyKey.response_code).json(idempotencyKey.response_body) } export class StorePostCartsCartLineItemsReq { diff --git a/packages/medusa/src/api/routes/store/carts/create-line-item/utils/handler-steps.ts b/packages/medusa/src/api/routes/store/carts/create-line-item/utils/handler-steps.ts new file mode 100644 index 0000000000..0dfe65b743 --- /dev/null +++ b/packages/medusa/src/api/routes/store/carts/create-line-item/utils/handler-steps.ts @@ -0,0 +1,67 @@ +import { AwilixContainer } from "awilix" +import { EntityManager } from "typeorm" +import { CartService, LineItemService } from "../../../../../../services" +import { FlagRouter } from "../../../../../../utils/flag-router" +import { defaultStoreCartFields, defaultStoreCartRelations } from "../../index" +import { IdempotencyCallbackResult } from "../../../../../../types/idempotency-key" +import { WithRequiredProperty } from "../../../../../../types/common" +import { Cart } from "../../../../../../models" + +export const CreateLineItemSteps = { + STARTED: "started", + FINISHED: "finished", +} + +export async function handleAddOrUpdateLineItem( + cartId: string, + data: { + metadata?: Record + customer_id?: string + variant_id: string + quantity: number + }, + { container, manager }: { container: AwilixContainer; manager: EntityManager } +): Promise { + const cartService: CartService = container.resolve("cartService") + const lineItemService: LineItemService = container.resolve("lineItemService") + const featureFlagRouter: FlagRouter = container.resolve("featureFlagRouter") + + const txCartService = cartService.withTransaction(manager) + + let cart = await txCartService.retrieve(cartId, { + select: ["id", "region_id", "customer_id"], + }) + + const line = await lineItemService + .withTransaction(manager) + .generate(data.variant_id, cart.region_id, data.quantity, { + customer_id: data.customer_id || cart.customer_id, + metadata: data.metadata, + }) + + await txCartService.addLineItem(cart.id, line, { + validateSalesChannels: featureFlagRouter.isFeatureEnabled("sales_channels"), + }) + + cart = await txCartService.retrieveWithTotals(cart.id, { + select: defaultStoreCartFields, + relations: [ + ...defaultStoreCartRelations, + "billing_address", + "region.payment_providers", + "payment_sessions", + "customer", + ], + }) + + if (cart.payment_sessions?.length) { + await txCartService.setPaymentSessions( + cart as WithRequiredProperty + ) + } + + return { + response_code: 200, + response_body: { cart }, + } +} diff --git a/packages/medusa/src/services/__mocks__/idempotency-key.js b/packages/medusa/src/services/__mocks__/idempotency-key.js index cfed46ca23..be1497666d 100644 --- a/packages/medusa/src/services/__mocks__/idempotency-key.js +++ b/packages/medusa/src/services/__mocks__/idempotency-key.js @@ -11,24 +11,20 @@ export const IdempotencyKeyService = { } }), workStage: jest.fn().mockImplementation(async (key, fn) => { - try { - const { recovery_point, response_code, response_body } = await fn( - MockManager - ) + const { recovery_point, response_code, response_body } = await fn( + MockManager + ) - if (recovery_point) { - return { - recovery_point, - } - } else { - return { - recovery_point: "finished", - response_body, - response_code, - } + if (recovery_point) { + return { + recovery_point, + } + } else { + return { + recovery_point: "finished", + response_body, + response_code, } - } catch (err) { - return { error: err } } }), } diff --git a/packages/medusa/src/services/__tests__/cart.js b/packages/medusa/src/services/__tests__/cart.js index c84e76d391..dcead01ab1 100644 --- a/packages/medusa/src/services/__tests__/cart.js +++ b/packages/medusa/src/services/__tests__/cart.js @@ -284,7 +284,8 @@ describe("CartService", () => { describe("addLineItem", () => { const lineItemService = { - update: jest.fn(), + update: jest.fn().mockImplementation(() => Promise.resolve()), + list: jest.fn().mockImplementation(() => Promise.resolve([])), create: jest.fn(), withTransaction: function () { return this @@ -353,7 +354,27 @@ describe("CartService", () => { manager: MockManager, totalsService, cartRepository, - lineItemService, + lineItemService: { + ...lineItemService, + list: jest.fn().mockImplementation((where) => { + if ( + where.cart_id === IdMap.getId("cartWithLine") && + where.variant_id === IdMap.getId("existing") && + where.should_merge + ) { + return Promise.resolve([ + { + ...where, + id: IdMap.getId("merger"), + quantity: 1, + metadata: {}, + }, + ]) + } + + return Promise.resolve([]) + }), + }, lineItemRepository: MockRepository(), newTotalsService: newTotalsServiceMock, eventBusService, @@ -453,12 +474,14 @@ describe("CartService", () => { variant_id: IdMap.getId("existing"), should_merge: true, quantity: 1, + metadata: {}, } await cartService.addLineItem(IdMap.getId("cartWithLine"), lineItem) - expect(lineItemService.update).toHaveBeenCalledTimes(1) - expect(lineItemService.update).toHaveBeenCalledWith( + expect(lineItemService.update).toHaveBeenCalledTimes(2) + expect(lineItemService.update).toHaveBeenNthCalledWith( + 1, IdMap.getId("merger"), { quantity: 2, @@ -519,8 +542,9 @@ describe("CartService", () => { describe("addLineItem w. SalesChannel", () => { const lineItemService = { - update: jest.fn(), + update: jest.fn().mockImplementation(() => Promise.resolve()), create: jest.fn(), + list: jest.fn().mockImplementation(() => Promise.resolve([])), withTransaction: function () { return this }, diff --git a/packages/medusa/src/services/cart.ts b/packages/medusa/src/services/cart.ts index 1ebd949bb1..600aa99676 100644 --- a/packages/medusa/src/services/cart.ts +++ b/packages/medusa/src/services/cart.ts @@ -549,15 +549,15 @@ class CartService extends TransactionBaseService { /** * Check if line item's variant belongs to the cart's sales channel. * - * @param cart - the cart for the line item + * @param sales_channel_id - the cart for the line item * @param lineItem - the line item being added * @return a boolean indicating validation result */ protected async validateLineItem( - cart: Cart, + { sales_channel_id }: { sales_channel_id: string | null }, lineItem: LineItem ): Promise { - if (!cart.sales_channel_id) { + if (!sales_channel_id) { return true } @@ -570,7 +570,7 @@ class CartService extends TransactionBaseService { .withTransaction(this.manager_) .filterProductsBySalesChannel( [lineItemVariant.product_id], - cart.sales_channel_id + sales_channel_id ) ).length } @@ -588,21 +588,16 @@ class CartService extends TransactionBaseService { cartId: string, lineItem: LineItem, config = { validateSalesChannels: true } - ): Promise { + ): Promise { + const select: (keyof Cart)[] = ["id"] + + if (this.featureFlagRouter_.isFeatureEnabled("sales_channels")) { + select.push("sales_channel_id") + } + return await this.atomicPhase_( async (transactionManager: EntityManager) => { - const cart = await this.retrieve(cartId, { - relations: [ - "shipping_methods", - "items", - "items.adjustments", - "payment_sessions", - "items.variant", - "items.variant.product", - "discounts", - "discounts.rule", - ], - }) + let cart = await this.retrieve(cartId, { select }) if (this.featureFlagRouter_.isFeatureEnabled("sales_channels")) { if (config.validateSalesChannels) { @@ -615,14 +610,25 @@ class CartService extends TransactionBaseService { } } + const lineItemServiceTx = + this.lineItemService_.withTransaction(transactionManager) + let currentItem: LineItem | undefined if (lineItem.should_merge) { - currentItem = cart.items.find((item) => { - if (item.should_merge && item.variant_id === lineItem.variant_id) { - return isEqual(item.metadata, lineItem.metadata) - } - return false - }) + const [existingItem] = await lineItemServiceTx.list( + { + cart_id: cart.id, + variant_id: lineItem.variant_id, + should_merge: true, + }, + { take: 1, select: ["id", "metadata", "quantity"] } + ) + if ( + existingItem && + isEqual(existingItem.metadata, lineItem.metadata) + ) { + currentItem = existingItem + } } // If content matches one of the line items currently in the cart we can @@ -637,44 +643,33 @@ class CartService extends TransactionBaseService { .confirmInventory(lineItem.variant_id, quantity) if (currentItem) { - await this.lineItemService_ - .withTransaction(transactionManager) - .update(currentItem.id, { - quantity: currentItem.quantity, - }) + await lineItemServiceTx.update(currentItem.id, { + quantity: currentItem.quantity, + }) } else { - await this.lineItemService_ - .withTransaction(transactionManager) - .create({ - ...lineItem, - has_shipping: false, - cart_id: cartId, - }) + await lineItemServiceTx.create({ + ...lineItem, + has_shipping: false, + cart_id: cart.id, + }) } - const lineItemRepository = transactionManager.getCustomRepository( - this.lineItemRepository_ - ) - await lineItemRepository.update( - { - id: In(cart.items.map((item) => item.id)), - }, - { - has_shipping: false, - } - ) + await lineItemServiceTx + .update( + { cart_id: cartId, has_shipping: true }, + { has_shipping: false } + ) + .catch(() => void 0) - const result = await this.retrieve(cartId, { + cart = await this.retrieve(cart.id, { relations: ["items", "discounts", "discounts.rule", "region"], }) - await this.refreshAdjustments_(result) + await this.refreshAdjustments_(cart) await this.eventBus_ .withTransaction(transactionManager) - .emit(CartService.Events.UPDATED, result) - - return result + .emit(CartService.Events.UPDATED, { id: cart.id }) } ) } diff --git a/packages/medusa/src/services/idempotency-key.ts b/packages/medusa/src/services/idempotency-key.ts index 62d1254ebd..11f0955ef1 100644 --- a/packages/medusa/src/services/idempotency-key.ts +++ b/packages/medusa/src/services/idempotency-key.ts @@ -4,7 +4,10 @@ import { TransactionBaseService } from "../interfaces" import { DeepPartial, EntityManager } from "typeorm" import { IdempotencyKeyRepository } from "../repositories/idempotency-key" import { IdempotencyKey } from "../models" -import { CreateIdempotencyKeyInput } from "../types/idempotency-key" +import { + CreateIdempotencyKeyInput, + IdempotencyCallbackResult, +} from "../types/idempotency-key" const KEY_LOCKED_TIMEOUT = 1000 @@ -163,14 +166,9 @@ class IdempotencyKeyService extends TransactionBaseService { */ async workStage( idempotencyKey: string, - callback: (transactionManager: EntityManager) => Promise< - | { - recovery_point?: string - response_code?: number - response_body?: Record - } - | never - > + callback: ( + transactionManager: EntityManager + ) => Promise ): Promise { return await this.atomicPhase_(async (manager) => { const { recovery_point, response_code, response_body } = await callback( diff --git a/packages/medusa/src/services/line-item.ts b/packages/medusa/src/services/line-item.ts index 39c3e98e04..e2bea63406 100644 --- a/packages/medusa/src/services/line-item.ts +++ b/packages/medusa/src/services/line-item.ts @@ -285,8 +285,8 @@ class LineItemService extends TransactionBaseService { this.lineItemRepository_ ) - const lineItem = lineItemRepository.create(data) - return await lineItemRepository.save(lineItem) + const item = lineItemRepository.create(data) + return await lineItemRepository.save(item) } ) } diff --git a/packages/medusa/src/types/idempotency-key.ts b/packages/medusa/src/types/idempotency-key.ts index 5459bac4fe..1d885ee8c7 100644 --- a/packages/medusa/src/types/idempotency-key.ts +++ b/packages/medusa/src/types/idempotency-key.ts @@ -4,3 +4,9 @@ export type CreateIdempotencyKeyInput = { request_path: string idempotency_key?: string } + +export type IdempotencyCallbackResult = { + recovery_point?: string + response_code?: number + response_body?: Record +} diff --git a/packages/medusa/src/utils/exception-formatter.ts b/packages/medusa/src/utils/exception-formatter.ts index 8b361b9eeb..685ee3cf42 100644 --- a/packages/medusa/src/utils/exception-formatter.ts +++ b/packages/medusa/src/utils/exception-formatter.ts @@ -3,7 +3,9 @@ import { MedusaError } from "medusa-core-utils" export enum PostgresError { DUPLICATE_ERROR = "23505", FOREIGN_KEY_ERROR = "23503", + SERIALIZATION_FAILURE = "40001", } + export const formatException = (err): MedusaError => { switch (err.code) { case PostgresError.DUPLICATE_ERROR: @@ -35,6 +37,12 @@ export const formatException = (err): MedusaError => { } ${matches[2]} does not exist.` ) } + case PostgresError.SERIALIZATION_FAILURE: { + return new MedusaError( + MedusaError.Types.CONFLICT, + err?.detail ?? err?.message + ) + } default: return err } diff --git a/packages/medusa/src/utils/idempotency/index.ts b/packages/medusa/src/utils/idempotency/index.ts new file mode 100644 index 0000000000..8d7196fd83 --- /dev/null +++ b/packages/medusa/src/utils/idempotency/index.ts @@ -0,0 +1,2 @@ +export * from "./run-idempotency-step" +export * from "./initialize-idempotency-request" diff --git a/packages/medusa/src/utils/idempotency/initialize-idempotency-request.ts b/packages/medusa/src/utils/idempotency/initialize-idempotency-request.ts new file mode 100644 index 0000000000..ae8bc96022 --- /dev/null +++ b/packages/medusa/src/utils/idempotency/initialize-idempotency-request.ts @@ -0,0 +1,26 @@ +import { Request, Response } from "express" +import { IdempotencyKey } from "../../models" +import IdempotencyKeyService from "../../services/idempotency-key" +import { EntityManager } from "typeorm" + +export async function initializeIdempotencyRequest( + req: Request, + res: Response +): Promise { + const idempotencyKeyService: IdempotencyKeyService = req.scope.resolve( + "idempotencyKeyService" + ) + const manager: EntityManager = req.scope.resolve("manager") + + const headerKey = req.get("Idempotency-Key") || "" + + let idempotencyKey + idempotencyKey = await idempotencyKeyService + .withTransaction(manager) + .initializeRequest(headerKey, req.method, req.params, req.path) + + res.setHeader("Access-Control-Expose-Headers", "Idempotency-Key") + res.setHeader("Idempotency-Key", idempotencyKey.idempotency_key) + + return idempotencyKey +} diff --git a/packages/medusa/src/utils/idempotency/run-idempotency-step.ts b/packages/medusa/src/utils/idempotency/run-idempotency-step.ts new file mode 100644 index 0000000000..b40f3deabd --- /dev/null +++ b/packages/medusa/src/utils/idempotency/run-idempotency-step.ts @@ -0,0 +1,40 @@ +import { IdempotencyCallbackResult } from "../../types/idempotency-key" +import { EntityManager } from "typeorm" +import { IdempotencyKey } from "../../models" +import { AwilixContainer } from "awilix" +import IdempotencyKeyService from "../../services/idempotency-key" +import { IsolationLevel } from "typeorm/driver/types/IsolationLevel" + +export type RunIdempotencyStepOptions = { + manager: EntityManager + idempotencyKey: IdempotencyKey + container: AwilixContainer + isolationLevel: IsolationLevel +} + +export async function runIdempotencyStep( + handler: ({ manager: EntityManager }) => Promise, + { + manager, + idempotencyKey, + container, + isolationLevel, + }: RunIdempotencyStepOptions +) { + const idempotencyKeyService: IdempotencyKeyService = container.resolve( + "idempotencyKeyService" + ) + return await manager.transaction( + isolationLevel, + async (transactionManager) => { + const idempotencyKey_ = await idempotencyKeyService + .withTransaction(transactionManager) + .workStage(idempotencyKey.idempotency_key, async (stageManager) => { + return await handler({ manager: stageManager }) + }) + idempotencyKey.response_code = idempotencyKey_.response_code + idempotencyKey.response_body = idempotencyKey_.response_body + idempotencyKey.recovery_point = idempotencyKey_.recovery_point + } + ) +}