diff --git a/doc/release-notes/changelog.md b/doc/release-notes/changelog.md index 9293cef33..3f27ba756 100644 --- a/doc/release-notes/changelog.md +++ b/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) diff --git a/lib/queryBuilder/operations/eager/WhereInEagerOperation.js b/lib/queryBuilder/operations/eager/WhereInEagerOperation.js index a923fc058..e72c4c0b0 100644 --- a/lib/queryBuilder/operations/eager/WhereInEagerOperation.js +++ b/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); } diff --git a/lib/queryBuilder/operations/select/Selection.js b/lib/queryBuilder/operations/select/Selection.js index f821b3f03..b950c6a94 100644 --- a/lib/queryBuilder/operations/select/Selection.js +++ b/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`. diff --git a/lib/relations/RelationFindOperation.js b/lib/relations/RelationFindOperation.js index c2d015183..7636b3906 100644 --- a/lib/relations/RelationFindOperation.js +++ b/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); } diff --git a/tests/integration/withGraph.js b/tests/integration/withGraph.js index 7cb848bd9..74664a222 100644 --- a/tests/integration/withGraph.js +++ b/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'], diff --git a/tests/unit/queryBuilder/QueryBuilder.js b/tests/unit/queryBuilder/QueryBuilder.js index 47a121670..0dfa220b3 100644 --- a/tests/unit/queryBuilder/QueryBuilder.js +++ b/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', () => {