fix: raise notice to delete dependent fields after workspace reset #230
Conversation
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)scripts/sql/reset_db.sql (3)
The new deletion operation for
Critical: Move org_id retrieval to beginning of function The Move this line to the beginning of the function, right after the variable declarations: DECLARE
rcount integer;
_org_id varchar(255);
BEGIN
+ _org_id := (SELECT org_id FROM workspaces WHERE id = _workspace_id);
RAISE NOTICE 'Deleting data from workspace % ', _workspace_id;
Critical: Security and structural improvements needed While the commands are now on a single line as requested, there are several critical issues to address:
Consider this improved version that addresses all issues: - RAISE NOTICE E'\n\n\n\n\n\n\n\n\nSwitch to prod db and run the below queries to delete dependent fields';
- RAISE NOTICE E'rollback;begin; delete from platform_schema.dependent_expense_field_mappings where expense_field_id in (select id from platform_schema.expense_fields where org_id =''%'' and type=''DEPENDENT_SELECT''); delete from platform_schema.expense_fields where org_id = ''%'' and type = ''DEPENDENT_SELECT'';\n\n\n\n\n\n\n\n\n\n\n', _org_id, _org_id;
+ RAISE NOTICE E'\nAction Required: Switch to production database and execute the cleanup function:\n';
+ RAISE NOTICE E'SELECT platform_schema.cleanup_dependent_fields(''%'');\n', _org_id;Create this helper function in the production database: CREATE OR REPLACE FUNCTION platform_schema.cleanup_dependent_fields(IN _org_id varchar) RETURNS void AS $$
BEGIN
IF _org_id IS NULL THEN
RAISE EXCEPTION 'Organization ID cannot be null';
END IF;
DELETE FROM platform_schema.dependent_expense_field_mappings
WHERE expense_field_id IN (
SELECT id
FROM platform_schema.expense_fields
WHERE org_id = _org_id
AND type = 'DEPENDENT_SELECT'
);
DELETE FROM platform_schema.expense_fields
WHERE org_id = _org_id
AND type = 'DEPENDENT_SELECT';
END;
$$ LANGUAGE plpgsql;Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
scripts/sql/reset_db.sql (1)
206-208: Improve notice readability and provide contextThe notice could be more informative and better formatted:
- Explain why these steps need to be run in prod
- Consider using a single
E'\n'with fewer newlines- Add a warning about the impact of these deletions
- RAISE NOTICE E'\n\n\n\n\n\n\n\n\nSwitch to prod db and run the below queries to delete dependent fields + RAISE NOTICE E'\n\n---IMPORTANT---\nThe following cleanup steps must be run in the production database to remove dependent expense fields and their mappings.\nWarning: This will permanently delete field configurations.\n'Also applies to: 215-215
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
scripts/sql/reset_db.sql(1 hunks)
🔇 Additional comments (1)
scripts/sql/reset_db.sql (1)
Line range hint 1-216: Verify deletion order and table coverage
The function deletes data from multiple related tables. Let's verify:
- The deletion order respects foreign key constraints
- All related tables are included
- No orphaned records are left behind
✅ Verification successful
The deletion order in the function appears correct and comprehensive
The delete_workspace function follows a proper deletion order that respects referential integrity:
- Starts with dependent tables (errors, export details, summaries) that don't have child references
- Proceeds with transaction-related tables (expenses, invoices) and their child records
- Handles mapping and settings tables that depend on workspace
- Removes credential and attribute tables
- Finally cleans up user associations and the workspace itself
The deletion sequence is well-structured to prevent foreign key violations, and the function includes comprehensive logging of deleted records. The additional cleanup of dependent fields in the platform schema is also properly handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify table relationships and deletion order
# Find all tables referenced in foreign key constraints
psql -c "\d+" | grep -A1 "Foreign-key constraints:" | grep "REFERENCES" | sort -u
# List all tables in the database for comparison
psql -c "\dt"
# Check for any existing foreign key violations
psql -c "SELECT conname, conrelid::regclass, confrelid::regclass
FROM pg_constraint
WHERE confrelid::regclass::text LIKE '%workspaces%'
OR conrelid::regclass::text LIKE '%workspaces%'"
Length of output: 612
Script:
#!/bin/bash
# Let's analyze the SQL file to understand table relationships through CREATE TABLE statements
# and foreign key declarations
# Search for CREATE TABLE statements
echo "=== Table Definitions ==="
rg "CREATE TABLE" -A 10 --type sql
# Search for FOREIGN KEY constraints
echo -e "\n=== Foreign Key Constraints ==="
rg "FOREIGN KEY" --type sql
# Search for references to workspace_id to ensure we haven't missed any tables
echo -e "\n=== Tables with workspace_id ==="
rg "workspace_id" --type sql | grep -i "create table\|alter table"
Length of output: 354
Script:
#!/bin/bash
# Let's analyze the delete_workspace function implementation to understand the deletion order
# and table dependencies by looking at the file content
echo "=== Analyzing delete_workspace function ==="
cat scripts/sql/reset_db.sql
# Also search for any other SQL files that might contain table definitions
echo -e "\n=== Other SQL files ==="
fd -e sql --type f
Length of output: 6791
scripts/sql/reset_db.sql
Outdated
| delete from platform_schema.dependent_expense_field_mappings where expense_field_id in (select id from platform_schema.expense_fields where org_id =''%'' and type=''DEPENDENT_SELECT''); | ||
|
|
||
| delete from platform_schema.expense_fields where org_id = ''%'' and type = ''DEPENDENT_SELECT'';\n\n\n\n\n\n\n\n\n\n\n', _org_id, _org_id; |
There was a problem hiding this comment.
Security: Fix SQL injection vulnerability and improve architecture
The current implementation has several issues:
- Using string formatting for SQL queries is vulnerable to SQL injection
- Manual execution in production is error-prone
- No validation of
_org_idbefore use
Consider creating a separate function in the production database to handle this cleanup:
CREATE OR REPLACE FUNCTION cleanup_dependent_fields(IN _org_id varchar) RETURNS void AS $$
BEGIN
IF _org_id IS NULL THEN
RAISE EXCEPTION 'Organization ID cannot be null';
END IF;
DELETE FROM platform_schema.dependent_expense_field_mappings
WHERE expense_field_id IN (
SELECT id
FROM platform_schema.expense_fields
WHERE org_id = _org_id
AND type = 'DEPENDENT_SELECT'
);
DELETE FROM platform_schema.expense_fields
WHERE org_id = _org_id
AND type = 'DEPENDENT_SELECT';
END;
$$ LANGUAGE plpgsql;Then modify the notice to reference this function:
- delete from platform_schema.dependent_expense_field_mappings where expense_field_id in (select id from platform_schema.expense_fields where org_id =''%'' and type=''DEPENDENT_SELECT'');
- delete from platform_schema.expense_fields where org_id = ''%'' and type = ''DEPENDENT_SELECT'';\n\n\n\n\n\n\n\n\n\n\n'
+ SELECT cleanup_dependent_fields(''%'');\n\n'
scripts/sql/reset_db.sql
Outdated
| rollback; | ||
| begin; |
There was a problem hiding this comment.
Critical: Remove incorrect transaction control statements
The ROLLBACK statement will undo all the previous deletion operations, and the new BEGIN serves no purpose as it only wraps a NOTICE statement. These statements should be removed.
- rollback;
- begin;
scripts/sql/reset_db.sql
Outdated
| @@ -200,6 +200,20 @@ | |||
| WHERE w.id = _workspace_id; | |||
| GET DIAGNOSTICS rcount = ROW_COUNT; | |||
| RAISE NOTICE 'Deleted % workspaces', rcount; | |||
|
|
|||
| _org_id := (SELECT fyle_org_id FROM workspaces WHERE id = _workspace_id); | |||
There was a problem hiding this comment.
Critical: Variable assignment will always return NULL
The _org_id assignment happens after the workspaces table has been deleted (see line 202), making this SELECT statement always return NULL. Move this assignment to the beginning of the function, right after the variable declarations.
DECLARE
rcount integer;
_org_id varchar(255);
BEGIN
+ _org_id := (SELECT fyle_org_id FROM workspaces WHERE id = _workspace_id);
RAISE NOTICE 'Deleting data from workspace % ', _workspace_id;Committable suggestion skipped: line range outside the PR's diff.
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
* fix: dependent field queries not printing the org id * fix: delete `expense_attributes_deletion_cache` while workspace reset (#232)
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Clickup
app.clickup.com
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes