Skip to content

ObjectQL.delete's single-id cascade is not transactional — a refusal mid-cascade leaves earlier children deleted while the response says the delete failed #7413

Description

@baozhoutao

Summary

ObjectQL.delete() runs its cascade outside any transaction. cascadeDeleteRelations is a
plain for loop that recursively calls this.delete() / this.update(), and every one of those
calls commits on its own. So when the cascade is refused partway through — by a permission check,
by the engine's own restrict branch, or by an app beforeDelete/beforeUpdate hook on a later
child — the rows already deleted in earlier iterations stay deleted, while the request returns
a failure status and the parent row is left untouched.

The caller sees 409 (or 403) and reasonably concludes "nothing happened". In fact an arbitrary
prefix of the child set is gone, with no way to reconstruct it: a partial delete has no natural undo.

This is the same class of defect as #4620 (deleteManyData was fake-atomic), whose changeset
many-data-atomic-real-or-refused.md set the principle explicitly:

atomic — honoured for real, or refused. … a partial delete has no natural undo — a client
cannot reconstruct the rows from its own request.

ObjectQL.delete's single-id cascade path never got that treatment.

Where

packages/objectql (observed in @objectstack/objectql@17.0.0-rc.5, dist/core.js).

delete() — no transaction() / beginTransaction anywhere in the path:

async delete(object, options) {
  ...
  await this.executeWithMiddleware(opCtx, async () => {
    ...
    await this.triggerHooks("beforeDelete", hookContext);
    ...
    if (hookContext.input.id) {
      await this.cascadeDeleteRelations(object, hookContext.input.id, opCtx.context);
      result = await driver.delete(object, hookContext.input.id, hookContext.input.options);
    }
    ...

cascadeDeleteRelations() — a loop of independent, already-committed writes:

async cascadeDeleteRelations(object, id, context, depth = 0) {
  ...
  for (const child of objects) {
    for (const [fieldName, fdef] of Object.entries(fields)) {
      ...
      let behavior = fdef.type === "master_detail"
        ? (fdef.deleteBehavior === "restrict" ? "restrict" : "cascade")
        : (fdef.deleteBehavior || "set_null");
      if (behavior === "set_null" && fdef.required === true) behavior = "restrict";
      ...
      if (behavior === "restrict") { /* throws DELETE_RESTRICTED 409 */ }
      for (const dep of dependents) {
        if (behavior === "cascade") {
          await this.delete(childName, { where: { id: depId }, context });   // ← commits immediately
        } else {
          await this.update(childName, { id: depId, [fieldName]: null }, { context: referentialCtx });
        }
      }
    }
  }
}

Three properties combine into the defect:

  1. no transaction wraps the parent delete + its cascade;
  2. children are visited in getAllObjects() order, so which children are already gone when the
    failure lands is essentially arbitrary from the caller's point of view;
  3. the recursion re-enters delete(), which re-runs the full pipeline for each child —
    permission checks, beforeDelete hooks, nested cascade — i.e. plenty of places to fail late.

Reproduction (observed)

Setup — a downstream business app, all names generalised:

  • app_parent — a record with several cascade children;
  • app_child_snapshotlookup(app_parent) with deleteBehavior: 'cascade', 3 rows for the
    parent under test;
  • app_child_linklookup(app_parent) with the default behaviour (set_null), 1 row,
    carrying an app beforeUpdate hook that throws Object.assign(new Error(...), { status: 409 })
    when the parent it points at is still alive.

Steps:

  1. DELETE /api/v1/data/app_parent/<id> as a principal with full delete rights (reproduced with a
    platform-administrator account, so no app-level authorisation is in play);
  2. the engine cascades into app_child_snapshot first and deletes all 3 rows — committed;
  3. it then reaches app_child_link, issues the set_null update, and the app hook throws 409;
  4. the error propagates out of delete().

Observed, stable across repeated runs (each measured with a direct SQL count against the store,
not via the API):

assertion before after
HTTP status 409 (failure)
app_parent row exists yes yes — untouched
app_child_snapshot rows 3 0 — silently destroyed
app_child_link rows 1 1 — untouched

So the request reports failure and leaves the parent in place, while 3 child records that were
never individually addressed are permanently gone.

The source of the mid-cascade refusal is incidental — the same shape is reachable with no app
code at all: give the parent one cascade child and one required lookup child (the latter is
coerced to restrict by the line quoted above). The engine deletes the cascade child's rows, then
its own restrict branch throws DELETE_RESTRICTED 409 for the required one. Nothing outside
packages/objectql is needed to reach the inconsistent state. (Stated from the source; the table
above is the variant I actually measured.)

Expected

Either of the two, matching the principle #4620 already established for the batch path:

  1. Atomic for real — wrap the parent delete and its whole cascade in engine.transaction(), so
    a refusal anywhere rolls the entire thing back and "409" honestly means "nothing changed"; or
  2. Refused up front — if the default driver cannot provide a transaction (the documented caveat
    on ObjectQL.transaction() in ADR-0119), resolve the full cascade set and pre-flight every
    refusal source before the first destructive write, so the operation either fails having
    changed nothing, or proceeds knowing it can finish.

Silently best-effort is the one option that should not remain, because the failure is invisible:
the response says the delete did not happen.

Prior art / why this is not a duplicate

Searched before filing: cascade transaction, cascade delete atomic, cascade rollback,
delete rollback transaction, cascadeDeleteRelations, cascade delete, deleteBehavior
(full-text), plus cascade / atomic / transaction / rollback title-scoped, plus a full scan
of the ~1000 filenames in .changeset/. No existing issue covers single-id cascade atomicity.

Impact

Any app whose objects use deleteBehavior: 'cascade' and whose children can refuse a delete —
which includes the ordinary case of per-object permissions differing between parent and child,
since the recursive this.delete() re-authorises every child independently. A failed delete
becomes silent data loss, and it is invisible in exactly the situation where the operator has been
told the operation did not go through.

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions