Skip to content

fix(search): end unified search pagination and order results stably - #9071

Open
AlyNotMe wants to merge 1 commit into
LibreSign:mainfrom
AlyNotMe:fix/unified-search-pagination
Open

AlyNotMe wants to merge 1 commit into
LibreSign:mainfrom
AlyNotMe:fix/unified-search-pagination

Conversation

@AlyNotMe

@AlyNotMe AlyNotMe commented Oct 9, 2026

Copy link
Copy Markdown

Closes #9067

What

  • Validate the Unified Search cursor and limit before any database access
  • Request limit + 1 rows to detect a next page; return complete() when
    there is none and paginated() only when more rows remain
  • Apply the SQL offset directly (setFirstResult/setMaxResults)
    instead of deriving a fractional page number
  • Add id as a tie-breaker after created_at so equal timestamps keep
    a stable order. The shared query builder is untouched, so other file
    lists keep their current sorting

Tests

  • Unit: cursor/limit validation (the mapper is never called on invalid
    input), limit + 1 request, empty/partial/exact-limit/next-page cases,
    and the extra row is never returned
  • Database: stable order with equal timestamps, consecutive pages without
    duplicates or gaps, offset not a multiple of the limit, limit respected
  • Ran composer test:unit, composer test:integration (new test),
    composer cs:check and composer psalm locally

Fix the Unified Search provider so pagination terminates correctly and
returns stable results.

- Validate the cursor (null, int or digits-only string) and the limit
  before any database access
- Fetch one extra row to know if a next page exists, and return
  complete() instead of always returning paginated()
- Apply the SQL offset directly instead of deriving a fractional page
- Order by created_at then id so equal timestamps give a stable order

Related issue: LibreSign#9067
Type: Bug fix

- [x] Validate cursor and limit in FileSearchProvider::search()
- [x] Return complete() when no next page exists
- [x] Use setFirstResult/setMaxResults in getFilesToSearchProvider()
- [x] Add id as ordering tie-breaker
- [x] Add unit tests for the provider contract
- [x] Add database tests for ordering and offset

Signed-off-by: KMS <243513302+AlyNotMe@users.noreply.github.com>
@AlyNotMe
AlyNotMe requested a review from a team as a code owner October 9, 2026 13:51
@YvesCesar
YvesCesar self-requested a review October 13, 2026 15:27

@YvesCesar YvesCesar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlyNotMe

Thanks for the PR! I reviewed and tested it locally on MySQL:

  • tests/php/Unit/Search/FileSearchProviderTest.php: 21 tests passing
  • tests/php/Integration/Db/SignRequestMapperSearchPaginationTest.php: 5 tests passing
  • Running the same tests against the previous code fails, so they really cover the fix.

Cursor validation, the limit + 1 next-page detection and the f.id tie-breaker all look correct to me. No changes requested from my side.

@AlyNotMe

Copy link
Copy Markdown
Author

Thanks for the review, @YvesCesar, and thanks @vitormattos for assigning me!

About the failing PHPUnit integration jobs: they are all the same
PageControllerTest ArgumentCountError (PageController::__construct():
Argument #10 $validationEffectivePolicyService not passed). That constructor
argument was added on main by 20d806a and PageControllerTest was not
updated, so it also fails without this PR. Happy to open a small separate
PR to fix that test if you'd like.

One more thing: I'm a Node.js backend developer looking for my first
developer job, and the companies I'm talking to ask me for real open source
contributions in PHP. I enjoyed working on this one, and I'd be glad to
keep contributing to LibreSign. Is there anything I should know to become a
regular contributor, and are there other good first issues you would
recommend I take next? I'll comment on an issue before starting, as the
contributing guide asks.

Thanks again for your time!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix Unified Search pagination and deterministic ordering

2 participants