Resolve issues from review in pinchbv#359 and add new tests for wrong…

… onConflict values
mqus · Jun 20, 2020 · 5ddc9aa · 5ddc9aa
1 parent 46e7e28
commit 5ddc9aa
Show file tree

Hide file tree

Showing 12 changed files with 78 additions and 46 deletions.
diff --git a/floor_generator/lib/misc/extension/dart_object_extension.dart b/floor_generator/lib/misc/extension/dart_object_extension.dart
@@ -1,26 +1,26 @@
 import 'package:analyzer/dart/constant/value.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:floor_annotation/floor_annotation.dart';
-
-import '../annotations.dart';
+import 'package:meta/meta.dart';
 
 extension DartObjectExtension on DartObject {
-  String toEnumValueString() {
+  /// get the String representation of the enum value, or the result of
+  /// [orElse] if the enum was not valid. [orElse]
+  String toEnumValueString({@required String orElse()}) {
     final interfaceType = type as InterfaceType;
     final enumValue = interfaceType.element.fields
         .where((element) => element.isEnumConstant)
         .map((fieldElement) => fieldElement.name)
         .singleWhere((valueName) => getField(valueName) != null,
-            orElse: () => null);
+            orElse: orElse);
 
     return '$interfaceType.$enumValue';
   }
 
-  @nullable
-  ForeignKeyAction toForeignKeyAction() {
-    final enumValueString = toEnumValueString();
+  ForeignKeyAction toForeignKeyAction({@required ForeignKeyAction orElse()}) {
+    final enumValueString = toEnumValueString(orElse: () => null);
     return ForeignKeyAction.values.singleWhere(
         (foreignKeyAction) => foreignKeyAction.toString() == enumValueString,
-        orElse: () => null);
+        orElse: orElse);
   }
 }
diff --git a/floor_generator/lib/misc/extension/foreign_key_action.dart b/floor_generator/lib/misc/extension/foreign_key_action.dart
@@ -3,7 +3,7 @@ import 'package:floor_generator/misc/annotations.dart';
 
 extension ToSQL on ForeignKeyAction {
   @nonNull
-  String get toSQL {
+  String toSql() {
     switch (this) {
       case ForeignKeyAction.noAction:
         return 'NO ACTION';
@@ -15,8 +15,9 @@ extension ToSQL on ForeignKeyAction {
         return 'SET DEFAULT';
       case ForeignKeyAction.cascade:
         return 'CASCADE';
-      default:
-        return null;
+      default: // can only match null
+        throw ArgumentError('toSql() should not be called on a null value. '
+            'This is a bug in floor.');
     }
   }
 }
diff --git a/floor_generator/lib/processor/entity_processor.dart b/floor_generator/lib/processor/entity_processor.dart
@@ -12,7 +12,6 @@ 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';
-import 'package:meta/meta.dart';
 
 class EntityProcessor extends QueryableProcessor<Entity> {
   final EntityProcessorError _processorError;
@@ -81,10 +80,10 @@ class EntityProcessor extends QueryableProcessor<Entity> {
           }
 
           final onUpdate =
-              _getForeignKeyAction(foreignKeyObject, isUpdate: true);
+              _getForeignKeyAction(foreignKeyObject, ForeignKeyField.onUpdate);
 
           final onDelete =
-              _getForeignKeyAction(foreignKeyObject, isUpdate: false);
+              _getForeignKeyAction(foreignKeyObject, ForeignKeyField.onDelete);
 
           return ForeignKey(
             parentName,
@@ -206,22 +205,16 @@ class EntityProcessor extends QueryableProcessor<Entity> {
   }
 
   @nonNull
-  annotations.ForeignKeyAction _getForeignKeyAction(DartObject foreignKeyObject,
-      {@required bool isUpdate}) {
-    final fieldName =
-        isUpdate ? ForeignKeyField.onUpdate : ForeignKeyField.onDelete;
-
-    final field = foreignKeyObject.getField(fieldName);
+  annotations.ForeignKeyAction _getForeignKeyAction(
+      DartObject foreignKeyObject, String triggerName) {
+    final field = foreignKeyObject.getField(triggerName);
     if (field == null) {
       // field was not defined, return default value
       return annotations.ForeignKeyAction.noAction;
     }
 
-    final fka = field.toForeignKeyAction();
-    if (fka == null) {
-      // field was not defined correctly
-      throw _processorError.wrongForeignKeyAction(field, isUpdate);
-    }
-    return fka;
+    return field.toForeignKeyAction(
+        orElse: () =>
+            throw _processorError.wrongForeignKeyAction(field, triggerName));
   }
 }
diff --git a/floor_generator/lib/processor/error/entity_processor_error.dart b/floor_generator/lib/processor/error/entity_processor_error.dart
@@ -72,9 +72,9 @@ class EntityProcessorError {
   }
 
   InvalidGenerationSourceError wrongForeignKeyAction(
-      DartObject field, bool isUpdate) {
+      DartObject field, String triggerName) {
     return InvalidGenerationSourceError(
-      'No ForeignKeyAction with the value $field exists for the on${isUpdate ? 'Update' : 'Delete'} trigger.',
+      'No ForeignKeyAction with the value $field exists for the $triggerName trigger.',
       todo:
           'Make sure to add a correct ForeignKeyAction like `ForeignKeyAction.noAction` or leave it out entirely.',
       element: _classElement,

diff --git a/floor_generator/lib/processor/insertion_method_processor.dart b/floor_generator/lib/processor/insertion_method_processor.dart
@@ -1,7 +1,7 @@
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:floor_annotation/floor_annotation.dart' as annotations
-    show Insert;
+    show Insert, OnConflictStrategy;
 import 'package:floor_generator/misc/annotations.dart';
 import 'package:floor_generator/misc/change_method_processor_helper.dart';
 import 'package:floor_generator/misc/constants.dart';
@@ -87,7 +87,11 @@ class InsertionMethodProcessor implements Processor<InsertionMethod> {
     return _methodElement
         .getAnnotation(annotations.Insert)
         .getField(AnnotationField.onConflict)
-        .toEnumValueString();
+        .toEnumValueString(
+            orElse: () => throw InvalidGenerationSourceError(
+                  'Value of ${AnnotationField.onConflict} must be one of ${annotations.OnConflictStrategy.values.map((e) => e.toString()).join(',')}',
+                  element: _methodElement,
+                ));
   }
 
   void _assertMethodReturnsFuture(final DartType returnType) {

diff --git a/floor_generator/lib/processor/update_method_processor.dart b/floor_generator/lib/processor/update_method_processor.dart
@@ -1,7 +1,7 @@
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:floor_annotation/floor_annotation.dart' as annotations
-    show Update;
+    show Update, OnConflictStrategy;
 import 'package:floor_generator/misc/annotations.dart';
 import 'package:floor_generator/misc/change_method_processor_helper.dart';
 import 'package:floor_generator/misc/constants.dart';
@@ -69,7 +69,11 @@ class UpdateMethodProcessor implements Processor<UpdateMethod> {
     return _methodElement
         .getAnnotation(annotations.Update)
         .getField(AnnotationField.onConflict)
-        .toEnumValueString();
+        .toEnumValueString(
+            orElse: () => throw InvalidGenerationSourceError(
+                  'Value of ${AnnotationField.onConflict} must be one of ${annotations.OnConflictStrategy.values.map((e) => e.toString()).join(',')}',
+                  element: _methodElement,
+                ));
   }
 
   @nonNull

diff --git a/floor_generator/lib/value_object/foreign_key.dart b/floor_generator/lib/value_object/foreign_key.dart
@@ -29,8 +29,8 @@ class ForeignKey {
 
     return 'FOREIGN KEY ($escapedChildColumns)'
         ' REFERENCES `$parentName` ($escapedParentColumns)'
-        ' ON UPDATE ${onUpdate.toSQL}'
-        ' ON DELETE ${onDelete.toSQL}';
+        ' ON UPDATE ${onUpdate.toSql()}'
+        ' ON DELETE ${onDelete.toSql()}';
   }
 
   final _listEquality = const ListEquality<String>();

diff --git a/floor_generator/test/misc/foreign_key_action_test.dart b/floor_generator/test/misc/foreign_key_action_test.dart
@@ -5,33 +5,32 @@ import 'package:test/test.dart';
 void main() {
   group('foreign key action strings', () {
     test('NO ACTION', () {
-      final actual = annotations.ForeignKeyAction.noAction.toSQL;
+      final actual = annotations.ForeignKeyAction.noAction.toSql();
       expect(actual, equals('NO ACTION'));
     });
 
     test('RESTRICT', () {
-      final actual = annotations.ForeignKeyAction.restrict.toSQL;
+      final actual = annotations.ForeignKeyAction.restrict.toSql();
       expect(actual, equals('RESTRICT'));
     });
 
     test('SET NULL', () {
-      final actual = annotations.ForeignKeyAction.setNull.toSQL;
+      final actual = annotations.ForeignKeyAction.setNull.toSql();
       expect(actual, equals('SET NULL'));
     });
 
     test('SET DEFAULT', () {
-      final actual = annotations.ForeignKeyAction.setDefault.toSQL;
+      final actual = annotations.ForeignKeyAction.setDefault.toSql();
       expect(actual, equals('SET DEFAULT'));
     });
 
     test('CASCADE', () {
-      final actual = annotations.ForeignKeyAction.cascade.toSQL;
+      final actual = annotations.ForeignKeyAction.cascade.toSql();
       expect(actual, equals('CASCADE'));
     });
 
-    test('null annotation returns null', () {
-      final actual = null.toSQL;
-      expect(actual, isNull);
+    test('null throws ArgumentError', () {
+      expect(() => null.toSql(), throwsArgumentError);
     });
   });
 }
diff --git a/floor_generator/test/processor/entity_processor_test.dart b/floor_generator/test/processor/entity_processor_test.dart
@@ -1,5 +1,6 @@
 import 'package:analyzer/dart/element/element.dart';
 import 'package:build_test/build_test.dart';
+import 'package:floor_generator/misc/constants.dart';
 import 'package:floor_generator/processor/entity_processor.dart';
 import 'package:floor_generator/processor/error/entity_processor_error.dart';
 import 'package:floor_generator/processor/field_processor.dart';
@@ -172,8 +173,8 @@ void main() {
       expect(
           processor.process,
           throwsInvalidGenerationSourceError(
-              EntityProcessorError(classElements[1])
-                  .wrongForeignKeyAction(MockDartObject(), true)));
+              EntityProcessorError(classElements[1]).wrongForeignKeyAction(
+                  MockDartObject(), ForeignKeyField.onUpdate)));
     });
   });
 }

diff --git a/floor_generator/test/processor/insertion_method_processor_test.dart b/floor_generator/test/processor/insertion_method_processor_test.dart
@@ -1,4 +1,5 @@
 import 'package:floor_generator/processor/insertion_method_processor.dart';
+import 'package:source_gen/source_gen.dart';
 import 'package:test/test.dart';
 
 import '../test_utils.dart';
@@ -18,4 +19,18 @@ void main() {
 
     expect(actual, equals('OnConflictStrategy.replace'));
   });
+
+  test('Error on wrong onConflict value', () async {
+    final insertionMethod = await '''
+      @Insert(onConflict: OnConflictStrategy.doesnotexist)
+      Future<void> insertPerson(Person person);
+    '''
+        .asDaoMethodElement();
+    final entities = await getPersonEntity();
+
+    final actual =
+        () => InsertionMethodProcessor(insertionMethod, [entities]).process();
+
+    expect(actual, throwsA(const TypeMatcher<InvalidGenerationSourceError>()));
+  });
 }
diff --git a/floor_generator/test/processor/update_method_processor_test.dart b/floor_generator/test/processor/update_method_processor_test.dart
@@ -1,4 +1,5 @@
 import 'package:floor_generator/processor/update_method_processor.dart';
+import 'package:source_gen/source_gen.dart';
 import 'package:test/test.dart';
 
 import '../test_utils.dart';
@@ -17,4 +18,18 @@ void main() {
 
     expect(actual, equals('OnConflictStrategy.replace'));
   });
+
+  test('Error on wrong onConflict value', () async {
+    final insertionMethod = await '''
+      @Update(onConflict: OnConflictStrategy.doesnotexist)
+      Future<void> updatePerson(Person person);
+   '''
+        .asDaoMethodElement();
+    final entities = await getPersonEntity();
+
+    final actual =
+        () => UpdateMethodProcessor(insertionMethod, [entities]).process();
+
+    expect(actual, throwsA(const TypeMatcher<InvalidGenerationSourceError>()));
+  });
 }
diff --git a/floor_generator/test/value_object/entity_test.dart b/floor_generator/test/value_object/entity_test.dart
@@ -133,8 +133,8 @@ void main() {
           'FOREIGN KEY (`${foreignKey.childColumns[0]}`) '
           'REFERENCES `${foreignKey.parentName}` '
           '(`${foreignKey.parentColumns[0]}`) '
-          'ON UPDATE ${foreignKey.onUpdate.toSQL} '
-          'ON DELETE ${foreignKey.onDelete.toSQL}'
+          'ON UPDATE ${foreignKey.onUpdate.toSql()} '
+          'ON DELETE ${foreignKey.onDelete.toSql()}'
           ')';
       expect(actual, equals(expected));
     });