Fetch the unchanged model in patchAndFetch() and updateAndFetch() with an empty patch

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

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();
 

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);
   }

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'), []);
     };
   }
 

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) {

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) {

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)

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', () => {

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()', () => {