Keep visiting every operation when a callback removes or inserts operations (#2269, 3.x)

doc/release-notes/changelog.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## 3.9.3
+
+### Fixes
+
+- Queries that mix `withGraphJoined()` and `withGraphFetched()` and select all columns of the root table, e.g. with `select('*')` or `select('Person.*')`, load the fetched relations again. Since 3.8.0, `withGraphJoined()` replaces the `*` with the table's columns, which made the query skip the build step of `withGraphFetched()`, so its relations were missing from the result without an error [#2269](https://github.com/Vincit/objection.js/issues/2269)
+- An `onBuild()` hook that removes operations, e.g. with `clearSelect()`, no longer makes the query skip building the operation after them, e.g. the `where` of `findById()`, which made the query read, update or delete all rows
+
 ## 3.9.2
 
 ### Fixes

lib/queryBuilder/QueryBuilderOperationSupport.js

@@ -316,8 +316,16 @@ class QueryBuilderOperationSupport {
 
   forEachOperation(operationSelector, callback, match = true) {
     const selector = buildFunctionForOperationSelector(operationSelector);
+    let operations = this._operations;
+    let visited = null;
+
+    for (let i = 0; i < operations.length; ++i) {
+      const op = operations[i];
+
+      if (visited?.has(op)) {
+        continue;
+      }
 
-    for (const op of this._operations) {
       if (selector(op) === match && callback(op) === false) {
         break;
       }
@@ -331,6 +339,15 @@ class QueryBuilderOperationSupport {
       if (childRes === false) {
         break;
       }
+
+      // Callbacks may add, remove or replace operations, e.g. in `onBuild` hooks,
+      // which creates a new array, see `addOperation()`. Start over on it,
+      // skipping the visited operations (#2269).
+      if (this._operations !== operations) {
+        visited = new Set([...(visited ?? []), ...operations.slice(0, i + 1)]);
+        operations = this._operations;
+        i = -1;
+      }
     }
 
     return this;
@@ -402,16 +419,7 @@ class QueryBuilderOperationSupport {
     }
   }
 
-  addOperation(operation, args) {
-    const ret = this.addOperationUsingMethod('push', operation, args);
-    return ret;
-  }
-
-  addOperationToFront(operation, args) {
-    return this.addOperationUsingMethod('unshift', operation, args);
-  }
-
-  addOperationUsingMethod(arrayMethod, operation, args) {
+  addOperation(operation, args, toFront = false) {
     const shouldAdd = this.callOperationMethod(operation, 'onAdd', [this, args]);
 
     if (shouldAdd) {
@@ -423,22 +431,25 @@ class QueryBuilderOperationSupport {
         const { operation: parentOperation, hookName } = last(this._activeOperations);
         parentOperation.addChildOperation(hookName, operation);
       } else {
-        this._operations[arrayMethod](operation);
+        // Changes create a new array, which `forEachOperation()` notices.
+        this._operations = toFront
+          ? [operation, ...this._operations]
+          : [...this._operations, operation];
       }
     }
 
     return this;
   }
 
+  addOperationToFront(operation, args) {
+    return this.addOperation(operation, args, true);
+  }
+
   removeOperation(operation) {
     if (operation.parentOperation) {
       operation.parentOperation.removeChildOperation(operation);
     } else {
-      const index = this._operations.indexOf(operation);
-
-      if (index !== -1) {
-        this._operations.splice(index, 1);
-      }
+      this._operations = this._operations.filter((it) => it !== operation);
     }
 
     return this;
@@ -448,11 +459,7 @@ class QueryBuilderOperationSupport {
     if (operation.parentOperation) {
       operation.parentOperation.replaceChildOperation(operation, newOperation);
     } else {
-      const index = this._operations.indexOf(operation);
-
-      if (index !== -1) {
-        this._operations[index] = newOperation;
-      }
+      this._operations = this._operations.map((it) => (it === operation ? newOperation : it));
     }
 
     return this;

tests/integration/withGraph.js

@@ -3440,6 +3440,28 @@ module.exports = (session) => {
         });
       });
 
+      it('should fetch the fetched relations when selecting all columns of the root table (#2269)', () => {
+        const cases = orders.flatMap((order) => [
+          ['*', order],
+          ['Model1.*', order],
+        ]);
+
+        return promiseMap(cases, ([selection, order]) => {
+          const query = callInOrder(
+            Model1.query().select(selection).where('Model1.id', 1),
+            order,
+            'model1Relation1',
+            'model1Relation2',
+          );
+
+          return query.then((models) => {
+            expect(models).to.have.length(1);
+            expect(models[0].model1Relation1.id).to.equal(2);
+            expect(models[0].model1Relation2.map((it) => it.idCol).sort()).to.eql([1, 2]);
+          });
+        });
+      });
+
       it('should filter by the joined relation while the fetched relation is complete', () => {
         return promiseMap(orders, (order) => {
           let query = Model1.query().where('model1Relation1.model1Prop1', 'hello 2');

tests/unit/queryBuilder/QueryBuilder.js

@@ -368,6 +368,17 @@ describe('QueryBuilder', () => {
     });
   });
 
+  it('should build the operations after the ones that an onBuild hook removes (#2269)', () => {
+    expect(
+      QueryBuilder.forClass(TestModel)
+        .select('a')
+        .onBuild((builder) => builder.clearSelect().select('b'))
+        .findById(1)
+        .toKnexQuery()
+        .toString(),
+    ).to.equal('select "b" from "Model" where "Model"."id" = 1');
+  });
+
   it('should have knex query builder methods', () => {
     // Doesn't test all the methods. Just enough to make sure the method calls are correctly
     // passed to the knex query builder.