diff --git a/beads/cache.go b/beads/cache.go new file mode 100644 index 0000000000000000000000000000000000000000..b5d26953860b51feb60c2c3402e1aa43422ddebc --- /dev/null +++ b/beads/cache.go @@ -0,0 +1,103 @@ +package beads + +import ( + "sync" + "time" +) + +// --- the cache the cross-database readings share ------------------------------ +// +// Two readings here open every database one caller may browse: /ready's +// aggregation (ReadyAcross) and the prefix index behind cross-database issue +// links (PrefixesAcross). Opening N stores per request is exactly what the +// per-request browse discipline does not scale to, and both are bounded the same +// way: +// +// 1. A head-hash gate. Opening a session and listing branches is cheap; reading +// and projecting rows is not. When the head has not moved, the cached +// projection stands and no row is read. +// 2. A TTL (ReadyCacheTTL), so a cache can never be the reason a reader sees +// yesterday's answer. +// 3. A ceiling on how many databases one call opens (ReadyMaxDatabases), +// applied by the aggregations themselves. +// +// The bounds are one set of numbers, named in ready.go where /ready needed them +// first. What differs between the two readings is only the projection being +// cached, which is what the type parameter is: a second cache with its own +// lifetime rules is how two pages start disagreeing about how fresh "fresh" is. +// +// What is cached is always a projection and never an open session. An open store +// is a file handle and a memory mapping; caching those is the thing per-request +// opening exists to prevent. + +// cached is one database's cached projection plus the two facts the bounds are +// checked against: the head it was read at, and when it was stored. +type cached[T any] struct { + head string + at time.Time + value T +} + +// projectionCache is a small mutex-guarded map from repository id to one cached +// projection. The repository id is the identity it is keyed on: no two databases +// share it and it survives a rename. +// +// The zero value is usable — the map is allocated on first store — so a cache +// can be a field of a value that has no constructor. +type projectionCache[T any] struct { + mu sync.Mutex + entries map[int]cached[T] +} + +// lookup returns the cached projection for a database when it was read at the +// same head and has not expired. Both conditions, not either: the head hash is +// what makes it correct, the TTL is what makes it bounded. +func (c *projectionCache[T]) lookup(id int, head string, now time.Time) (T, bool) { + var zero T + if head == "" { + // A database whose head cannot be named cannot be gated on one. + return zero, false + } + c.mu.Lock() + defer c.mu.Unlock() + e, ok := c.entries[id] + if !ok || e.head != head || now.Sub(e.at) >= ReadyCacheTTL { + return zero, false + } + return e.value, true +} + +// store records a projection read at head, dropping expired entries — and, if +// that was not enough, everything — when the map is at its ceiling. A cache is +// not a store: over the ceiling it starts again rather than growing with the +// instance. +func (c *projectionCache[T]) store(id int, head string, now time.Time, value T) { + if head == "" { + return + } + c.mu.Lock() + defer c.mu.Unlock() + if c.entries == nil { + c.entries = make(map[int]cached[T]) + } + if len(c.entries) >= readyCacheMaxEntries { + for k, old := range c.entries { + if now.Sub(old.at) >= ReadyCacheTTL { + delete(c.entries, k) + } + } + if len(c.entries) >= readyCacheMaxEntries { + c.entries = make(map[int]cached[T], readyCacheMaxEntries) + } + } + c.entries[id] = cached[T]{head: head, at: now, value: value} +} + +// size reports how many entries the cache holds. It exists for the tests that +// assert the ceiling: the bound is the point of the map, and only a count can +// check it. +func (c *projectionCache[T]) size() int { + c.mu.Lock() + defer c.mu.Unlock() + return len(c.entries) +} diff --git a/beads/prefixes.go b/beads/prefixes.go new file mode 100644 index 0000000000000000000000000000000000000000..5dea333690fdda528b1ce067b8fa43efea7202c0 --- /dev/null +++ b/beads/prefixes.go @@ -0,0 +1,254 @@ +package beads + +import ( + "context" + "regexp" + "sort" + "strings" + "time" + + "sourcecraft.dev/bigbes/sr-ht-dolt/browse" +) + +// --- which database owns "-" -------------------------------------- +// +// A global-tracker issue that says "blocked by artifacts-nex.2" is naming a row +// in another database, and without this the reader has to work out which one and +// go there by hand. Two levels of tracker only pay for themselves if they point +// at each other. +// +// This file answers the one question that needs several databases: given an +// id-shaped string in some text, which database — of the ones this caller may +// browse — owns it. Whether the answer becomes a link, and what that link looks +// like, is the renderer's business; this package renders nothing. +// +// Authorization is not here, exactly as it is not in ReadyAcross: handing this +// function a database is the statement that the caller may read it. A database +// the caller may not browse is one the caller's index has never heard of, which +// is what makes an id belonging to it plain text and not a hint that it exists. + +// prefixKey is the config row every beads database stores its own id prefix in +// ("global", "artifacts", "sr-ht-dolt", …). It sits in the same config table the +// memories do, beside compact_tier2_days and the rest of the tracker's settings. +const prefixKey = "issue_prefix" + +// PrefixCache is the prefix index's projection cache: one prefix per database, +// gated on the head hash and expiring on ReadyCacheTTL — /ready's bounds and not +// a second set (see cache.go). The zero value is usable, so a holder that has no +// constructor can carry one as a field. +type PrefixCache struct { + projectionCache[string] +} + +// PrefixIndex is prefix → the database that owns it, over the databases one +// caller may browse. It is built per caller and must not be shared between them: +// what it holds is precisely the set of databases that caller is allowed to know +// exists. +type PrefixIndex struct { + byPrefix map[string]ReadyDatabase + // Failed lists the databases that could not be read. It is here for the + // caller's log and for nothing else: a database that could not be read costs + // the ids it owns their link, and a page may not say more than that. + Failed []ReadyFailure +} + +// Reference is one id found in a text: where it sits, what it says, and which +// database owns it. +type Reference struct { + Start, End int // byte offsets of the id within the scanned text + ID string // the id exactly as written + Database ReadyDatabase +} + +// idPattern matches an id-shaped token: a prefix of one or more lowercase +// alphanumeric segments joined by hyphens, then a hyphen, then the suffix bd +// generates — `[0-9a-z]+` with optional dotted parts, which is the `46c.2` +// subtask form. +// +// The suffix carries no hyphen, so the last hyphen of a token is always the +// prefix/suffix boundary and the split below is unambiguous even for a prefix +// that is itself hyphenated ("sr-ht-dolt-44n.6" is sr-ht-dolt's 44n.6, never +// sr-ht's dolt-44n.6). The word boundaries keep a token from being found inside +// a longer word, so "Xartifacts-46c" names nothing. +// +// The token being id-shaped is not what makes it an id: only a prefix the index +// knows makes it one. Everything else is left as it was written. +var idPattern = regexp.MustCompile(`\b[0-9a-z_]+(?:-[0-9a-z_]+)*-[0-9a-z]+(?:\.[0-9a-z]+)*\b`) + +// PrefixesAcross builds the index over the databases it is handed: for each, the +// issue_prefix its config names. +// +// It is bounded exactly as ReadyAcross is — the same head-hash gate, the same +// TTL, the same ceiling — because it is the same read pattern: N stores opened +// for one request. It is cheaper per database, though: the projection is one row +// of a dozen-row table, and the fingerprint is not asked for separately, since a +// config table carrying issue_prefix is itself the statement that this is a bd +// tracker. A database with no such row simply owns no prefix, and that answer is +// cached like any other. +// +// now is the clock the TTL is measured against, passed in rather than read here +// for the reason ReadyAcross takes one: this package reads no hidden clock. +// +// It returns no error. A database that cannot be opened or read costs the ids it +// owns their link and nothing else; it lands in Failed for the caller's log. +func PrefixesAcross( + ctx context.Context, + dbs []ReadyDatabase, + open ReadyOpener, + cache *PrefixCache, + now time.Time, +) *PrefixIndex { + index := &PrefixIndex{byPrefix: map[string]ReadyDatabase{}} + + if len(dbs) > ReadyMaxDatabases { + // The first Max in the order the caller listed them. The caller puts the + // database whose page this is first, so the one prefix a page cannot do + // without is the one the ceiling can never drop. + dbs = dbs[:ReadyMaxDatabases] + } + + // A prefix two databases both claim is dropped rather than awarded to + // whichever was listed first: linking to one of two candidates would be a + // guess rendered as a fact, and the id is readable as text either way. + ambiguous := map[string]bool{} + for _, d := range dbs { + prefix, err := databasePrefix(ctx, d, open, cache, now) + if err != nil { + index.Failed = append(index.Failed, ReadyFailure{Database: d, Err: err}) + continue + } + if prefix == "" { + continue + } + if ambiguous[prefix] { + continue + } + if _, taken := index.byPrefix[prefix]; taken { + ambiguous[prefix] = true + delete(index.byPrefix, prefix) + continue + } + index.byPrefix[prefix] = d + } + return index +} + +// databasePrefix returns one database's issue prefix, from the cache when the +// head has not moved and by reading config otherwise. +// +// The order is the whole point of the gate: open, list branches, and only then +// consult the cache. Opening a session and reading the branch list is cheap; +// reading rows is not, and on a hit no row is touched. +func databasePrefix( + ctx context.Context, + d ReadyDatabase, + open ReadyOpener, + cache *PrefixCache, + now time.Time, +) (string, error) { + sess, err := open(ctx, d) + if err != nil { + return "", err + } + defer sess.Close() + + branches, err := sess.Branches(ctx) + if err != nil { + return "", err + } + ref := browse.DefaultBranch(branches) + if ref == "" { + // A store with no branches carries no config either: nothing to read and + // nothing to cache. + return "", nil + } + head := headHashOf(branches, ref) + if prefix, ok := cache.lookup(d.ID, head, now); ok { + return prefix, nil + } + + prefix, err := readPrefix(ctx, sess, ref) + if err != nil { + return "", err + } + cache.store(d.ID, head, now, prefix) + return prefix, nil +} + +// readPrefix reads the issue_prefix row out of a database's config table. A +// missing table — this is not a bd tracker — is no prefix, the treatment every +// optional table gets here. +func readPrefix(ctx context.Context, sess BrowseSession, ref string) (string, error) { + rows, _, err := readRowsOptional(ctx, sess, ref, memoryTable) + if err != nil { + return "", err + } + if rows == nil { + return "", nil + } + cols := indexCols(rows.Columns) + for _, r := range rows.Rows { + if cell(cols, r, "key") != prefixKey { + continue + } + // Lowercased because that is the case the ids themselves are written in, + // and the index is looked up by what the text says. + return strings.ToLower(strings.TrimSpace(cell(cols, r, "value"))), nil + } + return "", nil +} + +// Lookup returns the database owning prefix. A nil index — no database was +// readable, or the caller did not build one for this page — knows no prefix, +// which is the same answer as an unknown one: not linked. +func (ix *PrefixIndex) Lookup(prefix string) (ReadyDatabase, bool) { + if ix == nil { + return ReadyDatabase{}, false + } + d, ok := ix.byPrefix[prefix] + return d, ok +} + +// Prefixes lists the prefixes the index knows, in order. It exists for the +// caller's log line and for tests. +func (ix *PrefixIndex) Prefixes() []string { + if ix == nil { + return nil + } + out := make([]string, 0, len(ix.byPrefix)) + for p := range ix.byPrefix { + out = append(out, p) + } + sort.Strings(out) + return out +} + +// Scan returns every id in text whose prefix this index knows, in order and +// non-overlapping. +// +// It answers where the ids are and nothing about how they should be rendered: +// the caller escapes the text it was handed and wraps these ranges, which is the +// only order in which stored text can become HTML safely. +// +// An id-shaped token whose prefix the index does not know yields nothing at all, +// which is what makes an id in a database the caller may not browse +// indistinguishable from one that matches nothing. +func (ix *PrefixIndex) Scan(text string) []Reference { + if ix == nil || len(ix.byPrefix) == 0 || text == "" { + return nil + } + var out []Reference + for _, m := range idPattern.FindAllStringIndex(text, -1) { + token := text[m[0]:m[1]] + cut := strings.LastIndex(token, "-") + if cut <= 0 { + continue + } + d, ok := ix.byPrefix[token[:cut]] + if !ok { + continue + } + out = append(out, Reference{Start: m[0], End: m[1], ID: token, Database: d}) + } + return out +} diff --git a/beads/prefixes_test.go b/beads/prefixes_test.go new file mode 100644 index 0000000000000000000000000000000000000000..6dd34c30315fdd6d049b3cd5e57f749582b3548c --- /dev/null +++ b/beads/prefixes_test.go @@ -0,0 +1,285 @@ +package beads + +import ( + "errors" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-dolt/browse" +) + +// --- fixtures ---------------------------------------------------------------- + +// prefixTracker is a beads database whose config names an issue prefix, with the +// settings a real tracker carries beside it — the projection has to pick one row +// out of the table rather than read the first one. +func prefixTracker(head, prefix string) *fakeReadyDB { + return &fakeReadyDB{ + branches: []browse.Branch{{Name: "main", Head: head}}, + tables: beadsTables(), + rows: map[string]*browse.RowPage{ + "config": { + Columns: []string{"key", "value"}, + Rows: [][]string{ + {"compact_tier2_days", "30"}, + {"kv.memory.handoff", "not a prefix"}, + {"issue_prefix", prefix}, + }, + Total: 3, + }, + }, + } +} + +// prefixFixture is two trackers, "global" and "artifacts", as the instance has +// them: one cross-project tracker and one per-project. +func prefixFixture() (*readyInstance, []ReadyDatabase) { + in := &readyInstance{dbs: map[int]*fakeReadyDB{ + 1: prefixTracker("h-global", "global"), + 2: prefixTracker("h-artifacts", "artifacts"), + }} + return in, []ReadyDatabase{ + {ID: 1, OwnerName: "bigbes", Name: "beads-global"}, + {ID: 2, OwnerName: "bigbes", Name: "sourcehut-artifacts"}, + } +} + +// --- tests ------------------------------------------------------------------- + +// The index: one prefix per database, read out of config, and nothing claimed +// for a prefix no database named. +func TestPrefixesAcrossIndexesTheDatabasesItIsHanded(t *testing.T) { + in, dbs := prefixFixture() + + index := PrefixesAcross(t.Context(), dbs, in.open, &PrefixCache{}, readyNow) + + assert.Equal(t, []string{"artifacts", "global"}, index.Prefixes()) + d, ok := index.Lookup("global") + require.True(t, ok) + assert.Equal(t, "bigbes/beads-global", d.Slug()) + d, ok = index.Lookup("artifacts") + require.True(t, ok) + assert.Equal(t, "bigbes/sourcehut-artifacts", d.Slug()) + _, ok = index.Lookup("nosuch") + assert.False(t, ok, "a prefix no database named is not in the index") + assert.Empty(t, index.Failed) + + // Every session opened is closed again: this is the read pattern the + // per-request browse discipline is bounded against. + for id, f := range in.dbs { + assert.Equal(t, f.opens, f.closes, "database %d: %d opens, %d closes", id, f.opens, f.closes) + } +} + +// The head-hash gate: a second build with unmoved heads reads no rows. Asserted +// by counting reads on the fake, never by timing. +func TestPrefixesAcrossHeadHashGateSkipsTheRead(t *testing.T) { + in, dbs := prefixFixture() + cache := &PrefixCache{} + + first := PrefixesAcross(t.Context(), dbs, in.open, cache, readyNow) + require.Len(t, first.Prefixes(), 2) + global := in.dbs[1] + require.Greater(t, global.rowReads, 0, "the first build must read config") + reads := global.rowReads + + second := PrefixesAcross(t.Context(), dbs, in.open, cache, readyNow.Add(30*time.Second)) + + assert.Equal(t, reads, global.rowReads, "a second build with an unmoved head must read no rows") + assert.Equal(t, first.Prefixes(), second.Prefixes(), "and it is the same answer") + // The session is still opened and its branches listed — that is what the gate + // is gated on, and it is the cheap half. + assert.Equal(t, 2, global.opens) + assert.Equal(t, 2, global.closes) +} + +// A head that moved is a prefix that may have been renamed: read again, even +// well inside the TTL. +func TestPrefixesAcrossMovedHeadForcesAReread(t *testing.T) { + in, dbs := prefixFixture() + cache := &PrefixCache{} + global := in.dbs[1] + + PrefixesAcross(t.Context(), dbs, in.open, cache, readyNow) + reads := global.rowReads + + global.branches = []browse.Branch{{Name: "main", Head: "h-global-2"}} + PrefixesAcross(t.Context(), dbs, in.open, cache, readyNow.Add(time.Second)) + + assert.Greater(t, global.rowReads, reads, "a moved head must be re-read") + assert.Equal(t, 1, in.dbs[2].rowReads, "the sibling's head did not move") +} + +// The TTL is the same one /ready is bounded by, because it is the same cache. +func TestPrefixesAcrossTTLExpiryForcesAReread(t *testing.T) { + in, dbs := prefixFixture() + cache := &PrefixCache{} + global := in.dbs[1] + + PrefixesAcross(t.Context(), dbs, in.open, cache, readyNow) + reads := global.rowReads + + PrefixesAcross(t.Context(), dbs, in.open, cache, readyNow.Add(ReadyCacheTTL-time.Nanosecond)) + assert.Equal(t, reads, global.rowReads, "inside the TTL the projection stands") + + PrefixesAcross(t.Context(), dbs, in.open, cache, readyNow.Add(ReadyCacheTTL)) + assert.Greater(t, global.rowReads, reads, "at the TTL the projection is re-read") +} + +// The ceiling: at most ReadyMaxDatabases stores opened per build, and the first +// database listed — the caller puts the page's own database there — is inside +// it. +func TestPrefixesAcrossCeiling(t *testing.T) { + in := &readyInstance{dbs: map[int]*fakeReadyDB{}} + var dbs []ReadyDatabase + for i := 1; i <= ReadyMaxDatabases+3; i++ { + in.dbs[i] = prefixTracker(fmt.Sprintf("h%d", i), fmt.Sprintf("p%02d", i)) + dbs = append(dbs, ReadyDatabase{ID: i, OwnerName: "alice", Name: fmt.Sprintf("db%02d", i)}) + } + + index := PrefixesAcross(t.Context(), dbs, in.open, &PrefixCache{}, readyNow) + + assert.Len(t, index.Prefixes(), ReadyMaxDatabases) + _, ok := index.Lookup("p01") + assert.True(t, ok, "the first database listed is never dropped by the ceiling") + for i := ReadyMaxDatabases + 1; i <= ReadyMaxDatabases+3; i++ { + assert.Zero(t, in.dbs[i].opens, "database %d is past the ceiling", i) + } +} + +// Two databases claiming one prefix: neither is linked. Linking to one of two +// candidates would be a guess rendered as a fact, and the id reads as text +// either way. +func TestPrefixesAcrossDropsAnAmbiguousPrefix(t *testing.T) { + in := &readyInstance{dbs: map[int]*fakeReadyDB{ + 1: prefixTracker("h1", "shared"), + 2: prefixTracker("h2", "shared"), + 3: prefixTracker("h3", "mine"), + }} + dbs := []ReadyDatabase{ + {ID: 1, OwnerName: "alice", Name: "one"}, + {ID: 2, OwnerName: "bob", Name: "two"}, + {ID: 3, OwnerName: "carol", Name: "three"}, + } + + index := PrefixesAcross(t.Context(), dbs, in.open, &PrefixCache{}, readyNow) + + _, ok := index.Lookup("shared") + assert.False(t, ok, "a prefix two databases claim belongs to neither") + assert.Equal(t, []string{"mine"}, index.Prefixes(), "the unambiguous ones are unaffected") +} + +// A database that names no prefix — no config table at all, or a config without +// the row — is simply not in the index. +func TestPrefixesAcrossSkipsADatabaseWithNoPrefix(t *testing.T) { + noConfig := &fakeReadyDB{branches: []browse.Branch{{Name: "main", Head: "h1"}}, tables: beadsTables()} + noRow := &fakeReadyDB{ + branches: []browse.Branch{{Name: "main", Head: "h2"}}, + tables: beadsTables(), + rows: map[string]*browse.RowPage{ + "config": {Columns: []string{"key", "value"}, Rows: [][]string{{"compact_tier2_days", "30"}}, Total: 1}, + }, + } + empty := &fakeReadyDB{} // a store with no branches: never pushed to + in := &readyInstance{dbs: map[int]*fakeReadyDB{1: noConfig, 2: noRow, 3: empty}} + dbs := []ReadyDatabase{ + {ID: 1, OwnerName: "alice", Name: "plain"}, + {ID: 2, OwnerName: "alice", Name: "settings-only"}, + {ID: 3, OwnerName: "alice", Name: "fresh"}, + } + + index := PrefixesAcross(t.Context(), dbs, in.open, &PrefixCache{}, readyNow) + + assert.Empty(t, index.Prefixes()) + assert.Empty(t, index.Failed, "naming no prefix is not a failure") + assert.Zero(t, empty.rowReads, "a store with no branches is not read") +} + +// A database that cannot be opened, or whose config cannot be read, costs the +// ids it owns their link and nothing else. +func TestPrefixesAcrossFailingDatabaseCostsItselfOnly(t *testing.T) { + in, dbs := prefixFixture() + in.dbs[1].openErr = errors.New("browse: open store: no such file or directory") + + index := PrefixesAcross(t.Context(), dbs, in.open, &PrefixCache{}, readyNow) + + require.Len(t, index.Failed, 1) + assert.Equal(t, "bigbes/beads-global", index.Failed[0].Database.Slug()) + assert.ErrorContains(t, index.Failed[0].Err, "no such file") + assert.Equal(t, []string{"artifacts"}, index.Prefixes()) + + // A store that opens but cannot list its branches fails the same way, and + // still closes its session. + in2, dbs2 := prefixFixture() + in2.dbs[1].branchesErr = errors.New("browse: list branches: corrupt chunk") + index2 := PrefixesAcross(t.Context(), dbs2, in2.open, &PrefixCache{}, readyNow) + require.Len(t, index2.Failed, 1) + assert.Equal(t, []string{"artifacts"}, index2.Prefixes()) + assert.Equal(t, 1, in2.dbs[1].closes) +} + +// The pattern: what counts as an id and what does not. The suffix carries no +// hyphen, which is what makes the split unambiguous for a hyphenated prefix. +func TestPrefixIndexScan(t *testing.T) { + index := &PrefixIndex{byPrefix: map[string]ReadyDatabase{ + "global": {ID: 1, OwnerName: "bigbes", Name: "beads-global"}, + "artifacts": {ID: 2, OwnerName: "bigbes", Name: "sourcehut-artifacts"}, + "sr-ht-dolt": {ID: 3, OwnerName: "bigbes", Name: "sourcehut-dolt"}, + }} + + for _, tc := range []struct { + name string + text string + want []string + }{ + {"a plain id", "blocked by artifacts-nex", []string{"artifacts-nex"}}, + {"the subtask form", "see artifacts-46c.2 for the bucket", []string{"artifacts-46c.2"}}, + {"a hyphenated prefix", "sr-ht-dolt-44n.6 lands first", []string{"sr-ht-dolt-44n.6"}}, + {"several in one line", "global-b08 needs artifacts-46c.2", []string{"global-b08", "artifacts-46c.2"}}, + {"an unknown prefix", "nosuch-46c and other-1 are not ids", nil}, + {"a hyphenated word", "the read-only surface", nil}, + {"an id at the end of a sentence", "closed by artifacts-46c.", []string{"artifacts-46c"}}, + {"an id in brackets", "(artifacts-46c) and [global-b08]", []string{"artifacts-46c", "global-b08"}}, + {"an id inside a longer word", "xxartifacts-46c and artifactsx-1", nil}, + {"uppercase is not an id", "ARTIFACTS-46C", nil}, + {"a prefix on its own", "the artifacts tracker", nil}, + {"a shorter known prefix inside a longer token", "some-global-b08", nil}, + {"empty text", "", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + var got []string + for _, ref := range index.Scan(tc.text) { + got = append(got, ref.ID) + assert.Equal(t, ref.ID, tc.text[ref.Start:ref.End], + "the offsets must name the id they carry") + } + assert.Equal(t, tc.want, got) + }) + } + + // Each reference names the database that owns it. + refs := index.Scan("global-b08 blocks artifacts-46c.2") + require.Len(t, refs, 2) + assert.Equal(t, "bigbes/beads-global", refs[0].Database.Slug()) + assert.Equal(t, "bigbes/sourcehut-artifacts", refs[1].Database.Slug()) +} + +// An index that was never built knows no prefix — the same answer an unknown one +// gets, which is what keeps "the caller may not see that database" and "there is +// no such database" indistinguishable. +func TestNilPrefixIndexKnowsNothing(t *testing.T) { + var index *PrefixIndex + + assert.Nil(t, index.Scan("artifacts-46c.2")) + assert.Nil(t, index.Prefixes()) + _, ok := index.Lookup("artifacts") + assert.False(t, ok) + + // So does an empty one. + empty := PrefixesAcross(t.Context(), nil, nil, &PrefixCache{}, readyNow) + assert.Nil(t, empty.Scan("artifacts-46c.2")) +} diff --git a/beads/ready.go b/beads/ready.go index 3d68aaa5055dcca736fb51cf4c960d1c354b146e..b3406620f17486139703defd96196d2882832d7c 100644 --- a/beads/ready.go +++ b/beads/ready.go @@ -5,7 +5,6 @@ "context" "net/url" "sort" "strings" - "sync" "time" "sourcecraft.dev/bigbes/sr-ht-dolt/browse" @@ -26,6 +25,10 @@ // already decided which databases this caller may browse (see the package // comment: this package renders nothing and authorizes nothing); handing it a // database is the statement that the caller may read it. +// The bounds every cross-database reading here is held to. They are named for +// /ready because that is the page that needed them first; the prefix index +// behind cross-database issue links is bounded by these same three numbers +// rather than by a second set of its own (see cache.go). const ( // ReadyMaxDatabases bounds how many databases one call opens. Opening N // stores per request is exactly what the per-request browse discipline does @@ -186,28 +189,23 @@ } return true } -// ReadyCache holds one projection per database, keyed by the repository id and -// gated on the head hash. It is deliberately small: a mutex, a map, and the two -// bounds above. +// ReadyCache holds one ready projection per database, keyed by the repository id +// and gated on the head hash. The gate, the TTL and the entry ceiling are the +// shared projectionCache's (cache.go), which the prefix index is bounded by too: +// one set of rules, one lifetime. // // What it holds is the projection — the ready cards — and never an open // session. An open store is a file handle and a memory mapping; caching those is // the thing per-request opening exists to prevent. -// -// The zero value is not usable; call NewReadyCache. A nil *ReadyCache is a -// programming error and panics on first use rather than quietly disabling the -// bound it exists to enforce. type ReadyCache struct { - mu sync.Mutex - entries map[int]readyEntry + projectionCache[readyEntry] } -// readyEntry is one database's cached projection. +// readyEntry is one database's cached ready projection. The head it was read at +// and the time it was stored are the cache's, not this struct's. type readyEntry struct { - head string // the branch head the projection was read at; the gate - ref string // the branch it was read from - at time.Time // when it was stored; the TTL - beads bool // the fingerprint held — a false entry is a database to skip + ref string // the branch it was read from + beads bool // the fingerprint held — a false entry is a database to skip // commit is the head commit for the group's freshness line. It cannot go // stale under an unmoved head, so it is cached with the cards and a cache hit // reads no log either. @@ -215,47 +213,11 @@ commit *browse.CommitInfo cards []Card } -// NewReadyCache returns an empty cache. +// NewReadyCache returns an empty cache. The zero value works too — the map is +// allocated on first store — and this exists for the callers that hold one by +// pointer. func NewReadyCache() *ReadyCache { - return &ReadyCache{entries: map[int]readyEntry{}} -} - -// lookup returns the cached projection for a database when it was read at the -// same head and has not expired. Both conditions, not either: the head hash is -// what makes it correct, the TTL is what makes it bounded. -func (c *ReadyCache) lookup(id int, head string, now time.Time) (readyEntry, bool) { - if head == "" { - // A database whose head cannot be named cannot be gated on one. - return readyEntry{}, false - } - c.mu.Lock() - defer c.mu.Unlock() - e, ok := c.entries[id] - if !ok || e.head != head || now.Sub(e.at) >= ReadyCacheTTL { - return readyEntry{}, false - } - return e, true -} - -// store records a projection, dropping expired entries — and, if that was not -// enough, everything — when the map is at its ceiling. -func (c *ReadyCache) store(id int, e readyEntry) { - if e.head == "" { - return - } - c.mu.Lock() - defer c.mu.Unlock() - if len(c.entries) >= readyCacheMaxEntries { - for k, old := range c.entries { - if e.at.Sub(old.at) >= ReadyCacheTTL { - delete(c.entries, k) - } - } - if len(c.entries) >= readyCacheMaxEntries { - c.entries = make(map[int]readyEntry, readyCacheMaxEntries) - } - } - c.entries[id] = e + return &ReadyCache{} } // ReadyAcross collects the ready set of every database it is handed, grouped by @@ -388,11 +350,11 @@ tables, err := sess.Tables(ctx, ref) if err != nil { return readyEntry{}, err } - entry := readyEntry{head: head, ref: ref, at: now, beads: Applies(tables)} + entry := readyEntry{ref: ref, beads: Applies(tables)} if !entry.beads { // Cached too: a database that is not a tracker is not one on the next // request either, and the fingerprint is worth exactly one table listing. - cache.store(d.ID, entry) + cache.store(d.ID, head, now, entry) return entry, nil } @@ -402,7 +364,7 @@ return readyEntry{}, err } entry.cards = cards entry.commit = readyHead(ctx, sess, ref) - cache.store(d.ID, entry) + cache.store(d.ID, head, now, entry) return entry, nil } diff --git a/beads/ready_test.go b/beads/ready_test.go index 725ee6427f02176b04fc98ef474d32ed029fe6b6..29871f4f39b6f4bdc9f8a0ec847ade14859452c8 100644 --- a/beads/ready_test.go +++ b/beads/ready_test.go @@ -468,10 +468,7 @@ // instance with more databases than the ceiling must not grow it without end. func TestReadyCacheIsBoundedByEntryCount(t *testing.T) { cache := NewReadyCache() for i := 1; i <= readyCacheMaxEntries+10; i++ { - cache.store(i, readyEntry{head: fmt.Sprintf("h%d", i), at: readyNow, beads: true}) + cache.store(i, fmt.Sprintf("h%d", i), readyNow, readyEntry{beads: true}) } - cache.mu.Lock() - n := len(cache.entries) - cache.mu.Unlock() - assert.LessOrEqual(t, n, readyCacheMaxEntries) + assert.LessOrEqual(t, cache.size(), readyCacheMaxEntries) } diff --git a/web/beads.go b/web/beads.go index 8d40584f7d4c9ff807491ecdae0c29084b5fd770..f843fd906707fc9dfa7a5b131314732a43e8505c 100644 --- a/web/beads.go +++ b/web/beads.go @@ -2,7 +2,14 @@ package web import ( "context" + "fmt" + "html/template" + "log/slog" + "net/http" "net/url" + "strings" + + "go.bigb.es/auxilia/scribe" "sourcecraft.dev/bigbes/sr-ht-dolt/beads" "sourcecraft.dev/bigbes/sr-ht-dolt/browse" @@ -21,7 +28,30 @@ // The reading itself is not here: the fingerprint, the lane bucketing, the ready // rule and the whole view model live in the beads package, which the MCP surface // shares. This type is only the View adapter — slug, label, template, and the // hand-off of the request's ref and query. -type beadsView struct{} +type beadsView struct { + // prefixes is the cross-database link index's cache: one issue prefix per + // database, gated on that database's head hash and expiring on + // beads.ReadyCacheTTL — /ready's bounds, shared rather than restated (see + // beads/cache.go). + // + // It lives on the view because the view is the one piece of per-process state + // this rendering has, and because a zero value is a working cache: views are + // registered as a bare &beadsView{}, here and in every test. + // + // The cache is keyed on the database and never on the caller, which is safe + // precisely because what it holds is a database's own prefix — a fact about + // the store, not about who may see it. Who may see it is decided per request, + // before a store is opened, and the index built from it belongs to that + // request alone. + prefixes beads.PrefixCache +} + +// The two Data.Mode values that render one issue rather than a board. They are +// what decides whether this request pays for the cross-database index at all. +const ( + beadsModeDetail = "detail" + beadsModeEpic = "epic" +) func (*beadsView) Name() string { return "beads" } func (*beadsView) Label() string { return "Beads" } @@ -40,3 +70,129 @@ return nil, err } return data, nil } + +// --- cross-database issue links ----------------------------------------------- + +// beadLinks renders one stored text with the issue ids in it linked to the +// databases that own them. It is the envelope's .Links on the issue detail pane +// and nil everywhere else, and a nil one renders the text with no links at all — +// which is a whole answer, not a degraded one: an id nobody here owns is text. +type beadLinks struct { + index *beads.PrefixIndex +} + +// Text renders a stored text — a description, a comment body, an event summary — +// as HTML with every recognised issue id wrapped in a link. +// +// It escapes first and wraps second. The result is built as a sequence of +// escaped segments and anchors this function generated itself, and only the +// finished whole is marked template.HTML: marking user-stored text as HTML and +// then running a regexp over it is how a stored payload becomes a rendered one. +// Nothing that came out of the database is ever handed to the browser unescaped, +// including the id inside the anchor and the href built from it. +func (l *beadLinks) Text(s string) template.HTML { + var refs []beads.Reference + if l != nil { + refs = l.index.Scan(s) + } + var b strings.Builder + b.Grow(len(s)) + last := 0 + for _, ref := range refs { + b.WriteString(template.HTMLEscapeString(s[last:ref.Start])) + b.WriteString(``) + b.WriteString(template.HTMLEscapeString(ref.ID)) + b.WriteString(``) + last = ref.End + } + b.WriteString(template.HTMLEscapeString(s[last:])) + return template.HTML(b.String()) +} + +// beadIssueHref is the detail-pane URL of one issue in one database: the same +// address beads.html writes for a dependency edge, built here because this one +// is assembled in Go rather than by the template. +func beadIssueHref(owner, name, id string) string { + return "/~" + url.PathEscape(owner) + "/" + url.PathEscape(name) + + "/view/beads?issue=" + url.QueryEscape(id) +} + +// beadCrossLinks builds the link index for one render, and returns nil for every +// page that is not an issue detail pane: the board carries ids in card headers +// that are already links, and nothing else in this service renders stored prose. +// Building the index opens a store per database, so it is asked for by the one +// rendering that needs it and never as part of the envelope. +// +// The visibility rule is the /ready page's, for the same reason: enumerate with +// ListReposForViewer (the listing rule), then ask core.Allowed/OpBrowse per +// database (the access rule), and do both before a single store is opened. A +// database this caller may not browse never enters the index, so an id belonging +// to it renders as plain text — indistinguishable from one whose prefix matches +// nothing. That indistinguishability is the point: a tooltip, a class or an +// "unknown tracker" marker would each publish the existence of a database this +// caller is not allowed to know about. +// +// A listing that fails costs the page its links and not the page. This is +// decoration on top of an answer, exactly as the freshness line is, and it may +// never be the reason a reader gets a 500 instead of an issue. +func (a *app) beadCrossLinks(r *http.Request, view View, repo *core.Repo, data any) *beadLinks { + bv, ok := view.(*beadsView) + if !ok { + return nil + } + d, ok := data.(*beads.Data) + if !ok || (d.Mode != beadsModeDetail && d.Mode != beadsModeEpic) { + return nil + } + + _, caller := callerOf(r.Context()) + repos, err := a.cfg.Repos.ListReposForViewer(r.Context(), caller) + if err != nil { + slog.Error("listing databases for the issue link index failed", + "component", "web", scribe.Err(err)) + return nil + } + + // The disk path never reaches beads: it is this service's arrangement of its + // own storage. The opener closes over the map, so a database that was filtered + // out here has no path to be opened by. + paths := make(map[int]string, len(repos)) + dbs := make([]beads.ReadyDatabase, 0, len(repos)) + for _, cand := range repos { + if !core.Allowed(caller, cand, a.effectiveACL(r, caller, cand), core.OpBrowse) { + continue + } + paths[cand.ID] = cand.Path + entry := beads.ReadyDatabase{ID: cand.ID, OwnerName: cand.OwnerName, Name: cand.Name} + if cand.ID == repo.ID { + // The database whose page this is goes first, so the one prefix the + // page cannot do without — its own, the ids that used to link and must + // keep linking — can never be the one the ceiling drops. + dbs = append([]beads.ReadyDatabase{entry}, dbs...) + continue + } + dbs = append(dbs, entry) + } + + open := func(ctx context.Context, d beads.ReadyDatabase) (beads.ReadySession, error) { + path, ok := paths[d.ID] + if !ok { + return nil, fmt.Errorf("web: no store path for database %s", d.Slug()) + } + return a.cfg.Browse.Open(ctx, path) + } + + index := beads.PrefixesAcross(r.Context(), dbs, open, &bv.prefixes, timeNow()) + + // A store that cannot be read is a fact about this deployment and belongs in + // the log with its error. The page says nothing at all: the ids that database + // owns simply stay text. + for _, f := range index.Failed { + slog.Warn("reading a database for the issue link index failed", + "component", "web", "database", f.Database.Slug(), scribe.Err(f.Err)) + } + return &beadLinks{index: index} +} diff --git a/web/beads_test.go b/web/beads_test.go index 7da03803228c00871655319cc74cd0de7c722ac4..3a926999092199d31c2881154393a78aba9af4e5 100644 --- a/web/beads_test.go +++ b/web/beads_test.go @@ -1,9 +1,11 @@ package web import ( + "errors" "html" "net/http" "net/url" + "regexp" "strings" "testing" @@ -591,6 +593,302 @@ if strings.Contains(body, notWant) { t.Errorf("the board rendered the command block (%q); body=%s", notWant, body) } } +} + +// --- cross-database issue links ------------------------------------------------ + +// The database whose detail pane these tests render: prefix "alpha", one issue +// whose four bodies, one comment and history name issues in other databases — +// one visible, one this caller may not browse, one belonging to nobody — plus a +// script tag stored in the description. +func beadsLinkFixture() *fakeSession { + sess := linkTracker("h-alpha", "alpha") + sess.rowsByTable["issues"] = &browse.RowPage{ + Columns: []string{"id", "title", "status", "priority", "issue_type", "assignee", + "created_at", "description", "design", "acceptance_criteria", "notes"}, + Rows: [][]string{{ + "alpha-1", "The one with references", "open", "1", "task", "alice", "2024-01-01", + "blocked by beta-46c.2, and by alpha-2.\nnosuch-9z is nobody's.\n", + "the shape is beta-nex's", + "beta-46c.2 is closed", + "secret-9a1 stays a secret", + }}, + Total: 1, + } + sess.rowsByTable["comments"] = &browse.RowPage{ + Columns: []string{"issue_id", "author", "text", "created_at"}, + Rows: [][]string{ + {"alpha-1", "alice", "landing with beta-46c.2", "2024-01-05 09:00:00"}, + }, + Total: 1, + } + sess.rowsByTable["events"] = &browse.RowPage{ + Columns: []string{"id", "issue_id", "event_type", "actor", "old_value", "new_value", "comment", "created_at"}, + Rows: [][]string{ + {"e1", "alpha-1", "updated", "alice", "NULL", `{"design":"the shape is beta-nex's"}`, "NULL", "2024-01-06 09:00:00"}, + }, + Total: 1, + } + return sess +} + +// linkTracker is a beads database whose config names an issue prefix, with no +// issues in it. It stands for the other databases on the instance: what the +// index reads out of one is the prefix and nothing else. +func linkTracker(head, prefix string) *fakeSession { + return &fakeSession{ + branches: []browse.Branch{{Name: "main", Head: head}}, + tables: beadsTables(), + rowsByTable: map[string]*browse.RowPage{ + "issues": {Columns: []string{"id", "title", "status"}, Total: 0}, + "dependencies": {Columns: []string{"id", "issue_id", "depends_on_issue_id", "type"}, + Total: 0}, + "config": { + Columns: []string{"key", "value"}, + Rows: [][]string{ + {"compact_tier2_days", "30"}, + {"issue_prefix", prefix}, + }, + Total: 2, + }, + }, + } +} + +// linkHarness is the instance these tests read: the current database +// (alice/alpha), a second public one whose prefix is "beta", and a PRIVATE one +// whose prefix is "secret" and which the caller may not browse. +func linkHarness(t *testing.T) (h *harness, beta, secret *fakeSession) { + t.Helper() + h = newHarness(t) + beta = linkTracker("h-beta", "beta") + secret = linkTracker("h-secret", "secret") + addTracker(h, "alice", 1, "alpha", core.VisibilityPublic, beadsLinkFixture()) + addTracker(h, "bob", 2, "beta", core.VisibilityPublic, beta) + addTracker(h, "dave", 9, "secrets", core.VisibilityPrivate, secret) + setViews(t, h, &beadsView{}) + return h, beta, secret +} + +// An id whose prefix names a database this caller may browse becomes a link to +// that database's detail pane; an id in the current database keeps linking to +// this one; an id whose prefix matches nothing is left as it was written. +func TestBeadsDetailLinksIdsToTheDatabaseThatOwnsThem(t *testing.T) { + h, _, _ := linkHarness(t) + + rec := h.do("GET", "/~alice/alpha/view/beads?issue=alpha-1", nil, nil) + if rec.Code != http.StatusOK { + t.Fatalf("detail: got %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + body := rec.Body.String() + + // The sibling database, in each of the long-text bodies that name it. + want := `beta-46c.2` + for _, field := range []string{"Description", "Acceptance criteria"} { + if got := fieldBody(t, body, field); !strings.Contains(got, want) { + t.Errorf("the %s body did not link the sibling's id: %s", field, got) + } + } + // The design body's id, which is another database's too. + if got := fieldBody(t, body, "Design"); !strings.Contains(got, + `beta-nex`) { + t.Errorf("the design body's id did not link: %s", got) + } + // This database's own id links where it has always linked: here. + if !strings.Contains(body, `alpha-2`) { + t.Errorf("an id in the current database did not link to it; body=%s", body) + } + // A prefix no database on this instance claims is not an id. + if !strings.Contains(body, "nosuch-9z is nobody's.") { + t.Errorf("an unknown prefix should be left alone; body=%s", body) + } + if strings.Contains(body, "issue=nosuch-9z") { + t.Errorf("an unknown prefix was linked; body=%s", body) + } +} + +// A database this caller may not browse is not in the index, so the ids it owns +// render as plain text — with nothing at all to distinguish them from an id that +// matches nothing. No tooltip, no class, no marker: each of those would publish +// the existence of a database this caller is not allowed to know about. +func TestBeadsDetailLeavesAnInvisibleDatabaseUnlinked(t *testing.T) { + h, _, secret := linkHarness(t) + + rec := h.do("GET", "/~alice/alpha/view/beads?issue=alpha-1", nil, nil) + if rec.Code != http.StatusOK { + t.Fatalf("detail: got %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + body := rec.Body.String() + + // The text says what it said. + if !strings.Contains(body, "secret-9a1 stays a secret") { + t.Errorf("the notes body lost its text; body=%s", body) + } + // And says nothing else: not a link, not a marker, not the database's name, + // its owner, or its head. + for _, notWant := range []string{ + "issue=secret-9a1", ">secret-9a1", "secrets", "dave", "h-secret", "unknown tracker", + } { + if strings.Contains(body, notWant) { + t.Errorf("the page revealed %q about a database the caller may not browse; body=%s", notWant, body) + } + } + if secret.opens != 0 { + t.Errorf("a database the caller may not browse was opened %d times", secret.opens) + } + + // Its owner, who may browse it, gets the link — which is what makes the + // absence above a visibility rule and not a broken index. + owner := h.do("GET", "/~alice/alpha/view/beads?issue=alpha-1", testCaller(9, "dave"), nil) + if owner.Code != http.StatusOK { + t.Fatalf("owner detail: got %d; body=%s", owner.Code, owner.Body.String()) + } + if !strings.Contains(owner.Body.String(), `secret-9a1`) { + t.Errorf("the owner of the private tracker should see the link; body=%s", owner.Body.String()) + } +} + +// Stored text is escaped first and linked second. A description carrying a +// script tag reaches the page as text, and the anchors this rendering generated +// are the only markup in the body it produced. +func TestBeadsDetailEscapesBeforeItLinks(t *testing.T) { + h, _, _ := linkHarness(t) + + rec := h.do("GET", "/~alice/alpha/view/beads?issue=alpha-1", nil, nil) + if rec.Code != http.StatusOK { + t.Fatalf("detail: got %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + body := rec.Body.String() + + if strings.Contains(body, "|)$`) + for _, tag := range regexp.MustCompile(`<[^>]*>`).FindAllString(desc, -1) { + if !anchor.MatchString(tag) { + t.Errorf("unexpected markup %q in the rendered description: %s", tag, desc) + } + } + // Escaped, the description still says exactly what was stored. + if got := html.UnescapeString(regexp.MustCompile(`]*>`).ReplaceAllString(desc, "")); got != + "blocked by beta-46c.2, and by alpha-2.\nnosuch-9z is nobody's.\n" { + t.Errorf("the rendered description does not decode to the stored text: %q", got) + } +} + +// The ids in a comment body and in a history summary are linked too: they are +// where a hand-off between two trackers is usually written. +func TestBeadsDetailLinksCommentsAndHistory(t *testing.T) { + h, _, _ := linkHarness(t) + + rec := h.do("GET", "/~alice/alpha/view/beads?issue=alpha-1", nil, nil) + if rec.Code != http.StatusOK { + t.Fatalf("detail: got %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + body := rec.Body.String() + + comment := `
landing with beta-46c.2
` + if !strings.Contains(body, comment) { + t.Errorf("a comment body's id did not link; body=%s", body) + } + summary := `updated design to the shape is beta-nex's` + if !strings.Contains(body, summary) { + t.Errorf("a history summary's id did not link; body=%s", body) + } +} + +// The index is bounded exactly as /ready is: a second render whose heads have +// not moved reads no rows again. Counted on the fake, never timed. +func TestBeadsDetailPrefixIndexIsBounded(t *testing.T) { + h, beta, _ := linkHarness(t) + + first := h.do("GET", "/~alice/alpha/view/beads?issue=alpha-1", nil, nil) + if first.Code != http.StatusOK { + t.Fatalf("first detail: got %d", first.Code) + } + reads := beta.rowReads + if reads == 0 { + t.Fatalf("the first render must read the sibling's config") + } + + second := h.do("GET", "/~alice/alpha/view/beads?issue=alpha-1", nil, nil) + if second.Code != http.StatusOK { + t.Fatalf("second detail: got %d", second.Code) + } + if beta.rowReads != reads { + t.Errorf("an unmoved head must cost no row reads: %d then %d", reads, beta.rowReads) + } + // The store is still opened and its branches listed — that is what the gate + // is gated on, and it is the cheap half. + if beta.opens != 2 { + t.Errorf("the sibling should be opened once per render, got %d", beta.opens) + } + // And the page is the same page, cache or not. + if first.Body.String() != second.Body.String() { + t.Errorf("the cached render differs from the first one") + } +} + +// The board carries no stored prose, so it builds no index and opens no other +// database. The index is paid for by the one rendering that needs it. +func TestBeadsBoardBuildsNoLinkIndex(t *testing.T) { + h, beta, _ := linkHarness(t) + + rec := h.do("GET", "/~alice/alpha/view/beads", nil, nil) + if rec.Code != http.StatusOK { + t.Fatalf("board: got %d; body=%s", rec.Code, rec.Body.String()) + } + if beta.opens != 0 { + t.Errorf("the board opened another database %d times", beta.opens) + } + if strings.Contains(rec.Body.String(), "/~bob/beta/view/beads") { + t.Errorf("the board linked into another database; body=%s", rec.Body.String()) + } +} + +// An index that could not be built at all costs the page its links and not the +// page: the ids are text, exactly as they were before this existed. +func TestBeadsDetailSurvivesAnUnbuildableIndex(t *testing.T) { + h, _, _ := linkHarness(t) + h.store.listErr = errors.New("db: list repositories: connection refused") + + rec := h.do("GET", "/~alice/alpha/view/beads?issue=alpha-1", nil, nil) + if rec.Code != http.StatusOK { + t.Fatalf("detail: got %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + body := rec.Body.String() + if !strings.Contains(body, "blocked by beta-46c.2, and by alpha-2.") { + t.Errorf("the description is missing; body=%s", body) + } + if strings.Contains(body, "issue=beta-46c.2") { + t.Errorf("an index that was never built linked something; body=%s", body) + } + if strings.Contains(body, "connection refused") { + t.Errorf("the listing error reached the reader; body=%s", body) + } +} + +// fieldBody returns the contents of the
 that follows
+// the named field label.
+func fieldBody(t *testing.T, body, label string) string {
+	t.Helper()
+	head := `
` + label + `
`
+	i := strings.Index(body, head)
+	if i < 0 {
+		t.Fatalf("no %s body in the page: %s", label, body)
+	}
+	rest := body[i+len(head):]
+	j := strings.Index(rest, "
") + if j < 0 { + t.Fatalf("unterminated %s body", label) + } + return rest[:j] } func TestBeadsEpicViewRender(t *testing.T) { diff --git a/web/handlers_view.go b/web/handlers_view.go index 400d18ccd8ebdfc274d816a35e3cef419a862290..f379c9d4c2abd2f80e2b13bdca5c408064d1a1e8 100644 --- a/web/handlers_view.go +++ b/web/handlers_view.go @@ -87,6 +87,12 @@ Branches []browse.Branch Views []View Head *browse.CommitInfo Data any + // Links renders the issue ids inside stored prose as links to the database + // that owns them. It is built by the one rendering that needs it — the + // beads detail pane — and is nil for every other page here, because + // building it opens a store per database the caller may browse. A nil one + // still renders text; see beadLinks.Text. + Links *beadLinks }{ Page: a.page(r, view.Label()+" — "+repo.OwnerName+"/"+repo.Name), Repo: repo, @@ -95,6 +101,7 @@ Branches: branches, Views: applicableViews(a.views, tables), Head: headCommit(r.Context(), sess, ref), Data: data, + Links: a.beadCrossLinks(r, view, repo, data), } a.render(w, http.StatusOK, pageName(view.Template()), envelope) } diff --git a/web/templates/beads.html b/web/templates/beads.html index 338bedade43acb764520756bf53a51a807af1409..7db6f2ba5859ecc1c910d5f8e3116b92a97d435e 100644 --- a/web/templates/beads.html +++ b/web/templates/beads.html @@ -261,10 +261,14 @@ {{if .UpdatedAt}}Updated{{.UpdatedAt}}{{end}} {{if .ClosedAt}}Closed{{.ClosedAt}}{{end}} - {{if .Description}}
Description
{{.Description}}
{{end}} - {{if .Design}}
Design
{{.Design}}
{{end}} - {{if .AcceptanceCriteria}}
Acceptance criteria
{{.AcceptanceCriteria}}
{{end}} - {{if .Notes}}
Notes
{{.Notes}}
{{end}} + {{/* The long-text bodies go through $.Links, which escapes the stored text and + then wraps the issue ids it recognises in links to the database that owns + them. An id whose prefix belongs to no database this viewer may browse is + left as the text it is. */}} + {{if .Description}}
Description
{{$.Links.Text .Description}}
{{end}} + {{if .Design}}
Design
{{$.Links.Text .Design}}
{{end}} + {{if .AcceptanceCriteria}}
Acceptance criteria
{{$.Links.Text .AcceptanceCriteria}}
{{end}} + {{if .Notes}}
Notes
{{$.Links.Text .Notes}}
{{end}} {{if eq $.Data.Mode "epic"}} @@ -366,7 +370,7 @@ {{if $.Data.Comments}} {{range $.Data.Comments}}
{{.Author}} {{if .CreatedAt}}· {{.CreatedAt}}{{end}}
-
{{.Text}}
+
{{$.Links.Text .Text}}
{{end}} {{else}}

No comments.

{{end}} @@ -384,10 +388,10 @@
    {{range $.Data.History}}
  • - {{.Actor}} {{.Summary}} + {{.Actor}} {{$.Links.Text .Summary}} {{if .CreatedAt}}· {{.CreatedAt}}{{end}}
    - {{if .Text}}
    {{.Text}}
    {{end}} + {{if .Text}}
    {{$.Links.Text .Text}}
    {{end}}
  • {{end}}
diff --git a/web/web_test.go b/web/web_test.go index c0d7ede5286b0634e9bb62306618362e1225a549..08e4bd739856925cd6f83087854f7448acc6c197 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -46,7 +46,10 @@ keys map[int][]*db.DoltKey // userID -> keys nextID int nextKeyID int - createErr error + createErr error + // listErr, when set, makes ListReposForViewer fail — the metadata store + // unreachable, which the pages that enumerate databases have to survive. + listErr error createdCalls []*core.Repo deletedRepos []int } @@ -112,6 +115,9 @@ // ListReposForViewer mirrors db.Store's instance-wide listing rule: PUBLIC to // everyone, plus whatever the viewer owns or holds an ACL on. Sorted by id so a // test that depends on the order it hands to /ready gets the same one twice. func (f *fakeStore) ListReposForViewer(_ context.Context, viewer *core.Caller) ([]*core.Repo, error) { + if f.listErr != nil { + return nil, f.listErr + } var out []*core.Repo for _, r := range f.byID { visible := r.Visibility == core.VisibilityPublic