diff --git a/firestore/build.gradle.kts b/firestore/build.gradle.kts index 6074dab4d..d048e6c63 100644 --- a/firestore/build.gradle.kts +++ b/firestore/build.gradle.kts @@ -19,6 +19,9 @@ android { testOptions { targetSdk = Config.SdkVersions.target + unitTests { + isIncludeAndroidResources = true + } } lint { @@ -64,6 +67,12 @@ dependencies { lintChecks(project(":lint")) + testImplementation(libs.junit) + testImplementation(libs.robolectric) + testImplementation(libs.test.core) + testImplementation(libs.mockito.core) + testImplementation(libs.androidx.paging) + androidTestImplementation(libs.arch.core.testing) androidTestImplementation(libs.test.core) androidTestImplementation(libs.junit) diff --git a/firestore/src/main/java/com/firebase/ui/firestore/paging/FirestorePagingSource.java b/firestore/src/main/java/com/firebase/ui/firestore/paging/FirestorePagingSource.java index d10dd2b1b..4a6942bf9 100644 --- a/firestore/src/main/java/com/firebase/ui/firestore/paging/FirestorePagingSource.java +++ b/firestore/src/main/java/com/firebase/ui/firestore/paging/FirestorePagingSource.java @@ -47,13 +47,14 @@ public Single> loadSingle(@NonNull LoadPar } return toLoadResult(snapshot.getDocuments(), nextPage); } catch (ExecutionException e) { - if (e.getCause() instanceof Exception) { - // throw the original Exception - throw (Exception) e.getCause(); - } - // Only throw a new Exception when the original - // Throwable cannot be cast to Exception - throw new Exception(e); + // Report the original cause rather than the ExecutionException wrapper. + Throwable cause = e.getCause(); + return new LoadResult.Error(cause == null ? e : cause); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return new LoadResult.Error(e); + } catch (Exception e) { + return new LoadResult.Error(e); } }).subscribeOn(Schedulers.io()).onErrorReturn(LoadResult.Error::new); } diff --git a/firestore/src/main/java/com/firebase/ui/firestore/paging/PageKey.java b/firestore/src/main/java/com/firebase/ui/firestore/paging/PageKey.java index 45d4e7c7a..db3fc6a9a 100644 --- a/firestore/src/main/java/com/firebase/ui/firestore/paging/PageKey.java +++ b/firestore/src/main/java/com/firebase/ui/firestore/paging/PageKey.java @@ -3,6 +3,8 @@ import com.google.firebase.firestore.DocumentSnapshot; import com.google.firebase.firestore.Query; +import java.util.Objects; + import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.RestrictTo; @@ -43,18 +45,25 @@ public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; PageKey key = (PageKey) o; - if (mStartAfter == null && key.mStartAfter == null && - mEndBefore == null && key.mEndBefore == null) - return true; - return mStartAfter.getId().equals(key.mStartAfter.getId()) && - mEndBefore.getId().equals(key.mEndBefore.getId()); + return Objects.equals(documentId(mStartAfter), documentId(key.mStartAfter)) && + Objects.equals(documentId(mEndBefore), documentId(key.mEndBefore)); + } + + @Override + public int hashCode() { + return Objects.hash(documentId(mStartAfter), documentId(mEndBefore)); + } + + @Nullable + private static String documentId(@Nullable DocumentSnapshot snapshot) { + return snapshot == null ? null : snapshot.getId(); } @Override @NonNull public String toString() { - String startAfter = mStartAfter == null ? null : mStartAfter.getId(); - String endBefore = mEndBefore == null ? null : mEndBefore.getId(); + String startAfter = documentId(mStartAfter); + String endBefore = documentId(mEndBefore); return "PageKey{" + "StartAfter=" + startAfter + ", EndBefore=" + endBefore + diff --git a/firestore/src/androidTest/java/com/firebase/ui/firestore/paging/FirestorePagingSourceTest.java b/firestore/src/test/java/com/firebase/ui/firestore/paging/FirestorePagingSourceTest.java similarity index 53% rename from firestore/src/androidTest/java/com/firebase/ui/firestore/paging/FirestorePagingSourceTest.java rename to firestore/src/test/java/com/firebase/ui/firestore/paging/FirestorePagingSourceTest.java index 3cec97075..810efb616 100644 --- a/firestore/src/androidTest/java/com/firebase/ui/firestore/paging/FirestorePagingSourceTest.java +++ b/firestore/src/test/java/com/firebase/ui/firestore/paging/FirestorePagingSourceTest.java @@ -1,5 +1,9 @@ package com.firebase.ui.firestore.paging; +import com.google.android.gms.tasks.OnCanceledListener; +import com.google.android.gms.tasks.OnFailureListener; +import com.google.android.gms.tasks.OnSuccessListener; +import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.Tasks; import com.google.firebase.firestore.DocumentSnapshot; import com.google.firebase.firestore.Query; @@ -11,24 +15,35 @@ import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executor; +import java.util.concurrent.TimeUnit; import androidx.paging.PagingSource; import androidx.paging.PagingSource.LoadParams.Append; import androidx.paging.PagingSource.LoadParams.Refresh; import androidx.paging.PagingSource.LoadResult.Page; -import androidx.test.ext.junit.runners.AndroidJUnit4; +import io.reactivex.rxjava3.disposables.Disposable; +import io.reactivex.rxjava3.functions.Consumer; +import io.reactivex.rxjava3.plugins.RxJavaPlugins; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -@RunWith(AndroidJUnit4.class) +@RunWith(RobolectricTestRunner.class) +@Config(manifest = Config.NONE) public class FirestorePagingSourceTest { @Mock @@ -105,6 +120,53 @@ public void testLoadAfter_failure() { assertEquals(expected, actual); } + /** + * Cancelling a load interrupts the thread blocked in Tasks.await(). The resulting + * InterruptedException must not escape the callable: by then the subscription is disposed, so + * RxJava would report it as an UndeliverableException and crash the app. + * + * See https://github.com/firebase/FirebaseUI-Android/issues/2008 + */ + @Test + public void testLoadSingle_interruptedWhileDisposed_doesNotReportUndeliverableException() + throws Exception { + FirestorePagingSource pagingSource = new FirestorePagingSource(mMockQuery, Source.DEFAULT); + + // Counted down once Tasks.await() starts registering listeners, which means the load is + // genuinely running and about to block. Without waiting on this, the test could dispose + // before the callable ever ran — and would then pass even against the unfixed source. + CountDownLatch loadInFlight = new CountDownLatch(1); + mockQueryNeverCompletes(loadInFlight); + + List undeliverableErrors = new CopyOnWriteArrayList<>(); + CountDownLatch undeliverableReported = new CountDownLatch(1); + Consumer originalErrorHandler = RxJavaPlugins.getErrorHandler(); + RxJavaPlugins.setErrorHandler(throwable -> { + undeliverableErrors.add(throwable); + undeliverableReported.countDown(); + }); + + try { + Refresh refreshRequest = new Refresh<>(null, 2, false); + Disposable disposable = pagingSource.loadSingle(refreshRequest) + .subscribe(result -> { }, error -> { }); + + assertTrue("Load never reached Tasks.await(), so nothing was interrupted", + loadInFlight.await(5, TimeUnit.SECONDS)); + + // Interrupts the worker thread blocked in Tasks.await(). + disposable.dispose(); + + assertFalse("Interrupting an in-flight load must not surface as an undeliverable " + + "RxJava exception: " + undeliverableErrors, + undeliverableReported.await(2, TimeUnit.SECONDS)); + } finally { + RxJavaPlugins.setErrorHandler(originalErrorHandler); + } + + verify(mMockQuery).get(Source.DEFAULT); + } + private void initMockQuery() { when(mMockQuery.startAfter(any(DocumentSnapshot.class))).thenReturn(mMockQuery); when(mMockQuery.endBefore(any(DocumentSnapshot.class))).thenReturn(mMockQuery); @@ -121,4 +183,25 @@ private void mockQuerySuccess(List snapshots) { private void mockQueryFailure(Exception exception) { when(mMockQuery.get(Source.DEFAULT)).thenReturn(Tasks.forException(exception)); } + + /** + * Stubs the query with a task that never completes, so Tasks.await() blocks until the thread + * is interrupted. {@code inFlight} is counted down once await() has started. + */ + @SuppressWarnings("unchecked") + private void mockQueryNeverCompletes(CountDownLatch inFlight) { + Task neverCompletes = mock(Task.class); + when(neverCompletes.isComplete()).thenReturn(false); + when(neverCompletes.addOnSuccessListener(any(Executor.class), any(OnSuccessListener.class))) + .thenAnswer(invocation -> { + inFlight.countDown(); + return neverCompletes; + }); + when(neverCompletes.addOnFailureListener(any(Executor.class), any(OnFailureListener.class))) + .thenReturn(neverCompletes); + when(neverCompletes.addOnCanceledListener(any(Executor.class), any(OnCanceledListener.class))) + .thenReturn(neverCompletes); + + when(mMockQuery.get(Source.DEFAULT)).thenReturn(neverCompletes); + } }