test(messages_search): drift-guard + empty-keyword accept-path DSL assertions#396
test(messages_search): drift-guard + empty-keyword accept-path DSL assertions#396yujiawei wants to merge 1 commit into
Conversation
…path DSL assertions Two non-blocking P2 follow-ups from the #395 review: 1. Drift guard between hasEffectiveFilters (validate.go) and addCommonFilters (dsl.go). A reflection test forces every exported SearchFilters field to register an effective sample; per-sample assertions then prove the validator guard and the DSL builder agree on that field. Adding a new shared filter to one side without the other now fails CI instead of silently 400-rejecting a legitimate empty-keyword listing or degenerating into a full-channel scan. 2. Accept-path DSL assertions for the keyword-optional listing path: empty keyword + real sender_ids / sent_at window / p2p spaceId now have explicit tests proving the corresponding terms/range/spaceId clauses are emitted, not just that the reject path (blank sender_ids, no multi_match) is blocked. Test-only; no production code touched. Part of #395
lml2468
left a comment
There was a problem hiding this comment.
✅ APPROVE — 模范级的纯测试 PR(+223/-0,无生产代码改动),精准把前几轮反复提的 hasEffectiveFilters/addCommonFilters 口径 drift nit 升级成机械回归护栏。本地实跑全过,无 blocking。
价值:把"靠人记得 lockstep"变成"加字段必红测"
前几轮(#391/#395)大家反复提:hasEffectiveFilters(validator guard)与 addCommonFilters(DSL builder)各自硬编码同一组 filter 字段,加新 filter 漏改一侧会 drift——漏 guard 侧→误拒合法空keyword请求 400;漏 builder 侧→guard 放行但 DSL 无子句→退化全频道扫描(正是 guard 要防的)。本 PR 用测试把这个 drift 变成红测:
- filter_drift_test.go:effectiveFilterSamples 注册表(每字段一个有效样本)+ 反射 TestSearchFilters_NoUnregisteredFields 强制新增 SearchFilters 字段必须登记,否则红测并提示"3 处 lockstep(guard/builder/本测试)";TestEffectiveFilterSamples_GuardAndBuilderAgree 对每样本双侧断言(guard 认可 + builder 真出 filter 子句),任一侧忘了就红。
- accept_path_dsl_test.go:补"接受路径"断言——之前只测拒绝路径(空keyword 无 multi_match),这组证空keyword+真filter 时 DSL 真出对应子句(terms on from / range on timestamp / spaceId term),堵住"回归丢了 filter 子句但旧测试仍绿、悄悄变全扫"的盲区。
核实
- 纯测试,无生产代码改动(2 文件全 _test.go,+223/-0),零回归风险。
- 7 个测试本地实跑全部 PASS,go vet 干净。
- 断言期望值正确:时间戳 from:1767196800(2026-01-01)/to:1798732799(2026-12-31末) 与 parseSentAt 实际输出一致(测试跑过自证);blank sender_ids 被 drop 不进 terms;p2p spaceId term 保留(跨 Space 隔离)。
- drift-guard 覆盖完整:SearchFilters 恰好 3 个导出字段(SenderIDs/SentAtFrom/SentAtTo)与注册表一一对应,反射测试 PASS 自证无遗漏;未来加字段会强制红测。
无 blocking、无 should-fix。这种把"评审中发现的口径一致性隐患"固化成机械护栏的测试,正是防止同类回归的最佳实践。批准。
—— Octo-PR (lml2468)
Jerry-Xin
left a comment
There was a problem hiding this comment.
Test-only PR is in scope for octo-server: it strengthens modules/messages_search coverage around empty-keyword search guards and OpenSearch DSL construction.
💬 Non-blocking
- 🔵 Suggestion:
modules/messages_search/filter_drift_test.go:78only checks that some"filter"key exists afteraddCommonFilters. That is enough for current one-field samples, but future samples could accidentally include multiple effective fields and mask a missing clause. Consider asserting field-specific DSL markers per sample if this registry grows. - 🔵 Suggestion:
modules/messages_search/accept_path_dsl_test.go:129ignores thejson.Marshalerror. It is practically unreachable afterextractDSL, but handling it would match the helper style used earlier in the file.
✅ Highlights
- The PR correctly targets the drift risk between
hasEffectiveFiltersandaddCommonFilters. - The new
_searchand_search_allaccept-path tests cover the important empty-keyword listing path that existing rejection-focused tests did not. - Local verification passed:
go test ./modules/messages_search/....
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
octo-server PR#396 Review Report
PR: #396
Head SHA: abb84d940068f36f5084608a7e47a6f68bf537db
Title: test(messages_search): drift-guard + empty-keyword accept-path DSL assertions
Reviewer: Octo-Q (automated review)
Scope: Test-only (2 new files, +223/-0, zero production code changes)
1. Verification Summary
| Item | Status | Evidence |
|---|---|---|
effectiveFilterSamples covers all 3 SearchFilters fields |
✅ | validate.go:20-24 — struct has exactly SenderIDs, SentAtFrom, SentAtTo; all 3 registered in filter_drift_test.go:33-37 |
hasEffectiveFilters ↔ addCommonFilters parity |
✅ | validate.go:163-178 checks same 3 fields as dsl.go:22-48; drift test asserts both sides per sample |
| Reflection guard covers future field additions | ✅ | filter_drift_test.go:44-55 iterates reflect.TypeOf(SearchFilters{}).NumField() — any new exported field without a sample fails immediately |
| Epoch timestamps in range assertions | ✅ | parseSentAt("2026-01-01", true) in Asia/Shanghai → 1767196800; parseSentAt("2026-12-31", false) → 1798732799 — independently verified via Python datetime |
| Blank sender ID dropped in terms clause | ✅ | addCommonFilters (dsl.go:26-33) skips s == "" before appending to terms slice |
applySpaceIDScope emits spaceId term for p2p |
✅ | dsl.go:80-93 — channelType == channelTypePerson && spaceID != "" → b.Filter(TermQuery("spaceId", spaceID)) |
applyChannelAndRevoked emits channelId term |
✅ | dsl.go:59-62 — b.Filter(TermQuery("channelId", normChannelID)) |
buildSearchAllDSL accept path |
✅ | search_all.go:125-148 — calls addCommonFilters same as _search; type-alias SearchAllReq = SearchMessagesReq (validate.go:76) |
| No overlap / conflict with existing tests | ✅ | Existing dsl_test.go::TestBuildSearchMessagesDSL_NoKeywordSkipsMultiMatch proves rejection path; new tests prove acceptance path — complementary |
2. Findings
No P0/P1 issues found.
P2 — Minor observations (non-blocking)
P2-1: Fragile blank-sender assertion (accept_path_dsl_test.go:56-58)
if strings.Contains(body, `""`) && strings.Contains(body, `"from":["",`) {The first condition ("") matches any empty string in the entire JSON body. The compound check is sufficient for the current test data (no other empty strings present), but a future refactor adding empty-string metadata could produce a false positive. Consider parsing the JSON and inspecting the from array directly. Severity: nit. Diff-scope: new (this PR).
P2-2: _search_all accept-path test less comprehensive than _search (accept_path_dsl_test.go:122-139)
TestBuildSearchAllDSL_EmptyKeywordRealSenderEmitsTerms only asserts terms + u-real. It does not verify channelId, spaceId, or range clauses for _search_all. The _search side has full clause coverage across 4 tests. Acceptable since addCommonFilters is shared code, but a combined-filters test for _search_all would close the gap. Severity: nit. Diff-scope: new (this PR).
3. Suggestions
- Consider JSON-parsing the DSL body in assertions rather than
strings.Containsfor stronger structural guarantees (especially P2-1). - A
TestBuildSearchAllDSL_AllFiltersEmitAllClausesmirroring the_searchcombined test would make the_search_allpath equally bulletproof.
4. Extra Observations
- The drift-guard design is well-thought-out: the reflection test catches unregistered fields, and the per-sample guard+builder assertion catches registration without implementation. The failure message pointing at all 3 wire-up locations is excellent DX.
- The
effectiveFilterSamplesmap is intentionally minimal (one sample per field). The combined-filters test inTestEmptyKeyword_AllFiltersEmitAllClausescovers interaction effects.
5. Data-Flow Trace
| Consumed Data | Upstream Source | Reaches Consumer? |
|---|---|---|
SearchFilters.SenderIDs |
Test constructs []string{"", "u-real"} |
✅ → addCommonFilters drops "", keeps "u-real" → terms query on from |
SearchFilters.SentAtFrom |
Test sets "2026-01-01" |
✅ → parseSentAt in Asia/Shanghai → epoch 1767196800 → range.gte on timestamp |
SearchFilters.SentAtTo |
Test sets "2026-12-31" |
✅ → parseSentAt + end-of-day → epoch 1798732799 → range.lte on timestamp |
spaceID param |
Test passes "spaceX" |
✅ → applySpaceIDScope for channelTypePerson → term on spaceId |
normChannelID param |
Test passes "G1" / "fake-cid" |
✅ → applyChannelAndRevoked → term on channelId |
hasEffectiveFilters return |
Reads SenderIDs, SentAtFrom, SentAtTo from SearchFilters |
✅ → drift test asserts true for each sample |
| Reflection field iteration | reflect.TypeOf(SearchFilters{}).NumField() = 3 |
✅ → all 3 fields present in effectiveFilterSamples |
6. R5 Blind-Spot Checklist
- C1 (dual-path parity): N/A — test-only PR, no add/remove or create/delete paths touched.
- C2 (control-flow ordering / nesting): Clear — test helpers (
emptyKeywordDSLBody) call production functions once with no nesting or double-application risk.extractDSLis an existing helper reused correctly. - C3 (authorization boundary ≠ capability): N/A — no auth/authz code modified.
- C4 (authorization lifecycle / container-member cascade): N/A — no auth code modified.
7. Cross-Round Blocker Recheck
N/A — first review of this PR.
[Octo-Q] verdict: APPROVE
Test-only PR with well-structured anti-drift guards and accept-path assertions. All test assertions verified against production code data flows. No P0/P1 findings. Two minor nits (P2) noted for optional improvement.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #396 (octo-server)
Scope: test-only PR at head abb84d94 — adds filter_drift_test.go (84 LOC) and accept_path_dsl_test.go (139 LOC) under modules/messages_search/. No production code touched (confirmed by diff stat: +223/-0, 2 files).
1. Verification results
| Check | Result | Evidence |
|---|---|---|
go test ./modules/messages_search/... |
✅ PASS | clean |
go test -race ./modules/messages_search/ |
✅ PASS | no data races |
go vet ./modules/messages_search/... |
✅ clean | |
gofmt -l on both new files |
✅ clean | |
go build ./modules/messages_search/... |
✅ OK | |
| Drift guard is load-bearing | ✅ verified | injecting a MessageTypes []string field into SearchFilters makes TestSearchFilters_NoUnregisteredFields fail exactly as designed (with the 3-place wiring message) |
| Hardcoded epochs CI-portable | ✅ verified | 1767196800/1798732799 = 2026-01-01/2026-12-31 in Asia/Shanghai; userTimeZone is pinned to Asia/Shanghai with a FixedZone("CST", 8*3600) fallback (util.go:10), so the values do not depend on the CI host's TZ — no flakiness |
All PR claims hold up.
2. Assessment
This is a high-quality test-only follow-up to #395. Two complementary mechanisms:
-
filter_drift_test.go— a reflection-driven invariant tyingvalidate.go::hasEffectiveFilters(the empty-search guard) todsl.go::addCommonFilters(the query builder).TestSearchFilters_NoUnregisteredFieldsforces every exportedSearchFiltersfield to register aneffectiveFilterSamplesentry;TestEffectiveFilterSamples_GuardAndBuilderAgreethen proves, per field, that the guard counts it and the builder emits a clause. This closes the real failure modes the description names: a new shared filter wired into only one side either 400-rejects a legitimate empty-keyword listing or silently degenerates into a full-channel scan. The guard provably catches both drift directions (I confirmed: guard-but-not-builder → empty bool → no"filter"key → red; builder-but-not-guard →hasEffectiveFilters=false → red). -
accept_path_dsl_test.go— converts the prior "reject-path only" coverage into live accept-path assertions: empty keyword + realsender_ids/sent_at/p2pspaceIdactually emit the correspondingterms/range/spaceIdclauses on both_searchand_search_all. A regression that dropped a filter clause would have passed the old suite while turning every listing into a full-channel scan.
Tests are deterministic, correctly exercise the production code paths, and the assertions match actual olivere/elastic serialization.
3. Non-blocking suggestions (P2 — optional, no merge gate)
These are clarity/robustness nits in the test assertions; none affects correctness, determinism, or the load-bearing property:
accept_path_dsl_test.go:53— the blank-sender-id drop checkstrings.Contains(body, "") && strings.Contains(body, "\"from\":[\"\",")has a dead first conjunct (Contains(body, "")is always true) and is coupled to slice order: it catches a leading blank id but would silently pass a trailing one (["u-real",""]). For the input the test actually uses (blank-first) it works, and blank-drop is independently covered byvalidate_test.go. Consider parsing the JSON and asserting thefromterms equal exactly["u-real"].accept_path_dsl_test.go:108(TestEmptyKeyword_AllFiltersEmitAllClauses) — the bare"from"substring can be satisfied by the range clause'sfromlower-bound key, not only the terms field. The companion"terms"+"u-real"assertions still uniquely pin the sender clause, so there's no false confidence; tightening to"terms":{"from"would remove the ambiguity.filter_drift_test.go:78— the builder-side assertion checks for a generic"filter"key rather than the field-specific OS key (from/timestamp). Sound today under the one-field-per-sample invariant; asserting the per-field target key would harden it against a future sample that sets two fields.accept_path_dsl_test.go:83-84— comment says "spaceId itself plus a sender id keep it a legitimate request," butspaceIdis not aSearchFiltersfield and plays no role inhasEffectiveFilters; only the sender id satisfies the guard. The assertion itself is correct — just the rationale comment over-claims. Worth rewording so a future maintainer doesn't infer that a spaceId-only empty-keyword p2p request would pass validation (it would 400).
4. Coverage notes (blind spots, not defects)
- The accept-path proof covers
_searchand_search_allonly;_search_files/_search_media/_search_aroundalso calladdCommonFiltersbut have no "filter reaches the builder" assertion.space_filter_test.gopins all four endpoints forspaceId(a separate code path) but not foraddCommonFiltersreachability. A symmetric table-driven test across the five builders would close this — fine as a fast-follow, not required here. - The drift guard binds
hasEffectiveFilters ↔ addCommonFilters; a filter that other endpoints need butaddCommonFiltersomits is outside its stated scope (by design).
Verdict
APPROVED. No P0/P1. All correctness, build, vet, fmt, race, and load-bearing checks pass; the suggestions above are optional polish that do not block merge.
QA Verdict: APPROVEScope: Test-only PR ( Findings
Verify
Verdict: APPROVE. The test design is a real anti-drift hinge, the accept-path assertions close a documented coverage gap, and the only CI red is in an unrelated module due to a missing IM service in the runner. |
Security Verdict: CLEAREDScope: Test-only PR. Two new STRIDE / OWASP sweep
AuthZ posture (relevant for context): the production guard under test ( Crypto: no crypto code, no key material, no entropy source touched. N/A. Dependency / SBOM diff: CI evidence: Verdict: CLEARED. Test-only change that hardens an existing read-side boundary; zero new attack surface. |
Code Verdict: APPROVEScope: 2 new files, 223 added lines, test code only. Same package Design
Correctness
Readability / Maintainability
Nits (non-blocking)
Verify
Verdict: APPROVE. Disciplined, well-documented test additions that turn an implicit invariant (validator/builder field-set parity) into an enforced one and close a real accept-path coverage gap. Nits are stylistic. |
Aggregate Verdict: APPROVED — 等人工合并
下一步: wait-human-merge。绝不自动合并。 管理 by pr-tick-octo-server autopilot · tracking: OCTO-497 |
What
Two non-blocking test-only follow-ups from the #395 review (empty-keyword search + empty-search guard).
1. Anti-drift guard:
hasEffectiveFilters↔addCommonFiltersvalidate.go::hasEffectiveFilters(the empty-search guard) anddsl.go::addCommonFilters(the query builder) independently hardcode the sameSearchFiltersfield set. Adding a new shared filter (e.g.message_types) to one without the other silently breaks the "empty keyword + new filter" listing path:400.filter_drift_test.goadds a reflection test that forces every exportedSearchFiltersfield to register an effective sample, plus a per-sample assertion that the guard counts it AND the builder emits an OpenSearch clause for it. A new field with no sample fails immediately with a message pointing at the three places to wire it. Verified load-bearing: temporarily adding aMessageTypesfield made the test fail as designed.2. Accept-path DSL assertions
The existing empty-keyword tests only prove the reject path (no
multi_match, blanksender_idsdropped).accept_path_dsl_test.goproves the listing path is alive: empty keyword + realsender_ids/sent_atwindow / p2pspaceIdactually emit the correspondingterms/range/spaceIdclauses on both_searchand_search_all. A regression that dropped a filter clause would have passed the old suite while silently turning every listing into a full-channel scan.Test
go test ./modules/messages_search/...→ PASSgo build ./...,go vet ./modules/messages_search/...→ cleangofmtcleanTest-only; no production code touched.
Part of #395