[management] add domain and service cleanup migration#5850
Merged
pascal-fischer merged 2 commits intomainfrom Apr 11, 2026
Merged
[management] add domain and service cleanup migration#5850pascal-fischer merged 2 commits intomainfrom
pascal-fischer merged 2 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a generic migration Changes
Sequence Diagram(s)sequenceDiagram
actor Migrator
participant Cleanup as CleanupOrphanedResources
participant Schema as SchemaParser
participant FK as FKDetector
participant DB as Database
Migrator->>Cleanup: Invoke cleanup(child T, parent R, fkColumn)
Cleanup->>DB: Check child table exists
alt child table missing
Cleanup-->>Migrator: return (no-op)
else
Cleanup->>DB: Check parent table exists
alt parent table missing
Cleanup-->>Migrator: return (no-op)
else
Cleanup->>Schema: Parse child & parent models -> table names
Cleanup->>Schema: Verify child has fkColumn
alt fkColumn missing
Cleanup-->>Migrator: return (error)
else
Cleanup->>FK: hasForeignKey(db, childTable, fkColumn)?
alt FK exists
Cleanup-->>Migrator: return (no-op)
else
Cleanup->>DB: DELETE FROM child WHERE fkColumn NOT IN (SELECT id FROM parent)
Cleanup-->>Migrator: return (success)
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/migration/migration_test.go`:
- Around line 594-620: The test
TestCleanupOrphanedResources_SkipsWhenForeignKeyExists executes PostgreSQL-only
DDL (DROP CONSTRAINT, NOT VALID) but setupDatabase() defaults to SQLite, causing
failures on non-Postgres backends; gate the test to run only for Postgres by
detecting the store engine and skipping otherwise (e.g. check
os.Getenv("NETBIRD_STORE_ENGINE") == "postgres" or inspect db.Dialector().Name()
after calling setupDatabase(t)), and call t.Skipf with a short message before
running any Postgres-specific SQL so the rest of the test (the ALTER/NOT VALID
statements and migration.CleanupOrphanedResources[testChildWithFK, testParent])
only runs on Postgres.
In `@management/server/migration/migration.go`:
- Around line 493-529: Update hasForeignKey to return (bool, error) and
propagate DB errors instead of swallowing them: change the signature of
hasForeignKey(db *gorm.DB, table, column string) to hasForeignKey(db *gorm.DB,
table, column string) (bool, error), capture and check the error returned by
each db.Raw(...).Scan(...) call (for postgres/mysql and for sqlite where you
call db.Raw(fmt.Sprintf("PRAGMA foreign_key_list(%s)", table)).Scan(&fks)) and
return false with that error on failure; for sqlite still inspect fkInfo.From
entries and return (true, nil) when found or (false, nil) when none found; then
update callers (e.g., CleanupOrphanedResources) to handle the error return and
abort/propagate instead of assuming false.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23c5f1cd-0b45-4544-acae-61ec8347f380
📒 Files selected for processing (3)
management/server/migration/migration.gomanagement/server/migration/migration_test.gomanagement/server/store/store.go
Comment on lines
+493
to
+529
| func hasForeignKey(db *gorm.DB, table, column string) bool { | ||
| var count int64 | ||
|
|
||
| switch db.Name() { | ||
| case "postgres": | ||
| db.Raw(` | ||
| SELECT COUNT(*) FROM information_schema.key_column_usage kcu | ||
| JOIN information_schema.table_constraints tc | ||
| ON tc.constraint_name = kcu.constraint_name | ||
| AND tc.table_schema = kcu.table_schema | ||
| WHERE tc.constraint_type = 'FOREIGN KEY' | ||
| AND kcu.table_name = ? | ||
| AND kcu.column_name = ? | ||
| `, table, column).Scan(&count) | ||
| case "mysql": | ||
| db.Raw(` | ||
| SELECT COUNT(*) FROM information_schema.key_column_usage | ||
| WHERE table_schema = DATABASE() | ||
| AND table_name = ? | ||
| AND column_name = ? | ||
| AND referenced_table_name IS NOT NULL | ||
| `, table, column).Scan(&count) | ||
| default: // sqlite | ||
| type fkInfo struct { | ||
| From string | ||
| } | ||
| var fks []fkInfo | ||
| db.Raw(fmt.Sprintf("PRAGMA foreign_key_list(%s)", table)).Scan(&fks) | ||
| for _, fk := range fks { | ||
| if fk.From == column { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| return count > 0 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the hasForeignKey function and surrounding context
rg -n "func hasForeignKey" management/server/migration/ -A 50Repository: netbirdio/netbird
Length of output: 3828
🏁 Script executed:
# Also check for CleanupOrphanedResources to see how hasForeignKey is called
rg -n "CleanupOrphanedResources" management/server/migration/ -B 5 -A 20Repository: netbirdio/netbird
Length of output: 15120
🏁 Script executed:
# Check if hasForeignKey is called elsewhere in the codebase
rg -n "hasForeignKey" --type goRepository: netbirdio/netbird
Length of output: 397
🏁 Script executed:
# Get the context around the hasForeignKey call at line 567
sed -n '560,580p' management/server/migration/migration.goRepository: netbirdio/netbird
Length of output: 816
🏁 Script executed:
# Check if there's error checking after hasForeignKey call
sed -n '560,585p' management/server/migration/migration.goRepository: netbirdio/netbird
Length of output: 990
🏁 Script executed:
# Verify the exact error handling in hasForeignKey - check if Error is ever checked
sed -n '493,530p' management/server/migration/migration.go | grep -i "error\|\.Error"Repository: netbirdio/netbird
Length of output: 43
Change hasForeignKey to return (bool, error) so metadata lookup failures abort instead of silently allowing data loss.
hasForeignKey() ignores .Scan().Error for every dialect. If metadata introspection fails (connection issue, permission denial, SQL syntax error), the function returns false, and CleanupOrphanedResources() proceeds with the destructive DELETE as if no FK existed. Return (bool, error) and have the caller check and propagate errors.
Proposed fix
-func hasForeignKey(db *gorm.DB, table, column string) bool {
+func hasForeignKey(db *gorm.DB, table, column string) (bool, error) {
var count int64
switch db.Name() {
case "postgres":
- db.Raw(`
+ if err := db.Raw(`
SELECT COUNT(*) FROM information_schema.key_column_usage kcu
JOIN information_schema.table_constraints tc
ON tc.constraint_name = kcu.constraint_name
AND tc.table_schema = kcu.table_schema
WHERE tc.constraint_type = 'FOREIGN KEY'
AND kcu.table_name = ?
AND kcu.column_name = ?
- `, table, column).Scan(&count)
+ `, table, column).Scan(&count).Error; err != nil {
+ return false, fmt.Errorf("inspect foreign keys for %s.%s: %w", table, column, err)
+ }
case "mysql":
- db.Raw(`
+ if err := db.Raw(`
SELECT COUNT(*) FROM information_schema.key_column_usage
WHERE table_schema = DATABASE()
AND table_name = ?
AND column_name = ?
AND referenced_table_name IS NOT NULL
- `, table, column).Scan(&count)
+ `, table, column).Scan(&count).Error; err != nil {
+ return false, fmt.Errorf("inspect foreign keys for %s.%s: %w", table, column, err)
+ }
default: // sqlite
type fkInfo struct {
From string
}
var fks []fkInfo
- db.Raw(fmt.Sprintf("PRAGMA foreign_key_list(%s)", table)).Scan(&fks)
+ if err := db.Raw(fmt.Sprintf("PRAGMA foreign_key_list(%s)", table)).Scan(&fks).Error; err != nil {
+ return false, fmt.Errorf("inspect foreign keys for %s.%s: %w", table, column, err)
+ }
for _, fk := range fks {
if fk.From == column {
- return true
+ return true, nil
}
}
- return false
+ return false, nil
}
- return count > 0
+ return count > 0, nil
}- if hasForeignKey(db, childTable, fkColumn) {
+ hasFK, err := hasForeignKey(db, childTable, fkColumn)
+ if err != nil {
+ return err
+ }
+ if hasFK {
log.WithContext(ctx).Debugf("foreign key constraint for %s already exists on %s, no cleanup needed", fkColumn, childTable)
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/migration/migration.go` around lines 493 - 529, Update
hasForeignKey to return (bool, error) and propagate DB errors instead of
swallowing them: change the signature of hasForeignKey(db *gorm.DB, table,
column string) to hasForeignKey(db *gorm.DB, table, column string) (bool,
error), capture and check the error returned by each db.Raw(...).Scan(...) call
(for postgres/mysql and for sqlite where you call db.Raw(fmt.Sprintf("PRAGMA
foreign_key_list(%s)", table)).Scan(&fks)) and return false with that error on
failure; for sqlite still inspect fkInfo.From entries and return (true, nil)
when found or (false, nil) when none found; then update callers (e.g.,
CleanupOrphanedResources) to handle the error return and abort/propagate instead
of assuming false.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/migration/migration_test.go`:
- Around line 621-628: The FOREIGN_KEY_CHECKS toggles are session-scoped so
calling db.Exec separately can hit different connections; wrap the sequence in a
single pinned connection (use db.Transaction or db.Begin) and run all Exec calls
inside that transaction to ensure they execute on the same connection.
Concretely, replace the separate db.Exec calls with db.Transaction(func(tx
*gorm.DB) error { tx.Exec("SET FOREIGN_KEY_CHECKS = 0"); tx.Exec("ALTER TABLE
test_children DROP FOREIGN KEY fk_test_children_parent"); tx.Exec("DELETE FROM
test_parents WHERE id = ?", "p2"); tx.Exec("ALTER TABLE test_children ADD
CONSTRAINT fk_test_children_parent FOREIGN KEY (parent_id) REFERENCES
test_parents(id)"); tx.Exec("SET FOREIGN_KEY_CHECKS = 1"); return tx.Error })
(or use db.DB().Conn(ctx) and exec on the returned *sql.Conn) so all statements
run on the same connection.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 893ab728-0979-4f87-9230-6843edfd700b
📒 Files selected for processing (1)
management/server/migration/migration_test.go
Comment on lines
+621
to
+628
| require.NoError(t, db.Exec("SET FOREIGN_KEY_CHECKS = 0").Error) | ||
| require.NoError(t, db.Exec("ALTER TABLE test_children DROP FOREIGN KEY fk_test_children_parent").Error) | ||
| require.NoError(t, db.Exec("DELETE FROM test_parents WHERE id = ?", "p2").Error) | ||
| require.NoError(t, db.Exec( | ||
| "ALTER TABLE test_children ADD CONSTRAINT fk_test_children_parent "+ | ||
| "FOREIGN KEY (parent_id) REFERENCES test_parents(id)", | ||
| ).Error) | ||
| require.NoError(t, db.Exec("SET FOREIGN_KEY_CHECKS = 1").Error) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current MySQL FK test uses unpinned sequential Exec calls.
rg -n -C3 'SET FOREIGN_KEY_CHECKS|DROP FOREIGN KEY|ADD CONSTRAINT fk_test_children_parent' management/server/migration/migration_test.go
rg -n -C2 'Transaction\\(|\\.DB\\(\\)|Conn\\(' management/server/migration/migration_test.goRepository: netbirdio/netbird
Length of output: 1260
🌐 Web query:
In MySQL, is FOREIGN_KEY_CHECKS session-scoped, and in Go database/sql can consecutive Exec calls on *sql.DB run on different pooled connections unless pinned via sql.Conn or a transaction?
💡 Result:
Yes, FOREIGN_KEY_CHECKS is session-scoped in MySQL. It can be set per session with SET foreign_key_checks=0 (or 1), affecting only that connection/session. There is also a global scope (SET GLOBAL), but session settings override for the current connection, and new sessions inherit from global. Consecutive Exec calls on *sql.DB can run on different pooled connections, as *sql.DB manages a connection pool and each call acquires a connection from the pool (possibly different ones sequentially). To pin to the same connection, use sql.Conn (via db.Conn(ctx)) or a transaction (tx := db.BeginTx(ctx, nil)), where operations are guaranteed on the same connection.
Citations:
- 1: https://dev.mysql.com/doc/refman/en/server-system-variable-reference.html
- 2: https://stackoverflow.com/questions/8538636/does-mysql-foreign-key-checks-affect-the-entire-database
- 3: https://oneuptime.com/blog/post/2026-03-31-mysql-session-variables-vs-global-variables/view
- 4: https://dev.mysql.com/doc/refman/8.1/en/server-system-variables.html
- 5: https://dev.mysql.com/doc/refman/8.4/en/create-table-foreign-keys.html
- 6: https://go.dev/doc/database/manage-connections
- 7: https://pkg.go.dev/database/sql@go1.24.3
- 8: https://aloksinhanov.medium.com/the-db-connection-confusion-in-go-d48574c399c5
- 9: How to guarantee to reuse the same connection? go-sql-driver/mysql#373
Pin MySQL FK-check statements to one connection to avoid flaky behavior.
FOREIGN_KEY_CHECKS is session-scoped in MySQL. Each db.Exec() call on *sql.DB can acquire a different connection from the pool, making it nondeterministic whether checks remain disabled across all statements on lines 621–628.
Suggested fix (pin to a dedicated SQL connection for the mysql branch)
case "mysql":
- require.NoError(t, db.Exec("SET FOREIGN_KEY_CHECKS = 0").Error)
- require.NoError(t, db.Exec("ALTER TABLE test_children DROP FOREIGN KEY fk_test_children_parent").Error)
- require.NoError(t, db.Exec("DELETE FROM test_parents WHERE id = ?", "p2").Error)
- require.NoError(t, db.Exec(
- "ALTER TABLE test_children ADD CONSTRAINT fk_test_children_parent "+
- "FOREIGN KEY (parent_id) REFERENCES test_parents(id)",
- ).Error)
- require.NoError(t, db.Exec("SET FOREIGN_KEY_CHECKS = 1").Error)
+ sqlDB, err := db.DB()
+ require.NoError(t, err)
+ conn, err := sqlDB.Conn(context.Background())
+ require.NoError(t, err)
+ t.Cleanup(func() {
+ _, _ = conn.ExecContext(context.Background(), "SET FOREIGN_KEY_CHECKS = 1")
+ _ = conn.Close()
+ })
+
+ _, err = conn.ExecContext(context.Background(), "SET FOREIGN_KEY_CHECKS = 0")
+ require.NoError(t, err)
+ _, err = conn.ExecContext(context.Background(), "ALTER TABLE test_children DROP FOREIGN KEY fk_test_children_parent")
+ require.NoError(t, err)
+ _, err = conn.ExecContext(context.Background(), "DELETE FROM test_parents WHERE id = ?", "p2")
+ require.NoError(t, err)
+ _, err = conn.ExecContext(
+ context.Background(),
+ "ALTER TABLE test_children ADD CONSTRAINT fk_test_children_parent "+
+ "FOREIGN KEY (parent_id) REFERENCES test_parents(id)",
+ )
+ require.NoError(t, err)
+ _, err = conn.ExecContext(context.Background(), "SET FOREIGN_KEY_CHECKS = 1")
+ require.NoError(t, err)
}📝 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
| require.NoError(t, db.Exec("SET FOREIGN_KEY_CHECKS = 0").Error) | |
| require.NoError(t, db.Exec("ALTER TABLE test_children DROP FOREIGN KEY fk_test_children_parent").Error) | |
| require.NoError(t, db.Exec("DELETE FROM test_parents WHERE id = ?", "p2").Error) | |
| require.NoError(t, db.Exec( | |
| "ALTER TABLE test_children ADD CONSTRAINT fk_test_children_parent "+ | |
| "FOREIGN KEY (parent_id) REFERENCES test_parents(id)", | |
| ).Error) | |
| require.NoError(t, db.Exec("SET FOREIGN_KEY_CHECKS = 1").Error) | |
| case "mysql": | |
| sqlDB, err := db.DB() | |
| require.NoError(t, err) | |
| conn, err := sqlDB.Conn(context.Background()) | |
| require.NoError(t, err) | |
| t.Cleanup(func() { | |
| _, _ = conn.ExecContext(context.Background(), "SET FOREIGN_KEY_CHECKS = 1") | |
| _ = conn.Close() | |
| }) | |
| _, err = conn.ExecContext(context.Background(), "SET FOREIGN_KEY_CHECKS = 0") | |
| require.NoError(t, err) | |
| _, err = conn.ExecContext(context.Background(), "ALTER TABLE test_children DROP FOREIGN KEY fk_test_children_parent") | |
| require.NoError(t, err) | |
| _, err = conn.ExecContext(context.Background(), "DELETE FROM test_parents WHERE id = ?", "p2") | |
| require.NoError(t, err) | |
| _, err = conn.ExecContext( | |
| context.Background(), | |
| "ALTER TABLE test_children ADD CONSTRAINT fk_test_children_parent "+ | |
| "FOREIGN KEY (parent_id) REFERENCES test_parents(id)", | |
| ) | |
| require.NoError(t, err) | |
| _, err = conn.ExecContext(context.Background(), "SET FOREIGN_KEY_CHECKS = 1") | |
| require.NoError(t, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/migration/migration_test.go` around lines 621 - 628, The
FOREIGN_KEY_CHECKS toggles are session-scoped so calling db.Exec separately can
hit different connections; wrap the sequence in a single pinned connection (use
db.Transaction or db.Begin) and run all Exec calls inside that transaction to
ensure they execute on the same connection. Concretely, replace the separate
db.Exec calls with db.Transaction(func(tx *gorm.DB) error { tx.Exec("SET
FOREIGN_KEY_CHECKS = 0"); tx.Exec("ALTER TABLE test_children DROP FOREIGN KEY
fk_test_children_parent"); tx.Exec("DELETE FROM test_parents WHERE id = ?",
"p2"); tx.Exec("ALTER TABLE test_children ADD CONSTRAINT fk_test_children_parent
FOREIGN KEY (parent_id) REFERENCES test_parents(id)"); tx.Exec("SET
FOREIGN_KEY_CHECKS = 1"); return tx.Error }) (or use db.DB().Conn(ctx) and exec
on the returned *sql.Conn) so all statements run on the same connection.
bcmmbaga
approved these changes
Apr 10, 2026
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.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Bug Fixes
Tests
Chores