From 6d3e3928a8e06bee9e78e7fb3b374326a83458ee Mon Sep 17 00:00:00 2001 From: Joao Dordio Date: Wed, 26 Aug 2026 00:24:49 +0100 Subject: [PATCH] [SDK-675] Restore offline queueing for disableDevice and registerDeviceToken users/disableDevice was offline-queueable until 3.7.0, when it was removed from offlineApiSet for cross-SDK list parity. iOS later added queueing for it, Android never restored it, which left Android as the only SDK that silently drops a device disable made while the network is down. Adds both users/disableDevice and users/registerDeviceToken back to offlineApiSet. Register is queued alongside disable deliberately: the queue drains in scheduledAt order, so a logout-then-login sequence replays as disable-then-register and leaves the device enabled. Queueing the disable on its own would let a stale disable land after the new user registered and kill a live registration, because the backend merge is last-write-wins. A queued disable is also preserved across logout, via deleteAllTasksExcept, so it still reaches the user it was created for rather than being purged with the rest of the queue. Discarded tasks now settle their handlers instead of never calling back. The setEmail/setUserId completion handlers travel with the queued registerDeviceToken, so logging in as a different user used to strand them and an app dismissing a spinner in that callback would wait forever. The reset is identity-guarded so it only clears the handler pair the registration was created with, otherwise it would drop the incoming login's handlers. checkstyle: IterableApi.java sat at 1999 lines against the default 2000 cap, so any change touching it breaks the build. Suppressed for that one file rather than raising the global limit. The real split is tracked in SDK-677. --- CHANGELOG.md | 4 + checkstyle.xml | 10 + .../com/iterable/iterableapi/IterableApi.java | 23 +- .../iterableapi/IterableTaskStorage.java | 59 ++- .../iterableapi/OfflineRequestProcessor.java | 65 ++- .../OfflineDisableDeviceQueueTest.java | 222 +++++++++++ .../OfflinePurgedRequestCallbackTest.java | 369 ++++++++++++++++++ .../OfflineRequestProcessorTest.java | 56 ++- 8 files changed, 788 insertions(+), 20 deletions(-) create mode 100644 iterableapi/src/test/java/com/iterable/iterableapi/OfflineDisableDeviceQueueTest.java create mode 100644 iterableapi/src/test/java/com/iterable/iterableapi/OfflinePurgedRequestCallbackTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d40ce8f5..aef8d05bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] +### Fixed +- Restored offline support for `disablePush()`. When offline mode is enabled, a `users/disableDevice` request made while the network is unavailable is once again persisted and retried instead of being dropped. This behaviour shipped in 3.5.16 and regressed in 3.7.0, leaving Android as the only SDK that silently lost a device disable when the network was down. A queued `disableDevice` is now also preserved across logout (`setEmail`/`setUserId` to a different user), so the disable still reaches the user it was created for. +- `users/registerDeviceToken` is now queued in offline mode as well, matching the iOS SDK. Push registration made while the network is unavailable is retried instead of being lost, and because the offline queue drains in `scheduledAt` order, a logout-then-login sequence replays as disable-then-register and leaves the device enabled. Note that the offline queue only drains while the app is in the foreground, so a Firebase token refresh received in the background is now sent on the next foreground rather than immediately. +- A queued request that is discarded before it can be sent now calls its failure handler instead of never calling back at all. This matters most for the completion handlers passed to `setEmail`/`setUserId`: they travel with the queued `users/registerDeviceToken`, so logging in as a different user used to strand them, and an app that dismisses a login spinner in that callback would wait forever. The failure reason states that the request was discarded because the user logged out. ## [3.10.0] ### Added diff --git a/checkstyle.xml b/checkstyle.xml index f0fada454..69ecfdb7a 100644 --- a/checkstyle.xml +++ b/checkstyle.xml @@ -8,6 +8,16 @@ + + + + + + diff --git a/iterableapi/src/main/java/com/iterable/iterableapi/IterableApi.java b/iterableapi/src/main/java/com/iterable/iterableapi/IterableApi.java index 95eb354ba..89c70e5a8 100644 --- a/iterableapi/src/main/java/com/iterable/iterableapi/IterableApi.java +++ b/iterableapi/src/main/java/com/iterable/iterableapi/IterableApi.java @@ -766,12 +766,13 @@ private IterableHelper.SuccessHandler getSuccessHandler() { IterableHelper.SuccessHandler wrappedSuccessHandler = null; if (_setUserSuccessCallbackHandler != null || (config.enableUnknownUserActivation && getVisitorUsageTracked() && config.identityResolution.getReplayOnVisitorToKnown())) { final IterableHelper.SuccessHandler originalSuccessHandler = _setUserSuccessCallbackHandler; + final IterableHelper.FailureHandler pairedFailureHandler = _setUserFailureCallbackHandler; wrappedSuccessHandler = data -> { trackConsentOnDeviceRegistration(); if (originalSuccessHandler != null) { originalSuccessHandler.onSuccess(data); - resetCallbackHandlers(); + resetCallbackHandlers(originalSuccessHandler, pairedFailureHandler); } }; } @@ -782,12 +783,13 @@ private IterableHelper.FailureHandler getFailureHandler() { IterableHelper.FailureHandler wrappedFailureHandler = null; if (_setUserFailureCallbackHandler != null || (config.enableUnknownUserActivation && getVisitorUsageTracked() && config.identityResolution.getReplayOnVisitorToKnown())) { final IterableHelper.FailureHandler originalFailureHandler = _setUserFailureCallbackHandler; + final IterableHelper.SuccessHandler pairedSuccessHandler = _setUserSuccessCallbackHandler; wrappedFailureHandler = (reason, data) -> { trackConsentOnDeviceRegistration(); if (originalFailureHandler != null) { originalFailureHandler.onFailure(reason, data); - resetCallbackHandlers(); + resetCallbackHandlers(pairedSuccessHandler, originalFailureHandler); } }; } @@ -798,6 +800,23 @@ private void resetCallbackHandlers() { _setUserFailureCallbackHandler = null; _setUserSuccessCallbackHandler = null; } + + /** + * Clears the handler pair a device registration was started with, and only that pair. A + * registration outcome can arrive after the next setEmail/setUserId has installed its own + * handlers: it always could if the request was slow, and it does so routinely now that a queued + * registration is settled by the logout that discards it. Clearing unconditionally would drop + * the incoming login's handlers, so its own callback would never fire. + */ + private void resetCallbackHandlers(@Nullable IterableHelper.SuccessHandler successHandler, + @Nullable IterableHelper.FailureHandler failureHandler) { + if (_setUserSuccessCallbackHandler == successHandler) { + _setUserSuccessCallbackHandler = null; + } + if (_setUserFailureCallbackHandler == failureHandler) { + _setUserFailureCallbackHandler = null; + } + } //endregion //region SDK initialization diff --git a/iterableapi/src/main/java/com/iterable/iterableapi/IterableTaskStorage.java b/iterableapi/src/main/java/com/iterable/iterableapi/IterableTaskStorage.java index aa8f50211..051d6c59a 100644 --- a/iterableapi/src/main/java/com/iterable/iterableapi/IterableTaskStorage.java +++ b/iterableapi/src/main/java/com/iterable/iterableapi/IterableTaskStorage.java @@ -317,13 +317,64 @@ IterableTask getNextScheduledTaskNotRequiringJwt(ApiEndpointClassification class /** * Deletes all the entries from the OfflineTask table. + * + * @return ids of the deleted tasks, so their parked callbacks can be settled */ - void deleteAllTasks() { + @NonNull + ArrayList deleteAllTasks() { if (!isDatabaseReady()) { - return; + return new ArrayList<>(); } - int numberOfRowsDeleted = database.delete(ITERABLE_TASK_TABLE_NAME, null, null); - IterableLogger.v(TAG, "Deleted " + numberOfRowsDeleted + " offline tasks"); + ArrayList deletedTaskIds = deleteAndReturnIds(null, null); + IterableLogger.v(TAG, "Deleted " + deletedTaskIds.size() + " offline tasks"); + return deletedTaskIds; + } + + /** + * Deletes all the entries from the OfflineTask table except those with the given name. + * Task names are the request's resource path, set when the task is scheduled. + * + * @param name name of the tasks to preserve + * @return ids of the deleted tasks, so their parked callbacks can be settled + */ + @NonNull + ArrayList deleteAllTasksExcept(@NonNull String name) { + if (!isDatabaseReady()) { + return new ArrayList<>(); + } + // NAME is nullable in the schema, and `NAME != ?` evaluates to NULL rather than true + // for a null name, so unnamed rows would survive the purge without the IS NULL branch. + ArrayList deletedTaskIds = deleteAndReturnIds( + NAME + " IS NULL OR " + NAME + " != ?", new String[]{name}); + IterableLogger.v(TAG, "Deleted " + deletedTaskIds.size() + " offline tasks, preserved " + name); + return deletedTaskIds; + } + + /** + * Deletes the matching rows and returns their ids. Only the id column is read, so knowing which + * rows a bulk delete removed costs one extra query rather than deserializing every queued + * request. The query and the delete share a transaction, so a task created in between cannot be + * deleted without its parked callback being settled. + */ + @NonNull + private ArrayList deleteAndReturnIds(@Nullable String selection, @Nullable String[] selectionArgs) { + ArrayList taskIds = new ArrayList<>(); + database.beginTransaction(); + try { + Cursor cursor = database.query(ITERABLE_TASK_TABLE_NAME, new String[]{TASK_ID}, + selection, selectionArgs, null, null, null); + if (cursor.moveToFirst()) { + do { + taskIds.add(cursor.getString(0)); + } while (cursor.moveToNext()); + } + cursor.close(); + database.delete(ITERABLE_TASK_TABLE_NAME, selection, selectionArgs); + database.setTransactionSuccessful(); + } finally { + database.endTransaction(); + } + return taskIds; } /** diff --git a/iterableapi/src/main/java/com/iterable/iterableapi/OfflineRequestProcessor.java b/iterableapi/src/main/java/com/iterable/iterableapi/OfflineRequestProcessor.java index ea5de7e45..32dee1f82 100644 --- a/iterableapi/src/main/java/com/iterable/iterableapi/OfflineRequestProcessor.java +++ b/iterableapi/src/main/java/com/iterable/iterableapi/OfflineRequestProcessor.java @@ -1,6 +1,8 @@ package com.iterable.iterableapi; import android.content.Context; +import android.os.Handler; +import android.os.Looper; import androidx.annotation.MainThread; import androidx.annotation.NonNull; @@ -10,9 +12,12 @@ import org.json.JSONException; import org.json.JSONObject; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Set; class OfflineRequestProcessor implements RequestProcessor { @@ -34,7 +39,13 @@ class OfflineRequestProcessor implements RequestProcessor { IterableConstants.ENDPOINT_UPDATE_CART, IterableConstants.ENDPOINT_TRACK_EMBEDDED_RECEIVED, IterableConstants.ENDPOINT_TRACK_EMBEDDED_CLICK, - IterableConstants.ENDPOINT_TRACK_EMBEDDED_SESSION + IterableConstants.ENDPOINT_TRACK_EMBEDDED_SESSION, + IterableConstants.ENDPOINT_DISABLE_DEVICE, + // Queued alongside disableDevice so a logout-then-login sequence replays in + // scheduledAt order as disable-then-register. Queueing the disable on its own + // would let a stale disable land after the new user registered and silently kill + // a live push registration, because the backend merge is last-write-wins. + IterableConstants.ENDPOINT_REGISTER_DEVICE_TOKEN )); OfflineRequestProcessor(Context context) { @@ -103,15 +114,26 @@ public void processPostRequest(@Nullable String apiKey, @NonNull String resource @Override public void onLogout(Context context) { - taskStorage.deleteAllTasks(); + // A queued disableDevice is the logout itself retrying, so it has to outlive the purge. + // It carries the identity it was created with, so it still targets the outgoing user. + taskScheduler.onTasksPurged(taskStorage.deleteAllTasksExcept(IterableConstants.ENDPOINT_DISABLE_DEVICE)); } boolean isRequestOfflineCompatible(String baseUrl) { return offlineApiSet.contains(baseUrl); } + + @VisibleForTesting + static Set getOfflineApiSet() { + return Collections.unmodifiableSet(offlineApiSet); + } } class TaskScheduler implements IterableTaskRunner.TaskCompletedListener { + @VisibleForTesting + static final String PURGED_ON_LOGOUT_REASON = + "Request was discarded before it could be sent because the user logged out"; + static HashMap successCallbackMap = new HashMap<>(); static HashMap failureCallbackMap = new HashMap<>(); private final IterableTaskStorage taskStorage; @@ -142,6 +164,45 @@ void scheduleTask(IterableApiRequest request, @Nullable IterableHelper.SuccessHa failureCallbackMap.put(taskId, onFailure); } + /** + * Settles the callbacks parked for tasks that were deleted from the queue before they could run. + * {@link #onTaskCompleted} is only ever reached from {@link IterableTaskRunner}, and a deleted + * task is never run, so without this the app's handler never fires and the map entries live for + * the rest of the process. Most visibly, {@code setEmail}'s completion handlers travel with the + * queued {@code registerDeviceToken}, so an app dismissing a login spinner in that callback + * would wait forever. + * + * @param taskIds ids of the tasks the purge removed + */ + void onTasksPurged(@NonNull List taskIds) { + // Unparked before anything is notified, so a handler that logs back in cannot see, re-fire + // or re-purge an entry this call already owns. + final List orphanedHandlers = new ArrayList<>(); + for (String taskId : taskIds) { + IterableHelper.FailureHandler onFailure = failureCallbackMap.remove(taskId); + successCallbackMap.remove(taskId); + if (onFailure != null) { + orphanedHandlers.add(onFailure); + } + } + if (orphanedHandlers.isEmpty()) { + return; + } + + // Same thread and looper a real failure would arrive on. It also puts the notification after + // the purge and after the logout that triggered it, so app code re-entering the SDK from the + // handler runs against a settled queue. + new Handler(Looper.getMainLooper()).post(() -> { + for (IterableHelper.FailureHandler onFailure : orphanedHandlers) { + try { + onFailure.onFailure(PURGED_ON_LOGOUT_REASON, null); + } catch (Exception e) { + IterableLogger.e("TaskScheduler", "Failed to notify a discarded request's failure handler", e); + } + } + }); + } + @MainThread @Override public void onTaskCompleted(String taskId, IterableTaskRunner.TaskResult result, IterableApiResponse response) { diff --git a/iterableapi/src/test/java/com/iterable/iterableapi/OfflineDisableDeviceQueueTest.java b/iterableapi/src/test/java/com/iterable/iterableapi/OfflineDisableDeviceQueueTest.java new file mode 100644 index 000000000..4bc43a213 --- /dev/null +++ b/iterableapi/src/test/java/com/iterable/iterableapi/OfflineDisableDeviceQueueTest.java @@ -0,0 +1,222 @@ +package com.iterable.iterableapi; + +import android.content.Context; + +import androidx.annotation.NonNull; + +import com.iterable.iterableapi.unit.TestRunner; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.util.ArrayList; +import java.util.concurrent.TimeUnit; + +import okhttp3.mockwebserver.Dispatcher; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; + +import static android.os.Looper.getMainLooper; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.robolectric.Shadows.shadowOf; + +/** + * End-to-end coverage for the offline device registration queue. + * + * These tests deliberately go through {@link IterableApi#disablePush()} / + * {@link IterableApi#registerForPush()} and the real {@link OfflineRequestProcessor} and + * {@link IterableTaskStorage} rather than inserting rows by hand. Hand-inserted rows prove + * nothing about whether the endpoint is actually offline compatible, which is why the 3.7.0 + * regression went unnoticed. + */ +@RunWith(TestRunner.class) +public class OfflineDisableDeviceQueueTest extends BaseTest { + + private static final String TEST_TOKEN = "testToken"; + private static final String DISABLE_PATH = "/" + IterableConstants.ENDPOINT_DISABLE_DEVICE; + private static final String REGISTER_PATH = "/" + IterableConstants.ENDPOINT_REGISTER_DEVICE_TOKEN; + + private MockWebServer server; + private IterableTaskStorage taskStorage; + private IterableTaskRunner taskRunner; + private IterableNetworkConnectivityManager mockNetworkConnectivityManager; + private IterablePushRegistrationTask.Util.UtilImpl originalPushRegistrationUtil; + + @Before + public void setUp() throws Exception { + server = new MockWebServer(); + server.setDispatcher(new Dispatcher() { + @NonNull + @Override + public MockResponse dispatch(@NonNull RecordedRequest request) { + return new MockResponse().setResponseCode(200).setBody("{}"); + } + }); + IterableApi.overrideURLEndpointPath(server.url("").toString()); + + originalPushRegistrationUtil = IterablePushRegistrationTask.Util.instance; + IterablePushRegistrationTask.Util.UtilImpl pushRegistrationUtilMock = + mock(IterablePushRegistrationTask.Util.UtilImpl.class); + when(pushRegistrationUtilMock.getSenderId(any(Context.class))).thenReturn("12345"); + when(pushRegistrationUtilMock.getFirebaseToken()).thenReturn(TEST_TOKEN); + IterablePushRegistrationTask.Util.instance = pushRegistrationUtilMock; + + IterableTestUtils.createIterableApi(); + + taskStorage = IterableTaskStorage.sharedInstance(getContext()); + taskStorage.deleteAllTasks(); + IterableApi.getInstance().apiClient.setOfflineProcessingEnabled(true); + + // A runner we can drive deterministically over the same storage the SDK writes to. + // The processor builds its own runner, but its network thread is never idled here, so + // it cannot flush behind our back. + mockNetworkConnectivityManager = mock(IterableNetworkConnectivityManager.class); + IterableActivityMonitor mockActivityMonitor = mock(IterableActivityMonitor.class); + when(mockActivityMonitor.isInForeground()).thenReturn(true); + HealthMonitor mockHealthMonitor = mock(HealthMonitor.class); + when(mockHealthMonitor.canProcess()).thenReturn(true); + taskRunner = new IterableTaskRunner(taskStorage, mockActivityMonitor, + mockNetworkConnectivityManager, mockHealthMonitor, new ApiEndpointClassification()); + } + + @After + public void tearDown() throws Exception { + // IterableTaskStorage is a singleton that outlives the test, so a runner left + // registered here would keep flushing tasks created by the next test. + taskStorage.removeDatabaseStatusListener(taskRunner); + taskStorage.deleteAllTasks(); + IterableApi.getInstance().apiClient.setOfflineProcessingEnabled(false); + IterablePushRegistrationTask.Util.instance = originalPushRegistrationUtil; + server.shutdown(); + server = null; + } + + @Test + public void testDisablePushWhileOfflineIsQueuedSurvivesLogoutAndIsSent() throws Exception { + when(mockNetworkConnectivityManager.isConnected()).thenReturn(false); + + IterableApi.getInstance().disablePush(); + shadowOf(getMainLooper()).idle(); + + assertEquals("disablePush() while offline should persist a task", 1, taskStorage.getNumberOfTasks()); + assertEquals(IterableConstants.ENDPOINT_DISABLE_DEVICE, onlyTask().name); + + drainTaskRunner(); + assertNull("nothing should be sent while the network is down", nextDeviceRequestPath(200)); + assertEquals(1, taskStorage.getNumberOfTasks()); + + // Something unrelated in the queue, so the purge has something to actually delete. + IterableApi.getInstance().track("offlineEvent"); + shadowOf(getMainLooper()).idle(); + assertEquals(2, taskStorage.getNumberOfTasks()); + + IterableApi.getInstance().apiClient.onLogout(); + + assertEquals("logout should keep the queued disable and drop everything else", + 1, taskStorage.getNumberOfTasks()); + assertEquals(IterableConstants.ENDPOINT_DISABLE_DEVICE, onlyTask().name); + + when(mockNetworkConnectivityManager.isConnected()).thenReturn(true); + drainTaskRunner(); + + assertEquals("the preserved disable should be sent once back online", + DISABLE_PATH, nextDeviceRequestPath(1000)); + assertEquals(0, taskStorage.getNumberOfTasks()); + } + + @Test + public void testQueuedDisableIsReplayedBeforeALaterRegister() throws Exception { + when(mockNetworkConnectivityManager.isConnected()).thenReturn(false); + + IterableApi.getInstance().disablePush(); + shadowOf(getMainLooper()).idle(); + IterableApi.getInstance().apiClient.onLogout(); + + // scheduledAt has millisecond resolution and `order by scheduled` has no tie-break, + // so put a real gap between the two tasks rather than relying on wall-clock luck. + Thread.sleep(5); + + // registerDeviceToken is dispatched onto its own thread by IterableApi, so wait for + // the row rather than idling loopers. + IterableApi.getInstance().registerForPush(); + awaitTaskCount(2); + + when(mockNetworkConnectivityManager.isConnected()).thenReturn(true); + drainTaskRunner(); + + // FIFO by scheduledAt is what stops the stale disable from undoing the new + // registration: the disable was queued first, so it has to go out first. + assertEquals(DISABLE_PATH, nextDeviceRequestPath(1000)); + assertEquals(REGISTER_PATH, nextDeviceRequestPath(1000)); + assertEquals(0, taskStorage.getNumberOfTasks()); + } + + /** + * NAME is nullable in the schema, and SQL three-valued logic makes `name != ?` evaluate to NULL + * rather than true for a null name, so a bare inequality would leave unnamed rows behind forever. + * A null name is not reachable through the SDK today, which is why it needs asserting. + */ + @Test + public void testLogoutPurgeAlsoRemovesRowsWithNoName() throws Exception { + String unnamedTaskId = taskStorage.createTask(null, IterableTaskType.API, "{}"); + String disableTaskId = taskStorage.createTask(IterableConstants.ENDPOINT_DISABLE_DEVICE, IterableTaskType.API, "{}"); + assertEquals(2, taskStorage.getNumberOfTasks()); + + taskStorage.deleteAllTasksExcept(IterableConstants.ENDPOINT_DISABLE_DEVICE); + + ArrayList remaining = taskStorage.getAllTaskIds(); + assertEquals("only the disable is spared; the null-named row must go", 1, remaining.size()); + assertEquals(disableTaskId, remaining.get(0)); + assertNull(taskStorage.getTask(unnamedTaskId)); + } + + /** + * Returns the path of the next device registration request, skipping the unrelated traffic + * (in-app sync, remote config) that SDK initialization produces. + */ + private String nextDeviceRequestPath(long timeoutMs) throws InterruptedException { + long deadline = System.currentTimeMillis() + timeoutMs; + do { + RecordedRequest request = server.takeRequest(timeoutMs, TimeUnit.MILLISECONDS); + if (request == null) { + return null; + } + String path = request.getPath(); + if (DISABLE_PATH.equals(path) || REGISTER_PATH.equals(path)) { + return path; + } + } while (System.currentTimeMillis() < deadline); + return null; + } + + private void awaitTaskCount(long expected) throws InterruptedException { + long deadline = System.currentTimeMillis() + 2000; + while (System.currentTimeMillis() < deadline) { + shadowOf(getMainLooper()).idle(); + if (taskStorage.getNumberOfTasks() == expected) { + return; + } + Thread.sleep(10); + } + assertEquals(expected, taskStorage.getNumberOfTasks()); + } + + private IterableTask onlyTask() { + ArrayList ids = taskStorage.getAllTaskIds(); + assertEquals(1, ids.size()); + return taskStorage.getTask(ids.get(0)); + } + + private void drainTaskRunner() { + taskRunner.onTaskCreated(null); + shadowOf(taskRunner.handler.getLooper()).idle(); + shadowOf(getMainLooper()).idle(); + } +} diff --git a/iterableapi/src/test/java/com/iterable/iterableapi/OfflinePurgedRequestCallbackTest.java b/iterableapi/src/test/java/com/iterable/iterableapi/OfflinePurgedRequestCallbackTest.java new file mode 100644 index 000000000..06a2f4245 --- /dev/null +++ b/iterableapi/src/test/java/com/iterable/iterableapi/OfflinePurgedRequestCallbackTest.java @@ -0,0 +1,369 @@ +package com.iterable.iterableapi; + +import android.content.Context; + +import androidx.annotation.NonNull; + +import com.iterable.iterableapi.unit.TestRunner; + +import org.json.JSONObject; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import okhttp3.mockwebserver.Dispatcher; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; + +import static android.os.Looper.getMainLooper; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.robolectric.Shadows.shadowOf; + +/** + * Covers what happens to a queued request's completion handlers when the request is deleted from the + * offline queue before it can be sent. + * + * The handlers are parked in {@link TaskScheduler}'s static maps at schedule time and are only ever + * settled from {@link IterableTaskRunner}, which never runs a deleted task. Now that + * {@code users/registerDeviceToken} is queued, the handlers an app passes to {@code setEmail} and + * {@code setUserId} travel into that queue, so a logout purge used to strand them. + */ +@RunWith(TestRunner.class) +public class OfflinePurgedRequestCallbackTest extends BaseTest { + + private static final String TEST_TOKEN = "testToken"; + + private MockWebServer server; + private IterableTaskStorage taskStorage; + private IterableTaskRunner taskRunner; + private TaskScheduler scheduler; + private OfflineRequestProcessor processor; + private IterableNetworkConnectivityManager mockNetworkConnectivityManager; + private IterablePushRegistrationTask.Util.UtilImpl originalPushRegistrationUtil; + + @Before + public void setUp() { + server = new MockWebServer(); + server.setDispatcher(new Dispatcher() { + @NonNull + @Override + public MockResponse dispatch(@NonNull RecordedRequest request) { + return new MockResponse().setResponseCode(200).setBody("{}"); + } + }); + IterableApi.overrideURLEndpointPath(server.url("").toString()); + + originalPushRegistrationUtil = IterablePushRegistrationTask.Util.instance; + IterablePushRegistrationTask.Util.UtilImpl pushRegistrationUtilMock = + mock(IterablePushRegistrationTask.Util.UtilImpl.class); + when(pushRegistrationUtilMock.getSenderId(any(Context.class))).thenReturn("12345"); + when(pushRegistrationUtilMock.getFirebaseToken()).thenReturn(TEST_TOKEN); + IterablePushRegistrationTask.Util.instance = pushRegistrationUtilMock; + + taskStorage = IterableTaskStorage.sharedInstance(getContext()); + taskStorage.deleteAllTasks(); + // Static and shared with every other test in the JVM. + TaskScheduler.successCallbackMap.clear(); + TaskScheduler.failureCallbackMap.clear(); + + // A runner we can drive deterministically over the same storage the SDK writes to. Its + // network thread is never idled except through drainTaskRunner(), so it cannot flush behind + // an assertion's back. + mockNetworkConnectivityManager = mock(IterableNetworkConnectivityManager.class); + IterableActivityMonitor mockActivityMonitor = mock(IterableActivityMonitor.class); + when(mockActivityMonitor.isInForeground()).thenReturn(true); + HealthMonitor mockHealthMonitor = mock(HealthMonitor.class); + when(mockHealthMonitor.canProcess()).thenReturn(true); + when(mockHealthMonitor.canSchedule()).thenReturn(true); + taskRunner = new IterableTaskRunner(taskStorage, mockActivityMonitor, + mockNetworkConnectivityManager, mockHealthMonitor, new ApiEndpointClassification()); + scheduler = new TaskScheduler(taskStorage, taskRunner); + processor = new OfflineRequestProcessor(scheduler, taskRunner, taskStorage, mockHealthMonitor); + } + + @After + public void tearDown() throws Exception { + // IterableTaskStorage is a singleton that outlives the test, so a runner left registered + // here would keep flushing tasks created by the next test. + taskStorage.removeDatabaseStatusListener(taskRunner); + taskStorage.deleteAllTasks(); + TaskScheduler.successCallbackMap.clear(); + TaskScheduler.failureCallbackMap.clear(); + IterableApi.getInstance().apiClient.setOfflineProcessingEnabled(false); + IterablePushRegistrationTask.Util.instance = originalPushRegistrationUtil; + server.shutdown(); + server = null; + } + + @Test + public void testPurgedRegisterFailsTheAppsHandlerAndLeavesNothingParked() { + AtomicInteger successCalls = new AtomicInteger(); + AtomicReference failureReason = new AtomicReference<>(); + AtomicReference failureData = new AtomicReference<>(new JSONObject()); + + String registerTaskId = schedule(IterableConstants.ENDPOINT_REGISTER_DEVICE_TOKEN, + data -> successCalls.incrementAndGet(), + (reason, data) -> { + failureReason.set(reason); + failureData.set(data); + }); + assertEquals(1, taskStorage.getNumberOfTasks()); + + processor.onLogout(getContext()); + shadowOf(getMainLooper()).idle(); + + assertEquals(TaskScheduler.PURGED_ON_LOGOUT_REASON, failureReason.get()); + assertNull("a request that was never sent has no response body", failureData.get()); + assertEquals("a request that was never sent must not report success", 0, successCalls.get()); + + assertEquals(0, taskStorage.getNumberOfTasks()); + assertFalse(TaskScheduler.successCallbackMap.containsKey(registerTaskId)); + assertFalse(TaskScheduler.failureCallbackMap.containsKey(registerTaskId)); + assertTrue("nothing may be left parked for a purged task", TaskScheduler.successCallbackMap.isEmpty()); + assertTrue("nothing may be left parked for a purged task", TaskScheduler.failureCallbackMap.isEmpty()); + } + + @Test + public void testPreservedDisableDeviceKeepsItsHandlersParked() { + AtomicInteger disableSuccessCalls = new AtomicInteger(); + AtomicInteger disableFailureCalls = new AtomicInteger(); + AtomicInteger registerFailureCalls = new AtomicInteger(); + + String disableTaskId = schedule(IterableConstants.ENDPOINT_DISABLE_DEVICE, + data -> disableSuccessCalls.incrementAndGet(), + (reason, data) -> disableFailureCalls.incrementAndGet()); + schedule(IterableConstants.ENDPOINT_REGISTER_DEVICE_TOKEN, null, + (reason, data) -> registerFailureCalls.incrementAndGet()); + assertEquals(2, taskStorage.getNumberOfTasks()); + + processor.onLogout(getContext()); + shadowOf(getMainLooper()).idle(); + + assertEquals("the register was discarded, so its handler has to fire", 1, registerFailureCalls.get()); + assertEquals("the disable survived the purge and is still going to be sent", + 0, disableFailureCalls.get()); + assertEquals(0, disableSuccessCalls.get()); + + assertEquals(1, taskStorage.getNumberOfTasks()); + assertTrue(TaskScheduler.successCallbackMap.containsKey(disableTaskId)); + assertTrue(TaskScheduler.failureCallbackMap.containsKey(disableTaskId)); + assertEquals(1, TaskScheduler.failureCallbackMap.size()); + } + + /** + * Orphaning is not specific to the register endpoint, so the unconditional purge settles handlers + * too. Nothing in production reaches it today, which is exactly why it needs a test. + */ + @Test + public void testDeleteAllTasksAlsoSettlesParkedHandlers() { + AtomicInteger trackFailureCalls = new AtomicInteger(); + AtomicInteger disableFailureCalls = new AtomicInteger(); + + schedule(IterableConstants.ENDPOINT_TRACK, null, (reason, data) -> trackFailureCalls.incrementAndGet()); + schedule(IterableConstants.ENDPOINT_DISABLE_DEVICE, null, (reason, data) -> disableFailureCalls.incrementAndGet()); + + scheduler.onTasksPurged(taskStorage.deleteAllTasks()); + shadowOf(getMainLooper()).idle(); + + assertEquals(1, trackFailureCalls.get()); + assertEquals("deleteAllTasks() spares nothing, including the disable", 1, disableFailureCalls.get()); + assertEquals(0, taskStorage.getNumberOfTasks()); + assertTrue(TaskScheduler.successCallbackMap.isEmpty()); + assertTrue(TaskScheduler.failureCallbackMap.isEmpty()); + } + + /** + * The failure handler is app code running during a logout, so it can legitimately queue more work + * or trigger another purge. Neither may corrupt the purge that is notifying it or recurse. + */ + @Test + public void testHandlerThatQueuesAndPurgesAgainDoesNotCorruptThePurge() { + AtomicInteger firstFailureCalls = new AtomicInteger(); + AtomicInteger nestedFailureCalls = new AtomicInteger(); + AtomicReference reentrantError = new AtomicReference<>(); + + schedule(IterableConstants.ENDPOINT_REGISTER_DEVICE_TOKEN, null, (reason, data) -> { + firstFailureCalls.incrementAndGet(); + try { + schedule(IterableConstants.ENDPOINT_TRACK, null, (r, d) -> nestedFailureCalls.incrementAndGet()); + processor.onLogout(getContext()); + } catch (Throwable t) { + reentrantError.set(t); + } + }); + schedule(IterableConstants.ENDPOINT_TRACK, null, (reason, data) -> { }); + + processor.onLogout(getContext()); + shadowOf(getMainLooper()).idle(); + + assertNull("re-entering the SDK from a purge handler must not throw", reentrantError.get()); + assertEquals("the handler fires exactly once, not once per nested purge", 1, firstFailureCalls.get()); + assertEquals("the task queued from the handler is purged and settled too", 1, nestedFailureCalls.get()); + assertEquals(0, taskStorage.getNumberOfTasks()); + assertTrue(TaskScheduler.successCallbackMap.isEmpty()); + assertTrue(TaskScheduler.failureCallbackMap.isEmpty()); + } + + /** + * The whole chain as an app sees it: {@code setEmail} parks its completion handlers, + * autoPushRegistration queues the register that carries them, and the next {@code setEmail} + * discards that register. An app dismissing a login spinner in the first {@code setEmail}'s + * callback used to wait forever. + */ + @Test + public void testNextLoginSettlesThePreviousLoginsQueuedRegister() throws Exception { + initializeWithOfflineQueue(true); + + AtomicInteger successA = new AtomicInteger(); + AtomicReference failureA = new AtomicReference<>(); + + IterableApi.getInstance().setEmail("userA@example.com", + data -> successA.incrementAndGet(), + (reason, data) -> failureA.set(reason)); + awaitTaskCount(1); + assertEquals(IterableConstants.ENDPOINT_REGISTER_DEVICE_TOKEN, onlyTaskName()); + + // Logout queues a disableDevice (which is preserved), purges userA's register, then queues + // userB's own register. + IterableApi.getInstance().setEmail("userB@example.com"); + awaitTaskCount(2); + + assertEquals("userA's queued registration was discarded, so its handler has to fire", + TaskScheduler.PURGED_ON_LOGOUT_REASON, failureA.get()); + assertEquals("userA never registered, so its success handler must not fire", 0, successA.get()); + } + + /** + * Settling a discarded register runs the wrapper {@code registerDeviceToken} built around the + * app's handlers, and that wrapper clears the handler pair it was created with. The next login's + * pair has to survive that: {@code registerDeviceToken} reads the handler fields on a thread + * {@code IterableApi} starts, so in production it can read them either side of the purge + * notification. This forces the order that used to lose them. + */ + @Test + public void testSettlingADiscardedRegisterDoesNotClearTheNextLoginsHandlers() throws Exception { + initializeWithOfflineQueue(true); + + AtomicInteger successA = new AtomicInteger(); + AtomicReference failureA = new AtomicReference<>(); + AtomicInteger successB = new AtomicInteger(); + AtomicInteger failureB = new AtomicInteger(); + + IterableApi.getInstance().setEmail("userA@example.com", + data -> successA.incrementAndGet(), + (reason, data) -> failureA.set(reason)); + awaitParkedFailureHandler(); + + // No token from here on, so nothing dispatches a request by itself and the only registration + // left in the test is the explicit one below. + when(IterablePushRegistrationTask.Util.instance.getFirebaseToken()).thenReturn(null); + + IterableApi.getInstance().setEmail("userB@example.com", + data -> successB.incrementAndGet(), + (reason, data) -> failureB.incrementAndGet()); + awaitTaskCount(0); + + assertEquals(TaskScheduler.PURGED_ON_LOGOUT_REASON, failureA.get()); + assertEquals(0, successA.get()); + assertEquals("userA's discarded register must not report as userB's failure", 0, failureB.get()); + + // What IterableApi's registration thread does once it gets a token, now that the purge has + // already settled userA's handlers. + IterableApi.getInstance().registerDeviceToken("userB@example.com", null, null, + "pushIntegration", TEST_TOKEN, null, IterableApi.getInstance().getDeviceAttributes()); + awaitTaskCount(1); + drainTaskRunner(); + + assertEquals("userB's registration must complete against userB's own handler", + 1, successB.get()); + assertEquals(0, failureB.get()); + } + + /** + * The register is queued from a thread IterableApi starts, and its handler is parked a moment + * after the row is written, so wait for the handler rather than for the row. + */ + private void awaitParkedFailureHandler() throws InterruptedException { + long deadline = System.currentTimeMillis() + 5000; + while (System.currentTimeMillis() < deadline) { + shadowOf(getMainLooper()).idle(); + for (String taskId : taskStorage.getAllTaskIds()) { + if (TaskScheduler.failureCallbackMap.get(taskId) != null) { + return; + } + } + Thread.sleep(10); + } + throw new AssertionError("no queued task ever parked a wrapped failure handler"); + } + + private void initializeWithOfflineQueue(boolean autoPushRegistration) { + IterableApi.initialize(getContext(), "apiKeyA", new IterableConfig.Builder() + .setAutoPushRegistration(autoPushRegistration) + .setPushIntegrationName("pushIntegration") + .setKeychainEncryption(false) + .build()); + IterableApi.getInstance().apiClient.setOfflineProcessingEnabled(true); + } + + private String onlyTaskName() { + ArrayList taskIds = taskStorage.getAllTaskIds(); + assertEquals(1, taskIds.size()); + IterableTask task = taskStorage.getTask(taskIds.get(0)); + assertNotNull(task); + return task.name; + } + + private String schedule(String resourcePath, + IterableHelper.SuccessHandler onSuccess, + IterableHelper.FailureHandler onFailure) { + Set before = new HashSet<>(taskStorage.getAllTaskIds()); + IterableApiRequest request = new IterableApiRequest("apiKeyA", resourcePath, new JSONObject(), + IterableApiRequest.POST, null, onSuccess, onFailure); + scheduler.scheduleTask(request, onSuccess, onFailure); + for (String taskId : taskStorage.getAllTaskIds()) { + if (!before.contains(taskId)) { + return taskId; + } + } + throw new AssertionError("no task was created for " + resourcePath); + } + + /** + * registerDeviceToken is dispatched onto its own thread by IterableApi, so wait for the row + * rather than idling loopers. + */ + private void awaitTaskCount(long expected) throws InterruptedException { + long deadline = System.currentTimeMillis() + 5000; + while (System.currentTimeMillis() < deadline) { + shadowOf(getMainLooper()).idle(); + if (taskStorage.getNumberOfTasks() == expected) { + return; + } + Thread.sleep(10); + } + assertEquals(expected, taskStorage.getNumberOfTasks()); + } + + private void drainTaskRunner() { + when(mockNetworkConnectivityManager.isConnected()).thenReturn(true); + taskRunner.onTaskCreated(null); + shadowOf(taskRunner.handler.getLooper()).idle(); + shadowOf(getMainLooper()).idle(); + } +} diff --git a/iterableapi/src/test/java/com/iterable/iterableapi/OfflineRequestProcessorTest.java b/iterableapi/src/test/java/com/iterable/iterableapi/OfflineRequestProcessorTest.java index a8175a886..9ed432e41 100644 --- a/iterableapi/src/test/java/com/iterable/iterableapi/OfflineRequestProcessorTest.java +++ b/iterableapi/src/test/java/com/iterable/iterableapi/OfflineRequestProcessorTest.java @@ -7,10 +7,16 @@ import org.junit.Test; import org.junit.runner.RunWith; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -57,18 +63,44 @@ public void testOnlineRequestWhenDBError() { @Test public void testAllOfflineApisUseTaskScheduler() { - String[] offlineApis = new String[]{ - IterableConstants.ENDPOINT_TRACK, - IterableConstants.ENDPOINT_TRACK_PUSH_OPEN, - IterableConstants.ENDPOINT_TRACK_PURCHASE, - IterableConstants.ENDPOINT_TRACK_INAPP_OPEN, - IterableConstants.ENDPOINT_TRACK_INAPP_CLICK, - IterableConstants.ENDPOINT_TRACK_INAPP_CLOSE, - IterableConstants.ENDPOINT_TRACK_INBOX_SESSION, - IterableConstants.ENDPOINT_TRACK_INAPP_DELIVERY, - IterableConstants.ENDPOINT_INAPP_CONSUME}; - for (String uri : offlineApis) { - assertTrue(offlineRequestProcessor.isRequestOfflineCompatible(uri)); + for (String uri : EXPECTED_OFFLINE_APIS) { + assertTrue(uri + " should be offline compatible", offlineRequestProcessor.isRequestOfflineCompatible(uri)); } } + + /** + * The offline API set encodes a product decision about which requests survive a network + * outage, so it must not drift silently. ENDPOINT_DISABLE_DEVICE was added in 3.5.16 and + * quietly dropped again in 3.7.0 because nothing asserted its membership. Any edit to the + * set now has to be made here too, which forces the change to be reviewed deliberately. + */ + @Test + public void testOfflineApiSetMembershipIsExact() { + assertEquals(EXPECTED_OFFLINE_APIS, OfflineRequestProcessor.getOfflineApiSet()); + } + + @Test + public void testLogoutPreservesQueuedDisableDeviceTasks() { + offlineRequestProcessor.onLogout(null); + verify(mockTaskStorage).deleteAllTasksExcept(IterableConstants.ENDPOINT_DISABLE_DEVICE); + verify(mockTaskStorage, never()).deleteAllTasks(); + } + + private static final Set EXPECTED_OFFLINE_APIS = new HashSet<>(Arrays.asList( + IterableConstants.ENDPOINT_TRACK, + IterableConstants.ENDPOINT_TRACK_PUSH_OPEN, + IterableConstants.ENDPOINT_TRACK_PURCHASE, + IterableConstants.ENDPOINT_TRACK_INAPP_OPEN, + IterableConstants.ENDPOINT_TRACK_INAPP_CLICK, + IterableConstants.ENDPOINT_TRACK_INAPP_CLOSE, + IterableConstants.ENDPOINT_TRACK_INBOX_SESSION, + IterableConstants.ENDPOINT_TRACK_INAPP_DELIVERY, + IterableConstants.ENDPOINT_INAPP_CONSUME, + IterableConstants.ENDPOINT_UPDATE_CART, + IterableConstants.ENDPOINT_TRACK_EMBEDDED_RECEIVED, + IterableConstants.ENDPOINT_TRACK_EMBEDDED_CLICK, + IterableConstants.ENDPOINT_TRACK_EMBEDDED_SESSION, + IterableConstants.ENDPOINT_DISABLE_DEVICE, + IterableConstants.ENDPOINT_REGISTER_DEVICE_TOKEN + )); }