From 0ed25cdb21bd8c9532c0dc322f17527b7107f087 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 14 Aug 2026 04:11:30 +0700 Subject: [PATCH 1/2] fix(query): stop insert_item from silently dropping aggregate query wrappers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Query::insert_item (and AggregateSumQuery::insert_item) treated any range-collision as mergeable and rewrote the colliding pair via QueryItem::merge, which can only produce plain key/range variants. When one side was an aggregate meta-variant (AggregateCountOnRange, AggregateSumOnRange, AggregateCountAndSumOnRange), the merge silently erased the aggregate wrapper: a query built as AggregateCountAndSumOnRange(Range(a..z)) followed by insert_key("extra") became a plain [Range(a..z)] — a completely different, non-aggregate query with no error. Aggregate wrappers are now never range-merged: an exact structural duplicate is deduplicated, and anything else that overlaps an aggregate item is kept as a separate item. The resulting multi-item shape is rejected by the existing validate_aggregate_* entry points (both prove and get paths route through has_*_anywhere detection), so the semantic conflict surfaces as an explicit validation error instead of a silent wrapper drop. QueryItem::merge documents the no-aggregates precondition and enforces it with a debug_assert. No version gating: this only changes in-memory query construction, not proof wire format or state transitions, and any caller that previously hit the merge was already getting silently wrong (non-aggregate) results. Co-Authored-By: Claude Fable 5 --- .../src/aggregate_sum_query/insert.rs | 36 +++-- grovedb-query/src/insert.rs | 143 ++++++++++++++++-- grovedb-query/src/query_item/merge.rs | 15 ++ grovedb-query/src/query_item/mod.rs | 17 +++ 4 files changed, 193 insertions(+), 18 deletions(-) diff --git a/grovedb-query/src/aggregate_sum_query/insert.rs b/grovedb-query/src/aggregate_sum_query/insert.rs index 1928960f7..29006d5da 100644 --- a/grovedb-query/src/aggregate_sum_query/insert.rs +++ b/grovedb-query/src/aggregate_sum_query/insert.rs @@ -126,8 +126,9 @@ impl AggregateSumQuery { /// Adds a range of all potential values to the query, so that the query /// will return all values /// - /// All other items in the query will be discarded as you are now getting - /// back all elements. + /// All other plain key/range items in the query will be discarded as you + /// are now getting back all elements. Aggregate meta-items + /// (`AggregateCountOnRange` etc.) are kept — see [`Self::insert_item`]. pub fn insert_all(&mut self) { let range = QueryItem::RangeFull(RangeFull); self.insert_item(range); @@ -138,17 +139,32 @@ impl AggregateSumQuery { /// then merged together so that the query includes the minimum number of /// items (with no items covering any duplicate parts of keyspace) while /// still including every key or range that has been added to the query. + /// + /// Aggregate meta-variants (`AggregateCountOnRange`, + /// `AggregateSumOnRange`, `AggregateCountAndSumOnRange`) are **never** + /// range-merged: merging would erase the aggregate wrapper and silently + /// turn the item into a plain range. An exact structural duplicate is + /// deduplicated; anything else that overlaps an aggregate item is kept + /// as a separate item. (Aggregate meta-variants are not valid + /// `AggregateSumQuery` items in the first place, so this only preserves + /// the invalid shape for downstream rejection instead of laundering it + /// into a valid-looking plain range.) pub fn insert_item(&mut self, mut item: QueryItem) { - // since `QueryItem::eq` considers items equal if they collide at all - // (including keys within ranges or ranges which partially overlap), - // `items.take` will remove the first item which collides + let item_is_aggregate = item.is_aggregate(); self.items = self .items .iter() .filter_map(|our_item| { - if our_item.is_key() && item.is_key() && our_item == &item { + if our_item == &item { + // Exact structural duplicate (key, range, or aggregate): + // drop the existing copy, `item` replaces it below. None + } else if item_is_aggregate || our_item.is_aggregate() { + // Aggregate wrappers are not mergeable with anything — + // keep both items instead of silently dropping the + // wrapper. + Some(our_item.clone()) } else if our_item.collides_with(&item) { item.merge_assign(our_item); None @@ -159,9 +175,11 @@ impl AggregateSumQuery { .collect(); // Insert item at the correct sorted position. - // Ord-equal items are always removed by the collision filter above, - // so binary_search always returns Err. We use unwrap_or_else to - // extract the insertion index from either variant without panicking. + // `QueryItem::cmp` compares by covered range, so an aggregate item + // and a plain item covering the same range are Ord-equal while being + // kept as distinct items above; binary_search may therefore return + // either Ok or Err. We use unwrap_or_else to extract the insertion + // index from either variant without panicking. let pos = self.items.binary_search(&item).unwrap_or_else(|e| e); self.items.insert(pos, item); } diff --git a/grovedb-query/src/insert.rs b/grovedb-query/src/insert.rs index 29b09dff2..8feeb4971 100644 --- a/grovedb-query/src/insert.rs +++ b/grovedb-query/src/insert.rs @@ -125,8 +125,9 @@ impl Query { /// Adds a range of all potential values to the query, so that the query /// will return all values /// - /// All other items in the query will be discarded as you are now getting - /// back all elements. + /// All other plain key/range items in the query will be discarded as you + /// are now getting back all elements. Aggregate meta-items + /// (`AggregateCountOnRange` etc.) are kept — see [`Self::insert_item`]. pub fn insert_all(&mut self) { let range = QueryItem::RangeFull(RangeFull); self.insert_item(range); @@ -137,17 +138,30 @@ impl Query { /// then merged together so that the query includes the minimum number of /// items (with no items covering any duplicate parts of keyspace) while /// still including every key or range that has been added to the query. + /// + /// Aggregate meta-variants (`AggregateCountOnRange`, + /// `AggregateSumOnRange`, `AggregateCountAndSumOnRange`) are **never** + /// range-merged: merging would erase the aggregate wrapper and silently + /// turn the query into a plain range query. An exact structural + /// duplicate is deduplicated; anything else that overlaps an aggregate + /// item is kept as a separate item, producing a multi-item query that + /// the `validate_aggregate_*` entry points reject at prove/execute time. pub fn insert_item(&mut self, mut item: QueryItem) { - // since `QueryItem::eq` considers items equal if they collide at all - // (including keys within ranges or ranges which partially overlap), - // `items.take` will remove the first item which collides + let item_is_aggregate = item.is_aggregate(); self.items = self .items .iter() .filter_map(|our_item| { - if our_item.is_key() && item.is_key() && our_item == &item { + if our_item == &item { + // Exact structural duplicate (key, range, or aggregate): + // drop the existing copy, `item` replaces it below. None + } else if item_is_aggregate || our_item.is_aggregate() { + // Aggregate wrappers are not mergeable with anything — + // keep both items so the semantic conflict surfaces as a + // validation error instead of a silent wrapper drop. + Some(our_item.clone()) } else if our_item.collides_with(&item) { item.merge_assign(our_item); None @@ -158,9 +172,11 @@ impl Query { .collect(); // Insert item at the correct sorted position. - // Ord-equal items are always removed by the collision filter above, - // so binary_search always returns Err. We use unwrap_or_else to - // extract the insertion index from either variant without panicking. + // `QueryItem::cmp` compares by covered range, so an aggregate item + // and a plain item covering the same range are Ord-equal while being + // kept as distinct items above; binary_search may therefore return + // either Ok or Err. We use unwrap_or_else to extract the insertion + // index from either variant without panicking. let pos = self.items.binary_search(&item).unwrap_or_else(|e| e); self.items.insert(pos, item); } @@ -196,5 +212,114 @@ mod tests { assert_matches!(query.items.as_slice(), [QueryItem::Key(v)] if v == &value); } + + #[test] + fn test_insert_key_into_aggregate_query_preserves_aggregate_wrapper() { + // Regression test: inserting a key that falls inside the inner + // range of an aggregate meta-item used to merge the two items, + // silently dropping the aggregate wrapper and turning the query + // into a plain range query. + let mut query = Query::new_aggregate_count_and_sum_on_range(QueryItem::Range( + b"a".to_vec()..b"z".to_vec(), + )); + + query.insert_key(b"extra".to_vec()); + + assert_matches!( + query.items.as_slice(), + [ + QueryItem::AggregateCountAndSumOnRange(inner), + QueryItem::Key(k), + ] if matches!(inner.as_ref(), QueryItem::Range(r) if r.start == b"a" && r.end == b"z") + && k == b"extra" + ); + // The resulting malformed shape must be rejected downstream. + assert!(query.validate_aggregate_count_and_sum_on_range().is_err()); + } + + #[test] + fn test_insert_range_into_aggregate_count_query_preserves_aggregate_wrapper() { + let mut query = + Query::new_aggregate_count_on_range(QueryItem::Range(b"a".to_vec()..b"m".to_vec())); + + // Overlapping plain range must not be merged into the aggregate. + query.insert_range(b"f".to_vec()..b"z".to_vec()); + + assert_matches!( + query.items.as_slice(), + [ + QueryItem::AggregateCountOnRange(_), + QueryItem::Range(r), + ] if r.start == b"f" && r.end == b"z" + ); + assert!(query.validate_aggregate_count_on_range().is_err()); + } + + #[test] + fn test_insert_all_into_aggregate_sum_query_preserves_aggregate_wrapper() { + let mut query = Query::new_aggregate_sum_on_range(QueryItem::RangeInclusive( + b"a".to_vec()..=b"z".to_vec(), + )); + + // RangeFull collides with everything; the aggregate must survive. + query.insert_all(); + + assert_matches!( + query.items.as_slice(), + [QueryItem::RangeFull(_), QueryItem::AggregateSumOnRange(_)] + | [QueryItem::AggregateSumOnRange(_), QueryItem::RangeFull(_)] + ); + assert!(query.validate_aggregate_sum_on_range().is_err()); + } + + #[test] + fn test_insert_aggregate_item_into_plain_query_keeps_both_items() { + let mut query = Query::new(); + query.insert_range(b"a".to_vec()..b"z".to_vec()); + + query.insert_item(QueryItem::AggregateSumOnRange(Box::new(QueryItem::Range( + b"b".to_vec()..b"c".to_vec(), + )))); + + assert_matches!( + query.items.as_slice(), + [QueryItem::Range(_), QueryItem::AggregateSumOnRange(_)] + | [QueryItem::AggregateSumOnRange(_), QueryItem::Range(_)] + ); + assert!(query.validate_aggregate_sum_on_range().is_err()); + } + + #[test] + fn test_insert_identical_aggregate_item_twice_dedupes() { + let aggregate = QueryItem::AggregateCountOnRange(Box::new(QueryItem::Range( + b"a".to_vec()..b"z".to_vec(), + ))); + + let mut query = Query::new(); + query.insert_item(aggregate.clone()); + query.insert_item(aggregate.clone()); + + assert_eq!(query.items.as_slice(), [aggregate]); + } + + #[test] + fn test_insert_colliding_non_identical_aggregates_keeps_both() { + // Two overlapping aggregates of the same kind stay separate; the + // malformed multi-item shape is rejected by the validator instead + // of being silently merged. + let mut query = + Query::new_aggregate_count_on_range(QueryItem::Range(b"a".to_vec()..b"m".to_vec())); + + query.insert_item(QueryItem::AggregateCountOnRange(Box::new( + QueryItem::Range(b"f".to_vec()..b"z".to_vec()), + ))); + + assert_eq!(query.items.len(), 2); + assert!(query + .items + .iter() + .all(|item| item.is_aggregate_count_on_range())); + assert!(query.validate_aggregate_count_on_range().is_err()); + } } } diff --git a/grovedb-query/src/query_item/merge.rs b/grovedb-query/src/query_item/merge.rs index 4b42b6b19..48f3845bc 100644 --- a/grovedb-query/src/query_item/merge.rs +++ b/grovedb-query/src/query_item/merge.rs @@ -7,7 +7,22 @@ use crate::query_item::QueryItem; impl QueryItem { /// Merge two overlapping query items into one that covers both ranges. + /// + /// Neither `self` nor `other` may be an aggregate meta-variant + /// (`AggregateCountOnRange`, `AggregateSumOnRange`, + /// `AggregateCountAndSumOnRange`): the result is always a plain + /// key/range variant, so merging an aggregate would silently erase the + /// aggregate wrapper and change the meaning of the query. Callers + /// (`Query::insert_item` / `AggregateSumQuery::insert_item`) must keep + /// aggregate items out of merging; this precondition is checked with a + /// `debug_assert!`. pub fn merge(&self, other: &Self) -> Self { + debug_assert!( + !self.is_aggregate() && !other.is_aggregate(), + "QueryItem::merge must not be called with aggregate meta-variants; merging would \ + drop the aggregate wrapper" + ); + if self.is_key() && other.is_key() && self == other { return self.clone(); } diff --git a/grovedb-query/src/query_item/mod.rs b/grovedb-query/src/query_item/mod.rs index 26539679f..669be0a18 100644 --- a/grovedb-query/src/query_item/mod.rs +++ b/grovedb-query/src/query_item/mod.rs @@ -1003,6 +1003,23 @@ impl QueryItem { } } + /// Returns `true` if this query item is any aggregate meta-variant + /// (`AggregateCountOnRange`, `AggregateSumOnRange`, or + /// `AggregateCountAndSumOnRange`). + /// + /// Aggregate meta-variants change the *interpretation* of the wrapped + /// range (return a count/sum instead of the elements), so they must + /// never be range-merged with other query items — merging would erase + /// the wrapper and silently turn the query into a plain range query. + pub const fn is_aggregate(&self) -> bool { + matches!( + self, + QueryItem::AggregateCountOnRange(_) + | QueryItem::AggregateSumOnRange(_) + | QueryItem::AggregateCountAndSumOnRange(_) + ) + } + /// Returns `true` if this query item is the count-only meta-variant. pub const fn is_aggregate_count_on_range(&self) -> bool { matches!(self, QueryItem::AggregateCountOnRange(_)) From 411263271ced41982b4f47f02656d8f3bf2730b9 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 14 Aug 2026 04:16:24 +0700 Subject: [PATCH 2/2] fix(query): enforce QueryItem::merge aggregate precondition in release builds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review: merge is public API, so the debug_assert compiled out in release builds and an external direct caller could still silently drop the aggregate wrapper. Upgrade to a hard assert! — precondition violation is a programmer error, and the insert_item guards keep the in-repo (panic-free) paths from ever reaching it. Co-Authored-By: Claude Fable 5 --- grovedb-query/src/query_item/merge.rs | 28 ++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/grovedb-query/src/query_item/merge.rs b/grovedb-query/src/query_item/merge.rs index 48f3845bc..f5660f650 100644 --- a/grovedb-query/src/query_item/merge.rs +++ b/grovedb-query/src/query_item/merge.rs @@ -14,10 +14,16 @@ impl QueryItem { /// key/range variant, so merging an aggregate would silently erase the /// aggregate wrapper and change the meaning of the query. Callers /// (`Query::insert_item` / `AggregateSumQuery::insert_item`) must keep - /// aggregate items out of merging; this precondition is checked with a - /// `debug_assert!`. + /// aggregate items out of merging. + /// + /// # Panics + /// + /// Panics if `self` or `other` is an aggregate meta-variant. This is an + /// invariant-enforcement panic (violating the precondition would + /// otherwise silently change query semantics); it is enforced in release + /// builds too because `merge` is public API. pub fn merge(&self, other: &Self) -> Self { - debug_assert!( + assert!( !self.is_aggregate() && !other.is_aggregate(), "QueryItem::merge must not be called with aggregate meta-variants; merging would \ drop the aggregate wrapper" @@ -87,6 +93,11 @@ impl QueryItem { } /// Merges another QueryItem into this one in-place. + /// + /// # Panics + /// + /// Panics if `self` or `other` is an aggregate meta-variant — see + /// [`Self::merge`]. pub fn merge_assign(&mut self, other: &Self) { *self = self.merge(other); } @@ -112,4 +123,15 @@ mod tests { assert_matches!(merged, QueryItem::Key(v) if v == value); } + + #[test] + #[should_panic(expected = "must not be called with aggregate meta-variants")] + fn test_merge_rejects_aggregate_meta_variants() { + let aggregate = QueryItem::AggregateCountOnRange(Box::new(QueryItem::Range( + b"a".to_vec()..b"z".to_vec(), + ))); + let key = QueryItem::Key(b"extra".to_vec()); + + let _ = aggregate.merge(&key); + } }