fix(services/vercel-blob): send the pagination cursor so listing terminates - #8071
Conversation
…inates
The lister stores the cursor the server returns:
ctx.done = !resp.has_more;
if let Some(cursor) = resp.cursor {
ctx.token = cursor;
}
and nothing ever reads it back. VercelBlobCore::list took only a prefix
and a limit, so page two was byte-identical to page one:
https://blob.vercel-storage.com?prefix={prefix}&limit={limit}
ctx.token is the only write of that field in the crate. Meanwhile
PageLister::next loops while ctx.done is false, so a listing the server
truncates never advances: the same page is fetched and pushed again for
as long as has_more stays true. Operator::list collects into a Vec and
grows without bound; lister() streams the same entries forever.
Vercel documents cursor beside prefix and limit -- "a string obtained
from a previous list response to be used for reading the next page of
results" -- and the default limit is 1000, so any prefix holding more
than that reaches this without the caller setting anything.
list now takes the cursor and appends it when non-empty, and the URL
construction moves into build_list_url so it can be asserted directly.
The four internal lookups pass "": each asks for Some(1) to resolve a
single blob's URL and never paginates.
One extra guard in the lister: has_more true with no cursor now ends the
listing rather than leaving ctx.done false. That truncates, but the only
other option is re-requesting the same page indefinitely.
Four unit tests. Dropping the cursor from the URL again fails exactly
the two that involve one; the first-page and root-prefix tests hold
either way.
| ctx: &OperationContext, | ||
| prefix: &str, | ||
| limit: Option<usize>, | ||
| cursor: &str, |
| if let Some(limit) = limit { | ||
| url.push_str(&format!("&limit={limit}")) | ||
| } | ||
| let url = build_list_url(prefix, limit, cursor); |
| pub(super) use error::*; | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
Review points from @erickguan, all three applied: - cursor is Option<&str> rather than an empty string standing in for "no cursor"; the four single-blob lookups pass None, and the lister turns an empty ctx.token into None - build_list_url removed, the format! goes back inline - test module removed The functional change is unchanged: the cursor is sent when there is one, percent-encoded because it is an opaque server token.
|
All three applied in 7f1812c.
let cursor = if ctx.token.is_empty() {
None
} else {
Some(ctx.token.as_str())
};
if let Some(cursor) = cursor {
url.push_str(&format!("&cursor={}", percent_encode_path(cursor)));
}Flagging one thing that goes with the tests, not to argue for keeping them: one of the four asserted that the cursor is percent-encoded. A Vercel cursor is an opaque server token, so an unescaped
|
Matching the shape asked for on apache#8068, apache#8069 and apache#8071 rather than waiting to be asked again: the extracted helper goes, the construction is inline, and the test module goes with it. The fix is unchanged -- the concatenation is already relative to the root, so the second build_rel_path is what truncated it, and "/" as the listed path must not become a leading slash on the entry.
Matching the shape asked for on apache#8068, apache#8069 and apache#8071 rather than waiting to be asked again. The function goes back to being a method as it was, and the three tests added with it go. The change is now the two etag lines: a listed directory reports the etag stored with its record, and the synthesised directory placeholder reports none. The relative_to_root tests already in this file are from apache#8048 and are untouched.
Matching the shape asked for on apache#8068, apache#8069 and apache#8071 rather than waiting to be asked again: the two extracted helpers go and the branch is written inline, leaving the existing folder loop untouched. resp becomes mut so embedded can be taken out of it, which keeps the rest of the response available for the file case below.
#8070) * fix(services/ipmfs): stop stripping the root twice from listed entries The listing loop concatenates the path being listed with a files/ls name, then runs the result through build_rel_path a second time: let path = match object.mode() { EntryMode::FILE => format!("{}{}", self.path, object.name), ... }; let path = build_rel_path(&self.root, &path); self.path is what the operator handed the service, so it is already relative to the root -- IpmfsCore::ipmfs_ls is what turns it into a rooted absolute path for the request. The concatenation is therefore root-relative before that call, and the call removes a prefix that is not there. Under the default root "/" the second strip happens to cancel out, which is why this has gone unnoticed. Under any other root, in release: root "/abc/", listing "dir/", name "a" build_rel_path("/abc/", "dir/a") -> "a" the directory is dropped from every entry path root "/abc/", listing the root itself (self.path is "/"), name "a" build_rel_path("/abc/", "/a") panicked: start byte index 5 is out of bounds for string of length 2 In a debug build both cases trip the debug_assert! inside build_rel_path instead. The construction moves into build_entry_path, which takes no root at all -- an entry path does not depend on one. The only thing it has to handle is that self.path is "/" when the root itself is listed, and an entry path carries no leading slash. The self-entry twenty lines above is left alone: it does build_abs_path then build_rel_path, a deliberate round trip, and is correct. Three unit tests. Putting the old expression back passes all of them with a root of "/" and fails three of them with a root of "/abc/" -- which is the shape of the bug. Note for rebasing: #7801 renames this very call to build_relative_path. If that lands first this needs a one-word rebase; the call itself goes away here. * Inline the entry-path construction and drop the test module Matching the shape asked for on #8068, #8069 and #8071 rather than waiting to be asked again: the extracted helper goes, the construction is inline, and the test module goes with it. The fix is unchanged -- the concatenation is already relative to the root, so the second build_rel_path is what truncated it, and "/" as the listed path must not become a leading slash on the entry. * Drop the inline comment
Which issue does this PR close?
None — found while reading the service.
Rationale for this change
VercelBlobLister::next_pagestores the cursor the server returns:and nothing ever reads it back — that assignment is the only mention of
ctx.tokenin the crate.VercelBlobCore::listtook only a prefix and a limit:so page two is byte-identical to page one.
PageLister::nextloops whilectx.doneis false:A truncated listing therefore never advances: the same page is fetched and pushed again for as long as
has_morestays true.Operator::listcollects into aVecand grows without bound;lister()streams the same entries forever.Vercel documents
cursoralongsideprefixandlimit— "A string obtained from a previouslistresponse to be used for reading the next page of results" — and notes the defaultlimitis 1000, so any prefix holding more than that reaches this with the caller setting no options at all.What changes are included in this PR?
listnow takes the cursor and appends it when non-empty, and the URL construction moves intobuild_list_urlso it can be asserted directly. The four internal lookups pass""— each asks forSome(1)to resolve a single blob's URL and never paginates.One extra guard in the lister:
has_moretrue with no cursor now ends the listing rather than leavingctx.donefalse. That truncates, but the only other option is re-requesting the same page indefinitely. Happy to drop it if you would rather not carry the branch.Tests
Four unit tests. Removing the cursor from the URL again fails exactly the two that involve one:
the_first_page_carries_no_cursora_later_page_resumes_from_the_cursorthe_cursor_is_percent_encodedthe_root_prefix_is_sent_emptymain)The cursor is percent-encoded because it is an opaque server token: an unescaped
&or=would silently graft extra parameters onto the query, the same reasonprefixis encoded one line above.cargo test -p opendal-service-vercel-blob --lib→ 6 passed.cargo fmt --all -- --checkandcargo clippy -p opendal-service-vercel-blob --all-targetsboth clean.Are there any user-facing changes?
Yes — a
vercel-bloblisting that the server truncates now advances through its pages and terminates, instead of re-emitting the first page indefinitely.VercelBlobCore::listgains acursorparameter, butmod coreis private and the crate re-exports onlyVercelBlobBuilderandVercelBlobConfig, so nothing outside the crate can name it.