Merge branch '75-query-during-query' into 'main'

Fix incomplete results if query runs while visiting results

See merge request objectbox/objectbox-dart!53

Author: greenrobot-team

objectbox/CHANGELOG.md

@@ -1,5 +1,8 @@
 ## latest
 
+* Resolve an issue where not all query results are returned, when an entity constructor or property 
+  setter itself executes a query. [#550](https://github.com/objectbox/objectbox-dart/issues/550)
+
 ## 2.2.0 (2023-08-08)
 
 * For Flutter apps running on Android 6 (or older): added `loadObjectBoxLibraryAndroidCompat()` to 

objectbox/lib/src/native/bindings/data_visitor.dart

@@ -1,58 +1,59 @@
 import 'dart:ffi';
 
-import '../../modelinfo/entity_definition.dart';
-import '../store.dart';
 import 'bindings.dart';
+import 'helpers.dart';
 
-// ignore_for_file: public_member_api_docs
+/// Callback for reading data one-by-one, see [visit].
+typedef VisitCallback = bool Function(Pointer<Uint8> data, int size);
 
-/// When you want to pass a dart callback to a C function you cannot use lambdas
-/// and instead the callback must be a static function - giving a lambda to
-/// [Pointer.fromFunction()] won't compile:
-///      Error: fromFunction expects a static function as parameter.
-///      dart:ffi only supports calling static Dart functions from native code.
+/// Currently FFI's Pointer.fromFunction only allows to pass a static Dart
+/// callback function. When passing a closure it would throw at runtime:
+///     Error: fromFunction expects a static function as parameter.
+///     dart:ffi only supports calling static Dart functions from native code.
+///     Closures and tear-offs are not supported because they can capture context.
 ///
-/// With Dart being all synchronous and not sharing memory at all within a
-/// single isolate, we can just alter a single global callback variable.
-/// Therefore, let's have a single static function [_forwarder] converted to a
-/// native visitor pointer [_nativeVisitor], calling [_callback] in the end.
-
-bool Function(Pointer<Uint8> data, int size) _callback = _callback;
+/// So given that and
+/// - it is required that each query has its own callback,
+/// - it is possible that as part of visiting results another query is created
+///   and visits its results (e.g. query run in entity constructor or setter) and
+/// - Dart code within an isolate is executed synchronously:
+///
+/// Create a single static callback function [_callbackWrapper] that wraps
+/// the actual Dart callback of the query currently visiting results.
+/// Keep callbacks on a [_callbackStack] to restore the callback of an outer
+/// query once a nested query is finished visiting results.
+List<VisitCallback> _callbackStack = [];
 
-bool _forwarder(Pointer<Uint8> dataPtr, int size, Pointer<Void> _) =>
-    _callback(dataPtr, size);
+bool _callbackWrapper(Pointer<Uint8> dataPtr, int size, Pointer<Void> _) =>
+    _callbackStack.last(dataPtr, size);
 
-final Pointer<obx_data_visitor> _nativeVisitor =
-    Pointer.fromFunction(_forwarder, false);
+final Pointer<obx_data_visitor> _callbackWrapperPtr =
+    Pointer.fromFunction(_callbackWrapper, false);
 
-/// The callback for reading data one-by-one.
+/// Visits query results.
 ///
+/// Pass a [callback] for reading data one-by-one:
 /// - [data] is the read data buffer.
 /// - [size] specifies the length of the read data.
 /// - Return true to keep going, false to cancel.
 @pragma('vm:prefer-inline')
-Pointer<obx_data_visitor> dataVisitor(
-    bool Function(Pointer<Uint8> data, int size) callback) {
-  _callback = callback;
-  return _nativeVisitor;
+void visit(Pointer<OBX_query> queryPtr, VisitCallback callback) {
+  // Keep callback in case another query is created and visits results
+  // within the callback.
+  _callbackStack.add(callback);
+  final code = C.query_visit(queryPtr, _callbackWrapperPtr, nullptr);
+  _callbackStack.removeLast();
+  // Clean callback from stack before potentially throwing.
+  checkObx(code);
 }
 
-@pragma('vm:prefer-inline')
-Pointer<obx_data_visitor> objectCollector<T>(List<T> list, Store store,
-        EntityDefinition<T> entity, ObjectCollectorError outError) =>
-    dataVisitor((Pointer<Uint8> data, int size) {
-      try {
-        list.add(entity.objectFromData(store, data, size));
-        return true;
-      } catch (e) {
-        outError.error = e;
-        return false;
-      }
-    });
-
-class ObjectCollectorError {
+/// Can be used with [visit] to get an error out of the callback.
+class ObjectVisitorError {
+  /// Set this e.g. to an exception that occurred inside the callback.
   Object? error;
 
+  /// Call once visiting is finished. If an exception is set to [error] will
+  /// re-throw it.
   void throwIfError() {
     if (error != null) throw error!;
   }

objectbox/lib/src/native/query/query.dart

@@ -835,17 +835,18 @@ class Query<T> implements Finalizable {
   /// results. Note: [offset] and [limit] are respected, if set.
   T? findFirst() {
     T? result;
-    Object? error;
-    final visitor = dataVisitor((Pointer<Uint8> data, int size) {
+    final errorWrapper = ObjectVisitorError();
+    visitCallBack(Pointer<Uint8> data, int size) {
       try {
         result = _entity.objectFromData(_store, data, size);
       } catch (e) {
-        error = e;
+        errorWrapper.error = e;
       }
       return false; // we only want to visit the first element
-    });
-    checkObx(C.query_visit(_ptr, visitor, nullptr));
-    if (error != null) throw error!;
+    }
+
+    visit(_ptr, visitCallBack);
+    errorWrapper.throwIfError();
     return result;
   }
 
@@ -868,24 +869,25 @@ class Query<T> implements Finalizable {
   /// higher than one, otherwise the check for non-unique result won't work.
   T? findUnique() {
     T? result;
-    Object? error;
-    final visitor = dataVisitor((Pointer<Uint8> data, int size) {
+    final errorWrapper = ObjectVisitorError();
+    visitCallback(Pointer<Uint8> data, int size) {
       if (result == null) {
         try {
           result = _entity.objectFromData(_store, data, size);
           return true;
         } catch (e) {
-          error = e;
+          errorWrapper.error = e;
           return false;
         }
       } else {
-        error = NonUniqueResultException(
+        errorWrapper.error = NonUniqueResultException(
             'Query findUnique() matched more than one object');
         return false;
       }
-    });
-    checkObx(C.query_visit(_ptr, visitor, nullptr));
-    if (error != null) throw error!;
+    }
+
+    visit(_ptr, visitCallback);
+    errorWrapper.throwIfError();
     return result;
   }
 
@@ -924,9 +926,18 @@ class Query<T> implements Finalizable {
   /// Finds Objects matching the query.
   List<T> find() {
     final result = <T>[];
-    final errorWrapper = ObjectCollectorError();
-    final collector = objectCollector(result, _store, _entity, errorWrapper);
-    checkObx(C.query_visit(_ptr, collector, nullptr));
+    final errorWrapper = ObjectVisitorError();
+    visitCallback(Pointer<Uint8> data, int size) {
+      try {
+        result.add(_entity.objectFromData(_store, data, size));
+        return true;
+      } catch (e) {
+        errorWrapper.error = e;
+        return false;
+      }
+    }
+
+    visit(_ptr, visitCallback);
     errorWrapper.throwIfError();
     return result;
   }
@@ -1174,7 +1185,7 @@ class Query<T> implements Finalizable {
         var dataPtrBatch = List<int>.filled(maxBatchSize, 0);
         var sizeBatch = List<int>.filled(maxBatchSize, 0);
         var batchSize = 0;
-        final visitor = dataVisitor((Pointer<Uint8> data, int size) {
+        visitCallback(Pointer<Uint8> data, int size) {
           // Currently returning all results, even if the stream has been closed
           // before (e.g. only first element taken). Would need a way to check
           // for exit command on commandPort synchronously.
@@ -1190,11 +1201,12 @@ class Query<T> implements Finalizable {
             batchSize = 0;
           }
           return true;
-        });
+        }
+
         final queryPtr =
             Pointer<OBX_query>.fromAddress(isolateInit.queryPtrAddress);
         try {
-          checkObx(C.query_visit(queryPtr, visitor, nullptr));
+          visit(queryPtr, visitCallback);
         } catch (e) {
           resultPort.send(e);
           return;

objectbox_test/test/box_test.dart

@@ -1072,7 +1072,7 @@ void main() {
   });
 
   // https://github.com/objectbox/objectbox-dart/issues/550
-  test("read during object creation", () {
+  test("get with nested get reads all properties", () {
     final box = env.store.box<TestEntityReadDuringRead>();
     final id =
         box.put(TestEntityReadDuringRead()..strings2 = ["A2", "B2", "C3"]);
@@ -1086,6 +1086,48 @@ void main() {
     readDuringReadCalledFromSetter = null; // Do not leak the box instance.
     expect(actual.strings2, hasLength(3));
   });
+
+  // https://github.com/objectbox/objectbox-dart/issues/550
+  test("query with nested query returns all results", () {
+    final box = env.store.box<TestEntityReadDuringRead>();
+    box.put(TestEntityReadDuringRead());
+    box.put(TestEntityReadDuringRead());
+    // Do a query of another box (to avoid stack overflow)
+    // as part of calling a property setter.
+    env.box.putMany(simpleItems());
+    var nestedCountUnexpected = false;
+    readDuringReadCalledFromSetter = () {
+      final query = env.box.query().build();
+      final count = query.find().length;
+      if (count != 6) nestedCountUnexpected = true;
+      query.close();
+    };
+    final query = box.query().build();
+    final all = query.find();
+    query.close();
+
+    expect(all.length, 2);
+    expect(nestedCountUnexpected, false);
+
+    readDuringReadCalledFromSetter = null; // Do not leak the box instance.
+  });
+
+  // https://github.com/objectbox/objectbox-dart/issues/550
+  test("query with nested throwing query forwards error", () {
+    final box = env.store.box<TestEntityReadDuringRead>();
+    final id = box.put(TestEntityReadDuringRead());
+    // Query for an object that will throw when being read
+    // as part of calling a property setter.
+    final throwingBox = env.store.box<ThrowingInConverters>();
+    throwingBox.put(ThrowingInConverters(throwOnGet: true));
+    readDuringReadCalledFromSetter = () {
+      throwingBox.query().build().find();
+    };
+    // The outer query should forward the exception of the nested query.
+    expect(() => box.get(id), ThrowingInConverters.throwsIn("Setter"));
+
+    readDuringReadCalledFromSetter = null; // Do not leak the box instance.
+  });
 }
 
 List<TestEntity> simpleItems() => ['One', 'Two', 'Three', 'Four', 'Five', 'Six']