From d5fed806f71b0d01252276883c58faeecbb8ca28 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 20 Nov 2024 12:04:14 -0300 Subject: [PATCH 1/3] Fix mistake in 3344206 This was not caught, because we have no tests for the presence or occupancy features. --- Sources/AblyChat/Room.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/Sources/AblyChat/Room.swift b/Sources/AblyChat/Room.swift index e0c724b2..82356d26 100644 --- a/Sources/AblyChat/Room.swift +++ b/Sources/AblyChat/Room.swift @@ -142,7 +142,6 @@ internal actor DefaultRoom // channel setup for presence and occupancy if feature == .presence { - let channelOptions = ARTRealtimeChannelOptions() let presenceOptions = roomOptions.presence if presenceOptions?.enter ?? false { From 00bb2e1a663a713f519de6ae8c32a0e79e3bc2f6 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 20 Nov 2024 15:33:57 -0300 Subject: [PATCH 2/3] Comment out code for setting presence modes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This code is not actually doing anything at the moment; the way in which we’re currently fetching channels means that these modes get overwritten, which we’ll fix in #132. But, when we fix the aforementioned, we’ll see that setting these flags leads to some weird Realtime behaviours, so we’ll turn them off for now and restore them separately in #133. --- Sources/AblyChat/Room.swift | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/Sources/AblyChat/Room.swift b/Sources/AblyChat/Room.swift index 82356d26..c0e71a89 100644 --- a/Sources/AblyChat/Room.swift +++ b/Sources/AblyChat/Room.swift @@ -131,7 +131,7 @@ internal actor DefaultRoom internal var contributor: DefaultRoomLifecycleContributor } - private static func createFeatureChannelPartialDependencies(roomID: String, roomOptions: RoomOptions, realtime: RealtimeClient) -> [RoomFeature: FeatureChannelPartialDependencies] { + private static func createFeatureChannelPartialDependencies(roomID: String, roomOptions _: RoomOptions, realtime: RealtimeClient) -> [RoomFeature: FeatureChannelPartialDependencies] { .init(uniqueKeysWithValues: [ RoomFeature.messages, RoomFeature.reactions, @@ -142,15 +142,18 @@ internal actor DefaultRoom // channel setup for presence and occupancy if feature == .presence { - let presenceOptions = roomOptions.presence - - if presenceOptions?.enter ?? false { - channelOptions.modes.insert(.presence) - } - - if presenceOptions?.subscribe ?? false { - channelOptions.modes.insert(.presenceSubscribe) - } + // TODO: Restore this code once we understand weird Realtime behaviour and spec points (https://github.com/ably-labs/ably-chat-swift/issues/133) + /* + let presenceOptions = roomOptions.presence + + if presenceOptions?.enter ?? false { + channelOptions.modes.insert(.presence) + } + + if presenceOptions?.subscribe ?? false { + channelOptions.modes.insert(.presenceSubscribe) + } + */ } else if feature == .occupancy { channelOptions.params = ["occupancy": "metrics"] } From f29dc7b7c97c2d4ccbf887be3f77a86154d9283b Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 20 Nov 2024 12:02:44 -0300 Subject: [PATCH 3/3] Implement CHA-RC3a (only fetch each channel once) Based on spec at 28d09cc. Resolves #132. --- Sources/AblyChat/Room.swift | 69 ++++++++++++++-------- Tests/AblyChatTests/DefaultRoomTests.swift | 23 ++++++++ 2 files changed, 68 insertions(+), 24 deletions(-) diff --git a/Sources/AblyChat/Room.swift b/Sources/AblyChat/Room.swift index c0e71a89..49189ff2 100644 --- a/Sources/AblyChat/Room.swift +++ b/Sources/AblyChat/Room.swift @@ -131,36 +131,57 @@ internal actor DefaultRoom internal var contributor: DefaultRoomLifecycleContributor } - private static func createFeatureChannelPartialDependencies(roomID: String, roomOptions _: RoomOptions, realtime: RealtimeClient) -> [RoomFeature: FeatureChannelPartialDependencies] { - .init(uniqueKeysWithValues: [ - RoomFeature.messages, - RoomFeature.reactions, - RoomFeature.presence, - RoomFeature.occupancy, - ].map { feature in + /// The returned dictionary is guaranteed to have an entry for each element of `features`. + private static func createChannelsForFeatures(_ features: [RoomFeature], roomID: String, roomOptions _: RoomOptions, realtime: RealtimeClient) -> [RoomFeature: RealtimeChannelProtocol] { + // CHA-RC3a + + // Multiple features can share a realtime channel. We fetch each realtime channel exactly once, merging the channel options for the various features that use this channel. + + let featuresGroupedByChannelName = Dictionary(grouping: features) { $0.channelNameForRoomID(roomID) } + + let pairsOfFeatureAndChannel = featuresGroupedByChannelName.flatMap { channelName, features in var channelOptions = RealtimeChannelOptions() // channel setup for presence and occupancy - if feature == .presence { - // TODO: Restore this code once we understand weird Realtime behaviour and spec points (https://github.com/ably-labs/ably-chat-swift/issues/133) - /* - let presenceOptions = roomOptions.presence - - if presenceOptions?.enter ?? false { - channelOptions.modes.insert(.presence) - } - - if presenceOptions?.subscribe ?? false { - channelOptions.modes.insert(.presenceSubscribe) - } - */ - } else if feature == .occupancy { - channelOptions.params = ["occupancy": "metrics"] + for feature in features { + if feature == .presence { + // TODO: Restore this code once we understand weird Realtime behaviour and spec points (https://github.com/ably-labs/ably-chat-swift/issues/133) + /* + let presenceOptions = roomOptions.presence + + if presenceOptions?.enter ?? false { + channelOptions.modes.insert(.presence) + } + + if presenceOptions?.subscribe ?? false { + channelOptions.modes.insert(.presenceSubscribe) + } + */ + } else if feature == .occupancy { + var params: [String: String] = channelOptions.params ?? [:] + params["occupancy"] = "metrics" + channelOptions.params = params + } } - let channel = realtime.getChannel(feature.channelNameForRoomID(roomID), opts: channelOptions) - let contributor = DefaultRoomLifecycleContributor(channel: .init(underlyingChannel: channel), feature: feature) + let channel = realtime.getChannel(channelName, opts: channelOptions) + return features.map { ($0, channel) } + } + return Dictionary(uniqueKeysWithValues: pairsOfFeatureAndChannel) + } + + private static func createFeatureChannelPartialDependencies(roomID: String, roomOptions: RoomOptions, realtime: RealtimeClient) -> [RoomFeature: FeatureChannelPartialDependencies] { + let features: [RoomFeature] = [ + .messages, + .reactions, + .presence, + .occupancy, + ] + let channelsByFeature = createChannelsForFeatures(features, roomID: roomID, roomOptions: roomOptions, realtime: realtime) + + return .init(uniqueKeysWithValues: channelsByFeature.map { feature, channel in + let contributor = DefaultRoomLifecycleContributor(channel: .init(underlyingChannel: channel), feature: feature) return (feature, .init(channel: channel, contributor: contributor)) }) } diff --git a/Tests/AblyChatTests/DefaultRoomTests.swift b/Tests/AblyChatTests/DefaultRoomTests.swift index c3f85e84..ca89167e 100644 --- a/Tests/AblyChatTests/DefaultRoomTests.swift +++ b/Tests/AblyChatTests/DefaultRoomTests.swift @@ -23,6 +23,29 @@ struct DefaultRoomTests { #expect(channelsGetArguments.allSatisfy { $0.options.attachOnSubscribe == false }) } + // @spec CHA-RC3a + @Test + func fetchesEachChannelOnce() async throws { + // Given: A DefaultRoom instance, configured to use presence and occupancy + let channelsList = [ + MockRealtimeChannel(name: "basketball::$chat::$chatMessages", attachResult: .success), + MockRealtimeChannel(name: "basketball::$chat::$reactions", attachResult: .success), + ] + let channels = MockChannels(channels: channelsList) + let realtime = MockRealtime.create(channels: channels) + let roomOptions = RoomOptions(presence: PresenceOptions(), occupancy: OccupancyOptions()) + _ = try await DefaultRoom(realtime: realtime, chatAPI: ChatAPI(realtime: realtime), roomID: "basketball", options: roomOptions, logger: TestLogger(), lifecycleManagerFactory: MockRoomLifecycleManagerFactory()) + + // Then: It fetches the …$chatMessages channel (which is used by messages, presence, and occupancy) only once, and the options with which it does so are the result of merging the options used by the presence feature and those used by the occupancy feature + let channelsGetArguments = channels.getArguments + #expect(channelsGetArguments.map(\.name).sorted() == ["basketball::$chat::$chatMessages", "basketball::$chat::$reactions"]) + + let chatMessagesChannelGetOptions = try #require(channelsGetArguments.first { $0.name == "basketball::$chat::$chatMessages" }?.options) + #expect(chatMessagesChannelGetOptions.params?["occupancy"] == "metrics") + // TODO: Restore this code once we understand weird Realtime behaviour and spec points (https://github.com/ably-labs/ably-chat-swift/issues/133) +// #expect(chatMessagesChannelGetOptions.modes == [.presence, .presenceSubscribe]) + } + // MARK: - Features // @spec CHA-M1