Update map "replace data", MAP_SET, MAP_REMOVE to set entry tombstonedAt

Based on spec referenced in 040987b; same comment re testing applies
too.

Author: lawrence-forooghian

Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift

@@ -320,11 +320,14 @@ internal final class InternalDefaultLiveMap: Sendable {
     /// Applies a `MAP_REMOVE` operation to a key, per RTLM8.
     ///
     /// This is currently exposed just so that the tests can test RTLM8 without having to go through a convoluted replaceData(…) call, but I _think_ that it's going to be used in further contexts when we introduce the handling of incoming object operations in a future spec PR.
-    internal func testsOnly_applyMapRemoveOperation(key: String, operationTimeserial: String?) -> LiveObjectUpdate<DefaultLiveMapUpdate> {
+    internal func testsOnly_applyMapRemoveOperation(key: String, operationTimeserial: String?, operationSerialTimestamp: Date?) -> LiveObjectUpdate<DefaultLiveMapUpdate> {
         mutex.withLock {
             mutableState.applyMapRemoveOperation(
                 key: key,
                 operationTimeserial: operationTimeserial,
+                operationSerialTimestamp: operationSerialTimestamp,
+                logger: logger,
+                clock: clock,
             )
         }
     }
@@ -396,7 +399,25 @@ internal final class InternalDefaultLiveMap: Sendable {
             liveObjectMutableState.createOperationIsMerged = false
 
             // RTLM6c: Set data to ObjectState.map.entries, or to an empty map if it does not exist
-            data = state.map?.entries?.mapValues { .init(objectsMapEntry: $0) } ?? [:]
+            data = state.map?.entries?.mapValues { entry in
+                // Set tombstonedAt for tombstoned entries
+                let tombstonedAt: Date?
+                if entry.tombstone == true {
+                    // RTLM6c1a
+                    if let serialTimestamp = entry.serialTimestamp {
+                        tombstonedAt = serialTimestamp
+                    } else {
+                        // RTLM6c1b
+                        logger.log("serialTimestamp not found in ObjectsMapEntry, using local clock for tombstone timestamp", level: .debug)
+                        // RTLM6cb1
+                        tombstonedAt = clock.now
+                    }
+                } else {
+                    tombstonedAt = nil
+                }
+
+                return .init(objectsMapEntry: entry, tombstonedAt: tombstonedAt)
+            } ?? [:]
 
             // RTLM6d: If ObjectState.createOp is present, merge the initial value into the LiveMap as described in RTLM17
             return if let createOp = state.createOp {
@@ -426,10 +447,13 @@ internal final class InternalDefaultLiveMap: Sendable {
                 entries.map { key, entry in
                     if entry.tombstone == true {
                         // RTLM17a2: If ObjectsMapEntry.tombstone is true, apply the MAP_REMOVE operation
-                        // as described in RTLM8, passing in the current key as ObjectsMapOp, and ObjectsMapEntry.timeserial as the operation's serial
+                        // as described in RTLM8, passing in the current key as ObjectsMapOp, ObjectsMapEntry.timeserial as the operation's serial, and ObjectsMapEntry.serialTimestamp as the operation's serial timestamp
                         applyMapRemoveOperation(
                             key: key,
                             operationTimeserial: entry.timeserial,
+                            operationSerialTimestamp: entry.serialTimestamp,
+                            logger: logger,
+                            clock: clock,
                         )
                     } else {
                         // RTLM17a1: If ObjectsMapEntry.tombstone is false, apply the MAP_SET operation
@@ -538,6 +562,9 @@ internal final class InternalDefaultLiveMap: Sendable {
                 let update = applyMapRemoveOperation(
                     key: mapOp.key,
                     operationTimeserial: applicableOperation.objectMessageSerial,
+                    operationSerialTimestamp: objectMessageSerialTimestamp,
+                    logger: logger,
+                    clock: clock,
                 )
                 // RTLM15d3a
                 liveObjectMutableState.emit(update, on: userCallbackQueue)
@@ -578,17 +605,17 @@ internal final class InternalDefaultLiveMap: Sendable {
                 // RTLM7a2: Otherwise, apply the operation
                 // RTLM7a2a: Set ObjectsMapEntry.data to the ObjectData from the operation
                 // RTLM7a2b: Set ObjectsMapEntry.timeserial to the operation's serial
-                // RTLM7a2c: Set ObjectsMapEntry.tombstone to false
+                // RTLM7a2c: Set ObjectsMapEntry.tombstone to false (same as RTLM7a2d: Set ObjectsMapEntry.tombstonedAt to nil)
                 var updatedEntry = existingEntry
                 updatedEntry.data = operationData
                 updatedEntry.timeserial = operationTimeserial
-                updatedEntry.tombstone = false
+                updatedEntry.tombstonedAt = nil
                 data[key] = updatedEntry
             } else {
                 // RTLM7b: If an entry does not exist in the private data for the specified key
                 // RTLM7b1: Create a new entry in data for the specified key with the provided ObjectData and the operation's serial
-                // RTLM7b2: Set ObjectsMapEntry.tombstone for the new entry to false
-                data[key] = InternalObjectsMapEntry(tombstone: false, timeserial: operationTimeserial, data: operationData)
+                // RTLM7b2: Set ObjectsMapEntry.tombstone for the new entry to false (same as RTLM7b3: Set tombstonedAt to nil)
+                data[key] = InternalObjectsMapEntry(tombstonedAt: nil, timeserial: operationTimeserial, data: operationData)
             }
 
             // RTLM7c: If the operation has a non-empty ObjectData.objectId attribute
@@ -602,9 +629,21 @@ internal final class InternalDefaultLiveMap: Sendable {
         }
 
         /// Applies a `MAP_REMOVE` operation to a key, per RTLM8.
-        internal mutating func applyMapRemoveOperation(key: String, operationTimeserial: String?) -> LiveObjectUpdate<DefaultLiveMapUpdate> {
+        internal mutating func applyMapRemoveOperation(key: String, operationTimeserial: String?, operationSerialTimestamp: Date?, logger: Logger, clock: SimpleClock) -> LiveObjectUpdate<DefaultLiveMapUpdate> {
             // (Note that, where the spec tells us to set ObjectsMapEntry.data to nil, we actually set it to an empty ObjectData, which is equivalent, since it contains no data)
 
+            // Calculate the tombstonedAt for the new or updated entry per RTLM8f
+            let tombstonedAt: Date?
+            if let operationSerialTimestamp {
+                // RTLM8f1
+                tombstonedAt = operationSerialTimestamp
+            } else {
+                // RTLM8f2
+                logger.log("serialTimestamp not provided for MAP_REMOVE, using local clock for tombstone timestamp", level: .debug)
+                // RTLM8f2a
+                tombstonedAt = clock.now
+            }
+
             // RTLM8a: If an entry exists in the private data for the specified key
             if let existingEntry = data[key] {
                 // RTLM8a1: If the operation cannot be applied as per RTLM9, discard the operation
@@ -614,17 +653,19 @@ internal final class InternalDefaultLiveMap: Sendable {
                 // RTLM8a2: Otherwise, apply the operation
                 // RTLM8a2a: Set ObjectsMapEntry.data to undefined/null
                 // RTLM8a2b: Set ObjectsMapEntry.timeserial to the operation's serial
-                // RTLM8a2c: Set ObjectsMapEntry.tombstone to true
+                // RTLM8a2c: Set ObjectsMapEntry.tombstone to true (equivalent to next point)
+                // RTLM8a2d: Set ObjectsMapEntry.tombstonedAt per RTLM8a2d
                 var updatedEntry = existingEntry
                 updatedEntry.data = ObjectData()
                 updatedEntry.timeserial = operationTimeserial
-                updatedEntry.tombstone = true
+                updatedEntry.tombstonedAt = tombstonedAt
                 data[key] = updatedEntry
             } else {
                 // RTLM8b: If an entry does not exist in the private data for the specified key
                 // RTLM8b1: Create a new entry in data for the specified key, with ObjectsMapEntry.data set to undefined/null and the operation's serial
                 // RTLM8b2: Set ObjectsMapEntry.tombstone for the new entry to true
-                data[key] = InternalObjectsMapEntry(tombstone: true, timeserial: operationTimeserial, data: ObjectData())
+                // RTLM8b3: Set ObjectsMapEntry.tombstonedAt per RTLM8f
+                data[key] = InternalObjectsMapEntry(tombstonedAt: tombstonedAt, timeserial: operationTimeserial, data: ObjectData())
             }
 
             return .update(DefaultLiveMapUpdate(update: [key: .removed]))

Sources/AblyLiveObjects/Internal/InternalObjectsMapEntry.swift

@@ -1,13 +1,20 @@
-/// The entries stored in a `LiveMap`'s data. Same as an `ObjectsMapEntry` but with an additional `tombstonedAt` property, per RTLM3a. (This property will be added in an upcoming commit.)
+import Foundation
+
+/// The entries stored in a `LiveMap`'s data. Same as an `ObjectsMapEntry` but with an additional `tombstonedAt` property, per RTLM3a.
 internal struct InternalObjectsMapEntry {
-    internal var tombstone: Bool? // OME2a
+    internal var tombstonedAt: Date? // RTLM3a
+    internal var tombstone: Bool {
+        // TODO: Confirm that we don't need to store this (https://github.com/ably/specification/pull/350/files#r2213895661)
+        tombstonedAt != nil
+    }
+
     internal var timeserial: String? // OME2b
     internal var data: ObjectData // OME2c
 }
 
 internal extension InternalObjectsMapEntry {
-    init(objectsMapEntry: ObjectsMapEntry) {
-        tombstone = objectsMapEntry.tombstone
+    init(objectsMapEntry: ObjectsMapEntry, tombstonedAt: Date?) {
+        self.tombstonedAt = tombstonedAt
         timeserial = objectsMapEntry.timeserial
         data = objectsMapEntry.data
     }

Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift

@@ -406,12 +406,12 @@ struct TestFactories {
     ///
     /// This should be kept in sync with ``mapEntry``.
     static func internalMapEntry(
-        tombstone: Bool? = false,
+        tombstonedAt: Date? = nil,
         timeserial: String? = "ts1",
         data: ObjectData,
     ) -> InternalObjectsMapEntry {
         InternalObjectsMapEntry(
-            tombstone: tombstone,
+            tombstonedAt: tombstonedAt,
             timeserial: timeserial,
             data: data,
         )
@@ -440,13 +440,13 @@ struct TestFactories {
     static func internalStringMapEntry(
         key: String = "testKey",
         value: String = "testValue",
-        tombstone: Bool? = false,
+        tombstonedAt: Date? = nil,
         timeserial: String? = "ts1",
     ) -> (key: String, entry: InternalObjectsMapEntry) {
         (
             key: key,
             entry: internalMapEntry(
-                tombstone: tombstone,
+                tombstonedAt: nil,
                 timeserial: timeserial,
                 data: ObjectData(string: value),
             ),

Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift

@@ -39,7 +39,7 @@ struct InternalDefaultLiveMapTests {
         func returnsNilWhenEntryIsTombstoned() throws {
             let logger = TestLogger()
             let entry = TestFactories.internalMapEntry(
-                tombstone: true,
+                tombstonedAt: Date(),
                 data: ObjectData(boolean: true), // Value doesn't matter as it's tombstoned
             )
             let coreSDK = MockCoreSDK(channelState: .attaching)
@@ -317,12 +317,11 @@ struct InternalDefaultLiveMapTests {
             let delegate = MockLiveMapObjectPoolDelegate()
             let map = InternalDefaultLiveMap(
                 testsOnly_data: [
-                    // tombstone is nil, so not considered tombstoned
+                    // tombstonedAt is nil, so not considered tombstoned
                     "active1": TestFactories.internalMapEntry(data: ObjectData(string: "value1")),
-                    // tombstone is false, so not considered tombstoned[
-                    "active2": TestFactories.internalMapEntry(tombstone: false, data: ObjectData(string: "value2")),
-                    "tombstoned": TestFactories.internalMapEntry(tombstone: true, data: ObjectData(string: "tombstoned")),
-                    "tombstoned2": TestFactories.internalMapEntry(tombstone: true, data: ObjectData(string: "tombstoned2")),
+                    // tombstonedAt is false, so not considered tombstoned
+                    "tombstoned": TestFactories.internalMapEntry(tombstonedAt: Date(), data: ObjectData(string: "tombstoned")),
+                    "tombstoned2": TestFactories.internalMapEntry(tombstonedAt: Date(), data: ObjectData(string: "tombstoned2")),
                 ],
                 objectID: "arbitrary",
                 logger: logger,
@@ -332,24 +331,23 @@ struct InternalDefaultLiveMapTests {
 
             // Test size - should only count non-tombstoned entries
             let size = try map.size(coreSDK: coreSDK)
-            #expect(size == 2)
+            #expect(size == 1)
 
             // Test entries - should only return non-tombstoned entries
             let entries = try map.entries(coreSDK: coreSDK, delegate: delegate)
-            #expect(entries.count == 2)
-            #expect(Set(entries.map(\.key)) == ["active1", "active2"])
+            #expect(entries.count == 1)
+            #expect(Set(entries.map(\.key)) == ["active1"])
             #expect(entries.first { $0.key == "active1" }?.value.stringValue == "value1")
-            #expect(entries.first { $0.key == "active2" }?.value.stringValue == "value2")
 
             // Test keys - should only return keys from non-tombstoned entries
             let keys = try map.keys(coreSDK: coreSDK, delegate: delegate)
-            #expect(keys.count == 2)
-            #expect(Set(keys) == ["active1", "active2"])
+            #expect(keys.count == 1)
+            #expect(Set(keys) == ["active1"])
 
             // Test values - should only return values from non-tombstoned entries
             let values = try map.values(coreSDK: coreSDK, delegate: delegate)
-            #expect(values.count == 2)
-            #expect(Set(values.compactMap(\.stringValue)) == Set(["value1", "value2"]))
+            #expect(values.count == 1)
+            #expect(Set(values.compactMap(\.stringValue)) == Set(["value1"]))
         }
 
         // MARK: - Consistency Tests
@@ -507,7 +505,7 @@ struct InternalDefaultLiveMapTests {
                 let delegate = MockLiveMapObjectPoolDelegate()
                 let coreSDK = MockCoreSDK(channelState: .attaching)
                 let map = InternalDefaultLiveMap(
-                    testsOnly_data: ["key1": TestFactories.internalMapEntry(tombstone: true, timeserial: "ts1", data: ObjectData(string: "existing"))],
+                    testsOnly_data: ["key1": TestFactories.internalMapEntry(tombstonedAt: Date(), timeserial: "ts1", data: ObjectData(string: "existing"))],
                     objectID: "arbitrary",
                     logger: logger,
                     userCallbackQueue: .main,
@@ -687,7 +685,7 @@ struct InternalDefaultLiveMapTests {
                 )
 
                 // Try to apply operation with lower timeserial (ts1 < ts2), cannot be applied per RTLM9
-                let update = map.testsOnly_applyMapRemoveOperation(key: "key1", operationTimeserial: "ts1")
+                let update = map.testsOnly_applyMapRemoveOperation(key: "key1", operationTimeserial: "ts1", operationSerialTimestamp: nil)
 
                 // Verify the operation was discarded - existing data unchanged
                 #expect(try map.get(key: "key1", coreSDK: coreSDK, delegate: delegate)?.stringValue == "existing")
@@ -705,15 +703,15 @@ struct InternalDefaultLiveMapTests {
                 let delegate = MockLiveMapObjectPoolDelegate()
                 let coreSDK = MockCoreSDK(channelState: .attaching)
                 let map = InternalDefaultLiveMap(
-                    testsOnly_data: ["key1": TestFactories.internalMapEntry(tombstone: false, timeserial: "ts1", data: ObjectData(string: "existing"))],
+                    testsOnly_data: ["key1": TestFactories.internalMapEntry(tombstonedAt: nil, timeserial: "ts1", data: ObjectData(string: "existing"))],
                     objectID: "arbitrary",
                     logger: logger,
                     userCallbackQueue: .main,
                     clock: MockSimpleClock(),
                 )
 
                 // Apply operation with higher timeserial (ts2 > ts1), so can be applied per RTLM9
-                let update = map.testsOnly_applyMapRemoveOperation(key: "key1", operationTimeserial: "ts2")
+                let update = map.testsOnly_applyMapRemoveOperation(key: "key1", operationTimeserial: "ts2", operationSerialTimestamp: nil)
 
                 // Verify the operation was applied
                 #expect(try map.get(key: "key1", coreSDK: coreSDK, delegate: delegate) == nil)
@@ -747,7 +745,7 @@ struct InternalDefaultLiveMapTests {
                 let logger = TestLogger()
                 let map = InternalDefaultLiveMap.createZeroValued(objectID: "arbitrary", logger: logger, userCallbackQueue: .main, clock: MockSimpleClock())
 
-                let update = map.testsOnly_applyMapRemoveOperation(key: "newKey", operationTimeserial: "ts1")
+                let update = map.testsOnly_applyMapRemoveOperation(key: "newKey", operationTimeserial: "ts1", operationSerialTimestamp: nil)
 
                 // Verify new entry was created
                 let entry = map.testsOnly_data["newKey"]
@@ -769,7 +767,7 @@ struct InternalDefaultLiveMapTests {
                 let logger = TestLogger()
                 let map = InternalDefaultLiveMap.createZeroValued(objectID: "arbitrary", logger: logger, userCallbackQueue: .main, clock: MockSimpleClock())
 
-                _ = map.testsOnly_applyMapRemoveOperation(key: "newKey", operationTimeserial: "ts1")
+                _ = map.testsOnly_applyMapRemoveOperation(key: "newKey", operationTimeserial: "ts1", operationSerialTimestamp: nil)
 
                 // Verify tombstone is true for new entry
                 #expect(map.testsOnly_data["newKey"]?.tombstone == true)