From a84f1eab930c85e5de3760394d9eadcf25229452 Mon Sep 17 00:00:00 2001 From: mxngls Date: Wed, 11 Feb 2026 09:52:20 +0900 Subject: [PATCH] Fix get_many_to_many_counts pagination with fetch N+1 Same issue as the get_many_to_many cursor fix in the previous commit, but simpler: the counts endpoint groups by subject so each subject appears exactly once, meaning a subject-only cursor is sufficient. The only problem was that when items.len() == limit, the code couldn't distinguish "exactly limit items exist" from "more items exist but we stopped at limit", causing a false cursor on the final page. Fix: accumulate limit+1 items, only emit a cursor when more than limit exist, then truncate. Applied to both MemStorage (take N+1 in the iterator chain) and RocksDB (allow one extra group in the BTreeMap, pop it before building results). --- constellation/src/storage/mem_store.rs | 5 +- constellation/src/storage/mod.rs | 66 +++++++++++++++++++++++- constellation/src/storage/rocks_store.rs | 26 +++++----- 3 files changed, 82 insertions(+), 15 deletions(-) diff --git a/constellation/src/storage/mem_store.rs b/constellation/src/storage/mem_store.rs index 52658ad..b61c905 100644 --- a/constellation/src/storage/mem_store.rs +++ b/constellation/src/storage/mem_store.rs @@ -198,9 +198,10 @@ impl LinkReader for MemStorage { items = items .into_iter() .skip_while(|(t, _, _)| after.as_ref().map(|a| t <= a).unwrap_or(false)) - .take(limit as usize) + .take(limit as usize + 1) .collect(); - let next = if items.len() as u64 >= limit { + let next = if items.len() as u64 > limit { + items.truncate(limit as usize); items.last().map(|(t, _, _)| t.clone()) } else { None diff --git a/constellation/src/storage/mod.rs b/constellation/src/storage/mod.rs index c72a5b0..f5d8f49 100644 --- a/constellation/src/storage/mod.rs +++ b/constellation/src/storage/mod.rs @@ -1716,6 +1716,70 @@ mod tests { next: None, } ); + + // Pagination edge cases: we have 2 grouped results (b.com and c.com) + + // Case 1: limit > items (limit=10, items=2) -> next should be None + let result = storage.get_many_to_many_counts( + "a.com", + "app.t.c", + ".abc.uri", + ".def.uri", + 10, + None, + &HashSet::new(), + &HashSet::new(), + )?; + assert_eq!(result.items.len(), 2); + assert_eq!(result.next, None, "next should be None when items < limit"); + + // Case 2: limit == items (limit=2, items=2) -> next should be None + let result = storage.get_many_to_many_counts( + "a.com", + "app.t.c", + ".abc.uri", + ".def.uri", + 2, + None, + &HashSet::new(), + &HashSet::new(), + )?; + assert_eq!(result.items.len(), 2); + assert_eq!( + result.next, None, + "next should be None when items == limit (no more pages)" + ); + + // Case 3: limit < items (limit=1, items=2) -> next should be Some + let result = storage.get_many_to_many_counts( + "a.com", + "app.t.c", + ".abc.uri", + ".def.uri", + 1, + None, + &HashSet::new(), + &HashSet::new(), + )?; + assert_eq!(result.items.len(), 1); + assert!( + result.next.is_some(), + "next should be Some when items > limit" + ); + + // Verify second page returns remaining item with no cursor + let result2 = storage.get_many_to_many_counts( + "a.com", + "app.t.c", + ".abc.uri", + ".def.uri", + 1, + result.next, + &HashSet::new(), + &HashSet::new(), + )?; + assert_eq!(result2.items.len(), 1); + assert_eq!(result2.next, None, "next should be None on final page"); }); test_each_storage!(get_m2m_empty, |storage| { @@ -1787,7 +1851,7 @@ mod tests { ); }); - test_each_storage!(get_m2m_no_filters, |storage| { + test_each_storage!(get_m2m_filters, |storage| { storage.push( &ActionableEvent::CreateLinks { record_id: RecordId { diff --git a/constellation/src/storage/rocks_store.rs b/constellation/src/storage/rocks_store.rs index 3e0ab44..da02d33 100644 --- a/constellation/src/storage/rocks_store.rs +++ b/constellation/src/storage/rocks_store.rs @@ -1033,9 +1033,9 @@ impl LinkReader for RocksStorage { // aand we can skip target ids that must be on future pages // (this check continues after the did-lookup, which we have to do) - let page_is_full = grouped_counts.len() as u64 >= limit; + let page_is_full = grouped_counts.len() as u64 > limit; if page_is_full { - let current_max = grouped_counts.keys().next_back().unwrap(); // limit should be non-zero bleh + let current_max = grouped_counts.keys().next_back().unwrap(); if fwd_target > *current_max { continue; } @@ -1071,6 +1071,18 @@ impl LinkReader for RocksStorage { } } + // If we accumulated more than limit groups, there's another page. + // Pop the extra before building items so it doesn't appear in results. + let next = if grouped_counts.len() as u64 > limit { + grouped_counts.pop_last(); + grouped_counts + .keys() + .next_back() + .map(|k| format!("{}", k.0)) + } else { + None + }; + let mut items: Vec<(String, u64, u64)> = Vec::with_capacity(grouped_counts.len()); for (target_id, (n, dids)) in &grouped_counts { let Some(target) = self @@ -1083,16 +1095,6 @@ impl LinkReader for RocksStorage { items.push((target.0 .0, *n, dids.len() as u64)); } - let next = if grouped_counts.len() as u64 >= limit { - // yeah.... it's a number saved as a string......sorry - grouped_counts - .keys() - .next_back() - .map(|k| format!("{}", k.0)) - } else { - None - }; - Ok(PagedOrderedCollection { items, next }) } -- 2.51.2