diff --git a/.changeset/crm-e2e-gaps.md b/.changeset/crm-e2e-gaps.md new file mode 100644 index 0000000000..d7415e33e0 --- /dev/null +++ b/.changeset/crm-e2e-gaps.md @@ -0,0 +1,10 @@ +--- +"@objectstack/objectql": minor +"@objectstack/rest": minor +--- + +fix: two runtime gaps found by driving the CRM example end-to-end. + +**Delete of a parent with a required-FK child no longer fails with a misleading " 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 " 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. + +**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. diff --git a/packages/objectql/src/engine-cascade-delete.test.ts b/packages/objectql/src/engine-cascade-delete.test.ts new file mode 100644 index 0000000000..2699651704 --- /dev/null +++ b/packages/objectql/src/engine-cascade-delete.test.ts @@ -0,0 +1,141 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Cascade-on-delete behavior for parent→child foreign keys, with a REAL + * {@link ObjectQL} engine + in-memory driver. + * + * Regression: deleting a parent whose child has a *required* lookup FK used to + * default to `set_null`, issuing an UPDATE that cleared the required FK — which + * the child's validator rejected with a misleading " is required" 400 + * naming a field that isn't even on the object being deleted (CRM e2e gap). + * A required FK can't be nulled, so the defaulted `set_null` now escalates to + * `restrict`: the delete is refused with a clear dependent-count message + * (`DELETE_RESTRICTED`, 409). Explicit `cascade`/`restrict` and optional + * (nullable) lookups are unaffected. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; + +const acct = { + name: 'acct', + label: 'Account', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + }, +}; +const oppRequired = { + name: 'opp', + label: 'Opportunity', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + // required lookup → can't be nulled + account: { name: 'account', type: 'lookup' as const, reference: 'acct', required: true }, + }, +}; +const noteOptional = { + name: 'note', + label: 'Note', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + body: { name: 'body', type: 'text' as const }, + // optional lookup → default set_null is valid + account: { name: 'account', type: 'lookup' as const, reference: 'acct' }, + }, +}; +const taskCascade = { + name: 'task', + label: 'Task', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + title: { name: 'title', type: 'text' as const }, + // required FK but author explicitly opted into cascade + account: { name: 'account', type: 'lookup' as const, reference: 'acct', required: true, deleteBehavior: 'cascade' }, + }, +}; + +function makeMemoryDriver() { + const stores = new Map>>(); + const storeFor = (o: string) => { let s = stores.get(o); if (!s) { s = new Map(); stores.set(o, s); } return s; }; + let nextId = 0; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const exp = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (exp ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(o: string, ast: any) { return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); }, + findStream() { throw new Error('ns'); }, + async findOne(o: string, ast: any) { for (const r of storeFor(o).values()) if (matches(r, ast?.where)) return r; return null; }, + async create(o: string, data: Record) { + nextId += 1; const id = (data.id as string) ?? `r_${nextId}`; const row = { ...data, id }; storeFor(o).set(id, row); return row; + }, + async update(o: string, id: string, data: Record) { + const s = storeFor(o); const cur = s.get(id); if (!cur) throw new Error(`nf ${o}/${id}`); + const up = { ...cur, ...data, id }; s.set(id, up); return up; + }, + async upsert(o: string, data: Record) { const id = data.id as string | undefined; return id && storeFor(o).has(id) ? this.update(o, id, data) : this.create(o, data); }, + async delete(o: string, id: string) { return storeFor(o).delete(id); }, + async count(o: string, ast: any) { return (await this.find(o, ast)).length; }, + async bulkCreate(o: string, rows: Record[]) { return Promise.all(rows.map((r) => this.create(o, r))); }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, async commit() {}, async rollback() {}, + }; + return { driver, stores }; +} + +describe('cascadeDeleteRelations — required FK escalates set_null → restrict', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = new ObjectQL(); + const { driver } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + for (const o of [acct, oppRequired, noteOptional, taskCascade]) engine.registry.registerObject(o as any); + }); + + it('refuses to delete a parent with a REQUIRED-FK child (DELETE_RESTRICTED, 409) and leaves both rows', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + await engine.insert('opp', { name: 'Deal', account: a.id }); + + await expect(engine.delete('acct', { where: { id: a.id } } as any)) + .rejects.toMatchObject({ code: 'DELETE_RESTRICTED', status: 409, dependentObject: 'opp', dependentCount: 1 }); + + // Nothing was deleted or mutated. + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeTruthy(); + expect((await engine.find('opp', {})).length).toBe(1); + }); + + it('deletes a parent that has no dependents', async () => { + const a = await engine.insert('acct', { name: 'Empty' }); + await engine.delete('acct', { where: { id: a.id } } as any); + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeNull(); + }); + + it('nulls the FK for an OPTIONAL (nullable) lookup child and deletes the parent', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + const n = await engine.insert('note', { body: 'hi', account: a.id }); + await engine.delete('acct', { where: { id: a.id } } as any); + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeNull(); + const note = await engine.findOne('note', { where: { id: n.id } }); + expect(note).toBeTruthy(); + expect((note as any).account).toBeNull(); + }); + + it('honors an explicit deleteBehavior:cascade on a required FK (children removed, no escalation)', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + const t = await engine.insert('task', { title: 'Follow up', account: a.id }); + await engine.delete('acct', { where: { id: a.id } } as any); + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeNull(); + expect(await engine.findOne('task', { where: { id: t.id } })).toBeNull(); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 236212cbe2..8f54bf34af 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2210,7 +2210,10 @@ export class ObjectQL implements IDataEngine { * - `set_null` → clear the foreign key, * - `restrict` → refuse the delete when dependents exist. * `master_detail` defaults to `cascade` (the parent owns the child - * lifecycle); `lookup` defaults to `set_null`. Only runs for single-id + * lifecycle); `lookup` defaults to `set_null` — except a `set_null` default + * on a REQUIRED lookup escalates to `restrict` (you can't null a NOT NULL + * FK; restricting with a clear dependent-count message beats a misleading + * " is required" 400 from the child). Only runs for single-id * deletes — multi/predicate deletes skip cascade (logged). */ private async cascadeDeleteRelations( @@ -2243,11 +2246,24 @@ export class ObjectQL implements IDataEngine { // child FK is typically required, so set_null would be invalid). Only // an explicit `restrict` deviates. A plain lookup honors its // configured deleteBehavior (default set_null). - const behavior: string = + let behavior: string = fdef.type === 'master_detail' ? (fdef.deleteBehavior === 'restrict' ? 'restrict' : 'cascade') : (fdef.deleteBehavior || 'set_null'); + // A REQUIRED foreign key cannot be nulled — set_null would issue an + // UPDATE clearing the FK, which the child's required-field validator + // rejects with a misleading " is required" 400 (the field isn't + // even on the object being deleted). That's a contradiction, not the + // author's intent: a required FK means the child can't exist detached, + // so deleting the parent must be RESTRICTed (SQL's default for a + // NOT NULL FK). Authors who want the children gone set + // deleteBehavior:'cascade' explicitly. This only escalates the + // *defaulted* set_null; an explicit cascade/restrict is untouched. + if (behavior === 'set_null' && fdef.required === true) { + behavior = 'restrict'; + } + let dependents: any[]; try { dependents = await this.find(childName, { where: { [fieldName]: id }, context } as any); @@ -2257,9 +2273,19 @@ export class ObjectQL implements IDataEngine { if (!dependents || dependents.length === 0) continue; if (behavior === 'restrict') { - throw new Error( - `Cannot delete ${object} (${id}): ${dependents.length} dependent ${childName} record(s) via ${fieldName}`, + const reason = fdef.deleteBehavior !== 'restrict' && fdef.required === true + ? ` (${fieldName} is required, so it cannot be cleared)` + : ''; + const err: any = new Error( + `Cannot delete ${object} (${id}): ${dependents.length} dependent ${childName} record(s) reference it via ${fieldName}${reason}. ` + + `Delete or reassign them first, or set deleteBehavior:'cascade' on ${childName}.${fieldName}.`, ); + err.code = 'DELETE_RESTRICTED'; + err.status = 409; + err.object = object; + err.dependentObject = childName; + err.dependentCount = dependents.length; + throw err; } for (const dep of dependents) { diff --git a/packages/objectql/src/protocol.ts b/packages/objectql/src/protocol.ts index be8c5961ab..90611b3d29 100644 --- a/packages/objectql/src/protocol.ts +++ b/packages/objectql/src/protocol.ts @@ -2571,186 +2571,6 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol { }; } - // ========================================== - // Lead Convert (M10.6) - // ========================================== - /** - * Convert a qualified Lead into an Account + Contact (+ optional - * Opportunity) and mark the Lead as converted. Mirrors the Salesforce - * lead-conversion model: - * - * - If `accountId` is provided, the lead's company info is NOT used - * to create a new account; the new contact and opportunity link to - * the existing account instead. - * - If `contactId` is provided, no new contact is created either — - * useful when the lead is a new contact at an existing account. - * - `createOpportunity` defaults to true; pass `false` to convert - * without producing an opportunity (some teams convert "logos - * only" first). - * - Lead is updated atomically: `is_converted=true`, - * `converted_account`/`converted_contact`/`converted_opportunity` - * pointers, `converted_date`, and `status='converted'`. - * - * Atomicity is enforced via the default driver's transaction support - * when available; otherwise a best-effort compensation (delete - * already-created child records on failure) is attempted. Permission - * checks on each child object are inherited from the caller's - * execution context so SecurityPlugin still gates account/contact/ - * opportunity creates. - */ - async convertLead(request: { - leadId: string; - accountId?: string; - contactId?: string; - createOpportunity?: boolean; - opportunity?: { - name?: string; - amount?: number; - close_date?: string; - stage?: string; - }; - convertedStatus?: string; - context?: any; - }): Promise<{ - lead: any; - account: any; - contact: any; - opportunity: any | null; - }> { - const leadId = String(request.leadId || '').trim(); - if (!leadId) { - const err: any = new Error('leadId is required'); - err.status = 400; - err.code = 'INVALID_REQUEST'; - throw err; - } - const ctx = request.context; - const ctxOpt = ctx !== undefined ? { context: ctx } : undefined; - - // Load lead - const lead = await this.engine.findOne('lead', { where: { id: leadId }, ...(ctxOpt as any) } as any); - if (!lead) { - const err: any = new Error(`Lead '${leadId}' not found`); - err.status = 404; - err.code = 'LEAD_NOT_FOUND'; - throw err; - } - if (lead.is_converted) { - const err: any = new Error(`Lead '${leadId}' is already converted`); - err.status = 409; - err.code = 'LEAD_ALREADY_CONVERTED'; - throw err; - } - - // Wrap the whole conversion in a single DB transaction so that a - // partial failure (e.g. opportunity insert fails after we've - // already created the account/contact) rolls back atomically - // instead of leaving orphan rows. Falls back to direct execution - // on drivers without transaction support — in that case the - // operations are still ordered so callers see the same partial - // state we'd get from any non-atomic sequence. - const runConversion = async (trxCtx: any) => { - const opCtx = trxCtx ?? ctx; - const trxCtxOpt = opCtx !== undefined ? { context: opCtx } : undefined; - - // 1) Account - let account: any; - if (request.accountId) { - account = await this.engine.findOne('account', { where: { id: request.accountId }, ...(trxCtxOpt as any) } as any); - if (!account) { - const err: any = new Error(`Account '${request.accountId}' not found`); - err.status = 404; - err.code = 'ACCOUNT_NOT_FOUND'; - throw err; - } - } else { - const accountPayload: Record = { - name: lead.company || `${lead.first_name ?? ''} ${lead.last_name ?? ''}`.trim() || 'Untitled Account', - }; - if (lead.industry) accountPayload.industry = lead.industry; - if (lead.annual_revenue) accountPayload.annual_revenue = lead.annual_revenue; - if (lead.number_of_employees) accountPayload.employees = lead.number_of_employees; - if (lead.website) accountPayload.website = lead.website; - if (lead.phone) accountPayload.phone = lead.phone; - if (lead.address) accountPayload.billing_address = lead.address; - if (lead.owner) accountPayload.owner = lead.owner; - account = await this.engine.insert('account', accountPayload, trxCtxOpt as any); - } - - // 2) Contact - let contact: any; - if (request.contactId) { - contact = await this.engine.findOne('contact', { where: { id: request.contactId }, ...(trxCtxOpt as any) } as any); - if (!contact) { - const err: any = new Error(`Contact '${request.contactId}' not found`); - err.status = 404; - err.code = 'CONTACT_NOT_FOUND'; - throw err; - } - } else { - const contactPayload: Record = { - first_name: lead.first_name ?? '', - last_name: lead.last_name ?? lead.company ?? 'Unknown', - }; - if (lead.salutation) contactPayload.salutation = lead.salutation; - if (lead.email) contactPayload.email = lead.email; - if (lead.phone) contactPayload.phone = lead.phone; - if (lead.mobile) contactPayload.mobile = lead.mobile; - if (lead.title) contactPayload.title = lead.title; - if (lead.address) contactPayload.mailing_address = lead.address; - if (lead.owner) contactPayload.owner = lead.owner; - if (account?.id) contactPayload.account = account.id; - contact = await this.engine.insert('contact', contactPayload, trxCtxOpt as any); - } - - // 3) Opportunity (optional) - let opportunity: any | null = null; - const shouldCreateOpp = request.createOpportunity !== false; - if (shouldCreateOpp) { - const oppOverrides = request.opportunity ?? {}; - const defaultName = oppOverrides.name - || `${account?.name ?? lead.company ?? 'Lead'} - New Opportunity`; - const defaultClose = oppOverrides.close_date - || new Date(Date.now() + 30 * 24 * 60 * 60 * 1000).toISOString().slice(0, 10); - const oppPayload: Record = { - name: defaultName, - stage: oppOverrides.stage ?? 'qualification', - close_date: defaultClose, - }; - if (oppOverrides.amount !== undefined) oppPayload.amount = oppOverrides.amount; - else if (lead.annual_revenue) oppPayload.amount = lead.annual_revenue; - if (account?.id) oppPayload.account = account.id; - if (contact?.id) oppPayload.primary_contact = contact.id; - if (lead.owner) oppPayload.owner = lead.owner; - if (lead.lead_source) oppPayload.lead_source = lead.lead_source; - opportunity = await this.engine.insert('opportunity', oppPayload, trxCtxOpt as any); - } - - // 4) Mark lead converted - const leadUpdate: Record = { - is_converted: true, - status: request.convertedStatus ?? 'converted', - converted_account: account?.id ?? null, - converted_contact: contact?.id ?? null, - converted_opportunity: opportunity?.id ?? null, - converted_date: new Date().toISOString(), - }; - const updatedLead = await this.engine.update('lead', leadUpdate, { - where: { id: leadId }, - ...(trxCtxOpt as any), - } as any); - - return { - lead: updatedLead ?? { ...lead, ...leadUpdate }, - account, - contact, - opportunity, - }; - }; - - return (this.engine as any).transaction(runConversion, ctx); - } - // ========================================== // Metadata Caching // ========================================== diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index ace53a1fb5..d5865438a5 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -48,6 +48,20 @@ function isMetaEnvelope(value: any): boolean { * returns a misleading 404. */ export function mapDataError(error: any, object?: string): { status: number; body: Record } { + // Referential-integrity restrict on delete → 409 with the dependent count. + // Surfaced FIRST so the structured fields survive the generic catch-alls. + if (error?.code === 'DELETE_RESTRICTED') { + return { + status: 409, + body: { + error: error?.message ?? 'Cannot delete: dependent records exist', + code: 'DELETE_RESTRICTED', + ...(error?.dependentObject ? { dependentObject: error.dependentObject } : {}), + ...(typeof error?.dependentCount === 'number' ? { dependentCount: error.dependentCount } : {}), + ...(object ? { object } : {}), + }, + }; + } // Optimistic-Concurrency-Control mismatch → 409 with current state. // Surfaced FIRST so the structured fields (`currentVersion`, // `currentRecord`) are preserved instead of being squashed into the @@ -2931,54 +2945,19 @@ export class RestServer { /** * Register object-specific action endpoints that don't fit the - * generic CRUD shape. These are domain operations (Salesforce - * convertLead, etc.) where the protocol implementation does its own - * multi-record orchestration and we just need a thin HTTP route. + * generic CRUD shape — domain operations where the protocol does its + * own orchestration and we just need a thin HTTP route. * - * POST {basePath}/data/lead/:id/convert — M10.6 lead conversion. + * POST {basePath}/data/:object/:id/clone — record clone (gated by + * `enable.clone`). This is object-agnostic by design: it works for any + * authored object regardless of namespace, unlike a hardcoded + * per-object route would. */ private registerDataActionEndpoints(basePath: string): void { const isScoped = basePath.includes('/environments/:environmentId'); const { crud } = this.config; const dataPath = `${basePath}${crud.dataPrefix}`; - // POST /data/lead/:id/convert - this.routeManager.register({ - method: 'POST', - path: `${dataPath}/lead/:id/convert`, - handler: async (req: any, res: any) => { - try { - const environmentId = isScoped ? req.params?.environmentId : undefined; - const p = await this.resolveProtocol(environmentId, req); - const context = await this.resolveExecCtx(environmentId, req); - if (this.enforceAuth(req, res, context)) return; - const convertLead = (p as any).convertLead; - if (typeof convertLead !== 'function') { - res.status(501).json({ code: 'NOT_IMPLEMENTED', error: 'Lead convert not supported by this protocol' }); - return; - } - const body = req.body ?? {}; - const result = await convertLead.call(p, { - leadId: req.params.id, - accountId: body.accountId, - contactId: body.contactId, - createOpportunity: body.createOpportunity, - opportunity: body.opportunity, - convertedStatus: body.convertedStatus, - ...(context ? { context } : {}), - }); - res.json(result); - } catch (error: any) { - logError('[REST] Unhandled error:', error); - sendError(res, error, 'lead'); - } - }, - metadata: { - summary: 'Convert a Lead into Account + Contact (+ optional Opportunity)', - tags: ['data', 'lead'], - }, - }); - // POST /data/:object/:id/clone — duplicate a record (gated by the // object's `enable.clone` capability, default on). Optional JSON body // `{ overrides?: {...} }` (or a bare field map) is applied on top of