Return when total < request page #1766
Conversation
WalkthroughAdded a boundary check in getCachedTxsInfo to return no results when the requested start index is >= total items; bumped internal version from v4.7.4 to v4.7.5. Minor formatting change in processAndCacheTxHistoryInfo only. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HistoryLogic as History Logic (getCachedTxsInfo)
participant Cache
Client->>HistoryLogic: requestTxs(start, limit)
opt start < total
HistoryLogic->>Cache: fetchSlice(start, limit)
Cache-->>HistoryLogic: txItems, total
HistoryLogic-->>Client: items, total, hasMore
end
opt start >= total
HistoryLogic-->>Client: nil, 0, false
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1766 +/- ##
===========================================
- Coverage 36.54% 36.53% -0.01%
===========================================
Files 247 247
Lines 21186 21188 +2
===========================================
Hits 7742 7742
- Misses 12614 12616 +2
Partials 830 830
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bridge-history-api/internal/logic/history_logic.go (1)
365-377: Off‑by‑one and cache‑hit semantics for out‑of‑range pagesWith 0‑based indices, valid positions are
[0, total-1], sostart == totalis already out of range. Usingstart > totalmeans the PR’s own example (10 records, pageSize=5, page=3 →start=10,total=10) will not be caught here and will still flow toZRevRangeand then down the “cache miss” path.Also, returning
isHit=falsefor an out‑of‑range page forces a DB query and can trigger the"cache miss after write, expect hit"log inprocessAndCacheTxHistoryInfo, even though the cache already contains the full dataset and we know this page is simply empty.Consider tightening the guard and treating this as a cache hit with an empty page:
func (h *HistoryLogic) getCachedTxsInfo(ctx context.Context, cacheKey string, pageNum, pageSize uint64) ([]*types.TxHistoryInfo, uint64, bool, error) { start := int64((pageNum - 1) * pageSize) end := start + int64(pageSize) - 1 total, err := h.redis.ZCard(ctx, cacheKey).Result() if err != nil { log.Error("failed to get zcard result", "error", err) return nil, 0, false, err } - if start > total { - return nil, 0, false, nil - } - - if total == 0 { - return nil, 0, false, nil - } + // No cache populated yet. + if total == 0 { + return nil, 0, false, nil + } + + // Cache exists, but requested page is beyond available items. + // Treat this as an empty but valid page and avoid hitting DB again. + if start >= total { + return nil, 0, true, nil + }This aligns the behavior with the PR description (e.g., page 3 when there are only 10 items) and avoids unnecessary DB work and noisy error logs in
processAndCacheTxHistoryInfo.
🧹 Nitpick comments (2)
bridge-history-api/internal/logic/history_logic.go (1)
447-458: processAndCacheTxHistoryInfo behavior remains correct; log depends on getCachedTxsInfo semanticsThe logic here is fine and the only change is formatting. Note that if
getCachedTxsInfocontinues to returnisHit=falsefor out‑of‑range pages after caching, this"cache miss after write, expect hit"branch will still fire (witherr == nil). If you adopt the adjustedgetCachedTxsInfosemantics above (treating out‑of‑range asisHit=true), this path will only indicate real inconsistencies.common/version/version.go (1)
8-8: Version tag bump to v4.7.5 looks consistentThe tag update cleanly advances the reported protocol version and is consistent with the rest of the file; no behavioral issues here. As a minor process note, ensure the PR title follows Conventional Commits (e.g.,
fix: return when total < request page) so it passes squash‑merge policies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bridge-history-api/internal/logic/history_logic.go(2 hunks)common/version/version.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests
- GitHub Check: test
- GitHub Check: check
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bridge-history-api/internal/logic/history_logic.go (1)
361-376: Boundary check is logically correct; add tests and considerisHit/totalsemantics for out-of-range pagesThe new
if start >= total { return nil, 0, false, nil }guard is correct for 0-based pagination and avoids an unnecessary Redis range call when the requested page is beyond the last item.Two follow-ups worth addressing:
Tests / coverage
Codecov reports these new lines as uncovered. Please add tests that exercise:
- A non-empty sorted set where
(pageNum-1)*pageSizeis equal tototal(out-of-range page).- Verifying that
getCachedTxsInforeturns an empty slice and no error for that case.
This will bring patch coverage up and lock in the desired behavior.
isHitandtotalbehavior
For “page beyond last item” you currently returnisHit=falseandtotal=0, same as before. That means upstream methods will still treat this as a cache miss and go back to the DB even though the full dataset is already cached, and clients lose the realtotalcount.
Consider instead treating this as a cache hit with an empty page, e.g.:if start >= total {
- return nil, 0, false, nil
- return nil, uint64(total), true, nil
}This would (a) prevent unnecessary DB work on out-of-range pages and (b) keep `total` consistent with pages that do have results. If you go this route, you should also ensure call sites and metrics treat this as an expected “empty last page” rather than a miss. </blockquote></details> </blockquote></details>🧹 Nitpick comments (1)
bridge-history-api/internal/logic/history_logic.go (1)
446-457: Clarify!isHithandling after cache write; current log uses a nilerrIn
processAndCacheTxHistoryInfo, whengetCachedTxsInforeturns!isHitbuterr == nil, the code logs:log.Error("cache miss after write, expect hit", ..., "error", err) return nil, 0, errGiven the current
getCachedTxsInfologic, reaching this block implies no error occurred, soerris alwaysnil. That results in an error log with a nilerrorfield and returns a nil error to the caller, which is confusing both for logs and for callers.Two options to improve this:
If a cache miss here is truly exceptional, construct and return a real error instead of reusing
err:if !isHit {
log.Error("cache miss after write, expect hit", "cached key", cacheKey, "page", page, "page size", pageSize, "error", err)
return nil, 0, err
}
- if !isHit {
- cacheErr := errors.New("cache miss after write, expect hit")
- log.Error("cache miss after write, expect hit", "cached key", cacheKey, "page", page, "page size", pageSize, "error", cacheErr)
- return nil, 0, cacheErr
- }
- If you adopt the suggested change where out-of-range pages are treated as cache hits with empty results, then
!isHithere should only indicate genuine anomalies; you could keep the structure above but this path would be much rarer.Either way, cleaning this up will make logs and error propagation more accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bridge-history-api/internal/logic/history_logic.go(2 hunks)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
Imagine there are 10 records in db, the size of one page is 5, the user queries it like this:
The
page=3&page_size=5query is more than the number of records in db, so we should return early when start > total.The
page=3&page_size=5query result cannot be found in cache, so we try to select from db, however the db returns no records, so we cannot cache the result. So every time the user queriespage=3&page_size=5, this query will hit the db.PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.