diff --git a/doc/release-notes/changelog.md b/doc/release-notes/changelog.md index bc15607a2..f95cc1294 100644 --- a/doc/release-notes/changelog.md +++ b/doc/release-notes/changelog.md @@ -1,5 +1,12 @@ # Changelog +## 4.1.1 + +### Fixes + +- `patchAndFetchById()`, `updateAndFetchById()`, `patchAndFetch()` and `updateAndFetch()` with an empty patch return the unchanged model if the row matches, and `undefined` only if it doesn't. Since 1.5.0 they always returned `undefined` [#517](https://github.com/ditojs/objection/issues/517) +- `asFindQuery()` in static after hooks runs the `onBuild` hooks of the query's context once. Since 2.0.0 it ran them a second time on the built write query, e.g. adding a join twice + ## 4.1.0 ### Changes diff --git a/lib/queryBuilder/QueryBuilder.js b/lib/queryBuilder/QueryBuilder.js index 0ea913f26..240d9ed9e 100644 --- a/lib/queryBuilder/QueryBuilder.js +++ b/lib/queryBuilder/QueryBuilder.js @@ -76,6 +76,7 @@ export class QueryBuilder extends QueryBuilderBase { self._findOperationOptions = modelClass.defaultFindOptions; self._relatedQueryFor = null; self._internalSelections = new Set(); + self._contextOperations = null; self._findOperationFactory = findOperationFactory; self._insertOperationFactory = insertOperationFactory; @@ -481,6 +482,45 @@ export class QueryBuilder extends QueryBuilderBase { return this; } + // Turn a write query into a find query with the same filters, e.g. for the + // `asFindQuery()` of static hooks. + toFindQuery() { + const findQuery = this.clone(); + // The clone has the operations of this query in the same order: + findQuery._operations = findQuery._operations.filter( + (_, index) => !this._contextOperations?.has(this._operations[index]), + ); + const operationsToReplace = []; + const operationsToRemove = []; + + findQuery.forEachOperation( + (op) => op.hasToFindOperation(), + (op) => { + const findOp = op.toFindOperation(findQuery); + + if (!findOp) { + operationsToRemove.push(op); + } else { + operationsToReplace.push({ op, findOp }); + } + }, + ); + + for (const op of operationsToRemove) { + findQuery.removeOperation(op); + } + + for (const { op, findOp } of operationsToReplace) { + findQuery.replaceOperation(op, findOp); + } + + // The `runAfter()` callbacks of the write (including the ones installed by + // `throwIfNotFound()` and `traverse()`) are meant for its result, not for + // the result of the find query. The same goes for the value it was resolved + // with, e.g. the 0 of an empty update, and for its fetched graph: + return findQuery.clear('runAfter').clearResolve().clearWithGraphFetched(); + } + clearWithGraph() { this.clear(EagerOperation); this._graphAlgorithm = null; @@ -1642,8 +1682,13 @@ function moveEagerOperationToEnd(builder) { } function callOnBuildHooks(builder) { + // Remember the operations that the `onBuild` hooks of the context add, for + // `toFindQuery()` to leave out, as they run again when it's built: + // `withSchema()` adds its operation to the front, so compare by identity: + const operations = new Set(builder._operations); callOnBuildFuncs(builder, builder.context().onBuild); callOnBuildFuncs(builder, builder.internalContext().onBuild); + builder._contextOperations = new Set(builder._operations.filter((op) => !operations.has(op))); builder.executeOnBuild(); diff --git a/lib/queryBuilder/QueryBuilderOperationSupport.js b/lib/queryBuilder/QueryBuilderOperationSupport.js index 0023d5057..c6f2dede2 100644 --- a/lib/queryBuilder/QueryBuilderOperationSupport.js +++ b/lib/queryBuilder/QueryBuilderOperationSupport.js @@ -239,35 +239,6 @@ export class QueryBuilderOperationSupport { return this; } - toFindQuery() { - const findQuery = this.clone(); - const operationsToReplace = []; - const operationsToRemove = []; - - findQuery.forEachOperation( - (op) => op.hasToFindOperation(), - (op) => { - const findOp = op.toFindOperation(findQuery); - - if (!findOp) { - operationsToRemove.push(op); - } else { - operationsToReplace.push({ op, findOp }); - } - }, - ); - - for (const op of operationsToRemove) { - findQuery.removeOperation(op); - } - - for (const { op, findOp } of operationsToReplace) { - findQuery.replaceOperation(op, findOp); - } - - return findQuery; - } - clearSelect() { return this.clear(SelectSelector); } diff --git a/lib/queryBuilder/StaticHookArguments.js b/lib/queryBuilder/StaticHookArguments.js index b84e289fc..a2f497b19 100644 --- a/lib/queryBuilder/StaticHookArguments.js +++ b/lib/queryBuilder/StaticHookArguments.js @@ -23,17 +23,9 @@ export class StaticHookArguments { get asFindQuery() { return () => { - // The `runAfter()` callbacks of the query being executed (including the - // ones installed by `throwIfNotFound()` and `traverse()`) are meant for - // its result, not for the result of this find query. The same goes for - // the value it was resolved with, e.g. the 0 of an update that only - // changes the join rows of a many-to-many relation. The query resolves to - // an array also for single-result writes, unless the hook adds `first()`: - return this[BUILDER_SYMBOL].toFindQuery() - .clear('runAfter') - .clearResolve() - .clearWithGraphFetched() - .addOperation(new AsArrayOperation('asArray'), []); + // The query resolves to an array also for single-result writes, unless + // the hook adds `first()`: + return this[BUILDER_SYMBOL].toFindQuery().addOperation(new AsArrayOperation('asArray'), []); }; } diff --git a/lib/queryBuilder/operations/UpdateAndFetchOperation.js b/lib/queryBuilder/operations/UpdateAndFetchOperation.js index 159d4eddb..ff7a2098f 100644 --- a/lib/queryBuilder/operations/UpdateAndFetchOperation.js +++ b/lib/queryBuilder/operations/UpdateAndFetchOperation.js @@ -46,22 +46,26 @@ export class UpdateAndFetchOperation extends DelegateOperation { } async onAfter2(builder, numUpdated) { - if (numUpdated == 0) { + if (numUpdated == 0 && !this.delegate.hasNothingToUpdate) { // If nothing was updated, we should fetch nothing. await super.onAfter2(builder, numUpdated); return undefined; } - const fetched = await builder - .emptyInstance() - .childQueryOf(builder) - .copyFrom(builder, (op) => !!this.selectOperations?.has(op)) - .modify((builder) => { - if (!this.skipIdWhere) { - builder.findById(this.id); - } - }) - .castTo(builder.resultModelClass()); + // An empty update writes nothing, so fetch the row only if it matches the + // filters of the update: + const fetchQuery = this.delegate.hasNothingToUpdate + ? builder.toFindQuery().first() + : builder + .emptyInstance() + .childQueryOf(builder) + .copyFrom(builder, (op) => !!this.selectOperations?.has(op)) + .modify((builder) => { + if (!this.skipIdWhere) { + builder.findById(this.id); + } + }); + const fetched = await fetchQuery.castTo(builder.resultModelClass()); if (fetched) { if (!fetched.$isObjectionModel) { diff --git a/testUtils/TestSession.js b/testUtils/TestSession.js index 230e84f05..fcf97b5a2 100644 --- a/testUtils/TestSession.js +++ b/testUtils/TestSession.js @@ -418,6 +418,17 @@ class TestSession { return knexUtils.isMsSql(this.knex); } + // The schema that the test tables are created in, e.g. for `withSchema()`: + defaultSchema() { + return this.isPostgres() + ? 'public' + : this.isMySql() + ? this.knex.client.config.connection.database + : this.isMsSql() + ? 'dbo' + : 'main'; + } + // The text that a number written into a text column reads back as: // better-sqlite3 binds numbers as REAL. numberAsText(number) { diff --git a/tests/integration/patch.js b/tests/integration/patch.js index 7a9ff759a..ae5541ad0 100644 --- a/tests/integration/patch.js +++ b/tests/integration/patch.js @@ -642,6 +642,36 @@ export default (session) => { expectPartEql(rows[2], { id: 3, model1Prop1: 'hello 3' }); }); }); + + it('should fetch the unchanged model with an empty patch', async () => { + const fetchedModel = await Model1.query().patchAndFetchById(2, {}); + expect(fetchedModel).toMatchObject({ id: 2, model1Prop1: 'hello 2' }); + }); + + it('should run onBuild hooks of the context once with an empty patch', async () => { + const fetchedModel = await Model1.query() + .context({ + onBuild: (query) => query.leftJoin('Model1 as parent', 'parent.id', 'Model1.model1Id'), + }) + .patchAndFetchById(2, {}); + + expect(fetchedModel).toMatchObject({ id: 2, model1Prop1: 'hello 2' }); + }); + + it('should keep the filters of an empty patch with withSchema()', async () => { + const query = () => Model1.query().withSchema(session.defaultSchema()); + + expect(await query().patchAndFetchById(2, {})).toMatchObject({ + id: 2, + model1Prop1: 'hello 2', + }); + expect(await query().patchAndFetchById(2, {}).where('id', -1)).toBeUndefined(); + }); + + it('should fetch nothing with an empty patch if no row matches', async () => { + expect(await Model1.query().patchAndFetchById(4, {})).toBeUndefined(); + expect(await Model1.query().patchAndFetchById(2, {}).where('id', -1)).toBeUndefined(); + }); }); describe('.$query().patch()', () => { @@ -902,6 +932,14 @@ export default (session) => { ]); } }); + + it('should fetch the unchanged model with an empty patch', async () => { + const model = ModelOne.fromJson({ id: 1 }); + const updated = await model.$query().patchAndFetch({}); + + expect(updated).toMatchObject({ id: 1, model1Prop1: 'hello 1', model1Prop2: null }); + expectPartEql(model, { id: 1, model1Prop1: 'hello 1', model1Prop2: null }); + }); }); describe('.$relatedQuery().patch()', () => { @@ -1264,6 +1302,22 @@ export default (session) => { }); }); + it('should fetch a related object with an empty patch using patchAndFetchById', async () => { + const parent = await Model1.query().findById(1); + + expect( + await parent.$relatedQuery('model1Relation3').patchAndFetchById(4, {}), + ).toMatchObject({ + idCol: 4, + model2Prop1: 'foo 2', + extra1: 'extra 12', + }); + // `idCol` 1 isn't related to the parent: + expect( + await parent.$relatedQuery('model1Relation3').patchAndFetchById(1, {}), + ).toBeUndefined(); + }); + it('should patch a related object with extras using patchAndFetchById', () => { return Model1.query() .findById(1) diff --git a/tests/integration/staticHooks.js b/tests/integration/staticHooks.js index 3217cef25..e50c44432 100644 --- a/tests/integration/staticHooks.js +++ b/tests/integration/staticHooks.js @@ -1764,6 +1764,36 @@ export default (session) => { expect(Movie.afterUpdate.calls.length).toBe(1); }); }); + + it('`asFindQuery` should run the onBuild hooks of the context once', async () => { + let found = null; + Movie.afterUpdate = createHookSpy(async ({ asFindQuery }) => { + found = await asFindQuery().select('movies.name'); + }); + + await Movie.query() + .context({ + onBuild: (query) => query.leftJoin('actorsMovies', 'actorsMovies.id', 'movies.id'), + }) + .patch({ name: 'Hungergames 2' }) + .where('movies.name', 'like', 'Hunger%'); + + expect(found).toEqual([{ name: 'Hungergames 2' }]); + }); + + it('`asFindQuery` should keep the filters of the query with withSchema()', async () => { + let found = null; + Movie.afterUpdate = createHookSpy(async ({ asFindQuery }) => { + found = await asFindQuery().select('movies.name'); + }); + + await Movie.query() + .withSchema(session.defaultSchema()) + .patch({ name: 'Hungergames 2' }) + .where('movies.name', 'like', 'Hunger%'); + + expect(found).toEqual([{ name: 'Hungergames 2' }]); + }); }); describe('$query', () => { @@ -2531,6 +2561,27 @@ export default (session) => { expect(Movie.afterDelete.calls.length).toBe(1); }); }); + + it('`asFindQuery` should run the onBuild hooks of the context once', async () => { + // SQLite can't delete with joins. + if (session.isSqlite()) { + return; + } + + let found = null; + Movie.afterDelete = createHookSpy(async ({ asFindQuery }) => { + found = await asFindQuery().select('movies.name'); + }); + + await Movie.query() + .context({ + onBuild: (query) => query.leftJoin('movies as other', 'other.id', 'movies.id'), + }) + .delete() + .where('movies.name', 'Hungergames'); + + expect(found).toEqual([]); + }); }); describe('$query', () => { @@ -3144,6 +3195,26 @@ export default (session) => { expect(Movie.afterInsert.calls.length).toBe(1); }); }); + + it('`asFindQuery` should run the onBuild hooks of the context once', async () => { + let found = null; + Movie.afterInsert = createHookSpy(async ({ asFindQuery }) => { + found = await asFindQuery().select('movies.name').orderBy('movies.name'); + }); + + await Movie.query() + .context({ + onBuild: (query) => query.leftJoin('actorsMovies', 'actorsMovies.id', 'movies.id'), + }) + .insert({ name: 'Inserted' }); + + expect(found.map(({ name }) => name)).toEqual([ + 'A Star is Born', + 'Hungergames', + 'Inserted', + 'Silver Linings Playbook', + ]); + }); }); describe('$query', () => { diff --git a/tests/integration/update.js b/tests/integration/update.js index 23e464b6b..7f4d07366 100644 --- a/tests/integration/update.js +++ b/tests/integration/update.js @@ -435,6 +435,12 @@ export default (session) => { expect(fetchedModel).not.toHaveProperty('model1Prop2'); expect(fetchedModel).not.toHaveProperty('model1Id'); }); + + it('should fetch the unchanged model with an empty update', async () => { + const fetchedModel = await Model1.query().updateAndFetchById(2, {}); + expect(fetchedModel).toMatchObject({ id: 2, model1Prop1: 'hello 2' }); + expect(await Model1.query().updateAndFetchById(4, {})).toBeUndefined(); + }); }); describe('.$query().update()', () => {