feat(minibf): adjust max scan limit via config#911
Conversation
📝 WalkthroughWalkthroughThis change introduces a configurable maximum scan items limit to the MinibfConfig struct and updates the pagination enforcement mechanism to use this configurable value. The new field is threaded through route handlers to enforce per-request scan limits instead of using a hardcoded default. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minibf/src/pagination.rs (1)
211-217:⚠️ Potential issue | 🟡 Minor
Some(0)silently disables all scan-protected endpoints.When
max_scan_itemsis explicitly configured as0,limitbecomes0and the first request withpage=1, count=1(yielding1 * 1 = 1 > 0) immediately returnsScanLimitExceeded. Every paginated call to every protected endpoint will be rejected with an unhelpful 400 error, with no indication that the endpoint is effectively disabled.Consider clamping the configured value to a minimum of 1 (or
count_max = 100) at resolution time:🛡️ Proposed fix
pub fn enforce_max_scan_limit(&self, max_scan_items: Option<u64>) -> Result<(), PaginationError> { - let limit = max_scan_items.unwrap_or(DEFAULT_MAX_SCAN_ITEMS); + let limit = max_scan_items + .filter(|&v| v > 0) + .unwrap_or(DEFAULT_MAX_SCAN_ITEMS); if self.page * self.count as u64 > limit {Alternatively, document in
MinibfConfigthatmax_scan_items = 0is treated as "use default" rather than "allow zero items".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/minibf/src/pagination.rs` around lines 211 - 217, The current enforce_max_scan_limit (max_scan_items: Option<u64>) treats Some(0) as a hard zero which disables endpoints; change the resolution of max_scan_items so 0 is not allowed—either clamp to a minimum of 1 (or to DEFAULT_MAX_SCAN_ITEMS) when computing limit or treat Some(0) as None (use DEFAULT_MAX_SCAN_ITEMS); update the logic in enforce_max_scan_limit to derive limit from max_scan_items with that clamp/translate and then keep the existing comparison (return Err(PaginationError::ScanLimitExceeded) only when the computed limit is exceeded).
🧹 Nitpick comments (1)
crates/minibf/src/pagination.rs (1)
300-342: Consider adding unit tests forenforce_max_scan_limit.The test module covers
should_skipand theInvalidFromToerror message, butenforce_max_scan_limithas no direct unit tests. Given this is the primary change, tests covering (a)None→ default fallback, (b)Some(custom_limit)enforced correctly, and (c)ScanLimitExceededreturned when limit is exceeded would prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/minibf/src/pagination.rs` around lines 300 - 342, Add three unit tests in the existing tests module that exercise enforce_max_scan_limit: (1) a test where PaginationParameters.scan_limit is None and enforce_max_scan_limit returns the default fallback limit (assert returned limit equals expected DEFAULT_SCAN_LIMIT), (2) a test where scan_limit is Some(custom) and enforce_max_scan_limit returns that custom value, and (3) a test that simulates a scan count greater than the enforced limit and asserts the function returns PaginationError::ScanLimitExceeded (or that calling code converts it into the appropriate response body/message). Locate and call the enforce_max_scan_limit function (or the method on Pagination if it’s implemented there) and use PaginationParameters/Pagination constructors as needed to set up parameters and trigger each branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/minibf/src/pagination.rs`:
- Around line 211-217: The current enforce_max_scan_limit (max_scan_items:
Option<u64>) treats Some(0) as a hard zero which disables endpoints; change the
resolution of max_scan_items so 0 is not allowed—either clamp to a minimum of 1
(or to DEFAULT_MAX_SCAN_ITEMS) when computing limit or treat Some(0) as None
(use DEFAULT_MAX_SCAN_ITEMS); update the logic in enforce_max_scan_limit to
derive limit from max_scan_items with that clamp/translate and then keep the
existing comparison (return Err(PaginationError::ScanLimitExceeded) only when
the computed limit is exceeded).
---
Nitpick comments:
In `@crates/minibf/src/pagination.rs`:
- Around line 300-342: Add three unit tests in the existing tests module that
exercise enforce_max_scan_limit: (1) a test where
PaginationParameters.scan_limit is None and enforce_max_scan_limit returns the
default fallback limit (assert returned limit equals expected
DEFAULT_SCAN_LIMIT), (2) a test where scan_limit is Some(custom) and
enforce_max_scan_limit returns that custom value, and (3) a test that simulates
a scan count greater than the enforced limit and asserts the function returns
PaginationError::ScanLimitExceeded (or that calling code converts it into the
appropriate response body/message). Locate and call the enforce_max_scan_limit
function (or the method on Pagination if it’s implemented there) and use
PaginationParameters/Pagination constructors as needed to set up parameters and
trigger each branch.
Summary by CodeRabbit