Skip to content

Fix unregistering pusher local error #2901

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

Merged
merged 3 commits into from
May 23, 2024
Merged

Fix unregistering pusher local error #2901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce23135
Do not fail un-registration if Firebase token is not known. Fixes #2895
bmarty May 23, 2024
d066f03
Add logs on pusher registration
bmarty May 23, 2024
3796def
Ignore local errors when unregistering UnifiedPush, to not block swit…
bmarty May 23, 2024
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
5 changes: 4 additions & 1 deletion ...es/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,16 @@ class DefaultPushService @Inject constructor(
pushProvider: PushProvider,
distributor: Distributor,
): Result<Unit> {
Timber.d("Registering with ${pushProvider.name}/${distributor.name}}")
val userPushStore = userPushStoreFactory.getOrCreate(matrixClient.sessionId)
val currentPushProviderName = userPushStore.getPushProviderName()
val currentPushProvider = pushProviders.find { it.name == currentPushProviderName }
val currentDistributorValue = currentPushProvider?.getCurrentDistributor(matrixClient)?.value
if (currentPushProviderName != pushProvider.name || currentDistributorValue != distributor.value) {
// Unregister previous one if any
currentPushProvider?.unregister(matrixClient)
currentPushProvider
?.also { Timber.d("Unregistering previous push provider $currentPushProviderName/$currentDistributorValue") }
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this could be logged before this call and not need an .also {}.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, not really since here currentPushProvider can still be null, so here ?. for the also is useful.
I agree that the code is a bit hard to follow, I tried to add some if(x (!= null) statement but it's getting worse.

?.unregister(matrixClient)
?.onFailure {
Timber.w(it, "Failed to unregister previous push provider")
return Result.failure(it)
Expand Down
11 changes: 5 additions & 6 deletions ...c/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,13 @@ class FirebasePushProvider @Inject constructor(
override suspend fun getCurrentDistributor(matrixClient: MatrixClient) = firebaseDistributor

override suspend fun unregister(matrixClient: MatrixClient): Result<Unit> {
val pushKey = firebaseStore.getFcmToken() ?: return Result.failure<Unit>(
IllegalStateException(
"Unable to unregister pusher, Firebase token is not known."
)
).also {
val pushKey = firebaseStore.getFcmToken()
return if (pushKey == null) {
Timber.tag(loggerTag.value).w("Unable to unregister pusher, Firebase token is not known.")
Result.success(Unit)
} else {
pusherSubscriber.unregisterPusher(matrixClient, pushKey, FirebaseConfig.PUSHER_HTTP_URL)
}
return pusherSubscriber.unregisterPusher(matrixClient, pushKey, FirebaseConfig.PUSHER_HTTP_URL)
}

override suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig? {
Expand Down
7 changes: 2 additions & 5 deletions ...st/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,14 @@ class FirebasePushProviderTest {
}

@Test
fun `unregister ko no token`() = runTest {
fun `unregister no token - in this case, the error is ignored`() = runTest {
val firebasePushProvider = createFirebasePushProvider(
firebaseStore = InMemoryFirebaseStore(
token = null
),
pusherSubscriber = FakePusherSubscriber(
unregisterPusherResult = { _, _, _ -> Result.success(Unit) }
)
)
val result = firebasePushProvider.unregister(FakeMatrixClient())
assertThat(result.isFailure).isTrue()
assertThat(result.isSuccess).isTrue()
}

@Test
Expand Down
7 changes: 6 additions & 1 deletion ...in/io/element/android/libraries/pushproviders/unifiedpush/UnregisterUnifiedPushUseCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import io.element.android.libraries.di.ApplicationContext
import io.element.android.libraries.matrix.api.MatrixClient
import io.element.android.libraries.pushproviders.api.PusherSubscriber
import org.unifiedpush.android.connector.UnifiedPush
import timber.log.Timber
import javax.inject.Inject

interface UnregisterUnifiedPushUseCase {
Expand All @@ -39,7 +40,11 @@ class DefaultUnregisterUnifiedPushUseCase @Inject constructor(
val endpoint = unifiedPushStore.getEndpoint(clientSecret)
val gateway = unifiedPushStore.getPushGateway(clientSecret)
if (endpoint == null || gateway == null) {
return Result.failure(IllegalStateException("No endpoint or gateway found for client secret"))
Timber.w("No endpoint or gateway found for client secret")
// Ensure we don't have any remaining data, but ignore this error
unifiedPushStore.storeUpEndpoint(clientSecret, null)
unifiedPushStore.storePushGateway(clientSecret, null)
return Result.success(Unit)
}
return pusherSubscriber.unregisterPusher(matrixClient, endpoint, gateway)
.onSuccess {
Expand Down
28 changes: 24 additions & 4 deletions ...nt/android/libraries/pushproviders/unifiedpush/DefaultUnregisterUnifiedPushUseCaseTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,29 +64,49 @@ class DefaultUnregisterUnifiedPushUseCaseTest {
}

@Test
fun `test un registration error - no endpoint`() = runTest {
fun `test un registration error - no endpoint - will not unregister but return success`() = runTest {
val matrixClient = FakeMatrixClient()
val storeUpEndpointResult = lambdaRecorder { _: String, _: String? -> }
val storePushGatewayResult = lambdaRecorder { _: String, _: String? -> }
val useCase = createDefaultUnregisterUnifiedPushUseCase(
unifiedPushStore = FakeUnifiedPushStore(
getEndpointResult = { null },
getPushGatewayResult = { "aGateway" },
storeUpEndpointResult = storeUpEndpointResult,
storePushGatewayResult = storePushGatewayResult,
),
)
val result = useCase.execute(matrixClient, A_SECRET)
assertThat(result.isFailure).isTrue()
assertThat(result.isSuccess).isTrue()
storeUpEndpointResult.assertions()
.isCalledOnce()
.with(value(A_SECRET), value(null))
storePushGatewayResult.assertions()
.isCalledOnce()
.with(value(A_SECRET), value(null))
}

@Test
fun `test un registration error - no gateway`() = runTest {
fun `test un registration error - no gateway - will not unregister but return success`() = runTest {
val matrixClient = FakeMatrixClient()
val storeUpEndpointResult = lambdaRecorder { _: String, _: String? -> }
val storePushGatewayResult = lambdaRecorder { _: String, _: String? -> }
val useCase = createDefaultUnregisterUnifiedPushUseCase(
unifiedPushStore = FakeUnifiedPushStore(
getEndpointResult = { "aEndpoint" },
getPushGatewayResult = { null },
storeUpEndpointResult = storeUpEndpointResult,
storePushGatewayResult = storePushGatewayResult,
),
)
val result = useCase.execute(matrixClient, A_SECRET)
assertThat(result.isFailure).isTrue()
assertThat(result.isSuccess).isTrue()
storeUpEndpointResult.assertions()
.isCalledOnce()
.with(value(A_SECRET), value(null))
storePushGatewayResult.assertions()
.isCalledOnce()
.with(value(A_SECRET), value(null))
}

@Test
Expand Down
Loading