Merge pull request #82 from ably-labs/12-associated-value-RoomLifecycle

Move error inside `RoomLifecycle`

Example/AblyChatExample/Mocks/MockClients.swift

@@ -412,7 +412,7 @@ actor MockRoomStatus: RoomStatus {
 
     private func createSubscription() -> MockSubscription<RoomStatusChange> {
         let subscription = MockSubscription<RoomStatusChange>(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

Sources/AblyChat/RoomLifecycleManager.swift

@@ -48,7 +48,6 @@ internal actor RoomLifecycleManager<Contributor: RoomLifecycleContributor> {
     }
 
     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<Contributor: RoomLifecycleContributor> {
                     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<Contributor: RoomLifecycleContributor> {
                     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<Contributor: RoomLifecycleContributor> {
         #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<Contributor: RoomLifecycleContributor> {
                 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<Contributor: RoomLifecycleContributor> {
                     }
 
                     // 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

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<RoomStatusChange>
 }
 
-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
     }
 }
 

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
+        }
+    }
+}

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<RoomStatusChange>, Subscription<RoomStatusChange>.StatusChangeWithError> {
+        compactMap { statusChange in
+            if case let .suspended(error) = statusChange.current {
+                StatusChangeWithError(statusChange: statusChange, error: error)
+            } else {
+                nil
+            }
+        }
+    }
+
+    func failedElements() async -> AsyncCompactMapSequence<Subscription<RoomStatusChange>, Subscription<RoomStatusChange>.StatusChangeWithError> {
+        compactMap { statusChange in
+            if case let .failed(error) = statusChange.current {
+                StatusChangeWithError(statusChange: statusChange, error: error)
+            } else {
+                nil
+            }
+        }
+    }
+}

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,24 +943,24 @@ 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
         )
 
         await contributor.channel.emitStateChange(contributorStateChange)
 
         // 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))
     }
 }