Implement CHA-PR3h and similar

Resolves #151.

Author: lawrence-forooghian

Sources/AblyChat/Errors.swift

@@ -11,8 +11,6 @@ public let errorDomain = "AblyChatErrorDomain"
  The error codes for errors in the ``errorDomain`` error domain.
  */
 public enum ErrorCode: Int {
-    case nonspecific = 40000
-
     /// ``Rooms.get(roomID:options:)`` was called with a different set of room options than was used on a previous call. You must first release the existing room instance using ``Rooms.release(roomID:)``.
     ///
     /// TODO this code is a guess, revisit in https://github.com/ably-labs/ably-chat-swift/issues/32
@@ -38,7 +36,6 @@ public enum ErrorCode: Int {
 
     /// Has a case for each of the ``ErrorCode`` cases that imply a fixed status code.
     internal enum CaseThatImpliesFixedStatusCode {
-        case nonspecific
         case inconsistentRoomOptions
         case messagesAttachmentFailed
         case presenceAttachmentFailed
@@ -53,12 +50,9 @@ public enum ErrorCode: Int {
         case roomInFailedState
         case roomIsReleasing
         case roomIsReleased
-        case roomInInvalidState
 
         internal var toNumericErrorCode: ErrorCode {
             switch self {
-            case .nonspecific:
-                .nonspecific
             case .inconsistentRoomOptions:
                 .inconsistentRoomOptions
             case .messagesAttachmentFailed:
@@ -87,17 +81,14 @@ public enum ErrorCode: Int {
                 .roomIsReleasing
             case .roomIsReleased:
                 .roomIsReleased
-            case .roomInInvalidState:
-                .roomInInvalidState
             }
         }
 
         /// The ``ARTErrorInfo.statusCode`` that should be returned for this error.
         internal var statusCode: Int {
             // These status codes are taken from the "Chat-specific Error Codes" section of the spec.
             switch self {
-            case .nonspecific,
-                 .inconsistentRoomOptions,
+            case .inconsistentRoomOptions,
                  .roomInFailedState,
                  .roomIsReleasing,
                  .roomIsReleased:
@@ -112,18 +103,21 @@ public enum ErrorCode: Int {
                 .presenceDetachmentFailed,
                 .reactionsDetachmentFailed,
                 .occupancyDetachmentFailed,
-                .typingDetachmentFailed,
-                // CHA-RL9c
-                .roomInInvalidState:
+                .typingDetachmentFailed:
                 500
             }
         }
     }
 
     /// Has a case for each of the ``ErrorCode`` cases that do not imply a fixed status code.
     internal enum CaseThatImpliesVariableStatusCode {
+        case roomInInvalidState
+
         internal var toNumericErrorCode: ErrorCode {
-            switch self {}
+            switch self {
+            case .roomInInvalidState:
+                .roomInInvalidState
+            }
         }
     }
 }
@@ -169,8 +163,7 @@ internal enum ChatError {
     case roomIsReleasing
     case roomIsReleased
     case presenceOperationRequiresRoomAttach(feature: RoomFeature)
-    case presenceOperationDisallowedForCurrentRoomStatus(feature: RoomFeature)
-    case roomInInvalidState(cause: ARTErrorInfo?)
+    case roomTransitionedToInvalidStateForPresenceOperation(cause: ARTErrorInfo?)
 
     internal var codeAndStatusCode: ErrorCodeAndStatusCode {
         switch self {
@@ -208,11 +201,12 @@ internal enum ChatError {
             .fixedStatusCode(.roomIsReleasing)
         case .roomIsReleased:
             .fixedStatusCode(.roomIsReleased)
-        case .roomInInvalidState:
-            .fixedStatusCode(.roomInInvalidState)
-        case .presenceOperationRequiresRoomAttach,
-             .presenceOperationDisallowedForCurrentRoomStatus:
-            .fixedStatusCode(.nonspecific)
+        case .roomTransitionedToInvalidStateForPresenceOperation:
+            // CHA-RL9c
+            .variableStatusCode(.roomInInvalidState, statusCode: 500)
+        case .presenceOperationRequiresRoomAttach:
+            // CHA-PR3h, CHA-PR10h, CHA-PR6h, CHA-T2g
+            .variableStatusCode(.roomInInvalidState, statusCode: 400)
         }
     }
 
@@ -268,9 +262,7 @@ internal enum ChatError {
             "Cannot perform operation because the room is in a released state."
         case let .presenceOperationRequiresRoomAttach(feature):
             "To perform this \(Self.descriptionOfFeature(feature)) operation, you must first attach the room."
-        case let .presenceOperationDisallowedForCurrentRoomStatus(feature):
-            "This \(Self.descriptionOfFeature(feature)) operation can not be performed given the current room status."
-        case .roomInInvalidState:
+        case .roomTransitionedToInvalidStateForPresenceOperation:
             "The room operation failed because the room was in an invalid state."
         }
     }
@@ -282,14 +274,13 @@ internal enum ChatError {
             underlyingError
         case let .detachmentFailed(_, underlyingError):
             underlyingError
-        case let .roomInInvalidState(cause):
+        case let .roomTransitionedToInvalidStateForPresenceOperation(cause):
             cause
         case .inconsistentRoomOptions,
              .roomInFailedState,
              .roomIsReleasing,
              .roomIsReleased,
-             .presenceOperationRequiresRoomAttach,
-             .presenceOperationDisallowedForCurrentRoomStatus:
+             .presenceOperationRequiresRoomAttach:
             nil
         }
     }

Sources/AblyChat/RoomFeature.swift

@@ -41,12 +41,11 @@ internal protocol FeatureChannel: Sendable, EmitsDiscontinuities {
 
     /// Waits until we can perform presence operations on the contributors of this room without triggering an implicit attach.
     ///
-    /// Implements the checks described by CHA-PR3d, CHA-PR3e, CHA-PR3f, and CHA-PR3g (and similar ones described by other functionality that performs contributor presence operations). Namely:
+    /// Implements the checks described by CHA-PR3d, CHA-PR3e, and CHA-PR3h (and similar ones described by other functionality that performs contributor presence operations). Namely:
     ///
-    /// - CHA-RL9, which is invoked by CHA-PR3d, CHA-PR10d, CHA-PR6c, CHA-T2c: If the room is in the ATTACHING status, it waits for the next room status change. If the new status is ATTACHED, it returns. Else, it throws an `ARTErrorInfo` derived from ``ChatError.roomInInvalidState(cause:)``.
+    /// - CHA-RL9, which is invoked by CHA-PR3d, CHA-PR10d, CHA-PR6c, CHA-T2c: If the room is in the ATTACHING status, it waits for the next room status change. If the new status is ATTACHED, it returns. Else, it throws an `ARTErrorInfo` derived from ``ChatError.roomTransitionedToInvalidStateForPresenceOperation(cause:)``.
     /// - CHA-PR3e, CHA-PR10e, CHA-PR6d, CHA-T2d: If the room is in the ATTACHED status, it returns immediately.
-    /// - CHA-PR3f, CHA-PR10f, CHA-PR6e, CHA-T2e: If the room is in the DETACHED status, it throws an `ARTErrorInfo` derived from ``ChatError.presenceOperationRequiresRoomAttach(feature:)``.
-    /// - // CHA-PR3g, CHA-PR10g, CHA-PR6f, CHA-T2f: If the room is in any other status, it throws an `ARTErrorInfo` derived from ``ChatError.presenceOperationDisallowedForCurrentRoomStatus(feature:)``.
+    /// - CHA-PR3h, CHA-PR10h, CHA-PR6h, CHA-T2g: If the room is in any other status, it throws an `ARTErrorInfo` derived from ``ChatError.presenceOperationRequiresRoomAttach(feature:)``.
     ///
     /// - Parameters:
     ///   - requester: The room feature that wishes to perform a presence operation. This is only used for customising the message of the thrown error.

Sources/AblyChat/RoomLifecycleManager.swift

@@ -1225,17 +1225,14 @@ internal actor DefaultRoomLifecycleManager<Contributor: RoomLifecycleContributor
             // TODO: decide what to do if nextRoomStatusChange is nil; I believe that this will happen if the current Task is cancelled. For now, will just treat it as an invalid status change. Handle it properly in https://github.com/ably-labs/ably-chat-swift/issues/29
             if nextRoomStatusChange?.current != .attached {
                 // CHA-RL9c
-                throw .init(chatError: .roomInInvalidState(cause: nextRoomStatusChange?.current.error))
+                throw .init(chatError: .roomTransitionedToInvalidStateForPresenceOperation(cause: nextRoomStatusChange?.current.error))
             }
         case .attached:
             // CHA-PR3e, CHA-PR10e, CHA-PR6d, CHA-T2d
             break
-        case .detached:
-            // CHA-PR3f, CHA-PR10f, CHA-PR6e, CHA-T2e
-            throw .init(chatError: .presenceOperationRequiresRoomAttach(feature: requester))
         default:
-            // CHA-PR3g, CHA-PR10g, CHA-PR6f, CHA-T2f
-            throw .init(chatError: .presenceOperationDisallowedForCurrentRoomStatus(feature: requester))
+            // CHA-PR3h, CHA-PR10h, CHA-PR6h, CHA-T2g
+            throw .init(chatError: .presenceOperationRequiresRoomAttach(feature: requester))
         }
     }
 

Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift

@@ -2087,7 +2087,7 @@ struct DefaultRoomLifecycleManagerTests {
         let contributorAttachError = ARTErrorInfo.createUnknownError() // arbitrary
         contributorAttachOperation.complete(behavior: .completeAndChangeState(.failure(contributorAttachError), newState: .failed))
 
-        // Then: The call to `waitToBeAbleToPerformPresenceOperations(requestedByFeature:)` fails with a `roomInInvalidState` error, whose cause is the error associated with the room status change
+        // Then: The call to `waitToBeAbleToPerformPresenceOperations(requestedByFeature:)` fails with a `roomInInvalidState` error with status code 500, whose cause is the error associated with the room status change
         var caughtError: Error?
         do {
             try await waitToBeAbleToPerformPresenceOperationsResult
@@ -2096,7 +2096,7 @@ struct DefaultRoomLifecycleManagerTests {
         }
 
         let expectedCause = ARTErrorInfo(chatError: .attachmentFailed(feature: .messages, underlyingError: contributorAttachError)) // using our knowledge of CHA-RL1h4
-        #expect(isChatError(caughtError, withCodeAndStatusCode: .fixedStatusCode(.roomInInvalidState), cause: expectedCause))
+        #expect(isChatError(caughtError, withCodeAndStatusCode: .variableStatusCode(.roomInInvalidState, statusCode: 500), cause: expectedCause))
     }
 
     // @specPartial CHA-PR3e - Tests the wait described in the spec point, but not that the feature actually performs this wait nor the side effect. TODO change this to a specOneOf once the feature is implemented
@@ -2115,40 +2115,15 @@ struct DefaultRoomLifecycleManagerTests {
         try await manager.waitToBeAbleToPerformPresenceOperations(requestedByFeature: .messages /* arbitrary */ )
     }
 
-    // @specPartial CHA-PR3f - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
-    // @specPartial CHA-PR10f - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
-    // @specPartial CHA-PR6e - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
-    // @specPartial CHA-T2e - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
-    @Test
-    func waitToBeAbleToPerformPresenceOperations_whenDetached() async throws {
-        // Given: A DefaultRoomLifecycleManager in the DETACHED status
-        let manager = await createManager(
-            forTestingWhatHappensWhenCurrentlyIn: .detached
-        )
-
-        // (Note: I wanted to use #expect(…, throws:) below, but for some reason it made the compiler _crash_! No idea why. So, gave up on that.)
-
-        // When: `waitToBeAbleToPerformPresenceOperations(requestedByFeature:)` is called on the lifecycle manager
-        var caughtError: ARTErrorInfo?
-        do {
-            try await manager.waitToBeAbleToPerformPresenceOperations(requestedByFeature: .messages /* arbitrary */ )
-        } catch {
-            caughtError = error
-        }
-
-        // Then: It throws a presenceOperationRequiresRoomAttach error for that feature (which we just check via its error code, i.e. `.nonspecific`, and its message)
-        #expect(isChatError(caughtError, withCodeAndStatusCode: .fixedStatusCode(.nonspecific), message: "To perform this messages operation, you must first attach the room."))
-    }
-
-    // @specPartial CHA-PR3f - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
-    // @specPartial CHA-PR10f - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
-    // @specPartial CHA-PR6e - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
-    // @specPartial CHA-T2e - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
+    // @specPartial CHA-PR3h - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
+    // @specPartial CHA-PR10h - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
+    // @specPartial CHA-PR6h - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
+    // @specPartial CHA-T2g - Tests the wait described in the spec point, but not that the feature actually performs this wait. TODO change this to a specOneOf once the feature is implemented
     @Test
     func waitToBeAbleToPerformPresenceOperations_whenAnyOtherStatus() async throws {
-        // Given: A DefaultRoomLifecycleManager in a status other than ATTACHING, ATTACHED or DETACHED
+        // Given: A DefaultRoomLifecycleManager in a status other than ATTACHING or ATTACHED
         let manager = await createManager(
-            forTestingWhatHappensWhenCurrentlyIn: .detaching(detachOperationID: .init()) // arbitrary given the above constraints
+            forTestingWhatHappensWhenCurrentlyIn: .detached // arbitrary given the above constraints
         )
 
         // (Note: I wanted to use #expect(…, throws:) below, but for some reason it made the compiler _crash_! No idea why. So, gave up on that.)
@@ -2161,7 +2136,7 @@ struct DefaultRoomLifecycleManagerTests {
             caughtError = error
         }
 
-        // Then: It throws a presenceOperationDisallowedForCurrentRoomStatus error for that feature (which we just check via its error code, i.e. `.nonspecific`, and its message)
-        #expect(isChatError(caughtError, withCodeAndStatusCode: .fixedStatusCode(.nonspecific), message: "This messages operation can not be performed given the current room status."))
+        // Then: It throws a roomInInvalidState error for that feature, with status code 400, and a message explaining that the room must first be attached
+        #expect(isChatError(caughtError, withCodeAndStatusCode: .variableStatusCode(.roomInInvalidState, statusCode: 400), message: "To perform this messages operation, you must first attach the room."))
     }
 }