From 5f807ee657c81178f1bf04f695b20ebdb996762c Mon Sep 17 00:00:00 2001 From: Oli Juhl <59018053+olivermrbl@users.noreply.github.com> Date: Fri, 2 Jan 2026 11:16:29 +0100 Subject: [PATCH] chore: Ensure isolated order deletion (#14405) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Ensure that address deletion during order deletion is handled correctly with respect to cascading deletes. Currently, when deleting an order that does not have a shipping or billing address, we incorrectly attempt to delete all order addresses. This happens because `undefined` address IDs are not handled properly during deletion. More specifically, when deleting an order without addresses, the following method is called with these arguments: ```ts await deleteOrderAddresses([undefined, undefined]) ``` This triggers deletion of all order addresses. To make matters worse, because we have a cascade delete defined from order addresses to orders, the same call also deletes all associated orders. The root cause is the following filtering logic: ```ts const orderAddressIds = orders .map((order) => [order.shipping_address_id, order.billing_address_id]) .flat(1) ``` For orders without addresses, this produces `[undefined, undefined]` as input filter, which our delete methods interpret as: ```ts await delete({ '$or': [ { id: undefined }, { id: undefined } ] }) ``` This effectively translates to “delete all addresses.” In this PR, we make two changes to prevent this going forward: 1. Filter out undefined address IDs before attempting deletion 2. Remove the cascade delete from address to order, as this is an aggressive constraint The latter change is open for discussion, but cascading deletes from a child entity to a parent is slightly off IMO. Let me know your thoughts. ## Checklist Please ensure the following before requesting a review: - [x] I have added a **changeset** for this PR - Every non-breaking change should be marked as a **patch** - To add a changeset, run `yarn changeset` and follow the prompts - [x] The changes are covered by relevant **tests** - [x] I have verified the code works as intended locally - [ ] I have linked the related issue(s) if applicable --- ## Additional Context Add any additional context, related issues, or references that might help the reviewer understand this PR. --- > [!NOTE] > Strengthens order deletion to avoid unintended cascades and validates behavior with tests. > > - Update FK constraints in migration to `ON DELETE SET NULL` for `order.shipping_address_id` and `order.billing_address_id` (was `CASCADE`) > - In `order-module-service.ts` `deleteOrders`, filter falsy address IDs and conditionally batch-delete `order_address`/`order_change` via `promiseAll` > - Add integration tests `integration-tests/__tests__/delete-order.spec.ts` covering deletion of orders and associated entities, deleting orders without addresses (no cross-order impact), and address deletion setting `NULL` on the order > - Add changeset marking `@medusajs/order` patch > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8e4ab59af407ec865f73fbf286ec93e710915c8e. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .changeset/angry-rivers-collect.md | 5 + .../__tests__/delete-order.spec.ts | 225 ++++++++++++++++++ .../src/migrations/Migration20251225120947.ts | 51 ++++ .../src/services/order-module-service.ts | 16 +- 4 files changed, 295 insertions(+), 2 deletions(-) create mode 100644 .changeset/angry-rivers-collect.md create mode 100644 packages/modules/order/integration-tests/__tests__/delete-order.spec.ts create mode 100644 packages/modules/order/src/migrations/Migration20251225120947.ts diff --git a/.changeset/angry-rivers-collect.md b/.changeset/angry-rivers-collect.md new file mode 100644 index 0000000000..8105bda788 --- /dev/null +++ b/.changeset/angry-rivers-collect.md @@ -0,0 +1,5 @@ +--- +"@medusajs/order": patch +--- + +chore: Ensure isolated order deletion diff --git a/packages/modules/order/integration-tests/__tests__/delete-order.spec.ts b/packages/modules/order/integration-tests/__tests__/delete-order.spec.ts new file mode 100644 index 0000000000..3d010ce944 --- /dev/null +++ b/packages/modules/order/integration-tests/__tests__/delete-order.spec.ts @@ -0,0 +1,225 @@ +import { + CreateOrderDTO, + IOrderModuleService, + OrderDTO, +} from "@medusajs/framework/types" +import { Modules } from "@medusajs/framework/utils" +import { moduleIntegrationTestRunner } from "@medusajs/test-utils" + +jest.setTimeout(100000) + +moduleIntegrationTestRunner({ + moduleName: Modules.ORDER, + testSuite: ({ service, MikroOrmWrapper }) => { + describe("deleteOrders", () => { + let order: OrderDTO + + beforeEach(async () => { + order = await service.createOrders({ + email: "foo@bar.com", + items: [ + { + title: "Item 1", + subtitle: "Subtitle 1", + thumbnail: "thumbnail1.jpg", + quantity: 1, + product_id: "product1", + product_title: "Product 1", + product_description: "Description 1", + product_subtitle: "Product Subtitle 1", + product_type: "Type 1", + product_type_id: "type_1", + product_collection: "Collection 1", + product_handle: "handle1", + variant_id: "variant1", + variant_sku: "SKU1", + variant_barcode: "Barcode1", + variant_title: "Variant 1", + variant_option_values: { + color: "Red", + size: "Large", + }, + requires_shipping: true, + is_discountable: true, + is_tax_inclusive: true, + compare_at_unit_price: 10, + unit_price: 20, + tax_lines: [ + { + description: "Tax 1", + tax_rate_id: "tax_usa", + code: "code", + rate: 10, + provider_id: "taxify_master", + }, + ], + adjustments: [ + { + code: "VIP_10", + amount: 10, + description: "VIP discount", + promotion_id: "prom_123", + provider_id: "coupon_kings", + }, + ], + }, + ], + shipping_methods: [ + { + name: "Test shipping method", + amount: 10, + data: {}, + tax_lines: [ + { + description: "shipping Tax 1", + tax_rate_id: "tax_usa_shipping", + code: "code", + rate: 10, + }, + ], + adjustments: [ + { + code: "VIP_10", + amount: 1, + description: "VIP discount", + promotion_id: "prom_123", + }, + ], + }, + ], + shipping_address: { + first_name: "Test", + last_name: "Test", + address_1: "Test", + city: "Test", + country_code: "US", + postal_code: "12345", + phone: "12345", + }, + billing_address: { + first_name: "Test", + last_name: "Test", + address_1: "Test", + city: "Test", + country_code: "US", + postal_code: "12345", + }, + currency_code: "usd", + customer_id: "joe", + } as CreateOrderDTO) + }) + + it("should delete an order, shipping method, taxes, adjustments, and items", async function () { + const lineItemsBefore = await service.listOrderLineItems({}) + const itemAdjustmentsBefore = + await service.listOrderLineItemAdjustments({}) + const itemTaxLinesBefore = await service.listOrderLineItemTaxLines({}) + const shippingMethodTaxLinesBefore = + await service.listOrderShippingMethodTaxLines({}) + const shippingMethodAdjustmentsBefore = + await service.listOrderShippingMethodAdjustments({}) + const shippingMethodsBefore = await service.listOrderShippingMethods( + {}, + {} + ) + + expect(lineItemsBefore.length).toEqual(1) + expect(itemAdjustmentsBefore.length).toEqual(1) + expect(itemTaxLinesBefore.length).toEqual(1) + expect(shippingMethodTaxLinesBefore.length).toEqual(1) + expect(shippingMethodAdjustmentsBefore.length).toEqual(1) + expect(shippingMethodsBefore.length).toEqual(1) + + await service.deleteOrders(order.id) + + const lineItems = await service.listOrderLineItems({}) + const itemAdjustments = await service.listOrderLineItemAdjustments({}) + const itemTaxLines = await service.listOrderLineItemTaxLines({}) + const shippingMethodTaxLines = + await service.listOrderShippingMethodTaxLines({}) + const shippingMethodAdjustments = + await service.listOrderShippingMethodAdjustments({}) + const shippingMethods = await service.listOrderShippingMethods({}, {}) + + expect(lineItems).toEqual([]) + expect(itemAdjustments).toEqual([]) + expect(itemTaxLines).toEqual([]) + expect(shippingMethodTaxLines).toEqual([]) + expect(shippingMethodAdjustments).toEqual([]) + expect(shippingMethods).toEqual([]) + }) + + // CONTEXT: The following tests checks that deleting an order without an address does not affect other orders through cascade deletes. + // See the following advisory for more details: https://github.com/medusajs/medusa/security/advisories/GHSA-hc6q-q5gm-w585 + it("should delete an order without an address without affecting other orders", async function () { + const createdOrder = await service.retrieveOrder(order.id) + expect(createdOrder).toEqual( + expect.objectContaining({ + id: order.id, + }) + ) + + const orderWithoutAddress = await service.createOrders({ + email: "foo@bar.com", + currency_code: "usd", + customer_id: "joe", + } as CreateOrderDTO) + + await service.deleteOrders(orderWithoutAddress.id) + + const orders = await service.listOrders({}) + expect(orders.length).toEqual(1) + expect(orders[0].id).toEqual(order.id) + }) + + it("should delete an order address and set null on the order through the FK", async function () { + const createdOrder = await service.retrieveOrder(order.id) + expect(createdOrder).toEqual( + expect.objectContaining({ + id: order.id, + }) + ) + + const createdOrderWithAddresses = await service.createOrders({ + email: "foo@bar.com", + currency_code: "usd", + customer_id: "joe", + shipping_address: { + first_name: "Test", + last_name: "Test", + address_1: "Test", + city: "Test", + country_code: "US", + postal_code: "12345", + }, + billing_address: { + first_name: "Test", + last_name: "Test", + address_1: "Test", + city: "Test", + country_code: "US", + postal_code: "12345", + }, + } as CreateOrderDTO) + + const orderWithAddresses = await service.retrieveOrder( + createdOrderWithAddresses.id, + { relations: ["shipping_address", "billing_address"] } + ) + + await service.deleteOrderAddresses([ + orderWithAddresses.shipping_address?.id!, + orderWithAddresses.billing_address?.id!, + ]) + + const retrievedOrder = await service.retrieveOrder( + createdOrderWithAddresses.id, + { relations: ["shipping_address", "billing_address"] } + ) + + expect(retrievedOrder.shipping_address).toEqual(null) + expect(retrievedOrder.billing_address).toEqual(null) + }) + }) + }, +}) diff --git a/packages/modules/order/src/migrations/Migration20251225120947.ts b/packages/modules/order/src/migrations/Migration20251225120947.ts new file mode 100644 index 0000000000..ff86601416 --- /dev/null +++ b/packages/modules/order/src/migrations/Migration20251225120947.ts @@ -0,0 +1,51 @@ +import { Migration } from "@medusajs/framework/mikro-orm/migrations" + +export class Migration20251225120947 extends Migration { + override async up(): Promise { + this.addSql(` + ALTER TABLE "order" + DROP CONSTRAINT IF EXISTS "order_shipping_address_id_foreign"; + + ALTER TABLE "order" + ADD CONSTRAINT "order_shipping_address_id_foreign" + FOREIGN KEY ("shipping_address_id") + REFERENCES "order_address" ("id") + ON UPDATE CASCADE + ON DELETE SET NULL; + + ALTER TABLE "order" + DROP CONSTRAINT IF EXISTS "order_billing_address_id_foreign"; + + ALTER TABLE "order" + ADD CONSTRAINT "order_billing_address_id_foreign" + FOREIGN KEY ("billing_address_id") + REFERENCES "order_address" ("id") + ON UPDATE CASCADE + ON DELETE SET NULL; + `) + } + + override async down(): Promise { + this.addSql(` + ALTER TABLE "order" + DROP CONSTRAINT IF EXISTS "order_shipping_address_id_foreign"; + + ALTER TABLE "order" + ADD CONSTRAINT "order_shipping_address_id_foreign" + FOREIGN KEY ("shipping_address_id") + REFERENCES "order_address" ("id") + ON UPDATE CASCADE + ON DELETE CASCADE; + + ALTER TABLE "order" + DROP CONSTRAINT IF EXISTS "order_billing_address_id_foreign"; + + ALTER TABLE "order" + ADD CONSTRAINT "order_billing_address_id_foreign" + FOREIGN KEY ("billing_address_id") + REFERENCES "order_address" ("id") + ON UPDATE CASCADE + ON DELETE CASCADE; + `) + } +} diff --git a/packages/modules/order/src/services/order-module-service.ts b/packages/modules/order/src/services/order-module-service.ts index 8e5fd92849..d9d0484c7b 100644 --- a/packages/modules/order/src/services/order-module-service.ts +++ b/packages/modules/order/src/services/order-module-service.ts @@ -904,6 +904,7 @@ export default class OrderModuleService const orderAddressIds = orders .map((order) => [order.shipping_address_id, order.billing_address_id]) .flat(1) + .filter(Boolean) const orderChanges = await this.orderChangeService_.list( { order_id: ids }, @@ -931,8 +932,19 @@ export default class OrderModuleService (orderShipping) => orderShipping.shipping_method_id ) - await this.orderAddressService_.delete(orderAddressIds, sharedContext) - await this.orderChangeService_.delete(orderChangeIds, sharedContext) + const deletions: Promise[] = [] + + if (orderAddressIds.length) { + deletions.push(this.orderAddressService_.delete(orderAddressIds, sharedContext)) + } + + if (orderChangeIds.length) { + deletions.push(this.orderChangeService_.delete(orderChangeIds, sharedContext)) + } + + if (deletions.length) { + await promiseAll(deletions) + } // Delete order, order items, summary, shipping methods, transactions and credit lines await super.deleteOrders(ids, sharedContext)