Skip to content

fix(services/dbfs): root the put path and stop encoding paths in JSON bodies - #8079

Merged
erickguan merged 2 commits into
apache:mainfrom
PDGGK:fix-dbfs-json-body-paths
Aug 15, 2026
Merged

fix(services/dbfs): root the put path and stop encoding paths in JSON bodies#8079
erickguan merged 2 commits into
apache:mainfrom
PDGGK:fix-dbfs-json-body-paths

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

Two defects in how dbfs addresses paths, both in core.rs.

1. The dbfs/put body is not rooted.

let req_body = &json!({
    "path": path,          // <- operator-relative, as handed to DbfsBackend::write
    "contents": contents,
    "overwrite": true,
});

Every sibling roots it first — create_dir at :60, delete at :89, rename at :116, list at :142, get-status at :193 all call build_rooted_abs_path. Nothing else applies the root; it lives only in DbfsCore.root. So with any non-default root, a write and the stat, list or delete that follows it address different DBFS paths.

2. Four values are percent-encoded inside JSON request bodies.

"path": percent_encode_path(&p),                     // :65, :94
"source_path": percent_encode_path(&source),         // :126
"destination_path": percent_encode_path(&target),    // :127

Nothing URL-decodes a JSON string value. The genuine query strings at :149 and :200 are encoded, and the server does decode those — so the two halves of this file disagree with each other:

  • create_dir("my dir/") creates a directory literally named my%20dir,
  • the get-status that follows asks for my dir and gets NotFound.

For delete it is worse than an error: deleter.rs notes the server answers 200 even when the path does not exist, so the call reports success having removed nothing.

A repo-wide grep for percent_encode_path inside a json! across core/services matches these four lines and nothing else — dbfs is the only service in the tree that does it.

Blast radius

Neither change affects an ordinary path. percent_encode_path leaves A-Z a-z 0-9 / - _ . ! ~ * ' ( ) untouched, so a plain key serialises identically before and after. Only a path containing a space, %, #, ?, &, +, ,, :, =, @ or a non-ASCII byte changes — and those are exactly the ones broken today.

Tests

One, on the only one of the four that is a synchronous request builder:

let req = core.dbfs_create_file_request("my file.txt", Bytes::from_static(b"hello"))?;
let body: serde_json::Value = serde_json::from_slice(&req.into_body().to_bytes())?;
assert_eq!(body["path"], "/data/my file.txt");

It pins both halves at once — rooted, and not encoded. Reverting the rooting fails it and nothing else:

assertion `left == right` failed
  left: String("my file.txt")
 right: "/data/my file.txt"

The other three send inside async methods, so pinning them would mean restructuring the functions; their evidence is the in-file inconsistency above. Happy to add more if you would like the coverage.

Worth noting that services-dbfs has no directory under .github/services, so nothing has been exercising any of this.

Note for rebasing

#7801 renames build_rooted_abs_path in this file. If it lands first, the new call in dbfs_create_file_request needs the new name.

Are there any user-facing changes?

Yes, for services-dbfs: a write now lands at the same DBFS path that stat, list and delete use, and a path containing a character percent_encode_path escapes is no longer created, deleted or renamed under its escaped spelling.

Not in this PR

The lister emits root-prefixed entry paths — lister.rs never calls build_rel_path, unlike the other ~34 HTTP service listers. That is a separate file and a separate user-visible surface, so I have kept it out to stay independently revertable.

… bodies

Two defects in how this service addresses paths, both in core.rs.

The path in the dbfs/put body is not rooted:

    let req_body = &json!({
        "path": path,

Every sibling roots it first -- create_dir at :60, delete at :89,
rename at :116, list at :142, get-status at :193 all call
build_rooted_abs_path. The value arrives from DbfsBackend::write as the
operator-relative path and nothing else applies the root, so with any
non-default root a write and the stat, list or delete that follows it
address different DBFS paths.

And four values are percent-encoded inside JSON request bodies:

    "path": percent_encode_path(&p),                     // :65, :94
    "source_path": percent_encode_path(&source),         // :126
    "destination_path": percent_encode_path(&target),    // :127

Nothing URL-decodes a JSON string value. The genuine query strings at
:149 and :200 are encoded, and the server does decode those -- so the
two halves of this file disagree with each other. create_dir("my dir/")
creates a directory literally named my%20dir, while the get-status that
follows asks for "my dir" and gets NotFound. For delete it is worse than
an error: deleter.rs notes the server answers 200 even when the path
does not exist, so the call reports success having removed nothing.

A repo-wide grep for percent_encode_path inside json! across
core/services matches these four lines and nothing else; dbfs is the
only service in the tree that does it.

Neither change affects an ordinary path. percent_encode_path leaves
A-Z a-z 0-9 / - _ . ! ~ * ' ( ) untouched, so a plain key serialises
identically; only a path containing a space, %, #, ?, &, +, ,, :, =, @
or a non-ASCII byte changes, and those are the ones broken today.

One test, on the only one of the four that is a synchronous request
builder. The other three send inside async methods, so pinning them
would mean restructuring; their evidence is the in-file inconsistency
above. Reverting the rooting fails that test and nothing else.

The lister emits root-prefixed entry paths as well -- that is a separate
change to a separate file and is not in here.

Note for rebasing: apache#7801 renames build_rooted_abs_path in this file. If
it lands first the new call in dbfs_create_file_request needs the new
name.
@PDGGK
PDGGK requested a review from Xuanwo as a code owner August 15, 2026 05:48
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. releases-note/fix The PR fixes a bug or has a title that begins with "fix" labels Aug 15, 2026
Comment thread core/services/dbfs/src/core.rs Outdated

pub(super) use error::*;

#[cfg(test)]

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.

Remove tests.

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.

Removed. The diff is now 5 lines, all in core.rs.

For what it is worth, I think I have your bar straight now: a test earns its place when it establishes something reading the code cannot. That test asserted build_rooted_abs_path(&self.root, path) yields /data/my file.txt, which is plain from the call itself — whereas the thing you were after on #8067 was a real API response, which no hand-built fixture can stand in for. I will aim at that line rather than at a test count.

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. 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 dccfa00 into apache:main Aug 15, 2026
108 of 109 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