From e0938e942893dda1670ac125b4785c8b8941bfe7 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Wed, 22 Jul 2026 18:03:19 +0300 Subject: [PATCH] refactor: make the filter-polarity trap inexpressible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit service.SpaceFilter meant "empty membership selects nothing" — a newly created project has no members — while search.Query.Spaces was a bare []core.SpaceRef whose empty case meant every space. Passing a project's members into a query therefore turned an empty project into the whole corpus: a silent scope inversion, invisible when it happens, and passing every test written with a non-empty project. The filter moves to core.SpaceFilter with unexported fields, and search.Query takes it whole. There is no slice to hand over any more, so the inversion cannot be written. Its three states are distinct: every space (EverythingFilter), exactly these — possibly none — (SpacesFilter), and the zero value, which is neither. Search refuses the zero value rather than defaulting it, because both plausible defaults are wrong for one of the two callers that can produce one, and returns no hits for a filter that selects no space without asking the index. service.SpaceFilter is now an alias for the core type, so ResolveProject and its callers keep their names. Tests that built a Query without a scope now say core.EverythingFilter(), which is what they always meant. --- core/errors.go | 6 +- core/spacefilter.go | 160 +++++++++++++++++++++++++++++++++++++++ core/spacefilter_test.go | 94 +++++++++++++++++++++++ mcpsrv/mcpsrv_test.go | 10 ++- mcpsrv/search.go | 17 +++-- search/index_test.go | 102 +++++++++++++++++-------- search/mixed_test.go | 16 ++-- search/search.go | 33 ++++++-- service/project.go | 86 +++++---------------- service/project_test.go | 35 ++++++--- web/handlers.go | 7 +- web/web_test.go | 7 +- 12 files changed, 432 insertions(+), 141 deletions(-) create mode 100644 core/spacefilter.go create mode 100644 core/spacefilter_test.go diff --git a/core/errors.go b/core/errors.go index ab8b6daedd56d564f3fbda08569da36c6e08cf0b..253750f5b99ebaf0f1791ca87a26120d3c8dc51e 100644 --- a/core/errors.go +++ b/core/errors.go @@ -1,7 +1,9 @@ // Package core holds the pure domain logic of spec.sr.ht: owner and space name // validation, safe document paths, the globally-unique document ID grammar, the -// frontmatter schema contract, `.spec.yml` space policy, and the proposal state -// machine. +// frontmatter schema contract, `.spec.yml` space policy, the proposal state +// machine and branch naming, and [SpaceFilter] — what a project resolves to and +// what a search is scoped by, which is here rather than in either of them +// because it is the one thing they must agree about. // // It depends on nothing but the standard library and gopkg.in/yaml.v3, never // touches the network or the filesystem, and knows nothing about git, Postgres diff --git a/core/spacefilter.go b/core/spacefilter.go new file mode 100644 index 0000000000000000000000000000000000000000..d75ba920acbeb080f8a1483d352fef87012024e4 --- /dev/null +++ b/core/spacefilter.go @@ -0,0 +1,160 @@ +package core + +import "strings" + +// spaceScope is a SpaceFilter's polarity. It is unexported, and so is every +// field of the filter, because the whole point of the type is that "which +// spaces" cannot be expressed as a bare slice whose empty case means whatever +// the reader assumes. +type spaceScope uint8 + +const ( + // scopeUnset is the zero value: nobody has said which spaces. It is not one + // of the two legal answers, and a caller that reaches a query with it is + // refused rather than defaulted — a filter nobody set is either an error + // return that was ignored or a field somebody forgot, and both of the + // plausible defaults ("everything", "nothing") are wrong for one of them. + scopeUnset spaceScope = iota + // scopeAll excludes nothing: the meta-project. + scopeAll + // scopeNamed selects exactly the spaces named, and nothing when none are. + scopeNamed +) + +// SpaceFilter is which spaces an operation applies to: every space on the +// instance, or a named set — which may be empty. +// +// It exists because those two are not distinguishable in a []SpaceRef, and the +// two conventions for the empty slice both look reasonable in isolation: +// +// - "empty means every space" is what a query filter wants: no terms, no +// restriction. That is what search.Query used to mean by an empty Spaces. +// - "empty means no space" is what a project's membership means: a project is +// a saved filter, and a filter with no terms selects nothing. A freshly +// created project has no members. +// +// Hand a project's membership to a query built on the first convention and an +// empty project silently becomes the whole corpus — the exact opposite of what +// its author asked for, invisible when it happens, and passing every test +// written with a non-empty project. The type makes the inversion inexpressible: +// there is no slice to pass, only a filter that carries its own polarity. +// +// The zero value is neither answer; see scopeUnset. Build one with +// [EverythingFilter] or [SpacesFilter]. +type SpaceFilter struct { + scope spaceScope + refs []SpaceRef + // ids are the storage row ids of refs, index-aligned, and optional: the + // index filters by space reference while document_id, proposal and + // index_stamp key off the row id, so whoever resolved the filter supplies + // whichever it holds. + ids []int +} + +// EverythingFilter is the degenerate filter: it excludes nothing. +// +// This is the meta-project — "merge all my doc work into one searchable thing". +// It is deliberately not an enumeration of every space: enumerating would freeze +// the corpus as of the moment of the resolve, so a space created a second later +// would be missing from "everything" until somebody re-resolved. That is the +// sync job the design says the meta-project does not have, relocated into the +// query path. +func EverythingFilter() SpaceFilter { return SpaceFilter{scope: scopeAll} } + +// SpacesFilter selects exactly the spaces named, and nothing at all when none +// are named — which is what an empty project is, and it must keep meaning that. +// +// ids, when supplied, are the storage row ids of refs and index-aligned with +// them. A caller that holds only one of the two passes nil for the other. +func SpacesFilter(refs []SpaceRef, ids []int) SpaceFilter { + f := SpaceFilter{scope: scopeNamed} + if len(refs) > 0 { + f.refs = append(f.refs, refs...) + } + if len(ids) > 0 { + f.ids = append(f.ids, ids...) + } + return f +} + +// IsZero reports whether nobody has said which spaces this is: the zero value. +// It is not "no spaces" — that is SpacesFilter with an empty membership — and a +// caller holding one has a bug rather than a scope. +func (f SpaceFilter) IsZero() bool { return f.scope == scopeUnset } + +// Everything reports whether the filter excludes nothing. +func (f SpaceFilter) Everything() bool { return f.scope == scopeAll } + +// MatchesNothing reports whether the filter selects no space at all — an empty +// project, or the zero value. Worth asking before running a query: it is the +// one case where the answer is known without touching any index. +func (f SpaceFilter) MatchesNothing() bool { + return !f.Everything() && len(f.refs) == 0 && len(f.ids) == 0 +} + +// Matches reports whether a space is within the filter. +func (f SpaceFilter) Matches(ref SpaceRef) bool { + if f.Everything() { + return true + } + for _, r := range f.refs { + if r == ref { + return true + } + } + return false +} + +// MatchesID reports whether a space row id is within the filter. +func (f SpaceFilter) MatchesID(id int) bool { + if f.Everything() { + return true + } + for _, got := range f.ids { + if got == id { + return true + } + } + return false +} + +// Refs returns the spaces named by the filter, or nil for the meta-project — +// which is unenumerated on purpose, so a caller must ask Everything first +// rather than reading nil as "none". +func (f SpaceFilter) Refs() []SpaceRef { + if len(f.refs) == 0 { + return nil + } + out := make([]SpaceRef, len(f.refs)) + copy(out, f.refs) + return out +} + +// IDs returns the storage row ids of the spaces named by the filter, when the +// resolver supplied them. Same caveat as Refs. +func (f SpaceFilter) IDs() []int { + if len(f.ids) == 0 { + return nil + } + out := make([]int, len(f.ids)) + copy(out, f.ids) + return out +} + +// String renders the filter for logs and error messages, spelling out the +// polarity the type exists to keep visible. +func (f SpaceFilter) String() string { + switch { + case f.Everything(): + return "every space" + case f.IsZero(): + return "no space scope" + case len(f.refs) == 0: + return "no space" + } + names := make([]string, 0, len(f.refs)) + for _, r := range f.refs { + names = append(names, r.String()) + } + return strings.Join(names, ", ") +} diff --git a/core/spacefilter_test.go b/core/spacefilter_test.go new file mode 100644 index 0000000000000000000000000000000000000000..e5b61b1f74e7684ab9c7b0578009a8bbeb9a7bed --- /dev/null +++ b/core/spacefilter_test.go @@ -0,0 +1,94 @@ +package core + +import "testing" + +var ( + fxRfcs = SpaceRef{Owner: "bigbes", Name: "rfcs"} + fxNotes = SpaceRef{Owner: "bigbes", Name: "notes"} +) + +// The three states, and the property the type exists for: none of them can be +// mistaken for another, because none of them is a slice. +func TestSpaceFilterStates(t *testing.T) { + every := EverythingFilter() + if !every.Everything() || every.MatchesNothing() || every.IsZero() { + t.Fatalf("EverythingFilter = %s", every) + } + if !every.Matches(fxRfcs) || !every.MatchesID(99) { + t.Error("the meta filter matches every space") + } + // Unenumerated on purpose: a frozen list would omit every space created + // after the resolve. + if len(every.Refs()) != 0 || len(every.IDs()) != 0 { + t.Errorf("the meta filter enumerated %v", every.Refs()) + } + + // A project nobody has added a space to yet. This is the case that used to + // widen into the whole corpus when it was carried as an empty slice. + none := SpacesFilter(nil, nil) + if none.Everything() || none.IsZero() || !none.MatchesNothing() { + t.Fatalf("SpacesFilter(nil, nil) = %s", none) + } + if none.Matches(fxRfcs) || none.MatchesID(1) { + t.Error("a filter over no spaces matches nothing") + } + + named := SpacesFilter([]SpaceRef{fxRfcs}, []int{7}) + if named.Everything() || named.IsZero() || named.MatchesNothing() { + t.Fatalf("SpacesFilter(one space) = %s", named) + } + if !named.Matches(fxRfcs) || !named.MatchesID(7) { + t.Error("a member space must match") + } + if named.Matches(fxNotes) || named.MatchesID(8) { + t.Error("a space outside the filter must not match") + } + + // The zero value is neither answer, so a caller that never set a scope is + // distinguishable from both — which is what lets a query refuse it instead + // of guessing. + var unset SpaceFilter + if !unset.IsZero() || unset.Everything() || unset.Matches(fxRfcs) { + t.Fatalf("the zero filter = %s", unset) + } +} + +// The accessors hand back copies: a caller that appends to the refs it was +// given must not be able to widen somebody else's filter. +func TestSpaceFilterAccessorsCopy(t *testing.T) { + refs := []SpaceRef{fxRfcs} + f := SpacesFilter(refs, []int{7}) + + refs[0] = fxNotes + if !f.Matches(fxRfcs) || f.Matches(fxNotes) { + t.Error("the filter aliased the slice it was built from") + } + + got := f.Refs() + got[0] = fxNotes + if !f.Matches(fxRfcs) || f.Matches(fxNotes) { + t.Error("Refs handed out the filter's own slice") + } + ids := f.IDs() + ids[0] = 8 + if !f.MatchesID(7) || f.MatchesID(8) { + t.Error("IDs handed out the filter's own slice") + } +} + +func TestSpaceFilterString(t *testing.T) { + var unset SpaceFilter + for _, tc := range []struct { + f SpaceFilter + want string + }{ + {EverythingFilter(), "every space"}, + {SpacesFilter(nil, nil), "no space"}, + {unset, "no space scope"}, + {SpacesFilter([]SpaceRef{fxRfcs, fxNotes}, nil), "~bigbes/rfcs, ~bigbes/notes"}, + } { + if got := tc.f.String(); got != tc.want { + t.Errorf("String() = %q, want %q", got, tc.want) + } + } +} diff --git a/mcpsrv/mcpsrv_test.go b/mcpsrv/mcpsrv_test.go index 422fccc3fd9e7b44bda5e2c36919bd4ec0f9a9a1..2c9cab7b8de6579268dda7cc453c33da7930b426 100644 --- a/mcpsrv/mcpsrv_test.go +++ b/mcpsrv/mcpsrv_test.go @@ -409,11 +409,15 @@ func TestSearchSpaceFilter(t *testing.T) { require.Equal(t, []core.SpaceRef{ {Owner: "bigbes", Name: "rfcs"}, {Owner: "bigbes", Name: "notes"}, - }, s.last.Spaces) + }, s.last.Spaces.Refs()) + require.False(t, s.last.Spaces.Everything(), "naming spaces restricts the search") - // Omitting it is the meta-project: a filter that excludes nothing. + // Omitting it is the meta-project: a filter that excludes nothing. It + // reaches the index saying so, rather than as an empty list that the index + // would have to interpret. call(t, session, "spec_search", map[string]any{"query": "storage"}) - require.Nil(t, s.last.Spaces) + require.True(t, s.last.Spaces.Everything()) + require.False(t, s.last.Spaces.MatchesNothing()) // Sections pass through the same way. call(t, session, "spec_search", map[string]any{"query": "storage", "sections": []string{"specs"}}) diff --git a/mcpsrv/search.go b/mcpsrv/search.go index 428a261d4140ec0efffacac1010ffd89ebed16b2..4e6e73bf82ada9cf3e71c7d867b00cb9dd08b87e 100644 --- a/mcpsrv/search.go +++ b/mcpsrv/search.go @@ -100,19 +100,24 @@ func searchHandler(ctx context.Context, b Backend, in searchInput) (searchOutput // error rather than a dropped filter term: dropping one would silently widen // the search past the set the caller asked for, and a wider answer than // requested is indistinguishable from a correct one. -func parseSpaceFilter(in []string) ([]core.SpaceRef, error) { +// +// An omitted argument is every space — which the tool schema promises — and it +// is returned as the filter that says so. The distinction matters one layer +// down: "the agent named no spaces" is not the same as "the project the agent +// named holds no spaces", and only a filter can tell them apart. +func parseSpaceFilter(in []string) (core.SpaceFilter, error) { if len(in) == 0 { - return nil, nil + return core.EverythingFilter(), nil } - out := make([]core.SpaceRef, 0, len(in)) + refs := make([]core.SpaceRef, 0, len(in)) for _, s := range in { ref, err := parseSpace(s) if err != nil { - return nil, err + return core.SpaceFilter{}, err } - out = append(out, ref) + refs = append(refs, ref) } - return out, nil + return core.SpacesFilter(refs, nil), nil } // trimAll trims each element and refuses an empty one. search/ rejects an empty diff --git a/search/index_test.go b/search/index_test.go index 6001acd92df11b49435ea34be41854a13fa191f9..4e073a3b050e4e1522b80dfd055c54dc254a848d 100644 --- a/search/index_test.go +++ b/search/index_test.go @@ -23,7 +23,7 @@ func TestSearchIgnoresWhitespaceOnlyQuery(t *testing.T) { add("specs/storage.md", "---\ntitle: Needle handbook\n---\n\nreference material\n")) for _, q := range []string{"", " \t\n "} { - res, err := idx.Search(context.Background(), Query{Text: q}) + res, err := idx.Search(context.Background(), Query{Text: q, Spaces: core.EverythingFilter()}) require.NoError(t, err) require.Empty(t, res.Hits) require.Zero(t, res.Total) @@ -37,7 +37,7 @@ func TestTitleMatchOutranksBodyMatch(t *testing.T) { add("specs/handbook.md", "---\nid: SPEC-0001\ntitle: Needle handbook\n---\n\nreference material\n"). add("specs/other.md", "---\nid: SPEC-0002\ntitle: Other document\n---\n\na needle appears in the body\n")) - require.Equal(t, []string{"SPEC-0001", "SPEC-0002"}, hitIDs(t, idx, Query{Text: "needle"})) + require.Equal(t, []string{"SPEC-0001", "SPEC-0002"}, hitIDs(t, idx, Query{Text: "needle", Spaces: core.EverythingFilter()})) } // A project is a saved filter over the one global index. This is the whole of @@ -52,16 +52,16 @@ func TestProjectIsASpaceFilterOverOneIndex(t *testing.T) { idx := indexCorpus(t, rfcs, ops, other) // The meta-project: a filter that excludes nothing. - all := hitIDs(t, idx, Query{Text: "vocabulary"}) + all := hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.EverythingFilter()}) require.ElementsMatch(t, []string{"SPEC-0001", "NOTE-0001", "SPEC-0009"}, all) // A project over two of the three spaces. - project := hitIDs(t, idx, Query{Text: "vocabulary", Spaces: []core.SpaceRef{rfcs.Space, ops.Space}}) + project := hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.SpacesFilter([]core.SpaceRef{rfcs.Space, ops.Space}, nil)}) require.ElementsMatch(t, []string{"SPEC-0001", "NOTE-0001"}, project) // One space. require.Equal(t, []string{"SPEC-0009"}, - hitIDs(t, idx, Query{Text: "vocabulary", Spaces: []core.SpaceRef{other.Space}})) + hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.SpacesFilter([]core.SpaceRef{other.Space}, nil)})) } // Space names are matched whole. Filtering through an analyzed field — which is @@ -75,14 +75,52 @@ func TestSpaceFilterMatchesWholeNamesOnly(t *testing.T) { idx := indexCorpus(t, ops, home) require.Equal(t, []string{"NOTE-0002"}, - hitIDs(t, idx, Query{Text: "vocabulary", Spaces: []core.SpaceRef{home.Space}})) + hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.SpacesFilter([]core.SpaceRef{home.Space}, nil)})) +} + +// The whole reason Query.Spaces is a filter and not a []core.SpaceRef: an +// empty project selects nothing, and "nothing" must never widen to "the entire +// corpus" on the way into a query. With a slice it did — no terms read as no +// restriction — and every test written with a non-empty project passed anyway. +func TestEmptyProjectFindsNothingRatherThanEverything(t *testing.T) { + rfcs := newCorpus(t, "~bigbes/rfcs", "rev1"). + add("specs/storage.md", "---\nid: SPEC-0001\ntitle: Storage model\n---\n\nA shared vocabulary term.\n") + ops := newCorpus(t, "~bigbes/home-ops", "rev1"). + add("notes/hosts.md", "---\nid: NOTE-0001\ntitle: Hosts\n---\n\nAnother shared vocabulary term.\n") + idx := indexCorpus(t, rfcs, ops) + + // What service.ResolveProject returns for a project nobody has added a + // space to yet. + empty := core.SpacesFilter(nil, nil) + require.True(t, empty.MatchesNothing()) + require.False(t, empty.Everything()) + + res, err := idx.Search(context.Background(), Query{Text: "vocabulary", Spaces: empty}) + require.NoError(t, err) + require.Empty(t, res.Hits, "an empty project must return no hits, not the corpus") + require.Zero(t, res.Total) + + // The same query over the meta-project, to show the corpus was there to be + // returned and the filter is what withheld it. + require.Len(t, hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.EverythingFilter()}), 2) +} + +// A scope nobody set is neither answer, and is refused rather than defaulted: +// both plausible defaults are wrong for one of the two callers that could +// produce it. +func TestSearchRefusesAQueryWithNoScope(t *testing.T) { + idx := indexCorpus(t, newCorpus(t, "~bigbes/specs", "rev1"). + add("a.md", "---\ntitle: A\n---\n\nbody text here\n")) + + _, err := idx.Search(context.Background(), Query{Text: "body"}) + require.ErrorContains(t, err, "no space scope") } func TestSearchRejectsAnEmptySpaceFilter(t *testing.T) { idx := indexCorpus(t, newCorpus(t, "~bigbes/specs", "rev1"). add("a.md", "---\ntitle: A\n---\n\nbody text here\n")) - _, err := idx.Search(context.Background(), Query{Text: "body", Spaces: []core.SpaceRef{{}}}) + _, err := idx.Search(context.Background(), Query{Text: "body", Spaces: core.SpacesFilter([]core.SpaceRef{{}}, nil)}) require.ErrorContains(t, err, "empty space") } @@ -93,13 +131,13 @@ func TestLogEntriesAreExcludedUntilAskedFor(t *testing.T) { add("specs/storage.md", "---\nid: SPEC-0001\ntitle: Storage model\n---\n\nThe write path resolves a tree.\n"). add("log.md", "# Log\n\n## [2026-05-31] update | Storage model\nRewrote the write path section.\n")) - require.Equal(t, []string{"SPEC-0001"}, hitIDs(t, idx, Query{Text: "write path"})) + require.Equal(t, []string{"SPEC-0001"}, hitIDs(t, idx, Query{Text: "write path", Spaces: core.EverythingFilter()})) // Naming the section is the way back in. The log document itself carries no // body, so only its entry matches the text. require.Equal(t, []string{"log#2026-05-31-1"}, - hitIDs(t, idx, Query{Text: "write path", Sections: []string{doc.LogSection}})) + hitIDs(t, idx, Query{Text: "write path", Sections: []string{doc.LogSection}, Spaces: core.EverythingFilter()})) require.Equal(t, []string{"log"}, - hitIDs(t, idx, Query{Text: "Log", Sections: []string{doc.LogSection}}), + hitIDs(t, idx, Query{Text: "Log", Sections: []string{doc.LogSection}, Spaces: core.EverythingFilter()}), "the log document stays findable by name") } @@ -108,16 +146,16 @@ func TestSectionFilterRestrictsResults(t *testing.T) { add("specs/storage.md", "---\nid: SPEC-0001\ntitle: Storage\n---\n\nshared vocabulary term\n"). add("notes/scratch.md", "---\nid: NOTE-0001\ntitle: Scratch\n---\n\nshared vocabulary term\n")) - require.Equal(t, []string{"SPEC-0001"}, hitIDs(t, idx, Query{Text: "vocabulary", Sections: []string{"specs"}})) - require.Equal(t, []string{"NOTE-0001"}, hitIDs(t, idx, Query{Text: "vocabulary", Sections: []string{"notes"}})) - require.Len(t, hitIDs(t, idx, Query{Text: "vocabulary", Sections: []string{"specs", "notes"}}), 2) + require.Equal(t, []string{"SPEC-0001"}, hitIDs(t, idx, Query{Text: "vocabulary", Sections: []string{"specs"}, Spaces: core.EverythingFilter()})) + require.Equal(t, []string{"NOTE-0001"}, hitIDs(t, idx, Query{Text: "vocabulary", Sections: []string{"notes"}, Spaces: core.EverythingFilter()})) + require.Len(t, hitIDs(t, idx, Query{Text: "vocabulary", Sections: []string{"specs", "notes"}, Spaces: core.EverythingFilter()}), 2) } func TestSearchRejectsAnEmptySection(t *testing.T) { idx := indexCorpus(t, newCorpus(t, "~bigbes/specs", "rev1"). add("a.md", "---\ntitle: A\n---\n\nbody text here\n")) - _, err := idx.Search(context.Background(), Query{Text: "body", Sections: []string{""}}) + _, err := idx.Search(context.Background(), Query{Text: "body", Sections: []string{""}, Spaces: core.EverythingFilter()}) require.ErrorContains(t, err, "empty section") } @@ -125,7 +163,7 @@ func TestHitCarriesAPinnedAddress(t *testing.T) { idx := indexCorpus(t, newCorpus(t, "~bigbes/specs", "8f14e45fceea167a"). add("specs/storage.md", "---\nid: SPEC-0001\ntitle: Storage model\n---\n\nThe write path resolves a tree.\n")) - res, err := idx.Search(context.Background(), Query{Text: "resolves"}) + res, err := idx.Search(context.Background(), Query{Text: "resolves", Spaces: core.EverythingFilter()}) require.NoError(t, err) require.Len(t, res.Hits, 1) h := res.Hits[0] @@ -147,7 +185,7 @@ func TestSnippetIsHTMLEscaped(t *testing.T) { add("specs/x.md", "---\nid: SPEC-0001\ntitle: Escaping\n---\n\n"+ "A needle inside `` and more prose after it.\n")) - res, err := idx.Search(context.Background(), Query{Text: "needle"}) + res, err := idx.Search(context.Background(), Query{Text: "needle", Spaces: core.EverythingFilter()}) require.NoError(t, err) require.Len(t, res.Hits, 1) require.Contains(t, res.Hits[0].Snippet, "<script>") @@ -164,7 +202,7 @@ func TestRebuildSpaceReplacesTheSpaceAndOnlyTheSpace(t *testing.T) { add("notes/hosts.md", "---\nid: NOTE-0001\ntitle: Hosts\n---\n\nshared vocabulary term\n") idx := indexCorpus(t, rfcs, ops) require.ElementsMatch(t, []string{"SPEC-0001", "SPEC-0002", "NOTE-0001"}, - hitIDs(t, idx, Query{Text: "vocabulary"})) + hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.EverythingFilter()})) next := newCorpus(t, "~bigbes/rfcs", "rev2"). add("specs/keep.md", "---\nid: SPEC-0001\ntitle: Kept\n---\n\nshared vocabulary term, reworded\n"). @@ -176,9 +214,9 @@ func TestRebuildSpaceReplacesTheSpaceAndOnlyTheSpace(t *testing.T) { require.Positive(t, st.Took) require.ElementsMatch(t, []string{"SPEC-0001", "SPEC-0003", "NOTE-0001"}, - hitIDs(t, idx, Query{Text: "vocabulary"})) + hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.EverythingFilter()})) - res, err := idx.Search(context.Background(), Query{Text: "reworded"}) + res, err := idx.Search(context.Background(), Query{Text: "reworded", Spaces: core.EverythingFilter()}) require.NoError(t, err) require.Len(t, res.Hits, 1) require.Equal(t, "rev2", res.Hits[0].Rev, "a surviving document is re-indexed at the new revision") @@ -208,7 +246,7 @@ func TestDeleteSpaceEmptiesIt(t *testing.T) { st, err := idx.DeleteSpace(context.Background(), rfcs.Space) require.NoError(t, err) require.Equal(t, 1, st.Deleted) - require.Equal(t, []string{"NOTE-0001"}, hitIDs(t, idx, Query{Text: "vocabulary"})) + require.Equal(t, []string{"NOTE-0001"}, hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.EverythingFilter()})) n, err := idx.Count() require.NoError(t, err) @@ -232,7 +270,7 @@ func TestRebuildAllReplacesEverything(t *testing.T) { require.Equal(t, 2, st.Indexed) require.Positive(t, st.Took) - require.ElementsMatch(t, []string{"SPEC-0002", "NOTE-0001"}, hitIDs(t, idx, Query{Text: "vocabulary"})) + require.ElementsMatch(t, []string{"SPEC-0002", "NOTE-0001"}, hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.EverythingFilter()})) // Neither working directory survives a successful rebuild. for _, p := range tmpPaths(idx.Path()) { @@ -269,7 +307,7 @@ func TestOpenReusesAnExistingIndex(t *testing.T) { reopened, err := Open(path) require.NoError(t, err) t.Cleanup(func() { require.NoError(t, reopened.Close()) }) - require.Equal(t, []string{"SPEC-0001"}, hitIDs(t, reopened, Query{Text: "vocabulary"})) + require.Equal(t, []string{"SPEC-0001"}, hitIDs(t, reopened, Query{Text: "vocabulary", Spaces: core.EverythingFilter()})) } func TestOpenNeedsAPath(t *testing.T) { @@ -283,7 +321,7 @@ func TestClosedIndexRefusesWork(t *testing.T) { require.NoError(t, idx.Close()) require.NoError(t, idx.Close(), "closing twice is not an error") - _, err = idx.Search(context.Background(), Query{Text: "anything"}) + _, err = idx.Search(context.Background(), Query{Text: "anything", Spaces: core.EverythingFilter()}) require.ErrorContains(t, err, "index is closed") _, err = idx.Count() require.ErrorContains(t, err, "index is closed") @@ -299,17 +337,17 @@ func TestSearchPaginates(t *testing.T) { } idx := indexCorpus(t, c) - res, err := idx.Search(context.Background(), Query{Text: "vocabulary", Limit: 2}) + res, err := idx.Search(context.Background(), Query{Text: "vocabulary", Limit: 2, Spaces: core.EverythingFilter()}) require.NoError(t, err) require.Len(t, res.Hits, 2) require.Equal(t, uint64(5), res.Total, "Total counts matches, not returned hits") - page2, err := idx.Search(context.Background(), Query{Text: "vocabulary", Limit: 2, Offset: 2}) + page2, err := idx.Search(context.Background(), Query{Text: "vocabulary", Limit: 2, Offset: 2, Spaces: core.EverythingFilter()}) require.NoError(t, err) require.Len(t, page2.Hits, 2) require.NotEqual(t, res.Hits[0].ID, page2.Hits[0].ID) - _, err = idx.Search(context.Background(), Query{Text: "vocabulary", Offset: -1}) + _, err = idx.Search(context.Background(), Query{Text: "vocabulary", Offset: -1, Spaces: core.EverythingFilter()}) require.ErrorContains(t, err, "negative offset") } @@ -346,7 +384,7 @@ func TestSameIDInTwoSpacesStaysTwoDocuments(t *testing.T) { add("specs/storage.md", "# Storage\n\nshared vocabulary term\n") idx := indexCorpus(t, a, b) - res, err := idx.Search(context.Background(), Query{Text: "vocabulary"}) + res, err := idx.Search(context.Background(), Query{Text: "vocabulary", Spaces: core.EverythingFilter()}) require.NoError(t, err) require.Len(t, res.Hits, 2) require.ElementsMatch(t, @@ -387,7 +425,7 @@ func TestRebuildAllTimingOnARealisticCorpus(t *testing.T) { t.Logf("RebuildSpace over %d documents: %s", spaceStats.Indexed, spaceStats) for _, q := range []string{"ревизия", "proposal", "предложение"} { - res, err := idx.Search(context.Background(), Query{Text: q, Limit: 10}) + res, err := idx.Search(context.Background(), Query{Text: q, Limit: 10, Spaces: core.EverythingFilter()}) require.NoError(t, err) t.Logf("query %q over %d documents: %d matches in %s", q, st.Indexed, res.Total, res.Took) require.Positive(t, res.Total) @@ -422,7 +460,7 @@ func TestFailedRebuildAllLeavesTheLiveIndexIntact(t *testing.T) { _, err := idx.RebuildAll(ctx, next.extract(t)) require.ErrorIs(t, err, context.Canceled) - require.Equal(t, []string{"SPEC-0001"}, hitIDs(t, idx, Query{Text: "vocabulary"}), + require.Equal(t, []string{"SPEC-0001"}, hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.EverythingFilter()}), "the old index is still open and still answering") for _, p := range tmpPaths(idx.Path()) { _, err := os.Stat(p) @@ -447,7 +485,7 @@ func TestSearchesRunConcurrentlyWithARebuild(t *testing.T) { go func() { defer wg.Done() for range 20 { - _, err := idx.Search(context.Background(), Query{Text: "vocabulary"}) + _, err := idx.Search(context.Background(), Query{Text: "vocabulary", Spaces: core.EverythingFilter()}) assert.NoError(t, err) } }() @@ -462,7 +500,7 @@ func TestSearchesRunConcurrentlyWithARebuild(t *testing.T) { } wg.Wait() - require.Len(t, hitIDs(t, idx, Query{Text: "vocabulary", Limit: 100}), 50) + require.Len(t, hitIDs(t, idx, Query{Text: "vocabulary", Limit: 100, Spaces: core.EverythingFilter()}), 50) } // A document at the space root has no section. Excluding the log section must @@ -472,5 +510,5 @@ func TestRootLevelDocumentsSurviveTheDefaultLogExclusion(t *testing.T) { add("README.md", "---\nid: SPEC-0001\ntitle: Read me\n---\n\nshared vocabulary term\n"). add("specs/a.md", "---\nid: SPEC-0002\ntitle: A\n---\n\nshared vocabulary term\n")) - require.ElementsMatch(t, []string{"SPEC-0001", "SPEC-0002"}, hitIDs(t, idx, Query{Text: "vocabulary"})) + require.ElementsMatch(t, []string{"SPEC-0001", "SPEC-0002"}, hitIDs(t, idx, Query{Text: "vocabulary", Spaces: core.EverythingFilter()})) } diff --git a/search/mixed_test.go b/search/mixed_test.go index c83aaab3e2fec645cc73ae1e23523f396aa53e80..8a6fb4057cbc8af7d7b3dbee5c361b45a95a62fd 100644 --- a/search/mixed_test.go +++ b/search/mixed_test.go @@ -10,6 +10,8 @@ import ( "github.com/blevesearch/bleve/v2/analysis/lang/ru" "github.com/blevesearch/bleve/v2/registry" "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" ) // The design leaves "mixed Russian/English search" open, with warren's @@ -117,15 +119,15 @@ func TestPerLineRoutingFindsBothHalvesOfAMixedDocument(t *testing.T) { idx := indexCorpus(t, c) // Singular English query, plural in the text: only the stemmer bridges it. - require.Equal(t, []string{"SPEC-0003"}, hitIDs(t, idx, Query{Text: "attachment"}), + require.Equal(t, []string{"SPEC-0003"}, hitIDs(t, idx, Query{Text: "attachment", Spaces: core.EverythingFilter()}), "the English half of the mixed document is stemmed") // Singular Russian query, plural in the text. - require.Equal(t, []string{"SPEC-0003"}, hitIDs(t, idx, Query{Text: "вложение"}), + require.Equal(t, []string{"SPEC-0003"}, hitIDs(t, idx, Query{Text: "вложение", Spaces: core.EverythingFilter()}), "the Russian half of the mixed document is stemmed") // The single-language documents are unaffected. - require.Equal(t, []string{"SPEC-0001"}, hitIDs(t, idx, Query{Text: "proposal"})) - require.Equal(t, []string{"SPEC-0002"}, hitIDs(t, idx, Query{Text: "предложение"})) + require.Equal(t, []string{"SPEC-0001"}, hitIDs(t, idx, Query{Text: "proposal", Spaces: core.EverythingFilter()})) + require.Equal(t, []string{"SPEC-0002"}, hitIDs(t, idx, Query{Text: "предложение", Spaces: core.EverythingFilter()})) } // TestMixedDocumentIsLabelledByItsDominantLanguage checks the label a hit @@ -137,14 +139,14 @@ func TestMixedDocumentIsLabelledByItsDominantLanguage(t *testing.T) { add("specs/attachments.md", "---\nid: SPEC-0003\ntitle: Вложения и двоичные файлы\n---\n\n"+mixedSpecBody) idx := indexCorpus(t, c) - res, err := idx.Search(context.Background(), Query{Text: "attachment"}) + res, err := idx.Search(context.Background(), Query{Text: "attachment", Spaces: core.EverythingFilter()}) require.NoError(t, err) require.Len(t, res.Hits, 1) require.Equal(t, LangRU, res.Hits[0].Lang) require.Contains(t, res.Hits[0].Snippet, "attachments", "the snippet comes from the half that matched, even though it is not the document's language") - res, err = idx.Search(context.Background(), Query{Text: "proposal"}) + res, err = idx.Search(context.Background(), Query{Text: "proposal", Spaces: core.EverythingFilter()}) require.NoError(t, err) require.Len(t, res.Hits, 1) require.Equal(t, LangEN, res.Hits[0].Lang) @@ -160,6 +162,6 @@ func TestCyrillicTitleIsSearchableAndBoosted(t *testing.T) { "Здесь упоминается модель хранения данных, но документ совсем о другом предмете.\n") idx := indexCorpus(t, c) - require.Equal(t, []string{"SPEC-0002", "SPEC-0004"}, hitIDs(t, idx, Query{Text: "модель хранения"}), + require.Equal(t, []string{"SPEC-0002", "SPEC-0004"}, hitIDs(t, idx, Query{Text: "модель хранения", Spaces: core.EverythingFilter()}), "the document named by the query outranks the one that mentions it") } diff --git a/search/search.go b/search/search.go index 04b7c84a22af4acb7666ac4bf6659960942ef413..1f2009548cc1c33672c5c7d2f035be15ade29db1 100644 --- a/search/search.go +++ b/search/search.go @@ -67,9 +67,18 @@ type Query struct { Text string // Spaces restricts results to a set of spaces. This is what a project is: // the design's "a project is a saved filter over one global index, not a - // container" is this field and nothing else. Empty means every space, which - // is the meta-project. - Spaces []core.SpaceRef + // container" is this field and nothing else — a project resolves to a + // core.SpaceFilter and it is handed over whole. + // + // It is a filter rather than a []core.SpaceRef because the empty slice had + // two defensible meanings and the two are opposites: here it read as "no + // restriction", while a project's empty membership means "no space". An + // empty project passed into a query therefore used to return the whole + // corpus. The filter carries its own polarity, and the zero value is + // neither answer — Search refuses it rather than guessing, since a scope + // nobody set is a caller bug and both defaults are wrong for one of them. + // core.EverythingFilter() is how a caller says "every space". + Spaces core.SpaceFilter // Sections restricts results to top-level sections ("specs", "notes", // "reports"). Empty means every section except the activity log — see // doc.LogSection: log entries summarise other documents, so left in they @@ -121,6 +130,16 @@ func (x *Index) Search(ctx context.Context, q Query) (Results, error) { if q.Offset < 0 { return Results{}, fmt.Errorf("search: negative offset %d", q.Offset) } + if q.Spaces.IsZero() { + return Results{}, errors.New("search: query names no space scope; " + + "pass core.EverythingFilter() to search every space, or a project's filter to restrict it") + } + // A filter that selects no space — an empty project — has a known answer, + // and it is not "everything". Asking the index would be asking a question + // with no terms in it. + if q.Spaces.MatchesNothing() { + return Results{}, nil + } bq, err := buildQuery(text, q) if err != nil { return Results{}, err @@ -174,9 +193,11 @@ func buildQuery(text string, q Query) (query.Query, error) { match(fieldBodyRU, 1), )) - if len(q.Spaces) > 0 { - want := make([]query.Query, 0, len(q.Spaces)) - for _, sp := range q.Spaces { + // The meta-project adds no term at all: a filter that excludes nothing is + // the absence of a restriction, not the enumeration of every space. + if refs := q.Spaces.Refs(); !q.Spaces.Everything() { + want := make([]query.Query, 0, len(refs)) + for _, sp := range refs { if sp.Owner == "" || sp.Name == "" { return nil, errors.New("search: query carries an empty space") } diff --git a/service/project.go b/service/project.go index 7a57c54d46f8abd0d8227119b2b8a23bac90e2ec..5b6ad15cb613bb81c67d5b385331502872a2972a 100644 --- a/service/project.go +++ b/service/project.go @@ -25,37 +25,16 @@ type Project struct { } // SpaceFilter is what a project resolves to: the set of spaces a query is -// restricted to. It is the entirety of what a project *does*. +// restricted to. It is the entirety of what a project *does*, and it is +// core.SpaceFilter under this package's name. // -// All is not "IDs and Refs happen to be empty", and the distinction is -// load-bearing in both directions: -// -// - All true is the meta-project — a filter that excludes nothing. IDs and -// Refs are deliberately left empty rather than enumerated: enumerating -// would freeze the corpus as of the moment of the resolve, so a space -// created a second later would be missing from "everything" until somebody -// re-resolved. That is the sync job the design says the meta-project does -// not have, relocated into the query path. -// -// - All false with an empty membership is a project that selects *nothing*, -// and it must keep meaning that. Collapsing it into "everything" would -// turn a freshly created, not-yet-populated project into the whole corpus — -// the exact opposite of what its author asked for, and invisible when it -// happens. -// -// Callers translating this into a downstream filter must therefore branch on -// All, not on len(Refs). search.Query.Spaces in particular follows the opposite -// convention (empty means every space), so handing it Refs unconditionally -// turns "selects nothing" into "selects everything". -// -// IDs and Refs are index-aligned: IDs[i] is the row id of Refs[i]. Both are -// present because both are needed — the index filters by space reference, while -// document_id, proposal and index_stamp key off the row id. -type SpaceFilter struct { - All bool - IDs []int - Refs []core.SpaceRef -} +// It lives in core because search.Query takes the same type. The alternative — +// this package's struct translated into a query's own space list at each +// caller — is where the polarity trap lived: an empty project's membership is +// an empty slice, "no terms" reads as "no restriction" to a query filter, and a +// freshly created project silently became the whole corpus. There is no slice +// to hand over any more; a filter carries its own polarity all the way down. +type SpaceFilter = core.SpaceFilter // EverythingFilter is the degenerate filter: the meta-project, "merge all my // doc work into one searchable thing". @@ -67,38 +46,7 @@ type SpaceFilter struct { // meta-project must not be. As a filter that excludes nothing it needs no // storage, no migration data and no maintenance, and adding a space to the // service adds it to the meta-project by construction. -func EverythingFilter() SpaceFilter { return SpaceFilter{All: true} } - -// MatchesNothing reports whether the filter selects no space at all — an empty -// project. Worth asking explicitly before running a query, since it is the one -// case where the answer is known without touching the index. -func (f SpaceFilter) MatchesNothing() bool { return !f.All && len(f.IDs) == 0 } - -// Matches reports whether a space is within the filter. -func (f SpaceFilter) Matches(ref core.SpaceRef) bool { - if f.All { - return true - } - for _, r := range f.Refs { - if r == ref { - return true - } - } - return false -} - -// MatchesID reports whether a space row id is within the filter. -func (f SpaceFilter) MatchesID(id int) bool { - if f.All { - return true - } - for _, got := range f.IDs { - if got == id { - return true - } - } - return false -} +func EverythingFilter() SpaceFilter { return core.EverythingFilter() } // ResolveProject resolves a project reference to the space filter its queries // run under. This is the read the whole feature exists for. @@ -121,15 +69,15 @@ func (s *Service) ResolveProject(ctx context.Context, ref core.ProjectRef) (Spac if err != nil { return SpaceFilter{}, fmt.Errorf("service: resolve project %s: %w", ref, err) } - f := SpaceFilter{ - IDs: make([]int, 0, len(spaces)), - Refs: make([]core.SpaceRef, 0, len(spaces)), - } + refs := make([]core.SpaceRef, 0, len(spaces)) + ids := make([]int, 0, len(spaces)) for _, sp := range spaces { - f.IDs = append(f.IDs, sp.ID) - f.Refs = append(f.Refs, sp.Ref) + refs = append(refs, sp.Ref) + ids = append(ids, sp.ID) } - return f, nil + // Named, not "all": a project with no members selects nothing, and the + // filter says so wherever it is carried. + return core.SpacesFilter(refs, ids), nil } // CreateProject creates an empty project. diff --git a/service/project_test.go b/service/project_test.go index 416ef8c55c624f551b5315ba40f6601bfdd314dc..d18d6b94a19ad8e94ba2705801c04d91b94bff64 100644 --- a/service/project_test.go +++ b/service/project_test.go @@ -23,13 +23,13 @@ func TestEverythingFilterNeedsNoStorage(t *testing.T) { if err != nil { t.Fatalf("ResolveProject(meta): %v", err) } - if !f.All { + if !f.Everything() { t.Fatal("the meta-project must exclude nothing") } // Deliberately unenumerated: a frozen list would omit every space created // after the resolve. - if len(f.Refs) != 0 || len(f.IDs) != 0 { - t.Fatalf("the meta filter enumerated %d spaces; it must not", len(f.Refs)) + if len(f.Refs()) != 0 || len(f.IDs()) != 0 { + t.Fatalf("the meta filter enumerated %d spaces; it must not", len(f.Refs())) } if f.MatchesNothing() { t.Fatal("the meta filter matches everything, not nothing") @@ -41,9 +41,9 @@ func TestEverythingFilterNeedsNoStorage(t *testing.T) { // The trap this type exists to prevent: an empty project is not the corpus. func TestEmptyFilterIsNotEverything(t *testing.T) { - var empty SpaceFilter - if empty.All { - t.Fatal("the zero filter must not be the meta-project") + empty := core.SpacesFilter(nil, nil) + if empty.Everything() { + t.Fatal("a filter over no spaces must not be the meta-project") } if !empty.MatchesNothing() { t.Fatal("a filter with no terms selects nothing") @@ -51,11 +51,19 @@ func TestEmptyFilterIsNotEverything(t *testing.T) { if empty.Matches(fxSpace) || empty.MatchesID(1) { t.Fatal("a filter with no terms must match no space") } + + // And the state that is neither answer: nobody said. It is not silently + // one of the two — a query refuses it, which is what makes "I forgot to + // set the scope" a failure instead of a wrong-scoped answer. + var unset SpaceFilter + if !unset.IsZero() || unset.Everything() { + t.Fatal("the zero filter must be neither everything nor a set scope") + } } func TestSpaceFilterMatches(t *testing.T) { other := core.SpaceRef{Owner: "bigbes", Name: "notes"} - f := SpaceFilter{IDs: []int{7}, Refs: []core.SpaceRef{fxSpace}} + f := core.SpacesFilter([]core.SpaceRef{fxSpace}, []int{7}) if !f.Matches(fxSpace) || !f.MatchesID(7) { t.Error("a member space must match") } @@ -132,9 +140,12 @@ func TestResolveProject(t *testing.T) { if err != nil { t.Fatalf("ResolveProject: %v", err) } - if f.All { + if f.Everything() { t.Fatal("an empty project must not resolve to the meta-project") } + if f.IsZero() { + t.Fatal("a resolved project must carry a scope, not the zero filter") + } if !f.MatchesNothing() { t.Fatalf("an empty project resolved to %+v", f) } @@ -151,11 +162,11 @@ func TestResolveProject(t *testing.T) { if err != nil { t.Fatalf("ResolveProject: %v", err) } - if len(f.Refs) != 1 || f.Refs[0] != fxSpace { - t.Fatalf("filter refs = %+v", f.Refs) + if len(f.Refs()) != 1 || f.Refs()[0] != fxSpace { + t.Fatalf("filter refs = %+v", f.Refs()) } - if len(f.IDs) != 1 || f.IDs[0] != rfcs.ID { - t.Fatalf("filter ids = %+v, want [%d]", f.IDs, rfcs.ID) + if len(f.IDs()) != 1 || f.IDs()[0] != rfcs.ID { + t.Fatalf("filter ids = %+v, want [%d]", f.IDs(), rfcs.ID) } if !f.Matches(fxSpace) || !f.MatchesID(rfcs.ID) { t.Error("the member space must match") diff --git a/web/handlers.go b/web/handlers.go index 8f1769c3061dea20ba4d9a0bac9bea29e0e1f4fd..0635c56433b7739875f611acf6cfc9fcae73cf23 100644 --- a/web/handlers.go +++ b/web/handlers.go @@ -531,14 +531,17 @@ func (s *Server) handleSearch(w http.ResponseWriter, r *http.Request) { data.Spaces = append(data.Spaces, spaceLink{Ref: ref.String(), Href: "/" + ref.String()}) } - query := search.Query{Text: q, Limit: searchLimit} + // No space parameter is a search of every space, said in as many words: + // the filter carries its polarity, so "the viewer named no space" and "the + // viewer named a project with no spaces" cannot collapse into each other. + query := search.Query{Text: q, Limit: searchLimit, Spaces: core.EverythingFilter()} if spaceParam != "" { ref, err := core.ParseSpaceRef(spaceParam) if err != nil { s.fail(w, r, err) return } - query.Spaces = []core.SpaceRef{ref} + query.Spaces = core.SpacesFilter([]core.SpaceRef{ref}, nil) } if q != "" { diff --git a/web/web_test.go b/web/web_test.go index b188bdef1fc9e4663e10a8e251171ae7a39cae2c..b41c2dee70bf2c0be58fccfd563a263702f646af 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -645,8 +645,11 @@ func TestSearchRendersSnippetAsHTML(t *testing.T) { if searcher.last.Text != "authoritative" { t.Fatalf("query text = %q", searcher.last.Text) } - if len(searcher.last.Spaces) != 1 || searcher.last.Spaces[0] != demoSpace { - t.Fatalf("space filter = %+v", searcher.last.Spaces) + if refs := searcher.last.Spaces.Refs(); len(refs) != 1 || refs[0] != demoSpace { + t.Fatalf("space filter = %s", searcher.last.Spaces) + } + if searcher.last.Spaces.Everything() { + t.Fatal("naming a space must restrict the search, not widen it") } body := rec.Body.String() if !strings.Contains(body, "authoritative") {