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.

Sign up for GitHub

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: track bq migration for identify profile events with no attributes #702

Closed
wants to merge 1 commit into from
Closed

fix: track bq migration for identify profile events with no attributes #702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions Sources/Migration/DataPipelineMigrationAssistant.swift
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be considered scope creep but i think it would make sense, to cater this as well as its the same issue and can cause the same crash? also would save us from doing another release.

jsonAdapter.fromJsonString(registerPushTaskData.attributesJsonString!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I think we should handle all these cases in this file instead of waiting for issues to be reported. I'm okay if we don't add tests for them in this PR, but I would like to fix possible issues with similar cases in this file before releasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that, too. I don't anticipate that the register device BQ event will ever have a null attributes string because a device token should always be included in the attributes.

However, as we chatted about yesterday, we want to avoid force optional unwraps in the codebase. So, this extra bit of safety could be a good thing.

I do agree, I would prefer to ship this as far of this fix, too. I'll add a new PR to the stack with this change and we can ship them both together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened a PR in the stack for this #704

Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,15 @@ public class DataPipelineMigrationAssistant {
guard let trackTaskData: IdentifyProfileQueueTaskData = jsonAdapter.fromJson(taskData) else {
return false
}
if let attributedString = trackTaskData.attributesJsonString, attributedString.contains("null") {
Copy link
Contributor

@ami-aman ami-aman Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a profile is identified without any attributes, then the task added to BGQ looks like

added queue task data {"attributes_json_string":"null","identifier":"[email protected]"}

In case you want to reproduce the scenario :

  • Change repo branch to v2
  • Run sample app - APN-UIKit for instance
  • Make sure that is a fresh login and the logLevel is .debug for you to easily get the log above
  • Click Generate random login
  • Check the identify log and you will see that the JSON was created with "null"

But I do see that the fix that you made satisfies this condition too so we are good with the changes.

Copy link
Contributor Author

@levibostian levibostian Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing your expertise here. And thanks for sharing the steps.

I think the reason you're seeing the value "null" is a behavior of our sample app's implementation.

In the APN sample app, we are only calling CustomerIO.identify(id: "", body: ""). If the sample app was modified to call CustomerIO.identify(id: ""), I would no longer expect "null" to be the value of attributes_json_string.

Therefore, I think this bug is happening for customers who are calling the identify function without the body param.

Most important thing is that all of these use cases are handled. The test suite is testing it all so we're covered. Thanks for confirming this, too.

migrationHandler.processIdentifyFromBGQ(identifier: trackTaskData.identifier, timestamp: timestamp, body: nil)
return true
}
guard let profileAttributes: [String: Any] = jsonAdapter.fromJsonString(trackTaskData.attributesJsonString!) else {

// If there are no profile attributes or profile attributes not in a valid format, JSON adapter will return nil and we will perform a migration without the profile attributes.
guard let profileAttributesString: String = trackTaskData.attributesJsonString, let profileAttributes: [String: Any] = jsonAdapter.fromJsonString(profileAttributesString) else {
migrationHandler.processIdentifyFromBGQ(identifier: trackTaskData.identifier, timestamp: timestamp, body: nil)
return true
}

migrationHandler.processIdentifyFromBGQ(identifier: trackTaskData.identifier, timestamp: timestamp, body: profileAttributes)

return true
}

Expand Down
20 changes: 19 additions & 1 deletion Tests/Migration/DataPipelineMigrationAssistantTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ class DataPipelineMigrationAssistantTests: UnitTest {

// MARK: getAndProcessTask

func test_givenIdentifyProfileWithoutBody_expectTaskRunAndProcessedDeleted() {
func test_givenIdentifyProfileWithEmptyBody_expectTaskRunAndProcessedDeleted() {
let givenType = QueueTaskType.identifyProfile

let givenCreatedTask = QueueTaskMetadata.random
Expand All @@ -157,6 +157,24 @@ class DataPipelineMigrationAssistantTests: UnitTest {
XCTAssertEqual(backgroundQueueMock.deleteProcessedTaskCallsCount, 1)
}

func test_givenIdentifyProfileWithoutBody_expectTaskRunAndProcessedDeleted() {
let givenType = QueueTaskType.identifyProfile

let givenCreatedTask = QueueTaskMetadata.random

let trackDeliveryMetricData = IdentifyProfileQueueTaskData(identifier: String.random, attributesJsonString: nil)

guard let jsonData = try? JSONEncoder().encode(trackDeliveryMetricData) else {
XCTFail("Failed to create task data")
return
}

backgroundQueueMock.getTaskDetailReturnValue = TaskDetail(data: jsonData, taskType: givenType, timestamp: dateUtilStub.now)

XCTAssertNotNil(migrationAssistant.getAndProcessTask(for: givenCreatedTask, siteId: testSiteId))
XCTAssertEqual(backgroundQueueMock.deleteProcessedTaskCallsCount, 1)
}

func test_givenIdentifyProfileWithBody_expectTaskRunAndProcessedDeleted() {
let givenType = QueueTaskType.identifyProfile
let identifyProfileData = IdentifyProfileQueueTaskData(identifier: String.random, attributesJsonString: "{\"foo\": \"bar\"}")
Expand Down
Loading