Skip to content

Commit def8484

Browse files
keegancsmithkeegancsmith
authoredJan 6, 2020
db: remove Repos.Upsert (sourcegraph#7484)
The repos table is only mutated by repo-updater. All call sites for Repos.Upsert are either in tests or unused REST API endpoints. For tests I moved the upsert code into a _test.go file so it is still available for tests. For the unused API I grepped our org for calls to it, and it is unused.
·
v3.15.0v3.12.0-rc.1
1 parent 12b5745 commit def8484

File tree

8 files changed

+103
-151
lines changed

8 files changed

+103
-151
lines changed
 

‎cmd/frontend/backend/repos.go

-4
Original file line numberDiff line numberDiff line change
@@ -116,10 +116,6 @@ func (s *repos) Add(ctx context.Context, name api.RepoName) (err error) {
116116
return err
117117
}
118118

119-
func (s *repos) Upsert(ctx context.Context, op api.InsertRepoOp) error {
120-
return db.Repos.Upsert(ctx, op)
121-
}
122-
123119
func (s *repos) List(ctx context.Context, opt db.ReposListOptions) (repos []*types.Repo, err error) {
124120
if Mocks.Repos.List != nil {
125121
return Mocks.Repos.List(ctx, opt)

‎cmd/frontend/db/repos.go

-104
Original file line numberDiff line numberDiff line change
@@ -603,107 +603,3 @@ func allMatchingStrings(re *regexpsyntax.Regexp) (exact, contains, prefix, suffi
603603

604604
return nil, nil, nil, nil, nil
605605
}
606-
607-
const upsertSQL = `
608-
WITH upsert AS (
609-
UPDATE repo
610-
SET
611-
name = $1,
612-
description = $2,
613-
fork = $3,
614-
enabled = $4,
615-
external_id = NULLIF(BTRIM($5), ''),
616-
external_service_type = NULLIF(BTRIM($6), ''),
617-
external_service_id = NULLIF(BTRIM($7), ''),
618-
archived = $9
619-
WHERE name = $1 OR (
620-
external_id IS NOT NULL
621-
AND external_service_type IS NOT NULL
622-
AND external_service_id IS NOT NULL
623-
AND NULLIF(BTRIM($5), '') IS NOT NULL
624-
AND NULLIF(BTRIM($6), '') IS NOT NULL
625-
AND NULLIF(BTRIM($7), '') IS NOT NULL
626-
AND external_id = NULLIF(BTRIM($5), '')
627-
AND external_service_type = NULLIF(BTRIM($6), '')
628-
AND external_service_id = NULLIF(BTRIM($7), '')
629-
)
630-
RETURNING repo.name
631-
)
632-
633-
INSERT INTO repo (
634-
name,
635-
description,
636-
fork,
637-
language,
638-
enabled,
639-
external_id,
640-
external_service_type,
641-
external_service_id,
642-
archived
643-
) (
644-
SELECT
645-
$1 AS name,
646-
$2 AS description,
647-
$3 AS fork,
648-
$8 AS language,
649-
$4 AS enabled,
650-
NULLIF(BTRIM($5), '') AS external_id,
651-
NULLIF(BTRIM($6), '') AS external_service_type,
652-
NULLIF(BTRIM($7), '') AS external_service_id,
653-
$9 AS archived
654-
WHERE NOT EXISTS (SELECT 1 FROM upsert)
655-
)`
656-
657-
// Upsert updates the repository if it already exists (keyed on name) and
658-
// inserts it if it does not.
659-
//
660-
// If repo exists, op.Enabled is ignored.
661-
func (s *repos) Upsert(ctx context.Context, op api.InsertRepoOp) error {
662-
if Mocks.Repos.Upsert != nil {
663-
return Mocks.Repos.Upsert(op)
664-
}
665-
666-
insert := false
667-
language := ""
668-
enabled := op.Enabled
669-
670-
// We optimistically assume the repo is already in the table, so first
671-
// check if it is. We then fallback to the upsert functionality. The
672-
// upsert is logged as a modification to the DB, even if it is a no-op. So
673-
// we do this check to avoid log spam if postgres is configured with
674-
// log_statement='mod'.
675-
r, err := s.GetByName(ctx, op.Name)
676-
if err != nil {
677-
if _, ok := err.(*repoNotFoundErr); !ok {
678-
return err
679-
}
680-
insert = true // missing
681-
} else {
682-
enabled = true
683-
language = r.Language
684-
// Ignore Enabled for deciding to update
685-
insert = (op.Description != r.Description) ||
686-
(op.Fork != r.Fork) ||
687-
(!op.ExternalRepo.Equal(&r.ExternalRepo))
688-
}
689-
690-
if !insert {
691-
return nil
692-
}
693-
694-
_, err = dbconn.Global.ExecContext(
695-
ctx,
696-
upsertSQL,
697-
op.Name,
698-
op.Description,
699-
op.Fork,
700-
enabled,
701-
op.ExternalRepo.ID,
702-
op.ExternalRepo.ServiceType,
703-
op.ExternalRepo.ServiceID,
704-
language,
705-
op.Archived,
706-
)
707-
708-
return err
709-
}

‎cmd/frontend/db/repos_db_test.go

+103
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,109 @@ func (s *repos) Delete(ctx context.Context, repo api.RepoID) error {
6969
return err
7070
}
7171

72+
const upsertSQL = `
73+
WITH upsert AS (
74+
UPDATE repo
75+
SET
76+
name = $1,
77+
description = $2,
78+
fork = $3,
79+
enabled = $4,
80+
external_id = NULLIF(BTRIM($5), ''),
81+
external_service_type = NULLIF(BTRIM($6), ''),
82+
external_service_id = NULLIF(BTRIM($7), ''),
83+
archived = $9
84+
WHERE name = $1 OR (
85+
external_id IS NOT NULL
86+
AND external_service_type IS NOT NULL
87+
AND external_service_id IS NOT NULL
88+
AND NULLIF(BTRIM($5), '') IS NOT NULL
89+
AND NULLIF(BTRIM($6), '') IS NOT NULL
90+
AND NULLIF(BTRIM($7), '') IS NOT NULL
91+
AND external_id = NULLIF(BTRIM($5), '')
92+
AND external_service_type = NULLIF(BTRIM($6), '')
93+
AND external_service_id = NULLIF(BTRIM($7), '')
94+
)
95+
RETURNING repo.name
96+
)
97+
98+
INSERT INTO repo (
99+
name,
100+
description,
101+
fork,
102+
language,
103+
enabled,
104+
external_id,
105+
external_service_type,
106+
external_service_id,
107+
archived
108+
) (
109+
SELECT
110+
$1 AS name,
111+
$2 AS description,
112+
$3 AS fork,
113+
$8 AS language,
114+
$4 AS enabled,
115+
NULLIF(BTRIM($5), '') AS external_id,
116+
NULLIF(BTRIM($6), '') AS external_service_type,
117+
NULLIF(BTRIM($7), '') AS external_service_id,
118+
$9 AS archived
119+
WHERE NOT EXISTS (SELECT 1 FROM upsert)
120+
)`
121+
122+
// Upsert updates the repository if it already exists (keyed on name) and
123+
// inserts it if it does not.
124+
//
125+
// If repo exists, op.Enabled is ignored.
126+
//
127+
// Upsert exists for testing purposes only. Repository mutations are managed
128+
// by repo-updater.
129+
func (s *repos) Upsert(ctx context.Context, op api.InsertRepoOp) error {
130+
insert := false
131+
language := ""
132+
enabled := op.Enabled
133+
134+
// We optimistically assume the repo is already in the table, so first
135+
// check if it is. We then fallback to the upsert functionality. The
136+
// upsert is logged as a modification to the DB, even if it is a no-op. So
137+
// we do this check to avoid log spam if postgres is configured with
138+
// log_statement='mod'.
139+
r, err := s.GetByName(ctx, op.Name)
140+
if err != nil {
141+
if _, ok := err.(*repoNotFoundErr); !ok {
142+
return err
143+
}
144+
insert = true // missing
145+
} else {
146+
enabled = true
147+
language = r.Language
148+
// Ignore Enabled for deciding to update
149+
insert = (op.Description != r.Description) ||
150+
(op.Fork != r.Fork) ||
151+
(!op.ExternalRepo.Equal(&r.ExternalRepo))
152+
}
153+
154+
if !insert {
155+
return nil
156+
}
157+
158+
_, err = dbconn.Global.ExecContext(
159+
ctx,
160+
upsertSQL,
161+
op.Name,
162+
op.Description,
163+
op.Fork,
164+
enabled,
165+
op.ExternalRepo.ID,
166+
op.ExternalRepo.ServiceType,
167+
op.ExternalRepo.ServiceID,
168+
language,
169+
op.Archived,
170+
)
171+
172+
return err
173+
}
174+
72175
/*
73176
* Tests
74177
*/

‎cmd/frontend/db/repos_mock.go

-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ type MockRepos struct {
1414
GetByName func(ctx context.Context, repo api.RepoName) (*types.Repo, error)
1515
List func(v0 context.Context, v1 ReposListOptions) ([]*types.Repo, error)
1616
Count func(ctx context.Context, opt ReposListOptions) (int, error)
17-
Upsert func(api.InsertRepoOp) error
1817
}
1918

2019
func (s *MockRepos) MockGet(t *testing.T, wantRepo api.RepoID) (called *bool) {

‎cmd/frontend/internal/httpapi/httpapi.go

-1
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,6 @@ func NewInternalHandler(m *mux.Router, schema *graphql.Schema) http.Handler {
101101
m.Get(apirouter.ExternalServiceConfigs).Handler(trace.TraceRoute(handler(serveExternalServiceConfigs)))
102102
m.Get(apirouter.ExternalServicesList).Handler(trace.TraceRoute(handler(serveExternalServicesList)))
103103
m.Get(apirouter.PhabricatorRepoCreate).Handler(trace.TraceRoute(handler(servePhabricatorRepoCreate)))
104-
m.Get(apirouter.ReposCreateIfNotExists).Handler(trace.TraceRoute(handler(serveReposCreateIfNotExists)))
105104
reposList := &reposListServer{
106105
SourcegraphDotComMode: envvar.SourcegraphDotComMode(),
107106
Repos: backend.Repos,

‎cmd/frontend/internal/httpapi/internal.go

-30
Original file line numberDiff line numberDiff line change
@@ -35,36 +35,6 @@ func serveReposGetByName(w http.ResponseWriter, r *http.Request) error {
3535
return nil
3636
}
3737

38-
func serveReposCreateIfNotExists(w http.ResponseWriter, r *http.Request) error {
39-
var repo api.RepoCreateOrUpdateRequest
40-
err := json.NewDecoder(r.Body).Decode(&repo)
41-
if err != nil {
42-
return err
43-
}
44-
err = backend.Repos.Upsert(r.Context(), api.InsertRepoOp{
45-
Name: repo.RepoName,
46-
Description: repo.Description,
47-
Fork: repo.Fork,
48-
Archived: repo.Archived,
49-
Enabled: repo.Enabled,
50-
ExternalRepo: repo.ExternalRepo,
51-
})
52-
if err != nil {
53-
return err
54-
}
55-
sgRepo, err := backend.Repos.GetByName(r.Context(), repo.RepoName)
56-
if err != nil {
57-
return err
58-
}
59-
data, err := json.Marshal(sgRepo)
60-
if err != nil {
61-
return err
62-
}
63-
w.WriteHeader(http.StatusOK)
64-
w.Write(data)
65-
return nil
66-
}
67-
6838
func servePhabricatorRepoCreate(w http.ResponseWriter, r *http.Request) error {
6939
var repo api.PhabricatorRepoCreateRequest
7040
err := json.NewDecoder(r.Body).Decode(&repo)

‎cmd/frontend/internal/httpapi/router/router.go

-2
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ const (
3535
GitResolveRevision = "internal.git.resolve-revision"
3636
GitTar = "internal.git.tar"
3737
PhabricatorRepoCreate = "internal.phabricator.repo.create"
38-
ReposCreateIfNotExists = "internal.repos.create-if-not-exists"
3938
ReposGetByName = "internal.repos.get-by-name"
4039
ReposInventoryUncached = "internal.repos.inventory-uncached"
4140
ReposInventory = "internal.repos.inventory"
@@ -101,7 +100,6 @@ func NewInternal(base *mux.Router) *mux.Router {
101100
base.Path("/phabricator/repo-create").Methods("POST").Name(PhabricatorRepoCreate)
102101
base.Path("/external-services/configs").Methods("POST").Name(ExternalServiceConfigs)
103102
base.Path("/external-services/list").Methods("POST").Name(ExternalServicesList)
104-
base.Path("/repos/create-if-not-exists").Methods("POST").Name(ReposCreateIfNotExists)
105103
base.Path("/repos/inventory-uncached").Methods("POST").Name(ReposInventoryUncached)
106104
base.Path("/repos/inventory").Methods("POST").Name(ReposInventory)
107105
base.Path("/repos/list").Methods("POST").Name(ReposList)

‎internal/api/internal_client.go

-9
Original file line numberDiff line numberDiff line change
@@ -245,15 +245,6 @@ func (c *internalClient) SendEmail(ctx context.Context, message txtypes.Message)
245245
return c.postInternal(ctx, "send-email", &message, nil)
246246
}
247247

248-
func (c *internalClient) ReposCreateIfNotExists(ctx context.Context, op RepoCreateOrUpdateRequest) (*Repo, error) {
249-
var repo Repo
250-
err := c.postInternal(ctx, "repos/create-if-not-exists", op, &repo)
251-
if err != nil {
252-
return nil, err
253-
}
254-
return &repo, nil
255-
}
256-
257248
// ReposListEnabled returns a list of all enabled repository names.
258249
func (c *internalClient) ReposListEnabled(ctx context.Context) ([]RepoName, error) {
259250
var names []RepoName

0 commit comments

Comments
 (0)
Please sign in to comment.