fix(medusa): Idempotency workStage used within transaction (#2358)

.changeset/rich-ants-press.md

@@ -0,0 +1,5 @@
+---
+"@medusajs/medusa": minor
+---
+
+fix(medusa): Idempotency workStage used within transaction

integration-tests/api/__tests__/claims/index.js

@@ -25,7 +25,7 @@ describe("Claims", () => {
   beforeAll(async () => {
     const cwd = path.resolve(path.join(__dirname, "..", ".."))
     dbConnection = await initDb({ cwd })
-    medusaProcess = await setupServer({ cwd })
+    medusaProcess = await setupServer({ cwd, verbose: true })
   })
 
   afterAll(async () => {
@@ -202,6 +202,54 @@ describe("Claims", () => {
 
     expect(response.status).toEqual(200)
   })
+
+  test(" should throw and not have dangling claim order upon a claim creation without reasons on claim items", async () => {
+    const api = useApi()
+
+    await adminSeeder(dbConnection)
+
+    const order = await createReturnableOrder(dbConnection)
+    const option = await simpleShippingOptionFactory(dbConnection, {
+      region_id: "test-region",
+    })
+
+    const err = await api
+      .post(
+        `/admin/orders/${order.id}/claims`,
+        {
+          type: "replace",
+          shipping_methods: [
+            {
+              option_id: option.id,
+              price: 0,
+            },
+          ],
+          additional_items: [{ variant_id: "test-variant", quantity: 1 }],
+          claim_items: [
+            {
+              item_id: "test-item",
+              quantity: 1,
+            },
+          ],
+        },
+        {
+          headers: {
+            authorization: "Bearer test_token",
+          },
+        }
+      )
+      .catch((e) => e)
+
+    const claimOrders = await dbConnection.manager.query(
+      `SELECT *
+       FROM claim_order
+       WHERE order_id = '${order.id}'`
+    )
+
+    expect(err).toBeDefined()
+    expect(err.response.status).toBe(400)
+    expect(claimOrders).toEqual([])
+  })
 })
 
 const createReturnableOrder = async (dbConnection, options = {}) => {

packages/medusa/src/api/routes/admin/orders/create-claim.ts

@@ -1,4 +1,3 @@
-import { ClaimReason, ClaimType } from "../../../../models"
 import {
   IsArray,
   IsBoolean,
@@ -11,12 +10,13 @@ import {
   ValidateNested,
 } from "class-validator"
 import { defaultAdminOrdersFields, defaultAdminOrdersRelations } from "."
+import { ClaimReason, ClaimType } from "../../../../models"
 
-import { AddressPayload } from "../../../../types/common"
-import { ClaimTypeValue } from "../../../../types/claim"
-import { EntityManager } from "typeorm"
-import { MedusaError } from "medusa-core-utils"
 import { Type } from "class-transformer"
+import { MedusaError } from "medusa-core-utils"
+import { EntityManager } from "typeorm"
+import { ClaimTypeValue } from "../../../../types/claim"
+import { AddressPayload } from "../../../../types/common"
 import { validator } from "../../../../utils/validator"
 
 /**
@@ -223,8 +223,9 @@ export default async (req, res) => {
   while (inProgress) {
     switch (idempotencyKey.recovery_point) {
       case "started": {
-        await manager.transaction(async (transactionManager) => {
+        await manager
-          const { key, error } = await idempotencyKeyService
+          .transaction("SERIALIZABLE", async (transactionManager) => {
+            idempotencyKey = await idempotencyKeyService
               .withTransaction(transactionManager)
               .workStage(idempotencyKey.idempotency_key, async (manager) => {
                 const order = await orderService
@@ -264,20 +265,18 @@ export default async (req, res) => {
                   recovery_point: "claim_created",
                 }
               })
-
+          })
-          if (error) {
+          .catch((e) => {
             inProgress = false
-            err = error
+            err = e
-          } else {
-            idempotencyKey = key
-          }
           })
         break
       }
 
       case "claim_created": {
-        await manager.transaction(async (transactionManager) => {
+        await manager
-          const { key, error } = await idempotencyKeyService
+          .transaction("SERIALIZABLE", async (transactionManager) => {
+            idempotencyKey = await idempotencyKeyService
               .withTransaction(transactionManager)
               .workStage(idempotencyKey.idempotency_key, async (manager) => {
                 let claim = await claimService.withTransaction(manager).list({
@@ -303,20 +302,18 @@ export default async (req, res) => {
                   recovery_point: "refund_handled",
                 }
               })
-
+          })
-          if (error) {
+          .catch((e) => {
             inProgress = false
-            err = error
+            err = e
-          } else {
-            idempotencyKey = key
-          }
           })
         break
       }
 
       case "refund_handled": {
-        await manager.transaction(async (transactionManager) => {
+        await manager
-          const { key, error } = await idempotencyKeyService
+          .transaction("SERIALIZABLE", async (transactionManager) => {
+            idempotencyKey = await idempotencyKeyService
               .withTransaction(transactionManager)
               .workStage(idempotencyKey.idempotency_key, async (manager) => {
                 let order = await orderService
@@ -349,7 +346,9 @@ export default async (req, res) => {
                     .fulfill(claim.return_order.id)
                 }
 
-              order = await orderService.withTransaction(manager).retrieve(id, {
+                order = await orderService
+                  .withTransaction(manager)
+                  .retrieve(id, {
                     select: defaultAdminOrdersFields,
                     relations: defaultAdminOrdersRelations,
                   })
@@ -359,13 +358,10 @@ export default async (req, res) => {
                   response_body: { order },
                 }
               })
-
+          })
-          if (error) {
+          .catch((e) => {
             inProgress = false
-            err = error
+            err = e
-          } else {
-            idempotencyKey = key
-          }
           })
         break
       }

packages/medusa/src/api/routes/admin/orders/create-swap.ts

@@ -200,8 +200,9 @@ export default async (req, res) => {
   while (inProgress) {
     switch (idempotencyKey.recovery_point) {
       case "started": {
-        await manager.transaction(async (transactionManager) => {
+        await manager
-          const { key, error } = await idempotencyKeyService
+          .transaction("SERIALIZABLE", async (transactionManager) => {
+            idempotencyKey = await idempotencyKeyService
               .withTransaction(transactionManager)
               .workStage(idempotencyKey.idempotency_key, async (manager) => {
                 const order = await orderService
@@ -247,24 +248,20 @@ export default async (req, res) => {
                   recovery_point: "swap_created",
                 }
               })
-
+          })
-          if (error) {
+          .catch((e) => {
             inProgress = false
-            err = error
+            err = e
-          } else {
-            idempotencyKey = key
-          }
           })
         break
       }
 
       case "swap_created": {
-        await manager.transaction(async (transactionManager) => {
+        await manager
-          const { key, error } = await idempotencyKeyService
+          .transaction("SERIALIZABLE", async (transactionManager) => {
+            idempotencyKey = await idempotencyKeyService
               .withTransaction(transactionManager)
-            .workStage(
+              .workStage(idempotencyKey.idempotency_key, async (manager) => {
-              idempotencyKey.idempotency_key,
-              async (transactionManager: EntityManager) => {
                 const swaps = await swapService
                   .withTransaction(transactionManager)
                   .list({
@@ -289,15 +286,11 @@ export default async (req, res) => {
                   response_code: 200,
                   response_body: { order },
                 }
-              }
+              })
-            )
+          })
-
+          .catch((e) => {
-          if (error) {
             inProgress = false
-            err = error
+            err = e
-          } else {
-            idempotencyKey = key
-          }
           })
         break
       }

packages/medusa/src/api/routes/admin/orders/request-return.ts

@@ -175,8 +175,9 @@ export default async (req, res) => {
     while (inProgress) {
       switch (idempotencyKey.recovery_point) {
         case "started": {
-          await manager.transaction(async (transactionManager) => {
+          await manager
-            const { key, error } = await idempotencyKeyService
+            .transaction("SERIALIZABLE", async (transactionManager) => {
+              idempotencyKey = await idempotencyKeyService
                 .withTransaction(transactionManager)
                 .workStage(idempotencyKey.idempotency_key, async (manager) => {
                   const returnObj: ReturnObj = {
@@ -229,20 +230,18 @@ export default async (req, res) => {
                     recovery_point: "return_requested",
                   }
                 })
-
+            })
-            if (error) {
+            .catch((e) => {
               inProgress = false
-              err = error
+              err = e
-            } else {
-              idempotencyKey = key
-            }
             })
           break
         }
 
         case "return_requested": {
-          await manager.transaction(async (transactionManager) => {
+          await manager
-            const { key, error } = await idempotencyKeyService
+            .transaction("SERIALIZABLE", async (transactionManager) => {
+              idempotencyKey = await idempotencyKeyService
                 .withTransaction(transactionManager)
                 .workStage(idempotencyKey.idempotency_key, async (manager) => {
                   let order: Order | Return = await orderService
@@ -286,13 +285,10 @@ export default async (req, res) => {
                     response_body: { order },
                   }
                 })
-
+            })
-            if (error) {
+            .catch((e) => {
               inProgress = false
-              err = error
+              err = e
-            } else {
-              idempotencyKey = key
-            }
             })
           break
         }

packages/medusa/src/api/routes/store/carts/calculate-taxes.ts

@@ -73,12 +73,11 @@ export default async (req, res) => {
   while (inProgress) {
     switch (idempotencyKey.recovery_point) {
       case "started": {
-        await manager.transaction(async (transactionManager) => {
+        await manager
-          const { key, error } = await idempotencyKeyService
+          .transaction("SERIALIZABLE", async (transactionManager) => {
+            idempotencyKey = await idempotencyKeyService
               .withTransaction(transactionManager)
-            .workStage(
+              .workStage(idempotencyKey.idempotency_key, async (manager) => {
-              idempotencyKey.idempotency_key,
-              async (manager: EntityManager) => {
                 const cart = await cartService
                   .withTransaction(manager)
                   .retrieveWithTotals(id, {}, { force_taxes: true })
@@ -87,15 +86,11 @@ export default async (req, res) => {
                   response_code: 200,
                   response_body: { cart },
                 }
-              }
+              })
-            )
+          })
-
+          .catch((e) => {
-          if (error) {
             inProgress = false
-            err = error
+            err = e
-          } else {
-            idempotencyKey = key!
-          }
           })
         break
       }

packages/medusa/src/api/routes/store/carts/create-payment-sessions.ts

@@ -79,8 +79,9 @@ export default async (req, res) => {
     while (inProgress) {
       switch (idempotencyKey.recovery_point) {
         case "started": {
-          await manager.transaction(async (transactionManager) => {
+          await manager
-            const { key, error } = await idempotencyKeyService
+            .transaction("SERIALIZABLE", async (transactionManager) => {
+              idempotencyKey = await idempotencyKeyService
                 .withTransaction(transactionManager)
                 .workStage(
                   idempotencyKey.idempotency_key,
@@ -102,13 +103,10 @@ export default async (req, res) => {
                     }
                   }
                 )
-
+            })
-            if (error) {
+            .catch((e) => {
               inProgress = false
-              err = error
+              err = e
-            } else {
-              idempotencyKey = key
-            }
             })
           break
         }

packages/medusa/src/api/routes/store/returns/create-return.ts

@@ -13,7 +13,6 @@ import { MedusaError } from "medusa-core-utils"
 import { EntityManager } from "typeorm"
 import EventBusService from "../../../../services/event-bus"
 import IdempotencyKeyService from "../../../../services/idempotency-key"
-import OrderService from "../../../../services/order"
 import ReturnService from "../../../../services/return"
 import { validator } from "../../../../utils/validator"
 
@@ -142,7 +141,6 @@ export default async (req, res) => {
   res.setHeader("Idempotency-Key", idempotencyKey.idempotency_key)
 
   try {
-    const orderService: OrderService = req.scope.resolve("orderService")
     const returnService: ReturnService = req.scope.resolve("returnService")
     const eventBus: EventBusService = req.scope.resolve("eventBusService")
 
@@ -152,17 +150,11 @@ export default async (req, res) => {
     while (inProgress) {
       switch (idempotencyKey.recovery_point) {
         case "started": {
-          await manager.transaction(async (transactionManager) => {
+          await manager
-            const { key, error } = await idempotencyKeyService
+            .transaction("SERIALIZABLE", async (transactionManager) => {
+              idempotencyKey = await idempotencyKeyService
                 .withTransaction(transactionManager)
                 .workStage(idempotencyKey.idempotency_key, async (manager) => {
-                const order = await orderService
-                  .withTransaction(manager)
-                  .retrieve(returnDto.order_id, {
-                    select: ["refunded_total", "total"],
-                    relations: ["items"],
-                  })
-
                   const returnObj: any = {
                     order_id: returnDto.order_id,
                     idempotency_key: idempotencyKey.idempotency_key,
@@ -194,20 +186,18 @@ export default async (req, res) => {
                     recovery_point: "return_requested",
                   }
                 })
-
+            })
-            if (error) {
+            .catch((e) => {
               inProgress = false
-              err = error
+              err = e
-            } else {
-              idempotencyKey = key
-            }
             })
           break
         }
 
         case "return_requested": {
-          await manager.transaction(async (transactionManager) => {
+          await manager
-            const { key, error } = await idempotencyKeyService
+            .transaction("SERIALIZABLE", async (transactionManager) => {
+              idempotencyKey = await idempotencyKeyService
                 .withTransaction(transactionManager)
                 .workStage(idempotencyKey.idempotency_key, async (manager) => {
                   const returnOrders = await returnService
@@ -233,13 +223,10 @@ export default async (req, res) => {
                     response_body: { return: returnOrder },
                   }
                 })
-
+            })
-            if (error) {
+            .catch((e) => {
               inProgress = false
-              err = error
+              err = e
-            } else {
-              idempotencyKey = key
-            }
             })
           break
         }

packages/medusa/src/api/routes/store/swaps/create-swap.ts

@@ -173,8 +173,9 @@ export default async (req, res) => {
   while (inProgress) {
     switch (idempotencyKey.recovery_point) {
       case "started": {
-        await manager.transaction(async (transactionManager) => {
+        await manager
-          const { key, error } = await idempotencyKeyService
+          .transaction("SERIALIZABLE", async (transactionManager) => {
+            idempotencyKey = await idempotencyKeyService
               .withTransaction(transactionManager)
               .workStage(idempotencyKey.idempotency_key, async (manager) => {
                 const order = await orderService
@@ -223,24 +224,20 @@ export default async (req, res) => {
                   recovery_point: "swap_created",
                 }
               })
-
+          })
-          if (error) {
+          .catch((e) => {
             inProgress = false
-            err = error
+            err = e
-          } else {
-            idempotencyKey = key
-          }
           })
         break
       }
 
       case "swap_created": {
-        await manager.transaction(async (transactionManager) => {
+        await manager
-          const { key, error } = await idempotencyKeyService
+          .transaction("SERIALIZABLE", async (transactionManager) => {
+            idempotencyKey = await idempotencyKeyService
               .withTransaction(transactionManager)
-            .workStage(
+              .workStage(idempotencyKey.idempotency_key, async (manager) => {
-              idempotencyKey.idempotency_key,
-              async (transactionManager: EntityManager) => {
                 const swaps = await swapService
                   .withTransaction(transactionManager)
                   .list({
@@ -265,15 +262,11 @@ export default async (req, res) => {
                   response_code: 200,
                   response_body: { swap },
                 }
-              }
+              })
-            )
+          })
-
+          .catch((e) => {
-          if (error) {
             inProgress = false
-            err = error
+            err = e
-          } else {
-            idempotencyKey = key
-          }
           })
         break
       }

packages/medusa/src/services/__mocks__/idempotency-key.js

@@ -18,15 +18,13 @@ export const IdempotencyKeyService = {
 
       if (recovery_point) {
         return {
-          key: { recovery_point },
+          recovery_point,
         }
       } else {
         return {
-          key: {
           recovery_point: "finished",
           response_body,
           response_code,
-          },
         }
       }
     } catch (err) {

packages/medusa/src/services/claim.ts

@@ -1,16 +1,6 @@
-import ClaimItemService from "./claim-item"
+import { MedusaError } from "medusa-core-utils"
-import EventBusService from "./event-bus"
+import { DeepPartial, EntityManager } from "typeorm"
-import FulfillmentProviderService from "./fulfillment-provider"
+import { TransactionBaseService } from "../interfaces"
-import FulfillmentService from "./fulfillment"
-import InventoryService from "./inventory"
-import LineItemService from "./line-item"
-import PaymentProviderService from "./payment-provider"
-import RegionService from "./region"
-import ReturnService from "./return"
-import ShippingOptionService from "./shipping-option"
-import TaxProviderService from "./tax-provider"
-import TotalsService from "./totals"
-import { AddressRepository } from "../repositories/address"
 import {
   ClaimFulfillmentStatus,
   ClaimOrder,
@@ -20,15 +10,25 @@ import {
   LineItem,
   ReturnItem,
 } from "../models"
+import { AddressRepository } from "../repositories/address"
 import { ClaimRepository } from "../repositories/claim"
-import { DeepPartial, EntityManager } from "typeorm"
 import { LineItemRepository } from "../repositories/line-item"
-import { MedusaError } from "medusa-core-utils"
 import { ShippingMethodRepository } from "../repositories/shipping-method"
-import { TransactionBaseService } from "../interfaces"
-import { buildQuery, isDefined, setMetadata } from "../utils"
-import { FindConfig } from "../types/common"
 import { CreateClaimInput, UpdateClaimInput } from "../types/claim"
+import { FindConfig } from "../types/common"
+import { buildQuery, isDefined, setMetadata } from "../utils"
+import ClaimItemService from "./claim-item"
+import EventBusService from "./event-bus"
+import FulfillmentService from "./fulfillment"
+import FulfillmentProviderService from "./fulfillment-provider"
+import InventoryService from "./inventory"
+import LineItemService from "./line-item"
+import PaymentProviderService from "./payment-provider"
+import RegionService from "./region"
+import ReturnService from "./return"
+import ShippingOptionService from "./shipping-option"
+import TaxProviderService from "./tax-provider"
+import TotalsService from "./totals"
 
 type InjectedDependencies = {
   manager: EntityManager

packages/medusa/src/services/idempotency-key.ts

@@ -171,8 +171,7 @@ class IdempotencyKeyService extends TransactionBaseService {
         }
       | never
     >
-  ): Promise<{ key?: IdempotencyKey; error?: unknown }> {
+  ): Promise<IdempotencyKey> {
-    try {
     return await this.atomicPhase_(async (manager) => {
       const { recovery_point, response_code, response_body } = await callback(
         manager
@@ -187,12 +186,8 @@ class IdempotencyKeyService extends TransactionBaseService {
         data.response_code = response_code
       }
 
-        const key = await this.update(idempotencyKey, data)
+      return await this.update(idempotencyKey, data)
-        return { key }
+    })
-      }, "SERIALIZABLE")
-    } catch (err) {
-      return { error: err }
-    }
   }
 }
 

packages/medusa/src/strategies/__tests__/cart-completion.js

@@ -13,21 +13,23 @@ const IdempotencyKeyServiceMock = {
 
       if (recovery_point) {
         return {
-          key: { idempotency_key: key, recovery_point },
+          idempotency_key: key,
+          recovery_point,
         }
       } else {
         return {
-          key: {
           recovery_point: "finished",
           response_body,
           response_code,
-          },
         }
       }
     } catch (err) {
       return { error: err }
     }
   }),
+  update: jest.fn().mockImplementation((key, data) => {
+    return data
+  }),
 }
 
 const toTest = [

packages/medusa/src/strategies/cart-completion.ts

@@ -63,12 +63,11 @@ class CartCompletionStrategy extends AbstractCartCompletionStrategy {
     while (inProgress) {
       switch (idempotencyKey.recovery_point) {
         case "started": {
-          await this.manager_.transaction(async (transactionManager) => {
+          await this.manager_
-            const { key, error } = await idempotencyKeyService
+            .transaction("SERIALIZABLE", async (transactionManager) => {
+              idempotencyKey = await idempotencyKeyService
                 .withTransaction(transactionManager)
-              .workStage(
+                .workStage(idempotencyKey.idempotency_key, async (manager) => {
-                idempotencyKey.idempotency_key,
-                async (manager: EntityManager) => {
                   const cart = await cartService
                     .withTransaction(manager)
                     .retrieve(id)
@@ -89,25 +88,20 @@ class CartCompletionStrategy extends AbstractCartCompletionStrategy {
                   return {
                     recovery_point: "tax_lines_created",
                   }
-                }
+                })
-              )
+            })
-
+            .catch((e) => {
-            if (error) {
               inProgress = false
-              err = error
+              err = e
-            } else {
-              idempotencyKey = key as IdempotencyKey
-            }
             })
           break
         }
         case "tax_lines_created": {
-          await this.manager_.transaction(async (transactionManager) => {
+          await this.manager_
-            const { key, error } = await idempotencyKeyService
+            .transaction("SERIALIZABLE", async (transactionManager) => {
+              idempotencyKey = await idempotencyKeyService
                 .withTransaction(transactionManager)
-              .workStage(
+                .workStage(idempotencyKey.idempotency_key, async (manager) => {
-                idempotencyKey.idempotency_key,
-                async (manager: EntityManager) => {
                   const cart = await cartService
                     .withTransaction(manager)
                     .authorizePayment(id, {
@@ -138,26 +132,21 @@ class CartCompletionStrategy extends AbstractCartCompletionStrategy {
                   return {
                     recovery_point: "payment_authorized",
                   }
-                }
+                })
-              )
+            })
-
+            .catch((e) => {
-            if (error) {
               inProgress = false
-              err = error
+              err = e
-            } else {
-              idempotencyKey = key as IdempotencyKey
-            }
             })
           break
         }
 
         case "payment_authorized": {
-          await this.manager_.transaction(async (transactionManager) => {
+          await this.manager_
-            const { key, error } = await idempotencyKeyService
+            .transaction("SERIALIZABLE", async (transactionManager) => {
+              idempotencyKey = await idempotencyKeyService
                 .withTransaction(transactionManager)
-              .workStage(
+                .workStage(idempotencyKey.idempotency_key, async (manager) => {
-                idempotencyKey.idempotency_key,
-                async (manager: EntityManager) => {
                   const cart = await cartService
                     .withTransaction(manager)
                     .retrieveWithTotals(id, {
@@ -287,15 +276,11 @@ class CartCompletionStrategy extends AbstractCartCompletionStrategy {
                       }
                     }
                   }
-                }
+                })
-              )
+            })
-
+            .catch((e) => {
-            if (error) {
               inProgress = false
-              err = error
+              err = e
-            } else {
-              idempotencyKey = key as IdempotencyKey
-            }
             })
           break
         }