Skip to content

Fast external service deletion

Administrator requested to merge core/fast-external-service-deletion into main

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 be SELECT 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))
     }
 })
}

Merge request reports

Loading