Skip to content

Commit d29727e

Browse files
tshedortshedor
authored
fix(sqlite): resolve ambiguous column in association queries with ORDER BY statements #561 (#564)
1 parent 2f9c41d commit d29727e

File tree

5 files changed

+90
-89
lines changed

5 files changed

+90
-89
lines changed

‎example_rest/pubspec.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ dependencies:
1818
dependency_overrides:
1919
brick_sqlite_generators:
2020
path: ../packages/brick_sqlite_generators
21+
brick_rest:
22+
path: ../packages/brick_rest
2123

2224
dev_dependencies:
2325
brick_offline_first_with_rest_build:

‎packages/brick_sqlite/CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
## 4.0.1
2+
3+
- Fix ambiguous column in association queries with ORDER BY statements (#561)
4+
15
## 4.0.0
26

37
- **BREAKING CHANGE** Require `brick_core: >= 2.0.0` and remove support for `Query(providerArgs:)`; see [migration steps](https://github.com/GetDutchie/brick/issues/510)

‎packages/brick_sqlite/lib/src/helpers/query_sql_transformer.dart‎

Lines changed: 55 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ class QuerySqlTransformer<_Model extends SqliteModel> {
9090
if (_where.isNotEmpty) _statement.add(whereClause);
9191

9292
_statement.add(
93-
AllOtherClausesFragment(
93+
AllOtherClausesFragment<_Model>(
9494
query,
9595
fieldsToColumns: adapter.fieldsToSqliteColumns,
9696
modelDictionary: modelDictionary,
@@ -175,7 +175,9 @@ class QuerySqlTransformer<_Model extends SqliteModel> {
175175
/// Finally add the column to the complete phrase
176176
final sqliteColumn = passedAdapter.tableName != adapter.tableName || _innerJoins.isNotEmpty
177177
? '`${passedAdapter.tableName}`.${definition.columnName}'
178-
: definition.columnName;
178+
: _innerJoins.isNotEmpty
179+
? '`${adapter.tableName}`.${definition.columnName}'
180+
: definition.columnName;
179181
final where = WhereColumnFragment(condition, sqliteColumn);
180182
_values.addAll(where.values);
181183
return where.toString();
@@ -341,7 +343,7 @@ class WhereColumnFragment {
341343
}
342344

343345
/// Query modifiers such as `LIMIT`, `OFFSET`, etc. that require minimal logic.
344-
class AllOtherClausesFragment {
346+
class AllOtherClausesFragment<T extends SqliteModel> {
345347
///
346348
final Map<String, RuntimeSqliteColumnDefinition> fieldsToColumns;
347349

@@ -351,21 +353,6 @@ class AllOtherClausesFragment {
351353
///
352354
final Query? query;
353355

354-
/// Order matters. For example, LIMIT has to follow an ORDER BY but precede an OFFSET.
355-
static const _supportedOperators = <String, String>{
356-
'collate': 'COLLATE',
357-
'orderBy': 'ORDER BY',
358-
'groupBy': 'GROUP BY',
359-
'having': 'HAVING',
360-
'limit': 'LIMIT',
361-
'offset': 'OFFSET',
362-
};
363-
364-
/// These operators declare a column to compare against. The fields provided in
365-
/// [Query] or [SqliteProviderQuery] will have to be converted to their column name.
366-
/// For example, `orderBy: [OrderBy.asc('createdAt')]` must become `ORDER BY created_at ASC`.
367-
static const _operatorsDeclaringFields = <String>{'ORDER BY', 'GROUP BY', 'HAVING'};
368-
369356
/// Query modifiers such as `LIMIT`, `OFFSET`, etc. that require minimal logic.
370357
AllOtherClausesFragment(
371358
this.query, {
@@ -376,71 +363,59 @@ class AllOtherClausesFragment {
376363
@override
377364
String toString() {
378365
final providerQuery = query?.providerQueries[SqliteProvider] as SqliteProviderQuery?;
379-
final argsToSqlStatments = {
380-
if (providerQuery?.collate != null) 'collate': providerQuery?.collate,
381-
if (query?.limit != null) 'limit': query?.limit,
382-
if (query?.offset != null) 'offset': query?.offset,
383-
if (query?.orderBy.isNotEmpty ?? false)
384-
'orderBy': query?.orderBy.map((p) {
385-
final isAssociation = fieldsToColumns[p.evaluatedField]?.association ?? false;
386-
if (!isAssociation) return p.toString();
387-
388-
if (p.associationField == null) return p.toString();
389-
390-
final associationAdapter =
391-
modelDictionary.adapterFor[fieldsToColumns[p.evaluatedField]?.type];
392-
393-
return '`${associationAdapter?.tableName}`.${associationAdapter?.fieldsToSqliteColumns[p.associationField]?.columnName} ${p.ascending ? 'ASC' : 'DESC'}';
394-
}).join(', '),
395-
if (providerQuery?.groupBy != null) 'groupBy': providerQuery?.groupBy,
396-
if (providerQuery?.having != null) 'having': providerQuery?.having,
397-
};
398-
399-
return _supportedOperators.entries.fold<List<String>>(<String>[], (acc, entry) {
400-
final op = entry.value;
401-
var value = argsToSqlStatments[entry.key];
402-
403-
if (value == null) return acc;
404-
405-
if (_operatorsDeclaringFields.contains(op)) {
406-
value = value.toString().split(',').fold<String>(value.toString(),
407-
(modValue, innerValueClause) {
408-
// TODO(tshedor): revisit and remove providerArgs hacks here after
409-
// providerArgs is fully deprecated
410-
final fragment = innerValueClause.trim().split(' ');
411-
if (fragment.isEmpty) return modValue;
412-
413-
final fieldName = fragment.first;
414-
final columnDefinition = fieldsToColumns[fieldName];
415-
var columnName = columnDefinition?.columnName;
416-
if (columnName != null && modValue.contains(fieldName)) {
417-
if (columnDefinition!.type == DateTime) {
418-
columnName = 'datetime($columnName)';
419-
}
420-
return modValue.replaceAll(fieldName, columnName);
421-
}
422-
423-
final tableFragment = innerValueClause.trim().split('.');
424-
if (fragment.isEmpty) return modValue;
425-
426-
final tabledFieldName = tableFragment.last;
427-
final tabledColumnDefinition = fieldsToColumns[fieldName];
428-
var tabledColumnName = tabledColumnDefinition?.columnName;
429-
if (tabledColumnName != null && modValue.contains(fieldName)) {
430-
if (columnDefinition!.type == DateTime) {
431-
tabledColumnName = 'datetime($tabledColumnName)';
432-
}
433-
return modValue.replaceAll(tabledFieldName, tabledColumnName);
434-
}
435-
436-
return modValue;
437-
});
366+
final adapter = modelDictionary.adapterFor[T]!;
367+
368+
final orderBy = query?.orderBy.map((p) {
369+
final fieldDefinition = fieldsToColumns[p.evaluatedField];
370+
final isAssociation = fieldDefinition?.association ?? false;
371+
final field = '`${adapter.tableName}`.${fieldDefinition?.columnName ?? p.evaluatedField}';
372+
if (!isAssociation) {
373+
if (fieldDefinition?.type == DateTime) {
374+
return 'datetime($field) ${p.ascending ? 'ASC' : 'DESC'}';
375+
}
376+
return '$field ${p.ascending ? 'ASC' : 'DESC'}';
377+
}
378+
379+
if (p.associationField == null) {
380+
return '${fieldDefinition?.columnName ?? p.evaluatedField} ${p.ascending ? 'ASC' : 'DESC'}';
381+
}
382+
383+
final associationAdapter = modelDictionary.adapterFor[fieldDefinition?.type];
384+
final associationField =
385+
'`${associationAdapter?.tableName}`.${associationAdapter?.fieldsToSqliteColumns[p.associationField]?.columnName ?? p.associationField}';
386+
if (fieldDefinition?.type == DateTime) {
387+
return 'datetime($associationField) ${p.ascending ? 'ASC' : 'DESC'}';
438388
}
389+
return '$associationField ${p.ascending ? 'ASC' : 'DESC'}';
390+
}).join(', ');
439391

440-
acc.add('$op $value');
392+
return [
393+
if (providerQuery?.collate != null)
394+
'COLLATE ${_fieldToSqliteColumn(providerQuery!.collate!)}',
395+
if (query?.orderBy.isNotEmpty ?? false) 'ORDER BY $orderBy',
396+
if (providerQuery?.groupBy != null)
397+
'GROUP BY ${_fieldToSqliteColumn(providerQuery!.groupBy!)}',
398+
if (providerQuery?.having != null) 'HAVING ${_fieldToSqliteColumn(providerQuery!.having!)}',
399+
if (query?.limit != null) 'LIMIT ${query?.limit}',
400+
if (query?.offset != null) 'OFFSET ${query?.offset}',
401+
].join(' ');
402+
}
441403

442-
return acc;
443-
}).join(' ');
404+
String _fieldToSqliteColumn(String field) {
405+
final adapter = modelDictionary.adapterFor[T]!;
406+
// split on every whole word
407+
final parts = field.split(RegExp(r'\b'));
408+
var replacement = field;
409+
for (final part in parts) {
410+
final definition = fieldsToColumns[part];
411+
if (definition != null) {
412+
replacement = replacement.replaceAll(
413+
part,
414+
'`${adapter.tableName}`.${definition.columnName}',
415+
);
416+
}
417+
}
418+
return replacement;
444419
}
445420
}
446421

‎packages/brick_sqlite/pubspec.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ homepage: https://github.com/GetDutchie/brick/tree/main/packages/brick_sqlite
44
issue_tracker: https://github.com/GetDutchie/brick/issues
55
repository: https://github.com/GetDutchie/brick
66

7-
version: 4.0.0
7+
version: 4.0.1
88

99
environment:
1010
sdk: ">=3.4.0 <4.0.0"

‎packages/brick_sqlite/test/query_sql_transformer_test.dart‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,25 @@ void main() {
291291
sqliteStatementExpectation(statement, [1]);
292292
});
293293

294+
test('same field', () async {
295+
const statement =
296+
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` INNER JOIN `DemoModelAssoc` ON `DemoModel`.assoc_DemoModelAssoc_brick_id = `DemoModelAssoc`._brick_id WHERE `DemoModelAssoc`.id = ? AND `DemoModel`.id = ? ORDER BY `DemoModel`.id ASC';
297+
final sqliteQuery = QuerySqlTransformer<DemoModel>(
298+
modelDictionary: dictionary,
299+
query: const Query(
300+
where: [
301+
Where.exact('assoc', Where.exact('id', 1)),
302+
Where.exact('id', 2),
303+
],
304+
orderBy: [OrderBy.asc('id')],
305+
),
306+
);
307+
308+
expect(sqliteQuery.statement, statement);
309+
await db.rawQuery(sqliteQuery.statement, sqliteQuery.values);
310+
sqliteStatementExpectation(statement, [1, 2]);
311+
});
312+
294313
test('nested association', () async {
295314
const statement =
296315
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` INNER JOIN `_brick_DemoModel_many_assoc` ON `DemoModel`._brick_id = `_brick_DemoModel_many_assoc`.l_DemoModel_brick_id INNER JOIN `DemoModelAssoc` ON `DemoModelAssoc`._brick_id = `_brick_DemoModel_many_assoc`.f_DemoModelAssoc_brick_id WHERE `DemoModelAssoc`.full_name = ?';
@@ -364,7 +383,7 @@ void main() {
364383
});
365384

366385
test('#groupBy', () async {
367-
const statement = 'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` GROUP BY id';
386+
const statement = 'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` GROUP BY `DemoModel`.id';
368387
final sqliteQuery = QuerySqlTransformer<DemoModel>(
369388
modelDictionary: dictionary,
370389
query: const Query(forProviders: [SqliteProviderQuery(groupBy: 'id')]),
@@ -376,7 +395,7 @@ void main() {
376395
});
377396

378397
test('#having', () async {
379-
const statement = 'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` HAVING id';
398+
const statement = 'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` HAVING `DemoModel`.id';
380399
final sqliteQuery = QuerySqlTransformer<DemoModel>(
381400
modelDictionary: dictionary,
382401
query: const Query(forProviders: [SqliteProviderQuery(having: 'id')]),
@@ -425,7 +444,8 @@ void main() {
425444

426445
group('#orderBy', () {
427446
test('simple', () async {
428-
const statement = 'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY id DESC';
447+
const statement =
448+
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY `DemoModel`.id DESC';
429449
final sqliteQuery = QuerySqlTransformer<DemoModel>(
430450
modelDictionary: dictionary,
431451
query: const Query(orderBy: [OrderBy.desc('id')]),
@@ -438,7 +458,7 @@ void main() {
438458

439459
test('discovered columns share similar names', () async {
440460
const statement =
441-
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY last_name DESC';
461+
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY `DemoModel`.last_name DESC';
442462
final sqliteQuery = QuerySqlTransformer<DemoModel>(
443463
modelDictionary: dictionary,
444464
query: const Query(orderBy: [OrderBy.desc('lastName')]),
@@ -464,7 +484,7 @@ void main() {
464484

465485
test('compound values are expanded to column names', () async {
466486
const statement =
467-
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY many_assoc DESC, complex_field_name ASC';
487+
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY many_assoc DESC, `DemoModel`.complex_field_name ASC';
468488
final sqliteQuery = QuerySqlTransformer<DemoModel>(
469489
modelDictionary: dictionary,
470490
query: const Query(
@@ -480,7 +500,7 @@ void main() {
480500

481501
test('fields convert to column names in providerArgs values', () async {
482502
const statement =
483-
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY complex_field_name ASC GROUP BY complex_field_name HAVING complex_field_name > 1000';
503+
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY `DemoModel`.complex_field_name ASC GROUP BY `DemoModel`.complex_field_name HAVING `DemoModel`.complex_field_name > 1000';
484504
final sqliteQuery = QuerySqlTransformer<DemoModel>(
485505
modelDictionary: dictionary,
486506
query: const Query(
@@ -502,7 +522,7 @@ void main() {
502522

503523
test('date time is converted', () async {
504524
const statement =
505-
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY datetime(simple_time) DESC';
525+
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY datetime(`DemoModel`.simple_time) DESC';
506526
final sqliteQuery = QuerySqlTransformer<DemoModel>(
507527
modelDictionary: dictionary,
508528
query: const Query(orderBy: [OrderBy.desc('simpleTime')]),
@@ -519,7 +539,7 @@ void main() {
519539
// https://github.com/GetDutchie/brick/issues/429
520540
test('incorrectly cased columns are forwarded as is', () async {
521541
const statement =
522-
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY complex_field_name DESC';
542+
'SELECT DISTINCT `DemoModel`.* FROM `DemoModel` ORDER BY `DemoModel`.complex_field_name DESC';
523543
final sqliteQuery = QuerySqlTransformer<DemoModel>(
524544
modelDictionary: dictionary,
525545
query: const Query(orderBy: [OrderBy.desc('complex_field_name')]),

0 commit comments

Comments
 (0)