Skip to content

fix(services/lakefs): follow the pagination cursor instead of stopping after one page - #8088

Merged
erickguan merged 1 commit into
apache:mainfrom
PDGGK:fix-lakefs-pagination
Aug 15, 2026
Merged

fix(services/lakefs): follow the pagination cursor instead of stopping after one page#8088
erickguan merged 1 commit into
apache:mainfrom
PDGGK:fix-lakefs-pagination

Conversation

@PDGGK

@PDGGK PDGGK commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

None — found while reading the service.

Rationale for this change

The lister decodes LakefsListResponse, whose Pagination carries the cursor:

pub(super) struct Pagination {
    pub has_more: bool,
    pub max_per_page: u64,
    pub next_offset: String,
    pub results: u64,
}

and then sets ctx.done = true unconditionally. has_more and next_offset appear in the crate only in their own declarations (core.rs:319, :321) — nothing reads them.

So a listing returns the first server page and reports success. Nothing errors, nothing warns; the caller simply receives fewer entries than exist. lakefs does not declare list_with_limit, so no &amount is sent and the server's own default page size applies. And since it does not declare list_with_recursive either, a recursive listing is emulated by driving this lister per directory — so every level truncates independently.

What changes are included in this PR?

The cursor is fed back as after on later pages. The parameter already exists (core.rs:146), and the lister already sends the caller's start_after on the first page with the comment "start after should only be set for the first page" — this fills in the pages after it.

Why the encoding is in the same PR

core.rs:147 concatenated after raw:

url.push_str(&format!("&after={after}"));

next_offset is an object path chosen by the server. Driving the cursor is what makes a non-empty after reachable at all — before this change the parameter was only ever set from the caller's start_after. Shipping the cursor without the encoding would therefore introduce exactly the defect #8073 fixed across five services: a key with a space aborts the request with invalid uri character, one with # truncates the parameter and silently rewinds the page, one with & grafts a stray parameter. Splitting them would mean knowingly landing a regression and fixing it afterwards.

percent_encode_path is the right helper here — next_offset is a path, and it is what prefix on line 135 already uses.

Tests

None. The behaviour is the server's pagination contract, and there is no directory under .github/services for lakefs, so nothing exercises the lister either way.

What I can and cannot claim, stated plainly: that the fields are parsed and never read is verified by grep over the crate; that after is the parameter to feed next_offset back into rests on the two names and on after already being this endpoint's resume parameter. I have no lakeFS instance to confirm it against. If you would rather see it exercised first, say so and I will leave it.

cargo build, cargo clippy -p opendal-service-lakefs --all-targets (zero warnings) and cargo fmt --all -- --check are clean.

Are there any user-facing changes?

Yes: listing a directory with more entries than one server page returns all of them instead of silently stopping at the first page.

…g after one page

The lister decodes LakefsListResponse -- whose Pagination carries
has_more and next_offset -- and then sets ctx.done = true
unconditionally. Both fields appear in the crate only in their own
declarations at core.rs:319 and :321; nothing reads them.

So a listing returns the first server page and reports success. Nothing
errors, nothing warns; the caller simply receives fewer entries than
exist. lakefs does not declare list_with_limit, so no &amount is sent
and the server's own default page size applies, and recursive listing is
emulated by driving this lister per directory, so every level truncates
independently.

The cursor is now fed back as after on later pages. The parameter
already exists and the lister already sends the caller's start_after on
the first page, so this only fills in the pages after it.

The encoding belongs in the same change rather than a follow-up:
core.rs:147 concatenated after raw, and next_offset is an object path
chosen by the server. Driving the cursor is what makes a non-empty after
reachable at all, so shipping one without the other would introduce
exactly the defect apache#8073 fixed across five services -- a key with a
space aborting the request, one with # silently rewinding the page.
@PDGGK
PDGGK requested a review from Xuanwo as a code owner August 15, 2026 08:48
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. releases-note/fix The PR fixes a bug or has a title that begins with "fix" labels Aug 15, 2026
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Aug 15, 2026
@erickguan
erickguan merged commit e25a9af into apache:main Aug 15, 2026
111 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer releases-note/fix The PR fixes a bug or has a title that begins with "fix" size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants