From d51887c67fd3c718e1474bc05aeeb947b5c431c9 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 9 Oct 2024 17:25:16 -0300 Subject: [PATCH] Move error inside RoomLifecycle Lifecycle statuses have an error if and only if they have certain cases, so encode this constraint in the type system to avoid unnecessary unwraps. Part of #12. --- .../AblyChatExample/Mocks/MockClients.swift | 2 +- Sources/AblyChat/RoomLifecycleManager.swift | 22 +++++----- Sources/AblyChat/RoomStatus.swift | 34 +++++++++++---- .../Helpers/RoomLifecycle+Error.swift | 21 ++++++++++ .../Subscription+RoomStatusChange.swift | 34 +++++++++++++++ .../RoomLifecycleManagerTests.swift | 41 +++++++++---------- 6 files changed, 110 insertions(+), 44 deletions(-) create mode 100644 Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift create mode 100644 Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift diff --git a/Example/AblyChatExample/Mocks/MockClients.swift b/Example/AblyChatExample/Mocks/MockClients.swift index 3e8d1bc8..14a31d71 100644 --- a/Example/AblyChatExample/Mocks/MockClients.swift +++ b/Example/AblyChatExample/Mocks/MockClients.swift @@ -412,7 +412,7 @@ actor MockRoomStatus: RoomStatus { private func createSubscription() -> MockSubscription { let subscription = MockSubscription(randomElement: { - RoomStatusChange(current: [.attached, .attached, .attached, .attached, .attaching, .attaching, .suspended].randomElement()!, previous: .attaching) + RoomStatusChange(current: [.attached, .attached, .attached, .attached, .attaching, .attaching, .suspended(error: .createUnknownError())].randomElement()!, previous: .attaching) }, interval: 8) mockSubscriptions.append(subscription) return subscription diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 42041609..dbbf34ee 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -48,7 +48,6 @@ internal actor RoomLifecycleManager { } internal private(set) var current: RoomLifecycle - internal private(set) var error: ARTErrorInfo? // TODO: This currently allows the the tests to inject a value in order to test the spec points that are predicated on whether “a channel lifecycle operation is in progress”. In https://github.com/ably-labs/ably-chat-swift/issues/52 we’ll set this property based on whether there actually is a lifecycle operation in progress. private let hasOperationInProgress: Bool /// Manager state that relates to individual contributors, keyed by contributors’ ``Contributor.id``. Stored separately from ``contributors`` so that the latter can be a `let`, to make it clear that the contributors remain fixed for the lifetime of the manager. @@ -263,7 +262,7 @@ internal actor RoomLifecycleManager { preconditionFailure("FAILED state change event should have a reason") } - changeStatus(to: .failed, error: reason) + changeStatus(to: .failed(error: reason)) // TODO: CHA-RL4b5 is a bit unclear about how to handle failure, and whether they can be detached concurrently (asked in https://github.com/ably/specification/pull/200/files#r1777471810) for contributor in contributors { @@ -282,7 +281,7 @@ internal actor RoomLifecycleManager { preconditionFailure("SUSPENDED state change event should have a reason") } - changeStatus(to: .suspended, error: reason) + changeStatus(to: .suspended(error: reason)) } default: break @@ -295,13 +294,12 @@ internal actor RoomLifecycleManager { #endif } - /// Updates ``current`` and ``error`` and emits a status change event. - private func changeStatus(to new: RoomLifecycle, error: ARTErrorInfo? = nil) { - logger.log(message: "Transitioning from \(current) to \(new), error \(String(describing: error))", level: .info) + /// Updates ``current`` and emits a status change event. + private func changeStatus(to new: RoomLifecycle) { + logger.log(message: "Transitioning from \(current) to \(new)", level: .info) let previous = current current = new - self.error = error - let statusChange = RoomStatusChange(current: current, previous: previous, error: error) + let statusChange = RoomStatusChange(current: current, previous: previous) emitStatusChange(statusChange) } @@ -343,14 +341,14 @@ internal actor RoomLifecycleManager { case .suspended: // CHA-RL1h2 let error = ARTErrorInfo(chatError: .attachmentFailed(feature: contributor.feature, underlyingError: contributorAttachError)) - changeStatus(to: .suspended, error: error) + changeStatus(to: .suspended(error: error)) // CHA-RL1h3 throw error case .failed: // CHA-RL1h4 let error = ARTErrorInfo(chatError: .attachmentFailed(feature: contributor.feature, underlyingError: contributorAttachError)) - changeStatus(to: .failed, error: error) + changeStatus(to: .failed(error: error)) // CHA-RL1h5 // TODO: Implement the "asynchronously with respect to CHA-RL1h4" part of CHA-RL1h5 (https://github.com/ably-labs/ably-chat-swift/issues/50) @@ -450,8 +448,8 @@ internal actor RoomLifecycleManager { } // This check is CHA-RL2h2 - if current != .failed { - changeStatus(to: .failed, error: error) + if !current.isFailed { + changeStatus(to: .failed(error: error)) } default: // CHA-RL2h3: Retry until detach succeeds, with a pause before each attempt diff --git a/Sources/AblyChat/RoomStatus.swift b/Sources/AblyChat/RoomStatus.swift index b94488ae..d152b544 100644 --- a/Sources/AblyChat/RoomStatus.swift +++ b/Sources/AblyChat/RoomStatus.swift @@ -2,33 +2,49 @@ import Ably public protocol RoomStatus: AnyObject, Sendable { var current: RoomLifecycle { get async } - // TODO: (https://github.com/ably-labs/ably-chat-swift/issues/12): consider how to avoid the need for an unwrap - var error: ARTErrorInfo? { get async } func onChange(bufferingPolicy: BufferingPolicy) async -> Subscription } -public enum RoomLifecycle: Sendable { +public enum RoomLifecycle: Sendable, Equatable { case initialized case attaching case attached case detaching case detached - case suspended - case failed + case suspended(error: ARTErrorInfo) + case failed(error: ARTErrorInfo) case releasing case released + + // Helpers to allow us to test whether a `RoomLifecycle` value has a certain case, without caring about the associated value. These are useful for in contexts where we want to use a `Bool` to communicate a case. For example: + // + // 1. testing (e.g. `#expect(status.isFailed)`) + // 2. testing that a status does _not_ have a particular case (e.g. if !status.isFailed), which a `case` statement cannot succinctly express + + public var isSuspended: Bool { + if case .suspended = self { + true + } else { + false + } + } + + public var isFailed: Bool { + if case .failed = self { + true + } else { + false + } + } } public struct RoomStatusChange: Sendable { public var current: RoomLifecycle public var previous: RoomLifecycle - // TODO: (https://github.com/ably-labs/ably-chat-swift/issues/12): consider how to avoid the need for an unwrap - public var error: ARTErrorInfo? - public init(current: RoomLifecycle, previous: RoomLifecycle, error: ARTErrorInfo? = nil) { + public init(current: RoomLifecycle, previous: RoomLifecycle) { self.current = current self.previous = previous - self.error = error } } diff --git a/Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift b/Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift new file mode 100644 index 00000000..f2f33df2 --- /dev/null +++ b/Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift @@ -0,0 +1,21 @@ +import Ably +import AblyChat + +extension RoomLifecycle { + var error: ARTErrorInfo? { + switch self { + case let .failed(error): + error + case let .suspended(error): + error + case .initialized, + .attached, + .attaching, + .detached, + .detaching, + .releasing, + .released: + nil + } + } +} diff --git a/Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift b/Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift new file mode 100644 index 00000000..2bc30bb6 --- /dev/null +++ b/Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift @@ -0,0 +1,34 @@ +import Ably +import AblyChat + +/// Extensions for filtering a subscription by a given case, and then providing access to the values associated with these cases. +/// +/// This provides better ergonomics than writing e.g. `failedStatusChange = await subscription.first { $0.isSuspended }`, because it means that you don’t have to write another `if case` (or equivalent) to get access to the associated value of `failedStatusChange.current`. +extension Subscription where Element == RoomStatusChange { + struct StatusChangeWithError { + /// A status change whose `current` has an associated error; ``error`` provides access to this error. + var statusChange: RoomStatusChange + /// The error associated with `statusChange.current`. + var error: ARTErrorInfo + } + + func suspendedElements() async -> AsyncCompactMapSequence, Subscription.StatusChangeWithError> { + compactMap { statusChange in + if case let .suspended(error) = statusChange.current { + StatusChangeWithError(statusChange: statusChange, error: error) + } else { + nil + } + } + } + + func failedElements() async -> AsyncCompactMapSequence, Subscription.StatusChangeWithError> { + compactMap { statusChange in + if case let .failed(error) = statusChange.current { + StatusChangeWithError(statusChange: statusChange, error: error) + } else { + nil + } + } + } +} diff --git a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift index 14804d27..2cb0558b 100644 --- a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift @@ -79,13 +79,6 @@ struct RoomLifecycleManagerTests { #expect(await manager.current == .initialized) } - @Test - func error_startsAsNil() async { - let manager = await createManager() - - #expect(await manager.error == nil) - } - // MARK: - ATTACH operation // @spec CHA-RL1a @@ -229,7 +222,7 @@ struct RoomLifecycleManagerTests { let manager = await createManager(contributors: contributors) let statusChangeSubscription = await manager.onChange(bufferingPolicy: .unbounded) - async let maybeSuspendedStatusChange = statusChangeSubscription.first { $0.current == .suspended } + async let maybeSuspendedStatusChange = statusChangeSubscription.suspendedElements().first { _ in true } // When: `performAttachOperation()` is called on the lifecycle manager async let roomAttachResult: Void = manager.performAttachOperation() @@ -241,7 +234,7 @@ struct RoomLifecycleManagerTests { // 3. the room attach operation fails with this same error let suspendedStatusChange = try #require(await maybeSuspendedStatusChange) - #expect(await manager.current == .suspended) + #expect(await manager.current.isSuspended) var roomAttachError: Error? do { @@ -250,7 +243,7 @@ struct RoomLifecycleManagerTests { roomAttachError = error } - for error in await [suspendedStatusChange.error, manager.error, roomAttachError] { + for error in await [suspendedStatusChange.error, manager.current.error, roomAttachError] { #expect(isChatError(error, withCode: .messagesAttachmentFailed, cause: contributorAttachError)) } } @@ -280,7 +273,7 @@ struct RoomLifecycleManagerTests { let manager = await createManager(contributors: contributors) let statusChangeSubscription = await manager.onChange(bufferingPolicy: .unbounded) - async let maybeFailedStatusChange = statusChangeSubscription.first { $0.current == .failed } + async let maybeFailedStatusChange = statusChangeSubscription.failedElements().first { _ in true } // When: `performAttachOperation()` is called on the lifecycle manager async let roomAttachResult: Void = manager.performAttachOperation() @@ -291,7 +284,7 @@ struct RoomLifecycleManagerTests { // 3. the room attach operation fails with this same error let failedStatusChange = try #require(await maybeFailedStatusChange) - #expect(await manager.current == .failed) + #expect(await manager.current.isFailed) var roomAttachError: Error? do { @@ -300,7 +293,7 @@ struct RoomLifecycleManagerTests { roomAttachError = error } - for error in await [failedStatusChange.error, manager.error, roomAttachError] { + for error in await [failedStatusChange.error, manager.current.error, roomAttachError] { #expect(isChatError(error, withCode: .messagesAttachmentFailed, cause: contributorAttachError)) } } @@ -429,7 +422,11 @@ struct RoomLifecycleManagerTests { @Test func detach_whenFailed() async throws { // Given: A RoomLifecycleManager in the FAILED state - let manager = await createManager(forTestingWhatHappensWhenCurrentlyIn: .failed) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .failed( + error: .createUnknownError() /* arbitrary */ + ) + ) // When: `performAttachOperation()` is called on the lifecycle manager // Then: It throws a roomInFailedState error @@ -507,7 +504,7 @@ struct RoomLifecycleManagerTests { let manager = await createManager(contributors: contributors) let statusChangeSubscription = await manager.onChange(bufferingPolicy: .unbounded) - async let maybeFailedStatusChange = statusChangeSubscription.first { $0.current == .failed } + async let maybeFailedStatusChange = statusChangeSubscription.failedElements().first { _ in true } // When: `performDetachOperation()` is called on the lifecycle manager let maybeRoomDetachError: Error? @@ -845,7 +842,7 @@ struct RoomLifecycleManagerTests { let manager = await createManager(forTestingWhatHappensWhenHasOperationInProgress: false, contributors: contributors) let roomStatusSubscription = await manager.onChange(bufferingPolicy: .unbounded) - async let failedStatusChange = roomStatusSubscription.first { $0.current == .failed } + async let failedStatusChange = roomStatusSubscription.failedElements().first { _ in true } // When: A contributor emits an FAILED event let contributorStateChange = ARTChannelStateChange( @@ -864,7 +861,7 @@ struct RoomLifecycleManagerTests { // - the room status transitions to failed, with the error of the status change being the `reason` of the contributor FAILED event // - and it calls `detach` on all contributors _ = try #require(await failedStatusChange) - #expect(await manager.current == .failed) + #expect(await manager.current.isFailed) for contributor in contributors { #expect(await contributor.channel.detachCallCount == 1) @@ -946,14 +943,15 @@ struct RoomLifecycleManagerTests { let manager = await createManager(forTestingWhatHappensWhenHasOperationInProgress: false, contributors: [contributor]) let roomStatusSubscription = await manager.onChange(bufferingPolicy: .unbounded) - async let maybeSuspendedRoomStatusChange = roomStatusSubscription.first { $0.current == .suspended } + async let maybeSuspendedRoomStatusChange = roomStatusSubscription.suspendedElements().first { _ in true } // When: A contributor emits a state change to SUSPENDED + let contributorStateChangeReason = ARTErrorInfo(domain: "SomeDomain", code: 123) // arbitrary let contributorStateChange = ARTChannelStateChange( current: .suspended, previous: .attached, // arbitrary event: .suspended, - reason: ARTErrorInfo(domain: "SomeDomain", code: 123), // arbitrary + reason: contributorStateChangeReason, resumed: false // arbitrary ) @@ -961,9 +959,8 @@ struct RoomLifecycleManagerTests { // Then: The room transitions to SUSPENDED, and this state change has error equal to the contributor state change’s `reason` let suspendedRoomStatusChange = try #require(await maybeSuspendedRoomStatusChange) - #expect(suspendedRoomStatusChange.error === contributorStateChange.reason) + #expect(suspendedRoomStatusChange.error === contributorStateChangeReason) - #expect(await manager.current == .suspended) - #expect(await manager.error === contributorStateChange.reason) + #expect(await manager.current == .suspended(error: contributorStateChangeReason)) } }