From 3d4e721a18f70b93b3bf66a85cda80dcb66c830d Mon Sep 17 00:00:00 2001 From: Brandon Sneed Date: Tue, 21 Nov 2023 11:22:07 -0800 Subject: [PATCH] Ensure update(settings:type:) is only called with .initial once. --- Sources/Segment/Plugins.swift | 3 +- Sources/Segment/Settings.swift | 50 +++++++++++++------ Sources/Segment/State.swift | 36 ++++++++++--- Sources/Segment/Timeline.swift | 3 -- Tests/Segment-Tests/Analytics_Tests.swift | 44 ++++++++++++++++ .../Segment-Tests/Support/TestUtilities.swift | 7 +++ 6 files changed, 119 insertions(+), 24 deletions(-) diff --git a/Sources/Segment/Plugins.swift b/Sources/Segment/Plugins.swift index 9ce104c9..f305896e 100644 --- a/Sources/Segment/Plugins.swift +++ b/Sources/Segment/Plugins.swift @@ -93,7 +93,6 @@ extension Plugin { } } - // MARK: - Adding/Removing Plugins extension DestinationPlugin { @@ -127,6 +126,7 @@ extension DestinationPlugin { plugin.configure(analytics: analytics) } timeline.add(plugin: plugin) + analytics?.updateIfNecessary(plugin: plugin) return plugin } @@ -189,6 +189,7 @@ extension Analytics { public func add(plugin: Plugin) -> Plugin { plugin.configure(analytics: self) timeline.add(plugin: plugin) + updateIfNecessary(plugin: plugin) return plugin } diff --git a/Sources/Segment/Settings.swift b/Sources/Segment/Settings.swift index 5a52e164..1cc1abca 100644 --- a/Sources/Segment/Settings.swift +++ b/Sources/Segment/Settings.swift @@ -108,20 +108,44 @@ extension Settings: Equatable { } extension Analytics { - internal func update(settings: Settings, type: UpdateType) { - apply { (plugin) in - // tell all top level plugins to update. - update(plugin: plugin, settings: settings, type: type) + internal func update(settings: Settings) { + guard let system: System = store.currentState() else { return } + apply { plugin in + plugin.update(settings: settings, type: updateType(for: plugin, in: system)) + if let destPlugin = plugin as? DestinationPlugin { + destPlugin.apply { subPlugin in + subPlugin.update(settings: settings, type: updateType(for: subPlugin, in: system)) + } + } } } - internal func update(plugin: Plugin, settings: Settings, type: UpdateType) { - plugin.update(settings: settings, type: type) - // if it's a destination, tell it's plugins to update as well. - if let dest = plugin as? DestinationPlugin { - dest.apply { (subPlugin) in - subPlugin.update(settings: settings, type: type) + internal func updateIfNecessary(plugin: Plugin) { + guard let system: System = store.currentState() else { return } + // if we're already running, update has already been called for existing plugins, + // so we just wanna call it on this one if it hasn't been done already. + if system.running, let settings = system.settings { + let alreadyInitialized = system.initializedPlugins.contains { p in + return plugin === p } + if !alreadyInitialized { + store.dispatch(action: System.AddPluginToInitialized(plugin: plugin)) + plugin.update(settings: settings, type: .initial) + } else { + plugin.update(settings: settings, type: .refresh) + } + } + } + + internal func updateType(for plugin: Plugin, in system: System) -> UpdateType { + let alreadyInitialized = system.initializedPlugins.contains { p in + return plugin === p + } + if alreadyInitialized { + return .refresh + } else { + store.dispatch(action: System.AddPluginToInitialized(plugin: plugin)) + return .initial } } @@ -135,6 +159,7 @@ extension Analytics { operatingMode.run(queue: DispatchQueue.main) { if let state: System = self.store.currentState(), let settings = state.settings { self.store.dispatch(action: System.UpdateSettingsAction(settings: settings)) + self.update(settings: settings) } self.store.dispatch(action: System.ToggleRunningAction(running: true)) } @@ -145,9 +170,6 @@ extension Analytics { let writeKey = self.configuration.values.writeKey let httpClient = HTTPClient(analytics: self) - let systemState: System? = store.currentState() - let hasSettings = (systemState?.settings?.integrations != nil && systemState?.settings?.plan != nil) - let updateType = (hasSettings ? UpdateType.refresh : UpdateType.initial) // stop things; queue in case our settings have changed. store.dispatch(action: System.ToggleRunningAction(running: false)) @@ -158,7 +180,7 @@ extension Analytics { // this will cause them to be cached. self.store.dispatch(action: System.UpdateSettingsAction(settings: s)) // let plugins know we just received some settings.. - self.update(settings: s, type: updateType) + self.update(settings: s) } } // we're good to go back to a running state. diff --git a/Sources/Segment/State.swift b/Sources/Segment/State.swift index 8c49b540..ede47290 100644 --- a/Sources/Segment/State.swift +++ b/Sources/Segment/State.swift @@ -15,6 +15,7 @@ struct System: State { let settings: Settings? let running: Bool let enabled: Bool + let initializedPlugins: [Plugin] struct UpdateSettingsAction: Action { let settings: Settings @@ -23,7 +24,8 @@ struct System: State { let result = System(configuration: state.configuration, settings: settings, running: state.running, - enabled: state.enabled) + enabled: state.enabled, + initializedPlugins: state.initializedPlugins) return result } } @@ -35,7 +37,8 @@ struct System: State { return System(configuration: state.configuration, settings: state.settings, running: running, - enabled: state.enabled) + enabled: state.enabled, + initializedPlugins: state.initializedPlugins) } } @@ -46,7 +49,8 @@ struct System: State { return System(configuration: state.configuration, settings: state.settings, running: state.running, - enabled: enabled) + enabled: enabled, + initializedPlugins: state.initializedPlugins) } } @@ -57,7 +61,8 @@ struct System: State { return System(configuration: configuration, settings: state.settings, running: state.running, - enabled: state.enabled) + enabled: state.enabled, + initializedPlugins: state.initializedPlugins) } } @@ -73,7 +78,26 @@ struct System: State { return System(configuration: state.configuration, settings: settings, running: state.running, - enabled: state.enabled) + enabled: state.enabled, + initializedPlugins: state.initializedPlugins) + } + } + + struct AddPluginToInitialized: Action { + let plugin: Plugin + + func reduce(state: System) -> System { + var initializedPlugins = state.initializedPlugins + if !initializedPlugins.contains(where: { p in + return plugin === p + }) { + initializedPlugins.append(plugin) + } + return System(configuration: state.configuration, + settings: state.settings, + running: state.running, + enabled: state.enabled, + initializedPlugins: initializedPlugins) } } } @@ -147,7 +171,7 @@ extension System { settings = Settings(writeKey: configuration.values.writeKey, apiHost: HTTPClient.getDefaultAPIHost()) } } - return System(configuration: configuration, settings: settings, running: false, enabled: true) + return System(configuration: configuration, settings: settings, running: false, enabled: true, initializedPlugins: [Plugin]()) } } diff --git a/Sources/Segment/Timeline.swift b/Sources/Segment/Timeline.swift index 063c4366..6fcd2dfe 100644 --- a/Sources/Segment/Timeline.swift +++ b/Sources/Segment/Timeline.swift @@ -55,9 +55,6 @@ public class Timeline { internal class Mediator { internal func add(plugin: Plugin) { plugins.append(plugin) - if let settings = plugin.analytics?.settings() { - plugin.update(settings: settings, type: .initial) - } } internal func remove(plugin: Plugin) { diff --git a/Tests/Segment-Tests/Analytics_Tests.swift b/Tests/Segment-Tests/Analytics_Tests.swift index 700e6b04..5c8796e0 100644 --- a/Tests/Segment-Tests/Analytics_Tests.swift +++ b/Tests/Segment-Tests/Analytics_Tests.swift @@ -50,6 +50,50 @@ final class Analytics_Tests: XCTestCase { wait(for: [expectation], timeout: 1.0) } + func testDestinationInitialUpdateOnlyOnce() { + // need to clear settings for this one. + UserDefaults.standard.removePersistentDomain(forName: "com.segment.storage.test") + + let expectation = XCTestExpectation(description: "MyDestination Expectation") + let myDestination = MyDestination { + expectation.fulfill() + return true + } + + var settings = Settings(writeKey: "test") + if let existing = settings.integrations?.dictionaryValue { + var newIntegrations = existing + newIntegrations[myDestination.key] = true + settings.integrations = try! JSON(newIntegrations) + } + let configuration = Configuration(writeKey: "test") + configuration.defaultSettings(settings) + let analytics = Analytics(configuration: configuration) + + let ziggy1 = ZiggyPlugin() + analytics.add(plugin: myDestination) + analytics.add(plugin: ziggy1) + + waitUntilStarted(analytics: analytics) + + analytics.track(name: "testDestinationEnabled") + + let ziggy2 = ZiggyPlugin() + analytics.add(plugin: ziggy2) + + let dest = analytics.find(key: myDestination.key) + XCTAssertNotNil(dest) + XCTAssertTrue(dest is MyDestination) + + wait(for: [expectation], timeout: 1.0) + + XCTAssertEqual(myDestination.receivedInitialUpdate, 1) + XCTAssertEqual(ziggy1.receivedInitialUpdate, 1) + XCTAssertEqual(ziggy2.receivedInitialUpdate, 1) + + } + + func testDestinationEnabled() { // need to clear settings for this one. UserDefaults.standard.removePersistentDomain(forName: "com.segment.storage.test") diff --git a/Tests/Segment-Tests/Support/TestUtilities.swift b/Tests/Segment-Tests/Support/TestUtilities.swift index c9a08444..8d9ce2d2 100644 --- a/Tests/Segment-Tests/Support/TestUtilities.swift +++ b/Tests/Segment-Tests/Support/TestUtilities.swift @@ -42,6 +42,7 @@ class GooberPlugin: EventPlugin { class ZiggyPlugin: EventPlugin { let type: PluginType var analytics: Analytics? + var receivedInitialUpdate: Int = 0 var completion: (() -> Void)? @@ -49,6 +50,10 @@ class ZiggyPlugin: EventPlugin { self.type = .enrichment } + func update(settings: Settings, type: UpdateType) { + if type == .initial { receivedInitialUpdate += 1 } + } + func identify(event: IdentifyEvent) -> IdentifyEvent? { var newEvent = IdentifyEvent(existing: event) newEvent.userId = "ziggy" @@ -78,6 +83,7 @@ class MyDestination: DestinationPlugin { let trackCompletion: (() -> Bool)? let disabled: Bool + var receivedInitialUpdate: Int = 0 init(disabled: Bool = false, trackCompletion: (() -> Bool)? = nil) { self.key = "MyDestination" @@ -88,6 +94,7 @@ class MyDestination: DestinationPlugin { } func update(settings: Settings, type: UpdateType) { + if type == .initial { receivedInitialUpdate += 1 } if disabled == false { // add ourselves to the settings analytics?.manuallyEnableDestination(plugin: self)