fix: fromSub alias on a model, find and pluck on a union

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()`.

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<T> implements SharedMixin<T> {
   /// `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<T?> 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<T> implements SharedMixin<T> {
   }
 
   /// 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<List<Map<String, dynamic>>> _pluckRows(List<String> 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<T> implements SharedMixin<T> {
   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)

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<T> implements SharedMixin<T> {
@@ -281,7 +282,7 @@ mixin Insert<T> implements SharedMixin<T> {
     Map<String, dynamic> where, [
     Map<String, dynamic> values = const <String, dynamic>{},
   ]) async {
-    T? found = await _whereAll(where).getFirst();
+    T? found = await _whereAll(where, 'firstOrCreate').getFirst();
     if (found != null) return found;
     resetSubstitutionValues();
     return await queryBuilder.create(<String, dynamic>{...where, ...values});
@@ -298,7 +299,7 @@ mixin Insert<T> implements SharedMixin<T> {
     Map<String, dynamic> where, [
     Map<String, dynamic> values = const <String, dynamic>{},
   ]) async {
-    T? found = await _whereAll(where).getFirst();
+    T? found = await _whereAll(where, 'firstOrNew').getFirst();
     return found ??
         _fromData(<String, dynamic>{...where, ...values}, saved: false);
   }
@@ -318,7 +319,7 @@ mixin Insert<T> implements SharedMixin<T> {
     Map<String, dynamic> where,
     Map<String, dynamic> values,
   ) async {
-    T? found = await _whereAll(where).getFirst();
+    T? found = await _whereAll(where, 'updateOrCreate').getFirst();
     if (found == null) {
       resetSubstitutionValues();
       return await queryBuilder.create(<String, dynamic>{...where, ...values});
@@ -343,7 +344,10 @@ mixin Insert<T> implements SharedMixin<T> {
   }
 
   /// this query with `where(key, value)` for each pair of [where]
-  QueryBuilder<T> _whereAll(Map<String, dynamic> where) {
+  /// it throws a [StateError] on a union() query: the where would go to
+  /// the first part only.
+  QueryBuilder<T> _whereAll(Map<String, dynamic> where, String method) {
+    checkNoUnion(queryBuilder, method);
     for (MapEntry<String, dynamic> e in where.entries) {
       queryBuilder.where(e.key, e.value);
     }

packages/dox-orm/lib/src/model.dart

@@ -1,6 +1,7 @@
 import 'dart:async';
 
 import '../dox_orm.dart';
+import 'union.dart';
 
 class Model<T> extends QueryBuilder<T> {
   static final Map<String, Model<dynamic> Function()> _morphModels =
@@ -610,8 +611,9 @@ class Model<T> extends QueryBuilder<T> {
       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<T> extends QueryBuilder<T> {
     Map<String, dynamic> where, [
     Map<String, dynamic> values = const <String, dynamic>{},
   ]) async {
+    checkNoUnion(this, 'firstOrNew');
     for (MapEntry<String, dynamic> e in where.entries) {
       this.where(e.key, e.value);
     }
@@ -1136,6 +1139,7 @@ class Model<T> extends QueryBuilder<T> {
     Map<String, dynamic> where,
     Map<String, dynamic> values,
   ) async {
+    checkNoUnion(this, 'updateOrCreate');
     for (MapEntry<String, dynamic> e in where.entries) {
       this.where(e.key, e.value);
     }

packages/dox-orm/lib/src/select.dart

@@ -36,6 +36,11 @@ mixin Select<T> implements SharedMixin<T> {
   QueryBuilder<T> fromSub(QueryBuilder<dynamic> 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;
   }
 

packages/dox-orm/lib/src/union.dart

@@ -60,3 +60,15 @@ mixin Union<T> implements SharedMixin<T> {
     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<dynamic> q, String method) {
+  if (q.hasUnion) {
+    throw StateError(
+      '$method() can not be used on a union() query. '
+      'Use get() and read the rows.',
+    );
+  }
+}

packages/dox-orm/lib/src/utils/helper.dart

@@ -142,7 +142,7 @@ class QueryBuilderHelper<T> {
     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;
     }

packages/dox-orm/lib/src/where.dart

@@ -63,16 +63,20 @@ mixin Where<T> implements SharedMixin<T> {
 
   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<String>? 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";

packages/dox-orm/test/bug_fixes_test.dart

@@ -430,6 +430,89 @@ void main() async {
       );
       expect(() => Blog().findOrFail(5, null), throwsArgumentError);
     });
+
+    Future<List<Blog>> threeBlogs() async {
+      List<Blog> blogs = <Blog>[];
+      for (String status in <String>['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<Blog> blogs = await threeBlogs();
+      await blogs[1].delete();
+      QueryBuilder<Map<String, dynamic>> sub() =>
+          QueryBuilder.table('blog').select('uid, title, deleted_at');
+
+      /// the soft delete check uses `b.deleted_at`, not `blog.deleted_at`
+      List<Blog> rows = await Blog().fromSub(sub(), 'b').orderBy('uid').get();
+      expect(rows.map((Blog b) => b.title), <String>['x', 'z']);
+      expect(await Blog().fromSub(sub(), 'b').count(), 2);
+
+      /// chunk() orders by `b.uid`
+      List<String?> titles = <String?>[];
+      await Blog().fromSub(sub(), 'b').chunk(1, (List<Blog> page) {
+        titles.addAll(page.map((Blog b) => b.title));
+        return true;
+      });
+      expect(titles, <String>['x', 'z']);
+    });
+
+    test('update and delete after fromSub on a model', () async {
+      List<Blog> 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(<String, dynamic>{'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<Map<String, dynamic>> 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<String, dynamic> where = <String, dynamic>{'title': 'x'};
+      Map<String, dynamic> values = <String, dynamic>{'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 |', () {