Throw when a patchAndFetch() or updateAndFetch() query is executed a second time

doc/api/query-builder/other-methods.md

@@ -803,6 +803,8 @@ const promise = queryBuilder.execute();
 
 Executes the query and returns a Promise.
 
+A query can be executed again, e.g. by awaiting it twice, except for the `*AndFetch()` writes: they merge the fetched row into their patch, which a second run would write back, so it throws. Create a new query for each run instead. A run that fails before the fetch, e.g. on a deadlock, can be retried. Concurrent runs of the same query aren't detected.
+
 ##### Return value
 
 | Type      | Description                                                |

doc/release-notes/changelog.md

@@ -1,6 +1,10 @@
 # Changelog
 
-## 4.1.1
+## 4.2.0
+
+### Changes
+
+- `patchAndFetch()`, `patchAndFetchById()`, `updateAndFetch()` and `updateAndFetchById()` throw when the same query is executed a second time, e.g. awaited twice. Since at least 1.0.0, the second run wrote back the row that the first one fetched, undoing other writes in between [#519](https://github.com/ditojs/objection/issues/519)
 
 ### Fixes
 

lib/queryBuilder/operations/UpdateAndFetchOperation.js

@@ -13,6 +13,8 @@ export class UpdateAndFetchOperation extends DelegateOperation {
     this.id = null;
     this.skipIdWhere = false;
     this.selectOperations = null;
+    // Shared with the clones that each execution runs:
+    this.state = { isMerged: false };
   }
 
   get model() {
@@ -24,6 +26,19 @@ export class UpdateAndFetchOperation extends DelegateOperation {
     return this.delegate.onAdd(builder, args.slice(1));
   }
 
+  onBefore1(builder, result) {
+    // A second run would write back the row that the first one merged into the
+    // patch:
+    if (this.state.isMerged) {
+      throw new Error(
+        `Unable to execute ${this.name}() twice, as the fetched row was merged into ` +
+          'its patch. Create a new query for each run.',
+      );
+    }
+
+    return super.onBefore1(builder, result);
+  }
+
   onBefore3(builder, result) {
     // Selects have no effect on the update query, so remember them here to
     // apply them to the fetch query. Collect them before the query is built,
@@ -76,6 +91,7 @@ export class UpdateAndFetchOperation extends DelegateOperation {
       }
 
       this.model.$set(fetched);
+      this.state.isMerged = true;
       // A custom `select` may not include the id. The row was fetched by
       // id, so make sure the result still has it.
       if (!this.model.$hasId()) {
@@ -102,6 +118,7 @@ export class UpdateAndFetchOperation extends DelegateOperation {
 
     clone.id = this.id;
     clone.skipIdWhere = this.skipIdWhere;
+    clone.state = this.state;
 
     return clone;
   }

tests/integration/misc/executeWritesOnce.js

@@ -0,0 +1,72 @@
+import { describe, it, expect, beforeEach } from 'vitest';
+
+export default (session) => {
+  describe('executing an *AndFetch() query twice', () => {
+    const Model1 = session.models.Model1;
+
+    beforeEach(() => {
+      return session.populate([{ id: 1, model1Prop1: 'hello 1' }]);
+    });
+
+    it('should throw for patchAndFetchById()', async () => {
+      const query = Model1.query().patchAndFetchById(1, { model1Prop2: 2 });
+      await query;
+      await Model1.query().patch({ model1Prop1: 'changed' }).findById(1);
+      // A second run would write back the row that the first one fetched:
+      await expect(query).rejects.toThrow(
+        'Unable to execute patchAndFetch() twice, as the fetched row was merged into ' +
+          'its patch. Create a new query for each run.',
+      );
+      expect(await Model1.query().findById(1)).toMatchObject({ model1Prop1: 'changed' });
+    });
+
+    it('should throw for $query().patchAndFetch()', async () => {
+      const model = await Model1.query().findById(1);
+      const query = model.$query().patchAndFetch({ model1Prop2: 2 });
+      await query;
+      await expect(query).rejects.toThrow('Create a new query for each run.');
+    });
+
+    it('should run again after a run that failed before the fetch', async () => {
+      let shouldFail = true;
+      const query = Model1.query()
+        .context({
+          runBefore() {
+            if (shouldFail) {
+              throw new Error('deadlock');
+            }
+          },
+        })
+        .patchAndFetchById(1, { model1Prop2: 2 });
+
+      await expect(query).rejects.toThrow('deadlock');
+      shouldFail = false;
+      expect(await query).toMatchObject({ id: 1, model1Prop1: 'hello 1', model1Prop2: 2 });
+    });
+
+    it('should run patch(), delete() and finds again', async () => {
+      const patch = Model1.query().patch({ model1Prop2: 2 }).findById(1);
+      expect(await patch).toBe(1);
+      expect(await patch).toBe(1);
+
+      const find = Model1.query().findById(1);
+      expect(await find).toMatchObject({ id: 1 });
+      expect(await find).toMatchObject({ id: 1 });
+
+      const deleteQuery = Model1.query().delete().where('model1Prop1', 'nobody');
+      expect(await deleteQuery).toBe(0);
+      expect(await deleteQuery).toBe(0);
+    });
+
+    it.skipIf(session.isMsSql())('should run an insert with onConflict() again', async () => {
+      const query = Model1.query()
+        .insert({ id: 2, model1Prop1: 'inserted' })
+        .onConflict('id')
+        .merge();
+
+      await query;
+      await query;
+      expect(await Model1.query().findById(2)).toMatchObject({ model1Prop1: 'inserted' });
+    });
+  });
+};