From b8eed2f75b4ae17ddbeea9008c6ecdb1266411e2 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Fri, 8 Jan 2021 22:30:17 +0100 Subject: [PATCH] Make order by placeholders optional (#998) --- moor/lib/src/runtime/api/connection_user.dart | 9 +--- .../query_builder/components/order_by.dart | 14 +++--- .../runtime/query_builder/query_builder.dart | 3 ++ moor/test/data/tables/custom_tables.g.dart | 4 +- .../moor_files_integration_test.dart | 5 +++ .../lib/src/analyzer/options.g.dart | 1 + .../lib/src/writer/queries/query_writer.dart | 45 ++++++++++++------- sqlparser/lib/src/reader/parser/crud.dart | 2 +- 8 files changed, 50 insertions(+), 33 deletions(-) diff --git a/moor/lib/src/runtime/api/connection_user.dart b/moor/lib/src/runtime/api/connection_user.dart index 80cfd217..1e6b7fe3 100644 --- a/moor/lib/src/runtime/api/connection_user.dart +++ b/moor/lib/src/runtime/api/connection_user.dart @@ -437,14 +437,7 @@ abstract class DatabaseConnectionUser { if (hasMultipleTables != null) { context.hasMultipleTables = hasMultipleTables; } - - // we don't want ORDER BY clauses to write the ORDER BY tokens because those - // are already declared in sql - if (component is OrderBy) { - component.writeInto(context, writeOrderBy: false); - } else { - component.writeInto(context); - } + component.writeInto(context); return context; } diff --git a/moor/lib/src/runtime/query_builder/components/order_by.dart b/moor/lib/src/runtime/query_builder/components/order_by.dart index b64f5073..6a1a8c94 100644 --- a/moor/lib/src/runtime/query_builder/components/order_by.dart +++ b/moor/lib/src/runtime/query_builder/components/order_by.dart @@ -56,14 +56,18 @@ class OrderBy extends Component { final List terms; /// Constructs an order by clause by the [terms]. - OrderBy(this.terms); + const OrderBy(this.terms); + + /// Orders by nothing. + /// + /// In this case, the ordering of result rows is undefined. + const OrderBy.nothing() : this(const []); @override - void writeInto(GenerationContext context, {bool writeOrderBy = true}) { - if (writeOrderBy) { - context.buffer.write('ORDER BY '); - } + void writeInto(GenerationContext context) { + if (terms.isEmpty) return; + context.buffer.write('ORDER BY '); _writeCommaSeparated(context, terms); } } diff --git a/moor/lib/src/runtime/query_builder/query_builder.dart b/moor/lib/src/runtime/query_builder/query_builder.dart index 825520d8..da940c95 100644 --- a/moor/lib/src/runtime/query_builder/query_builder.dart +++ b/moor/lib/src/runtime/query_builder/query_builder.dart @@ -46,6 +46,9 @@ part 'migration.dart'; /// A component is anything that can appear in a sql query. abstract class Component { + /// Default, constant constructor. + const Component(); + /// Writes this component into the [context] by writing to its /// [GenerationContext.buffer] or by introducing bound variables. When writing /// into the buffer, no whitespace around the this component should be diff --git a/moor/test/data/tables/custom_tables.g.dart b/moor/test/data/tables/custom_tables.g.dart index 1be85227..5c8ef061 100644 --- a/moor/test/data/tables/custom_tables.g.dart +++ b/moor/test/data/tables/custom_tables.g.dart @@ -1589,14 +1589,14 @@ abstract class _$CustomTablesDb extends GeneratedDatabase { } Selectable readMultiple(List var1, - {required OrderBy clause}) { + {OrderBy clause = const OrderBy.nothing()}) { var $arrayStartIndex = 1; final expandedvar1 = $expandVar($arrayStartIndex, var1.length); $arrayStartIndex += var1.length; final generatedclause = $write(clause); $arrayStartIndex += generatedclause.amountOfVariables; return customSelect( - 'SELECT * FROM config WHERE config_key IN ($expandedvar1) ORDER BY ${generatedclause.sql}', + 'SELECT * FROM config WHERE config_key IN ($expandedvar1) ${generatedclause.sql}', variables: [ for (var $ in var1) Variable($), ...generatedclause.introducedVariables diff --git a/moor/test/parsed_sql/moor_files_integration_test.dart b/moor/test/parsed_sql/moor_files_integration_test.dart index 103d89e4..68599c75 100644 --- a/moor/test/parsed_sql/moor_files_integration_test.dart +++ b/moor/test/parsed_sql/moor_files_integration_test.dart @@ -131,6 +131,11 @@ void main() { verify(mock.runSelect(argThat(contains('with_defaults.a')), any)); }); + test('order by-params are ignored by default', () async { + await db.readMultiple(['foo']).get(); + verify(mock.runSelect(argThat(isNot(contains('with_defaults.a'))), any)); + }); + test('runs queries with nested results', () async { const row = { 'a': 'text for a', diff --git a/moor_generator/lib/src/analyzer/options.g.dart b/moor_generator/lib/src/analyzer/options.g.dart index cff41c28..b4c2e832 100644 --- a/moor_generator/lib/src/analyzer/options.g.dart +++ b/moor_generator/lib/src/analyzer/options.g.dart @@ -93,6 +93,7 @@ MoorOptions _$MoorOptionsFromJson(Map json) { 'modules': 'sqlite_modules', 'rawResultSetData': 'raw_result_set_data', 'generateValuesInCopyWith': 'generate_values_in_copy_with', + 'generateNamedParameters': 'named_parameters', }); } diff --git a/moor_generator/lib/src/writer/queries/query_writer.dart b/moor_generator/lib/src/writer/queries/query_writer.dart index 49120388..138d02c0 100644 --- a/moor_generator/lib/src/writer/queries/query_writer.dart +++ b/moor_generator/lib/src/writer/queries/query_writer.dart @@ -250,7 +250,7 @@ class QueryWriter { var needsComma = false; for (final element in query.elements) { // Placeholders with a default value generate optional (and thus, named) - // parameters. Since moor 3.5, we have an option to also generate named + // parameters. Since moor 4, we have an option to also generate named // parameters for named variables. final isNamed = (element is FoundDartPlaceholder && element.defaultValue != null) || @@ -277,23 +277,34 @@ class QueryWriter { if (needsComma) _buffer.write(', '); needsComma = true; - final type = optional.dartTypeCode(scope.generationOptions); - if (optional is FoundDartPlaceholder && optional.defaultValue != null) { - // Wrap the default expression in parentheses to avoid issues with the - // surrounding precedence in SQL. - final defaultSql = - "'(${escapeForDart(optional.defaultValue.toSql())})'"; - _buffer.write('$type ${optional.dartParameterName} = ' - 'const CustomExpression($defaultSql)'); - } else { - // No default value, this element is required if it's not nullable - final isNullable = - optional is FoundVariable && optional.nullableInDart; - if (!isNullable) { - _buffer..write(scope.required)..write(' '); - } + String defaultCode; - _buffer.write('$type ${optional.dartParameterName}'); + if (optional is FoundDartPlaceholder) { + final kind = optional.type; + if (kind == DartPlaceholderType.expression && + optional.defaultValue != null) { + // Wrap the default expression in parentheses to avoid issues with + // the surrounding precedence in SQL. + final defaultSql = + "'(${escapeForDart(optional.defaultValue.toSql())})'"; + defaultCode = 'const CustomExpression($defaultSql)'; + } else if (kind == DartPlaceholderType.orderBy) { + defaultCode = 'const OrderBy.nothing()'; + } + } + + final type = optional.dartTypeCode(scope.generationOptions); + + // No default value, this element is required if it's not nullable + final isNullable = optional is FoundVariable && optional.nullableInDart; + final isRequired = !isNullable && defaultCode == null; + if (isRequired) { + _buffer..write(scope.required)..write(' '); + } + + _buffer.write('$type ${optional.dartParameterName}'); + if (defaultCode != null) { + _buffer..write(' = ')..write(defaultCode); } } diff --git a/sqlparser/lib/src/reader/parser/crud.dart b/sqlparser/lib/src/reader/parser/crud.dart index 4b391afa..d539f888 100644 --- a/sqlparser/lib/src/reader/parser/crud.dart +++ b/sqlparser/lib/src/reader/parser/crud.dart @@ -478,7 +478,7 @@ mixin CrudParser on ParserBase { if (terms.length == 1 && terms.single is DartOrderingTermPlaceholder) { final termPlaceholder = terms.single as DartOrderingTermPlaceholder; return DartOrderByPlaceholder(name: termPlaceholder.name) - ..setSpan(termPlaceholder.first!, termPlaceholder.last!); + ..setSpan(orderToken, termPlaceholder.last!); } return OrderBy(terms: terms)..setSpan(orderToken, _previous);