fix(services/cloudflare-kv): strip only the root prefix from listed keys - #8048
Open
PDGGK wants to merge 1 commit into
Open
fix(services/cloudflare-kv): strip only the root prefix from listed keys#8048PDGGK wants to merge 1 commit into
PDGGK wants to merge 1 commit into
Conversation
The lister removed the service root with str::replace, which removes every occurrence anywhere in the key rather than the leading one. Any key that repeats the root as an inner path segment came back mangled: with root /data/, the key data/backup/data/file.txt was listed as backup/file.txt. It also matches mid-key, so xdata/file.txt became xfile.txt. The same expression appeared at both call sites -- once for the key in build_entry_for_item and once for self.path in the non-recursive branch -- so both now go through one relative_to_root helper. The sibling object-store listers use build_rel_path for this. That helper debug_asserts the path really does start with the root, a guarantee this service does not enforce on what the API returns, so the helper here stays total and leaves a non-root-prefixed key alone -- matching what replace did in that case. Three unit tests. Two of them fail against the previous expression; the third covers the root itself and a bare "/" root and passes either way, so the first two are not trivially red.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
None filed — reporting and fixing together.
Rationale of this change
CloudflareKvListerremoved the service root from listed keys withstr::replace:str::replaceremoves every occurrence anywhere in the string, not the leading one. So any key that repeats the root as an inner path segment comes back mangled:/data/data/backup/data/file.txtbackup/file.txtbackup/data/file.txt/data/xdata/file.txtxfile.txtxdata/file.txtThe second row shows it is not even anchored — a key merely containing the root loses that substring.
The same expression appeared at both call sites (
build_entry_for_itemfor the key, and the non-recursive branch forself.path), so both now go through onerelative_to_roothelper.On
build_rel_path: that is what the s3/oss/cos/obs listers use here, and it was my first choice. Itdebug_assert!s that the path really does start with the root — a guarantee this service does not enforce on what the API returns — so a key that is not root-prefixed would panic in debug builds wherereplacepreviously left it alone. The helper therefore stays total and preserves that behaviour. Happy to switch tobuild_rel_pathif you would rather have the assertion.Are there any user-facing changes?
Yes: listed entry names are now correct for keys that repeat the root as an inner segment. Keys that do not are unaffected.
Tests
Three unit tests in
lister.rs. Two fail against the previous expression:The third covers the root itself and a bare
/root and passes both before and after, which is what shows the other two are not trivially red.With the fix: 5/5 pass (including the two pre-existing config tests).
cargo fmt --checkexits 0 andcargo clippy --all-targetsis clean.