[Storage] Fix double encoding of path names on Datalake clients - #50083
[Storage] Fix double encoding of path names on Datalake clients #50083Isabelle (ibrandes) wants to merge 2 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 34 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Fixes a long-standing path-name encoding inconsistency in azure-storage-file-datalake where clients returned from rename / renameWithResponse / undeletePath could double-encode the path name on subsequent DFS calls, and updates stale JavaDoc that incorrectly told customers to pre-encode path names. Adds a comprehensive offline test suite that asserts the exact on-the-wire encoding behavior, plus a CHANGELOG entry.
Changes:
- Remove stale
Utility.urlEncode(...)usage fromDataLakeFile*/DataLakeDirectory*copy constructors to prevent double-encoding on returned clients. - Update JavaDoc across
DataLakeFileSystemClient/DataLakeFileSystemAsyncClientandDataLakePathClientBuilder.pathName(String)to instruct callers to pass unencoded names. - Add
DataLakePathNameEncodingTeststo lock down encoding behavior and prevent regressions; updateCHANGELOG.md.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/storage/azure-storage-file-datalake/src/test/java/com/azure/storage/file/datalake/DataLakePathNameEncodingTests.java | New offline test suite asserting verbatim storage + request-time percent-encoding behavior, including rename/undelete regressions. |
| sdk/storage/azure-storage-file-datalake/src/main/java/com/azure/storage/file/datalake/DataLakePathClientBuilder.java | JavaDoc update to clarify callers must pass unencoded path names. |
| sdk/storage/azure-storage-file-datalake/src/main/java/com/azure/storage/file/datalake/DataLakeFileSystemClient.java | JavaDoc updates to remove incorrect “pre-encode path” guidance. |
| sdk/storage/azure-storage-file-datalake/src/main/java/com/azure/storage/file/datalake/DataLakeFileSystemAsyncClient.java | Async JavaDoc updates mirroring sync guidance on raw path names. |
| sdk/storage/azure-storage-file-datalake/src/main/java/com/azure/storage/file/datalake/DataLakeFileClient.java | Stop re-encoding pathName when constructing returned file clients. |
| sdk/storage/azure-storage-file-datalake/src/main/java/com/azure/storage/file/datalake/DataLakeFileAsyncClient.java | Stop re-encoding pathName when constructing returned async file clients (and remove now-unused import). |
| sdk/storage/azure-storage-file-datalake/src/main/java/com/azure/storage/file/datalake/DataLakeDirectoryClient.java | Stop re-encoding pathName when constructing returned directory clients (and remove now-unused import). |
| sdk/storage/azure-storage-file-datalake/src/main/java/com/azure/storage/file/datalake/DataLakeDirectoryAsyncClient.java | Stop re-encoding pathName when constructing returned async directory clients (and remove now-unused import). |
| sdk/storage/azure-storage-file-datalake/CHANGELOG.md | Document the bug fix + JavaDoc correction under 12.29.0-beta.2 (Unreleased). |
Suppressed comments (1)
sdk/storage/azure-storage-file-datalake/src/test/java/com/azure/storage/file/datalake/DataLakePathNameEncodingTests.java:232
- This Stream.of(...) is currently written as a long single statement across a couple of very long lines, which is likely to trip the 120-character Checkstyle LineLength rule. Please wrap it with one Arguments.of(...) per line for readability and to keep line lengths under the limit.
private static Stream<Arguments> windowsReservedCharacterSupplier() {
return Stream.of(Arguments.of("a\"b", "a%22b"), Arguments.of("a\\b", "a%5Cb"), Arguments.of("a/b", "a%2Fb"),
Arguments.of("a:b", "a:b"), Arguments.of("a|b", "a%7Cb"), Arguments.of("a<b", "a%3Cb"),
Arguments.of("a>b", "a%3Eb"), Arguments.of("a*b", "a*b"), Arguments.of("a?b", "a%3Fb"));
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| dataLakePathClient.getServiceVersion(), dataLakePathClient.getAccountName(), | ||
| dataLakePathClient.getFileSystemName(), Utility.urlEncode(dataLakePathClient.pathName), | ||
| PathResourceType.DIRECTORY, dataLakePathClient.getSasToken(), dataLakePathClient.getCpkInfo(), | ||
| dataLakePathClient.getFileSystemName(), dataLakePathClient.pathName, PathResourceType.DIRECTORY, |
| dataLakePathClient.getServiceVersion(), dataLakePathClient.getAccountName(), | ||
| dataLakePathClient.getFileSystemName(), Utility.urlEncode(dataLakePathClient.pathName), | ||
| PathResourceType.FILE, dataLakePathClient.getSasToken(), dataLakePathClient.getCpkInfo(), | ||
| dataLakePathClient.getFileSystemName(), dataLakePathClient.pathName, PathResourceType.FILE, |
Fix double-encoding of path names on clients returned by
rename/undeletePath, and correct stale encoding javadocAddresses #48040
Two related fixes to
azure-storage-file-datalake, both surfaced while investigating a customer question about which special characters callers must percent-encode before passing a path name to the SDK:DataLakeFileClient/DataLakeDirectoryClient(and async equivalents) returned byrename,renameWithResponse, andundeletePathcarried a URL-encoded path name, so subsequent Data Lake endpoint requests made through that client targeted a double-encoded path.Reason for the change
The bug
DataLakeFileClient(DataLakePathClient),DataLakeDirectoryClient(DataLakePathClient),DataLakeFileAsyncClient(DataLakePathAsyncClient)andDataLakeDirectoryAsyncClient(DataLakePathAsyncClient)each appliedUtility.urlEncode(pathName)when copying the parent client's state.This was correct at the time it was written.
DataLakePathAsyncClientused to storeUtility.urlDecode(pathName), so the encode simply round-tripped the name back to its stored form. Commit4316086b1d0(blob, 12.19.0) andb93671528c7(datalake, 12.22.0) removed the decode — both shipped as documented breaking changes — but the matching encode in the copy constructors was left behind, turning a round-trip into a double-encode.The resulting client was internally inconsistent, which is why this went unnoticed:
getProperties,delete, …) reuse the parent'sBlockBlobClient, which was built from the raw name → correct URL.create,setAccessControl, …) andgetFileUrl()/getDirectoryUrl()use the re-encodedpathName→ double-encoded URL.Measured behaviour before the fix, renaming to
dest%20%25:renamerequest itself/fs/dest%2520%2525renamed.getProperties()(blob endpoint)/fs/dest%2520%2525renamed.create()(dfs endpoint)/fs/dest%252520%252525renamed.getFileUrl().../dest%252520%252525The existing recorded test
FileApiTest.renameUrlEncodedpasses despite the bug because it only assertsgetPropertiesWithResponse— the blob-endpoint half.I verified every call site of these four constructors (
renamein the four path clients,undeletePathin the two file system clients). In all six the sourcepathNameis the raw, user-supplied name, so dropping the encode is correct everywhere.The javadoc
DataLakePathAsyncClientstorespathNameverbatim and never decodes it. Encoding happens once, at request-build time:PathsImpldeclares@PathParam("path")withoutencoded = true, soSwaggerMethodParserappliesUrlEscapers.PATH_ESCAPER. Anything a caller pre-encodes therefore gets escaped a second time — the%in%20becomes%25.The javadoc on
DataLakeFileSystemClient.getFileClient/getDirectoryClient/createDirectory*/deleteDirectory*(and async equivalents) and onDataLakePathClientBuilder.pathName(String)still said:That is the pre-12.22.0 contract and is now precisely backwards.
Changes
emoved the stale
Utility.urlEncode(...)from the four copy constructors:DataLakeFileClient.java,DataLakeDirectoryClient.java,DataLakeFileAsyncClient.javaandDataLakeDirectoryAsyncClient.java.Javadoc fix — 25 occurrences replaced with:
12 in
DataLakeFileSystemClient, 12 inDataLakeFileSystemAsyncClient, plusDataLakePathClientBuilder.pathName(String).Deliberately not changed:
DataLakePathClientBuilder.endpoint(String)javadoc, which says a path embedded in the endpoint URL must be url-encoded. That one is correct — endpoint parsing goes throughBlobUrlParts.getBlobName(), which callsUtility.urlDecode(BlobUrlParts.java:152). Only the separatepathName(String)setter, which stores verbatim, was stale.New tests —
DataLakePathNameEncodingTests.java(406 lines, 119 tests). Fully offline; uses a request-capturing mockHttpClientto assert what the SDK actually puts on the wire rather than what it's documented to do. Covers:%and#no longer throwIllegalArgumentException: Illegal hex characters in escape (%) patternA–Z a–z 0–9 - . _ ~ ! $ & ' ( ) * + , ; = : @, encoded set(space) " # % / < > ? [ ] ^ \{ | } ` and non-ASCII" \ / : | < > * ?)x-ms-rename-sourceusesUtility.urlEncodewhile the URL path usesUrlEscapers.PATH_ESCAPER— two different but individually valid encodingsrename/undeletePathaddresses the same URL from both the blob and dfs code paths, and agrees with a freshly acquired clientCHANGELOG.md — entries under
12.29.0-beta.2→ Bugs Fixed and Other Changes.