Handle URL changes in latest Apple Platforms

[cham-s]

The latest Apple platforms automatically escape invalid URL characters - https://developer.apple.com/documentation/foundation/url/3126806-init

This aligns the behaviour across platform versions and Linux. Also updates a number of warnings in the tests

[0xTim]

Thanks!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.55%. Comparing base (a935b63) to head (d145fc2).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
- Coverage   82.85%   82.55%   -0.30%     
==========================================
  Files           6        6              
  Lines         659      642      -17     
==========================================
- Hits          546      530      -16     
+ Misses        113      112       -1     

Files	Coverage Δ
Sources/WebSocketKit/WebSocket+Connect.swift	98.86% <100.00%> (+0.27%)	⬆️

... and 5 files with indirect coverage changes

[0xTim]

@cham-s just taking a look at this to see why CI is failing - it's failing to build on latest macOS for me because ossl_ssize_t differs in size. I also have some test failures once that's fixed and a couple more deprecations in the async tests if you want to take a look

[cham-s]

@0xTim
I ran the tests indeed they fail with a message similar to the one shown inside the CI logs.
However the failures were already there prior to the PR.

websocket-kit/Tests/WebSocketKitTests/WebSocketKitTests.swift

Lines 481 to 487 in 4232d34

	func testBadURLInWebsocketConnect() async throws {
	XCTAssertThrowsError(try WebSocket.connect(to: "%w", on: self.elg, onUpgrade: { _ in }).wait()) {
	guard case .invalidURL = $0 as? WebSocketClient.Error else {
	return XCTFail("Expected .invalidURL but got \(String(reflecting: $0))")
	}
	}
	}

websocket-kit/Tests/WebSocketKitTests/AsyncWebSocketKitTests.swift

Lines 49 to 60 in 4232d34

	func testBadURLInWebsocketConnect() async throws {
	do {
	try await WebSocket.connect(to: "%w", on: self.elg, onUpgrade: { _ async in })
	XCTAssertThrowsError({}())
	} catch {
	XCTAssertThrowsError(try { throw error }()) {
	guard case .invalidURL = $0 as? WebSocketClient.Error else {
	return XCTFail("Expected .invalidURL but got \(String(reflecting: $0))")
	}
	}
	}
	}

The tests expect "%w" to fail because it's not a valid URI.
But it doesn't fail because the logic inside the code relies on URL(string: String) initializer to perform the validation
URL(string: String) has no notion of WebSocket so it only performs a general check on the URL structure.

websocket-kit/Sources/WebSocketKit/WebSocket+Connect.swift

Lines 16 to 33 in 4232d34

	public static func connect(
	to url: String,
	headers: HTTPHeaders = [:],
	configuration: WebSocketClient.Configuration = .init(),
	on eventLoopGroup: EventLoopGroup,
	onUpgrade: @Sendable @escaping (WebSocket) -> ()
	) -> EventLoopFuture<Void> {
	guard let url = URL(string: url) else {
	return eventLoopGroup.any().makeFailedFuture(WebSocketClient.Error.invalidURL)
	}
	return self.connect(
	to: url,
	headers: headers,
	configuration: configuration,
	on: eventLoopGroup,
	onUpgrade: onUpgrade
	)
	}

I verified it with a repl session:

swift repl

Welcome to Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4).
Type :help for assistance.
  1> import Foundation
  2>  
  3> let strangeWebSocketString = "%w"
strangeWebSocketString: String = "%w"
  4> let url = URL(string: strangeWebSocketString)
url: Foundation.URL? = "%25w"
  5> url == nil
$R0: Bool = false

As a fix I allowed myself to try to strengthen the guard statement with a check for string with an explicit prefix of ws:// or wss://.

    public static func connect(
        to url: String,
        headers: HTTPHeaders = [:],
        configuration: WebSocketClient.Configuration = .init(),
        on eventLoopGroup: EventLoopGroup,
        onUpgrade: @Sendable @escaping (WebSocket) -> ()
    ) -> EventLoopFuture<Void> {
        guard
            url.hasPrefix("ws://") || url.hasPrefix("wss://"),
            let url = URL(string: url)
        else {
            return eventLoopGroup.any().makeFailedFuture(WebSocketClient.Error.invalidURL)
        }
        return self.connect(
            to: url,
            headers: headers,
            configuration: configuration,
            on: eventLoopGroup,
            onUpgrade: onUpgrade
        )
    }

I don't know if this is the best solution but it makes sure the caller first, inputs a string with a correct WebSocket scheme then let URL(string: String) perform additional checks.

[0xTim]

Ok this is a change in behaviour in Swift - see https://developer.apple.com/documentation/foundation/url/3126806-init for an explanation

So I think the answer to this is use the new API if available https://developer.apple.com/documentation/foundation/url/4191020-init otherwise default to the old one. Another option if find a URL that still fails and update the test

[cham-s]

@0xTim thanks for the links, I wasn't aware of this change. The code has been updated accordingly.

[0xTim]

Original description:

This is not much but after building the package a couple of warnings and errors appeared the following changes made them disappear.

• Update the SSLTestHelpers.swift file with the updated version of NIOSSLTestHelpers.swift from the swift-nio-ssl package Tests directory.

• Remove yellow warnings caused by deprecation of invoking the onPong closure with a single argument by using underscore to ignore the data argument.

// Before
ws.onPong {
    $0.close(promise: closePromise)
    promise.succeed()
}

// After
ws.onPong { webSocket, _ in
    webSocket.close(promise: closePromise)
    promise.succeed()
}

Update SSLTestHelpers, remove deprecation warnings

[0xTim]

@cham-s I think we can remove the visionOS check - I don't mind the change of behaviour on that platform!

[cham-s]

Ok, done

[cham-s]

@0xTim tests are passing except for test / unit-tests / macos-unit (macos-13, ~14.3) .
I'm not sure what macos-13, ~14.3 means, I tried to guess and lowered the minimum version to macOS 14.3 but it still fails.

In any case it says extra argument 'encodingInvalidCharacters' meaning it passes the if statement

if #available(iOS 17.0, macOS 14.3, tvOS 17.0, watchOS 10.0, *) {
    optionalURL = URL(string: url, encodingInvalidCharacters: false)
} 

but doesn't recognise the API.

[gwynne]

macos-13, ~14.3 means latest macOS 13 (Ventura) with Xcode 14.3 and its associated toolchains. A #available for macOS 14 should not be passing in that environment.

[gwynne]

In any event, these URL changes are not correct; the result will be (even more) inconsistent behavior between platforms and platform versions, which is the opposite of what we want.

I withdraw this comment, it looks like I was confused as to what was actually happening here. Apologies for the noise!

[cham-s]

@gwynne #148 (comment) thanks for the info.
In this case it's strange that it passes the if #available and doesn't resort to the else branch to handle the url using the macOS 13 API.