diff --git a/web/handlers_repo.go b/web/handlers_repo.go index 42bb65a25b4f14ceba3b5541f65c362b6ee52d79..d748c2d57ae00204b935f1202c1550cedfe7d9ba 100644 --- a/web/handlers_repo.go +++ b/web/handlers_repo.go @@ -2,11 +2,14 @@ package web import ( "errors" + "log/slog" "net/http" "strings" "github.com/go-chi/chi/v5" "sourcecraft.dev/bigbes/sr-ht-core/config" + + "go.bigb.es/auxilia/scribe" "sourcecraft.dev/bigbes/sr-ht-ecore/chrome" "sourcecraft.dev/bigbes/sr-ht-ecore/pages" @@ -193,12 +196,25 @@ return } var ( - branches []browse.Branch - defBr string - commits []browse.CommitInfo - views []View - browseErr string + branches []browse.Branch + defBr string + commits []browse.CommitInfo + views []View + // browseFailed says only that the history could not be read. It used to + // be the browse layer's own error text, rendered into the page: that + // string names dolt internals and the store's on-disk path, which + // nothing else on this surface discloses and which a reader can do + // nothing with. A bool rather than a message, so no error can reach the + // template by being assigned to it later. + browseFailed bool ) + // browseFailure records the reason where it belongs — the operator's log, + // against the database it happened to — and leaves the page a fixed sentence. + browseFailure := func(what string, err error) { + browseFailed = true + slog.Warn(what, "component", "web", "database", repo.ID, scribe.Err(err)) + } + if sess, err := a.cfg.Browse.Open(r.Context(), repo.Path); err == nil { defer sess.Close() if bs, err := sess.Branches(r.Context()); err == nil { @@ -208,7 +224,7 @@ if defBr != "" { if cs, _, err := sess.Log(r.Context(), defBr, "", overviewCommitLimit); err == nil { commits = cs } else { - browseErr = err.Error() + browseFailure("reading a database's log for the overview failed", err) } // Fingerprint the tables at the default branch to compute the // optional alternative-view tabs. A browse failure here must not @@ -218,10 +234,10 @@ views = applicableViews(a.views, tables) } } } else { - browseErr = err.Error() + browseFailure("listing a database's branches for the overview failed", err) } } else { - browseErr = err.Error() + browseFailure("opening a database for the overview failed", err) } view := struct { @@ -232,7 +248,7 @@ DefaultBranch string Commits []browse.CommitInfo Views []View CloneURL string - BrowseError string + BrowseFailed bool }{ Page: a.page(r, repo.OwnerName+"/"+repo.Name+" — "+serviceName), Repo: repo, @@ -241,7 +257,7 @@ DefaultBranch: defBr, Commits: commits, Views: views, CloneURL: a.cloneURL(repo), - BrowseError: browseErr, + BrowseFailed: browseFailed, } a.render(w, http.StatusOK, "overview", view) } diff --git a/web/templates/overview.html b/web/templates/overview.html index 56b6d02fefac9fc1bd0e82048c52fb183afe26d2..b7abd8db54b83a5ab08eec213b989c62c630826f 100644 --- a/web/templates/overview.html +++ b/web/templates/overview.html @@ -41,8 +41,8 @@ {{end}}

{{icon "clock"}} Recent commits

- {{if .BrowseError}} -
Could not read history: {{.BrowseError}}
+ {{if .BrowseFailed}} +
Could not read history.
{{end}} {{if .Commits}} diff --git a/web/web_test.go b/web/web_test.go index 6fa962c72c1f64851f3831506164f70834dd7fed..b2dca419d64ca4a09f2992009a74cada9888b37c 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -544,6 +544,69 @@ assert.Equal(t, hidden.Body.String(), missing.Body.String(), "a database somebody may not see and one that is not there must render the same page") } +// browseDetail is a browse failure of the shape the layer really produces: a +// dolt internal, and the store's path on our disk. +const browseDetail = "browse: walk commits: open /var/lib/dolt/~alice/db/.dolt/noms/oldgen: no such file" + +// The overview used to render the browse layer's own error text into the page, +// under "Could not read history: ". A reader can do nothing with a chunk store's +// path, and nothing else on this surface discloses one. The page carries a fixed +// sentence and the detail goes to the log. +func TestOverviewDoesNotPrintTheBrowseError(t *testing.T) { + newOverview := func(t *testing.T) *harness { + t.Helper() + h := newHarness(t) + h.store.add(&core.Repo{Name: "db", OwnerID: 1, OwnerName: "alice", + Path: "/var/lib/dolt/~alice/db", Visibility: core.VisibilityPublic}) + return h + } + assertHidden := func(t *testing.T, body string) { + t.Helper() + assert.Contains(t, body, "Could not read history.") + assert.NotContains(t, body, browseDetail) + assert.NotContains(t, body, "/var/lib/dolt", "the store's path must not reach the reader") + assert.NotContains(t, body, "walk commits", "dolt's internals must not reach the reader") + } + + t.Run("a log that cannot be read", func(t *testing.T) { + h := newOverview(t) + h.browse.sess = &fakeSession{ + branches: []browse.Branch{{Name: "main", Head: "abcdef1234567890"}}, + logErr: errors.New(browseDetail), + } + + rec := h.do("GET", "/~alice/db", nil, nil) + require.Equal(t, http.StatusOK, rec.Code) + body := rec.Body.String() + assertHidden(t, body) + // The rest of the page is still the page: a history we cannot read is + // not a reason to withhold the branches we can. + assert.Contains(t, body, "main") + }) + + t.Run("a store that cannot be opened", func(t *testing.T) { + h := newOverview(t) + h.browse.errByPath = map[string]error{"/var/lib/dolt/~alice/db": errors.New(browseDetail)} + + rec := h.do("GET", "/~alice/db", nil, nil) + require.Equal(t, http.StatusOK, rec.Code) + assertHidden(t, rec.Body.String()) + }) + + // An empty database is a state and not a failure, and the page said so + // before this change too. It must keep saying it, with no warning attached. + t.Run("an empty database is not a failure", func(t *testing.T) { + h := newOverview(t) + h.browse.sess = &fakeSession{branches: []browse.Branch{{Name: "main", Head: "abcdef1234567890"}}} + + rec := h.do("GET", "/~alice/db", nil, nil) + require.Equal(t, http.StatusOK, rec.Code) + body := rec.Body.String() + assert.Contains(t, body, "No commits.") + assert.NotContains(t, body, "Could not read history") + }) +} + func TestDashboardLists(t *testing.T) { h := newHarness(t) h.store.add(&core.Repo{Name: "mine", OwnerID: 3, OwnerName: "bob", Path: "/m", Visibility: core.VisibilityPrivate})