[JGamboa]

📑 Description

Closes goravel/goravel#1017

Aligns db.Query.Paginate with orm.Query.Paginate:

• When total is nil, the COUNT query is skipped instead of dereferencing a nil pointer, which used to panic.
• page < 1 is treated as the first page. Before, (page-1)*limit went negative and turned into OFFSET 18446744073709551606 after the uint64 conversion.

offset := max((page-1)*limit, 0)
if total != nil {
    count, err := r.clone().Count()
    if err != nil {
        return err
    }
    *total = count
}

A negative limit is left unchanged (see the open question in the issue).

✅ Checks

• Added test cases for my code
  • TestPaginate in database/db/query_test.go covers a nil total, page = 0 and page = -1. It fails on master and passes here.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.91%. Comparing base (7ffe4a8) to head (a19f651).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1581   +/-   ##
=======================================
  Coverage   72.91%   72.91%           
=======================================
  Files         412      412           
  Lines       26893    26896    +3     
=======================================
+ Hits        19608    19612    +4     
+ Misses       7283     7282    -1     
  Partials        2        2           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JGamboa]

Review Ready

[goravel-agent]

🤖 Automated review

This is an AI-generated code review. Please double-check each finding before acting. You don't need to address every issue if they are inaccurate, but please point them out if any exist.

Summary

db.Query.Paginate now skips the COUNT query when total is nil and clamps page < 1 to the first page, with three new subtests that fail on master and pass here. The fix is correct and tested; the only real gap left is the unvalidated limit that the new clamp now pairs with a zero offset.

Verdict

• [NEW] Must Fix: 0 · Should Fix: 1 · Nits: 2

Findings

Should Fix

1. database/db/query.go:686 Negative limit now reads the whole table
   Problem: The offset clamp makes a negative limit produce OFFSET 0 LIMIT 18446744073709551615 (from uint64(limit)), where before it produced a huge offset and an empty result.
   Impact: A bad limit from a request parameter turns into a full-table fetch instead of failing or returning nothing, which is a memory and load risk.
   Fix: Reject or default a non-positive limit before the conversion — the clamp alone no longer contains it.

   // before
   offset := max((page-1)*limit, 0)
   // after
   if limit < 1 {
       limit = 1 // or return a declared error
   }
   offset := max((page-1)*limit, 0)

Nits (2)

Nits

1. database/db/query.go:686 Clamp hides integer overflow as page 1
   Problem: max((page-1)*limit, 0) cannot tell a page < 1 from an int product that overflowed negative, so a huge page silently returns the first page instead of an out-of-range offset.
   Impact: A malformed page value gets a valid-looking 200 with page 1 rows, which hides the bad input.
   Fix: Normalize the page before the arithmetic so overflow is not converted into "first page".

   // before
   offset := max((page-1)*limit, 0)
   // after
   page = max(page, 1)
   offset := (page - 1) * limit

2. database/db/query_test.go:1341 Assertion in error path checks nothing
   Problem: s.Empty(users) is always true because an unexpected SelectContext call already fails the test through the missing mock expectation, and nothing asserts that total was left untouched.
   Impact: The "count failed, so no page query and no total write" contract is only half pinned.
   Fix: Replace it with an assertion on the untouched total.

   // before
   s.Empty(users)
   // after
   s.Zero(total)

[JGamboa]

Thanks for the review. All three points are addressed in a19f651:

• Negative limit (Should Fix): a limit < 1 now returns the new declared error DatabaseInvalidPaginationLimit instead of becoming a huge LIMIT. I chose an error over defaulting to 1 so that bad input isn't silently turned into a valid page. Note that limit = 0 used to return an empty page and now returns this error. I can switch to defaulting if you prefer.
• Overflow nit: page is normalized with max(page, 1) before the multiplication.
• Test nit: the count error case now asserts s.Zero(total). New subtests cover limit 0 and -10, with no COUNT or page query.

[hwbrzzl]

Thanks, LGTM