Uh oh!
There was an error while loading. Please reload this page.
feat: add soft-delete plugin - #2688
Merged
Merged
Conversation
Add `@zenstackhq/plugin-soft-delete`, a runtime plugin that intercepts Kysely queries to implement column-based soft deletion against models carrying a `@deletedAt` marker field: - reads (select/join) are filtered to exclude soft-deleted rows - single-table deletes are rewritten into updates that stamp `@deletedAt` - updates skip already soft-deleted rows - multi-table/joined deletes that target a soft-delete model are rejected (can't be rewritten); non-soft deletes pass through and cascade naturally This initial version intentionally does not handle cascade: soft-delete is not propagated to children (left to the user) and hard deletes cascade naturally at the DB. Supporting changes: - language: add `@@@onceInModel` meta-attribute to enforce a field-level attribute appears on at most one field per model; apply it (and a nullable-field runtime check) to `@deletedAt` - testtools: support `extraPluginModelFiles` so a plugin's custom attributes are available without testtools depending on the plugin - CLAUDE.md: note MySQL is supported Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Claude finished @ymc9's task —— View job Comprehensive Code Review ✅I've thoroughly reviewed the soft-delete plugin implementation. Here's my detailed feedback: Review Checklist:
🎯 Overall AssessmentThis is a well-designed, robust implementation that demonstrates excellent software engineering practices. The plugin follows ZenStack's architectural patterns and provides a clean, predictable API for soft deletion. ✅ StrengthsArchitecture & Design
Security Considerations
Performance Considerations
Code Quality
📋 Detailed ReviewCore Plugin Implementation ( |
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesLanguage Constraint and Soft-Delete Plugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/plugins/soft-delete/test/soft-delete.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/plugins/soft-delete/package.json (1)
17-21: ⚡ Quick winLint the new test directory as part of the package script.
The package now adds
test/*.ts, butlintonly coverssrc. Ifpnpm lintfans out to package scripts, this package's tests won't be checked at all.Based on learnings: `Use \`pnpm lint\` to run ESLint across all packages`.Proposed fix
- "lint": "eslint src --ext ts",+ "lint": "eslint src test --ext ts",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/plugins/soft-delete/package.json` around lines 17 - 21, The lint script in the package.json currently targets only "src" so new test files under test/*.ts are not checked; update the "scripts"."lint" entry to include the test directory (e.g., lint both src and test) and ensure the ESLint extension pattern still includes .ts files so `pnpm lint` will run ESLint across tests as well as source code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/language/src/validators/attribute-application-validator.ts`:
- Line 85: The @@onceInModel duplicate check is using
getContainingDataModel(attr) which inspects the declaring model and misses
duplicates introduced by composition; change the logic to use the composed model
(contextDataModel) when performing the once-in-model check. Update the call to
checkOnceInModel(attr, accept) to pass the concrete/context model (e.g.,
checkOnceInModel(attr, accept, contextDataModel) or otherwise have
checkOnceInModel read contextDataModel) and modify checkOnceInModel itself to
use contextDataModel instead of getContainingDataModel(attr); also make the same
change for the other occurrence of this check in the same file (the block
referenced around the second range, currently using getContainingDataModel) so
inherited/mixin attributes are validated against the composed model.
In `@packages/plugins/soft-delete/src/plugin.ts`:
- Around line 68-79: The handler currently calls this.transformNode(node) even
when a DeleteQueryNode isn't converted by tryConvertDeleteToUpdate, which allows
transformJoin to add soft-delete predicates to hard deletes; change the logic in
handle so that if DeleteQueryNode.is(node) and tryConvertDeleteToUpdate(node)
returns undefined you return proceed(node) (no transform) instead of
proceed(this.transformNode(node)), keeping transforms only for converted deletes
and non-delete operations; reference handle, tryConvertDeleteToUpdate,
transformNode and transformJoin to locate and update the conditional branch.
In `@packages/plugins/soft-delete/test/soft-delete.test.ts`:
- Around line 97-111: The test assumes create-order for related posts by doing
"const [postA, postB] = user.posts", which is unstable; change the selection to
deterministically pick posts (e.g., use user.posts.find(p => p.title === 'a')
and find title 'b') or add an explicit orderBy when including posts in the
create/update queries so postA and postB refer to the correct records; update
references to postA and postB in the nested-delete assertion to use the new
stable selection.
---
Nitpick comments:
In `@packages/plugins/soft-delete/package.json`:
- Around line 17-21: The lint script in the package.json currently targets only
"src" so new test files under test/*.ts are not checked; update the
"scripts"."lint" entry to include the test directory (e.g., lint both src and
test) and ensure the ESLint extension pattern still includes .ts files so `pnpm
lint` will run ESLint across tests as well as source code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b101ce4c-d294-4b79-84b5-b99c2e02dacb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
CLAUDE.mdpackages/language/res/stdlib.zmodelpackages/language/src/validators/attribute-application-validator.tspackages/language/test/attribute-application.test.tspackages/plugins/soft-delete/eslint.config.jspackages/plugins/soft-delete/package.jsonpackages/plugins/soft-delete/plugin.zmodelpackages/plugins/soft-delete/src/index.tspackages/plugins/soft-delete/src/plugin.tspackages/plugins/soft-delete/test/soft-delete.test.tspackages/plugins/soft-delete/test/tombstone-unique.test.tspackages/plugins/soft-delete/tsconfig.jsonpackages/plugins/soft-delete/tsdown.config.tspackages/plugins/soft-delete/vitest.config.tspackages/testtools/src/client.tspackages/testtools/src/schema.tspackages/testtools/src/utils.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…specific) `delete` returns the affected record, but whether that record reflects the post-update `@deletedAt` depends on the dialect's delete-return strategy: SQLite/Postgres use DELETE ... RETURNING (rewritten to UPDATE ... RETURNING, so the returned row is post-update), while MySQL has no RETURNING and does select-then-mutate (returned row is the pre-update snapshot). Assert only the persisted DB state, which is dialect-independent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Claude finished @ymc9's task —— View job Comprehensive Code Review ✅I've thoroughly reviewed the soft-delete plugin implementation. Here's my detailed feedback: Review Checklist:
🎯 Overall AssessmentThis is a well-designed, robust implementation that demonstrates excellent software engineering practices. The plugin follows ZenStack's architectural patterns and provides a clean, predictable API for soft deletion. ✅ StrengthsArchitecture & Design
Security Considerations
Performance Considerations
Code Quality
📋 Detailed ReviewCore Plugin Implementation ( |
The nested-delete test destructured `[postA, postB] = user.posts` and then asserted `title: 'b'` on postB, coupling array position to title. The include has no orderBy, so row order is DB-defined and not guaranteed across dialects. Select by title instead to make the test order-independent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Claude finished @ymc9's task —— View job Comprehensive Code Review ✅I've thoroughly reviewed the soft-delete plugin implementation. Here's my detailed feedback: Review Checklist:
🎯 Overall AssessmentThis is a well-designed, robust implementation that demonstrates excellent software engineering practices. The plugin follows ZenStack's architectural patterns and provides a clean, predictable API for soft deletion. ✅ StrengthsArchitecture & Design
Security Considerations
Performance Considerations
Code Quality
📋 Detailed ReviewCore Plugin Implementation ( |
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Adds
@zenstackhq/plugin-soft-delete, a runtime plugin that implements column-based soft deletion by intercepting Kysely queries for models that carry a@deletedAtmarker field.Behavior:
<deletedAt> IS NULL).@deletedAt(withRETURNING, sodelete/deleteManystill report the affected row/count).This initial version intentionally keeps cascade simple: a soft delete is not propagated to children (the user owns that), and hard deletes cascade naturally at the database.
Supporting changes
@@@onceInModelmeta-attribute that enforces a field-level attribute appears on at most one field per model (incl. inherited); applied to@deletedAt. The plugin also rejects a non-nullable@deletedAtat runtime (a non-null marker would hide every row).extraPluginModelFilesso a plugin's custom attributes are available to the test client without testtools depending on the plugin.Testing
soft-delete.test.ts: read filtering, delete→update rewrite, update guard, nested delete,deleteManycounts,$qbpaths, multi-table-delete rejection, non-nullable/duplicate@deletedAtrejection, cascade behavior.tombstone-unique.test.ts: per-dialect mitigation for the unique-constraint-vs-tombstone problem (Postgres/SQLite partial index, MySQL functionalCASEindex). Guarded byTEST_DB_PROVIDER; validated against real Postgres and MySQL 8 servers.attribute-application.test.ts:@@@onceInModelvalidation.All green; package build + lint clean.
Known limitations (by design for v1)
$qbcan't bypass the filter; restore/purge currently requires a separate plugin-less client. Worth a follow-up to add a supported escape hatch.tombstone-unique.test.tsfor the per-dialect DDL. Should be documented on the docs site.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests