Skip to content
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.

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

Add "write" operation to HAConnection #55

Merged
merged 7 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
15 changes: 15 additions & 0 deletions Source/Internal/HAConnectionImpl.swift
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,19 @@ extension HAConnectionImpl {
}
}

private func sendWrite(_ sttBinaryHandlerId: UInt8, audioDataString: String?) {
// If there is no audioData, handlerID will be the payload alone indicating end of audio
var audioData = Data(base64Encoded: audioDataString ?? "") ?? Data()

// Prefix audioData with handler ID so the API can map the binary data
audioData.insert(sttBinaryHandlerId, at: 0)
workQueue.async { [connection] in
connection?.write(data: audioData) { [weak self] in
self?.responseController.didWrite()
}
}
}

func sendRaw(
identifier: HARequestIdentifier?,
request: HARequest
Expand All @@ -401,6 +414,8 @@ extension HAConnectionImpl {
sendWebSocket(identifier: identifier, request: request, command: command)
case let .rest(method, command):
sendRest(identifier: identifier!, request: request, method: method, command: command)
case let .sttData(data):
sendWrite(data.sttBinaryHandlerId, audioDataString: request.data["audioData"] as? String)
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this isn't invoking the completion blocks, but it may be useful to either wait to invoke it until the write occurs, or just invoke the completion right away. I know the server isn't ACK-ing this but it's still good form to be able to chain these together for throttling if you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Communicating to the response controller isn't enough? like done here:

 private func sendWrite(_ sttBinaryHandlerId: UInt8, audioDataString: String?) {
        // If there is no audioData, handlerID will be the payload alone indicating end of audio
        var audioData = Data(base64Encoded: audioDataString ?? "") ?? Data()

        // Prefix audioData with handler ID so the API can map the binary data
        audioData.insert(sttBinaryHandlerId, at: 0)
        workQueue.async { [connection] in
            connection?.write(data: audioData) { [weak self] in
                self?.responseController.didWrite()
            }
        }
    }

I noticed that when "sending" using websocket or rest, that's the only response, completion is not used for those either

Copy link
Member

@zacwest zacwest Mar 26, 2024

Choose a reason for hiding this comment

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

Not all of the tests check the invocation of the completion handler, but some do. e.g. testPlainSendSentSuccessful, testPlainSendSentFailurePromise, etc. You're making a new one with a unique code path, so you need to make sure all of the same guarantees happen somewhere.

The response controller only invokes based on identifiers - there's no identifier/callback for this write, so there's no way for it to know what to call back through.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, got it, ok I added the identifier and a test to validate it, I also added this code block inside "sendWrite":

 if let identifier, let request = self.requestController.single(for: identifier) {
                    callbackQueue.async {
                        request.resolve(.success(.empty))
                    }
                    requestController.clear(invocation: request)
                }

Because I can't add identifier to the data dictionary (since I am sending binary in this write) and it will also have no websocket event back from the connection.

Does that make sense?

}
}
}
Expand Down
5 changes: 4 additions & 1 deletion Source/Internal/RequestController/HARequestController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,17 @@ internal struct HARequestControllerAllowedSendKind: OptionSet {

static let webSocket: Self = .init(rawValue: 0b1)
static let rest: Self = .init(rawValue: 0b10)
static let all: Self = [.webSocket, .rest]
static let sttData: Self = .init(rawValue: 0b11)
static let all: Self = [.webSocket, .rest, .sttData]

func allows(requestType: HARequestType) -> Bool {
switch requestType {
case .webSocket:
return contains(.webSocket)
case .rest:
return contains(.rest)
case .sttData:
return contains(.sttData)
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions Source/Internal/ResponseController/HAResponseController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ internal protocol HAResponseController: AnyObject {
var phase: HAResponseControllerPhase { get }

func reset()
func didWrite()
func didReceive(event: Starscream.WebSocketEvent)
func didReceive(
for identifier: HARequestIdentifier,
Expand Down Expand Up @@ -187,4 +188,8 @@ internal class HAResponseControllerImpl: HAResponseController {
}
}
}

func didWrite() {
HAGlobal.log(.info, "Data written")
}
}
10 changes: 9 additions & 1 deletion Source/Requests/HARequestType.swift
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import Foundation

/// The command to issue
public enum HARequestType: Hashable, Comparable, ExpressibleByStringLiteral {
/// Sent over WebSocket, the command of the request
case webSocket(String)
/// Sent over REST, the HTTP method to use and the post-`api/` path
case rest(HAHTTPMethod, String)
/// Sent over WebSocket, the stt binary handler id
case sttData(HASttData)

/// Create a WebSocket request type by string literal
/// - Parameter value: The name of the WebSocket command
Expand All @@ -16,13 +20,15 @@ public enum HARequestType: Hashable, Comparable, ExpressibleByStringLiteral {
switch self {
case let .webSocket(command), let .rest(_, command):
return command
case .sttData:
return ""
}
}

/// The request is issued outside of the lifecycle of a connection
public var isPerpetual: Bool {
switch self {
case .webSocket: return false
case .webSocket, .sttData: return false
case .rest: return true
}
}
Expand All @@ -39,6 +45,8 @@ public enum HARequestType: Hashable, Comparable, ExpressibleByStringLiteral {
case let (.webSocket(lhsCommand), .webSocket(rhsCommand)),
let (.rest(_, lhsCommand), .rest(_, rhsCommand)):
return lhsCommand < rhsCommand
case (.sttData, _), (_, .sttData):
return false
}
}

Expand Down
8 changes: 8 additions & 0 deletions Source/Requests/HASttData.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/// Write audio data to websocket, sttBinaryHandlerId is provided by run-start in Assist pipeline
public struct HASttData: Hashable {
var sttBinaryHandlerId: UInt8

public init(sttBinaryHandlerId: UInt8) {
self.sttBinaryHandlerId = sttBinaryHandlerId
}
}
bgoncal marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions Tests/FakeEngine.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ internal class FakeEngine: Engine {

func write(data: Data, opcode: FrameOpCode, completion: (() -> Void)?) {
events.append(.writeData(data, opcode: opcode))
completion?()
}

func write(string: String, completion: (() -> Void)?) {
Expand Down
79 changes: 79 additions & 0 deletions Tests/HAConnectionImpl.test.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1461,6 +1461,81 @@ internal class HAConnectionImplTests: XCTestCase {
let container = connection.caches
XCTAssertEqual(ObjectIdentifier(container.connection), ObjectIdentifier(connection))
}

func testWriteDataRequestAddsToRequestController() {
connection.connectAutomatically = true
let expectedData = "Fake data".data(using: .utf8)!
let request = HARequest(
type: .sttData(.init(sttBinaryHandlerId: 1)),
data: [
"audioData": expectedData.base64EncodedString(),
]
)
connection.connect()
connection.send(request) { _ in }
bgoncal marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I mean this literal closure here. The completion block for send() - can you test it is invoked?

Copy link
Member Author

Choose a reason for hiding this comment

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

The original implementation of the "send" method didn't have any usage of the completion. Should this be altered?

XCTAssertNotNil(requestController.added.first(where: { invocation in
if case let .sttData(sttBinaryHandlerId) = invocation.request.type {
return sttBinaryHandlerId == .init(sttBinaryHandlerId: 1) && invocation.request
.data["audioData"] as? String == expectedData
.base64EncodedString()
}
return false
}))
}

func testWriteDataRequestsCallCompletion() {
let expectation = expectation(description: "Waiting for completion")
let expectedData = "Fake data".data(using: .utf8)!
let request = HARequest(
type: .sttData(.init(sttBinaryHandlerId: 1)),
data: [
"audioData": expectedData.base64EncodedString(),
]
)
connection.connect()
responseController.receivedWaitExpectation = expectation
connection.requestController(requestController, didPrepareRequest: request, with: .init(integerLiteral: 1))

wait(for: [expectation], timeout: 5.0)
}

func testSendRawWithSttRequest() {
connection.connect()
let expectedData = "Fake data".data(using: .utf8)!
let request = HARequest(
type: .sttData(.init(sttBinaryHandlerId: 1)),
data: [
"audioData": expectedData.base64EncodedString(),
]
)
var expectedAudioData = expectedData
expectedAudioData.insert(1, at: 0)

connection.sendRaw(identifier: nil, request: request)
waitForWorkQueue()
XCTAssertNotNil(engine.events.first { event in
event == .writeData(expectedAudioData, opcode: .binaryFrame)
})
}

func testWriteSttRequestCommand() {
let expectedData = "Fake data".data(using: .utf8)!
let request = HARequest(
type: .sttData(.init(sttBinaryHandlerId: 1)),
data: [
"audioData": expectedData.base64EncodedString(),
]
)
let request2 = HARequest(
type: .sttData(.init(sttBinaryHandlerId: 1)),
data: [
"audioData": expectedData.base64EncodedString(),
]
)

XCTAssertEqual(request.type.command, "")
XCTAssertEqual(request.type < request2.type, false)
}
}

extension WebSocketEvent: Equatable {
Expand Down Expand Up @@ -1614,6 +1689,10 @@ private class FakeHAResponseController: HAResponseController {
receivedWaitExpectation?.fulfill()
}

func didWrite() {
receivedWaitExpectation?.fulfill()
}

var receivedRestWaitExpectation: XCTestExpectation?
var receivedRest: [Swift.Result<(HTTPURLResponse, Data?), Error>] = []
func didReceive(for identifier: HARequestIdentifier, response: Swift.Result<(HTTPURLResponse, Data?), Error>) {
Expand Down
6 changes: 6 additions & 0 deletions Tests/HARequestController.test.swift
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,12 @@ internal class HARequestControllerTests: XCTestCase {
XCTAssertEqual(types, Set(["test3", "test4"]))
}

func testAddingWriteRequestAllowed() {
delegate.allowedSendKinds = .all
controller.add(.init(request: .init(type: .sttData(.init(sttBinaryHandlerId: 1)))))
XCTAssertEqual(delegate.didPrepare.count, 1)
}

func testCancelSingleBeforeSent() {
let invocation = HARequestInvocationSingle(
request: .init(type: "test1", data: [:]),
Expand Down
12 changes: 12 additions & 0 deletions Tests/HAResponseController.test.swift
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,18 @@ internal class HAResponseControllerTests: XCTestCase {
waitForCallback()
XCTAssertEqual(delegate.lastReceived, .result(identifier: 2, result: .success(.dictionary(resultDictionary))))
}

func testDidWriteEventLogs() {
let expectation = expectation(description: "Receive log")
HAGlobal.log = { level, message in
XCTAssertEqual(level, .info)
XCTAssertEqual(message, "Data written")
HAGlobal.log = { _, _ in }
expectation.fulfill()
}
controller.didWrite()
wait(for: [expectation], timeout: 2)
}
}

private extension HAResponseControllerTests {
Expand Down