Merge branch '196-query-build-fail-fatal' into 'dev'

Guard any Store pointer access with open check

See merge request objectbox/objectbox-java!128

Author: greenrobot-team

objectbox-java/src/main/java/io/objectbox/Box.java

@@ -574,7 +574,7 @@ public long panicModeRemoveAll() {
      * Returns a builder to create queries for Object matching supplied criteria.
      */
     public QueryBuilder<T> query() {
-        return new QueryBuilder<>(this, store.internalHandle(), store.getDbName(entityClass));
+        return new QueryBuilder<>(this, store.getNativeStore(), store.getDbName(entityClass));
     }
 
     /**

objectbox-java/src/main/java/io/objectbox/BoxStore.java

@@ -220,7 +220,8 @@ public static boolean isSyncServerAvailable() {
 
     private final File directory;
     private final String canonicalPath;
-    private final long handle;
+    /** Reference to the native store. Should probably get through {@link #getNativeStore()} instead. */
+    private long handle;
     private final Map<Class<?>, String> dbNameByClass = new HashMap<>();
     private final Map<Class<?>, Integer> entityTypeIdByClass = new HashMap<>();
     private final Map<Class<?>, EntityInfo<?>> propertiesByClass = new HashMap<>();
@@ -467,11 +468,12 @@ public static long sysProcStatusKb(String key) {
      * @return 0 if the size could not be determined (does not throw unless this store was already closed)
      */
     public long sizeOnDisk() {
-        checkOpen();
-        return nativeSizeOnDisk(handle);
+        return nativeSizeOnDisk(getNativeStore());
     }
 
     /**
+     * Closes this if this is finalized.
+     * <p>
      * Explicitly call {@link #close()} instead to avoid expensive finalization.
      */
     @SuppressWarnings("deprecation") // finalize()
@@ -481,8 +483,11 @@ protected void finalize() throws Throwable {
         super.finalize();
     }
 
+    /**
+     * Verifies this has not been {@link #close() closed}.
+     */
     private void checkOpen() {
-        if (closed) {
+        if (isClosed()) {
             throw new IllegalStateException("Store is closed");
         }
     }
@@ -533,13 +538,12 @@ <T> EntityInfo<T> getEntityInfo(Class<T> entityClass) {
      */
     @Internal
     public Transaction beginTx() {
-        checkOpen();
         // Because write TXs are typically not cached, initialCommitCount is not as relevant than for read TXs.
         int initialCommitCount = commitCount;
         if (debugTxWrite) {
             System.out.println("Begin TX with commit count " + initialCommitCount);
         }
-        long nativeTx = nativeBeginTx(handle);
+        long nativeTx = nativeBeginTx(getNativeStore());
         if (nativeTx == 0) throw new DbException("Could not create native transaction");
 
         Transaction tx = new Transaction(this, nativeTx, initialCommitCount);
@@ -555,7 +559,6 @@ public Transaction beginTx() {
      */
     @Internal
     public Transaction beginReadTx() {
-        checkOpen();
         // initialCommitCount should be acquired before starting the tx. In race conditions, there is a chance the
         // commitCount is already outdated. That's OK because it only gives a false positive for an TX being obsolete.
         // In contrast, a false negative would make a TX falsely not considered obsolete, and thus readers would not be
@@ -565,7 +568,7 @@ public Transaction beginReadTx() {
         if (debugTxRead) {
             System.out.println("Begin read TX with commit count " + initialCommitCount);
         }
-        long nativeTx = nativeBeginReadTx(handle);
+        long nativeTx = nativeBeginReadTx(getNativeStore());
         if (nativeTx == 0) throw new DbException("Could not create native read transaction");
 
         Transaction tx = new Transaction(this, nativeTx, initialCommitCount);
@@ -575,6 +578,9 @@ public Transaction beginReadTx() {
         return tx;
     }
 
+    /**
+     * If this was {@link #close() closed}.
+     */
     public boolean isClosed() {
         return closed;
     }
@@ -584,8 +590,7 @@ public boolean isClosed() {
      * If true the schema is not updated and write transactions are not possible.
      */
     public boolean isReadOnly() {
-        checkOpen();
-        return nativeIsReadOnly(handle);
+        return nativeIsReadOnly(getNativeStore());
     }
 
     /**
@@ -621,7 +626,9 @@ public void close() {
                 }
                 if (handle != 0) { // failed before native handle was created?
                     nativeDelete(handle);
-                    // TODO set handle to 0 and check in native methods
+                    // The Java API has open checks, but just in case re-set the handle so any native methods will
+                    // not crash due to an invalid pointer.
+                    handle = 0;
                 }
 
                 // When running the full unit test suite, we had 100+ threads before, hope this helps:
@@ -665,7 +672,7 @@ private void checkThreadTermination() {
      * Note: If false is returned, any number of files may have been deleted before the failure happened.
      */
     public boolean deleteAllFiles() {
-        if (!closed) {
+        if (!isClosed()) {
             throw new IllegalStateException("Store must be closed");
         }
         return deleteAllFiles(directory);
@@ -765,8 +772,7 @@ public static boolean deleteAllFiles(@Nullable File baseDirectoryOrNull, @Nullab
      * </ul>
      */
     public void removeAllObjects() {
-        checkOpen();
-        nativeDropAllData(handle);
+        nativeDropAllData(getNativeStore());
     }
 
     @Internal
@@ -1049,8 +1055,7 @@ public <R> void callInTxAsync(final Callable<R> callable, @Nullable final TxCall
      * @return String that is typically logged by the application.
      */
     public String diagnose() {
-        checkOpen();
-        return nativeDiagnose(handle);
+        return nativeDiagnose(getNativeStore());
     }
 
     /**
@@ -1069,13 +1074,11 @@ public long validate(long pageLimit, boolean checkLeafLevel) {
         if (pageLimit < 0) {
             throw new IllegalArgumentException("pageLimit must be zero or positive");
         }
-        checkOpen();
-        return nativeValidate(handle, pageLimit, checkLeafLevel);
+        return nativeValidate(getNativeStore(), pageLimit, checkLeafLevel);
     }
 
     public int cleanStaleReadTransactions() {
-        checkOpen();
-        return nativeCleanStaleReadTransactions(handle);
+        return nativeCleanStaleReadTransactions(getNativeStore());
     }
 
     /**
@@ -1090,11 +1093,6 @@ public void closeThreadResources() {
         // activeTx is cleaned up in finally blocks, so do not free them here
     }
 
-    @Internal
-    long internalHandle() {
-        return handle;
-    }
-
     /**
      * A {@link io.objectbox.reactive.DataObserver} can be subscribed to data changes using the returned builder.
      * The observer is supplied via {@link SubscriptionBuilder#observer(DataObserver)} and will be notified once a
@@ -1146,8 +1144,7 @@ public String startObjectBrowser() {
     @Nullable
     public String startObjectBrowser(int port) {
         verifyObjectBrowserNotRunning();
-        checkOpen();
-        String url = nativeStartObjectBrowser(handle, null, port);
+        String url = nativeStartObjectBrowser(getNativeStore(), null, port);
         if (url != null) {
             objectBrowserPort = port;
         }
@@ -1158,14 +1155,13 @@ public String startObjectBrowser(int port) {
     @Nullable
     public String startObjectBrowser(String urlToBindTo) {
         verifyObjectBrowserNotRunning();
-        checkOpen();
         int port;
         try {
             port = new URL(urlToBindTo).getPort(); // Gives -1 if not available
         } catch (MalformedURLException e) {
             throw new RuntimeException("Can not start Object Browser at " + urlToBindTo, e);
         }
-        String url = nativeStartObjectBrowser(handle, urlToBindTo, 0);
+        String url = nativeStartObjectBrowser(getNativeStore(), urlToBindTo, 0);
         if (url != null) {
             objectBrowserPort = port;
         }
@@ -1178,8 +1174,7 @@ public synchronized boolean stopObjectBrowser() {
             throw new IllegalStateException("ObjectBrowser has not been started before");
         }
         objectBrowserPort = 0;
-        checkOpen();
-        return nativeStopObjectBrowser(handle);
+        return nativeStopObjectBrowser(getNativeStore());
     }
 
     @Experimental
@@ -1204,8 +1199,7 @@ private void verifyObjectBrowserNotRunning() {
      * This for example allows central error handling or special logging for database-related exceptions.
      */
     public void setDbExceptionListener(@Nullable DbExceptionListener dbExceptionListener) {
-        checkOpen();
-        nativeSetDbExceptionListener(handle, dbExceptionListener);
+        nativeSetDbExceptionListener(getNativeStore(), dbExceptionListener);
     }
 
     @Internal
@@ -1234,18 +1228,19 @@ public TxCallback<?> internalFailedReadTxAttemptCallback() {
     }
 
     void setDebugFlags(int debugFlags) {
-        checkOpen();
-        nativeSetDebugFlags(handle, debugFlags);
+        nativeSetDebugFlags(getNativeStore(), debugFlags);
     }
 
     long panicModeRemoveAllObjects(int entityId) {
-        checkOpen();
-        return nativePanicModeRemoveAllObjects(handle, entityId);
+        return nativePanicModeRemoveAllObjects(getNativeStore(), entityId);
     }
 
     /**
-     * If you want to use the same ObjectBox store using the C API, e.g. via JNI, this gives the required pointer,
-     * which you have to pass on to obx_store_wrap().
+     * Gets the reference to the native store. Can be used with the C API to use the same store, e.g. via JNI, by
+     * passing it on to {@code obx_store_wrap()}.
+     * <p>
+     * Throws if the store is closed.
+     * <p>
      * The procedure is like this:<br>
      * 1) you create a BoxStore on the Java side<br>
      * 2) you call this method to get the native store pointer<br>

objectbox-java/src/main/java/io/objectbox/InternalAccess.java

@@ -23,13 +23,6 @@
 
 @Internal
 public class InternalAccess {
-    public static <T> Cursor<T> getReader(Box<T> box) {
-        return box.getReader();
-    }
-
-    public static long getHandle(BoxStore boxStore) {
-        return boxStore.internalHandle();
-    }
 
     public static Transaction getActiveTx(BoxStore boxStore) {
         Transaction tx = boxStore.activeTx.get();
@@ -40,10 +33,6 @@ public static Transaction getActiveTx(BoxStore boxStore) {
         return tx;
     }
 
-    public static long getHandle(Cursor reader) {
-        return reader.internalHandle();
-    }
-
     public static long getHandle(Transaction tx) {
         return tx.internalHandle();
     }
@@ -52,10 +41,6 @@ public static void setSyncClient(BoxStore boxStore, @Nullable SyncClient syncCli
         boxStore.setSyncClient(syncClient);
     }
 
-    public static <T> void releaseReader(Box<T> box, Cursor<T> reader) {
-        box.releaseReader(reader);
-    }
-
     public static <T> Cursor<T> getWriter(Box<T> box) {
         return box.getWriter();
     }
@@ -68,10 +53,6 @@ public static <T> long getActiveTxCursorHandle(Box<T> box) {
         return box.getActiveTxCursor().internalHandle();
     }
 
-    public static <T> void releaseWriter(Box<T> box, Cursor<T> writer) {
-        box.releaseWriter(writer);
-    }
-
     public static <T> void commitWriter(Box<T> box, Cursor<T> writer) {
         box.commitWriter(writer);
     }

objectbox-java/src/main/java/io/objectbox/sync/SyncClientImpl.java

@@ -48,7 +48,7 @@ public class SyncClientImpl implements SyncClient {
         this.serverUrl = builder.url;
         this.connectivityMonitor = builder.platform.getConnectivityMonitor();
 
-        long boxStoreHandle = InternalAccess.getHandle(builder.boxStore);
+        long boxStoreHandle = builder.boxStore.getNativeStore();
         long handle = nativeCreate(boxStoreHandle, serverUrl, builder.trustedCertPaths);
         if (handle == 0) {
             throw new RuntimeException("Failed to create sync client: handle is zero.");

objectbox-java/src/main/java/io/objectbox/sync/server/SyncServerImpl.java

@@ -2,7 +2,6 @@
 
 import javax.annotation.Nullable;
 
-import io.objectbox.InternalAccess;
 import io.objectbox.annotation.apihint.Internal;
 import io.objectbox.sync.SyncCredentials;
 import io.objectbox.sync.SyncCredentialsToken;
@@ -24,7 +23,7 @@ public class SyncServerImpl implements SyncServer {
     SyncServerImpl(SyncServerBuilder builder) {
         this.url = builder.url;
 
-        long storeHandle = InternalAccess.getHandle(builder.boxStore);
+        long storeHandle = builder.boxStore.getNativeStore();
         long handle = nativeCreate(storeHandle, url, builder.certificatePath);
         if (handle == 0) {
             throw new RuntimeException("Failed to create sync server: handle is zero.");

tests/objectbox-java-test/src/test/java/io/objectbox/BoxStoreTest.java

@@ -136,7 +136,7 @@ public void testClose() {
         assertThrowsStoreIsClosed(() -> store.subscribe(TestEntity.class));
         assertThrowsStoreIsClosed(store::startObjectBrowser);
         assertThrowsStoreIsClosed(() -> store.startObjectBrowser(12345));
-        assertThrowsStoreIsClosed(() -> store.startObjectBrowser(""));
+        assertThrowsStoreIsClosed(() -> store.startObjectBrowser("http://127.0.0.1"));
         // assertThrowsStoreIsClosed(store::stopObjectBrowser); // Requires mocking, not testing for now.
         assertThrowsStoreIsClosed(() -> store.setDbExceptionListener(null));
         // Internal thread pool is shut down as part of closing store, should no longer accept new work.

tests/objectbox-java-test/src/test/java/io/objectbox/query/QueryTest.java

@@ -16,27 +16,27 @@
 
 package io.objectbox.query;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Date;
+import java.util.List;
+
 import io.objectbox.Box;
 import io.objectbox.BoxStore;
 import io.objectbox.BoxStoreBuilder;
 import io.objectbox.TestEntity;
 import io.objectbox.TestEntity_;
 import io.objectbox.TestUtils;
+import io.objectbox.config.DebugFlags;
 import io.objectbox.exception.DbExceptionListener;
 import io.objectbox.exception.NonUniqueResultException;
-import io.objectbox.config.DebugFlags;
 import io.objectbox.query.QueryBuilder.StringOrder;
 import io.objectbox.relation.MyObjectBox;
 import io.objectbox.relation.Order;
 import io.objectbox.relation.Order_;
 import org.junit.Test;
 import org.junit.function.ThrowingRunnable;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Date;
-import java.util.List;
-
 import static io.objectbox.TestEntity_.simpleBoolean;
 import static io.objectbox.TestEntity_.simpleByteArray;
 import static io.objectbox.TestEntity_.simpleFloat;
@@ -58,6 +58,17 @@
 
 public class QueryTest extends AbstractQueryTest {
 
+    @Test
+    public void createIfStoreClosed_throws() {
+        store.close();
+
+        IllegalStateException ex = assertThrows(
+                IllegalStateException.class,
+                () -> box.query()
+        );
+        assertEquals("Store is closed", ex.getMessage());
+    }
+
     @Test
     public void testBuild() {
         try (Query<TestEntity> query = box.query().build()) {