-
Notifications
You must be signed in to change notification settings - Fork 18
fix: reconcile service_keys.identity_id FK to ON DELETE CASCADE on legacy DBs #252
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
Open
safayavatsal
wants to merge
2
commits into
highflame-ai:main
Choose a base branch
from
safayavatsal:fix/service-keys-fk-cascade-196
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| -- 040_service_keys_fk_cascade.down.sql | ||
| -- Reverses 040_service_keys_fk_cascade.up.sql | ||
| -- Flips the FK back to ON DELETE NO ACTION (the implicit default when no | ||
| -- ON DELETE clause is specified — the presumed pre-migration-006 legacy | ||
| -- shape). Verify against a dev1 snapshot before relying on this in a real | ||
| -- rollback; if the legacy constraint turns out to have had a different | ||
| -- ON DELETE behaviour, update this file to match before use. | ||
|
|
||
| ALTER TABLE service_keys | ||
| DROP CONSTRAINT IF EXISTS service_keys_identity_id_fkey; | ||
|
|
||
| ALTER TABLE service_keys | ||
| ADD CONSTRAINT service_keys_identity_id_fkey | ||
| FOREIGN KEY (identity_id) REFERENCES identities(id) ON DELETE NO ACTION; |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| -- 040_service_keys_fk_cascade.up.sql | ||
| -- Reconcile service_keys.identity_id FK to match migration 006's declared | ||
| -- ON DELETE CASCADE on legacy deployments where CREATE TABLE IF NOT EXISTS | ||
| -- was a no-op (the table predates the cascade and the constraint never got | ||
| -- re-applied). Fresh deployments are a no-op (constraint flip to identical | ||
| -- shape). See highflame-authn#109 / zeroid#187 for the user-visible incident | ||
| -- this drift originally produced, and zeroid#196 for the schema-drift root | ||
| -- cause this migration closes. | ||
|
|
||
| ALTER TABLE service_keys | ||
| DROP CONSTRAINT IF EXISTS service_keys_identity_id_fkey; | ||
|
|
||
| ALTER TABLE service_keys | ||
| ADD CONSTRAINT service_keys_identity_id_fkey | ||
| FOREIGN KEY (identity_id) REFERENCES identities(id) ON DELETE CASCADE; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| package integration_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/highflame-ai/zeroid/domain" | ||
| ) | ||
|
|
||
| // TestServiceKeysFKCascade_HardDeleteIdentity_CascadesKey is regression | ||
| // coverage for zeroid#196: migration 006 declares | ||
| // service_keys.identity_id REFERENCES identities(id) ON DELETE CASCADE, but | ||
| // CREATE TABLE IF NOT EXISTS is a no-op on any deployment where the table | ||
| // pre-existed the cascade — leaving legacy DBs with a non-cascading FK (the | ||
| // root cause of highflame-authn#109 / zeroid#187). Migration | ||
| // 040_service_keys_fk_cascade explicitly re-applies the cascade on every | ||
| // startup so all deployments converge on the declared shape. | ||
| // | ||
| // This test proves the FK itself (not just the current lucky call-ordering | ||
| // inside PurgeIdentity, which today deletes the identity before any service | ||
| // key row exists) permits a hard delete of a service-key-bearing identity to | ||
| // cascade cleanly. That's the invariant PurgeIdentity's compensating | ||
| // rollback silently depends on, and that any future hard-delete caller | ||
| // (e.g. a GDPR-erasure path) will need too. | ||
| func TestServiceKeysFKCascade_HardDeleteIdentity_CascadesKey(t *testing.T) { | ||
| ctx := context.Background() | ||
| reg := registerAgent(t, uid("fk-cascade")) // auto-creates a bootstrap service key | ||
|
|
||
| // Precondition: exactly one service key row references this identity. | ||
| n, err := testDB.NewSelect(). | ||
| Model((*domain.APIKey)(nil)). | ||
| Where("identity_id = ?", reg.AgentID). | ||
| Count(ctx) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 1, n, "precondition: registerAgent must create exactly one service key") | ||
|
|
||
| // Hard-delete the identity directly at the repo layer — the same | ||
| // DELETE FROM identities ... that IdentityRepository.Delete (and | ||
| // therefore PurgeIdentity) issues. Before the fix, this trips "violates | ||
| // foreign key constraint service_keys_identity_id_fkey" on any DB where | ||
| // the table predates the cascade. | ||
| _, err = testDB.NewDelete(). | ||
| Model((*domain.Identity)(nil)). | ||
| Where("id = ?", reg.AgentID). | ||
| Exec(ctx) | ||
| require.NoError(t, err, "hard delete of a service-key-bearing identity must cascade, not FK-violate") | ||
|
|
||
| // The service key must have been cascade-deleted along with its parent. | ||
| n, err = testDB.NewSelect(). | ||
| Model((*domain.APIKey)(nil)). | ||
| Where("identity_id = ?", reg.AgentID). | ||
| Count(ctx) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, 0, n, "service_keys row must cascade-delete with its parent identity") | ||
| } |
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.
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.
Missing transaction safety
While both operations target the same table, running DDL statements without an explicit transaction block could leave the table in an inconsistent state if the
ADD CONSTRAINTfails after theDROPsucceeds (e.g., ifidentitiestable is locked or validation finds violations). Wrap inBEGIN; ... COMMIT;to ensure atomicity.Suggested fix: