Implement simple Streams on DatabaseViews, fix multi-dao changelistener #320

mqus · 2020-05-01T16:32:48Z

Like I explained on the slack, here is a prototype for Queries with the result Stream<X> where X is a @DatabaseView.

As it won't detect the dependencies of each DatabaseView, it simply updates the Streams of all views if any entity was updated. If a library user is frequently changing his entities while having one particularly intensive view (like a join) as a Stream, it will get quite resource-heavy even if the entity and the DatabaseView of the Stream are not related at all. But its better than nothing.

Of course, I think we should strive to make the Streams of DatabaseViews better by making them aware of the entities they depend on but as mentioned previously this can get very complex.

I also fixed a related issue where Streams of Entity E in DAO B were only updated by insert/update/delete methods in DAO C if there was another query of a Stream of E in C because Streams were only tracked separately for each DAO even if the changeListener was database-global.

TODO:

remove error message for stream queries on views (it doesn't get thrown anymore but it's still there)
implement tests
- implement Tests (only adapted existing ones so far) for the Streams on Views
- implement Test for the fix
implement integration tests
- implement Tests for the Streams on Views
- implement Regression Test for the issue
add documentation and mention that it's expensive

I'll finish this up if you agree on the concept, @vitusortner .

codecov · 2020-05-01T16:37:02Z

Codecov Report

Merging #320 into develop will decrease coverage by 0.03%.
The diff coverage is 59.52%.

@@             Coverage Diff             @@
##           develop     #320      +/-   ##
===========================================
- Coverage    81.47%   81.43%   -0.04%     
===========================================
  Files           59       59              
  Lines         1506     1503       -3     
===========================================
- Hits          1227     1224       -3     
  Misses         279      279

Flag	Coverage Δ
#floor	`81.43% <59.52%> (-0.04%)`	⬇️
#floor_generator	`81.09% <57.50%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
.../processor/error/query_method_processor_error.dart	`100.00% <ø> (ø)`
floor_generator/lib/value_object/dao.dart	`4.00% <0.00%> (-0.35%)`	⬇️
floor_generator/lib/value_object/database.dart	`12.00% <22.22%> (+6.73%)`	⬆️
floor/lib/src/adapter/query_adapter.dart	`92.68% <100.00%> (ø)`
floor_generator/lib/processor/dao_processor.dart	`100.00% <100.00%> (ø)`
...enerator/lib/processor/query_method_processor.dart	`100.00% <100.00%> (ø)`
floor_generator/lib/writer/dao_writer.dart	`100.00% <100.00%> (ø)`
...loor_generator/lib/writer/query_method_writer.dart	`100.00% <100.00%> (ø)`
floor_generator/lib/value_object/view.dart	`90.00% <0.00%> (+30.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49fbff2...681d059. Read the comment docs.

vitusortner

Thanks for these changes! Until we have proper query parsing in place, this seems to be a good solution!

Can you please mark the area for the fix? I had trouble finding it 🙏

floor_generator/lib/processor/dao_processor.dart

floor_generator/lib/value_object/dao.dart

floor_generator/lib/writer/query_method_writer.dart

floor_generator/lib/generator.dart

floor_generator/lib/value_object/database.dart

cassioseffrin · 2020-05-20T20:39:07Z

Do you have an estimative when streams on views will be released?

mqus · 2020-05-20T21:32:11Z

Not exactly but maybe soon. There are just some tests left to do which I will try to tackle tomorrow. vitusortner will review again afterwards, although there shouldn't be major issues left, so this would then be ready to be merged. I don't know if there are plans for a new release.

But as mentioned, this is just a (possibly runtime-intensive)stopgap solution until I'm finished with integrating the sqlparser, which will do query-analysis to be more efficient when updating the Stream.
But I really can't give an ETA for that and help is appreciated.
EDIT: I added a Roadmap for the finished Queryparser to the tracking issue: #321

cassioseffrin · 2020-05-20T23:15:03Z

Query-analysis will be awesome! I am waiting for it!

mqus · 2020-05-21T11:49:46Z

@vitusortner I noticed that we actually don't test streams quite the right way. Floor's Streams will re-execute the query as soon someone will start to Listen (which is right this way), but that also means that each

expect(actual, emits([anything]));

in the tests just starts listening and reexecutes the query, not actually looking if an event was fired independently. Previously fired events will be discarded if no-one is actually listening. This means that our Stream tests are just checking if the insert/update/delete query got executed, not if the stream fired.

For the new tests added here I'm using

expect(actual, emitsInOrder([firstResult,secondResult]));

at the start of the test, which will keep the stream open and actually checks the stream output.

When doing this I noticed that some queries will be executed together before the re-execution of the Stream Query fires, meaning that some events get "lost" and other events will then get a double update.

For that I introduced a new helper function that will introduce artificial wait times to let floor reexecute the SELECT query before the test starts the next database action. I'll push that soon.

mqus · 2020-05-22T18:09:16Z

The last commit is not really necessary... you decide if you want it :)

README.md

floor/README.md

floor/test/integration/dao/person_dao.dart

floor_generator/lib/writer/query_method_writer.dart

floor_generator/lib/processor/dao_processor.dart

floor/test/integration/view/view_test.dart

floor/test/test_util/separate_queries.dart

floor/test/integration/stream_query_test.dart

vitusortner · 2020-05-23T09:59:59Z

@vitusortner I noticed that we actually don't test streams quite the right way. Floor's Streams will re-execute the query as soon someone will start to Listen (which is right this way), but that also means that each
expect(actual, emits([anything]));
in the tests just starts listening and reexecutes the query, not actually looking if an event was fired independently. Previously fired events will be discarded if no-one is actually listening. This means that our Stream tests are just checking if the insert/update/delete query got executed, not if the stream fired.

For the new tests added here I'm using
expect(actual, emitsInOrder([firstResult,secondResult]));
at the start of the test, which will keep the stream open and actually checks the stream output.

When doing this I noticed that some queries will be executed together before the re-execution of the Stream Query fires, meaning that some events get "lost" and other events will then get a double update.

For that I introduced a new helper function that will introduce artificial wait times to let floor reexecute the SELECT query before the test starts the next database action. I'll push that soon.

Good find! Thanks

some quick fixes Co-authored-by: Vitus <[email protected]>

vitusortner

Looking good 🥇 I'm excited to see this getting merged into the main branch. I'll also create a new release containing these changes in the next days.

As Codecov is blocking this PR from getting merged, I suggest that I'll merge it if you give your go?

mqus · 2020-05-23T19:46:54Z

As Codecov is blocking this PR from getting merged, I suggest that I'll merge it if you give your go?

yes please :)

mqus marked this pull request as draft May 1, 2020 16:33

mqus added the feature Request and implementation of a feature (release drafter) label May 1, 2020

mqus mentioned this pull request May 2, 2020

Add a query parser #321

Open

9 tasks

vitusortner reviewed May 8, 2020

View reviewed changes

mqus commented May 8, 2020

View reviewed changes

floor_generator/lib/generator.dart Outdated Show resolved Hide resolved

mqus commented May 8, 2020

View reviewed changes

floor_generator/lib/value_object/database.dart Show resolved Hide resolved

mqus added 4 commits May 20, 2020 16:08

Implement simple Streams on DatabaseViews, fix multi-dao changelistener

ddcb2b8

clean up

c135ce7

update generated db of example project

86d8a6d

apply suggestions from review

d2539a0

mqus force-pushed the expensive-view-streams branch from 7fd841c to d2539a0 Compare May 20, 2020 14:35

mqus added 2 commits May 20, 2020 22:50

Add tests, fix errors

9c97ff5

fix generated example code and run dartfmt in example/

15827f4

add tests

c927713

mqus changed the title ~~WIP: Implement simple Streams on DatabaseViews, fix multi-dao changelistener~~ Implement simple Streams on DatabaseViews, fix multi-dao changelistener May 21, 2020

mqus marked this pull request as ready for review May 21, 2020 12:26

mqus requested a review from vitusortner May 21, 2020 21:24

Simplify QueryMethodProcessor

b0b237f

mqus force-pushed the expensive-view-streams branch from e1d8d7c to b0b237f Compare May 22, 2020 18:06

vitusortner requested changes May 23, 2020

View reviewed changes

mqus and others added 3 commits May 23, 2020 19:17

Apply suggestions from code review

3f58399

some quick fixes Co-authored-by: Vitus <[email protected]>

Apply suggestions from second review, part 2

02c5c95

Merge branch 'develop' into expensive-view-streams

681d059

vitusortner approved these changes May 23, 2020

View reviewed changes

mqus mentioned this pull request May 23, 2020

Streams over single values do not emit null values #338

Closed

vitusortner merged commit 527b0cb into develop May 23, 2020

vitusortner deleted the expensive-view-streams branch May 23, 2020 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement simple Streams on DatabaseViews, fix multi-dao changelistener #320

Implement simple Streams on DatabaseViews, fix multi-dao changelistener #320

mqus commented May 1, 2020 •

edited

Loading

codecov bot commented May 1, 2020 •

edited

Loading

vitusortner left a comment

cassioseffrin commented May 20, 2020

mqus commented May 20, 2020 •

edited

Loading

cassioseffrin commented May 20, 2020

mqus commented May 21, 2020

mqus commented May 22, 2020

vitusortner commented May 23, 2020

vitusortner left a comment •

edited

Loading

mqus commented May 23, 2020

Implement simple Streams on DatabaseViews, fix multi-dao changelistener #320

Implement simple Streams on DatabaseViews, fix multi-dao changelistener #320

Conversation

mqus commented May 1, 2020 • edited Loading

TODO:

codecov bot commented May 1, 2020 • edited Loading

Codecov Report

vitusortner left a comment

Choose a reason for hiding this comment

cassioseffrin commented May 20, 2020

mqus commented May 20, 2020 • edited Loading

cassioseffrin commented May 20, 2020

mqus commented May 21, 2020

mqus commented May 22, 2020

vitusortner commented May 23, 2020

vitusortner left a comment • edited Loading

Choose a reason for hiding this comment

mqus commented May 23, 2020

mqus commented May 1, 2020 •

edited

Loading

codecov bot commented May 1, 2020 •

edited

Loading

mqus commented May 20, 2020 •

edited

Loading

vitusortner left a comment •

edited

Loading