Merge pull request #99 from square/jw/nully

Formalize behavior around Query.run() returning null.

Author: JakeWharton

sqlbrite/src/androidTest/java/com/squareup/sqlbrite/QueryTest.java

@@ -16,14 +16,17 @@
 package com.squareup.sqlbrite;
 
 import android.database.Cursor;
+import android.support.annotation.Nullable;
 import android.support.test.InstrumentationRegistry;
 import com.squareup.sqlbrite.SqlBrite.Query;
 import com.squareup.sqlbrite.TestDb.Employee;
 import java.util.List;
 import org.junit.Before;
 import org.junit.Test;
+import rx.Observable;
 import rx.functions.Func1;
 import rx.observables.BlockingObservable;
+import rx.observers.TestSubscriber;
 import rx.schedulers.Schedulers;
 
 import static com.google.common.truth.Truth.assertThat;
@@ -89,6 +92,22 @@ public final class QueryTest {
     }
   }
 
+  @Test public void mapToOneIgnoresNullCursor() {
+    Query nully = new Query() {
+      @Nullable @Override public Cursor run() {
+        return null;
+      }
+    };
+
+    TestSubscriber<Employee> subscriber = new TestSubscriber<>();
+    Observable.just(nully)
+        .lift(Query.mapToOne(Employee.MAPPER))
+        .subscribe(subscriber);
+
+    subscriber.assertNoValues();
+    subscriber.assertCompleted();
+  }
+
   @Test public void mapToOneOrDefault() {
     Employee employees = db.createQuery(TABLE_EMPLOYEE, SELECT_EMPLOYEES + " LIMIT 1")
         .lift(Query.mapToOneOrDefault(Employee.MAPPER, null))
@@ -146,6 +165,23 @@ public final class QueryTest {
     }
   }
 
+  @Test public void mapToOneOrDefaultReturnsDefaultWhenNullCursor() {
+    Employee defaultEmployee = new Employee("bob", "Bob Bobberson");
+    Query nully = new Query() {
+      @Nullable @Override public Cursor run() {
+        return null;
+      }
+    };
+
+    TestSubscriber<Employee> subscriber = new TestSubscriber<>();
+    Observable.just(nully)
+        .lift(Query.mapToOneOrDefault(Employee.MAPPER, defaultEmployee))
+        .subscribe(subscriber);
+
+    subscriber.assertValues(defaultEmployee);
+    subscriber.assertCompleted();
+  }
+
   @Test public void mapToList() {
     List<Employee> employees = db.createQuery(TABLE_EMPLOYEE, SELECT_EMPLOYEES)
         .lift(Query.mapToList(Employee.MAPPER))
@@ -184,4 +220,20 @@ public final class QueryTest {
           "OnError while emitting onNext value: SELECT username, name FROM employee");
     }
   }
+
+  @Test public void mapToListIgnoresNullCursor() {
+    Query nully = new Query() {
+      @Nullable @Override public Cursor run() {
+        return null;
+      }
+    };
+
+    TestSubscriber<List<Employee>> subscriber = new TestSubscriber<>();
+    Observable.just(nully)
+        .lift(Query.mapToList(Employee.MAPPER))
+        .subscribe(subscriber);
+
+    subscriber.assertNoValues();
+    subscriber.assertCompleted();
+  }
 }

sqlbrite/src/androidTest/java/com/squareup/sqlbrite/SqlBriteTest.java

@@ -2,13 +2,15 @@
 
 import android.database.Cursor;
 import android.database.MatrixCursor;
+import android.support.annotation.Nullable;
 import android.support.test.runner.AndroidJUnit4;
 import com.squareup.sqlbrite.SqlBrite.Query;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import rx.functions.Func1;
+import rx.observers.TestSubscriber;
 
 import static com.google.common.truth.Truth.assertThat;
 
@@ -51,6 +53,28 @@ public final class SqlBriteTest {
     assertThat(count.get()).isEqualTo(1);
   }
 
+  @Test public void asRowsEmptyWhenNullCursor() {
+    Query nully = new Query() {
+      @Nullable @Override public Cursor run() {
+        return null;
+      }
+    };
+
+    TestSubscriber<Name> subscriber = new TestSubscriber<>();
+    final AtomicInteger count = new AtomicInteger();
+    nully.asRows(new Func1<Cursor, Name>() {
+      @Override public Name call(Cursor cursor) {
+        count.incrementAndGet();
+        return Name.MAP.call(cursor);
+      }
+    }).subscribe(subscriber);
+
+    subscriber.assertNoValues();
+    subscriber.assertCompleted();
+
+    assertThat(count.get()).isEqualTo(0);
+  }
+
   static final class Name {
     static final Func1<Cursor, Name> MAP = new Func1<Cursor, Name>() {
       @Override public Name call(Cursor cursor) {

sqlbrite/src/main/java/com/squareup/sqlbrite/QueryToListOperator.java

@@ -22,6 +22,9 @@ public Subscriber<? super SqlBrite.Query> call(final Subscriber<? super List<T>>
       @Override public void onNext(SqlBrite.Query query) {
         try {
           Cursor cursor = query.run();
+          if (cursor == null) {
+            return;
+          }
           List<T> items = new ArrayList<>(cursor.getCount());
           try {
             for (int i = 1; cursor.moveToNext() && !subscriber.isUnsubscribed(); i++) {

sqlbrite/src/main/java/com/squareup/sqlbrite/QueryToOneOperator.java

@@ -24,18 +24,20 @@ final class QueryToOneOperator<T> implements Observable.Operator<T, SqlBrite.Que
         try {
           T item = null;
           Cursor cursor = query.run();
-          try {
-            if (cursor.moveToNext()) {
-              item = mapper.call(cursor);
-              if (item == null) {
-                throw new NullPointerException("Mapper returned null for row 1");
-              }
+          if (cursor != null) {
+            try {
               if (cursor.moveToNext()) {
-                throw new IllegalStateException("Cursor returned more than 1 row");
+                item = mapper.call(cursor);
+                if (item == null) {
+                  throw new NullPointerException("Mapper returned null for row 1");
+                }
+                if (cursor.moveToNext()) {
+                  throw new IllegalStateException("Cursor returned more than 1 row");
+                }
               }
+            } finally {
+              cursor.close();
             }
-          } finally {
-            cursor.close();
           }
           if (!subscriber.isUnsubscribed()) {
             if (item != null) {

sqlbrite/src/main/java/com/squareup/sqlbrite/SqlBrite.java

@@ -20,6 +20,7 @@
 import android.database.sqlite.SQLiteOpenHelper;
 import android.support.annotation.CheckResult;
 import android.support.annotation.NonNull;
+import android.support.annotation.Nullable;
 import android.util.Log;
 import java.util.List;
 import rx.Observable;
@@ -89,6 +90,8 @@ public static abstract class Query {
      * It is an error for a query to pass through this operator with more than 1 row in its result
      * set. Use {@code LIMIT 1} on the underlying SQL query to prevent this. Result sets with 0 rows
      * do not emit an item.
+     * <p>
+     * This operator ignores {@code null} cursors returned from {@link #run()}.
      *
      * @param mapper Maps the current {@link Cursor} row to {@code T}. May not return null.
      */
@@ -104,6 +107,8 @@ public static <T> Operator<T, Query> mapToOne(@NonNull Func1<Cursor, T> mapper)
      * It is an error for a query to pass through this operator with more than 1 row in its result
      * set. Use {@code LIMIT 1} on the underlying SQL query to prevent this. Result sets with 0 rows
      * emit {@code defaultValue}.
+     * <p>
+     * This operator emits {@code defaultValue} if {@code null} is returned from {@link #run()}.
      *
      * @param mapper Maps the current {@link Cursor} row to {@code T}. May not return null.
      * @param defaultValue Value returned if result set is empty
@@ -121,6 +126,8 @@ public static <T> Operator<T, Query> mapToOneOrDefault(@NonNull Func1<Cursor, T>
      * Be careful using this operator as it will always consume the entire cursor and create objects
      * for each row, every time this observable emits a new query. On tables whose queries update
      * frequently or very large result sets this can result in the creation of many objects.
+     * <p>
+     * This operator ignores {@code null} cursors returned from {@link #run()}.
      *
      * @param mapper Maps the current {@link Cursor} row to {@code T}. May not return null.
      */
@@ -129,9 +136,17 @@ public static <T> Operator<List<T>, Query> mapToList(@NonNull Func1<Cursor, T> m
       return new QueryToListOperator<>(mapper);
     }
 
-    /** Execute the query on the underlying database and return the resulting cursor. */
+    /**
+     * Execute the query on the underlying database and return the resulting cursor.
+     *
+     * @return A {@link Cursor} with query results, or {@code null} when the query could not be
+     * executed due to a problem with the underlying store. Unfortunately it is not well documented
+     * when {@code null} is returned. It usually involves a problem in communicating with the
+     * underlying store and should either be treated as failure or ignored for retry at a later
+     * time.
+     */
     @CheckResult // TODO @WorkerThread
-    // TODO Implementations might return null, which is gross. Throw?
+    @Nullable
     public abstract Cursor run();
 
     /**
@@ -153,18 +168,22 @@ public static <T> Operator<List<T>, Query> mapToList(@NonNull Func1<Cursor, T> m
      * <p>
      * Note: Limiting results or filtering will almost always be faster in the database as part of
      * a query and should be preferred, where possible.
+     * <p>
+     * The resulting observable will be empty if {@code null} is returned from {@link #run()}.
      */
     @CheckResult @NonNull
     public final <T> Observable<T> asRows(final Func1<Cursor, T> mapper) {
       return Observable.create(new Observable.OnSubscribe<T>() {
         @Override public void call(Subscriber<? super T> subscriber) {
           Cursor cursor = run();
-          try {
-            while (cursor.moveToNext() && !subscriber.isUnsubscribed()) {
-              subscriber.onNext(mapper.call(cursor));
+          if (cursor != null) {
+            try {
+              while (cursor.moveToNext() && !subscriber.isUnsubscribed()) {
+                subscriber.onNext(mapper.call(cursor));
+              }
+            } finally {
+              cursor.close();
             }
-          } finally {
-            cursor.close();
           }
           if (!subscriber.isUnsubscribed()) {
             subscriber.onCompleted();