~bigbes/sr-ht-dolt

74d2612eac643b9e6e038e6dd1a3dda9b7940519 — Eugene Blikh 24 days ago 8e89f8a
fix(db): map repository_path_key to ErrNameTaken

CreateRepo only recognized uq_repo_owner_id_name as a name collision, but
path is derived from (owner, name) by RepoDiskPath, so a duplicate always
violates both indexes -- and repository_path_key, declared inline on the
column, has the lower OID and is the one Postgres reports. ErrNameTaken was
therefore unreachable in practice and every duplicate surfaced as a raw
23505, breaking all three callers that branch on it:

  - /internal/repos returned 500 "create database" instead of an idempotent
    200, so git.sr.ht's post-update hook printed "companion provisioning
    failed (500)" on every push to a repo whose companion already existed
  - the web create form returned 500 instead of 409 "You already have a
    database with that name."
  - a lost remotesapi auto-create race failed with codes.Unavailable
    instead of adopting the winner's row

TestCreateRepoDuplicateName missed it by re-creating under a different
path, which only the name index catches; it is now table-driven over both.
3 files changed, 45 insertions(+), 18 deletions(-)

M db/repos.go
M db/repos_test.go
M db/store.go
M db/repos.go => db/repos.go +14 -4
@@ 39,9 39,18 @@ func scanRepo(sc rowScanner) (*core.Repo, error) {
// unit of work and roll both back on failure. r.Name, r.OwnerID, r.Path and
// r.Visibility must be set; ID, created and updated are assigned here and the
// populated repo is returned (with OwnerName carried through from the input,
// which the caller already knows). A uq_repo_owner_id_name violation is mapped
// to ErrNameTaken; any other unique violation (e.g. duplicate path) is returned
// unwrapped so the caller sees the true cause.
// which the caller already knows).
//
// Both unique violations that a same-name re-create can raise are mapped to
// ErrNameTaken: uq_repo_owner_id_name and repository_path_key. The path index
// is redundant with (owner_id, name) — Path is always derived from the pair by
// RepoDiskPath — but it is declared inline on the column, so its index has the
// lower OID and Postgres reports *it* first. Matching only the named constraint
// therefore never fired in practice and every duplicate surfaced as a raw
// 23505: a 500 on /internal/repos for each push of an existing companion, a 500
// instead of 409 on the web create form, and a failed adopt on a lost
// remotesapi auto-create race. Any other unique violation is returned unwrapped
// so the caller still sees the true cause.
func (s *Store) CreateRepo(ctx context.Context, r *core.Repo) (*core.Repo, error) {
	now := time.Now().UTC()
	const q = `


@@ 58,7 67,8 @@ RETURNING id`
	if err != nil {
		var pqErr *pq.Error
		if errors.As(err, &pqErr) && pqErr.Code == "23505" &&
			pqErr.Constraint == "uq_repo_owner_id_name" {
			(pqErr.Constraint == "uq_repo_owner_id_name" ||
				pqErr.Constraint == "repository_path_key") {
			return nil, ErrNameTaken
		}
		return nil, fmt.Errorf("insert repository: %w", err)

M db/repos_test.go => db/repos_test.go +29 -13
@@ 36,20 36,36 @@ func TestCreateAndGetRepo(t *testing.T) {
	}
}

// TestCreateRepoDuplicateName covers both unique indexes a same-name re-create
// can trip. The "same path" case is the one production actually takes — Path is
// derived from (owner, name), so a real duplicate violates both indexes and
// Postgres reports the lower-OID repository_path_key. The "different path" case
// only reaches uq_repo_owner_id_name; it needs the repos root to have moved
// after the first row was written.
func TestCreateRepoDuplicateName(t *testing.T) {
	s, db, cleanup := newTestStore(t)
	defer cleanup()
	ctx := context.Background()

	insertUser(t, db, 1, "alice", core.UserTypeUser)
	mkRepo(t, s, ctx, 1, "alice", "widgets", core.VisibilityPublic)

	_, err := s.CreateRepo(ctx, &core.Repo{
		Name: "widgets", OwnerID: 1, OwnerName: "alice",
		Path: "/var/lib/dolt/~alice/widgets2", Visibility: core.VisibilityPrivate,
	})
	if !errors.Is(err, ErrNameTaken) {
		t.Fatalf("expected ErrNameTaken, got %v", err)
	for _, tc := range []struct {
		name string
		path string
	}{
		{"same path (both indexes, path reported)", "/var/lib/dolt/~alice/widgets"},
		{"different path (name index only)", "/srv/dolt/~alice/widgets"},
	} {
		t.Run(tc.name, func(t *testing.T) {
			s, db, cleanup := newTestStore(t)
			defer cleanup()
			ctx := context.Background()

			insertUser(t, db, 1, "alice", core.UserTypeUser)
			mkRepo(t, s, ctx, 1, "alice", "widgets", core.VisibilityPublic)

			_, err := s.CreateRepo(ctx, &core.Repo{
				Name: "widgets", OwnerID: 1, OwnerName: "alice",
				Path: tc.path, Visibility: core.VisibilityPrivate,
			})
			if !errors.Is(err, ErrNameTaken) {
				t.Fatalf("expected ErrNameTaken, got %v", err)
			}
		})
	}
}


M db/store.go => db/store.go +2 -1
@@ 71,7 71,8 @@ var (
	// ErrNotFound is returned when a lookup, update or delete matched no row.
	ErrNotFound = errors.New("db: not found")
	// ErrNameTaken is returned by CreateRepo when the owner already has a
	// repository with the requested name (uq_repo_owner_id_name violation).
	// repository with the requested name (uq_repo_owner_id_name or the
	// equivalent repository_path_key violation).
	ErrNameTaken = errors.New("db: repository name already taken")
	// ErrKeyExists is returned by InsertKey when the key id (kid) is already
	// registered (dolt_key.kid UNIQUE violation).