Skip to content

Commit 6628770

Browse files
jinliu9508jinliu9508
authored
Merge pull request #2371 from OneSignal/fix-outcome-retry-on-400
Fix: ANR on POST to Outcomes endpoint
2 parents 059cc4d + b657f4b commit 6628770

File tree

4 files changed

+134
-20
lines changed

4 files changed

+134
-20
lines changed

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsController.kt‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.onesignal.session.internal.outcomes.impl
22

33
import android.os.Process
4+
import com.onesignal.common.NetworkUtils
45
import com.onesignal.common.exceptions.BackendException
56
import com.onesignal.common.threading.suspendifyOnThread
67
import com.onesignal.core.internal.config.ConfigModelStore
@@ -75,10 +76,15 @@ internal class OutcomeEventsController(
7576

7677
_outcomeEventsCache.deleteOldOutcomeEvent(event)
7778
} catch (ex: BackendException) {
78-
Logging.warn(
79-
"""OutcomeEventsController.sendSavedOutcomeEvent: Sending outcome with name: ${event.outcomeId} failed with status code: ${ex.statusCode} and response: ${ex.response}
80-
Outcome event was cached and will be reattempted on app cold start""",
81-
)
79+
val responseType = NetworkUtils.getResponseStatusType(ex.statusCode)
80+
val err = "OutcomeEventsController.sendSavedOutcomeEvent: Sending outcome with name: ${event.outcomeId} failed with status code: ${ex.statusCode} and response: ${ex.response}"
81+
82+
if (responseType == NetworkUtils.ResponseStatusType.RETRYABLE) {
83+
Logging.warn("$err Outcome event was cached and will be reattempted on app cold start")
84+
} else {
85+
Logging.error("$err Outcome event will be omitted!")
86+
_outcomeEventsCache.deleteOldOutcomeEvent(event)
87+
}
8288
}
8389
}
8490

@@ -220,14 +226,19 @@ Outcome event was cached and will be reattempted on app cold start""",
220226
// The only case where an actual success has occurred and the OutcomeEvent should be sent back
221227
return OutcomeEvent.fromOutcomeEventParamstoOutcomeEvent(eventParams)
222228
} catch (ex: BackendException) {
223-
Logging.warn(
224-
"""OutcomeEventsController.sendAndCreateOutcomeEvent: Sending outcome with name: $name failed with status code: ${ex.statusCode} and response: ${ex.response}
225-
Outcome event was cached and will be reattempted on app cold start""",
226-
)
227-
228-
// Only if we need to save and retry the outcome, then we will save the timestamp for future sending
229-
eventParams.timestamp = timestampSeconds
230-
_outcomeEventsCache.saveOutcomeEvent(eventParams)
229+
val responseType = NetworkUtils.getResponseStatusType(ex.statusCode)
230+
val err = "OutcomeEventsController.sendAndCreateOutcomeEvent: Sending outcome with name: $name failed with status code: ${ex.statusCode} and response: ${ex.response}"
231+
232+
if (responseType == NetworkUtils.ResponseStatusType.RETRYABLE) {
233+
Logging.warn("$err Outcome event was cached and will be reattempted on app cold start")
234+
235+
// Only if we need to save and retry the outcome, then we will save the timestamp for future sending
236+
eventParams.timestamp = timestampSeconds
237+
_outcomeEventsCache.saveOutcomeEvent(eventParams)
238+
} else {
239+
Logging.error("$err Outcome event will be omitted!")
240+
_outcomeEventsCache.deleteOldOutcomeEvent(eventParams)
241+
}
231242

232243
// Return null to determine not a failure, but not a success in terms of the request made
233244
return null

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsRepository.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import com.onesignal.session.internal.influence.Influence
88
import com.onesignal.session.internal.influence.InfluenceChannel
99
import com.onesignal.session.internal.influence.InfluenceType
1010
import com.onesignal.session.internal.influence.InfluenceType.Companion.fromString
11-
import com.onesignal.session.internal.outcomes.migrations.RemoveZeroSessionTimeRecords
11+
import com.onesignal.session.internal.outcomes.migrations.RemoveInvalidSessionTimeRecords
1212
import kotlinx.coroutines.Dispatchers
1313
import kotlinx.coroutines.withContext
1414
import org.json.JSONArray
@@ -102,7 +102,7 @@ internal class OutcomeEventsRepository(
102102
override suspend fun getAllEventsToSend(): List<OutcomeEventParams> {
103103
val events: MutableList<OutcomeEventParams> = ArrayList()
104104
withContext(Dispatchers.IO) {
105-
RemoveZeroSessionTimeRecords.run(_databaseProvider)
105+
RemoveInvalidSessionTimeRecords.run(_databaseProvider)
106106
_databaseProvider.os.query(OutcomeEventsTable.TABLE_NAME) { cursor ->
107107
if (cursor.moveToFirst()) {
108108
do {

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveZeroSessionTimeRecords.kt‎ renamed to ‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveInvalidSessionTimeRecords.kt‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,20 +4,22 @@ import com.onesignal.core.internal.database.IDatabaseProvider
44
import com.onesignal.session.internal.outcomes.impl.OutcomeEventsTable
55

66
/**
7-
* Purpose: Clean up invalid cached os__session_duration outcome records
8-
* with zero session_time produced in SDK versions 5.1.15 to 5.1.20 so we stop
9-
* sending these requests to the backend.
7+
* Purpose: Clean up invalid cached os__session_duration outcome records with
8+
* 1. zero session_time produced in SDK versions 5.1.15 to 5.1.20
9+
* 2. missing session_time produced in SDK
10+
* so we stop sending these requests to the backend.
1011
*
1112
* Issue: SessionService.backgroundRun() didn't account for it being run more
1213
* than one time in the background, when this happened it would create a
13-
* outcome record with zero time which is invalid.
14+
* outcome record with zero time or null which is invalid.
1415
*/
15-
object RemoveZeroSessionTimeRecords {
16+
object RemoveInvalidSessionTimeRecords {
1617
fun run(databaseProvider: IDatabaseProvider) {
1718
databaseProvider.os.delete(
1819
OutcomeEventsTable.TABLE_NAME,
1920
OutcomeEventsTable.COLUMN_NAME_NAME + " = \"os__session_duration\"" +
20-
" AND " + OutcomeEventsTable.COLUMN_NAME_SESSION_TIME + " = 0",
21+
" AND (" + OutcomeEventsTable.COLUMN_NAME_SESSION_TIME + " = 0" +
22+
" OR " + OutcomeEventsTable.COLUMN_NAME_SESSION_TIME + " IS NULL)",
2123
null,
2224
)
2325
}

‎OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/outcomes/OutcomeEventsControllerTests.kt‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -723,4 +723,105 @@ class OutcomeEventsControllerTests : FunSpec({
723723
}
724724
coVerify(exactly = 0) { mockOutcomeEventsRepository.deleteOldOutcomeEvent(any()) }
725725
}
726+
727+
test("send saved outcome event with 400 error deletes event and does not retry") {
728+
// Given
729+
val now = 111111L
730+
val mockSessionService = mockk<ISessionService>()
731+
every { mockSessionService.subscribe(any()) } just Runs
732+
733+
val subscriptionModel = createTestSubscriptionModel()
734+
735+
val mockSubscriptionManager = mockk<ISubscriptionManager>()
736+
every { mockSubscriptionManager.subscriptions.push } returns PushSubscription(subscriptionModel)
737+
738+
val mockInfluenceManager = mockk<IInfluenceManager>()
739+
val mockOutcomeEventsRepository = mockk<IOutcomeEventsRepository>()
740+
coEvery { mockOutcomeEventsRepository.cleanCachedUniqueOutcomeEventNotifications() } just runs
741+
coEvery { mockOutcomeEventsRepository.deleteOldOutcomeEvent(any()) } just runs
742+
coEvery { mockOutcomeEventsRepository.getAllEventsToSend() } returns
743+
listOf(
744+
OutcomeEventParams("outcomeId1", OutcomeSource(OutcomeSourceBody(JSONArray().put("notificationId1")), null), .4f, 0, 1111),
745+
)
746+
val mockOutcomeEventsPreferences = spyk<IOutcomeEventsPreferences>()
747+
val mockOutcomeEventsBackend = mockk<IOutcomeEventsBackendService>()
748+
coEvery { mockOutcomeEventsBackend.sendOutcomeEvent(any(), any(), any(), any(), any(), any()) } throws BackendException(400, "Bad Request")
749+
750+
val outcomeEventsController =
751+
OutcomeEventsController(
752+
mockSessionService,
753+
mockInfluenceManager,
754+
mockOutcomeEventsRepository,
755+
mockOutcomeEventsPreferences,
756+
mockOutcomeEventsBackend,
757+
MockHelper.configModelStore(),
758+
MockHelper.identityModelStore { it.onesignalId = "onesignalId" },
759+
mockSubscriptionManager,
760+
MockHelper.deviceService(),
761+
MockHelper.time(now),
762+
)
763+
764+
// When
765+
outcomeEventsController.start()
766+
delay(1000)
767+
768+
// Then
769+
coVerify(exactly = 1) {
770+
mockOutcomeEventsBackend.sendOutcomeEvent(any(), any(), any(), any(), any(), any())
771+
}
772+
coVerify(exactly = 1) {
773+
mockOutcomeEventsRepository.deleteOldOutcomeEvent(any())
774+
}
775+
}
776+
777+
test("send outcome event with 400 error deletes event and returns null") {
778+
// Given
779+
val now = 111L
780+
val mockSessionService = mockk<ISessionService>()
781+
every { mockSessionService.subscribe(any()) } just Runs
782+
783+
val mockInfluenceManager = mockk<IInfluenceManager>()
784+
every { mockInfluenceManager.influences } returns listOf(Influence(InfluenceChannel.NOTIFICATION, InfluenceType.UNATTRIBUTED, null))
785+
786+
val subscriptionModel = createTestSubscriptionModel()
787+
788+
val mockSubscriptionManager = mockk<ISubscriptionManager>()
789+
every { mockSubscriptionManager.subscriptions.push } returns PushSubscription(subscriptionModel)
790+
791+
val mockOutcomeEventsRepository = spyk<IOutcomeEventsRepository>()
792+
val mockOutcomeEventsPreferences = spyk<IOutcomeEventsPreferences>()
793+
val mockOutcomeEventsBackend = mockk<IOutcomeEventsBackendService>()
794+
coEvery { mockOutcomeEventsBackend.sendOutcomeEvent(any(), any(), any(), any(), any(), any()) } throws BackendException(400, "Bad Request")
795+
796+
val outcomeEventsController =
797+
OutcomeEventsController(
798+
mockSessionService,
799+
mockInfluenceManager,
800+
mockOutcomeEventsRepository,
801+
mockOutcomeEventsPreferences,
802+
mockOutcomeEventsBackend,
803+
MockHelper.configModelStore(),
804+
MockHelper.identityModelStore(),
805+
mockSubscriptionManager,
806+
MockHelper.deviceService(),
807+
MockHelper.time(now),
808+
)
809+
810+
// When
811+
val evnt = outcomeEventsController.sendOutcomeEvent("OUTCOME_1")
812+
813+
// Then
814+
evnt shouldBe null
815+
816+
coVerify(exactly = 1) {
817+
mockOutcomeEventsBackend.sendOutcomeEvent(any(), any(), any(), any(), any(), any())
818+
}
819+
coVerify(exactly = 1) {
820+
mockOutcomeEventsRepository.deleteOldOutcomeEvent(any())
821+
}
822+
// Verify event is NOT saved for retry (unlike other error codes)
823+
coVerify(exactly = 0) {
824+
mockOutcomeEventsRepository.saveOutcomeEvent(any())
825+
}
826+
}
726827
})

0 commit comments

Comments
 (0)