-
Notifications
You must be signed in to change notification settings - Fork 689
Implement CASCADE behavior enforcement for FOREIGN KEY constraints #2337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
- Updated parser to support ON DELETE/UPDATE CASCADE, SET NULL, SET DEFAULT, RESTRICT, NO ACTION - Added foreignKeys array to Table structure to track relationships - Implemented CASCADE delete in delete handler - Implemented CASCADE update in update handler - Added comprehensive test suite with 11 passing tests Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
- Refactor CASCADE delete into reusable helper function - Implement recursive CASCADE for grandchild rows - Add NOT NULL constraint checks for SET NULL and SET DEFAULT - Remove unused circular dependency tracking code - Add test for recursive CASCADE behavior Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
- Store row references instead of indices to avoid invalidation after recursive deletes - Simplify delete operation to call table.delete before setting newtable - Ensures correct behavior when CASCADE operations modify child arrays Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a lot of code. Are you sure this is the best way to solve this problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the minimum code needed for SQL-standard CASCADE behavior. The ~90 lines handle all 5 CASCADE actions (CASCADE, SET NULL, SET DEFAULT, RESTRICT, NO ACTION) with recursive support for grandchild rows and NOT NULL validation. The core logic is in the applyCascadeDeletes helper function which is reused for both WHERE and non-WHERE deletes to avoid duplication.
Commit b53bc38
src/72delete.js
Outdated
| }); | ||
|
|
||
| // Call table.delete for each row to update indices before setting newtable | ||
| rowsToDelete.forEach(function(row) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this only if there are indexes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Optimized to only call table.delete() when there are primary key or unique key indices that need updating. Added condition: if (table.pk || (table.uk && table.uk.length)) before the index update loop.
Commit b53bc38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like way too much code for this problem. Are you sure this is the best way to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ~200 lines in UPDATE handle all CASCADE actions for primary key changes, including pre-validation for RESTRICT, NOT NULL constraint checking, and tracking which columns are being updated to avoid unnecessary cascade operations. This is the minimum needed to properly implement SQL-99 CASCADE standard for UPDATE operations.
Commit b53bc38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure to assert the whole response object with the expected output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated all test assertions to use assert.deepEqual() for full object comparison instead of checking individual properties. This provides better test coverage and clearer failure messages. Also added unique table names and proper cleanup between tests.
Commit b53bc38
- Optimize DELETE to only call table.delete when indices exist (pk or uk) - Update all test assertions to use deepEqual for full object comparison - Add unique table names to tests I and L to prevent schema conflicts - Add DROP TABLE IF EXISTS at start of each test for clean state - Add afterEach cleanup hook to ensure all tables are dropped between tests Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
Description
AlaSQL parsed CASCADE syntax but did not enforce it. Deleting or updating parent rows left child rows orphaned.
Changes
Parser (
alasqlparser.jison)Metadata Storage (
23table.js,60createtable.js)table.foreignKeys[]with action metadataDELETE Handler (
72delete.js)applyCascadeDeletes()helper for multi-level cascadestable.delete()to update indices when primary key or unique keys existUPDATE Handler (
74update.js)Example
Testing
Added
test/test897.jswith 12 test cases covering all CASCADE actions, recursive cascades, and constraint violations.Test Quality Improvements:
assert.deepEqual()for complete object comparisonDROP TABLE IF EXISTSat test start for clean stateafterEachcleanup hook to ensure proper cleanup between testsOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.