diff --git a/packages/dox-orm/CHANGELOG.md b/packages/dox-orm/CHANGELOG.md index 508095e..50bef6e 100644 --- a/packages/dox-orm/CHANGELOG.md +++ b/packages/dox-orm/CHANGELOG.md @@ -1,5 +1,8 @@ ## Unreleased +- Fix: `fromSub()` on a model now uses the alias. Before, the soft delete check and the `chunk()` order used the model table, like `blog.deleted_at`, so `Blog().fromSub(sub, 'b').get()` failed with `no such column: blog.deleted_at`. Now they use `b.deleted_at` and `b.uid`. `update()`, `delete()` and `restore()` after `fromSub()` still run on the model table, as before. +- Fix: `find()`, `findOrFail()`, `firstOrNew()`, `firstOrCreate()` and `updateOrCreate()` on a `union()` query now throw a `StateError`. Before, their where went to the first part only and `LIMIT 1` to the whole union, so they could give a wrong row, and `firstOrCreate()` and `updateOrCreate()` could write the wrong row. Use `get()` on a union. `getFirst()` and `firstOrFail()` work as before. +- Fix: `pluck()`, `pluckMap()` and `valueOf()` on a `union()` query now throw a clear `StateError`. Before, only the first part got the select, so the database gave an SQL error. - Fix: many to many `sync()` and `toggle()` now report in `detached` only the rows that the database really deleted. Before, on a column that compares text without case, with stored `'ABC'` and `'abc'`, `sync(['ABC'])` reported `detached: ['abc']`, but the database kept both rows. - Fix: many to many `sync()` and `toggle()` now check all text ids that match a stored row only by case in one query. Before, they ran one query for each id. - Fix: `restore()` now sets `updated_at` too, like Laravel, when the model has timestamps. It uses a custom `updated_at` column, and sets nothing when the model has no timestamps. This is true for `blog.restore()` (it also sets the `updatedAt` field) and for a query like `Blog().where('id', 1).restore()`. diff --git a/packages/dox-orm/lib/src/get.dart b/packages/dox-orm/lib/src/get.dart index 4aa9d70..302b2b2 100644 --- a/packages/dox-orm/lib/src/get.dart +++ b/packages/dox-orm/lib/src/get.dart @@ -8,6 +8,7 @@ import 'limit.dart'; import 'order_by.dart'; import 'where.dart'; import 'shared_mixin.dart'; +import 'union.dart'; /// the select sql of [qb], as get() runs it. it is a function, not a /// query method, so it can not clash with the name of a model field. @@ -38,7 +39,10 @@ mixin Get implements SharedMixin { /// `find(5, null)`) throws an [ArgumentError]. /// This cannot be use with other query such as, where, join, delete. /// It works with lockForUpdate() and sharedLock(). + /// It throws a [StateError] on a union() query: the id would only be + /// checked in the first part. Use get() then. Future find(dynamic arg1, [dynamic arg2 = missingArg]) { + checkNoUnion(queryBuilder, 'find'); bool idOnly = identical(arg2, missingArg); if (!idOnly && arg1 is! String) { throw ArgumentError.value( @@ -590,7 +594,10 @@ mixin Get implements SharedMixin { } /// select only [columns], as `dox_pluck0`, `dox_pluck1`... + /// it throws a [StateError] on a union() query: only the first part + /// would get the select. Future>> _pluckRows(List columns) async { + checkNoUnion(queryBuilder, 'pluck(), pluckMap() and valueOf'); if (queryBuilder.getRawQuery().isNotEmpty) { throw StateError('pluck() and valueOf() can not be used with rawQuery()'); } @@ -721,7 +728,7 @@ mixin Get implements SharedMixin { String _qualified(String column) { /// the ORDER BY of a union takes only the plain output name if (column.contains('.') || queryBuilder.hasUnion) return column; - return '$tableName.$column'; + return '${tableAlias ?? tableName}.$column'; } /// the value of [key] in a row: a map, or a model (its original row) diff --git a/packages/dox-orm/lib/src/insert.dart b/packages/dox-orm/lib/src/insert.dart index 2e8705f..2db9ecc 100644 --- a/packages/dox-orm/lib/src/insert.dart +++ b/packages/dox-orm/lib/src/insert.dart @@ -1,6 +1,7 @@ import 'package:dox_orm/dox_orm.dart'; import 'shared_mixin.dart'; +import 'union.dart'; import 'update.dart'; mixin Insert implements SharedMixin { @@ -281,7 +282,7 @@ mixin Insert implements SharedMixin { Map where, [ Map values = const {}, ]) async { - T? found = await _whereAll(where).getFirst(); + T? found = await _whereAll(where, 'firstOrCreate').getFirst(); if (found != null) return found; resetSubstitutionValues(); return await queryBuilder.create({...where, ...values}); @@ -298,7 +299,7 @@ mixin Insert implements SharedMixin { Map where, [ Map values = const {}, ]) async { - T? found = await _whereAll(where).getFirst(); + T? found = await _whereAll(where, 'firstOrNew').getFirst(); return found ?? _fromData({...where, ...values}, saved: false); } @@ -318,7 +319,7 @@ mixin Insert implements SharedMixin { Map where, Map values, ) async { - T? found = await _whereAll(where).getFirst(); + T? found = await _whereAll(where, 'updateOrCreate').getFirst(); if (found == null) { resetSubstitutionValues(); return await queryBuilder.create({...where, ...values}); @@ -343,7 +344,10 @@ mixin Insert implements SharedMixin { } /// this query with `where(key, value)` for each pair of [where] - QueryBuilder _whereAll(Map where) { + /// it throws a [StateError] on a union() query: the where would go to + /// the first part only. + QueryBuilder _whereAll(Map where, String method) { + checkNoUnion(queryBuilder, method); for (MapEntry e in where.entries) { queryBuilder.where(e.key, e.value); } diff --git a/packages/dox-orm/lib/src/model.dart b/packages/dox-orm/lib/src/model.dart index d5e73dc..045b29b 100644 --- a/packages/dox-orm/lib/src/model.dart +++ b/packages/dox-orm/lib/src/model.dart @@ -1,6 +1,7 @@ import 'dart:async'; import '../dox_orm.dart'; +import 'union.dart'; class Model extends QueryBuilder { static final Map Function()> _morphModels = @@ -610,8 +611,9 @@ class Model extends QueryBuilder { withTrash(); /// only deleted rows, so every database counts the same rows (MySQL - /// counts only changed rows, Postgres and SQLite every matched row) - whereNotNull('${tableAlias ?? tableName}.deleted_at'); + /// counts only changed rows, Postgres and SQLite every matched row). + /// the UPDATE runs on the real table, so no alias of fromSub(). + whereNotNull('$tableName.deleted_at'); /// super.update(): deleted_at is not checked against fillable return await super.update(_restoreValues()) > 0; @@ -1105,6 +1107,7 @@ class Model extends QueryBuilder { Map where, [ Map values = const {}, ]) async { + checkNoUnion(this, 'firstOrNew'); for (MapEntry e in where.entries) { this.where(e.key, e.value); } @@ -1136,6 +1139,7 @@ class Model extends QueryBuilder { Map where, Map values, ) async { + checkNoUnion(this, 'updateOrCreate'); for (MapEntry e in where.entries) { this.where(e.key, e.value); } diff --git a/packages/dox-orm/lib/src/select.dart b/packages/dox-orm/lib/src/select.dart index 70cd422..e44b582 100644 --- a/packages/dox-orm/lib/src/select.dart +++ b/packages/dox-orm/lib/src/select.dart @@ -36,6 +36,11 @@ mixin Select implements SharedMixin { QueryBuilder fromSub(QueryBuilder query, String alias) { _fromSub = '(${helper.subQuery(query)}) AS ${helper.wrap(alias)}'; tableName = alias; + + /// a model reads its table name from its class, so the line above + /// does nothing there. the alias field keeps [alias] for the + /// columns that need the table in front, like `b.deleted_at`. + tableAlias = alias; return queryBuilder; } diff --git a/packages/dox-orm/lib/src/union.dart b/packages/dox-orm/lib/src/union.dart index 93a721a..9080b02 100644 --- a/packages/dox-orm/lib/src/union.dart +++ b/packages/dox-orm/lib/src/union.dart @@ -60,3 +60,15 @@ mixin Union implements SharedMixin { return queryBuilder; } } + +/// [method] adds its where or select to the first part of a union only, +/// so it gives a wrong row or an sql error. it throws a [StateError] +/// then. +void checkNoUnion(QueryBuilder q, String method) { + if (q.hasUnion) { + throw StateError( + '$method() can not be used on a union() query. ' + 'Use get() and read the rows.', + ); + } +} diff --git a/packages/dox-orm/lib/src/utils/helper.dart b/packages/dox-orm/lib/src/utils/helper.dart index 8d72e0a..b3a0b28 100644 --- a/packages/dox-orm/lib/src/utils/helper.dart +++ b/packages/dox-orm/lib/src/utils/helper.dart @@ -142,7 +142,7 @@ class QueryBuilderHelper { String join = queryBuilder.getJoinQuery(); String orderBy = queryBuilder.getOrderByQuery(); String limit = queryBuilder.getLimitQuery(); - String where = queryBuilder.getWhereQuery(); + String where = queryBuilder.getWhereQuery(write: true); if (join.isEmpty && orderBy.isEmpty && limit.isEmpty) { return where; } diff --git a/packages/dox-orm/lib/src/where.dart b/packages/dox-orm/lib/src/where.dart index 8ac4a8f..dbc0557 100644 --- a/packages/dox-orm/lib/src/where.dart +++ b/packages/dox-orm/lib/src/where.dart @@ -63,16 +63,20 @@ mixin Where implements SharedMixin { static final RegExp _leadingType = RegExp(r'^(AND|OR) '); - String getWhereQuery() { + /// the WHERE part. [write] is true for an UPDATE or DELETE: they run + /// on the real table, so the soft delete check uses the table name, + /// not the alias of fromSub(). + String getWhereQuery({bool write = false}) { queryBuilder.applyGlobalScopes(); List? wheres = _wheres; + String table = write ? tableName : tableAlias ?? tableName; /// wrap user conditions in brackets so an OR can not skip the /// soft delete check. onlyTrashed() keeps its check also when /// forceDelete() turns on withTrashed. if (isSoftDeletes && (trashedOnly || !withTrashed)) { String softDelete = - "${helper.wrap('${tableAlias ?? tableName}.deleted_at')} " + "${helper.wrap('$table.deleted_at')} " "${trashedOnly ? 'IS NOT NULL' : 'IS NULL'}"; if (wheres == null) { return " WHERE $softDelete"; diff --git a/packages/dox-orm/test/bug_fixes_test.dart b/packages/dox-orm/test/bug_fixes_test.dart index e835fcb..a16dcb7 100644 --- a/packages/dox-orm/test/bug_fixes_test.dart +++ b/packages/dox-orm/test/bug_fixes_test.dart @@ -430,6 +430,89 @@ void main() async { ); expect(() => Blog().findOrFail(5, null), throwsArgumentError); }); + + Future> threeBlogs() async { + List blogs = []; + for (String status in ['x', 'y', 'z']) { + Blog blog = Blog() + ..title = status + ..description = status + ..status = status; + await blog.save(); + blogs.add(blog); + } + return blogs; + } + + test('fromSub on a model uses the alias', () async { + List blogs = await threeBlogs(); + await blogs[1].delete(); + QueryBuilder> sub() => + QueryBuilder.table('blog').select('uid, title, deleted_at'); + + /// the soft delete check uses `b.deleted_at`, not `blog.deleted_at` + List rows = await Blog().fromSub(sub(), 'b').orderBy('uid').get(); + expect(rows.map((Blog b) => b.title), ['x', 'z']); + expect(await Blog().fromSub(sub(), 'b').count(), 2); + + /// chunk() orders by `b.uid` + List titles = []; + await Blog().fromSub(sub(), 'b').chunk(1, (List page) { + titles.addAll(page.map((Blog b) => b.title)); + return true; + }); + expect(titles, ['x', 'z']); + }); + + test('update and delete after fromSub on a model', () async { + List blogs = await threeBlogs(); + int id = blogs[0].uid!; + Blog q() => Blog() + ..fromSub(QueryBuilder.table('blog').select('uid'), 'b') + ..where('uid', id); + + /// writes run on the real table, so the soft delete check uses + /// `blog.deleted_at`, not `b.deleted_at` + expect(await q().update({'title': 'new'}), 1); + expect((await Blog().find(id))?.title, 'new'); + expect(await q().delete(), 1); + expect(await Blog().find(id), isNull); + expect(await q().restore(), isTrue); + expect((await Blog().find(id))?.title, 'new'); + }); + + test('find, pluck and first-or helpers throw on a union', () async { + await threeBlogs(); + QueryBuilder> q() => + QueryBuilder.table('blog') + .where('status', 'zzz') + .union(QueryBuilder.table('blog').where('status', 'y')); + expect(() => q().find(1), throwsStateError); + expect(() => q().find('title', 'x'), throwsStateError); + expect(() => q().findOrFail(1), throwsStateError); + expect(() => q().pluck('title'), throwsStateError); + expect(() => q().pluckMap('title', 'uid'), throwsStateError); + expect(() => q().valueOf('title'), throwsStateError); + Map where = {'title': 'x'}; + Map values = {'body': 'b'}; + expect(() => q().firstOrNew(where), throwsStateError); + expect(() => q().firstOrCreate(where, values), throwsStateError); + expect(() => q().updateOrCreate(where, values), throwsStateError); + + /// a model has its own firstOrNew() and updateOrCreate() + Blog blogs() => Blog() + ..where('status', 'zzz') + ..union(Blog().where('status', 'y')); + expect(() => blogs().firstOrNew(where), throwsStateError); + expect(() => blogs().firstOrCreate(where, values), throwsStateError); + expect(() => blogs().updateOrCreate(where, values), throwsStateError); + expect(await Blog().count(), 3); + + /// getFirst() and firstOrFail() put LIMIT 1 on the whole union, so + /// they give the right row + expect((await q().getFirst())?['status'], 'y'); + expect((await q().firstOrFail())['status'], 'y'); + }); }); group('Schema fixes |', () {