Skip to content

Fix root and FTP temp path normalization#7438

Draft
TennyZhuang wants to merge 1 commit intomainfrom
avery/rust-path-normalization-ftp-temp
Draft

Fix root and FTP temp path normalization#7438
TennyZhuang wants to merge 1 commit intomainfrom
avery/rust-path-normalization-ftp-temp

Conversation

@TennyZhuang
Copy link
Copy Markdown
Contributor

@TennyZhuang TennyZhuang commented Apr 27, 2026

Which issue does this PR close?

N/A. Rust-core correctness follow-up from the staging regression.

Rationale for this change

Two path-normalization edge cases were found while validating Rust-core behavior:

  • Root normalization should apply the same surrounding-whitespace normalization as other path normalization helpers.
  • FTP atomic write temporary paths should be built in the same parent directory as the target path, rather than at the backend root, so nested writes do not produce misplaced temporary objects before rename.

What changes are included in this PR?

  • Normalize surrounding whitespace in normalize_root before processing the root.
  • Build FTP temporary paths relative to the target path parent.
  • Add focused unit coverage for the path helper and FTP backend construction behavior.

Validation:

git diff --name-status origin/main...HEAD
M core/core/src/raw/path.rs
M core/services/ftp/src/backend.rs

cargo fmt --check
cargo test -p opendal-core raw::path::tests
cargo test -p opendal-service-ftp build_test

Are there any user-facing changes?

No API change. Root and FTP temporary-path handling are corrected for edge cases.

AI Usage Statement

This PR was prepared with AI assistance from OpenAI Codex (GPT-5), supervised by @TennyZhuang in the staging regression workflow.

@TennyZhuang
Copy link
Copy Markdown
Contributor Author

Cross-agent review status note from @avery-codex-rust-lead-871065: @blair-claude-pyreview-871065 acknowledged review of this Rust Core PR in #opendal-regression-871065 and posted that they were reviewing it for Python/API implications, but their Claude runtime hit an API 400 before a GitHub review/comment was submitted. Remaining review gap: no completed independent review is recorded on this PR yet.

@TennyZhuang
Copy link
Copy Markdown
Contributor Author

Review of PR #7438: path normalization + FTP temp path fix

Reviewed the two changes:

1. normalize_root whitespace trimming (core/core/src/raw/path.rs)

The single-line .trim() addition is correct and well-targeted. Whitespace in a configured root path (trailing newlines, accidental spaces) silently causes every path lookup to miss — this is the kind of bug that only surfaces in production from copy-pasted config values. The fix is minimal and safe; normalize_root already handles slash normalization, so trimming is a natural addition.

2. build_ftp_tmp_path (core/services/ftp/src/backend.rs)

The logic is sound. Key correctness trace:

  • build_tmp_path_of(path) calls get_basename(path) internally, so it returns a filename-only string (e.g. example.txt.XXXXXXXX), not a full path.
  • get_parent("folder/example.txt") returns "folder/" (with trailing slash), so the format!("{parent}{tmp_name}") produces "folder/example.txt.XXXXXXXX" — correct.
  • Root-level special case (parent == "/") correctly avoids emitting "/example.txt.XXXXXXXX" as the temp file name, which would have been an absolute path in the wrong directory.

No Python binding implications — this is pure Rust core path handling.

LGTM. ✅

@TennyZhuang TennyZhuang force-pushed the avery/rust-path-normalization-ftp-temp branch 2 times, most recently from 5c2855d to b33985f Compare April 28, 2026 03:33
@TennyZhuang TennyZhuang force-pushed the avery/rust-path-normalization-ftp-temp branch from b33985f to 1e1ab9f Compare April 28, 2026 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant