Feature: Add support for @DatabaseView annotations #262

mqus · 2020-02-15T19:57:27Z

This adds support for @DatabaseView annotations known from room. One test was added, further testing is needed. Implements #130. I tried to do minimal adjustments to the codebase while copying the handling of entities. I took over the Field handling from there and then just ignored the nullable property by implementing the createStatement function differently.

As I already said in #259, I may have no idea what I'm doing, so please tell me if I did something wrong or if I can do something better.

I'll try to add some more tests before this is ready to merge. Are there some specific things you would like to see tested?

mqus · 2020-02-16T13:23:36Z

The test that's currently failing is the same as in #259 and as far as I can tell, this fails even on the current develop branch. I assume that some underlying framework (dart?) got changed but I couldn't find the issue with my very limited knowledge of Streams.

vitusortner · 2020-02-17T08:34:24Z

At a first glance, this looks great 🎉 Thanks for your contribution! I'll review the full PR as soon as possible.

mqus · 2020-02-19T21:31:53Z

Some remarks about hacks I did and shouldn't change back myself right now:

floor_generator relies on floor_annotation via path: ../floor_annotation because it needs the new annotations. This has to be changed before the next release but can't be changed right now because nothing would work anymore.
the same for floor, which relies on floor_generator and floor_annotation.

I also messed up my branches a bit when starting out so view_processor.dart already checks for Uint8List, even if the whole rest of the code can't do anything with it. If you want to merge this PR before #259, I'll remove it. If #259 comes first, I'll rebase and use the proper getter there.

vitusortner · 2020-02-21T21:44:30Z

floor_generator relies on floor_annotation via path: ../floor_annotation because it needs the new annotations. This has to be changed before the next release but can't be changed right now because nothing would work anymore.

the same for floor, which relies on floor_generator and floor_annotation.

No problem. Normally, I only change the paths of the library's packages to point to the versions available on pub while creating a release. I must have forgotten to revert it to referring the local version. You can leave it as it is.

I also messed up my branches a bit when starting out so view_processor.dart already checks for Uint8List, even if the whole rest of the code can't do anything with it. If you want to merge this PR before #259, I'll remove it. If #259 comes first, I'll rebase and use the proper getter there.

Let's merge #259 before.

mqus · 2020-02-23T14:49:38Z

I rebased the PR onto the current develop branch and altered view_processor.dart as written before.
I know that an integration test for the Views is still missing and maybe more, do you have any suggestions on what/how should be tested?

codecov · 2020-03-11T12:48:45Z

Codecov Report

Merging #262 into develop will decrease coverage by 0.61%.
The diff coverage is 82.80%.

@@             Coverage Diff             @@
##           develop     #262      +/-   ##
===========================================
- Coverage    81.27%   80.66%   -0.62%     
===========================================
  Files           51       48       -3     
  Lines         1383     1298      -85     
===========================================
- Hits          1124     1047      -77     
+ Misses         259      251       -8

Flag	Coverage Δ
#floor	`?`
#floor_generator	`80.66% <82.80%> (+0.06%)`	⬆️

Impacted Files	Coverage Δ
floor_generator/lib/misc/constants.dart	`100.00% <ø> (ø)`
floor_generator/lib/value_object/database.dart	`5.26% <0.00%> (-0.62%)`	⬇️
...ator/lib/processor/error/view_processor_error.dart	`20.00% <20.00%> (ø)`
floor_generator/lib/value_object/query_method.dart	`64.28% <33.33%> (ø)`
floor_generator/lib/value_object/view.dart	`74.19% <74.19%> (ø)`
...or_generator/lib/processor/database_processor.dart	`76.27% <82.35%> (+1.27%)`	⬆️
...r_generator/lib/processor/queryable_processor.dart	`88.23% <88.23%> (ø)`
floor_generator/lib/processor/view_processor.dart	`95.65% <95.65%> (ø)`
floor_generator/lib/processor/dao_processor.dart	`100.00% <100.00%> (ø)`
...loor_generator/lib/processor/entity_processor.dart	`75.47% <100.00%> (-2.79%)`	⬇️
... and 20 more

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 d0727f8...465389d. Read the comment docs.

mqus · 2020-03-11T16:37:55Z

So, I rebased to the current develop and added some more tests (CodeCov seems to work even for PRs now, nice :) )

There is only one open question for me now: Streams will not work as expected for views right now. Should this also be done in this PR? If so, then there are three ways to get the tables/entities we have to watch for a specific DatabaseView:

We try to detect them in the select query for the view (Seems pretty impossible with all the possibilities to include table names and naming things similarly)
Users have to provide the Entities on which a view depends in the @DatabaseView annotation. (easy but could lead to an unsafe/leaky way of using the library, since we can't check or guarantee the relation)
SQLite/sqflite somehow provides this (I'm not aware of that)
Any other Ideas?

PS: Ah, right, I should document the DatabaseView usage in the README, or do you want to do it yourself?

Edit: I meant Streams. Fixed the comment to make more sense.

vitusortner

I've started a review a couple days ago. I'll just post the current progress now and will end the review sometime soon.

floor_annotation/lib/src/database.dart

floor_annotation/lib/src/database_view.dart

floor_generator/lib/value_object/database.dart

floor_generator/lib/processor/error/view_processor_error.dart

floor_generator/lib/processor/database_processor.dart

mqus · 2020-03-11T20:52:31Z

Thank you! I wont disrupt you then ;). Regarding Streams: Afaict Room seems to take the first option and includes a whole sqlite parser...

vitusortner

Looking pretty good! I've left some comments and I'm excited for your input.

floor/test/integration/database_test.dart

floor/test/integration/dao/name_dao.dart

floor_generator/lib/processor/dao_processor.dart

floor_generator/lib/processor/query_method_processor.dart

floor_generator/lib/value_object/dao.dart

floor_generator/test/processor/view_processor_test.dart

This adds initial support for @DatabaseView annotations known from room. One test was added, further testing is needed. Implements pinchbv#130. fix linting error

- Do not allow queries from views to be streamed - throw error if a user tries to do that and test that error - remove Stream<> queries from DAO in integration test - getStreamEntities will only return entities again - Create new Superclass for EntityProcessor and ViewProcessor named QueryableProcessor to deduplicate common functions - Code cleanup - clean up integration tests - improve code layout and comment wording in annotations, DatabaseProcessor, QueryMethodProcessor - fix toString and hashCode methods in Database (value object) - improve error message wording in DatabaseViewError

mqus · 2020-03-14T15:03:34Z

I rebased, integrated most of the suggestions and added an exception if a library user will try to use a Dao method which will stream the results of a query on a database view.

floor/test/integration/view_test/view_test.dart

floor_generator/lib/processor/queryable_processor.dart

floor_generator/lib/value_object/database.dart

vitusortner · 2020-03-14T15:27:00Z

floor_generator/lib/value_object/dao.dart

 import 'package:floor_generator/value_object/deletion_method.dart';
-import 'package:floor_generator/value_object/entity.dart';
 import 'package:floor_generator/value_object/insertion_method.dart';
 import 'package:floor_generator/value_object/query_method.dart';
 import 'package:floor_generator/value_object/transaction_method.dart';
 import 'package:floor_generator/value_object/update_method.dart';

+import 'entity.dart';


💡We could get rid of all the unnecessary import qualifiers.

import 'deletion_method.dart'; import 'entity.dart'; import 'insertion_method.dart'; import 'query_method.dart'; import 'transaction_method.dart'; import 'update_method.dart';

Then we should do it everywhere... but I think this should be part of another PR... I'll just put the entity import back where it was for now to make int consistent with the other statements...

- Clean up small details in code, according to the review - Improve tests - Simplify integration tests - split off and merge common QueryableProcessor tests into a separate file - move createClassElement function, which is now used in three test files to testutils - Add documentation to README.md

mqus · 2020-03-14T22:32:50Z

I adopted the suggestions from the review and added some documentation to the README files.

vitusortner

LGTM 👍Thanks for this contribution!

tulioccalazans · 2020-03-27T15:27:19Z

@vitusortner When this feature will be available as a release?

vitusortner · 2020-03-28T12:16:03Z

We've just created a new release with the version 0.12.0, which makes database views available and includes other features, fixes and improvements.

mqus force-pushed the feature/views branch from 0f8ca64 to 75a1820 Compare February 23, 2020 14:45

mqus force-pushed the feature/views branch from 75a1820 to 657c3ee Compare March 11, 2020 12:46

vitusortner reviewed Mar 11, 2020

View reviewed changes

vitusortner reviewed Mar 13, 2020

View reviewed changes

mqus force-pushed the feature/views branch from d6a8971 to b87c0f1 Compare March 14, 2020 14:20

mqus added 6 commits March 14, 2020 15:23

Feature: Add support for @DatabaseView annotations

328969a

This adds initial support for @DatabaseView annotations known from room. One test was added, further testing is needed. Implements pinchbv#130. fix linting error

Clean up Uint8List/Blob<->View interop

8c7e0d2

Add unit tests

49d9d59

Add integration tests for view

370505c

Adapt to upstream changes,split integrations tests

6272f02

mqus force-pushed the feature/views branch from b87c0f1 to 6272f02 Compare March 14, 2020 14:54

vitusortner reviewed Mar 14, 2020

View reviewed changes

vitusortner approved these changes Mar 15, 2020

View reviewed changes

vitusortner merged commit 56a2763 into pinchbv:develop Mar 15, 2020

akingyin1987 mentioned this pull request Mar 30, 2023

use Stream Invalid argument(s): Must not be null #751

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add support for @DatabaseView annotations #262

Feature: Add support for @DatabaseView annotations #262

mqus commented Feb 15, 2020

mqus commented Feb 16, 2020 •

edited

Loading

vitusortner commented Feb 17, 2020

mqus commented Feb 19, 2020 •

edited

Loading

vitusortner commented Feb 21, 2020

mqus commented Feb 23, 2020

codecov bot commented Mar 11, 2020 •

edited

Loading

mqus commented Mar 11, 2020 •

edited

Loading

vitusortner left a comment

mqus commented Mar 11, 2020

vitusortner left a comment

mqus commented Mar 14, 2020

vitusortner Mar 14, 2020

mqus Mar 14, 2020

mqus commented Mar 14, 2020

vitusortner left a comment

tulioccalazans commented Mar 27, 2020

vitusortner commented Mar 28, 2020

Feature: Add support for @DatabaseView annotations #262

Feature: Add support for @DatabaseView annotations #262

Conversation

mqus commented Feb 15, 2020

mqus commented Feb 16, 2020 • edited Loading

vitusortner commented Feb 17, 2020

mqus commented Feb 19, 2020 • edited Loading

vitusortner commented Feb 21, 2020

mqus commented Feb 23, 2020

codecov bot commented Mar 11, 2020 • edited Loading

Codecov Report

mqus commented Mar 11, 2020 • edited Loading

vitusortner left a comment

Choose a reason for hiding this comment

mqus commented Mar 11, 2020

vitusortner left a comment

Choose a reason for hiding this comment

mqus commented Mar 14, 2020

vitusortner Mar 14, 2020

Choose a reason for hiding this comment

mqus Mar 14, 2020

Choose a reason for hiding this comment

mqus commented Mar 14, 2020

vitusortner left a comment

Choose a reason for hiding this comment

tulioccalazans commented Mar 27, 2020

vitusortner commented Mar 28, 2020

mqus commented Feb 16, 2020 •

edited

Loading

mqus commented Feb 19, 2020 •

edited

Loading

codecov bot commented Mar 11, 2020 •

edited

Loading

mqus commented Mar 11, 2020 •

edited

Loading