Query stream issue #151

[RTrackerDev]

Minor fix for https://github.com/objectbox/objectbox-dart/issues/151

[RTrackerDev]

@greenrobot @Buggaboo @vaind could you check please?

[vaind]

And almost forgotten... is it possible to test this? Would a small test case to verify the issue was there before (and is not anymore after the changes) be too hard to do? That'd help make avoid future regressions.

[Buggaboo]

test(
      'Only observers of a single entity are notified, no cross-entity observer notification',
      () async {
    // setup listeners
    final box2 = Box<TestEntity2>(env.store);

    var counter1 = 0, counter2 = 0;

    final query2 = box2.query().build();
    final queryStream2 = query2.findStream();
    final subscription2 = queryStream2.listen((_) {
      counter2++;
    });

    final query1 = box.query().build();
    final queryStream1 = query1.findStream();
    final subscription1 = queryStream1.listen((_) {
      counter1++;
    });

    // counter2 test #.1
    final t2 = TestEntity2();
    box2.put(t2);

    await Future.delayed(Duration(seconds: 0));
    expect(counter1, 0);
    expect(counter2, 1);

    // counter1 test #.1
    final t1 = TestEntity();
    box.put(t1);

    await Future.delayed(Duration(seconds: 0));
    expect(counter1, 1);
    expect(counter2, 1);

    // counter1 many test #.2
    final ts1 = [1, 2, 3].map((i) => TestEntity(tInt: i)).toList();
    box.putMany(ts1);

    await Future.delayed(Duration(seconds: 0));
    expect(counter1, 2);
    expect(counter2, 1);

    // counter2 many test #.2
    final ts2 = [1, 2, 3].map((i) => TestEntity2()).toList();
    box2.putMany(ts2);

    await Future.delayed(Duration(seconds: 0));
    expect(counter1, 2);
    expect(counter2, 2);

    query1.close();
    query2.close();

    await subscription1.cancel();
    await subscription2.cancel();
  });

  // TODO query.stream test

I'm looking into it.

[RTrackerDev]

And almost forgotten... is it possible to test this? Would a small test case to verify the issue was there before (and is not anymore after the changes) be too hard to do? That'd help make avoid future regressions.

@vaind I wanted to add tests but I have problems running on mac OS

[Buggaboo]

Have you run 'install.sh'?

Also stripped the 'dart' exec?

[RTrackerDev]

Have you run 'install.sh'?

Also stripped the 'dart' exec?

install.sh ?

[RTrackerDev]

Have you run 'install.sh'?

Also stripped the 'dart' exec?

Thank you i resolved it

[Buggaboo]

I have this feeling the callback in C is not behaving the way it's documented.

Nope. Improper testing on my side.

[RTrackerDev]

@vaind @Buggaboo I added unit test and resolve conflicts

[RTrackerDev]

I have this feeling the callback in C is not behaving the way it's documented.

He is behaving correctly. The problem is that everyone subscribes to a single stream controller, which notifies everyone.

[RTrackerDev]

@Buggaboo In my opinion, the best solution is to move this StreamController as value to the Query and when the Query is closed, close the controller

[Buggaboo]

@vaind Do we still have to close Queries? Or did I forgot to add them?

[Buggaboo]

I found another (conceptual) bug. Fixing it.

The number of C callback notifications should not increase linearly along with the number of dart listeners. Only one C observer per store should be initialized to serve notifications.

[RTrackerDev]

I found another (conceptual) bug. Fixing it.

The number of C callback notifications should not increase linearly along with the number of dart listeners. Only one C observer per store should be initialized to serve notifications.

@vaind @Buggaboo But this PR you could merge.

[Buggaboo]

I took your changes with a fix for another bug I found. I have to redo it again because I based off the wrong branch.

[Buggaboo]

I found another (conceptual) bug. Fixing it. The number of C callback notifications should not increase linearly along with the number of dart listeners. Only one C observer per store should be initialized to serve notifications.

@vaind @Buggaboo But this PR you could merge.

I was looking at an old branch :(, that other problem was fixed already.

[RTrackerDev]

I found another (conceptual) bug. Fixing it. The number of C callback notifications should not increase linearly along with the number of dart listeners. Only one C observer per store should be initialized to serve notifications.

@vaind @Buggaboo But this PR you could merge.

I was looking at an old branch :(, that other problem was fixed already.

What problem was fixed?

[Buggaboo]

I found another (conceptual) bug. Fixing it. The number of C callback notifications should not increase linearly along with the number of dart listeners. Only one C observer per store should be initialized to serve notifications.

@vaind @Buggaboo But this PR you could merge.

I was looking at an old branch :(, that other problem was fixed already.

What problem was fixed?

Only one C observer per store should be initialized to serve notifications.

[Buggaboo]

@vaind, I sent @RTrackerDev a PR with an extra test and a type check appeaser. Once that's merged to this branch, this one is ready to go.

[RTrackerDev]

@vaind @Buggaboo Could you merge if its Ok?

[Buggaboo]

@vaind could do it. I don't have the permissions. @RTrackerDev he's a busy guy. Regardless, your contribution is appreciated. (At least by me)

[vaind]

thx for the fix @RTrackerDev and the support @Buggaboo, merging.

FYI there will be major changes to query streams implementation, to support asynchronous callbacks (across multiple isolates), see #145.