Open cham-s opened 7 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 82.55%. Comparing base (
a935b63
) to head (d145fc2
). Report is 3 commits behind head on main.
@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
@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.
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.
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.
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
@0xTim thanks for the links, I wasn't aware of this change. The code has been updated accordingly.
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() }
<!-- When this PR is merged, the title and body will be -->
Update SSLTestHelpers, remove deprecation warnings
<!-- used to generate a release automatically. -->
@cham-s I think we can remove the visionOS check - I don't mind the change of behaviour on that platform!
Ok, done
@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.
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.
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!
@gwynne https://github.com/vapor/websocket-kit/pull/148#issuecomment-2073286704 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.
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