Add support for restrict as an on-delete action#2313
Open
Add support for restrict as an on-delete action#2313
restrict as an on-delete action#2313Conversation
Contributor
📝 WalkthroughWalkthroughAdds explicit on-delete semantics by introducing 'restrict' alongside 'cascade' across types, UI, CLI, platform mapping, server cascade resolution, DB migrations, tests, and docs; enforces direction-aware restrict constraints during transaction delete planning and improves diff-based on-delete patching. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / UI
participant CLI as CLI Renderer
participant Platform as Platform API
participant Server as Transaction Validator
participant DB as Database
User->>CLI: Submit schema change (on-delete -> "restrict")
CLI->>CLI: Compute diff (prev vs new on-delete)
CLI->>Platform: Emit REMOVE(prev) + SET(new) attribute ops
Platform->>Server: Apply attribute ops / build LinkDef (cascade|restrict)
Server->>Server: Resolve cascade paths (use direction to pick rule)
alt Restrict violation detected
Server->>User: Return on-delete-restrict error
else Validation OK
Server->>DB: Execute deletes and apply schema updates
DB->>User: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
View Vercel preview at instant-www-js-on-delete-restrict-jsv.vercel.app. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/packages/components/src/types.ts (1)
288-289:⚠️ Potential issue | 🟡 Minor
SchemaAttrinterface not updated to include'restrict'option.The
onDeleteandonDeleteReversefields inSchemaAttrstill only allow'cascade', whileDBAttrnow uses the broaderOnDeletetype that includes'restrict'. This inconsistency may cause type errors when converting between these interfaces.🔧 Proposed fix
- onDelete?: 'cascade'; - onDeleteReverse?: 'cascade'; + onDelete?: OnDelete; + onDeleteReverse?: OnDelete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/components/src/types.ts` around lines 288 - 289, The SchemaAttr interface currently types onDelete and onDeleteReverse as only 'cascade', causing mismatch with DBAttr's OnDelete type (which includes 'restrict'); update SchemaAttr to use the same OnDelete type (or include 'restrict') for both onDelete and onDeleteReverse so conversion between SchemaAttr and DBAttr is type-safe—locate SchemaAttr and replace the literal 'cascade' union with the shared OnDelete type (or add 'restrict' to the union) to resolve the inconsistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/www/pages/docs/modeling-data.md`:
- Around line 395-400: The fenced code block containing the postAuthor object
should include a language tag to satisfy MD040; change the opening fence from
``` to ```typescript so the block starting with "postAuthor: {" (the
forward/reverse definition) is marked as TypeScript.
In `@server/src/instant/db/transaction.clj`:
- Around line 522-538: The keep-fn currently returns ent for all non-root
entities even when cascade-rule is :restrict; change it so that after the
existing restrict validation (ex/throw-validation-err! when the child is NOT
being deleted), if cascade-rule is :restrict and the child IS being deleted
(contains? original-deleted-entities [entity_id etype]) the function returns nil
(exclude the child from cascaded-entities) instead of returning ent; keep the
existing behavior of returning ent only for non-restrict cascade rules (use the
local names parent_id, entity_id, parent_etype, etype, attr_id, direction,
on-delete, cascade-rule, attrs, original-deleted-entities, res to locate and
modify the keep anonymous function).
---
Outside diff comments:
In `@client/packages/components/src/types.ts`:
- Around line 288-289: The SchemaAttr interface currently types onDelete and
onDeleteReverse as only 'cascade', causing mismatch with DBAttr's OnDelete type
(which includes 'restrict'); update SchemaAttr to use the same OnDelete type (or
include 'restrict') for both onDelete and onDeleteReverse so conversion between
SchemaAttr and DBAttr is type-safe—locate SchemaAttr and replace the literal
'cascade' union with the shared OnDelete type (or add 'restrict' to the union)
to resolve the inconsistency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)client/packages/components/src/types.ts (1)
288-289:⚠️ Potential issue | 🟠 Major
SchemaAttrtype definition does not match theOnDeletevalues being assigned to it.
SchemaAttr.onDeleteandSchemaAttr.onDeleteReverseare typed as'cascade'only, butschema.ts(lines 81-82, 108-109) directly assigns values fromDBAttr['on-delete']andDBAttr['on-delete-reverse'], which use theOnDeletetype ('cascade' | 'restrict' | null | undefined). This allows'restrict'values to bypass the type system. UpdateSchemaAttr.onDeleteandSchemaAttr.onDeleteReverseto use theOnDeletetype for consistency:onDelete?: OnDelete; onDeleteReverse?: OnDelete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/components/src/types.ts` around lines 288 - 289, SchemaAttr's onDelete and onDeleteReverse are currently typed too narrowly as 'cascade' causing values like 'restrict' from DBAttr['on-delete'] / DBAttr['on-delete-reverse'] to bypass the type system; update the SchemaAttr type by changing the onDelete and onDeleteReverse properties to use the OnDelete type (the same union used by DBAttr) so SchemaAttr.onDelete and SchemaAttr.onDeleteReverse accept 'cascade' | 'restrict' | null | undefined and keep types consistent with DBAttr and the code in schema.ts that assigns those values.
♻️ Duplicate comments (1)
client/www/pages/docs/modeling-data.md (1)
395-400: Add a language tag to the fenced code block.The code block is missing a language specifier, which triggers MD040 linting warnings.
Suggested fix
-``` +```typescript postAuthor: { forward: { on: "profiles", has: "many", label: "authoredPosts" }, reverse: { on: "posts", has: "one", label: "author", onDelete: "restrict" }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/www/pages/docs/modeling-data.md` around lines 395 - 400, The fenced code block containing the postAuthor relation example is missing a language tag which triggers MD040; update the block to include a language specifier (e.g., add "typescript" after the opening triple backticks) for the block that shows postAuthor with forward and reverse definitions (forward, reverse, label, onDelete) so the linter recognizes the language.
🧹 Nitpick comments (2)
client/packages/platform/src/api.ts (1)
422-438: LGTM!The conversion logic correctly handles both
'cascade'and'restrict'values for forward and reverseonDeletefields.💡 Optional: Extract a helper to reduce repetition
function mapOnDelete(value: string | null | undefined): 'cascade' | 'restrict' | undefined { if (value === 'cascade') return 'cascade'; if (value === 'restrict') return 'restrict'; return undefined; } // Then use: onDelete: mapOnDelete(attr['on-delete']), // ... onDelete: mapOnDelete(attr['on-delete-reverse']),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/platform/src/api.ts` around lines 422 - 438, The onDelete mapping logic is duplicated for forward and reverse fields; define a helper function mapOnDelete(value: string | null | undefined): 'cascade' | 'restrict' | undefined that returns 'cascade' for "cascade", 'restrict' for "restrict", otherwise undefined, and replace the inline ternaries in the onDelete assignments (the ones using attr['on-delete'] and attr['on-delete-reverse']) with calls to mapOnDelete(attr['on-delete']) and mapOnDelete(attr['on-delete-reverse']) respectively to remove repetition and centralize the mapping logic.server/src/instant/db/transaction.clj (1)
530-545: Redundant condition and inconsistent namespace alias.Two minor issues:
Redundant check (line 532): The condition
(= cascade-rule :restrict)is always true inside the:restrictcase branch and can be removed.Inconsistent alias (line 537): The file imports
[clojure.string :as string]but uses the fully-qualifiedclojure.string/joinhere instead ofstring/join.♻️ Proposed cleanup
(case cascade-rule :restrict - (when (and (= cascade-rule :restrict) - (not (contains? original-deleted-entities [entity_id etype]))) + (when-not (contains? original-deleted-entities [entity_id etype]) (ex/throw-validation-err! :on-delete-restrict (map vectorize-tx-step tx-step-maps) - [{:message (format (clojure.string/join + [{:message (format (string/join " " ["This transaction violates an on-delete constraint." "The `%s` entity cannot be deleted unless its" - "linked `%s` entity is deleted first."] ) + "linked `%s` entity is deleted first."]) parent_etype etype)}])) `#_else` ent))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/transaction.clj` around lines 530 - 545, Inside the :restrict branch of the case for cascade-rule, remove the redundant check (= cascade-rule :restrict) from the when condition (it’s always true in that branch) and replace the fully-qualified clojure.string/join call with the aliased string/join to match the file's import; the change affects the when that calls ex/throw-validation-err! (referencing vectorize-tx-step, tx-step-maps, parent_etype, etype) so ensure those symbols remain unchanged while simplifying the condition and using string/join.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx`:
- Around line 1014-1017: In the EditNamespaceDialog JSX where the reverse
cascade label renders using namespaceName and reverseNamespaceName, change the
text fragment "must be deleted" to "will be deleted automatically" so the
sentence reads "When a <strong>{namespaceName}</strong> entity is deleted, all
linked <strong>{reverseNamespaceName}</strong> will be deleted automatically" to
match the forward cascade label; locate the string in edit-namespace-dialog.tsx
near the JSX that references namespaceName and reverseNamespaceName and update
it accordingly.
In `@server/src/instant/db/transaction.clj`:
- Around line 551-555: Remove the unused :cascade-attr field from the generated
cascaded delete steps: in the code that builds delete steps from
cascaded-entities (the for over cascaded-entities producing maps with :op
:delete-entity, :eid, :etype, :cascade-attr), drop the :cascade-attr entry since
vectorize-tx-step only destructures :op, :eid and :etype for :delete-entity and
the field is unused elsewhere; if you intend to keep it for future use instead
of removing it, add a short comment next to the map construction explaining its
intended purpose and why it’s not consumed today.
---
Outside diff comments:
In `@client/packages/components/src/types.ts`:
- Around line 288-289: SchemaAttr's onDelete and onDeleteReverse are currently
typed too narrowly as 'cascade' causing values like 'restrict' from
DBAttr['on-delete'] / DBAttr['on-delete-reverse'] to bypass the type system;
update the SchemaAttr type by changing the onDelete and onDeleteReverse
properties to use the OnDelete type (the same union used by DBAttr) so
SchemaAttr.onDelete and SchemaAttr.onDeleteReverse accept 'cascade' | 'restrict'
| null | undefined and keep types consistent with DBAttr and the code in
schema.ts that assigns those values.
---
Duplicate comments:
In `@client/www/pages/docs/modeling-data.md`:
- Around line 395-400: The fenced code block containing the postAuthor relation
example is missing a language tag which triggers MD040; update the block to
include a language specifier (e.g., add "typescript" after the opening triple
backticks) for the block that shows postAuthor with forward and reverse
definitions (forward, reverse, label, onDelete) so the linter recognizes the
language.
---
Nitpick comments:
In `@client/packages/platform/src/api.ts`:
- Around line 422-438: The onDelete mapping logic is duplicated for forward and
reverse fields; define a helper function mapOnDelete(value: string | null |
undefined): 'cascade' | 'restrict' | undefined that returns 'cascade' for
"cascade", 'restrict' for "restrict", otherwise undefined, and replace the
inline ternaries in the onDelete assignments (the ones using attr['on-delete']
and attr['on-delete-reverse']) with calls to mapOnDelete(attr['on-delete']) and
mapOnDelete(attr['on-delete-reverse']) respectively to remove repetition and
centralize the mapping logic.
In `@server/src/instant/db/transaction.clj`:
- Around line 530-545: Inside the :restrict branch of the case for cascade-rule,
remove the redundant check (= cascade-rule :restrict) from the when condition
(it’s always true in that branch) and replace the fully-qualified
clojure.string/join call with the aliased string/join to match the file's
import; the change affects the when that calls ex/throw-validation-err!
(referencing vectorize-tx-step, tx-step-maps, parent_etype, etype) so ensure
those symbols remain unchanged while simplifying the condition and using
string/join.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
client/packages/cli/src/renderSchemaPlan.tsclient/packages/components/src/components/explorer/edit-namespace-dialog.tsxclient/packages/components/src/types.tsclient/packages/core/src/attrTypes.tsclient/packages/core/src/schemaTypes.tsclient/packages/platform/__tests__/src/migrations.test.tsclient/packages/platform/src/api.tsclient/packages/platform/src/schema.tsclient/www/pages/docs/modeling-data.mdserver/resources/migrations/100_on-delete-restrict.down.sqlserver/resources/migrations/100_on-delete-restrict.up.sqlserver/src/instant/config_edn.cljserver/src/instant/db/transaction.cljserver/src/instant/model/schema.cljserver/src/instant/util/test.cljserver/test/instant/db/transaction_test.clj
client/packages/components/src/components/explorer/edit-namespace-dialog.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/packages/components/src/types.ts (1)
288-289:⚠️ Potential issue | 🟠 Major
SchemaAttr.onDelete/onDeleteReversenot updated toOnDeletetype — TypeScript error in schema transformation code
DBAttr['on-delete']is typed asOnDelete(lines 240–241), which includes'restrict', butSchemaAttr.onDeleteandSchemaAttr.onDeleteReverse(lines 288–289) are still restricted to'cascade'only. This causes a type error inschema.ts(lines 81–82, 108–109) whereattrDesc['on-delete']is directly assigned toschemaAttr.onDelete, since'restrict' | null | undefinedcannot be assigned to a field typed as'cascade'. The edit dialog explicitly handles'restrict'values (edit-namespace-dialog.tsx lines 982, 1029), confirming the incomplete type is blocking actual functionality.The same issue exists in
client/www/lib/types.ts(lines 229–230).🛠️ Proposed fix
- onDelete?: 'cascade'; - onDeleteReverse?: 'cascade'; + onDelete?: OnDelete; + onDeleteReverse?: OnDelete;Apply the same fix to
client/www/lib/types.tsas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/components/src/types.ts` around lines 288 - 289, SchemaAttr.onDelete and SchemaAttr.onDeleteReverse are too narrow (only 'cascade') while DBAttr['on-delete'] uses the OnDelete union that includes 'restrict', causing assignment type errors in schema.ts (where attrDesc['on-delete'] is assigned to schemaAttr.onDelete) and blocking real UI behavior handled in edit-namespace-dialog.tsx; update the declarations for SchemaAttr.onDelete and SchemaAttr.onDeleteReverse in client/packages/components/src/types.ts (and mirror the same change in client/www/lib/types.ts) to use the OnDelete type (or the equivalent union including 'restrict' | 'cascade' | null | undefined) so the types align with DBAttr['on-delete'] and the schema transformation compiles.
♻️ Duplicate comments (1)
client/www/pages/docs/modeling-data.md (1)
395-400: Past review comment about missing language tag has been addressed.Line 395 now correctly includes the
```typescriptlanguage specifier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/www/pages/docs/modeling-data.md` around lines 395 - 400, The review noted the missing language tag has already been fixed for the code block defining postAuthor (the block containing the postAuthor object with forward/reverse relations), so remove or mark the duplicate review comment as resolved; search for the postAuthor snippet in docs/modeling-data.md and delete the redundant "[duplicate_comment]" review note or update the PR review thread to indicate the issue is addressed.
🧹 Nitpick comments (4)
client/packages/cli/src/renderSchemaPlan.ts (1)
57-57: Intentional!=— add a comment to prevent accidental tightening to!==.
null != undefinedevaluates tofalseunder loose equality, which correctly treats both as "no on-delete action." Changing to!==would makenull → undefinedtransitions appear as spurious diffs. The same applies to line 75 forprevOnDeleteReverse != newOnDeleteReverse.💬 Suggested inline comment
- if (prevOnDelete != newOnDelete) { + // Use loose != so that null and undefined are treated as equivalent ("no action") + if (prevOnDelete != newOnDelete) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/renderSchemaPlan.ts` at line 57, The loose equality checks using prevOnDelete != newOnDelete (and the analogous prevOnDeleteReverse != newOnDeleteReverse) are intentional to treat null and undefined as equivalent; add a short inline comment next to each check explaining that the loose != is deliberate to avoid treating null→undefined transitions as diffs and to prevent future accidental tightening to !==, e.g., "/* intentional loose equality: treat null and undefined as equivalent */" so reviewers understand the rationale.server/src/instant/util/test.clj (1)
85-98: Docstring does not cover the new:on-delete-restrict/:on-delete-reverse-restrictkeywords.The docstring at line 85 shows
:on-delete-reverse(cascade) as the only on-delete example, but thecasenow also handles:on-delete-restrictand:on-delete-reverse-restrict. Consider adding entries for both new options so callers know what to pass.📝 Proposed docstring update
Or a ref: - [[:B/c :C/bs] :many :unique? :on-delete-reverse] + [[:B/c :C/bs] :many :unique? :on-delete-reverse] ; reverse-side cascade + [[:B/c :C/bs] :many :unique? :on-delete-restrict] ; forward-side restrict + [[:B/c :C/bs] :many :unique? :on-delete-reverse-restrict] ; reverse-side restrict🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/util/test.clj` around lines 85 - 98, Update the docstring that documents the on-delete options to include entries for the new keywords :on-delete-restrict and :on-delete-reverse-restrict in addition to the existing :on-delete-reverse example; specifically add short example blocks showing the expected transformation (namespace, ident, reverse ns, reverse ident, type, cardinality, unique?, index?, and on-delete-restrict or on-delete-reverse-restrict values) so callers know to pass :on-delete-restrict and :on-delete-reverse-restrict similarly to :on-delete-reverse.client/packages/platform/src/api.ts (1)
422-438: Redundant type assertions and verbose ternary can be simplified.
attr['on-delete']is already typed asInstantDBAttrOnDelete | null | undefined, so the explicit exhaustive ternary withas 'cascade'/as 'restrict'casts is unnecessary. Both forward and reverseonDeletemappings reduce to:attr['on-delete'] ?? undefinedsince
null ?? undefined === undefinedand the string literals pass through unchanged.♻️ Proposed simplification
forward: { on: fe, has: fhas, label: flabel, required: attr['required?'] || undefined, - onDelete: - attr['on-delete'] === 'cascade' - ? ('cascade' as 'cascade') - : attr['on-delete'] === 'restrict' - ? ('restrict' as 'restrict') - : undefined, + onDelete: attr['on-delete'] ?? undefined, }, reverse: { on: re, has: rhas, label: rlabel, - onDelete: - attr['on-delete-reverse'] === 'cascade' - ? ('cascade' as 'cascade') - : attr['on-delete-reverse'] === 'restrict' - ? ('restrict' as 'restrict') - : undefined, + onDelete: attr['on-delete-reverse'] ?? undefined, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/platform/src/api.ts` around lines 422 - 438, The onDelete mapping uses redundant ternaries and type assertions; simplify both forward and reverse mappings by returning the existing typed value directly. Replace the nested ternaries that set onDelete in the forward mapping (currently using attr['on-delete'] === 'cascade' ? ('cascade' as 'cascade') : ...) with a direct pass-through like attr['on-delete'] ?? undefined, and do the same for the reverse mapping using attr['on-delete-reverse'] ?? undefined; this removes unnecessary casts and preserves the existing InstantDBAttrOnDelete | null | undefined semantics in the objects where on, has, label, onDelete are defined (refer to the reverse block and the variables re, rhas, rlabel).server/src/instant/db/transaction.clj (1)
530-545: Redundant condition insidecasebranch.On line 532,
(= cascade-rule :restrict)is always true because the enclosingcasealready matched:restrict. Thewhenonly needs the(not (contains? ...))check.♻️ Simplified condition
(case cascade-rule :restrict - (when (and (= cascade-rule :restrict) - (not (contains? original-deleted-entities [entity_id etype]))) + (when-not (contains? original-deleted-entities [entity_id etype]) (ex/throw-validation-err!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/transaction.clj` around lines 530 - 545, The code inside the case branch for :restrict redundantly checks (= cascade-rule :restrict) even though the enclosing case already matched :restrict; remove that redundant equality check and only guard the error throw with (not (contains? original-deleted-entities [entity_id etype])) so the logic in the branch (which calls ex/throw-validation-err! with vectorize-tx-step and tx-step-maps and formats parent_etype and etype) remains the same but with the simplified condition referencing cascade-rule, original-deleted-entities, entity_id, etype, tx-step-maps, vectorize-tx-step and ex/throw-validation-err!.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx`:
- Around line 977-998: Update the Checkbox label text in
edit-namespace-dialog.tsx (the Checkbox component rendering the "Restrict Delete
{reverseNamespaceName} → {namespaceName}" label) to clarify there is no temporal
requirement; replace the sentence "When a {reverseNamespaceName} entity is
deleted, all linked {namespaceName} must be deleted first or the transaction
will be blocked" with wording like "When a {reverseNamespaceName} entity is
deleted, all linked {namespaceName} must also be deleted in the same transaction
or the operation will be blocked" so it correctly reflects that deletions can
occur within the same transaction; ensure you update the JSX string that
references reverseNamespaceName and namespaceName inside the label prop.
- Around line 364-365: The code inconsistently uses undefined in AddAttrForm
payloads and null in EditAttrForm/updateRef for disabled on-delete values;
update AddAttrForm to use null instead of undefined when producing the
onDelete/onDeleteReverse values so both forms produce null for "no action", and
adjust the OnDelete type or payload typing if necessary to accept null; target
the AddAttrForm payload creation (the block that currently sets
onDelete/onDeleteReverse to undefined) and make it set null to match
EditAttrForm/updateRef and the useState initializations (onDelete,
onDeleteReverse).
In `@server/src/instant/db/transaction.clj`:
- Around line 434-442: The entids CTE declares five columns but the SELECT
returns six (including direction), causing a mismatch; update the entids CTE
column list used where entids is defined to include the direction column (i.e.,
add "direction" to the tuple of column names) so the declared columns match the
SELECT output, or alternatively remove/NULL the direction expression from the
SELECT—modify the entids CTE declaration to keep names in sync with the SELECT
(reference: entids CTE and the direction expression in its SELECT).
---
Outside diff comments:
In `@client/packages/components/src/types.ts`:
- Around line 288-289: SchemaAttr.onDelete and SchemaAttr.onDeleteReverse are
too narrow (only 'cascade') while DBAttr['on-delete'] uses the OnDelete union
that includes 'restrict', causing assignment type errors in schema.ts (where
attrDesc['on-delete'] is assigned to schemaAttr.onDelete) and blocking real UI
behavior handled in edit-namespace-dialog.tsx; update the declarations for
SchemaAttr.onDelete and SchemaAttr.onDeleteReverse in
client/packages/components/src/types.ts (and mirror the same change in
client/www/lib/types.ts) to use the OnDelete type (or the equivalent union
including 'restrict' | 'cascade' | null | undefined) so the types align with
DBAttr['on-delete'] and the schema transformation compiles.
---
Duplicate comments:
In `@client/www/pages/docs/modeling-data.md`:
- Around line 395-400: The review noted the missing language tag has already
been fixed for the code block defining postAuthor (the block containing the
postAuthor object with forward/reverse relations), so remove or mark the
duplicate review comment as resolved; search for the postAuthor snippet in
docs/modeling-data.md and delete the redundant "[duplicate_comment]" review note
or update the PR review thread to indicate the issue is addressed.
---
Nitpick comments:
In `@client/packages/cli/src/renderSchemaPlan.ts`:
- Line 57: The loose equality checks using prevOnDelete != newOnDelete (and the
analogous prevOnDeleteReverse != newOnDeleteReverse) are intentional to treat
null and undefined as equivalent; add a short inline comment next to each check
explaining that the loose != is deliberate to avoid treating null→undefined
transitions as diffs and to prevent future accidental tightening to !==, e.g.,
"/* intentional loose equality: treat null and undefined as equivalent */" so
reviewers understand the rationale.
In `@client/packages/platform/src/api.ts`:
- Around line 422-438: The onDelete mapping uses redundant ternaries and type
assertions; simplify both forward and reverse mappings by returning the existing
typed value directly. Replace the nested ternaries that set onDelete in the
forward mapping (currently using attr['on-delete'] === 'cascade' ? ('cascade' as
'cascade') : ...) with a direct pass-through like attr['on-delete'] ??
undefined, and do the same for the reverse mapping using
attr['on-delete-reverse'] ?? undefined; this removes unnecessary casts and
preserves the existing InstantDBAttrOnDelete | null | undefined semantics in the
objects where on, has, label, onDelete are defined (refer to the reverse block
and the variables re, rhas, rlabel).
In `@server/src/instant/db/transaction.clj`:
- Around line 530-545: The code inside the case branch for :restrict redundantly
checks (= cascade-rule :restrict) even though the enclosing case already matched
:restrict; remove that redundant equality check and only guard the error throw
with (not (contains? original-deleted-entities [entity_id etype])) so the logic
in the branch (which calls ex/throw-validation-err! with vectorize-tx-step and
tx-step-maps and formats parent_etype and etype) remains the same but with the
simplified condition referencing cascade-rule, original-deleted-entities,
entity_id, etype, tx-step-maps, vectorize-tx-step and ex/throw-validation-err!.
In `@server/src/instant/util/test.clj`:
- Around line 85-98: Update the docstring that documents the on-delete options
to include entries for the new keywords :on-delete-restrict and
:on-delete-reverse-restrict in addition to the existing :on-delete-reverse
example; specifically add short example blocks showing the expected
transformation (namespace, ident, reverse ns, reverse ident, type, cardinality,
unique?, index?, and on-delete-restrict or on-delete-reverse-restrict values) so
callers know to pass :on-delete-restrict and :on-delete-reverse-restrict
similarly to :on-delete-reverse.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
client/packages/cli/src/renderSchemaPlan.tsclient/packages/components/src/components/explorer/edit-namespace-dialog.tsxclient/packages/components/src/types.tsclient/packages/core/src/attrTypes.tsclient/packages/core/src/schemaTypes.tsclient/packages/platform/__tests__/src/migrations.test.tsclient/packages/platform/src/api.tsclient/packages/platform/src/schema.tsclient/www/pages/docs/modeling-data.mdserver/resources/migrations/100_on-delete-restrict.down.sqlserver/resources/migrations/100_on-delete-restrict.up.sqlserver/src/instant/config_edn.cljserver/src/instant/db/transaction.cljserver/src/instant/model/schema.cljserver/src/instant/util/test.cljserver/test/instant/db/transaction_test.clj
client/packages/components/src/components/explorer/edit-namespace-dialog.tsx
Show resolved
Hide resolved
client/packages/components/src/components/explorer/edit-namespace-dialog.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
client/packages/components/src/components/explorer/edit-namespace-dialog.tsx (1)
432-435:undefinedvsnullfor disabled on-delete (unresolved)
AddAttrFormstill passesundefinedwhen on-delete is disallowed, whileEditAttrForm(lines 1701–1704) passesnull. JSON serialization dropsundefinedkeys but preservesnull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx` around lines 432 - 435, The AddAttrForm currently sets 'on-delete' and 'on-delete-reverse' to undefined when disallowed, but EditAttrForm uses null; change AddAttrForm to pass null instead of undefined for those props so the keys are preserved during JSON serialization—update the object that sets 'on-delete' and 'on-delete-reverse' in edit-namespace-dialog.tsx (the AddAttrForm prop assignment) to use null when isOnDeleteAllowed/isOnDeleteReverseAllowed are false, matching EditAttrForm.
🧹 Nitpick comments (3)
server/src/instant/db/transaction.clj (1)
530-543: Redundant guard inside:restrictcase branch.On line 532,
(= cascade-rule :restrict)is always true because we're already inside the:restrictbranch of thecase. This is dead code.♻️ Suggested simplification
(case cascade-rule :restrict - (when (and (= cascade-rule :restrict) - (not (contains? original-deleted-entities [entity_id etype]))) + (when-not (contains? original-deleted-entities [entity_id etype]) (ex/throw-validation-err! :on-delete-restrict (map vectorize-tx-step tx-step-maps)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/transaction.clj` around lines 530 - 543, Inside the :restrict branch remove the redundant (= cascade-rule :restrict) guard since case already matched :restrict; change the when condition in the branch to only check (not (contains? original-deleted-entities [entity_id etype])) so the ex/throw-validation-err! call (using vectorize-tx-step, tx-step-maps, parent_etype, etype) fires only when the linked entity hasn't already been deleted.client/packages/components/src/components/explorer/edit-namespace-dialog.tsx (2)
926-928:onDelete/onDeleteReversestate persists across relationship-type changesIn
AddAttrForm, if a user selects cascade/restrict, then changes to a relationship type that disables on-delete (e.g., many-many), then switches back, the previously selected action reappears checked. Consider resettingonDelete/onDeleteReversetonullin thesetRelationshiphandler when the new type makes them disallowed.♻️ Example reset in RelationshipConfigurator's onChange
onChange={(v) => { setRelationship(v.value); + // Clear on-delete state when the new relationship doesn't allow it + const newIsOnDeleteAllowed = + v.value === 'one-one' || v.value === 'one-many'; + const newIsOnDeleteReverseAllowed = + v.value === 'one-one' || v.value === 'many-one'; + if (!newIsOnDeleteAllowed) setOnDelete(null); + if (!newIsOnDeleteReverseAllowed) setOnDeleteReverse(null); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx` around lines 926 - 928, The selected onDelete/onDeleteReverse values persist when switching relationship types; update the relationship change handler (where setRelationship is called, e.g., the onChange in RelationshipConfigurator/AddAttrForm) to clear disallowed cascade/restrict state by setting onDelete and onDeleteReverse to null whenever the newly chosen relationship type does not support them; specifically, inside the onChange that calls setRelationship(v.value) detect the target relationship type and include a reset (set onDelete=null and onDeleteReverse=null) when the new type disables on-delete behavior so stale selections are not shown when switching back and forth.
955-1046: Consider a three-option select/radio for the delete action instead of two independent checkboxes
onDeletecan benull | 'cascade' | 'restrict'— three mutually exclusive states. Representing them as two checkboxes that implicitly clear each other is functionally correct but unconventional; a<Select>or radio group with options None / Cascade / Restrict would make the mutual exclusivity explicit and reduce the UI surface area by half (one control per direction instead of two).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx` around lines 955 - 1046, The current UI uses two mutually-exclusive Checkbox controls (checked by onDelete === 'cascade' / 'restrict' and onDeleteReverse === ...) which is confusing; replace each checkbox pair with a single explicit three-option control (radio group or Select) for the directional delete setting. Update the controls wired to state variables onDelete and onDeleteReverse (and their setters setOnDelete and setOnDeleteReverse) so the new control exposes values null | 'cascade' | 'restrict', keeps the existing disabled/title/label text, and preserves isOnDeleteAllowed/isOnDeleteReverseAllowed logic; remove the paired Checkbox components and use one RadioGroup/Select per direction to enforce mutual exclusivity and simplify the UI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx`:
- Around line 1040-1043: Edit the reverse Restrict label in the
EditNamespaceDialog component so its wording matches the forward Restrict label:
replace the phrase "must also be deleted first" (the JSX string that
interpolates {namespaceName} and {reverseNamespaceName}) with a wording that
clarifies linked entities must be deleted within the same transaction (or omit
"first"), e.g., mirror the forward Restrict text; update the span inside
edit-namespace-dialog.tsx that renders namespaceName and reverseNamespaceName
accordingly.
In `@server/src/instant/db/transaction.clj`:
- Around line 537-543: The error text is using parent_etype (the root entity
from the recursive CTE) which misattributes the immediate parent in multi-hop
cascades; change the message to use the immediate parent's etype (the one-hop-up
etype produced by the CTE/row for the violating link) instead of parent_etype,
or explicitly note the transitive nature in the message; locate the error
construction using parent_etype and etype and replace parent_etype with the
immediate parent etype field (or augment the CTE to expose that field and
reference it) so the message correctly reports the entity directly linked to the
blocked delete.
---
Duplicate comments:
In
`@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx`:
- Around line 432-435: The AddAttrForm currently sets 'on-delete' and
'on-delete-reverse' to undefined when disallowed, but EditAttrForm uses null;
change AddAttrForm to pass null instead of undefined for those props so the keys
are preserved during JSON serialization—update the object that sets 'on-delete'
and 'on-delete-reverse' in edit-namespace-dialog.tsx (the AddAttrForm prop
assignment) to use null when isOnDeleteAllowed/isOnDeleteReverseAllowed are
false, matching EditAttrForm.
---
Nitpick comments:
In
`@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx`:
- Around line 926-928: The selected onDelete/onDeleteReverse values persist when
switching relationship types; update the relationship change handler (where
setRelationship is called, e.g., the onChange in
RelationshipConfigurator/AddAttrForm) to clear disallowed cascade/restrict state
by setting onDelete and onDeleteReverse to null whenever the newly chosen
relationship type does not support them; specifically, inside the onChange that
calls setRelationship(v.value) detect the target relationship type and include a
reset (set onDelete=null and onDeleteReverse=null) when the new type disables
on-delete behavior so stale selections are not shown when switching back and
forth.
- Around line 955-1046: The current UI uses two mutually-exclusive Checkbox
controls (checked by onDelete === 'cascade' / 'restrict' and onDeleteReverse ===
...) which is confusing; replace each checkbox pair with a single explicit
three-option control (radio group or Select) for the directional delete setting.
Update the controls wired to state variables onDelete and onDeleteReverse (and
their setters setOnDelete and setOnDeleteReverse) so the new control exposes
values null | 'cascade' | 'restrict', keeps the existing disabled/title/label
text, and preserves isOnDeleteAllowed/isOnDeleteReverseAllowed logic; remove the
paired Checkbox components and use one RadioGroup/Select per direction to
enforce mutual exclusivity and simplify the UI.
In `@server/src/instant/db/transaction.clj`:
- Around line 530-543: Inside the :restrict branch remove the redundant (=
cascade-rule :restrict) guard since case already matched :restrict; change the
when condition in the branch to only check (not (contains?
original-deleted-entities [entity_id etype])) so the ex/throw-validation-err!
call (using vectorize-tx-step, tx-step-maps, parent_etype, etype) fires only
when the linked entity hasn't already been deleted.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/packages/components/src/components/explorer/edit-namespace-dialog.tsxserver/src/instant/db/transaction.clj
client/packages/components/src/components/explorer/edit-namespace-dialog.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+537
to
+543
| [{:message (format (clojure.string/join | ||
| " " | ||
| ["This transaction violates an on-delete constraint." | ||
| "The `%s` entity cannot be deleted unless its" | ||
| "linked `%s` entity is deleted first."] ) | ||
| parent_etype | ||
| etype)}])) |
Contributor
There was a problem hiding this comment.
Error message may be misleading in multi-hop cascade scenarios.
parent_etype refers to the root entity's etype (inherited through the recursive CTE), not the immediate parent in the link chain. In a multi-hop cascade like A→B (cascade) → C (restrict), the error would say "The A entity cannot be deleted unless its linked C entity is deleted first", even though C is linked to B, not A directly.
Consider using the immediate parent's etype (the entity one hop up in the cascade chain) for a more precise error, or clarify in the message that the constraint is transitive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/instant/db/transaction.clj` around lines 537 - 543, The error text
is using parent_etype (the root entity from the recursive CTE) which
misattributes the immediate parent in multi-hop cascades; change the message to
use the immediate parent's etype (the one-hop-up etype produced by the CTE/row
for the violating link) instead of parent_etype, or explicitly note the
transitive nature in the message; locate the error construction using
parent_etype and etype and replace parent_etype with the immediate parent etype
field (or augment the CTE to expose that field and reference it) so the message
correctly reports the entity directly linked to the blocked delete.
Contributor
Author
There was a problem hiding this comment.
Write an test demonstrating the problem
Contributor
There was a problem hiding this comment.
@dwwoelfel, I'll write a test that demonstrates the misleading error message in multi-hop cascade scenarios.
Task: Add test demonstrating misleading error message in multi-hop cascade
This test will set up a scenario where:
- Entity A has a
cascaderelationship to B - Entity B has a
restrictrelationship to C - When attempting to delete A, the error incorrectly says "The
Aentity cannot be deleted unless its linkedCentity is deleted first" - But C is actually linked to B (not A), so the message should reference B as the immediate parent
🧠 Learnings used
Learnt from: dwwoelfel
Repo: instantdb/instant PR: 2283
File: server/src/instant/db/indexing_jobs.clj:291-309
Timestamp: 2026-02-13T23:44:20.187Z
Learning: In Clojure code, when calling cms/lookup, do not pass a database connection argument (especially write connections). This ensures the lookup uses the internal cache instead of bypassing it. Review all cms/lookup usages across server/src to confirm they do not supply a DB conn, and add a note if a specific call legitimately requires a connection (in which case caching behavior may differ).
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/www/lib/types.ts (1)
182-184:⚠️ Potential issue | 🟠 MajorUpdate
DBAttrto accept both 'cascade' and 'restrict' for on-delete fields.The wire-format
DBAttrinterface at lines 182-183 still limits'on-delete'and'on-delete-reverse'to the literal'cascade', but the server now returns'restrict'as well. Since theOnDeletetype (already imported in this file) includes both values, update these fields to match the broader type used inSchemaAttr.Proposed fix
- 'on-delete'?: 'cascade'; - 'on-delete-reverse'?: 'cascade'; + 'on-delete'?: OnDelete; + 'on-delete-reverse'?: OnDelete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/www/lib/types.ts` around lines 182 - 184, The DBAttr interface currently restricts 'on-delete' and 'on-delete-reverse' to the literal 'cascade'; update both fields in DBAttr to use the imported OnDelete type (same as SchemaAttr) so they accept 'cascade' or 'restrict' as returned by the server; locate DBAttr and replace the string literal types for 'on-delete' and 'on-delete-reverse' with OnDelete.
♻️ Duplicate comments (2)
client/packages/components/src/components/explorer/edit-namespace-dialog.tsx (1)
432-435:undefinedvsnullinconsistency for disabled on-delete values (same asEditAttrForm).This inconsistency is still present from the prior review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx` around lines 432 - 435, The code passes undefined for disabled delete handlers ('on-delete' and 'on-delete-reverse') causing inconsistency with EditAttrForm which uses null; make the behavior consistent by returning null instead of undefined when handlers are not allowed—update the ternaries around isOnDeleteAllowed/onDelete and isOnDeleteReverseAllowed/onDeleteReverse to yield null (not undefined) so both EditNamespaceDialog and EditAttrForm use the same disabled marker.server/src/instant/db/transaction.clj (1)
557-563:⚠️ Potential issue | 🟡 MinorError message misleadingly attributes the constraint to the root entity in multi-hop cascades.
parent_etypepropagates the root deleted entity's etype through the CTE, not the immediate entity that holds the:restrictlink. In a multi-hop scenario (A→B cascade, B→C restrict), the error reads "TheAentity cannot be deleted unless its linkedCentity is deleted first", when the actual constraining relationship is between B and C.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/transaction.clj` around lines 557 - 563, The error message uses parent_etype (which currently carries the root deleted entity type through the CTE) so multi-hop cascades misattribute the constraint; modify the CTE and downstream code to propagate and use the immediate parent entity type (e.g., add/propagate an immediate_parent_etype or parent_of_current column in the recursive CTE) and change the error construction to use that immediate_parent_etype instead of the root parent_etype when formatting the message for the restrict violation (references: parent_etype and etype in transaction.clj).
🧹 Nitpick comments (3)
client/packages/components/src/components/explorer/edit-namespace-dialog.tsx (2)
378-381: StaleonDeletestate when switching to a relationship that disallows on-delete.
onDelete/onDeleteReverseare not cleared whenrelationshipchanges tomany-manyormany-one(whereisOnDeleteAllowedbecomesfalse). The payload guard is correct, but if the user setscascade, switches tomany-many, then back toone-one, the cascade checkbox silently reactivates. AuseEffectthat resetsonDelete/onDeleteReversetonullwhen the respectiveisOnDelete*Allowedflag flips tofalsewould make the state transitions explicit.♻️ Suggested approach (AddAttrForm; same pattern applies to EditAttrForm)
+ useEffect(() => { + if (!isOnDeleteAllowed) setOnDelete(null); + }, [isOnDeleteAllowed]); + + useEffect(() => { + if (!isOnDeleteReverseAllowed) setOnDeleteReverse(null); + }, [isOnDeleteReverseAllowed]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx` around lines 378 - 381, The onDelete/onDeleteReverse state isn't cleared when relationship changes to types that disallow on-delete, causing stale values to reappear; add a useEffect in EditNamespaceDialog (or the component containing isOnDeleteAllowed/isOnDeleteReverseAllowed) that watches isOnDeleteAllowed and isOnDeleteReverseAllowed and when either becomes false sets the corresponding onDelete or onDeleteReverse state to null (or the initial empty value). Locate the declarations for isOnDeleteAllowed and isOnDeleteReverseAllowed and the state variables onDelete/onDeleteReverse, then implement the effect(s) to explicitly reset the state whenever the allowed flag flips to false.
955-998: Consider radio buttons or a Select for mutually exclusive Cascade / Restrict choices.
onDeleteis a single-valued field ('cascade','restrict', ornull), but it's rendered as two independentCheckboxcomponents. Visually, checkboxes imply independent toggling, which may confuse users. A radio-button group (None/Cascade/Restrict) or aSelectwould make the mutual-exclusivity explicit and is the standard pattern for this kind of choice.The same applies to the reverse direction (lines 1023–1047).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx` around lines 955 - 998, The UI uses two independent Checkbox components (checked logic tied to onDelete and state setters setOnDelete, with props isOnDeleteAllowed and constraints.attr) for a single-valued field onDelete, which is confusing; replace the two Checkbox controls with a single radio-group or Select (options: None / cascade / restrict) bound to onDelete, wire its onChange to call setOnDelete with the selected value (or null for None), preserve disabled behavior (use !isOnDeleteAllowed || constraints.attr.disabled), keep title={constraints.attr.message} and the current label copy (using reverseNamespaceName and namespaceName), and apply the same replacement for the reverse direction controls referenced in the file (the block around lines 1023–1047).server/src/instant/db/transaction.clj (1)
547-548: Redundant(= cascade-rule :restrict)guard inside the:restrictcase branch.The
casedispatch oncascade-rulealready guaranteescascade-ruleis:restrictin this branch; the innerandcondition is alwaystrue. The(= cascade-rule :restrict)term can simply be removed.♻️ Proposed fix
:restrict - (when (and (= cascade-rule :restrict) - (not (or (contains? original-deleted-entities [entity_id etype]) + (when (not (or (contains? original-deleted-entities [entity_id etype]) (contains? original-deleted-triples (case on-delete :on-delete [entity_id attr_id parent_id] :on-delete-reverse - [parent_id attr_id entity_id]))))) + [parent_id attr_id entity_id]))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/transaction.clj` around lines 547 - 548, Remove the redundant equality check inside the :restrict branch: the surrounding case already dispatches on cascade-rule being :restrict, so drop the unnecessary (= cascade-rule :restrict) term from the and-condition in the code that checks original-deleted-entities for [entity_id etype]; update the condition to directly use (and (not (or (contains? original-deleted-entities [entity_id etype]) ...))) so the logic references cascade-rule only via the case dispatch and relies on original-deleted-entities, entity_id, and etype as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/www/lib/types.ts`:
- Line 2: Export the OnDelete type from the components package main entry and
update the DB types to consume it: add an export for OnDelete in
client/packages/components/src/index.tsx (re-export the type from wherever it’s
defined so the package main entry exposes it), change the import in
client/www/lib/types.ts to import { OnDelete } from '@instantdb/components'
instead of the node_modules path, and update the DBAttr interface in
client/www/lib/types.ts (the DBAttr symbol where currently `'on-delete'?:
'cascade'`) to use the OnDelete type (`'on-delete'?: OnDelete`) so it matches
SchemaAttr which already uses OnDelete.
---
Outside diff comments:
In `@client/www/lib/types.ts`:
- Around line 182-184: The DBAttr interface currently restricts 'on-delete' and
'on-delete-reverse' to the literal 'cascade'; update both fields in DBAttr to
use the imported OnDelete type (same as SchemaAttr) so they accept 'cascade' or
'restrict' as returned by the server; locate DBAttr and replace the string
literal types for 'on-delete' and 'on-delete-reverse' with OnDelete.
---
Duplicate comments:
In
`@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx`:
- Around line 432-435: The code passes undefined for disabled delete handlers
('on-delete' and 'on-delete-reverse') causing inconsistency with EditAttrForm
which uses null; make the behavior consistent by returning null instead of
undefined when handlers are not allowed—update the ternaries around
isOnDeleteAllowed/onDelete and isOnDeleteReverseAllowed/onDeleteReverse to yield
null (not undefined) so both EditNamespaceDialog and EditAttrForm use the same
disabled marker.
In `@server/src/instant/db/transaction.clj`:
- Around line 557-563: The error message uses parent_etype (which currently
carries the root deleted entity type through the CTE) so multi-hop cascades
misattribute the constraint; modify the CTE and downstream code to propagate and
use the immediate parent entity type (e.g., add/propagate an
immediate_parent_etype or parent_of_current column in the recursive CTE) and
change the error construction to use that immediate_parent_etype instead of the
root parent_etype when formatting the message for the restrict violation
(references: parent_etype and etype in transaction.clj).
---
Nitpick comments:
In
`@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx`:
- Around line 378-381: The onDelete/onDeleteReverse state isn't cleared when
relationship changes to types that disallow on-delete, causing stale values to
reappear; add a useEffect in EditNamespaceDialog (or the component containing
isOnDeleteAllowed/isOnDeleteReverseAllowed) that watches isOnDeleteAllowed and
isOnDeleteReverseAllowed and when either becomes false sets the corresponding
onDelete or onDeleteReverse state to null (or the initial empty value). Locate
the declarations for isOnDeleteAllowed and isOnDeleteReverseAllowed and the
state variables onDelete/onDeleteReverse, then implement the effect(s) to
explicitly reset the state whenever the allowed flag flips to false.
- Around line 955-998: The UI uses two independent Checkbox components (checked
logic tied to onDelete and state setters setOnDelete, with props
isOnDeleteAllowed and constraints.attr) for a single-valued field onDelete,
which is confusing; replace the two Checkbox controls with a single radio-group
or Select (options: None / cascade / restrict) bound to onDelete, wire its
onChange to call setOnDelete with the selected value (or null for None),
preserve disabled behavior (use !isOnDeleteAllowed ||
constraints.attr.disabled), keep title={constraints.attr.message} and the
current label copy (using reverseNamespaceName and namespaceName), and apply the
same replacement for the reverse direction controls referenced in the file (the
block around lines 1023–1047).
In `@server/src/instant/db/transaction.clj`:
- Around line 547-548: Remove the redundant equality check inside the :restrict
branch: the surrounding case already dispatches on cascade-rule being :restrict,
so drop the unnecessary (= cascade-rule :restrict) term from the and-condition
in the code that checks original-deleted-entities for [entity_id etype]; update
the condition to directly use (and (not (or (contains? original-deleted-entities
[entity_id etype]) ...))) so the logic references cascade-rule only via the case
dispatch and relies on original-deleted-entities, entity_id, and etype as
before.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/packages/components/src/components/explorer/edit-namespace-dialog.tsxclient/www/lib/types.tsserver/src/instant/db/transaction.cljserver/test/instant/db/transaction_test.clj
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/packages/components/src/components/explorer/edit-namespace-dialog.tsx (1)
955-998: Consider using radio buttons or a select for mutually exclusive options.The cascade and restrict checkboxes share the same
onDeletestate, making them mutually exclusive. While functionally correct, checkboxes typically imply independent toggles. A radio button group or select dropdown would better communicate the exclusive nature of these options to users.This is a minor UX consideration and the current implementation works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx` around lines 955 - 998, Replace the two mutually-exclusive Checkbox controls (the ones using checked based on onDelete === 'cascade' and onDelete === 'restrict' and toggled via setOnDelete) with a radio-group or select so the UI communicates exclusivity; locate the Checkbox instances around the onDelete state, isOnDeleteAllowed and setOnDelete usage in edit-namespace-dialog.tsx and implement a Radio/Select control that uses onDelete as the value, setOnDelete as the onChange handler, preserves the disabled logic (disabled when !isOnDeleteAllowed || constraints.attr.disabled), and keeps the same labels/title/tooltip text for cascade and restrict options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@client/packages/components/src/components/explorer/edit-namespace-dialog.tsx`:
- Around line 955-998: Replace the two mutually-exclusive Checkbox controls (the
ones using checked based on onDelete === 'cascade' and onDelete === 'restrict'
and toggled via setOnDelete) with a radio-group or select so the UI communicates
exclusivity; locate the Checkbox instances around the onDelete state,
isOnDeleteAllowed and setOnDelete usage in edit-namespace-dialog.tsx and
implement a Radio/Select control that uses onDelete as the value, setOnDelete as
the onChange handler, preserves the disabled logic (disabled when
!isOnDeleteAllowed || constraints.attr.disabled), and keeps the same
labels/title/tooltip text for cascade and restrict options.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/src/instant/db/transaction.clj (1)
567-578:⚠️ Potential issue | 🔴 CriticalCritical:
restrictconstraint validation is not being enforced — missing error throw.The
:restrictcase has an emptywhenbody. When the constraint is violated (linked entity is NOT being deleted), this code should throw a validation error, but instead it silently returnsnil. The restrict constraint will not be enforced.Compare with the expected behavior from the PR description: "attempting to delete an entity that is still linked from other entities will cause the server to fail the transaction".
🐛 Proposed fix to add the validation error
(case cascade-rule :restrict (when (and (= cascade-rule :restrict) (not (or (contains? original-deleted-entities [entity_id etype]) (contains? original-deleted-triples (case on-delete :on-delete [entity_id attr_id parent_id] :on-delete-reverse [parent_id attr_id entity_id]))))) - - -) + (ex/throw-validation-err! + :on-delete-restrict + (map vectorize-tx-step tx-step-maps) + [{:message (format "This transaction violates an on-delete constraint. The `%s` entity cannot be deleted unless its linked `%s` entity is deleted first." + parent_etype + etype)}])) `#_else` ent))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/transaction.clj` around lines 567 - 578, The :restrict branch currently has an empty when body so constraint violations are never enforced; update the (case cascade-rule :restrict ...) branch so that when (and (= cascade-rule :restrict) (not (or (contains? original-deleted-entities [entity_id etype]) (contains? original-deleted-triples (case on-delete :on-delete [entity_id attr_id parent_id] :on-delete-reverse [parent_id attr_id entity_id]))))) is true it throws a validation error (e.g. throw or throw+ex-info) with a descriptive message and include identifying data (entity_id, etype, attr_id, parent_id, rule :restrict) so the transaction fails instead of returning nil.
🧹 Nitpick comments (2)
server/src/instant/db/transaction.clj (1)
569-570: Redundant condition insidecaseclause.The check
(= cascade-rule :restrict)on line 569-570 is redundant since this code is already inside the:restrictcase of thecaseexpression —cascade-ruleis guaranteed to be:restricthere.♻️ Suggested simplification (after adding the missing error throw)
(case cascade-rule :restrict - (when (and (= cascade-rule :restrict) - (not (or (contains? original-deleted-entities [entity_id etype]) + (when-not (or (contains? original-deleted-entities [entity_id etype]) (contains? original-deleted-triples (case on-delete :on-delete [entity_id attr_id parent_id] :on-delete-reverse - [parent_id attr_id entity_id]))))) + [parent_id attr_id entity_id]))) (ex/throw-validation-err! ...))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/transaction.clj` around lines 569 - 570, Inside the :restrict branch of the case on cascade-rule, remove the redundant (= cascade-rule :restrict) check from the when condition and instead directly check (not (or (contains? original-deleted-entities [entity_id etype]) ...)). Also add the missing error throw where that when fires (e.g., throw an appropriate exception or call the existing error helper) so deletion is blocked; reference the symbols cascade-rule, :restrict case, original-deleted-entities, entity_id and etype to locate and update the code in transaction.clj.server/test/instant/db/transaction_test.clj (1)
4878-4929: Fix ambiguous test names in unlinking scenarios.Line 4911 and Line 4923 use the same description, and the first one says “works” while asserting a failure. This makes failures hard to interpret.
♻️ Suggested rename-only cleanup
- (testing "deleting user works if you unlink the book" + (testing "deleting user is blocked when one book is re-linked in the same tx" (is (thrown-with-msg? clojure.lang.ExceptionInfo #"violates an on-delete constraint" (tx/transact! (aurora/conn-pool :write) attr-model app-id [[:retract-triple (suid "b2") (:book/author attr->id) (suid "a")] [:add-triple (suid "b2") (:book/author attr->id) (suid "a")] [:retract-triple (suid "b3") (:book/author attr->id) (suid "a")] [:delete-entity (suid "a") "user"]]))) - (testing "deleting user works if you unlink the book" + (testing "deleting user works after unlinking remaining books" (tx/transact! (aurora/conn-pool :write) attr-model app-id [[:add-triple (suid "b2") (:book/author attr->id) (suid "a")] [:retract-triple (suid "b2") (:book/author attr->id) (suid "a")] [:retract-triple (suid "b3") (:book/author attr->id) (suid "a")] [:delete-entity (suid "a") "user"]])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/instant/db/transaction_test.clj` around lines 4878 - 4929, Two testing blocks share the same description "deleting user works if you unlink the book" but the first actually asserts a failure; rename the first testing description to reflect that the delete is still blocked when unlinking is transient (e.g. "deleting user is still blocked if unlinking is transient") and leave the second as "deleting user works if you unlink the book"; update the string in the testing form wrapping the transaction that calls tx/transact! with the sequence [:retract-triple ...][:add-triple ...][:retract-triple ...][:delete-entity ...] so test failure messages are unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/test/instant/db/transaction_test.clj`:
- Around line 5002-5055: The leading comment in the test
on-delete-restrict-reverse-with-unlinking is incorrect: change "user <- book" to
"user -> book" (or "user -> book (user/books -> book/author)") to reflect the
defined relation [[:user/books :book/author] :on-delete-reverse-restrict] in the
test; update the comment near the top of the test to match the actual direction
used by the attr vector and test logic.
---
Duplicate comments:
In `@server/src/instant/db/transaction.clj`:
- Around line 567-578: The :restrict branch currently has an empty when body so
constraint violations are never enforced; update the (case cascade-rule
:restrict ...) branch so that when (and (= cascade-rule :restrict) (not (or
(contains? original-deleted-entities [entity_id etype]) (contains?
original-deleted-triples (case on-delete :on-delete [entity_id attr_id
parent_id] :on-delete-reverse [parent_id attr_id entity_id]))))) is true it
throws a validation error (e.g. throw or throw+ex-info) with a descriptive
message and include identifying data (entity_id, etype, attr_id, parent_id, rule
:restrict) so the transaction fails instead of returning nil.
---
Nitpick comments:
In `@server/src/instant/db/transaction.clj`:
- Around line 569-570: Inside the :restrict branch of the case on cascade-rule,
remove the redundant (= cascade-rule :restrict) check from the when condition
and instead directly check (not (or (contains? original-deleted-entities
[entity_id etype]) ...)). Also add the missing error throw where that when fires
(e.g., throw an appropriate exception or call the existing error helper) so
deletion is blocked; reference the symbols cascade-rule, :restrict case,
original-deleted-entities, entity_id and etype to locate and update the code in
transaction.clj.
In `@server/test/instant/db/transaction_test.clj`:
- Around line 4878-4929: Two testing blocks share the same description "deleting
user works if you unlink the book" but the first actually asserts a failure;
rename the first testing description to reflect that the delete is still blocked
when unlinking is transient (e.g. "deleting user is still blocked if unlinking
is transient") and leave the second as "deleting user works if you unlink the
book"; update the string in the testing form wrapping the transaction that calls
tx/transact! with the sequence [:retract-triple ...][:add-triple
...][:retract-triple ...][:delete-entity ...] so test failure messages are
unambiguous.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/www/lib/types.tsserver/src/instant/db/transaction.cljserver/test/instant/db/transaction_test.clj
Comment on lines
+5002
to
+5055
| (deftest on-delete-restrict-reverse-with-unlinking | ||
| (with-empty-app | ||
| (fn [{app-id :id}] | ||
| ;; user <- book | ||
| (let [attr->id (test-util/make-attrs | ||
| app-id | ||
| [[:user/name :unique? :index?] | ||
| [:book/title :unique? :index?] | ||
| [[:user/books :book/author] :many :unique? :on-delete-reverse-restrict]]) | ||
| ids #{(suid "a") (suid "b1") (suid "b2") (suid "b3")} | ||
| attr-model (attr-model/get-by-app-id app-id)] | ||
|
|
||
| (test-util/insert-entities | ||
| app-id attr->id | ||
| [{:db/id (suid "a") :user/name "Leo Tolstoy" :user/books [(suid "b1") (suid "b2") (suid "b3")]} | ||
| {:db/id (suid "b1") :book/title "War and Peace"} | ||
| {:db/id (suid "b2") :book/title "Anna Karenina"} | ||
| {:db/id (suid "b3") :book/title "Death of Ivan Ilyich"}]) | ||
| (is (= #{(suid "a") (suid "b1") (suid "b2") (suid "b3")} | ||
| (test-util/find-entids-by-ids app-id attr->id ids))) | ||
|
|
||
| (testing "deleting book doesn’t delete user" | ||
| (tx/transact! (aurora/conn-pool :write) attr-model app-id [[:delete-entity (suid "b1") "book"]]) | ||
| (is (= #{(suid "a") (suid "b2") (suid "b3")} | ||
| (test-util/find-entids-by-ids app-id attr->id ids)))) | ||
|
|
||
| (testing "deleting user is blocked if you don't delete the book" | ||
| (is (thrown-with-msg? clojure.lang.ExceptionInfo | ||
| #"violates an on-delete constraint" | ||
| (tx/transact! (aurora/conn-pool :write) attr-model app-id [[:delete-entity (suid "a") "user"]]))) | ||
| (is (= #{(suid "a") (suid "b2") (suid "b3")} | ||
| (test-util/find-entids-by-ids app-id attr->id ids)))) | ||
|
|
||
| (testing "deleting user is blocked if you add the triple after retracting it" | ||
| (is (thrown-with-msg? | ||
| clojure.lang.ExceptionInfo | ||
| #"violates an on-delete constraint" | ||
| (tx/transact! (aurora/conn-pool :write) attr-model app-id | ||
| [[:retract-triple (suid "a") (:user/books attr->id) (suid "b2")] | ||
| [:add-triple (suid "a") (:user/books attr->id) (suid "b3")] | ||
| [:retract-triple (suid "a") (:user/books attr->id) (suid "b3")] | ||
| [:delete-entity (suid "a") "user"]]))) | ||
| (is (= #{(suid "a") (suid "b2") (suid "b3")} | ||
| (test-util/find-entids-by-ids app-id attr->id ids)))) | ||
|
|
||
| (testing "deleting user works if you also unlink the book" | ||
| (tx/transact! (aurora/conn-pool :write) attr-model app-id | ||
| [[:add-triple (suid "a") (:user/books attr->id) (suid "b3")] | ||
| [:retract-triple (suid "a") (:user/books attr->id) (suid "b2")] | ||
| [:retract-triple (suid "a") (:user/books attr->id) (suid "b3")] | ||
| [:delete-entity (suid "a") "user"]]) | ||
| (is (= #{(suid "b2") (suid "b3")} | ||
| (test-util/find-entids-by-ids app-id attr->id ids)))))))) | ||
|
|
Contributor
There was a problem hiding this comment.
Correct the relation direction comment for consistency.
Line 5005 says user <- book, but this test defines [[:user/books :book/author] ... :on-delete-reverse-restrict] (user → book). The mismatch is minor but misleading during test triage.
📝 Suggested one-line fix
- ;; user <- book
+ ;; user -> book🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/test/instant/db/transaction_test.clj` around lines 5002 - 5055, The
leading comment in the test on-delete-restrict-reverse-with-unlinking is
incorrect: change "user <- book" to "user -> book" (or "user -> book (user/books
-> book/author)") to reflect the defined relation [[:user/books :book/author]
:on-delete-reverse-restrict] in the test; update the comment near the top of the
test to match the actual direction used by the attr vector and test logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/test/instant/db/transaction_test.clj (1)
5005-5005:⚠️ Potential issue | 🟡 MinorCorrect the relation direction comment (
user -> book).Line 5005 says
user <- book, but the attr relation is[[:user/books :book/author] ...], i.e.user -> book.📝 One-line fix
- ;; user <- book + ;; user -> book🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/instant/db/transaction_test.clj` at line 5005, The inline relation-direction comment is wrong: change the comment that reads "user <- book" to "user -> book" to match the attribute relation `[[:user/books :book/author] ...]` (i.e., user/books points to book/author); update the comment near that attr vector so it correctly reflects the direction user -> book.
🧹 Nitpick comments (1)
server/src/instant/db/transaction.clj (1)
569-569: Redundant condition inside case branch.Line 569 checks
(= cascade-rule :restrict)inside the:restrictcase branch, which is always true. This is harmless but adds unnecessary noise.♻️ Suggested simplification
(case cascade-rule :restrict - (when (and (= cascade-rule :restrict) - (not (or (contains? original-deleted-entities [entity_id etype]) + (when (not (or (contains? original-deleted-entities [entity_id etype]) (contains? original-deleted-triples (case on-delete :on-delete [entity_id attr_id parent_id] :on-delete-reverse - [parent_id attr_id entity_id]))))) + [parent_id attr_id entity_id]))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/transaction.clj` at line 569, Inside the case handling for the :restrict branch in transaction.clj, remove the redundant equality check of (= cascade-rule :restrict) — it's always true inside that branch; update the when condition that currently uses (and (= cascade-rule :restrict) ...) to only test the remaining predicate(s) so the :restrict case body uses just the necessary condition(s) (locate the :restrict case handling that references cascade-rule to apply this change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/test/instant/db/transaction_test.clj`:
- Around line 4911-4927: The two adjacent testing blocks use the same
description and one is misleading: change the first testing string (the block
that asserts thrown-with-msg? around tx/transact!) to a clear failing title like
"deleting user throws on-delete constraint when book still linked", and change
the second testing string (the succeeding tx/transact! that succeeds) to a clear
success title like "deleting user works after unlinking book"; update only the
descriptive strings passed to testing that wrap the tx/transact! blocks so the
expectations remain unchanged.
- Around line 5035-5043: The test's sequence currently retracts (suid "b2") but
then adds and retracts (suid "b3"), so it doesn't model "add the triple after
retracting it"; update the transaction vector passed to tx/transact! in this
test (the entries using :retract-triple and :add-triple for the user/books edge)
to re-add and then retract the same suid "b2" (i.e., replace the (suid "b3")
occurrences with (suid "b2") so the sequence is [:retract-triple (suid "a")
(:user/books attr->id) (suid "b2")], [:add-triple (suid "a") (:user/books
attr->id) (suid "b2")], [:retract-triple (suid "a") (:user/books attr->id) (suid
"b2")] before the [:delete-entity (suid "a") "user"] call).
---
Duplicate comments:
In `@server/test/instant/db/transaction_test.clj`:
- Line 5005: The inline relation-direction comment is wrong: change the comment
that reads "user <- book" to "user -> book" to match the attribute relation
`[[:user/books :book/author] ...]` (i.e., user/books points to book/author);
update the comment near that attr vector so it correctly reflects the direction
user -> book.
---
Nitpick comments:
In `@server/src/instant/db/transaction.clj`:
- Line 569: Inside the case handling for the :restrict branch in
transaction.clj, remove the redundant equality check of (= cascade-rule
:restrict) — it's always true inside that branch; update the when condition that
currently uses (and (= cascade-rule :restrict) ...) to only test the remaining
predicate(s) so the :restrict case body uses just the necessary condition(s)
(locate the :restrict case handling that references cascade-rule to apply this
change).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/www/lib/types.tsserver/src/instant/db/transaction.cljserver/test/instant/db/transaction_test.cljserver/test/instant/reactive/session_test.clj
Comment on lines
+4911
to
+4927
| (testing "deleting user works if you unlink the book" | ||
| (is (thrown-with-msg? clojure.lang.ExceptionInfo | ||
| #"violates an on-delete constraint" | ||
| (tx/transact! (aurora/conn-pool :write) attr-model app-id | ||
| [[:retract-triple (suid "b2") (:book/author attr->id) (suid "a")] | ||
| [:add-triple (suid "b2") (:book/author attr->id) (suid "a")] | ||
| [:retract-triple (suid "b3") (:book/author attr->id) (suid "a")] | ||
| [:delete-entity (suid "a") "user"]]))) | ||
|
|
||
| (is (= #{(suid "a") (suid "b2") (suid "b3")} | ||
| (test-util/find-entids-by-ids app-id attr->id ids)))) | ||
|
|
||
| (testing "deleting user works if you unlink the book" | ||
| (tx/transact! (aurora/conn-pool :write) attr-model app-id [[:add-triple (suid "b2") (:book/author attr->id) (suid "a")] | ||
| [:retract-triple (suid "b2") (:book/author attr->id) (suid "a")] | ||
| [:retract-triple (suid "b3") (:book/author attr->id) (suid "a")] | ||
| [:delete-entity (suid "a") "user"]]) |
Contributor
There was a problem hiding this comment.
Fix duplicated/misleading test names in unlinking cases.
At Line 4911 the test title says “works” but expects an exception, and Line 4923 reuses the same title for the success case. This makes failures hard to interpret.
✏️ Suggested rename
- (testing "deleting user works if you unlink the book"
+ (testing "deleting user is blocked if a book is relinked in the same tx"
@@
- (testing "deleting user works if you unlink the book"
+ (testing "deleting user works if all books are unlinked in the same tx"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/test/instant/db/transaction_test.clj` around lines 4911 - 4927, The
two adjacent testing blocks use the same description and one is misleading:
change the first testing string (the block that asserts thrown-with-msg? around
tx/transact!) to a clear failing title like "deleting user throws on-delete
constraint when book still linked", and change the second testing string (the
succeeding tx/transact! that succeeds) to a clear success title like "deleting
user works after unlinking book"; update only the descriptive strings passed to
testing that wrap the tx/transact! blocks so the expectations remain unchanged.
Comment on lines
+5035
to
+5043
| (testing "deleting user is blocked if you add the triple after retracting it" | ||
| (is (thrown-with-msg? | ||
| clojure.lang.ExceptionInfo | ||
| #"violates an on-delete constraint" | ||
| (tx/transact! (aurora/conn-pool :write) attr-model app-id | ||
| [[:retract-triple (suid "a") (:user/books attr->id) (suid "b2")] | ||
| [:add-triple (suid "a") (:user/books attr->id) (suid "b3")] | ||
| [:retract-triple (suid "a") (:user/books attr->id) (suid "b3")] | ||
| [:delete-entity (suid "a") "user"]]))) |
Contributor
There was a problem hiding this comment.
Blocked-case logic re-adds the wrong edge, so the scenario is not what the test name says.
At Line 5041, the test retracts b2 but re-adds b3. That does not model “add the triple after retracting it” for the same link and can invalidate this assertion.
🐛 Suggested fix
- [[:retract-triple (suid "a") (:user/books attr->id) (suid "b2")]
- [:add-triple (suid "a") (:user/books attr->id) (suid "b3")]
+ [[:retract-triple (suid "a") (:user/books attr->id) (suid "b2")]
+ [:add-triple (suid "a") (:user/books attr->id) (suid "b2")]
[:retract-triple (suid "a") (:user/books attr->id) (suid "b3")]
[:delete-entity (suid "a") "user"]])))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (testing "deleting user is blocked if you add the triple after retracting it" | |
| (is (thrown-with-msg? | |
| clojure.lang.ExceptionInfo | |
| #"violates an on-delete constraint" | |
| (tx/transact! (aurora/conn-pool :write) attr-model app-id | |
| [[:retract-triple (suid "a") (:user/books attr->id) (suid "b2")] | |
| [:add-triple (suid "a") (:user/books attr->id) (suid "b3")] | |
| [:retract-triple (suid "a") (:user/books attr->id) (suid "b3")] | |
| [:delete-entity (suid "a") "user"]]))) | |
| (testing "deleting user is blocked if you add the triple after retracting it" | |
| (is (thrown-with-msg? | |
| clojure.lang.ExceptionInfo | |
| #"violates an on-delete constraint" | |
| (tx/transact! (aurora/conn-pool :write) attr-model app-id | |
| [[:retract-triple (suid "a") (:user/books attr->id) (suid "b2")] | |
| [:add-triple (suid "a") (:user/books attr->id) (suid "b2")] | |
| [:retract-triple (suid "a") (:user/books attr->id) (suid "b3")] | |
| [:delete-entity (suid "a") "user"]]))) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/test/instant/db/transaction_test.clj` around lines 5035 - 5043, The
test's sequence currently retracts (suid "b2") but then adds and retracts (suid
"b3"), so it doesn't model "add the triple after retracting it"; update the
transaction vector passed to tx/transact! in this test (the entries using
:retract-triple and :add-triple for the user/books edge) to re-add and then
retract the same suid "b2" (i.e., replace the (suid "b3") occurrences with (suid
"b2") so the sequence is [:retract-triple (suid "a") (:user/books attr->id)
(suid "b2")], [:add-triple (suid "a") (:user/books attr->id) (suid "b2")],
[:retract-triple (suid "a") (:user/books attr->id) (suid "b2")] before the
[:delete-entity (suid "a") "user"] call).
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
2 participants
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.
Adds support for
restrictas anon-deleteaction (currently we only supportcascade).If you try to delete the linked entity without deleting the things it is linked to, we will fail the transaction on the server.
Unlike cascade, I didn't implement the check on the client--it's harder to implement because it needs to see all of the transactions to make sure that the user didn't remove the linked entity in another part of the transaction. The cascade rule can operate from the single delete rule.
It will act like permissions, where it doesn't fail until it gets to the server.
The error looks like this:
{ "hint": { "data-type": "on-delete-restrict", "input": [ [ "delete-entity", "b9f972de-24a7-46a0-bb58-45960698e9e5", "one" ] ], "errors": [ { "message": "This transaction violates an on-delete constraint. The `one` entity cannot be deleted unless its linked `many` entity is deleted first." } ] }, "name": "InstantAPIError", "status": 400, "body": { "type": "validation-failed", "message": "Validation failed for on-delete-restrict: This transaction violates an on-delete constraint. The `one` entity cannot be deleted unless its linked `many` entity is deleted first.", "hint": { "data-type": "on-delete-restrict", "input": [ [ "delete-entity", "b9f972de-24a7-46a0-bb58-45960698e9e5", "one" ] ], "errors": [ { "message": "This transaction violates an on-delete constraint. The `one` entity cannot be deleted unless its linked `many` entity is deleted first." } ] }, "trace-id": "8503d69422f42e763ac8ed0de0fe59cb" } }