Fix issues from review

- 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
pinchbv · Mar 14, 2020 · d6a8971 · d6a8971
1 parent ee2806f
commit d6a8971
Show file tree

Hide file tree

Showing 17 changed files with 185 additions and 209 deletions.
diff --git a/floor/pubspec.yaml b/floor/pubspec.yaml
@@ -13,7 +13,7 @@ dependencies:
   path: ^1.6.4
   sqflite: ^1.2.0
   floor_annotation:
-    path: ../floor_annotation
+    path: ../floor_annotation/
   meta: ^1.1.8
   flutter:
     sdk: flutter

diff --git a/floor/test/integration/dao/name_dao.dart b/floor/test/integration/dao/name_dao.dart
@@ -7,18 +7,9 @@ abstract class NameDao {
   @Query('SELECT * FROM names ORDER BY name ASC')
   Future<List<Name>> findAllNames();
 
-  @Query('SELECT * FROM names ORDER BY name ASC')
-  Stream<List<Name>> findAllNamesAsStream();
-
   @Query('SELECT * FROM names WHERE name = :name')
   Future<Name> findExactName(String name);
 
-  @Query('SELECT * FROM names WHERE name = :name')
-  Stream<Name> findExactNameAsStream(int id);
-
   @Query('SELECT * FROM names WHERE name LIKE :suffix ORDER BY name ASC')
   Future<List<Name>> findNamesLike(String suffix);
-
-  @Query('SELECT * FROM names WHERE name LIKE :suffix')
-  Stream<List<Name>> findNamesLikeAsStream(String suffix);
 }
diff --git a/floor/test/integration/database_test.dart b/floor/test/integration/database_test.dart
@@ -304,36 +304,37 @@ void main() {
         test('query view with exact value', () async {
           final person = Person(1, 'Frank');
           await personDao.insertPerson(person);
-          final expectedName = Name('Frank');
+
           final actual = await nameDao.findExactName('Frank');
 
-          expect(actual, equals(expectedName));
+        final expected = Name('Frank');
+          expect(actual, equals(expected));
         });
 
         test('query view with LIKE', () async {
-          final person = Person(1, 'Leo');
+          final persons = [Person(1, 'Leo'), Person(2, 'Frank')];
+          await personDao.insertPersons(persons);
+
           final dog = Dog(1, 'Romeo', 'Rome', 1);
-          await personDao.insertPerson(person);
-          await personDao.insertPerson(Person(2, 'Frank'));
           await dogDao.insertDog(dog);
 
-          final expectedNames = [Name('Leo'), Name('Romeo')];
           final actual = await nameDao.findNamesLike('%eo');
 
-          expect(actual, equals(expectedNames));
+          final expected = [Name('Leo'), Name('Romeo')];
+          expect(actual, equals(expected));
         });
 
         test('query view with all values', () async {
-          final person = Person(1, 'Leo');
+          final persons = [Person(1, 'Leo'), Person(2, 'Frank')];
+          await personDao.insertPersons(persons);
+
           final dog = Dog(1, 'Romeo', 'Rome', 1);
-          await personDao.insertPerson(person);
-          await personDao.insertPerson(Person(2, 'Frank'));
           await dogDao.insertDog(dog);
 
-          final expectedNames = [Name('Frank'), Name('Leo'), Name('Romeo')];
           final actual = await nameDao.findAllNames();
 
-          expect(actual, equals(expectedNames));
+          final expected = [Name('Frank'), Name('Leo'), Name('Romeo')];
+          expect(actual, equals(expected));
         });
       });
     });

diff --git a/floor_annotation/lib/src/database.dart b/floor_annotation/lib/src/database.dart
@@ -12,6 +12,9 @@ class Database {
   final List<Type> views;
 
   /// Marks a class as a FloorDatabase.
-  const Database(
-      {@required this.version, @required this.entities, this.views = const []});
+  const Database({
+    @required this.version,
+    @required this.entities,
+    this.views = const [],
+  });
 }
diff --git a/floor_annotation/lib/src/database_view.dart b/floor_annotation/lib/src/database_view.dart
@@ -6,7 +6,7 @@ class DatabaseView {
   /// The SELECT query on which the view is based on.
   final String query;
 
-  /// Marks a class as a database entity (table).
+  /// Marks a class as a database view (a fixed select statement).
   const DatabaseView(
     this.query, {
     this.viewName,

diff --git a/floor_generator/lib/processor/dao_processor.dart b/floor_generator/lib/processor/dao_processor.dart
@@ -14,7 +14,6 @@ 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/queryable.dart';
 import 'package:floor_generator/value_object/transaction_method.dart';
 import 'package:floor_generator/value_object/update_method.dart';
 
@@ -121,10 +120,10 @@ class DaoProcessor extends Processor<Dao> {
         .toList();
   }
 
-  List<Queryable> _getStreamEntities(final List<QueryMethod> queryMethods) {
+  List<Entity> _getStreamEntities(final List<QueryMethod> queryMethods) {
     return queryMethods
         .where((method) => method.returnsStream)
-        .map((method) => method.queryable)
+        .map((method) => method.queryable as Entity)
         .toList();
   }
 }
diff --git a/floor_generator/lib/processor/database_processor.dart b/floor_generator/lib/processor/database_processor.dart
@@ -34,7 +34,13 @@ class DatabaseProcessor extends Processor<Database> {
     final version = _getDatabaseVersion();
 
     return Database(
-        _classElement, databaseName, entities, views, daoGetters, version);
+      _classElement,
+      databaseName,
+      entities,
+      views,
+      daoGetters,
+      version,
+    );
   }
 
   @nonNull
@@ -105,7 +111,7 @@ class DatabaseProcessor extends Processor<Database> {
 
   @nonNull
   List<View> _getViews(final ClassElement databaseClassElement) {
-    final views = _classElement
+    return _classElement
         .getAnnotation(annotations.Database)
         .getField(AnnotationField.DATABASE_VIEWS)
         ?.toListValue()
@@ -114,8 +120,6 @@ class DatabaseProcessor extends Processor<Database> {
         ?.where(_isView)
         ?.map((classElement) => ViewProcessor(classElement).process())
         ?.toList();
-
-    return views;
   }
 
   @nonNull

diff --git a/floor_generator/lib/processor/entity_processor.dart b/floor_generator/lib/processor/entity_processor.dart
@@ -1,66 +1,54 @@
 import 'package:analyzer/dart/constant/value.dart';
 import 'package:analyzer/dart/element/element.dart';
-import 'package:analyzer/dart/element/type.dart';
 import 'package:floor_annotation/floor_annotation.dart' as annotations;
 import 'package:floor_generator/misc/annotations.dart';
 import 'package:floor_generator/misc/constants.dart';
 import 'package:floor_generator/misc/foreign_key_action.dart';
 import 'package:floor_generator/misc/type_utils.dart';
 import 'package:floor_generator/processor/error/entity_processor_error.dart';
-import 'package:floor_generator/processor/field_processor.dart';
-import 'package:floor_generator/processor/processor.dart';
+import 'package:floor_generator/processor/queryable_processor.dart';
 import 'package:floor_generator/value_object/entity.dart';
 import 'package:floor_generator/value_object/field.dart';
 import 'package:floor_generator/value_object/foreign_key.dart';
 import 'package:floor_generator/value_object/index.dart';
 import 'package:floor_generator/value_object/primary_key.dart';
 
-class EntityProcessor extends Processor<Entity> {
-  final ClassElement _classElement;
+class EntityProcessor extends QueryableProcessor<Entity> {
   final EntityProcessorError _processorError;
 
   EntityProcessor(final ClassElement classElement)
-      : assert(classElement != null),
-        _classElement = classElement,
-        _processorError = EntityProcessorError(classElement);
+      : _processorError = EntityProcessorError(classElement),
+        super(classElement);
 
   @nonNull
   @override
   Entity process() {
     final name = _getName();
-    final fields = _getFields();
+    final fields = getFields();
 
     return Entity(
-      _classElement,
+      classElement,
       name,
       fields,
       _getPrimaryKey(fields),
       _getForeignKeys(),
       _getIndices(fields, name),
-      _getConstructor(fields),
+      getConstructor(fields),
     );
   }
 
   @nonNull
   String _getName() {
-    return _classElement
+    return classElement
             .getAnnotation(annotations.Entity)
             .getField(AnnotationField.ENTITY_TABLE_NAME)
             .toStringValue() ??
-        _classElement.displayName;
-  }
-
-  @nonNull
-  List<Field> _getFields() {
-    return _classElement.fields
-        .where((fieldElement) => fieldElement.shouldBeIncluded())
-        .map((field) => FieldProcessor(field).process())
-        .toList();
+        classElement.displayName;
   }
 
   @nonNull
   List<ForeignKey> _getForeignKeys() {
-    return _classElement
+    return classElement
             .getAnnotation(annotations.Entity)
             .getField(AnnotationField.ENTITY_FOREIGN_KEYS)
             ?.toListValue()
@@ -114,7 +102,7 @@ class EntityProcessor extends Processor<Entity> {
 
   @nonNull
   List<Index> _getIndices(final List<Field> fields, final String tableName) {
-    return _classElement
+    return classElement
             .getAnnotation(annotations.Entity)
             .getField(AnnotationField.ENTITY_INDICES)
             ?.toListValue()
@@ -182,7 +170,7 @@ class EntityProcessor extends Processor<Entity> {
 
   @nullable
   PrimaryKey _getCompoundPrimaryKey(final List<Field> fields) {
-    final compoundPrimaryKeyColumnNames = _classElement
+    final compoundPrimaryKeyColumnNames = classElement
         .getAnnotation(annotations.Entity)
         .getField(AnnotationField.ENTITY_PRIMARY_KEYS)
         ?.toListValue()
@@ -219,65 +207,4 @@ class EntityProcessor extends Processor<Entity> {
 
     return PrimaryKey([primaryKeyField], autoGenerate);
   }
-
-  @nonNull
-  String _getConstructor(final List<Field> fields) {
-    final constructorParameters = _classElement.constructors.first.parameters;
-    final parameterValues = constructorParameters
-        .map((parameterElement) => _getParameterValue(parameterElement, fields))
-        .where((parameterValue) => parameterValue != null)
-        .join(', ');
-
-    return '${_classElement.displayName}($parameterValues)';
-  }
-
-  /// Returns `null` whenever field is @ignored
-  @nullable
-  String _getParameterValue(
-    final ParameterElement parameterElement,
-    final List<Field> fields,
-  ) {
-    final parameterName = parameterElement.displayName;
-    final field = fields.firstWhere(
-      (field) => field.name == parameterName,
-      orElse: () => null, // whenever field is @ignored
-    );
-    if (field != null) {
-      final parameterValue = "row['${field.columnName}']";
-      final castedParameterValue =
-          _castParameterValue(parameterElement.type, parameterValue);
-      if (parameterElement.isNamed) {
-        return '$parameterName: $castedParameterValue';
-      }
-      return castedParameterValue; // also covers positional parameter
-    } else {
-      return null;
-    }
-  }
-
-  @nonNull
-  String _castParameterValue(
-    final DartType parameterType,
-    final String parameterValue,
-  ) {
-    if (parameterType.isDartCoreBool) {
-      return '($parameterValue as int) != 0'; // maps int to bool
-    } else if (parameterType.isDartCoreString) {
-      return '$parameterValue as String';
-    } else if (parameterType.isDartCoreInt) {
-      return '$parameterValue as int';
-    } else if (parameterType.isUint8List) {
-      return '$parameterValue as Uint8List';
-    } else {
-      return '$parameterValue as double'; // must be double
-    }
-  }
-}
-
-extension on FieldElement {
-  bool shouldBeIncluded() {
-    final isIgnored = hasAnnotation(annotations.ignore.runtimeType);
-    final isHashCode = displayName == 'hashCode';
-    return !(isStatic || isHashCode || isIgnored);
-  }
 }
diff --git a/floor_generator/lib/processor/error/query_method_processor_error.dart b/floor_generator/lib/processor/error/query_method_processor_error.dart
@@ -35,4 +35,13 @@ class QueryMethodProcessorError {
       element: _methodElement,
     );
   }
+
+  // ignore: non_constant_identifier_names
+  InvalidGenerationSourceError get VIEW_NOT_STREAMABLE {
+    return InvalidGenerationSourceError(
+      'Queries on a view can not be returned as a Stream yet.',
+      todo: 'Don\'t use Stream as the return type of a Query on a View.',
+      element: _methodElement,
+    );
+  }
 }
diff --git a/floor_generator/lib/processor/error/view_processor_error.dart b/floor_generator/lib/processor/error/view_processor_error.dart
@@ -11,9 +11,9 @@ class ViewProcessorError {
   // ignore: non_constant_identifier_names
   InvalidGenerationSourceError get MISSING_QUERY {
     return InvalidGenerationSourceError(
-      'There is no SELECT Query defined on the entity ${_classElement.displayName}.',
+      'There is no SELECT query defined on the database view ${_classElement.displayName}.',
       todo:
-          'Define a SELECT query for this View with @DatabaseView(\'SELECT [...]\') ',
+          'Define a SELECT query for this database view with @DatabaseView(\'SELECT [...]\') ',
       element: _classElement,
     );
   }

diff --git a/floor_generator/lib/processor/query_method_processor.dart b/floor_generator/lib/processor/query_method_processor.dart
@@ -48,7 +48,7 @@ class QueryMethodProcessor extends Processor<QueryMethod> {
       returnsStream,
     );
 
-    final Queryable queryable = _entities.firstWhere(
+    final queryable = _entities.firstWhere(
             (entity) =>
                 entity.classElement.displayName ==
                 flattenedReturnType.getDisplayString(),
@@ -57,7 +57,8 @@ class QueryMethodProcessor extends Processor<QueryMethod> {
             (view) =>
                 view.classElement.displayName ==
                 flattenedReturnType.getDisplayString(),
-            orElse: () => null);
+            orElse: () => null); // doesn't return entity nor view
+    _assertViewQueryDoesNotReturnStream(queryable, returnsStream);
 
     return QueryMethod(
       _methodElement,
@@ -137,6 +138,15 @@ class QueryMethodProcessor extends Processor<QueryMethod> {
     }
   }
 
+  void _assertViewQueryDoesNotReturnStream(
+    final Queryable queryable,
+    final bool returnsStream,
+  ) {
+    if (queryable != null && queryable is View && returnsStream) {
+      throw _processorError.VIEW_NOT_STREAMABLE;
+    }
+  }
+
   void _assertQueryParameters(
     final String query,
     final List<ParameterElement> parameterElements,