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

Jump to bottom

fix: prevent registering device with empty identifier existing BQ tasks #392

Merged
merged 2 commits into from
Oct 11, 2023
Merged

fix: prevent registering device with empty identifier existing BQ tasks #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73e13ef
fix: prevent registering device with empty identifier existing BQ tasks
levibostian Oct 6, 2023
fe3ea42
tests: add tests aound extension added
levibostian Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions Sources/Common/Background Queue/QueueRunner.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,13 @@ private extension CioQueueRunner {
return onComplete(failureIfDontDecodeTaskData)
}

// If a HTTP request is made to register device token with an empty identifier, either (1) a 302 will occur or (2) a "devices" profile will be created in a CIO workspace. Neither are ideal.
// The SDK has logic to prevent *new* register device tasks being added to the BQ if identifier is empty but existing tasks in the BQ will still occur.
// This logic is to clear those tasks from the BQ.
Comment on lines +95 to +97
Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully, this comment explains why this PR was made and why this change is needed.

If you are thinking, "BQ tasks expire after 3 days. These tasks should be deleted?". For registering push tokens, these tasks never expire.

This group start requirement can be removed because Track API will return a 200 if you try to delete a device token for a device that does not exist. However, you cannot modify a BQ task once it's in the BQ. Therefore, this bug would not be fixed by making group start nil for register device token.

if taskData.profileIdentifier.isBlankOrEmpty() {
return onComplete(.success(()))
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason, we return success for this task, which will eventually also delete instead of just deleting?

The register device token is blocked by the Identify task, so it should run in the first place, but in case if identify task has been run and was faulty e.g. empty identifier then why not just delete this task?

Copy link
Member Author

Choose a reason for hiding this comment

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

is there a reason, we return success for this task, which will eventually also delete instead of just deleting?

You are correct. The reason for returning success is to tell the BQ to delete the task.

but in case if identify task has been run and was faulty e.g. empty identifier

I am not sure if it would be faulty? The problem that we have been experiencing is HTTP requests with empty identifier ID for this register device endpoint. I don't recall the SDK having issues with identifying a profile with the API with an empty identifier. Therefore, I don't think we need to modify identify profile BQ tasks?

Are you suggesting that we modify the solution to a SDK migration where we iterate the BQ and delete tasks from it?

Assuming that is what your comment is suggesting...From my experience building BQs, I have learned that BQ code gets complex quickly when you begin modifying tasks that are already in the BQ. An approach I have found to be less complex is to do the modify logic when that BQ task is executing. Therfore, that's where this solution comes from. When the BQ task runs, tell the BQ to delete it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was suggesting, something like this?

if taskData.profileIdentifier.isBlankOrEmpty()
queuStore.delete(taskID)
return

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see.

I would not recommend using the code inside of this function since the queue runner will then fail when it attempts to delete the task from the queue storage. By returning success here, we are telling the queue storage to delete it for us.

A 1 time SDK data migration would be the approach that I would suggest as an alternative solution to this PR. I hesitate because of my suggestion to not modify BQ tasks once they are in the BQ and prefer solutions like this. But, technically, a migration would work here.

}

performHttpRequest(
endpoint: .registerDevice(identifier: taskData.profileIdentifier),
requestBody: taskData.attributesJsonString?.data,
Expand Down
4 changes: 4 additions & 0 deletions Sources/Common/Extensions/ResultExtensions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,8 @@ extension Result {
default: return nil
}
}

var isSuccess: Bool {
success != nil
}
}
36 changes: 36 additions & 0 deletions Tests/Common/Background Queue/QueueRunnerTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
@testable import CioInternalCommon
import Foundation
import SharedTests
import XCTest

class QueueRunnerTest: UnitTest {
private var runner: CioQueueRunner!

private let httpClientMock = HttpClientMock()

override func setUp() {
super.setUp()

runner = CioQueueRunner(jsonAdapter: jsonAdapter, logger: log, httpClient: httpClientMock, hooksManager: diGraph.hooksManager, sdkConfig: sdkConfig)
}

// MARK: registerPushToken

func test_registerPushToken_givenIdentifierEmpty_expectHttpRequestNotMade_expectBQDeletesTask() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have a test for the failure of the result as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume you're suggesting adding a test for when the endpoint returns back a 401, 500, or some other HTTP failure?

Assuming that is what you're saying, the logic for this endpoint is identical to all other endpoints in how to handle HTTP failures. Therfore, I find a test like that to be not as valuable because it's duplicated logic for all endpoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Result class seems to be added for this PR, and codecov also points out that the failure variable isn't tested for this class. So even if the logic might be similar for other endpoints, we still would have not tested this newly added class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good call-out.

I added a function to Swift's built-in Result class. Looking at the diff, I do see that I didn't name the file correctly and I did not write tests for the added function.

I just pushed a commit taking care of those things. Thanks for bringing that up.

let givenRegisterTokenTask = RegisterPushNotificationQueueTaskData(profileIdentifier: " ", attributesJsonString: nil)

let givenQueueTask = QueueTask(storageId: .random, type: QueueTaskType.registerPushToken.rawValue, data: jsonAdapter.toJson(givenRegisterTokenTask)!, runResults: QueueTaskRunResults(totalRuns: 0))

let expectToCompleteRunning = expectation(description: "expect to complete running")
var actualResult: Result<Void, HttpRequestError>!
runner.runTask(givenQueueTask) { result in
actualResult = result
expectToCompleteRunning.fulfill()
}

waitForExpectations()

XCTAssertTrue(actualResult.isSuccess) // the BQ will delete the task if http was successful
XCTAssertFalse(httpClientMock.mockCalled)
}
}
20 changes: 20 additions & 0 deletions Tests/Common/Extensions/ResultExtensionsTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
@testable import CioInternalCommon
import Foundation
import SharedTests
import XCTest

class ResultExtensionsTest: UnitTest {
// MARK: isSuccess

func test_givenSuccess_expectIsSuccessTrue() {
let result: Result<String, Error> = .success("hello")

XCTAssertTrue(result.isSuccess)
}

func test_givenFailure_expectIsSuccessFalse() {
let result: Result<String, Error> = .failure(HttpRequestError.cancelled)

XCTAssertFalse(result.isSuccess)
}
}