Skip to content

URL.path should not strip trailing slash for root paths on Windows #1038

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

Merged
merged 2 commits into from
Nov 11, 2024
Merged

URL.path should not strip trailing slash for root paths on Windows #1038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6cf304c
(139379934) URL should not strip trailing slash for root paths on Win…
jrflat Nov 7, 2024
9d325ee
Use PathIsRootW to ensure we don't strip a root slash
jrflat Nov 9, 2024
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
52 changes: 34 additions & 18 deletions Sources/FoundationEssentials/URL/URL.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
public struct URLResourceKey {}
#endif

#if os(Windows)
import WinSDK
#endif

#if FOUNDATION_FRAMEWORK
internal import _ForSwiftFoundation
internal import CoreFoundation_Private.CFURL
Expand Down Expand Up @@ -1349,31 +1353,43 @@ public struct URL: Equatable, Sendable, Hashable {
}
}

private static func windowsPath(for posixPath: String) -> String {
let utf8 = posixPath.utf8
guard utf8.count >= 4 else {
return posixPath
#if os(Windows)
private static func windowsPath(for urlPath: String) -> String {
var iter = urlPath.utf8.makeIterator()
guard iter.next() == ._slash else {
return decodeFilePath(urlPath._droppingTrailingSlashes)
}
// "C:\" is standardized to "/C:/" on initialization.
if let driveLetter = iter.next(), driveLetter.isAlpha,
iter.next() == ._colon,
iter.next() == ._slash {
// Strip trailing slashes from the path, which preserves a root "/".
let path = String(Substring(urlPath.utf8.dropFirst(3)))._droppingTrailingSlashes
// Don't include a leading slash before the drive letter
return "\(Unicode.Scalar(driveLetter)):\(decodeFilePath(path))"
}
// "C:\" is standardized to "/C:/" on initialization
let array = Array(utf8)
if array[0] == ._slash,
array[1].isAlpha,
array[2] == ._colon,
array[3] == ._slash {
return String(Substring(utf8.dropFirst()))
// There are many flavors of UNC paths, so use PathIsRootW to ensure
// we don't strip a trailing slash that represents a root.
let path = decodeFilePath(urlPath)
return path.replacing(._slash, with: ._backslash).withCString(encodedAs: UTF16.self) { pwszPath in
guard !PathIsRootW(pwszPath) else {
return path
Copy link
Member

Choose a reason for hiding this comment

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

I think that we still get one path incorrect now: /S:. This a non-UNC, drive-relative path. We could either resolve that to an absolute path or return it as S:. You can also have a more complex path such as S:utils\build.ps1 (which I believe would be encoded as /s:utils/build.ps1.

Copy link
Contributor Author

@jrflat jrflat Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, more work definitely needs to be done to support drive-relative paths in URL. Currently, S:utils\build.ps1 is actually treated as a relative path, relative to the current working directory (but not necessarily the cwd of S:), so .path won't resolve it correctly.

I think we should get the cwd of S: during URL(filePath:) initialization and use that cwd path as the base URL, then strip S: from the relative path. If that sounds good I can open up a follow-on PR, since it'll be slightly more involved than this change, which fixes C:\ and other absolute paths' behaviors.

Copy link
Member

Choose a reason for hiding this comment

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

I think that we should do some checking if the call needs to be done, but otherwise, this sounds like a good way forward. Doing that in a follow up is totally fine.

}
return path._droppingTrailingSlashes
}
return posixPath
}
#endif

private static func fileSystemPath(for urlPath: String) -> String {
private static func decodeFilePath(_ path: some StringProtocol) -> String {
let charsToLeaveEncoded: Set<UInt8> = [._slash, 0]
guard let posixPath = Parser.percentDecode(urlPath._droppingTrailingSlashes, excluding: charsToLeaveEncoded) else {
return ""
}
return Parser.percentDecode(path, excluding: charsToLeaveEncoded) ?? ""
}

private static func fileSystemPath(for urlPath: String) -> String {
#if os(Windows)
return windowsPath(for: posixPath)
return windowsPath(for: urlPath)
#else
return posixPath
return decodeFilePath(urlPath._droppingTrailingSlashes)
#endif
}

Expand Down
39 changes: 38 additions & 1 deletion Tests/FoundationEssentialsTests/URLTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,13 +340,50 @@ final class URLTests : XCTestCase {

#if os(Windows)
func testURLWindowsDriveLetterPath() throws {
let url = URL(filePath: "C:\\test\\path", directoryHint: .notDirectory)
var url = URL(filePath: #"C:\test\path"#, directoryHint: .notDirectory)
// .absoluteString and .path() use the RFC 8089 URL path
XCTAssertEqual(url.absoluteString, "file:///C:/test/path")
XCTAssertEqual(url.path(), "/C:/test/path")
// .path and .fileSystemPath strip the leading slash
XCTAssertEqual(url.path, "C:/test/path")
XCTAssertEqual(url.fileSystemPath, "C:/test/path")

url = URL(filePath: #"C:\"#, directoryHint: .isDirectory)
XCTAssertEqual(url.absoluteString, "file:///C:/")
XCTAssertEqual(url.path(), "/C:/")
XCTAssertEqual(url.path, "C:/")
XCTAssertEqual(url.fileSystemPath, "C:/")

url = URL(filePath: #"C:\\\"#, directoryHint: .isDirectory)
XCTAssertEqual(url.absoluteString, "file:///C:///")
XCTAssertEqual(url.path(), "/C:///")
XCTAssertEqual(url.path, "C:/")
XCTAssertEqual(url.fileSystemPath, "C:/")

url = URL(filePath: #"\C:\"#, directoryHint: .isDirectory)
XCTAssertEqual(url.absoluteString, "file:///C:/")
XCTAssertEqual(url.path(), "/C:/")
XCTAssertEqual(url.path, "C:/")
XCTAssertEqual(url.fileSystemPath, "C:/")

let base = URL(filePath: #"\d:\path\"#, directoryHint: .isDirectory)
url = URL(filePath: #"%43:\fake\letter"#, directoryHint: .notDirectory, relativeTo: base)
// ":" is encoded to "%3A" in the first path segment so it's not mistaken as the scheme separator
XCTAssertEqual(url.relativeString, "%2543%3A/fake/letter")
XCTAssertEqual(url.path(), "/d:/path/%2543%3A/fake/letter")
XCTAssertEqual(url.path, "d:/path/%43:/fake/letter")
XCTAssertEqual(url.fileSystemPath, "d:/path/%43:/fake/letter")

let cwd = URL.currentDirectory()
var iter = cwd.path().utf8.makeIterator()
if iter.next() == ._slash,
let driveLetter = iter.next(), driveLetter.isLetter!,
iter.next() == ._colon {
let path = #"\\?\"# + "\(Unicode.Scalar(driveLetter))" + #":\"#
url = URL(filePath: path, directoryHint: .isDirectory)
XCTAssertEqual(url.path.last, "/")
XCTAssertEqual(url.fileSystemPath.last, "/")
}
}
#endif

Expand Down