Keep selections aliased as join columns in withGraphFetched() and relation queries (#111)

doc/release-notes/changelog.md

@@ -43,6 +43,8 @@
 - `upsertGraph()` keeps the current model of a `HasOneRelation` or `HasOneThroughRelation` when the graph holds a model marked with `#unrelate` or `#delete` that isn't the current one, or a replacement that `noInsert` or `noRelate` skips, like for `BelongsToOneRelation` since 3.9. 3.x deleted or unrelated the current model. To-many relations are unchanged: their array is the full list. See [migration](/release-notes/migration.md#upsertgraph-keeps-the-current-to-one-model-for-ignored-models) [#261](https://github.com/ditojs/objection/issues/261)
 - `upsertGraph()` throws a `NotFoundError` when a relate or update finds no row to write, e.g. a row moved below a new row while `ON DELETE CASCADE` deletes it with its old parent, or a row deleted by a concurrent query, instead of reporting success and returning the row. Writes that a `beforeUpdate` hook resolves, e.g. with `cancelQuery()`, and patches that leave no column to write still succeed [#214](https://github.com/ditojs/objection/issues/214)
 - Aggregates like `count()` and `sum()` select their aliases, not the columns they're passed, but still replace the select all of the query. `withGraphFetched()` and `withGraphJoined()` select the join columns that queries with aggregates like `count('* as n')` or `count('ownerId')` need, where 3.x left them out and returned empty relations. Grouped queries need to group by these columns too, or the database rejects them. In `withGraphJoined()`, the root query selects only its id columns next to aggregates instead of all its columns. See [migration](/release-notes/migration.md#aggregates-don-t-select-the-columns-they-re-passed) [#111](https://github.com/ditojs/objection/issues/111)
+- `upsertGraph()` throws a `NotFoundError` when a relate or update finds no row to write, e.g. a row moved below a new row while `ON DELETE CASCADE` deletes it with its old parent, or a row deleted by a concurrent query, instead of reporting success and returning the row [#214](https://github.com/ditojs/objection/issues/214)
+- Aggregates like `count()` and `sum()` select their aliases, not the columns they're passed, but still replace the select all of the query. `withGraphFetched()` and `withGraphJoined()` select the join columns that queries with aggregates like `count('* as n')` or `count('ownerId')` need, where 3.x left them out and returned empty relations. `withGraphFetched()` and relation queries use an aggregate, raw SQL or a subquery aliased as a join column, like `max('ownerId as ownerId')`, instead of selecting the column again. Grouped queries need to group by these columns too, or the database rejects them. In `withGraphJoined()`, the root query selects only its id columns next to aggregates instead of all its columns. See [migration](/release-notes/migration.md#aggregates-don-t-select-the-columns-they-re-passed) [#111](https://github.com/ditojs/objection/issues/111)
 - Relation expressions with anything after `*` or `^` in a relation path, like `'parent.^.pets'`, `'*.pets'` or `'children.*.pets'`, or with `*` and `^` in a list, like `'[children, *]'`, throw an error instead of silently ignoring the rest: in `withGraphFetched()`, `withGraphJoined()`, `allowGraph()`, `joinRelated()` and other relation expressions, and in the graph options of `insertGraph()` and `upsertGraph()`. See [migration](/release-notes/migration.md#misplaced-and-in-relation-expressions-throw) [#221](https://github.com/ditojs/objection/issues/221)
 - `update()`, `patch()` and `delete()` throw an error for `offset()` when the query is executed, instead of warning that knex leaves it out, and on MySQL, `delete()` throws for `orderBy()` with `limit()`, as knex leaves out the order. A many-to-many `patch()` / `update()` with join table extras on MySQL and SQL Server honours the offset and doesn't throw. See [migration](/release-notes/migration.md#offset-in-updates-and-deletes-throws) [#286](https://github.com/ditojs/objection/issues/286) [#235](https://github.com/ditojs/objection/issues/235)
 - JSON field expressions (`ref('column:path')`, `'column:path'` keys in `patch()`), `ref('column').castJson()` and the `whereJson*()` methods of objection throw an error on SQL Server and Oracle instead of warning, as they generate SQL that these databases don't understand, and so do `whereJsonSupersetOf()` and `whereJsonSubsetOf()` on SQLite. Other databases than PostgreSQL, MySQL and SQLite still get a warning. See [migration](/release-notes/migration.md#json-field-expressions-throw-on-sql-server-and-oracle) [#113](https://github.com/ditojs/objection/issues/113)

lib/queryBuilder/operations/eager/WhereInEagerOperation.js

@@ -8,6 +8,7 @@ import { ValidationErrorType } from '../../../model/ValidationError.js';
 import { createModifier } from '../../../utils/createModifier.js';
 import { RelationDoesNotExistError } from '../../../model/RelationDoesNotExistError.js';
 import { RelationOwner } from '../../../relations/RelationOwner.js';
+import { hasAliasSelection } from '../select/Selection.js';
 
 export class WhereInEagerOperation extends EagerOperation {
   constructor(name, opt) {
@@ -240,7 +241,11 @@ function findRelationPropsToSelect(builder, relationsToFetch) {
       const prop = ownerProp.props[c];
       const col = ownerProp.cols[c];
 
-      if (!builder.hasSelectionAs(fullCol, col) && selectionsToAdd.indexOf(fullCol) === -1) {
+      if (
+        !builder.hasSelectionAs(fullCol, col) &&
+        !hasAliasSelection(builder, col) &&
+        selectionsToAdd.indexOf(fullCol) === -1
+      ) {
         selectedProps.push(prop);
         selectionsToAdd.push(fullCol);
       }

lib/queryBuilder/operations/select/Selection.js

@@ -95,6 +95,18 @@ Object.defineProperties(Selection.prototype, {
   },
 });
 
+// A selection without a column, of an aggregate, raw SQL or a subquery, selects
+// the column that its alias names, e.g. `max('ownerId as ownerId')` the join
+// column that the related models are assigned by. Selecting the column once
+// more would replace its value.
+export function hasAliasSelection(builder, alias) {
+  return builder
+    .findAllSelections()
+    .some(
+      (selection) => selection.column === null && isSameIdentifier(builder, selection.alias, alias),
+    );
+}
+
 // Identifiers that only differ in a way that knex's `wrapIdentifier` mapping
 // (for example `knexSnakeCaseMappers()`) cancels out are the same identifier:
 // `dataExtension` and `data_extension` both end up as `data_extension`.

lib/relations/RelationFindOperation.js

@@ -1,6 +1,7 @@
 import { FindOperation } from '../queryBuilder/operations/FindOperation.js';
 import { RangeOperation } from '../queryBuilder/operations/RangeOperation.js';
 import { asArray } from '../utils/objectUtils.js';
+import { hasAliasSelection } from '../queryBuilder/operations/select/Selection.js';
 
 export class RelationFindOperation extends FindOperation {
   constructor(name, opt) {
@@ -193,7 +194,11 @@ export class RelationFindOperation extends FindOperation {
       const prop = relatedProp.props[c];
       const col = relatedProp.cols[c];
 
-      if (!builder.hasSelectionAs(fullCol, col) && addedSelects.indexOf(fullCol) === -1) {
+      if (
+        !builder.hasSelectionAs(fullCol, col) &&
+        !hasAliasSelection(builder, col) &&
+        addedSelects.indexOf(fullCol) === -1
+      ) {
         this.omitFromResult(builder, prop);
         addedSelects.push(fullCol);
       }

tests/integration/withGraph.js

@@ -1324,6 +1324,53 @@ export default (session) => {
         });
     });
 
+    it('should keep an aggregate aliased as the join column (#111)', async () => {
+      const models = await Model1.query()
+        .max('model1Id as model1Id')
+        .count('* as count')
+        .whereNotNull('model1Id')
+        .groupBy('model1Id')
+        .orderBy('model1Id')
+        .withGraphFetched('model1Relation1');
+
+      expect(
+        models.map((model) => [model.model1Id, Number(model.count), model.model1Relation1.id]),
+      ).toEqual([
+        [2, 1, 2],
+        [3, 1, 3],
+        [4, 1, 4],
+        [7, 1, 7],
+        [9, 1, 9],
+      ]);
+    });
+
+    it('should keep raw and subquery selections aliased as the join column (#111)', async () => {
+      const selections = [
+        raw('?? as ??', ['Model1.model1Id', 'model1Id']),
+        Model1.query()
+          .alias('m')
+          .select('m.model1Id')
+          .whereColumn('m.id', 'Model1.id')
+          .as('model1Id'),
+      ];
+
+      for (const selection of selections) {
+        const models = await Model1.query()
+          .select('id', selection)
+          .whereNotNull('model1Id')
+          .orderBy('id')
+          .withGraphFetched('model1Relation1');
+
+        expect(models.map((model) => [model.model1Id, model.model1Relation1.id])).toEqual([
+          [2, 2],
+          [3, 3],
+          [4, 4],
+          [7, 7],
+          [9, 9],
+        ]);
+      }
+    });
+
     it('should work with zero id', () => {
       return promiseMap(
         ['withGraphFetched', 'withGraphJoined'],

tests/unit/queryBuilder/QueryBuilder.js

@@ -5334,6 +5334,7 @@ describe('QueryBuilder', () => {
         'Person.id',
       );
       expectIdError(Person.query().select('*').max('pets.id as pets:id'), 'pets:id', 'pets.id');
+      expectIdError(Person.query().max('pets.id as id'), 'id', 'Person.id');
     });
 
     it('should let raws, subqueries and aggregates replace its columns', () => {