From cf1e94d384523a529516724aa3801b53e6b3a5b1 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Thu, 23 Jun 2022 20:56:56 +0200 Subject: [PATCH] Automatically make some converters nullable --- .../snippets/type_converters/converters.dart | 7 ++- drift/test/generated/custom_tables.g.dart | 2 +- drift/test/generated/tables.drift | 3 +- drift/test/generated/todos.dart | 6 +-- drift_dev/lib/src/analyzer/dart_types.dart | 27 +++++++---- drift_dev/lib/src/model/types.dart | 3 +- .../lib/src/model/used_type_converter.dart | 46 +++++++++++++++---- .../lib/src/writer/tables/table_writer.dart | 14 +++++- .../analyzer/dart/type_converter_test.dart | 11 ++++- 9 files changed, 83 insertions(+), 36 deletions(-) diff --git a/docs/lib/snippets/type_converters/converters.dart b/docs/lib/snippets/type_converters/converters.dart index 48dd5abb..909b28ad 100644 --- a/docs/lib/snippets/type_converters/converters.dart +++ b/docs/lib/snippets/type_converters/converters.dart @@ -22,17 +22,16 @@ class Preferences { // #docregion converter // stores preferences as strings -class PreferenceConverter extends NullAwareTypeConverter - with JsonTypeConverter { +class PreferenceConverter extends TypeConverter { const PreferenceConverter(); @override - Preferences requireMapToDart(String fromDb) { + Preferences mapToDart(String fromDb) { return Preferences.fromJson(json.decode(fromDb) as Map); } @override - String requireMapToSql(Preferences value) { + String mapToSql(Preferences value) { return json.encode(value.toJson()); } } diff --git a/drift/test/generated/custom_tables.g.dart b/drift/test/generated/custom_tables.g.dart index 60734e41..04be69d1 100644 --- a/drift/test/generated/custom_tables.g.dart +++ b/drift/test/generated/custom_tables.g.dart @@ -282,7 +282,7 @@ class ConfigTable extends Table with TableInfo { } static TypeConverter $converter0 = - const NullAwareTypeConverter.wrap(SyncTypeConverter()); + NullAwareTypeConverter.wrap(const SyncTypeConverter()); static TypeConverter $converter1 = const NullAwareTypeConverter.wrap( EnumIndexConverter(SyncType.values)); diff --git a/drift/test/generated/tables.drift b/drift/test/generated/tables.drift index a106d03b..0148a192 100644 --- a/drift/test/generated/tables.drift +++ b/drift/test/generated/tables.drift @@ -1,4 +1,3 @@ -import 'package:drift/drift.dart'; import 'data_classes.dart'; import 'converter.dart'; @@ -22,7 +21,7 @@ CREATE TABLE with_constraints ( create table config ( config_key TEXT not null primary key, config_value TEXT, - sync_state INTEGER MAPPED BY `const NullAwareTypeConverter.wrap(SyncTypeConverter())`, + sync_state INTEGER MAPPED BY `const SyncTypeConverter()`, sync_state_implicit ENUM(SyncType) ) STRICT AS "Config"; diff --git a/drift/test/generated/todos.dart b/drift/test/generated/todos.dart index c8832858..30d4d479 100644 --- a/drift/test/generated/todos.dart +++ b/drift/test/generated/todos.dart @@ -98,10 +98,8 @@ class CustomRowClass { class PureDefaults extends Table { // name after keyword to ensure it's escaped properly - TextColumn get txt => text() - .named('insert') - .map(JsonTypeConverter.asNullable(const CustomJsonConverter())) - .nullable()(); + TextColumn get txt => + text().named('insert').map(const CustomJsonConverter()).nullable()(); @override Set get primaryKey => {txt}; diff --git a/drift_dev/lib/src/analyzer/dart_types.dart b/drift_dev/lib/src/analyzer/dart_types.dart index 0ca02539..d60cfc54 100644 --- a/drift_dev/lib/src/analyzer/dart_types.dart +++ b/drift_dev/lib/src/analyzer/dart_types.dart @@ -134,8 +134,17 @@ UsedTypeConverter? readTypeConverter( final appliesToJsonToo = helper.isJsonAwareTypeConverter(staticType, library); + // Make the type converter support nulls by just mapping null to null if this + // converter is otherwise non-nullable in both directions but applied on a + // nullable column + final skipForNull = !dartTypeNullable && !sqlTypeNullable && columnIsNullable; + if (sqlTypeNullable != columnIsNullable) { - if (columnIsNullable) { + if (!columnIsNullable) { + reportError('This column is non-nullable in the database, but has a ' + 'type converter with a nullable SQL type, meaning that it may ' + "potentially map to `null` which can't be stored in the database."); + } else if (!skipForNull) { final alternative = appliesToJsonToo ? 'JsonTypeConverter.asNullable' : 'NullAwareTypeConverter.wrap'; @@ -144,23 +153,20 @@ UsedTypeConverter? readTypeConverter( "nullable SQL type, meaning that it won't be able to map `null` " 'from the database to Dart.\n' 'Try wrapping the converter in `$alternative`'); - } else { - reportError('This column is non-nullable in the database, but has a ' - 'type converter with a nullable SQL type, meaning that it may ' - "potentially map to `null` which can't be stored in the database."); } } - _checkType(columnType, columnIsNullable, null, sqlType, library.typeProvider, + _checkType(columnType, null, sqlType, library.typeProvider, library.typeSystem, reportError); return UsedTypeConverter( expression: dartExpression.toSource(), dartType: resolvedDartType ?? DriftDartType.of(dartType), sqlType: sqlType, - mapsToNullableDart: dartTypeNullable, - mapsToNullableSql: sqlTypeNullable, + dartTypeIsNullable: dartTypeNullable, + sqlTypeIsNullable: sqlTypeNullable, alsoAppliesToJsonConversion: appliesToJsonToo, + skipForNulls: skipForNull, ); } @@ -191,7 +197,6 @@ void _checkParameterType( _checkType( column.type, - column.nullable, column.typeConverter, element.type, library.typeProvider, @@ -202,7 +207,6 @@ void _checkParameterType( void _checkType( ColumnType columnType, - bool columnIsNullable, UsedTypeConverter? typeConverter, DartType typeToCheck, TypeProvider typeProvider, @@ -212,6 +216,9 @@ void _checkType( DriftDartType expectedDartType; if (typeConverter != null) { expectedDartType = typeConverter.dartType; + if (typeConverter.skipForNulls) { + typeToCheck = typeSystem.promoteToNonNull(typeToCheck); + } } else { expectedDartType = DriftDartType.of(typeProvider.typeFor(columnType)); } diff --git a/drift_dev/lib/src/model/types.dart b/drift_dev/lib/src/model/types.dart index d9c764b2..da1b6a6d 100644 --- a/drift_dev/lib/src/model/types.dart +++ b/drift_dev/lib/src/model/types.dart @@ -110,7 +110,8 @@ extension OperationOnTypes on HasType { String dartTypeCode([GenerationOptions options = const GenerationOptions()]) { final converter = typeConverter; if (converter != null) { - final inner = converter.dartType.codeString(options); + var inner = converter.dartType.codeString(options); + if (converter.skipForNulls) inner += '?'; return isArray ? 'List<$inner>' : inner; } diff --git a/drift_dev/lib/src/model/used_type_converter.dart b/drift_dev/lib/src/model/used_type_converter.dart index 74813e06..9bd54a23 100644 --- a/drift_dev/lib/src/model/used_type_converter.dart +++ b/drift_dev/lib/src/model/used_type_converter.dart @@ -20,8 +20,12 @@ class UsedTypeConverter { /// vice-versa. final String expression; - /// The "Dart" type of this type converter. This is the type used on - /// companions and data classes. + /// The "Dart" type of this type converter. + /// + /// Note that, even when this type is non-nullable, the actual type used on + /// data classes and companions can still be nullable. For instance, when a + /// non-nullable type converter is applied to a nullable column, drift will + /// implicitly map `null` values to `null` without invoking the converter. final DriftDartType dartType; /// The "SQL" type of this type converter. This is the type used to represent @@ -31,17 +35,27 @@ class UsedTypeConverter { /// Whether the Dart-value output of this type converter is nullable. /// /// In other words, [dartType] is potentially nullable. - final bool mapsToNullableDart; + final bool dartTypeIsNullable; /// Whether the SQL-value output of this type converter is nullable. /// /// In other words, [sqlType] is potentially nullable. - final bool mapsToNullableSql; + final bool sqlTypeIsNullable; /// Whether this type converter should also be used in the generated JSON /// serialization. final bool alsoAppliesToJsonConversion; + /// Whether this type converter should be skipped for `null` values. + /// + /// This applies to type converters with a non-nullable Dart and SQL type if + /// the column is nullable. For those converters, drift maps `null` to `null` + /// without calling the type converter at all. + /// + /// This is implemented by wrapping it in a `NullAwareTypeConverter` in the + /// generated code. + final bool skipForNulls; + /// Type converters are stored as static fields in the table that created /// them. This will be the field name for this converter. String get fieldName => '\$converter$index'; @@ -53,9 +67,10 @@ class UsedTypeConverter { required this.expression, required this.dartType, required this.sqlType, - required this.mapsToNullableDart, - required this.mapsToNullableSql, + required this.dartTypeIsNullable, + required this.sqlTypeIsNullable, this.alsoAppliesToJsonConversion = false, + this.skipForNulls = false, }); factory UsedTypeConverter.forEnumColumn( @@ -89,8 +104,8 @@ class UsedTypeConverter { overiddenSource: creatingClass.name, nullabilitySuffix: suffix, ), - mapsToNullableDart: nullable, - mapsToNullableSql: nullable, + sqlTypeIsNullable: nullable, + dartTypeIsNullable: nullable, sqlType: nullable ? typeProvider.intElement.instantiate( typeArguments: const [], @@ -99,13 +114,24 @@ class UsedTypeConverter { ); } + bool get mapsToNullableDart => dartTypeIsNullable || skipForNulls; + + String dartTypeCode(GenerationOptions options) { + var type = dartType.codeString(options); + if (options.nnbd && skipForNulls) type += '?'; + + return type; + } + /// A suitable typename to store an instance of the type converter used here. String converterNameInCode(GenerationOptions options) { - final sqlDartType = sqlType.getDisplayString(withNullability: options.nnbd); + var sqlDartType = sqlType.getDisplayString(withNullability: options.nnbd); + if (options.nnbd && skipForNulls) sqlDartType += '?'; + final className = alsoAppliesToJsonConversion ? 'JsonTypeConverter' : 'TypeConverter'; - return '$className<${dartType.codeString(options)}, $sqlDartType>'; + return '$className<${dartTypeCode(options)}, $sqlDartType>'; } } diff --git a/drift_dev/lib/src/writer/tables/table_writer.dart b/drift_dev/lib/src/writer/tables/table_writer.dart index 5dbb510e..1fd72d33 100644 --- a/drift_dev/lib/src/writer/tables/table_writer.dart +++ b/drift_dev/lib/src/writer/tables/table_writer.dart @@ -100,7 +100,8 @@ abstract class TableOrViewWriter { if (converter != null) { // Generate a GeneratedColumnWithTypeConverter instance, as it has // additional methods to check for equality against a mapped value. - final mappedType = converter.dartType.codeString(options); + final mappedType = converter.dartTypeCode(options); + type = 'GeneratedColumnWithTypeConverter<$mappedType, $innerType>'; expressionBuffer ..write('.withConverter<') @@ -310,7 +311,16 @@ class TableWriter extends TableOrViewWriter { void _writeConvertersAsStaticFields() { for (final converter in table.converters) { final typeName = converter.converterNameInCode(scope.generationOptions); - final code = converter.expression; + var code = converter.expression; + + if (converter.skipForNulls) { + if (converter.alsoAppliesToJsonConversion) { + code = 'JsonTypeConverter.asNullable($code)'; + } else { + code = 'NullAwareTypeConverter.wrap($code)'; + } + } + buffer.write('static $typeName ${converter.fieldName} = $code;'); } } diff --git a/drift_dev/test/analyzer/dart/type_converter_test.dart b/drift_dev/test/analyzer/dart/type_converter_test.dart index b822b2fa..7d0678ac 100644 --- a/drift_dev/test/analyzer/dart/type_converter_test.dart +++ b/drift_dev/test/analyzer/dart/type_converter_test.dart @@ -1,4 +1,5 @@ import 'package:drift_dev/src/analyzer/errors.dart'; +import 'package:drift_dev/src/analyzer/runner/results.dart'; import 'package:drift_dev/src/model/table.dart'; import 'package:drift_dev/src/model/used_type_converter.dart'; import 'package:test/test.dart'; @@ -29,7 +30,8 @@ TypeConverter tc() => throw 'stub'; class Users extends Table { TextColumn get wrongSqlType => text().map(tc())(); TextColumn get illegalNull => text().map(tc())(); - TextColumn get illegalNonNull => text().map(tc()).nullable()(); + TextColumn get illegalNonNull => text().map(tc()).nullable()(); + TextColumn get implicitlyNullAware => text().map(tc()).nullable()(); } ''', 'a|lib/main.drift': ''' @@ -71,6 +73,8 @@ CREATE TABLE users ( test('warns about type issues around converters', () async { final result = await state.analyze('package:a/nullability.dart'); + final table = + (result.currentResult as ParsedDartFile).declaredTables.single; expect( result.errors.errors, @@ -85,9 +89,12 @@ CREATE TABLE users ( isA() .having((e) => e.message, 'message', contains('This column is nullable')) - .having((e) => e.span?.text, 'span', 'tc()'), + .having((e) => e.span?.text, 'span', 'tc()'), ], ); + + final implicitlyNullAware = table.columns[3]; + expect(implicitlyNullAware.typeConverter?.skipForNulls, isTrue); }); test('json converters in drift files', () {