Skip to content

fix(services/cloudflare-kv): report the stored etag for listed directories #8067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PDGGK wants to merge 4 commits into from
Closed

fix(services/cloudflare-kv): report the stored etag for listed directories #8067

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2694172
fix(services/cloudflare-kv): report the stored etag for listed direct…
PDGGK Aug 14, 2026
29c77fd
Keep build_entry_for_item a method and drop the added tests
PDGGK Aug 15, 2026
6edf1ec
Add a wire-response test for the listed directory etag
PDGGK Aug 15, 2026
5ad0aba
Say plainly that the fixture is reconstructed, not captured
PDGGK Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 69 additions & 4 deletions core/services/cloudflare-kv/src/lister.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ impl CloudflareKvLister {

let entry_metadata = if name.ends_with('/') {
Metadata::new(EntryMode::DIR)
.with_etag(build_tmp_path_of(&name))
.with_etag(metadata.etag)

@erickguan erickguan Aug 15, 2026

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.

Can you include a Cloudflare response from the wire in the test?

@PDGGK PDGGK Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in 6edf1ec — a list keys response goes through CfKvListResponse rather than a hand-built struct, so the deserialization is part of what is exercised:

{
    "success": true,
    "result": [
        { "name": "data/sub/",   "metadata": { "etag": "sub/.AvaaBbxz",   "last_modified": "...", "content_length": 0, "is_dir": true  } },
        { "name": "data/a.txt", "metadata": { "etag": "a.txt.xHzwzn53", "last_modified": "...", "content_length": 7, "is_dir": false } }
    ],
    "result_info": { "cursor": "" }
}

It asserts both entries, which is the premise the change rests on: a directory key's metadata blob carries the same CfKvMetadata a file's does, etag included, because this service wrote it itself through set. The etag values are shaped the way build_tmp_path_of mints them at write time, so the fixture matches what a real namespace would hold.

Restoring the old expression fails this test and nothing else — 5 passed, 1 failed.

I had stripped the tests from this PR preemptively after your notes on #8068 and #8069; this is a better test than the ones I removed, so thanks for asking for it rather than letting it go.

.with_content_length(0)
} else {
Metadata::new(EntryMode::FILE)
Expand All @@ -110,9 +110,9 @@ impl CloudflareKvLister {
let path_name = relative_to_root(root, &self.path);
let entry = oio::Entry::new(
&format!("{path_name}/"),
Metadata::new(EntryMode::DIR)
.with_etag(build_tmp_path_of(&path_name))
.with_content_length(0),
// This directory is inferred from the keys under it rather than read back from a
// record of its own, so there is no stored etag to report for it.
Metadata::new(EntryMode::DIR).with_content_length(0),
);
ctx.entries.push_back(entry);
}
Expand Down Expand Up @@ -192,6 +192,71 @@ impl oio::PageList for CloudflareKvLister {
mod tests {
use super::*;

#[test]
fn a_listed_directory_reports_the_etag_the_wire_response_carries() {
// Reconstructed, not captured: the envelope is the subset of the `list keys` response
// that CfKvListResponse reads, and the metadata blob is CfKvMetadata as serde emits it,
// since `set` sends `serde_json::to_string(&metadata)` as the metadata form part. The
// etags are shaped the way `build_tmp_path_of` mints them at write time. The point it
// pins is that a directory key carries the same CfKvMetadata a file does.
let body = r#"{

@erickguan erickguan Aug 15, 2026

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.

Where did you get this payload? How did you get it?

@PDGGK PDGGK Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Straight answer: I did not capture it. I have no Cloudflare account and never saw a live response. I reconstructed it from this repository, and my comment saying "as the Cloudflare API returns it" overstated that — corrected in the push just now to say plainly that it is reconstructed.

What each half is actually derived from:

The metadata blob is not Cloudflare's — it is ours. CloudflareWriter::write_once builds it and CloudflareKvCore::set sends it as a form part:

let cf_kv_metadata = CfKvMetadata {
    etag: build_tmp_path_of(&self.path),
    last_modified: Timestamp::now().to_string(),
    content_length: bs.len(),
    is_dir: self.path.ends_with('/'),
};
...
FormDataPart::new("metadata")
    .content(serde_json::to_string(&metadata)?)

So etag / last_modified / content_length / is_dir are exactly what serde emits for CfKvMetadata, and the etag values are shaped the way build_tmp_path_of mints them. That half is checkable against the source rather than taken on trust.

The envelope — success, result, result_info — I wrote to match what CfKvListResponse deserializes. Extra fields Cloudflare sends are ignored by serde, and fields it does not send would show up as a parse failure, so the test would not silently pass on a wrong shape. But I cannot claim the envelope is byte-faithful, only that it is a superset-compatible subset.

If that is not good enough, three options and I am happy with any of them:

  1. Keep it, with the corrected comment making the provenance explicit.
  2. Replace it with a real capture if you or someone with a namespace can paste one — I would rather use a genuine payload.
  3. Drop the JSON and go back to constructing CfKvListKey directly, which tests the entry logic without pretending to test the wire format.

Tell me which and I will push it.

"errors": [],
"messages": [],
"success": true,
"result": [
{
"name": "data/sub/",
"metadata": {
"etag": "sub/.AvaaBbxz",
"last_modified": "2024-01-01T00:00:00Z",
"content_length": 0,
"is_dir": true
}
},
{
"name": "data/a.txt",
"metadata": {
"etag": "a.txt.xHzwzn53",
"last_modified": "2024-01-01T00:00:00Z",
"content_length": 7,
"is_dir": false
}
}
],
"result_info": { "cursor": "" }
}"#;

let resp: CfKvListResponse = serde_json::from_str(body).expect("response must parse");
let items = resp.result.expect("result must be present");

let lister = CloudflareKvLister::new(
Arc::new(CloudflareKvCore {
api_token: "token".to_string(),
account_id: "account".to_string(),
namespace_id: "namespace".to_string(),
expiration_ttl: None,
info: ServiceInfo::new(crate::CLOUDFLARE_KV_SCHEME, "/data/", "namespace"),
capability: Capability::default(),
}),
OperationContext::default(),
"/",
false,
None,
);

let dir = lister
.build_entry_for_item(&items[0], "/data/")
.expect("directory entry");
assert_eq!(dir.path(), "sub/");
assert_eq!(dir.metadata().etag(), Some("sub/.AvaaBbxz"));

let file = lister
.build_entry_for_item(&items[1], "/data/")
.expect("file entry");
assert_eq!(file.path(), "a.txt");
assert_eq!(file.metadata().etag(), Some("a.txt.xHzwzn53"));
}

#[test]
fn relative_to_root_strips_only_the_prefix() {
// The root repeated as an inner segment must survive; only the leading copy goes.
Expand Down
Loading