Fast external service deletion
Created by: tsenart
TODO
- Fully test the Delete method of ExternalServiceStore. In a previous approach, I wrote some tests that should cover the core logic. Feel free to reuse some of this code below.
- Test deletion in a clone of the production database to see if this actually works. I'm concerned that the way we're disabling triggers for the rest of the transaction may not be supported in Cloud SQL due to us not having super user privileges. If that's the case, we may need to investigate other solutions.
- Find and fix stray raw
DELETE FROM external_service
queries to beSELECT delete_external_service
. Perhaps tests are OK to leave as they are. - Remove ability to delete through upsert, so that we force the usage of
SELECT delete_external_service
. - Ideally, but perhaps not now, prevent raw DELETE FROM external_service table via Postgres ROLEs?
- Later: See if it makes sense to apply the same technique here (i.e. pgpsql functions) to replace the trigger hell in repo and external_service_repos.
Fixes #18730
ExternalServiceStore.Delete test example
func testServiceDeleteExternalService(t *testing.T, store *repos.Store) func(*testing.T) {
ctx := context.Background()
return transact(ctx, store, func(t testing.TB, tx *repos.Store) {
t.Helper()
svc := repos.NewService(tx)
github1 := &types.ExternalService{Kind: extsvc.KindGitHub, DisplayName: "GitHub 1", Config: "{}"}
github2 := &types.ExternalService{Kind: extsvc.KindGitHub, DisplayName: "GitHub 2", Config: "{}"}
err := tx.ExternalServiceStore.Upsert(ctx, github1, github2)
if err != nil {
t.Fatal(err)
}
exclusive := &types.Repo{Name: "exclusive-repo-1", Sources: map[string]*types.SourceInfo{
github1.URN(): {ID: github1.URN()},
}}
shared := &types.Repo{Name: "shared-repo-1", Sources: map[string]*types.SourceInfo{
github1.URN(): {ID: github1.URN()},
github2.URN(): {ID: github2.URN()},
}}
err = tx.RepoStore.Create(ctx, exclusive, shared)
if err != nil {
t.Fatal(err)
}
err = svc.DeleteExternalService(ctx, github1.ID)
if err != nil {
t.Fatal(err)
}
// Only the repos exclusively owned by this external service should have been deleted.
_, err = tx.RepoStore.Get(ctx, exclusive.ID)
if have, want := err, (&database.RepoNotFoundErr{ID: exclusive.ID}); !cmp.Equal(have, want) {
t.Fatal("repo exclusively owned should have been deleted but wasn't")
}
have, err := tx.RepoStore.Get(ctx, shared.ID)
if err != nil {
t.Fatal(err)
}
delete(shared.Sources, github1.URN())
if want := shared; !cmp.Equal(have, want) {
t.Fatalf("shared repo diff: %v", cmp.Diff(have, want))
}
})
}