fix: two CRM end-to-end runtime gaps (delete cascade + dead convert route)#1984
Merged
Conversation
…oute)
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>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 15 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by driving the
app-crmexample as a real user end-to-end (boot → sign-up/session → CRUD → record clone → aggregation → domain actions → RBAC → console UI). The core path is solid (auth, CRUD, clone, aggregation, audit stamping, metadata, console all work); these two are the runtime gaps that surfaced.1. Delete of a parent with a required-FK child failed with a misleading error (data layer)
Deleting a
crm_accountthat hascrm_opportunitychildren returned:…naming
account, a field that isn't even oncrm_account. Root cause:cascadeDeleteRelationsdefaults alookupFK toset_null; for the requiredcrm_opportunity.accountFK it issued an UPDATE clearing the column, which the child's required-field validator rejected. A required FK can't be nulled — that's a contradiction, not the author's intent.Fix: a defaulted
set_nullon a required FK now escalates torestrict(SQL's standard for a NOT NULL FK):Explicit
cascade/restrictand optional (nullable) lookups are unchanged. Newengine-cascade-delete.test.ts(4 cases: required→restrict, no-dependents→ok, optional→set_null, explicit cascade→removed).2. Removed the hardcoded
POST /data/lead/:id/convertendpoint +convertLead(false surface)The route and
convertLeadbaked bare object names (lead/account/contact/opportunity) and a fixed Salesforce field mapping into the framework runtime. So it's unreachable by any real app:/data/crm_lead/:id/convert→ 404, and the literalleadobject doesn't exist. The CRM models conversion correctly as a flow (crm_convert_lead_wizardscreen flow) — a CRM-specific workflow doesn't belong hardcoded in the framework. Untested, undocumented, no consumers. Removed (narrow-and-true).Verification
409clear message; childless delete →200; record clone (same data-action group) unaffected →201; convert route gone →404.@objectstack/objectql637 (+4 cascade tests),@objectstack/rest121 green.Not fixed here — needs a product call (Gap 3)
A third finding is a deliberate posture, not a bug, so I did not flip it unilaterally: a freshly-authenticated user who resolves to zero permission sets has object-permission enforcement skipped (
security-plugin.ts:325if (permissionSets.length > 0)), i.e. fail-open when unconfigured. This lets apps with no security metadata "just work," but means once an app defines permission sets without assigning them to every user, unassigned users get full read/create instead of none. Flipping to default-deny would break every app that doesn't assign permission sets to all users (showcase, todo, the CRM itself). Tracking separately for a design decision (default-deny-when-object-has-any-permission-set, a default authenticated permission set, or an explicit secure-by-default flag).🤖 Generated with Claude Code