diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift index e4220e0be..107254fd2 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift @@ -246,8 +246,7 @@ class OSPropertyOperationExecutor: OSOperationExecutor { } OneSignalCoreImpl.sharedClient().execute(request) { response in - // On success, remove request from cache, and we do need to hydrate - // TODO: We need to hydrate after all ? What why ? + // On success, remove request from cache self.dispatchQueue.async { self.updateRequestQueue.removeAll(where: { $0 == request}) OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) @@ -255,6 +254,16 @@ class OSPropertyOperationExecutor: OSOperationExecutor { OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) } } + + // Re-assert the tags the server just confirmed by merging them back into the local model, + // to remedy a concurrent FetchUser whose response is missing the just-written tags + if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel), + let properties = response?["properties"] as? [String: Any], + let confirmedTags = properties["tags"] as? [String: String], + !confirmedTags.isEmpty { + OneSignalUserManagerImpl.sharedInstance._user?.propertiesModel.mergeConfirmedTags(confirmedTags) + } + if let onesignalId = request.identityModel.onesignalId { if let rywToken = response?["ryw_token"] as? String { diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift index 03d2901f6..0b966f7a3 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift @@ -151,6 +151,25 @@ class OSPropertiesModel: OSModel { self.set(property: "tags", newValue: tagsToSend) } + /** + Merges server-confirmed tags into the local model, without enqueuing a new delta. + A value of `""` removes the tag. + + Used to re-assert tags in order to remedy a concurrent FetchUser whose response + may be missing the just-written tags. + */ + func mergeConfirmedTags(_ serverTags: [String: String]) { + tagsLock.withLock { + for (key, value) in serverTags { + if value.isEmpty { + self.tags.removeValue(forKey: key) + } else { + self.tags[key] = value + } + } + } + } + public override func hydrateModel(_ response: [String: Any]) { for property in response { switch property.key { diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index 5b4a9d2c7..9c55ec928 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -189,4 +189,63 @@ final class OneSignalUserTests: XCTestCase { contains: expectedPayload) ) } + + /** + Unit test for the tag-merge primitive. + + `mergeConfirmedTags` must overlay only the provided keys onto the local model: update existing + values, remove keys whose value is `""`, add new keys, and leave all other (e.g. backend-managed) + tags untouched. It must never wholesale-replace the tag dictionary. + */ + func testMergeConfirmedTags_mergesAndRemovesWithoutTouchingOtherTags() throws { + /* Setup */ + let model = OSPropertiesModel(changeNotifier: OSEventProducer()) + model.hydrate(["tags": ["keep": "1", "update": "old", "remove": "2"]]) + + /* When */ + // "update" changes, "remove" is deleted (""), "add" is new; "keep" must be left untouched + model.mergeConfirmedTags(["update": "new", "remove": "", "add": "3"]) + + /* Then */ + XCTAssertEqual(model.tags, ["keep": "1", "update": "new", "add": "3"]) + } + + /** + Regression test: `getTags()` returns `{}` after a concurrent, stale `FetchUser` overwrites the + local tag model. + + A concurrent `FetchUser` whose response is missing a recently written tag clears the local tag + cache (`clearUserData()`) and hydrates without it. The `OSRequestUpdateProperties` 202 response + echoes the tags the server just confirmed, so merging those back into the local model must restore + the tags that the stale fetch cleared. + */ + func testUpdatePropertiesResponse_restoresTagsClearedByConcurrentFetch() throws { + /* Setup */ + let client = MockOneSignalClient() + MockUserRequests.setDefaultCreateAnonUserResponses(with: client) + let tags = ["tag_a": "value_a", "tag_b": "value_b"] + MockUserRequests.setAddTagsResponse(with: client, tags: tags) + OneSignalCoreImpl.setSharedClient(client) + + OneSignalUserManagerImpl.sharedInstance.start() + + // Let the anonymous user be created so it has a OneSignal ID for the update request + OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.5) + + /* When */ + // Tags are applied optimistically to the local model and queued as an update request + OneSignalUserManagerImpl.sharedInstance.addTags(tags) + + // Simulate a concurrent, stale FetchUser clearing the local tag cache before the + // UpdateProperties 202 response is processed + OneSignalUserManagerImpl.sharedInstance.user.propertiesModel.clearData() + XCTAssertTrue(OneSignalUserManagerImpl.sharedInstance.getTags().isEmpty) + + // Let the queued UpdateProperties request flush and its 202 echo be processed + OneSignalCoreMocks.waitForBackgroundThreads(seconds: 1) + + /* Then */ + // The confirmed tags from the 202 response are merged back into the local model + XCTAssertEqual(OneSignalUserManagerImpl.sharedInstance.getTags(), tags) + } }