chore: Ensure isolated order deletion (#14405)

## 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
> 
> <sup>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).</sup>
This commit is contained in:
Oli Juhl
2026-01-02 11:16:29 +01:00
committed by GitHub
parent 001923da2b
commit 5f807ee657
4 changed files with 295 additions and 2 deletions

View File

@@ -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<IOrderModuleService>({
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)
})
})
},
})

View File

@@ -0,0 +1,51 @@
import { Migration } from "@medusajs/framework/mikro-orm/migrations"
export class Migration20251225120947 extends Migration {
override async up(): Promise<void> {
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<void> {
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;
`)
}
}

View File

@@ -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<string[]>[] = []
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)