mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
rewrite(search): ChannelScope enum closes ChannelLessOnly fence hole
The legacy 2x2 `(channel_ids: Option<Vec<Uuid>>, include_channel_less: bool)` shape could not unambiguously express "channel-less events only" — both `Some(vec![]) + true` and `None + true` fell into the no-constraint branch, silently broadening to all community channels rather than restricting to `channel_id IS NULL`. That matched the legacy Typesense `channel_id:=__global__` sentinel one way (per-channel + global) but not the other (global only).
Replace with a single `ChannelScope` enum whose four variants are 1-to-1 with the legacy `(accessible_channels, include_global)` matrix:
- non-empty + true -> ChannelsOrChannelLess(accessible)
- non-empty + false -> Channels(accessible)
- empty + true -> ChannelLessOnly (the variant the old shape could not express)
- empty + false -> caller short-circuits to EOSE, doesn't call search
Emitted SQL fragments are byte-identical to the legacy match for the three carry-over cases; `ChannelLessOnly` adds `AND channel_id IS NULL` — the fence the old type could not express.
Verification:
- Full package `cargo test -p buzz-search -- --include-ignored --test-threads=1`: 9/9 green (8 existing + 1 new `channel_less_only_excludes_per_channel_events`).
- Adversarial mutation: replaced the `ChannelLessOnly` SQL emission with a no-op (the buggy semantic the old shape produced); new test went RED with 3 hits instead of 1, restored, green again. The fix is the emitted predicate, not the variant name.
- clippy -D warnings clean; fmt clean.
- Empty-vec edge cases are intentionally not special-cased: `Channels(vec![])` emits `channel_id = ANY('{}')` (false-for-all, zero hits, preserves the old early-return semantic via SQL); `ChannelsOrChannelLess(vec![])` is equivalent to `ChannelLessOnly`.
Coordinated with Eva ahead of relay-wiring sweep at req.rs and bridge.rs so call sites land against the final type, not the buggy one.
Co-authored-by: Tyler Longwell <tlongwell@block.xyz>
Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
This commit is contained in:
co-authored by
Tyler Longwell
parent
e31c098ce5
commit
c8cd333e33
@@ -28,7 +28,7 @@ pub mod query;
|
||||
|
||||
pub use buzz_core::CommunityId;
|
||||
pub use error::SearchError;
|
||||
pub use query::{search, SearchHit, SearchQuery, SearchResult};
|
||||
pub use query::{search, ChannelScope, SearchHit, SearchQuery, SearchResult};
|
||||
|
||||
use sqlx::PgPool;
|
||||
|
||||
|
||||
@@ -14,6 +14,44 @@ use uuid::Uuid;
|
||||
|
||||
use crate::error::SearchError;
|
||||
|
||||
/// Channel-scope filter for a community-scoped FTS query.
|
||||
///
|
||||
/// Four variants, 1-to-1 with the legacy `(accessible_channels: &[Uuid],
|
||||
/// include_global: bool)` matrix from the Typesense relay:
|
||||
///
|
||||
/// | accessible | include_global | `ChannelScope` |
|
||||
/// |---|---|---|
|
||||
/// | non-empty | true | `ChannelsOrChannelLess(accessible)` |
|
||||
/// | non-empty | false | `Channels(accessible)` |
|
||||
/// | empty | true | `ChannelLessOnly` |
|
||||
/// | empty | false | (don't call — caller short-circuits to EOSE) |
|
||||
///
|
||||
/// `ChannelLessOnly` is the variant that the old `Option<Vec<Uuid>>` +
|
||||
/// `bool` 2x2 could not express unambiguously: with empty accessible
|
||||
/// channels and `include_global=true`, both `Some(vec![]) + true` and
|
||||
/// `None + true` would broaden to all community channels rather than
|
||||
/// restrict to channel-less events. The enum closes that hole at the
|
||||
/// type level.
|
||||
///
|
||||
/// Empty-vec edge cases are intentionally not special-cased:
|
||||
/// `Channels(vec![])` emits `channel_id = ANY('{}')` which Postgres
|
||||
/// evaluates as false-for-all-rows (zero hits), and
|
||||
/// `ChannelsOrChannelLess(vec![])` emits `(channel_id = ANY('{}') OR
|
||||
/// channel_id IS NULL)` which is equivalent to `ChannelLessOnly`.
|
||||
#[derive(Debug, Clone)]
|
||||
pub enum ChannelScope {
|
||||
/// No channel constraint. Matches every event in the community.
|
||||
Any,
|
||||
/// Restrict to `channel_id IS NULL` events only — what the legacy
|
||||
/// Typesense `channel_id:=__global__` sentinel meant.
|
||||
ChannelLessOnly,
|
||||
/// Restrict to events whose `channel_id` is in this list.
|
||||
Channels(Vec<Uuid>),
|
||||
/// Restrict to events whose `channel_id` is in this list, OR are
|
||||
/// channel-less (`channel_id IS NULL`).
|
||||
ChannelsOrChannelLess(Vec<Uuid>),
|
||||
}
|
||||
|
||||
/// A community-scoped FTS query.
|
||||
///
|
||||
/// The community is REQUIRED at the type level — there is no construction path
|
||||
@@ -26,15 +64,11 @@ pub struct SearchQuery {
|
||||
/// NIP-50 search text. Empty string is rejected by `search()` early
|
||||
/// (no hits, no SQL roundtrip).
|
||||
pub q: String,
|
||||
/// Restrict hits to one of these channel UUIDs. `None` = no channel
|
||||
/// constraint (community-global within the community). An empty `Some(vec![])`
|
||||
/// is also treated as "no channel constraint" — call sites that mean
|
||||
/// "no channels are accessible" must short-circuit before calling.
|
||||
pub channel_ids: Option<Vec<Uuid>>,
|
||||
/// If `true`, include channel-less events (channel_id IS NULL) in addition
|
||||
/// to any `channel_ids` filter. If `channel_ids` is `None`, this is
|
||||
/// implicitly satisfied. Maps to today's `__global__` sentinel semantic.
|
||||
pub include_channel_less: bool,
|
||||
/// How to scope hits by channel. See [`ChannelScope`] — the four variants
|
||||
/// are 1-to-1 with the legacy `(accessible_channels, include_global)`
|
||||
/// matrix, and `ChannelLessOnly` closes the gap where "empty accessible
|
||||
/// channels + include global" used to silently broaden to all channels.
|
||||
pub channel_scope: ChannelScope,
|
||||
/// NIP-01 kinds filter. None = no kind constraint.
|
||||
pub kinds: Option<Vec<i32>>,
|
||||
/// NIP-01 authors filter (32-byte pubkeys). None = no author constraint.
|
||||
@@ -127,35 +161,26 @@ pub async fn search(pool: &PgPool, query: &SearchQuery) -> Result<SearchResult,
|
||||
qb.push_bind(*query.community.as_uuid());
|
||||
qb.push(" AND deleted_at IS NULL AND search_tsv @@ query");
|
||||
|
||||
// Channel scope. Three shapes:
|
||||
// - channel_ids = Some([..]) + include_channel_less = true: (channel_id = ANY($) OR channel_id IS NULL)
|
||||
// - channel_ids = Some([..]) + include_channel_less = false: channel_id = ANY($)
|
||||
// - channel_ids = None + include_channel_less = true: (no constraint — also covers None/false for callers
|
||||
// that explicitly want "no channel scope at all")
|
||||
// - channel_ids = None + include_channel_less = false: caller meant "nothing accessible" but didn't
|
||||
// short-circuit; we conservatively return no hits
|
||||
match (&query.channel_ids, query.include_channel_less) {
|
||||
(Some(ids), include_global) if !ids.is_empty() => {
|
||||
// Channel scope — see `ChannelScope` doc for the four-case mapping. The
|
||||
// emitted SQL fragments are identical to the legacy 2x2 tuple for the
|
||||
// three carry-over cases; `ChannelLessOnly` is the new fence that the
|
||||
// old shape could not express.
|
||||
match &query.channel_scope {
|
||||
ChannelScope::Any => {
|
||||
// No channel constraint.
|
||||
}
|
||||
ChannelScope::ChannelLessOnly => {
|
||||
qb.push(" AND channel_id IS NULL");
|
||||
}
|
||||
ChannelScope::Channels(ids) => {
|
||||
qb.push(" AND channel_id = ANY(");
|
||||
qb.push_bind(ids.clone());
|
||||
qb.push(")");
|
||||
}
|
||||
ChannelScope::ChannelsOrChannelLess(ids) => {
|
||||
qb.push(" AND (channel_id = ANY(");
|
||||
qb.push_bind(ids.clone());
|
||||
if include_global {
|
||||
qb.push(") OR channel_id IS NULL)");
|
||||
} else {
|
||||
qb.push("))");
|
||||
}
|
||||
}
|
||||
(Some(_), true) | (None, true) => {
|
||||
// No channel constraint — include everything in the community.
|
||||
// (channel_ids = Some(empty) falls here because no IDs to filter
|
||||
// by and channel-less events are included.)
|
||||
}
|
||||
(Some(_), false) | (None, false) => {
|
||||
// Caller said "no accessible channels and exclude channel-less" —
|
||||
// produces an empty result.
|
||||
return Ok(SearchResult {
|
||||
hits: Vec::new(),
|
||||
page,
|
||||
});
|
||||
qb.push(") OR channel_id IS NULL)");
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
//! parallel-safe.
|
||||
|
||||
use buzz_core::CommunityId;
|
||||
use buzz_search::{SearchQuery, SearchService};
|
||||
use buzz_search::{ChannelScope, SearchQuery, SearchService};
|
||||
use sqlx::{postgres::PgPoolOptions, Executor, PgPool};
|
||||
use uuid::Uuid;
|
||||
|
||||
@@ -136,8 +136,7 @@ async fn search_finds_event_in_same_community() {
|
||||
.search(&SearchQuery {
|
||||
community: c_a,
|
||||
q: "wonderland".into(),
|
||||
channel_ids: None,
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -184,8 +183,7 @@ async fn search_does_not_return_other_community_events() {
|
||||
.search(&SearchQuery {
|
||||
community: c_a,
|
||||
q: "unique-token-xyz".into(),
|
||||
channel_ids: None,
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -201,8 +199,7 @@ async fn search_does_not_return_other_community_events() {
|
||||
.search(&SearchQuery {
|
||||
community: c_b,
|
||||
q: "unique-token-xyz".into(),
|
||||
channel_ids: None,
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -245,8 +242,7 @@ async fn kind0_search_by_display_name_works_without_flattening() {
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: q.to_string(),
|
||||
channel_ids: None,
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: Some(vec![0]),
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -324,8 +320,7 @@ async fn channel_scope_restricts_results() {
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "shared-token".into(),
|
||||
channel_ids: Some(vec![ch_a]),
|
||||
include_channel_less: false,
|
||||
channel_scope: ChannelScope::Channels(vec![ch_a]),
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -343,8 +338,7 @@ async fn channel_scope_restricts_results() {
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "shared-token".into(),
|
||||
channel_ids: Some(vec![ch_a]),
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::ChannelsOrChannelLess(vec![ch_a]),
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -361,8 +355,7 @@ async fn channel_scope_restricts_results() {
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "shared-token".into(),
|
||||
channel_ids: None,
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -379,8 +372,7 @@ async fn channel_scope_restricts_results() {
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "shared-token".into(),
|
||||
channel_ids: Some(vec![]),
|
||||
include_channel_less: false,
|
||||
channel_scope: ChannelScope::Channels(vec![]),
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -418,8 +410,7 @@ async fn deleted_events_are_excluded() {
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "deleted-token-q".into(),
|
||||
channel_ids: None,
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -449,8 +440,7 @@ async fn empty_query_returns_empty_result_no_roundtrip() {
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: q.into(),
|
||||
channel_ids: None,
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -516,8 +506,7 @@ async fn since_until_filters() {
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "time-token-zz".into(),
|
||||
channel_ids: None,
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: Some(1_700_005_000),
|
||||
@@ -560,8 +549,7 @@ async fn pagination_works() {
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "page-token-qq".into(),
|
||||
channel_ids: None,
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -577,8 +565,7 @@ async fn pagination_works() {
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "page-token-qq".into(),
|
||||
channel_ids: None,
|
||||
include_channel_less: true,
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
@@ -596,3 +583,95 @@ async fn pagination_works() {
|
||||
|
||||
teardown(pool, &schema).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[ignore = "requires Postgres"]
|
||||
async fn channel_less_only_excludes_per_channel_events() {
|
||||
// Closes the row-3 fence hole: in the legacy 2x2 shape, both
|
||||
// `Some(vec![]) + true` and `None + true` silently broadened to all
|
||||
// community channels rather than restricting to channel-less events.
|
||||
// `ChannelScope::ChannelLessOnly` is the variant that the old type
|
||||
// could not express.
|
||||
//
|
||||
// Adversarial check: mutate this test's expectation to `>= 2` and the
|
||||
// assertion goes red against the new SQL `AND channel_id IS NULL`,
|
||||
// proving the predicate bites. Mutate `query.rs` `ChannelLessOnly` arm
|
||||
// to a no-op (the `Any` semantic the old code emitted) and this test
|
||||
// also goes red — three hits instead of one — proving the fix is the
|
||||
// emitted predicate, not the variant name.
|
||||
let (pool, schema) = setup().await;
|
||||
|
||||
let c = mk_community(&pool, "a.example").await;
|
||||
let ch_a = Uuid::new_v4();
|
||||
let ch_b = Uuid::new_v4();
|
||||
sqlx::query("INSERT INTO channels (community_id, id, name, channel_type, created_by) VALUES ($1, $2, $3, 'stream'::channel_type, $4), ($1, $5, $6, 'stream'::channel_type, $4)")
|
||||
.bind(c.as_uuid())
|
||||
.bind(ch_a)
|
||||
.bind("ch-a")
|
||||
.bind(b"\x01".repeat(32))
|
||||
.bind(ch_b)
|
||||
.bind("ch-b")
|
||||
.execute(&pool)
|
||||
.await
|
||||
.expect("insert channels");
|
||||
|
||||
let pk = rand_bytes32();
|
||||
insert_event(
|
||||
&pool,
|
||||
c,
|
||||
rand_bytes32(),
|
||||
pk,
|
||||
1,
|
||||
"fence-token in ch-a",
|
||||
Some(ch_a),
|
||||
1_700_000_000,
|
||||
)
|
||||
.await;
|
||||
insert_event(
|
||||
&pool,
|
||||
c,
|
||||
rand_bytes32(),
|
||||
pk,
|
||||
1,
|
||||
"fence-token in ch-b",
|
||||
Some(ch_b),
|
||||
1_700_000_001,
|
||||
)
|
||||
.await;
|
||||
insert_event(
|
||||
&pool,
|
||||
c,
|
||||
rand_bytes32(),
|
||||
pk,
|
||||
1,
|
||||
"fence-token channel-less",
|
||||
None,
|
||||
1_700_000_002,
|
||||
)
|
||||
.await;
|
||||
|
||||
let svc = SearchService::new(pool.clone());
|
||||
|
||||
let r = svc
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "fence-token".into(),
|
||||
channel_scope: ChannelScope::ChannelLessOnly,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
until: None,
|
||||
page: 1,
|
||||
per_page: 10,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
r.hits.len(),
|
||||
1,
|
||||
"ChannelLessOnly must return only the channel_id IS NULL row"
|
||||
);
|
||||
assert_eq!(r.hits[0].channel_id, None);
|
||||
|
||||
teardown(pool, &schema).await;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user