Skip to content

Commit b1027dc

Browse files
nit(coderabbit): doc-comment + test + error-wrap polish
Four low-value but clean tweaks from CodeRabbit on PR #661: - `grovedb-query/src/query_item/mod.rs`: refresh the stale `NonAggregateInner::deserialize` inline comment to mention both excluded aggregate variants (Count + Sum), matching the struct-level doc and `NON_AGGREGATE_VARIANTS`. - `grovedb/src/tests/aggregate_sum_query_tests.rs`: drop the redundant disjunction `msg.contains("must be a ProvableSumTree") || msg.contains("ProvableSumTree")` — the first clause already implies the second; pin the exact phrase. - `grovedb/src/tests/aggregate_sum_query_tests.rs`: harden `provable_sum_tree_overflow_at_i64_max_is_rejected` so it no longer silently passes when insert AND prove AND verify all accept an overflow. Replace the early-return-on-both-inserts-accepted with an explicit "at least one stage must reject" assertion. - `grovedb/src/operations/get/query.rs`: wrap the MerkError from `Merk::sum_aggregate_on_range` (and the count sibling) with contextual `CorruptedData(format!("query_aggregate_{sum,count} at path {:?}: {}", path_slices, e))` per the repo error-wrapping convention. Two test assertions updated from `MerkError(_)` to `CorruptedData(_)` to match. Workspace `cargo test --all-features`: 3102 pass / 0 fail (unchanged). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 1ec516a commit b1027dc

4 files changed

Lines changed: 52 additions & 28 deletions

File tree

grovedb-query/src/query_item/mod.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -317,9 +317,9 @@ impl<'de> Deserialize<'de> for NonAggregateInner {
317317
where
318318
D: Deserializer<'de>,
319319
{
320-
// Field set excludes "AggregateCountOnRange"; encountering that tag
321-
// produces a serde "unknown variant" error before any inner
322-
// recursion can happen.
320+
// Field set excludes both `AggregateCountOnRange` and
321+
// `AggregateSumOnRange`; encountering either tag produces a serde
322+
// "unknown variant" error before any inner recursion can happen.
323323
#[derive(Deserialize)]
324324
#[serde(field_identifier, rename_all = "snake_case")]
325325
enum Field {

grovedb/src/operations/get/query.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -663,7 +663,10 @@ where {
663663
&mut cost,
664664
subtree
665665
.sum_aggregate_on_range(&inner_range, grove_version)
666-
.map_err(Error::MerkError)
666+
.map_err(|e| Error::CorruptedData(format!(
667+
"query_aggregate_sum at path {:?}: {}",
668+
path_slices, e
669+
)))
667670
);
668671

669672
Ok(sum).wrap_with_cost(cost)
@@ -818,7 +821,10 @@ where {
818821
&mut cost,
819822
subtree
820823
.count_aggregate_on_range(&inner_range, grove_version)
821-
.map_err(Error::MerkError)
824+
.map_err(|e| Error::CorruptedData(format!(
825+
"query_aggregate_count at path {:?}: {}",
826+
path_slices, e
827+
)))
822828
);
823829

824830
Ok(count).wrap_with_cost(cost)

grovedb/src/tests/aggregate_count_query_tests.rs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1584,12 +1584,14 @@ mod tests {
15841584
.query_aggregate_count(&path_query, None, v)
15851585
.unwrap()
15861586
.expect_err("NormalTree must be rejected by the merk-level entry");
1587-
// The merk-level error gets wrapped in Error::MerkError; we just
1588-
// require *some* error rather than asserting on the exact variant
1589-
// since the merk layer's InvalidProofError formatting is internal.
1587+
// The merk-level error gets wrapped with contextual `CorruptedData`
1588+
// (callsite-specific path info — see `query_aggregate_count` in
1589+
// `operations/get/query.rs`). We just require *some* error rather
1590+
// than asserting on the exact variant since the merk layer's
1591+
// `InvalidProofError` formatting is internal.
15901592
match err {
1591-
crate::Error::MerkError(_) => {}
1592-
other => panic!("expected MerkError, got {:?}", other),
1593+
crate::Error::CorruptedData(_) => {}
1594+
other => panic!("expected CorruptedData wrapper, got {:?}", other),
15931595
}
15941596
}
15951597

grovedb/src/tests/aggregate_sum_query_tests.rs

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -362,26 +362,39 @@ mod tests {
362362
)
363363
.unwrap()
364364
.is_ok();
365-
if !ok1 || !ok2 {
366-
// Insertion already rejected the overflow — that's the
367-
// healthiest end state. Bail out.
368-
return;
369-
}
365+
let either_insert_rejected = !ok1 || !ok2;
366+
367+
// If both inserts succeeded, the overflow must be caught later —
368+
// either by the prover or by the verifier. If both inserts AND
369+
// the prover succeed AND the verifier accepts, that's the
370+
// silent-no-op regression we explicitly want to fail on.
370371
let pq = PathQuery::new_aggregate_sum_on_range(
371372
vec![TEST_LEAF.to_vec(), b"st".to_vec()],
372373
QueryItem::RangeInclusive(b"a".to_vec()..=b"b".to_vec()),
373374
);
374-
let prove_result = db.grove_db.prove_query(&pq, None, v).unwrap();
375-
match prove_result {
376-
Err(_) => { /* prover detected overflow — fine */ }
377-
Ok(proof) => {
378-
let verify_result = GroveDb::verify_aggregate_sum_query(&proof, &pq, v);
379-
assert!(
380-
verify_result.is_err(),
381-
"verifier must reject a sum that doesn't fit in i64"
382-
);
375+
let (prover_rejected, verifier_rejected) = if either_insert_rejected {
376+
// The insert side already detected the overflow; no need to
377+
// exercise prove/verify (they'd never reach the i128->i64
378+
// gate without inputs that overflow).
379+
(false, false)
380+
} else {
381+
match db.grove_db.prove_query(&pq, None, v).unwrap() {
382+
Err(_) => (true, false),
383+
Ok(proof) => {
384+
let verify_result = GroveDb::verify_aggregate_sum_query(&proof, &pq, v);
385+
(false, verify_result.is_err())
386+
}
383387
}
384-
}
388+
};
389+
390+
// Exactly the silent-no-op branch must NEVER be reached: at least
391+
// one of {insert, prove, verify} must reject the i64::MAX +
392+
// i64::MAX overflow.
393+
assert!(
394+
either_insert_rejected || prover_rejected || verifier_rejected,
395+
"BUG: i64::MAX + i64::MAX silently produced a wrong sum — insert, \
396+
prove, and verify all accepted the overflow"
397+
);
385398
}
386399

387400
// ---------- 7. i64::MAX + i64::MIN = -1 (intermediate overflows i64 but final fits) ----------
@@ -1036,7 +1049,7 @@ mod tests {
10361049
// names ProvableSumTree explicitly so we pin it.
10371050
let msg = format!("{e}");
10381051
assert!(
1039-
msg.contains("must be a ProvableSumTree") || msg.contains("ProvableSumTree"),
1052+
msg.contains("must be a ProvableSumTree"),
10401053
"verifier rejected as expected but with an unrelated message: {msg}"
10411054
);
10421055
}
@@ -1463,9 +1476,12 @@ mod tests {
14631476
.query_aggregate_sum(&path_query, None, v)
14641477
.unwrap()
14651478
.expect_err("NormalTree leaf must be rejected by merk-level gate");
1479+
// The merk-level error gets wrapped with contextual `CorruptedData`
1480+
// by `query_aggregate_sum` (callsite-specific path info — see
1481+
// `operations/get/query.rs`).
14661482
match err {
1467-
crate::Error::MerkError(_) => {}
1468-
other => panic!("expected MerkError, got {:?}", other),
1483+
crate::Error::CorruptedData(_) => {}
1484+
other => panic!("expected CorruptedData wrapper, got {:?}", other),
14691485
}
14701486
}
14711487

0 commit comments

Comments
 (0)