Integrate sqlparser #361

mqus · 2020-06-15T22:19:36Z

I have now finished an early working version of the integrated sqlparser. There are still some //todos open, most of them related to the typeConverters, because I assume that this PR will not be merged before #318 (though mostly because the query parser is not quite finished).

Features :

An even more intelligent generator overall and the foundation for more features
Query validation for @DatabaseView and @Query. (Provide more query validation #58)
Type derivation and matching for @DatabaseView and @Query.
Dependency and "What entities are affected" derivation, including foreign keys of entities
Better parameter handling
- automatic reordering
- multiple insertions of the same parameter (Query can't use method parameter more than once #142)
- better handling of list parameters (fixing sql-injections)
- removed string-replace functions which could mess up content in string literals
Outputting simple values in Queries (Impossible to return int value from COUNT(*) #200)
Cheaper streams on DatabaseViews

TODO:

Implement some Features/Refactor

Mapper for primitive types
Return type checker
Restructure AnalyzeResult creation
Maybe separate type checker from view converter
Check view query for variables
Map bool type to int with typehint in converters
Make listInsertionPositions a (sorted) list with dedicated type
Formulate missing errors
Check for and add missing errors

Cleanup

Check todo comments for missing functionality and expand this todo list
Add @nonNull/@nullable annotations to new functions and check usage
Add Docstrings to crucial functions

Integration of other PRs

integrating typeconverters
Maybe integrate Change ForeignKeyAction to enum in the generator #359, if it is merged.

Fixing existing tests

integration tests (pretty much already worked without any changes)
queryadapter unit tests
generator unit tests

Adding new tests

integration tests
queryadapter unit tests
generator unit tests
- Add regression test from Fix String escaping and add test #353

Update README

Document subtleties, such as executing a query only up to the first semicolon and not allowing more than 1000 parameter values (both limitations of the sqlite implementations of e.g. android)
Update Documentation regarding streams on Views

Small Guide to the PR

I used the library sqlparser from https://github.com/simolus3/moor, like mentioned before.
I added a new subdirectory to processor called query_analyzer. There are multiple files in there:

engine.dart/AnalyzerEngine is the functional wrapper for the sqlparser library, which is initialized once per database and holds the context for the analyzer. It stores dependencies, maps database names to Queryables and is able to analyze queries by wrapping sqlparsers engine. All entities and database views have to be registered here, with database views having to come after entities for a successful validation.
dependency_graph.dart is a small utility data structure for the engine to hold the dependency graph. It currently calculates the indirect dependencies without caching any results to keep it simple. There are no plans to include caching yet as generator performance is not a priority right now.
type_checker.dart contains functions to compare the types of view fields/output fields of a query output to the derived youtput from the associated query and reports errors.
visitors.dart Is a visitor which walks the sqlparser tree and stores all found variable references, such as ?, ?44, :varname, and so on. It is used by the QueryProcessor to first validate the parameters and then find and replace all named variables starting with a colon in the sql string with either a numbered variable (for normal parameters of the query) or a placeholder (for list parameters).

I also added query.dart and query_processor.dart, which are responsible for holding and processing the query, including the derivation of list parameter mappings, output types, dependencies and affected entities, as well as the validation of the query.

Finally, I also added a QueryMethodReturnType to the value_object folder to simplify the access to the return type description of query methods and restructured the query_method_writer.dart.

If there are any questions or suggestions, I'm happy to listen, while I work on the tasks written above.

Closes #321

codecov · 2020-06-15T22:23:00Z

Codecov Report

Merging #361 into develop will increase coverage by 3.13%.
The diff coverage is 90.00%.

@@             Coverage Diff             @@
##           develop     #361      +/-   ##
===========================================
+ Coverage    81.55%   84.68%   +3.13%     
===========================================
  Files           60       10      -50     
  Lines         1534      209    -1325     
===========================================
- Hits          1251      177    -1074     
+ Misses         283       32     -251

Flag	Coverage Δ
#floor	`84.68% <90.00%> (+3.13%)`	⬆️
#floor_generator	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
floor/lib/src/adapter/query_adapter.dart	`95.23% <90.00%> (+2.55%)`	⬆️
floor_generator/lib/value_object/index.dart
...or_generator/lib/value_object/deletion_method.dart
...loor_generator/lib/value_object/change_method.dart
...enerator/lib/misc/change_method_writer_helper.dart
.../lib/processor/error/database_processor_error.dart
floor_generator/lib/misc/foreign_key_action.dart
..._generator/lib/writer/database_builder_writer.dart
...enerator/lib/writer/transaction_method_writer.dart
...lib/processor/error/queryable_processor_error.dart
... and 16 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 7888578...6ac8111. Read the comment docs.

vitusortner · 2020-06-16T19:45:52Z

This looks very promising 👏 I'll make time for reviewing the changes over the next days. If there is anything in particular that I can help you with, just let me know!

mqus · 2020-06-19T13:14:59Z

In the meantime, let me give you a small taste of what we're getting with @simolus3's sqlparser. I currently fix the existing tests on the query_method writer and processor and many of the tests have some arbitrary queries in them, that don't reference any existing entities or wrong names, which weren't necessary to be right until now. This is the kind of error messages I get:

The query contained errors: Could not find Persons. Available are: sqlite_master, sqlite_sequence, Person, Name
package:_resolve_source/_resolve_source.dart:8:28
  ╷
8 │       Future<List<Person>> findPersonsWithNamesLike(String name);
  │                            ^^^^^^^^^^^^^^^^^^^^^^^^
  ╵

I think they're awesome. I'll look into also printing the line before that which actually contains the query.
EDIT: I actually didn't completely bring out the potential here, as most of sqlparsers errors come with a highlighted span which also shows you where you made the mistake. This looks like darts highlighting above, just with sql :)

mqus · 2020-06-19T16:47:23Z

If there is anything in particular that I can help you with, just let me know!

A list of testcases you would like to see covered, I have my own list but more is better :)

vitusortner · 2020-06-19T18:37:55Z

In the meantime, let me give you a small taste of what we're getting with @simolus3's sqlparser. I currently fix the existing tests on the query_method writer and processor and many of the tests have some arbitrary queries in them, that don't reference any existing entities or wrong names, which weren't necessary to be right until now. This is the kind of error messages I get:
The query contained errors: Could not find Persons. Available are: sqlite_master, sqlite_sequence, Person, Name
package:_resolve_source/_resolve_source.dart:8:28
  ╷
8 │       Future<List<Person>> findPersonsWithNamesLike(String name);
  │                            ^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
I think they're awesome. I'll look into also printing the line before that which actually contains the query.

That's amazing! The capabilities your changes provide to the project are more than great. Good job 👏

…ation test

…gfixes

…ve return types in dao methods

vitusortner · 2020-07-30T18:17:34Z

I'll look into it ASAP!

vitusortner

I'll deliver the review in parts. Here's the first part 🙌

vitusortner · 2020-08-01T06:43:37Z

README.md

+Your SQL queries will be validated completely while generating the code.
+These queries have to return either a `Future` or a `Stream` of an entity, a view, a primitive value or `void`.


❔ The query parser, in the future, will allows us to parse @Query results into non-primitive values, right? #94

This will need another PR but it should be as simple as adding another Queryable subtype and using the same processing as for entities/views. The type checking and the mapping already exists.

vitusortner · 2020-08-01T07:00:05Z

floor/test/adapter/query_adapter_test.dart

+      streamController.add(otherEntity);
+
+      await Future<void>.delayed(const Duration(milliseconds: 100));
+      await streamController.close();


💡 We can remove this line as we close StreamControllers in the tearDown block after each test.

Suggested change

await streamController.close();

If I remove that, the test will fail because of a deadlock/timeout, since the emitsInOrder still waits for the Stream to complete, but the Stream only gets closed after the test succeeded.

floor/test/integration/boolean_conversions/bool_test.dart

vitusortner · 2020-08-01T07:05:01Z

floor/test/integration/sqlparser_test/sqlparser_test.dart

+    await Future<void>.delayed(const Duration(milliseconds: 100));
+    await caseDao.updateName();
+
+    await Future<void>.delayed(const Duration(milliseconds: 100));
+    await caseDao.insertPerson(person1);
+
+    await Future<void>.delayed(const Duration(milliseconds: 100));
+    await caseDao.updateName();
+
+    await Future<void>.delayed(const Duration(milliseconds: 100));
+    await caseDao.insertPerson(person2);
+
+    await Future<void>.delayed(const Duration(milliseconds: 100));


❔ I guess without these delays, the tests don't run?

Yeah, they will not complete successfully and will have issues like the following:

Expected: should do the following in order: • emit an event that [] • emit an event that [] • emit an event that [Person:Person{id: 1, name: Baba}] • emit an event that [Person:Person{id: 1, name: Baba}] • emit an event that [Person:Person{id: 1, name: Baba}, Person:Person{id: 2, name: Me}] • emit an event that [Person:Person{id: 1, name: Baba}, Person:Person{id: 2, name: Me}] Actual: <Instance of '_BroadcastStream<List<Person>>'> Which: emitted • [] • [Person{id: 1, name: Baba}] which didn't emit an event that [] because it emitted an event that longer than expected at location [0]

Because the first two (or three) statements were completed together before the stream got to re-query the database.

vitusortner · 2020-08-01T07:06:54Z

floor/test/integration/sqlparser_test/sqlparser_test.dart

+      final list = ['a', 'b', 'c', 'd', 'e', 'f', null];
+
+      final aInList = await deepDao.isXinList('a', list);
+      expect(aInList, equals(true));


❔ The SQLite database returns "a" in this case? Which use cases do you see for mapping a result (of not INTEGER) to true when the return type is bool ?

In my case it returns 1 and not "a". This is also what I want to test: a simple function that does not refer to any table and returns a primitive boolean (or null) value.

sqlite> SELECT "a" IN ("a", "2", "s"); 1

isXinList is just a complicated way to write ['a', 'b', 'c', 'd', 'e', 'f', null].contains('a').

vitusortner · 2020-08-01T07:11:29Z

floor/test/integration/stream_query_test.dart

            [], // initial state,
+            [], // after inserting person1,
+            [], // after inserting person2,
            [dog1], // after inserting dog1
            [dog1], // after inserting dog2
-            //[], // after removing person1. Does not work because
-            // ForeignKey-relations are not considered yet (#321)
+            [], // after removing person1.


❤️ Good job! This is great!

Here I found out that this is still suboptimal, since it will be also triggered if a person is inserted, even if that will never change dog. 😆

This is what I meant in #373 .

vitusortner · 2020-08-01T07:12:43Z

floor_generator/lib/misc/string_utils.dart

  String decapitalize() {
    return '${this[0].toLowerCase()}${substring(1)}';
  }
+
+  /// Makes the first letter of the supplied string [value] lowercase.


Suggested change

/// Makes the first letter of the supplied string [value] lowercase.

/// Makes the first letter of the supplied string [value] uppercase.

floor_generator/lib/misc/type_utils.dart

vitusortner · 2020-08-01T13:41:51Z

floor_generator/lib/misc/type_utils.dart

+const sqlToBasicType = {
+  SqlType.blob: BasicType.blob,
+  SqlType.integer: BasicType.int,
+  SqlType.real: BasicType.real,
+  SqlType.text: BasicType.text,
+};


💡 Judging from the usages of this map, we could transform it into an extension function on SqlType (String).

Suggested change

const sqlToBasicType = {

SqlType.blob: BasicType.blob,

SqlType.integer: BasicType.int,

SqlType.real: BasicType.real,

SqlType.text: BasicType.text,

};

extension SqlTypeExtension on String {

BasicType toBasicType() {

switch (this) {

case SqlType.blob:

return BasicType.blob;

...

}

}

}```

vitusortner · 2020-08-01T13:42:40Z

floor_generator/lib/processor/query_analyzer/engine.dart

@@ -0,0 +1,76 @@
+import 'package:floor_generator/misc/annotations.dart';


💡 We could rename this file to match the class it's holding. engine.dart -> analyzer_engine.dart

vitusortner · 2020-08-01T13:43:06Z

floor_generator/lib/processor/query_analyzer/engine.dart

+import 'package:floor_generator/value_object/view.dart' as floor;
+import 'package:sqlparser/sqlparser.dart' hide Queryable;
+
+const String varlistPlaceholder = ':varlist';


💡 There is no need to explicitly define the type here as it can be inferred.

Suggested change

const String varlistPlaceholder = ':varlist';

cons varlistPlaceholder = ':varlist';

floor_generator/lib/processor/query_analyzer/engine.dart

vitusortner · 2020-08-01T13:55:19Z

floor_generator/lib/processor/query_analyzer/engine.dart

+    inner.registerTable(_convertEntityToTable(entity));
+
+    registry[entity.name] = entity;
+
+    //register dependencies
+    final directDependencies = entity.foreignKeys
+        .where((e) => e.canChangeChild)
+        .map((e) => e.parentName);
+    dependencies.add(entity.name, directDependencies);
+  }
+
+  void registerView(floor.View floorView, View convertedView) {


❔ Entities are converted in the analyzer engine but views are converted in the view processor. Do you see a way to either have both in the analyzer engine or both in the appropriate processor?

I thought about this and while entities can be converted without any fuzz (they don't have to be analyzed and can't throw any errors), Views have their query checked, have to be typechecked, can throw errors (which need the source reference). I had both solutions in the code at some time:

Add all entities and views to the engine at the DatabaseProcessor-level, after processing them.
This has the "downside" that the conversion of the views is just treated like a conversion for the engine, while it really is the second part of the processing, throwing errors and determining dependencies.

Add views and entities to the engine, as the last processing step inside their process() methods.
This solved the view issues in 1., but introduced the engine to the entity processor, which didn't really need it. It also made the tests for entities a lot more complex for gaining nothing in return.

So I chose to really handle them differently. This has it's own issues, like also having to treat them differently in tests, but I think that the tradeoffs are ok. Of course, this is just my opinion and I'm also not that sure on what the right way is. If you have a clear preference I'll gladly change this.

floor_generator/pubspec.yaml

…de building issues

mqus · 2020-08-16T00:35:24Z

pub run test_cov is failing, probably due to flutter/flutter#57864

mqus · 2021-02-11T00:11:30Z

I think that there are enough conflicts by now that this has to be rewritten from scratch.

I'm currently not up for it but if I have some advice for people who will try this in the future: take smaller steps. I rewrote some logic and also added a parser which already did 90% of the things it can do, all in one PR. This is too much to be reviewed reasonably, which is another reason why I think this PR has no hope of getting merged as-is, even when fixing all conflicts and updating it.

For those reasons, I will close this PR. Maybe I will have some time to go back and try again but currently I don't. I will gladly to give advice and reviews for future attempts of contributors, just ping me ;)

mqus added 10 commits June 12, 2020 14:51

Fix String escaping and add test

2a2fd87

Switch to raw strings

4f1d20d

Add tests for different quotes

a174323

fix analyzer error

620cd9a

Start to integrate sqlparser

9416a18

temporary state

7ec04ce

complete initial queryparsing

34ceedb

Start to integrate the analyzer into the qmprocessor

fc5a2c2

Add some todo comments

3d859a9

Finish alpha-grade version of the sqlparser integration

94aeac2

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

This was linked to issues Jun 15, 2020

Provide more query validation #58

Open

Query can't use method parameter more than once #142

Closed

Impossible to return int value from COUNT(*) #200

Closed

mqus mentioned this pull request Jun 15, 2020

Add a query parser #321

Open

9 tasks

mqus changed the title ~~Add a queryparser~~ Integrate sqlparser Jun 15, 2020

cleanup a bit, regenerate example code, add more todos

73b7192

mqus mentioned this pull request Jun 16, 2020

Fix String escaping and add test #353

Closed

mqus linked an issue Jun 16, 2020 that may be closed by this pull request

Can't compare fields against blank #352

Closed

Add quoting tests, use list for varlist positions, quickfix tests

0efaecd

mqus mentioned this pull request Jun 17, 2020

ORDER BY won't work with query parameters #362

Closed

mqus added 3 commits June 19, 2020 15:25

Update sqlparser, fix tests (for now)

bbb846a

fmt

513eabf

fix linter issues

956537a

refactor query analysis, use toLiteral for literal strings

09d50fe

mqus linked an issue Jul 27, 2020 that may be closed by this pull request

[0.6.0] Arguments mapped incorrectly in raw query #224

Closed

mqus added 4 commits July 29, 2020 22:02

Add some integration tests and update generated code in example

7b7b732

Extend QueryAdapter docs on noReturnQuery and add test idea to integr…

98df943

…ation test

Clean up files, add @nonnull, add docstrings. Update sqlparser for bu…

c802294

…gfixes

Add integration test for bool serialization and unit test for primiti…

7da688e

…ve return types in dao methods

mqus added 4 commits July 31, 2020 00:46

Add more unit tests and integration tests

6634f3b

Merge remote-tracking branch 'upstream/develop' into integrate-sqlparser

8279bbe

Merge remote-tracking branch 'upstream/develop' into integrate-sqlparser

2c3d326

integrate WITHOUT ROWID

6d8cc21

vitusortner reviewed Aug 1, 2020

View reviewed changes

simolus3 reviewed Aug 7, 2020

View reviewed changes

floor_generator/pubspec.yaml Outdated Show resolved Hide resolved

mqus mentioned this pull request Aug 12, 2020

Query can't use method parameter more than once #142

Closed

mqus added 3 commits August 13, 2020 00:48

Apply quick fixes to small review issues

9925320

Refactor dependencies to only propagate sql names of entities, fix co…

1d2a210

…de building issues

Add comment to pinned analyzer version and add more tests to daowriter

6ac8111

vitusortner mentioned this pull request Aug 22, 2020

How do I select scalar values? #392

Closed

mqus mentioned this pull request Aug 24, 2020

How can I use a data type parameter in the query function on the floor? #393

Closed

mqus mentioned this pull request Sep 16, 2020

Auto-generated code issue of parameters' sequence #399

Closed

vitusortner mentioned this pull request Nov 6, 2020

Query return type #426

Closed

vitusortner mentioned this pull request Nov 14, 2020

Reuse query params at the same query #428

Closed

vitusortner mentioned this pull request Nov 29, 2020

Error running FloorGenerator #174

Closed

vitusortner mentioned this pull request Jan 10, 2021

Order of parameters #480

Closed

mqus closed this Feb 11, 2021

mqus mentioned this pull request Mar 14, 2021

Improved Parameter mapping for query methods #509

Merged

MrCsabaToth mentioned this pull request Dec 24, 2021

Impossible to return int value from COUNT(*) #200

Closed

MrCsabaToth mentioned this pull request Feb 20, 2022

count statement should return number but it got NULL #625

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate sqlparser #361

Integrate sqlparser #361

mqus commented Jun 15, 2020 •

edited

Loading

codecov bot commented Jun 15, 2020 •

edited

Loading

vitusortner commented Jun 16, 2020

mqus commented Jun 19, 2020 •

edited

Loading

mqus commented Jun 19, 2020

vitusortner commented Jun 19, 2020

vitusortner commented Jul 30, 2020

vitusortner left a comment

vitusortner Aug 1, 2020

mqus Aug 12, 2020

vitusortner Aug 1, 2020

mqus Aug 12, 2020

vitusortner Aug 1, 2020

mqus Aug 12, 2020

vitusortner Aug 1, 2020

mqus Aug 12, 2020

vitusortner Aug 1, 2020

mqus Aug 12, 2020

vitusortner Aug 1, 2020

mqus Aug 12, 2020

vitusortner Aug 1, 2020

vitusortner Aug 1, 2020

vitusortner Aug 1, 2020

vitusortner Aug 1, 2020

mqus Aug 12, 2020

mqus commented Aug 16, 2020

mqus commented Feb 11, 2021

		Your SQL queries will be validated completely while generating the code.
		These queries have to return either a `Future` or a `Stream` of an entity, a view, a primitive value or `void`.

	/// Makes the first letter of the supplied string [value] lowercase.
	/// Makes the first letter of the supplied string [value] uppercase.

		@@ -0,0 +1,76 @@
		import 'package:floor_generator/misc/annotations.dart';

	const String varlistPlaceholder = ':varlist';
	cons varlistPlaceholder = ':varlist';

Integrate sqlparser #361

Integrate sqlparser #361

Conversation

mqus commented Jun 15, 2020 • edited Loading

Features :

TODO:

Small Guide to the PR

codecov bot commented Jun 15, 2020 • edited Loading

Codecov Report

vitusortner commented Jun 16, 2020

mqus commented Jun 19, 2020 • edited Loading

mqus commented Jun 19, 2020

vitusortner commented Jun 19, 2020

vitusortner commented Jul 30, 2020

vitusortner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mqus commented Aug 16, 2020

mqus commented Feb 11, 2021

mqus commented Jun 15, 2020 •

edited

Loading

codecov bot commented Jun 15, 2020 •

edited

Loading

mqus commented Jun 19, 2020 •

edited

Loading