Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: attributes thread safety issue #73

Merged
merged 3 commits into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .swiftformat
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
--enable semicolons
--semicolons never
--disable sortedImports
--disable sortImports
--importgrouping testable-bottom
--enable spaceAroundOperators
--operatorfunc spaced
Expand Down
1 change: 1 addition & 0 deletions .swiftlint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ opt_in_rules:

disabled_rules:
- opening_brace
- non_optional_string_data_conversion

# configurable rules can be customized from this configuration file
closing_brace: error
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ extension AWSClickstreamPlugin {
}

func registerGlobalProperties(_ properties: AnalyticsProperties) {
properties.forEach { key, value in
for (key, value) in properties {
analyticsClient.addGlobalAttribute(value, forKey: key)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class AnalyticsClient: AnalyticsClientBehaviour {
private(set) var simpleUserAttributes: [String: Any] = [:]
private let clickstream: ClickstreamContext
private(set) var userId: String?
let attributeLock = NSLock()
var autoRecordClient: AutoRecordEventClient

init(clickstream: ClickstreamContext,
Expand All @@ -45,6 +46,7 @@ class AnalyticsClient: AnalyticsClientBehaviour {
}

func addGlobalAttribute(_ attribute: AttributeValue, forKey key: String) {
attributeLock.lock()
let eventError = EventChecker.checkAttribute(
currentNumber: globalAttributes.count,
key: key,
Expand All @@ -54,9 +56,11 @@ class AnalyticsClient: AnalyticsClientBehaviour {
} else {
globalAttributes[key] = attribute
}
attributeLock.unlock()
}

func addUserAttribute(_ attribute: AttributeValue, forKey key: String) {
attributeLock.lock()
let eventError = EventChecker.checkUserAttribute(currentNumber: allUserAttributes.count,
key: key,
value: attribute)
Expand All @@ -72,22 +76,29 @@ class AnalyticsClient: AnalyticsClientBehaviour {
userAttribute["set_timestamp"] = Date().millisecondsSince1970
allUserAttributes[key] = userAttribute
}
attributeLock.unlock()
}

func removeGlobalAttribute(forKey key: String) {
attributeLock.lock()
globalAttributes[key] = nil
attributeLock.unlock()
}

func removeUserAttribute(forKey key: String) {
attributeLock.lock()
allUserAttributes[key] = nil
attributeLock.unlock()
}

func updateUserId(_ id: String?) {
if userId != id {
userId = id
UserDefaultsUtil.saveCurrentUserId(storage: clickstream.storage, userId: userId)
if let newUserId = id, !newUserId.isEmpty {
attributeLock.lock()
allUserAttributes = JsonObject()
attributeLock.unlock()
let userInfo = UserDefaultsUtil.getNewUserInfo(storage: clickstream.storage, userId: newUserId)
// swiftlint:disable force_cast
clickstream.userUniqueId = userInfo["user_unique_id"] as! String
Expand All @@ -105,7 +116,9 @@ class AnalyticsClient: AnalyticsClientBehaviour {
}

func updateUserAttributes() {
attributeLock.lock()
UserDefaultsUtil.updateUserAttributes(storage: clickstream.storage, userAttributes: allUserAttributes)
attributeLock.unlock()
}

// MARK: - Event recording
Expand All @@ -130,6 +143,9 @@ class AnalyticsClient: AnalyticsClientBehaviour {
}

func record(_ event: ClickstreamEvent) throws {
if event.eventType != Event.PresetEvent.CLICKSTREAM_ERROR{
attributeLock.lock()
}
for (key, attribute) in globalAttributes {
event.addGlobalAttribute(attribute, forKey: key)
}
Expand All @@ -147,6 +163,9 @@ class AnalyticsClient: AnalyticsClientBehaviour {
event.setUserAttribute(simpleUserAttributes)
}
try eventRecorder.save(event)
if event.eventType != Event.PresetEvent.CLICKSTREAM_ERROR{
attributeLock.unlock()
}
}

func recordEventError(_ eventError: EventChecker.EventError) {
Expand All @@ -167,13 +186,15 @@ class AnalyticsClient: AnalyticsClientBehaviour {
}

func getSimpleUserAttributes() -> [String: Any] {
attributeLock.lock()
simpleUserAttributes = [:]
simpleUserAttributes[Event.ReservedAttribute.USER_FIRST_TOUCH_TIMESTAMP]
= allUserAttributes[Event.ReservedAttribute.USER_FIRST_TOUCH_TIMESTAMP]
if allUserAttributes.keys.contains(Event.ReservedAttribute.USER_ID) {
simpleUserAttributes[Event.ReservedAttribute.USER_ID]
= allUserAttributes[Event.ReservedAttribute.USER_ID]
}
attributeLock.unlock()
return simpleUserAttributes
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class ClickstreamEvent: AnalyticsPropertiesModel {
private(set) lazy var attributes: [String: AttributeValue] = [:]
private(set) lazy var items: [ClickstreamAttribute] = []
private(set) lazy var userAttributes: [String: Any] = [:]
let attributeLock = NSLock()
let systemInfo: SystemInfo
let netWorkType: String

Expand Down Expand Up @@ -72,7 +73,11 @@ class ClickstreamEvent: AnalyticsPropertiesModel {
}

func setUserAttribute(_ attributes: [String: Any]) {
userAttributes = attributes
attributeLock.lock()
for attr in attributes {
userAttributes[attr.key] = attr.value
}
attributeLock.unlock()
}

func attribute(forKey key: String) -> AttributeValue? {
Expand Down Expand Up @@ -109,9 +114,11 @@ class ClickstreamEvent: AnalyticsPropertiesModel {
if !items.isEmpty {
event["items"] = items
}
attributeLock.lock()
if !userAttributes.isEmpty {
event["user"] = userAttributes
}
attributeLock.unlock()
event["attributes"] = getAttributeObject(from: attributes)
return event
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
@testable import Clickstream
import Foundation
import XCTest

class ClickstreamEventTest: XCTestCase {
let testAppId = "testAppId"
let storage = ClickstreamContextStorage(userDefaults: UserDefaults.standard)
Expand Down
14 changes: 14 additions & 0 deletions Tests/ClickstreamTests/Clickstream/EventRecorderTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,20 @@ class EventRecorderTest: XCTestCase {
XCTAssertEqual("carl", (userAttributes["_user_name"] as! JsonObject)["value"] as! String)
}

func testModifyGlobalUserAttributesWillNotAffectTheEventUserAttributes() throws {
var userInfo = JsonObject()
let currentTimeStamp = Date().millisecondsSince1970
var userNameInfo = JsonObject()
userNameInfo["value"] = "carl"
userNameInfo["set_timestamp"] = currentTimeStamp
userInfo["_user_name"] = userNameInfo
clickstreamEvent.setUserAttribute(userInfo)
userInfo = JsonObject()
userNameInfo["value"] = "mike"
userInfo["_user_name"] = userNameInfo
XCTAssertEqual((clickstreamEvent.userAttributes["_user_name"] as! JsonObject)["value"] as! String, "carl")
}

func testRecordEventWithInvalidAttribute() throws {
clickstreamEvent.addGlobalAttribute(Decimal.nan, forKey: "invalidDecimal")
try eventRecorder.save(clickstreamEvent)
Expand Down
20 changes: 20 additions & 0 deletions Tests/ClickstreamTests/IntegrationTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,26 @@ class IntegrationTest: XCTestCase {
XCTAssertEqual(1, eventCount)
}

func testModifyUserAttributesInMulitThread() throws {
for number in 1 ... 100 {
Task {
ClickstreamAnalytics.addUserAttributes([
"_user_age": 21,
"isFirstOpen": true,
"score": 85.2,
"_user_name": "name\(number)"
])
}
Task {
ClickstreamAnalytics.setUserId("userId\(number)")
}
Task {
ClickstreamAnalytics.recordEvent("testEvent\(number)")
}
}
Thread.sleep(forTimeInterval: 1)
}

// MARK: - Objc test

func testRecordEventForObjc() throws {
Expand Down
Loading