From 1ecdc3967cfc2a0c07bfeb91d9bebcf964c5b05c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ju=CC=88rg=20Lehni?= <juerg@scratchdisk.com>
Date: Mon, 12 Feb 2024 15:31:21 +0100
Subject: [PATCH] Don't call onError() with internal exception (#2603)

---
 lib/queryBuilder/InternalOptions.js           |  2 ++
 lib/queryBuilder/QueryBuilder.js              | 31 ++++++++++++++++---
 .../graph/patch/GraphPatchAction.js           | 29 ++++++-----------
 tests/integration/upsertGraph.js              | 28 +++++++++++++++++
 4 files changed, 66 insertions(+), 24 deletions(-)

diff --git a/lib/queryBuilder/InternalOptions.js b/lib/queryBuilder/InternalOptions.js
index ac9f16cef..1083d3de8 100644
--- a/lib/queryBuilder/InternalOptions.js
+++ b/lib/queryBuilder/InternalOptions.js
@@ -4,6 +4,7 @@ class InternalOptions {
   constructor() {
     this.skipUndefined = false;
     this.keepImplicitJoinProps = false;
+    this.returnImmediatelyValue = undefined;
     this.isInternalQuery = false;
     this.debug = false;
     this.schema = undefined;
@@ -14,6 +15,7 @@ class InternalOptions {
 
     copy.skipUndefined = this.skipUndefined;
     copy.keepImplicitJoinProps = this.keepImplicitJoinProps;
+    copy.returnImmediatelyValue = this.returnImmediatelyValue;
     copy.isInternalQuery = this.isInternalQuery;
     copy.debug = this.debug;
     copy.schema = this.schema;
diff --git a/lib/queryBuilder/QueryBuilder.js b/lib/queryBuilder/QueryBuilder.js
index 11df1cf9e..a5e0e4874 100644
--- a/lib/queryBuilder/QueryBuilder.js
+++ b/lib/queryBuilder/QueryBuilder.js
@@ -1119,7 +1119,24 @@ function afterExecute(builder, result) {
   return promise;
 }
 
+class ReturnImmediatelyException {
+  constructor(value) {
+    this.value = value;
+  }
+}
+
+function handleReturnImmediatelyValue(builder) {
+  const { returnImmediatelyValue } = builder.internalOptions();
+  if (returnImmediatelyValue !== undefined) {
+    throw new ReturnImmediatelyException(returnImmediatelyValue);
+  }
+}
+
 function handleExecuteError(builder, err) {
+  if (err instanceof ReturnImmediatelyException) {
+    return Promise.resolve(err.value);
+  }
+
   let promise = Promise.reject(wrapError(err));
 
   builder.forEachOperation(true, (op) => {
@@ -1139,9 +1156,11 @@ function chainOperationHooks(promise, builder, hookName) {
 
     builder.forEachOperation(true, (op) => {
       if (op.hasHook(hookName)) {
-        promise = promise.then((result) =>
-          builder.callAsyncOperationMethod(op, hookName, [builder, result]),
-        );
+        promise = promise.then((result) => {
+          const res = builder.callAsyncOperationMethod(op, hookName, [builder, result]);
+          handleReturnImmediatelyValue(builder);
+          return res;
+        });
       }
     });
 
@@ -1261,7 +1280,11 @@ async function chainHooks(promise, builder, func) {
     let promise = Promise.resolve(result);
 
     if (isFunction(func)) {
-      promise = promise.then((result) => func.call(builder, result, builder));
+      promise = promise.then((result) => {
+        const res = func.call(builder, result, builder);
+        handleReturnImmediatelyValue(builder);
+        return res;
+      });
     } else if (Array.isArray(func)) {
       func.forEach((func) => {
         promise = chainHooks(promise, builder, func);
diff --git a/lib/queryBuilder/graph/patch/GraphPatchAction.js b/lib/queryBuilder/graph/patch/GraphPatchAction.js
index a52afa11e..34a8a9298 100644
--- a/lib/queryBuilder/graph/patch/GraphPatchAction.js
+++ b/lib/queryBuilder/graph/patch/GraphPatchAction.js
@@ -82,24 +82,15 @@ class GraphPatchAction extends GraphAction {
       .copyFrom(builder, GraphAction.ReturningAllSelector);
 
     if (hasBeforeUpdate) {
-      updateBuilder
-        .context({
-          runBefore: () => {
-            // Call handleUpdate in the runBefore hook which runs after the
-            // $beforeUpdate hook, allowing it to modify the object before the
-            // updated properties are determined. See issue #2233.
-            if (hasBeforeUpdate && !handleUpdate()) {
-              throw new ReturnNullException();
-            }
-          },
-        })
-        .onError((error) => {
-          if (error instanceof ReturnNullException) {
-            return null;
-          } else {
-            return Promise.reject(error);
-          }
-        });
+      updateBuilder.internalContext().runBefore.push((result, builder) => {
+        // Call handleUpdate in the runBefore hook which runs after the
+        // $beforeUpdate hook, allowing it to modify the object before the
+        // updated properties are determined. See issue #2233.
+        if (hasBeforeUpdate && !handleUpdate()) {
+          builder.internalOptions().returnImmediatelyValue = null;
+        }
+        return result;
+      });
     }
 
     if (shouldPatch) {
@@ -235,8 +226,6 @@ class GraphPatchAction extends GraphAction {
   }
 }
 
-class ReturnNullException {}
-
 function childQueryOptions() {
   return {
     fork: true,
diff --git a/tests/integration/upsertGraph.js b/tests/integration/upsertGraph.js
index ea46e62d0..cbde48cf9 100644
--- a/tests/integration/upsertGraph.js
+++ b/tests/integration/upsertGraph.js
@@ -4131,6 +4131,34 @@ module.exports = (session) => {
         });
       });
 
+      describe('should not call onError() with internal exception (#2603)', () => {
+        let query;
+
+        before(() => {
+          query = Model1.query;
+        });
+
+        after(() => {
+          Model1.query = query;
+        });
+
+        it('should not call onError() with internal exception', async () => {
+          const upsert = { id: 2 };
+
+          let error = null;
+          Model1.query = function (trx) {
+            return query.call(this, trx).onError((err) => {
+              error = err;
+            });
+          };
+
+          await transaction(session.knex, (trx) =>
+            Model1.query(trx).upsertGraph(upsert, { fetchStrategy }),
+          );
+          expect(error).to.equal(null);
+        });
+      });
+
       if (session.isPostgres()) {
         describe('returning', () => {
           it('should propagate returning(*) to all update an insert operations', () => {