Skip to content

Commit 44c5348

Browse files
os-zhuangclaude
andauthored
fix: two CRM end-to-end runtime gaps (delete cascade + dead convert route) (#1984)
Found by driving the app-crm example as a real user (auth → CRUD → actions). 1. Delete of a parent with a REQUIRED-FK child failed with a misleading validation error. cascadeDeleteRelations defaulted a lookup FK to set_null; for a required FK that UPDATE-cleared the column, which the child validator rejected with `400 "<field> is required"` — naming a field not even on the object being deleted (delete crm_account w/ opportunities → "account is required"). A required FK can't be nulled, so a *defaulted* set_null now escalates to restrict: refuse with a clear `409 DELETE_RESTRICTED` { dependentObject, dependentCount } and an actionable message. Explicit cascade/restrict and nullable lookups are untouched. New engine-cascade-delete.test.ts (4 cases). 2. Removed the hardcoded `POST /data/lead/:id/convert` route + convertLead protocol method. It baked bare object names (lead/account/contact/ opportunity) + a fixed field mapping into the framework runtime, so it was unreachable for any namespaced app and unused by the CRM (which models conversion as a `crm_convert_lead_wizard` screen flow). False surface — removed. Untested/undocumented; no consumers. Verified live against a running CRM: delete-with-children → 409 w/ clear message; childless delete → 200; clone (same data-action group) unaffected; convert route gone. objectql 637, rest 121 green. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 6bec07e commit 44c5348

5 files changed

Lines changed: 201 additions & 225 deletions

File tree

.changeset/crm-e2e-gaps.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
"@objectstack/objectql": minor
3+
"@objectstack/rest": minor
4+
---
5+
6+
fix: two runtime gaps found by driving the CRM example end-to-end.
7+
8+
**Delete of a parent with a required-FK child no longer fails with a misleading "<field> is required" error.** `cascadeDeleteRelations` defaulted a `lookup` FK to `set_null`; for a *required* FK that issued an UPDATE clearing the column, which the child's validator rejected with a `400 "<field> is required"` naming a field that isn't even on the object being deleted (e.g. deleting a `crm_account` with opportunities → `"account is required"`). A required FK can't be nulled, so a *defaulted* `set_null` now escalates to `restrict`: the delete is refused with a clear `409 DELETE_RESTRICTED` carrying the dependent object + count (`"Cannot delete crm_account (…): 4 dependent crm_opportunity record(s) reference it via account … set deleteBehavior:'cascade'"`). Explicit `cascade`/`restrict` and optional (nullable) lookups are unchanged.
9+
10+
**Removed the hardcoded `POST /data/lead/:id/convert` endpoint + `convertLead` protocol method.** It hardcoded bare object names (`lead`/`account`/`contact`/`opportunity`) and a fixed Salesforce field mapping into the framework runtime, so it was unreachable by any real (namespaced) app — `/data/crm_lead/:id/convert` 404s, and the literal `lead` object doesn't exist. Lead conversion is an app concern modeled correctly as a flow (the CRM ships a `crm_convert_lead_wizard` screen flow); baking a CRM-specific workflow into the framework was false surface. Untested, undocumented, unused by the example. Removed.
Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Cascade-on-delete behavior for parent→child foreign keys, with a REAL
5+
* {@link ObjectQL} engine + in-memory driver.
6+
*
7+
* Regression: deleting a parent whose child has a *required* lookup FK used to
8+
* default to `set_null`, issuing an UPDATE that cleared the required FK — which
9+
* the child's validator rejected with a misleading "<field> is required" 400
10+
* naming a field that isn't even on the object being deleted (CRM e2e gap).
11+
* A required FK can't be nulled, so the defaulted `set_null` now escalates to
12+
* `restrict`: the delete is refused with a clear dependent-count message
13+
* (`DELETE_RESTRICTED`, 409). Explicit `cascade`/`restrict` and optional
14+
* (nullable) lookups are unaffected.
15+
*/
16+
17+
import { describe, it, expect, beforeEach } from 'vitest';
18+
import { ObjectQL } from './engine.js';
19+
20+
const acct = {
21+
name: 'acct',
22+
label: 'Account',
23+
fields: {
24+
id: { name: 'id', type: 'text' as const, primaryKey: true },
25+
name: { name: 'name', type: 'text' as const },
26+
},
27+
};
28+
const oppRequired = {
29+
name: 'opp',
30+
label: 'Opportunity',
31+
fields: {
32+
id: { name: 'id', type: 'text' as const, primaryKey: true },
33+
name: { name: 'name', type: 'text' as const },
34+
// required lookup → can't be nulled
35+
account: { name: 'account', type: 'lookup' as const, reference: 'acct', required: true },
36+
},
37+
};
38+
const noteOptional = {
39+
name: 'note',
40+
label: 'Note',
41+
fields: {
42+
id: { name: 'id', type: 'text' as const, primaryKey: true },
43+
body: { name: 'body', type: 'text' as const },
44+
// optional lookup → default set_null is valid
45+
account: { name: 'account', type: 'lookup' as const, reference: 'acct' },
46+
},
47+
};
48+
const taskCascade = {
49+
name: 'task',
50+
label: 'Task',
51+
fields: {
52+
id: { name: 'id', type: 'text' as const, primaryKey: true },
53+
title: { name: 'title', type: 'text' as const },
54+
// required FK but author explicitly opted into cascade
55+
account: { name: 'account', type: 'lookup' as const, reference: 'acct', required: true, deleteBehavior: 'cascade' },
56+
},
57+
};
58+
59+
function makeMemoryDriver() {
60+
const stores = new Map<string, Map<string, Record<string, unknown>>>();
61+
const storeFor = (o: string) => { let s = stores.get(o); if (!s) { s = new Map(); stores.set(o, s); } return s; };
62+
let nextId = 0;
63+
const matches = (row: Record<string, unknown>, where: any): boolean => {
64+
if (!where || typeof where !== 'object') return true;
65+
for (const [k, v] of Object.entries(where)) {
66+
if (k.startsWith('$')) continue;
67+
const exp = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v;
68+
if ((row[k] ?? null) !== (exp ?? null)) return false;
69+
}
70+
return true;
71+
};
72+
const driver: any = {
73+
name: 'memory', version: '0.0.0', supports: {},
74+
async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
75+
async find(o: string, ast: any) { return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); },
76+
findStream() { throw new Error('ns'); },
77+
async findOne(o: string, ast: any) { for (const r of storeFor(o).values()) if (matches(r, ast?.where)) return r; return null; },
78+
async create(o: string, data: Record<string, unknown>) {
79+
nextId += 1; const id = (data.id as string) ?? `r_${nextId}`; const row = { ...data, id }; storeFor(o).set(id, row); return row;
80+
},
81+
async update(o: string, id: string, data: Record<string, unknown>) {
82+
const s = storeFor(o); const cur = s.get(id); if (!cur) throw new Error(`nf ${o}/${id}`);
83+
const up = { ...cur, ...data, id }; s.set(id, up); return up;
84+
},
85+
async upsert(o: string, data: Record<string, unknown>) { const id = data.id as string | undefined; return id && storeFor(o).has(id) ? this.update(o, id, data) : this.create(o, data); },
86+
async delete(o: string, id: string) { return storeFor(o).delete(id); },
87+
async count(o: string, ast: any) { return (await this.find(o, ast)).length; },
88+
async bulkCreate(o: string, rows: Record<string, unknown>[]) { return Promise.all(rows.map((r) => this.create(o, r))); },
89+
async bulkUpdate() { return []; }, async bulkDelete() {},
90+
async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, async commit() {}, async rollback() {},
91+
};
92+
return { driver, stores };
93+
}
94+
95+
describe('cascadeDeleteRelations — required FK escalates set_null → restrict', () => {
96+
let engine: ObjectQL;
97+
98+
beforeEach(async () => {
99+
engine = new ObjectQL();
100+
const { driver } = makeMemoryDriver();
101+
engine.registerDriver(driver, true);
102+
await engine.init();
103+
for (const o of [acct, oppRequired, noteOptional, taskCascade]) engine.registry.registerObject(o as any);
104+
});
105+
106+
it('refuses to delete a parent with a REQUIRED-FK child (DELETE_RESTRICTED, 409) and leaves both rows', async () => {
107+
const a = await engine.insert('acct', { name: 'Acme' });
108+
await engine.insert('opp', { name: 'Deal', account: a.id });
109+
110+
await expect(engine.delete('acct', { where: { id: a.id } } as any))
111+
.rejects.toMatchObject({ code: 'DELETE_RESTRICTED', status: 409, dependentObject: 'opp', dependentCount: 1 });
112+
113+
// Nothing was deleted or mutated.
114+
expect(await engine.findOne('acct', { where: { id: a.id } })).toBeTruthy();
115+
expect((await engine.find('opp', {})).length).toBe(1);
116+
});
117+
118+
it('deletes a parent that has no dependents', async () => {
119+
const a = await engine.insert('acct', { name: 'Empty' });
120+
await engine.delete('acct', { where: { id: a.id } } as any);
121+
expect(await engine.findOne('acct', { where: { id: a.id } })).toBeNull();
122+
});
123+
124+
it('nulls the FK for an OPTIONAL (nullable) lookup child and deletes the parent', async () => {
125+
const a = await engine.insert('acct', { name: 'Acme' });
126+
const n = await engine.insert('note', { body: 'hi', account: a.id });
127+
await engine.delete('acct', { where: { id: a.id } } as any);
128+
expect(await engine.findOne('acct', { where: { id: a.id } })).toBeNull();
129+
const note = await engine.findOne('note', { where: { id: n.id } });
130+
expect(note).toBeTruthy();
131+
expect((note as any).account).toBeNull();
132+
});
133+
134+
it('honors an explicit deleteBehavior:cascade on a required FK (children removed, no escalation)', async () => {
135+
const a = await engine.insert('acct', { name: 'Acme' });
136+
const t = await engine.insert('task', { title: 'Follow up', account: a.id });
137+
await engine.delete('acct', { where: { id: a.id } } as any);
138+
expect(await engine.findOne('acct', { where: { id: a.id } })).toBeNull();
139+
expect(await engine.findOne('task', { where: { id: t.id } })).toBeNull();
140+
});
141+
});

packages/objectql/src/engine.ts

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2210,7 +2210,10 @@ export class ObjectQL implements IDataEngine {
22102210
* - `set_null` → clear the foreign key,
22112211
* - `restrict` → refuse the delete when dependents exist.
22122212
* `master_detail` defaults to `cascade` (the parent owns the child
2213-
* lifecycle); `lookup` defaults to `set_null`. Only runs for single-id
2213+
* lifecycle); `lookup` defaults to `set_null` — except a `set_null` default
2214+
* on a REQUIRED lookup escalates to `restrict` (you can't null a NOT NULL
2215+
* FK; restricting with a clear dependent-count message beats a misleading
2216+
* "<field> is required" 400 from the child). Only runs for single-id
22142217
* deletes — multi/predicate deletes skip cascade (logged).
22152218
*/
22162219
private async cascadeDeleteRelations(
@@ -2243,11 +2246,24 @@ export class ObjectQL implements IDataEngine {
22432246
// child FK is typically required, so set_null would be invalid). Only
22442247
// an explicit `restrict` deviates. A plain lookup honors its
22452248
// configured deleteBehavior (default set_null).
2246-
const behavior: string =
2249+
let behavior: string =
22472250
fdef.type === 'master_detail'
22482251
? (fdef.deleteBehavior === 'restrict' ? 'restrict' : 'cascade')
22492252
: (fdef.deleteBehavior || 'set_null');
22502253

2254+
// A REQUIRED foreign key cannot be nulled — set_null would issue an
2255+
// UPDATE clearing the FK, which the child's required-field validator
2256+
// rejects with a misleading "<field> is required" 400 (the field isn't
2257+
// even on the object being deleted). That's a contradiction, not the
2258+
// author's intent: a required FK means the child can't exist detached,
2259+
// so deleting the parent must be RESTRICTed (SQL's default for a
2260+
// NOT NULL FK). Authors who want the children gone set
2261+
// deleteBehavior:'cascade' explicitly. This only escalates the
2262+
// *defaulted* set_null; an explicit cascade/restrict is untouched.
2263+
if (behavior === 'set_null' && fdef.required === true) {
2264+
behavior = 'restrict';
2265+
}
2266+
22512267
let dependents: any[];
22522268
try {
22532269
dependents = await this.find(childName, { where: { [fieldName]: id }, context } as any);
@@ -2257,9 +2273,19 @@ export class ObjectQL implements IDataEngine {
22572273
if (!dependents || dependents.length === 0) continue;
22582274

22592275
if (behavior === 'restrict') {
2260-
throw new Error(
2261-
`Cannot delete ${object} (${id}): ${dependents.length} dependent ${childName} record(s) via ${fieldName}`,
2276+
const reason = fdef.deleteBehavior !== 'restrict' && fdef.required === true
2277+
? ` (${fieldName} is required, so it cannot be cleared)`
2278+
: '';
2279+
const err: any = new Error(
2280+
`Cannot delete ${object} (${id}): ${dependents.length} dependent ${childName} record(s) reference it via ${fieldName}${reason}. ` +
2281+
`Delete or reassign them first, or set deleteBehavior:'cascade' on ${childName}.${fieldName}.`,
22622282
);
2283+
err.code = 'DELETE_RESTRICTED';
2284+
err.status = 409;
2285+
err.object = object;
2286+
err.dependentObject = childName;
2287+
err.dependentCount = dependents.length;
2288+
throw err;
22632289
}
22642290

22652291
for (const dep of dependents) {

0 commit comments

Comments
 (0)