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

levibostian · 2023-10-06T16:11:49Z

Quick bug fix for customer to prevent /api/v1/customers//devices/ (empty profile identifier) requests from being made.

github-actions · 2023-10-06T16:12:05Z

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.

CocoaPods-FCM: levi/empty-identifier-registerdevice (1697029485)
APN-UIKit: levi/empty-identifier-registerdevice (1697029448)

levibostian · 2023-10-06T16:14:47Z

Sources/Common/Background Queue/QueueRunner.swift

+        // 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.


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.

the queue skips deleting tasks that are the start of a group.

register device token tasks start a group. Because delete device token tasks cannot delete a token if it's never registered.

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.

codecov · 2023-10-06T16:30:33Z

Codecov Report

Merging #392 (fe3ea42) into main (b0f309f) will decrease coverage by 0.55%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #392      +/-   ##
==========================================
- Coverage   60.07%   59.52%   -0.55%     
==========================================
  Files         119      119              
  Lines        4556     4566      +10     
==========================================
- Hits         2737     2718      -19     
- Misses       1819     1848      +29

Files	Coverage Δ
Sources/Common/Background Queue/QueueRunner.swift	`69.07% <100.00%> (+2.40%)`	⬆️
Sources/Common/Extensions/ResultExtensions.swift	`100.00% <100.00%> (ø)`

... and 5 files with indirect coverage changes

Shahroz16 · 2023-10-09T11:32:00Z

Sources/Common/Background Queue/QueueRunner.swift

+        // 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.
+        if taskData.profileIdentifier.isBlankOrEmpty() {
+            return onComplete(.success(()))


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?

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.

I was suggesting, something like this?

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

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.

Shahroz16 · 2023-10-09T11:36:28Z

Tests/Common/Background Queue/QueueRunnerTest.swift

+
+    // MARK: registerPushToken
+
+    func test_registerPushToken_givenIdentifierEmpty_expectHttpRequestNotMade_expectBQDeletesTask() {


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

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.

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.

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.

Shahroz16

Approving as these aren't blockers but rather different approaches.

Shahroz16 · 2023-10-11T10:58:58Z

Tests/Common/Background Queue/QueueRunnerTest.swift

+
+    // MARK: registerPushToken
+
+    func test_registerPushToken_givenIdentifierEmpty_expectHttpRequestNotMade_expectBQDeletesTask() {


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.

Shahroz16 · 2023-10-11T11:00:25Z

Sources/Common/Background Queue/QueueRunner.swift

+        // 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.
+        if taskData.profileIdentifier.isBlankOrEmpty() {
+            return onComplete(.success(()))


I was suggesting, something like this?

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

## [2.8.3](2.8.2...2.8.3) (2023-10-11) ### Bug Fixes * prevent registering device with empty identifier existing BQ tasks ([#392](#392)) ([867619f](867619f))

fix: prevent registering device with empty identifier existing BQ tasks

73e13ef

levibostian requested a review from a team October 6, 2023 16:12

levibostian commented Oct 6, 2023

View reviewed changes

levibostian marked this pull request as draft October 6, 2023 16:18

levibostian removed the request for review from a team October 6, 2023 16:19

levibostian marked this pull request as ready for review October 6, 2023 16:34

levibostian requested a review from a team October 6, 2023 16:34

Shahroz16 reviewed Oct 9, 2023

View reviewed changes

levibostian requested a review from Shahroz16 October 10, 2023 15:25

Shahroz16 approved these changes Oct 11, 2023

View reviewed changes

tests: add tests aound extension added

fe3ea42

levibostian merged commit 867619f into main Oct 11, 2023
10 checks passed

levibostian deleted the levi/empty-identifier-registerdevice branch October 11, 2023 13:24

github-actions bot pushed a commit that referenced this pull request Oct 11, 2023

chore: prepare for 2.8.3

31935d8

## [2.8.3](2.8.2...2.8.3) (2023-10-11) ### Bug Fixes * prevent registering device with empty identifier existing BQ tasks ([#392](#392)) ([867619f](867619f))

levibostian mentioned this pull request Oct 11, 2023

fix: remove BQ tasks register device with empty profile identifier customerio/customerio-reactnative#207

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

levibostian commented Oct 6, 2023 •

edited

github-actions bot commented Oct 6, 2023 •

edited

levibostian Oct 6, 2023 •

edited

codecov bot commented Oct 6, 2023 •

edited

Shahroz16 Oct 9, 2023

levibostian Oct 10, 2023

Shahroz16 Oct 11, 2023

levibostian Oct 11, 2023

Shahroz16 Oct 9, 2023

levibostian Oct 10, 2023

Shahroz16 Oct 11, 2023

levibostian Oct 11, 2023

Shahroz16 left a comment

Shahroz16 Oct 11, 2023

Shahroz16 Oct 11, 2023


		// MARK: registerPushToken

		func test_registerPushToken_givenIdentifierEmpty_expectHttpRequestNotMade_expectBQDeletesTask() {

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

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

Conversation

levibostian commented Oct 6, 2023 • edited

github-actions bot commented Oct 6, 2023 • edited

Sample app builds 📱

levibostian Oct 6, 2023 • edited

Choose a reason for hiding this comment

codecov bot commented Oct 6, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shahroz16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levibostian commented Oct 6, 2023 •

edited

github-actions bot commented Oct 6, 2023 •

edited

levibostian Oct 6, 2023 •

edited

codecov bot commented Oct 6, 2023 •

edited