diff --git a/beads/build.go b/beads/build.go index 51031d70561b845aae813eef56460e8d8ecd5062..659e21c07f89f662bb82a67a21724a470b58dfc0 100644 --- a/beads/build.go +++ b/beads/build.go @@ -77,7 +77,7 @@ opts := collectFilterOptions(issues, issueCols, labelsByIssue) // Bucket every matching issue into exactly one lane. var rolling, linedUp, stalled, pastStand []Card - for _, r := range issues.Rows { + for _, r := range rowsOf(issues) { id := cell(issueCols, r, "id") if !filter.matches(id, r, issueCols, labelsByIssue[id]) { continue @@ -182,9 +182,9 @@ ) *Data { // id → (title, status, whole row) for edge labels and the subtask rollup. titleByIssue := map[string]string{} statusByIssue := map[string]string{} - rowByID := make(map[string][]string, len(issues.Rows)) - var row []string - for _, r := range issues.Rows { + rowByID := make(map[string]rowCells, len(issues.Rows)) + var row rowCells + for _, r := range rowsOf(issues) { id := cell(issueCols, r, "id") titleByIssue[id] = cell(issueCols, r, "title") statusByIssue[id] = cell(issueCols, r, "status") @@ -195,7 +195,7 @@ } } data := &Data{Mode: "detail", Truncated: clip.truncated, ShownOf: clip.issuesTotal} - if row == nil { + if row.values == nil { // Unknown id: a detail pane with a nil Issue. Whether that is an answer or // an admission is Data.MissingBeyondCap's to tell — when the issues table // was clipped at Max the id may simply live in the tail that was never @@ -247,7 +247,7 @@ Status: st, Closed: statusCategory(st, catByStatus) == "closed", } } - for _, r := range deps.Rows { + for _, r := range rowsOf(deps) { from := cell(depCols, r, "issue_id") to := cell(depCols, r, "depends_on_issue_id") typ := cell(depCols, r, "type") @@ -292,7 +292,7 @@ // reach past the direct edges (a Depth>0 node), so they add the chain the // flat Depends-on / Depended-on-by lists can't show, without duplicating them. outAdj := map[string][]depLink{} // id → things it depends on inAdj := map[string][]depLink{} // id → things that depend on it - for _, r := range deps.Rows { + for _, r := range rowsOf(deps) { from := cell(depCols, r, "issue_id") to := cell(depCols, r, "depends_on_issue_id") if from == "" || to == "" { @@ -316,7 +316,7 @@ // counts towards the flag like any other input. if comments, commentsTotal, err := readRowsOptional(ctx, sess, ref, "comments"); err == nil && comments != nil { data.Truncated = data.Truncated || commentsTotal > Max ccols := indexCols(comments.Columns) - for _, r := range comments.Rows { + for _, r := range rowsOf(comments) { if cell(ccols, r, "issue_id") != want { continue } @@ -339,7 +339,7 @@ // in the History tab as humanized, time-ordered entries. if events, eventsTotal, err := readRowsOptional(ctx, sess, ref, "events"); err == nil && events != nil { data.Truncated = data.Truncated || eventsTotal > Max ecols := indexCols(events.Columns) - for _, r := range events.Rows { + for _, r := range rowsOf(events) { if cell(ecols, r, "issue_id") != want { continue } @@ -365,7 +365,7 @@ // collectFilterOptions gathers the distinct issue_type / priority / assignee // values and label names across all issues, sorted, for the filter dropdowns. func collectFilterOptions(issues *browse.RowPage, cols map[string]int, labelsByIssue map[string][]string) FilterOptions { types, prios, assignees, labels := map[string]bool{}, map[string]bool{}, map[string]bool{}, map[string]bool{} - for _, r := range issues.Rows { + for _, r := range rowsOf(issues) { if t := cell(cols, r, "issue_type"); t != "" { types[t] = true } @@ -392,7 +392,7 @@ // issueCreatedAt maps issue id → created_at string, for lane sorting. func issueCreatedAt(issues *browse.RowPage, cols map[string]int) map[string]string { m := make(map[string]string, len(issues.Rows)) - for _, r := range issues.Rows { + for _, r := range rowsOf(issues) { m[cell(cols, r, "id")] = cell(cols, r, "created_at") } return m diff --git a/beads/events.go b/beads/events.go index 95bf4649b6bd4322d96e2eecfd94c621794df7c0..a0de93f189f6f0e9dc2a1f5e1fc8a2b658cb6710 100644 --- a/beads/events.go +++ b/beads/events.go @@ -97,11 +97,16 @@ } return strings.Join(parts, ", ") } -// decodeJSONObject parses raw into a map, tolerating the browse NULL placeholder -// and non-object payloads (returns nil rather than erroring). +// decodeJSONObject parses raw into a map, tolerating an empty value and +// non-object payloads (returns nil rather than erroring). +// +// It used to special-case the string "NULL" as well, back when that string was +// how an absent value reached it. cell answers an absent value as "" now, so a +// "NULL" arriving here is four characters a row actually stores — which is not a +// JSON object, and takes the same nil the parse error gives it. func decodeJSONObject(raw string) map[string]any { raw = strings.TrimSpace(raw) - if raw == "" || raw == "NULL" { + if raw == "" { return nil } var m map[string]any diff --git a/beads/memory.go b/beads/memory.go index 2ee35ca661fb74a0fee2e0c560526940c2c8c9d6..d7aa2e5f93ba6f189c2eab09b315ef1c8f540377 100644 --- a/beads/memory.go +++ b/beads/memory.go @@ -158,7 +158,7 @@ // normalising first would make two spellings of the same text look equal. cols := indexCols(rows.Columns) raw := map[string]string{} texts := map[string]string{} - for _, r := range rows.Rows { + for _, r := range rowsOf(rows) { key := cell(cols, r, "key") if !strings.HasPrefix(key, memoryPrefix) { continue @@ -346,7 +346,7 @@ if rows == nil { return out, nil } cols := indexCols(rows.Columns) - for _, r := range rows.Rows { + for _, r := range rowsOf(rows) { if key := cell(cols, r, "key"); key != "" { out[key] = cell(cols, r, "value") } diff --git a/beads/milestones.go b/beads/milestones.go index 9dd24b4455d8550f1a3e67f8733f3bcff6585e92..024e1db51d21671a7e287ff99c3e0ae159601685 100644 --- a/beads/milestones.go +++ b/beads/milestones.go @@ -85,7 +85,7 @@ // child issue → its parent-child parents; used to nest tasks under epics. parentsByChild := map[string][]string{} if deps != nil { cols := indexCols(deps.Columns) - for _, r := range deps.Rows { + for _, r := range rowsOf(deps) { if !strings.EqualFold(cell(cols, r, "type"), "parent-child") { continue } @@ -104,7 +104,7 @@ issueCols := indexCols(issues.Columns) byLabel := map[string]*MilestoneDetail{} cardsByLabel := map[string][]Card{} unlabeled := 0 - for _, r := range issues.Rows { + for _, r := range rowsOf(issues) { id := cell(issueCols, r, "id") cat := statusCategory(cell(issueCols, r, "status"), catByStatus) card := Card{ diff --git a/beads/model.go b/beads/model.go index 3236ede8d0a393b2ef72c31ea8f5be41784ad3b8..49000b9768cd53ca3725412a9cd128f806685c37 100644 --- a/beads/model.go +++ b/beads/model.go @@ -108,7 +108,7 @@ return f.Query != "" || f.Type != "" || f.Priority != "" || f.Assignee != "" || f.Label != "" || f.Ready } // matches reports whether one issue row passes every set filter. -func (f Filter) matches(id string, row []string, cols map[string]int, labels []string) bool { +func (f Filter) matches(id string, row rowCells, cols map[string]int, labels []string) bool { if f.Type != "" && cell(cols, row, "issue_type") != f.Type { return false } diff --git a/beads/prefixes.go b/beads/prefixes.go index 5dea333690fdda528b1ce067b8fa43efea7202c0..2badc56112f02beea17a7364d161c4db5cd3bdae 100644 --- a/beads/prefixes.go +++ b/beads/prefixes.go @@ -187,7 +187,7 @@ if rows == nil { return "", nil } cols := indexCols(rows.Columns) - for _, r := range rows.Rows { + for _, r := range rowsOf(rows) { if cell(cols, r, "key") != prefixKey { continue } diff --git a/beads/ready.go b/beads/ready.go index b3406620f17486139703defd96196d2882832d7c..a4ca796026db3697f0448f9b236fc54d31c3b0f0 100644 --- a/beads/ready.go +++ b/beads/ready.go @@ -415,7 +415,7 @@ depIdx := indexDeps(deps, catByIssue) labelsByIssue := indexLabels(labels) var cards []Card - for _, r := range issues.Rows { + for _, r := range rowsOf(issues) { id := cell(issueCols, r, "id") cat := catByIssue[id] blocked := truthy(cell(issueCols, r, "is_blocked")) || depIdx.blockedOpen[id] diff --git a/beads/rows.go b/beads/rows.go index 54eef8de7ece7e248fae2aa32152a1da03606d64..b80c1335adc9e7bcb877922427058c6bda06c71b 100644 --- a/beads/rows.go +++ b/beads/rows.go @@ -43,14 +43,72 @@ } return m } +// rowCells is one row of a page together with the NULL mask browse returned +// beside it: the rendered strings, and the answer to "does this cell hold a +// value at all?" that the strings cannot carry. +// +// It is what every projection here iterates and what cell reads, so a row and +// its mask travel together and cannot be paired up wrongly at a call site. +type rowCells struct { + values []string + + // nulls is browse's mask for this row, or nil for a page that carried none — + // which is a hand-built page, since browse fills one for every page it + // returns. See cell for what an absent mask means. + nulls []bool +} + +// rowsOf pairs each row of a page with its own mask. A nil page — an optional +// table that is absent — has no rows, which is what the callers of +// readRowsOptional already treat it as. +// +// A row the mask does not cover gets a nil one rather than an all-false one: not +// knowing whether a cell holds a value is a different answer from knowing that +// it does, and cell reads the two differently. +func rowsOf(page *browse.RowPage) []rowCells { + if page == nil { + return nil + } + out := make([]rowCells, 0, len(page.Rows)) + for i, r := range page.Rows { + var mask []bool + if i < len(page.Nulls) { + mask = page.Nulls[i] + } + out = append(out, rowCells{values: r, nulls: mask}) + } + return out +} + // cell returns the named column's value for a row, or "" when the column is -// absent, out of range, or the literal browse NULL placeholder. -func cell(cols map[string]int, row []string, name string) string { +// absent, out of range, or holds no value at all. +// +// A cell that holds no value reads as "": every projection renders a missing +// timestamp, assignee or close reason as nothing, and that must not change. What +// the mask changes is the other reading of the same string — browse renders a +// real NULL as the text "NULL", so a row that *stores* those four characters +// rendered identically and was flattened to "" too, which turned a stored title +// into an empty one. The mask beside the row answers which of the two it is, so +// it decides here rather than the string. +// +// A cell no mask covers is read the way this package read every cell before the +// mask existed: "NULL" is absent. That is not a guess dressed up as an answer — +// it is the older reading, kept for the only pages that lack a mask, which are +// the ones built by hand rather than read from a store. browse fills a mask +// parallel to the rows for every page it returns, so no read of a database +// arrives here without one. +func cell(cols map[string]int, row rowCells, name string) string { i, ok := cols[name] - if !ok || i < 0 || i >= len(row) { + if !ok || i < 0 || i >= len(row.values) { return "" } - v := row[i] + if i < len(row.nulls) { + if row.nulls[i] { + return "" + } + return row.values[i] + } + v := row.values[i] if v == "NULL" { return "" } @@ -66,7 +124,7 @@ if statuses == nil { return out } cols := indexCols(statuses.Columns) - for _, r := range statuses.Rows { + for _, r := range rowsOf(statuses) { if name := cell(cols, r, "name"); name != "" { out[strings.ToLower(name)] = strings.ToLower(cell(cols, r, "category")) } @@ -82,7 +140,7 @@ if labels == nil { return out } cols := indexCols(labels.Columns) - for _, r := range labels.Rows { + for _, r := range rowsOf(labels) { id := cell(cols, r, "issue_id") lb := cell(cols, r, "label") if id != "" && lb != "" { @@ -96,7 +154,7 @@ // indexIssueCategories maps issue id → status category (open / in_progress / // closed), which is what decides whether a blocking target still blocks. func indexIssueCategories(issues *browse.RowPage, cols map[string]int, catByStatus map[string]string) map[string]string { out := make(map[string]string, len(issues.Rows)) - for _, r := range issues.Rows { + for _, r := range rowsOf(issues) { out[cell(cols, r, "id")] = statusCategory(cell(cols, r, "status"), catByStatus) } return out @@ -125,7 +183,7 @@ if deps == nil { return idx } cols := indexCols(deps.Columns) - for _, r := range deps.Rows { + for _, r := range rowsOf(deps) { from := cell(cols, r, "issue_id") to := cell(cols, r, "depends_on_issue_id") typ := strings.ToLower(cell(cols, r, "type")) @@ -151,7 +209,7 @@ // Every surface that says "ready" — the board's ⚡ marker and its ?ready=1 // filter, the cross-database /ready page, the MCP ready_work tool — comes // through here, because two surfaces that each spelled the rule out would // disagree the first time it changed. -func readyRow(cat string, blocked bool, row []string, cols map[string]int) bool { +func readyRow(cat string, blocked bool, row rowCells, cols map[string]int) bool { return cat == "open" && !blocked && !truthy(cell(cols, row, "is_template")) && !truthy(cell(cols, row, "ephemeral")) } diff --git a/beads/rows_null_test.go b/beads/rows_null_test.go new file mode 100644 index 0000000000000000000000000000000000000000..31327645a5b0934de89a32cfdc46f294d0655425 --- /dev/null +++ b/beads/rows_null_test.go @@ -0,0 +1,132 @@ +package beads + +import ( + "context" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-dolt/browse" +) + +// The two readings of the string "NULL", and the mask that tells them apart. +// +// browse renders a cell that holds no value as the text "NULL", which is what a +// row storing those four characters renders as too. Flattening both to "" — as +// this package did before the mask existed — is right for the first and wrong +// for the second: an issue titled "NULL" arrived with an empty title, and no +// projection could tell that it had one. + +// nullMaskFixture is one issues table carrying both readings in the same +// columns: i-absent holds no value in title or closed_at, i-stored stores the +// text "NULL" in both. +func nullMaskFixture() *fakeSession { + issues := &browse.RowPage{ + Columns: []string{"id", "title", "status", "priority", "issue_type", "assignee", "created_at", "closed_at", "is_blocked"}, + Rows: [][]string{ + {"i-absent", "NULL", "open", "1", "task", "alice", "2024-01-01", "NULL", "0"}, + {"i-stored", "NULL", "open", "1", "task", "alice", "2024-01-02", "NULL", "0"}, + }, + Nulls: [][]bool{ + // title and closed_at hold no value at all. + {false, true, false, false, false, false, false, true, false}, + // every cell holds a value; the two "NULL"s are stored text. + {false, false, false, false, false, false, false, false, false}, + }, + Total: 2, + } + deps := &browse.RowPage{ + Columns: []string{"id", "issue_id", "depends_on_issue_id", "type"}, + Rows: [][]string{}, + Nulls: [][]bool{}, + } + return &fakeSession{rowsByTable: map[string]*browse.RowPage{ + "issues": issues, + "dependencies": deps, + }} +} + +// cell is where the distinction is actually made, so it is asked directly first: +// the same string, read two ways, decided by the mask beside it. +func TestCellReadsTheNullMaskAndNotTheString(t *testing.T) { + cols := indexCols([]string{"id", "title"}) + + absent := rowCells{values: []string{"i-absent", "NULL"}, nulls: []bool{false, true}} + stored := rowCells{values: []string{"i-stored", "NULL"}, nulls: []bool{false, false}} + + assert.Equal(t, "", cell(cols, absent, "title"), + "a cell that holds no value reads as empty, which every projection depends on") + assert.Equal(t, "NULL", cell(cols, stored, "title"), + "a cell that stores the text \"NULL\" has a value, and it is that text") + + assert.Equal(t, "", cell(cols, stored, "nosuchcolumn"), "an absent column is still empty") +} + +// A page that carries no mask at all cannot answer the question, and the reading +// that predates the mask is all there is: "NULL" reads as absent. Every page +// browse returns carries one, so this is about a page built by hand. +func TestCellWithoutAMaskKeepsTheOlderReading(t *testing.T) { + cols := indexCols([]string{"id", "title"}) + unmasked := rowCells{values: []string{"i-1", "NULL"}} + + assert.Equal(t, "", cell(cols, unmasked, "title")) + assert.Equal(t, "i-1", cell(cols, unmasked, "id")) +} + +// rowsOf pairs each row with its own mask, and a page with none hands out rows +// that answer "unknown" rather than rows that answer "not null". +func TestRowsOfPairsEveryRowWithItsMask(t *testing.T) { + page := &browse.RowPage{ + Columns: []string{"id", "title"}, + Rows: [][]string{{"a", "NULL"}, {"b", "NULL"}}, + Nulls: [][]bool{{false, true}, {false, false}}, + } + rows := rowsOf(page) + require.Len(t, rows, 2) + assert.Equal(t, []bool{false, true}, rows[0].nulls) + assert.Equal(t, []bool{false, false}, rows[1].nulls) + + unmasked := rowsOf(&browse.RowPage{Columns: []string{"id"}, Rows: [][]string{{"a"}}}) + require.Len(t, unmasked, 1) + assert.Nil(t, unmasked[0].nulls, "a page with no mask answers no mask, not an all-false one") + + assert.Empty(t, rowsOf(nil), "a table that is absent has no rows to pair") +} + +// The projections are where it is felt: a title that is literally "NULL" must +// survive to the detail pane, and a closed_at that holds no value must keep +// reading as empty. +func TestProjectionsKeepAStoredNullAndDropARealOne(t *testing.T) { + ctx := context.Background() + + stored, err := Build(ctx, nullMaskFixture(), "main", url.Values{"issue": {"i-stored"}}) + require.NoError(t, err) + require.NotNil(t, stored.Issue) + assert.Equal(t, "NULL", stored.Issue.Title, + "the row stores those four characters: an empty title would lose them") + assert.Equal(t, "NULL", stored.Issue.ClosedAt) + + absent, err := Build(ctx, nullMaskFixture(), "main", url.Values{"issue": {"i-absent"}}) + require.NoError(t, err) + require.NotNil(t, absent.Issue) + assert.Equal(t, "", absent.Issue.Title, "no value is no text, as every projection reads it") + assert.Equal(t, "", absent.Issue.ClosedAt) +} + +// The board goes through the same rows, and the same two readings have to reach +// the cards. +func TestBoardCardsKeepAStoredNullTitle(t *testing.T) { + d, err := Build(context.Background(), nullMaskFixture(), "main", url.Values{}) + require.NoError(t, err) + + titles := map[string]string{} + for _, lane := range d.Lanes { + for _, c := range lane.Issues { + titles[c.ID] = c.Title + } + } + assert.Equal(t, "NULL", titles["i-stored"]) + assert.Equal(t, "", titles["i-absent"]) +}