Run the internal reads of BelongsToManyRelation writes without find hooks (#2667)

doc/release-notes/changelog.md

@@ -16,6 +16,7 @@
 - The `result` of the static `after*` hooks is what the query returns: a model with `first()`, `findById()`, `insert()` of a single model, `patchAndFetchById()` and queries for to-one relations, `undefined` if nothing is found, a number for updates and deletes, and arrays otherwise, instead of always an array. The hooks run as the last step of the query, after `first()` and the fetches of `insertAndFetch()` and `patchAndFetch()`, whose `afterInsert` and `afterUpdate` get the fetched model. In `before*` hooks, `result` is `undefined`. `insertAndFetch()` and `patchAndFetch()` throw if the `afterFind` of their fetch returns something else than the fetched models. See [migration](/release-notes/migration.md#static-hooks-get-the-result-in-the-shape-that-the-query-returns) [#66](https://github.com/ditojs/objection/issues/66) [#210](https://github.com/ditojs/objection/issues/210)
 - `runAfter()` callbacks, also the ones of `context()`, `throwIfNotFound()` and `traverse()`, run after the static `after*` hooks and get the result in the shape that the query returns, also when they're added before `first()`, and before `page()` and `range()` wrap it in `{ results, total }`. See [migration](/release-notes/migration.md#static-hooks-get-the-result-in-the-shape-that-the-query-returns) [#66](https://github.com/ditojs/objection/issues/66)
 - The outer queries of `upsertGraph()` and `upsertGraphAndFetch()` no longer call the static `beforeFind` and `afterFind` hooks of the root model class with the input models. The queries that run for the graph call the hooks. See [migration](/release-notes/migration.md#static-hooks-get-the-result-in-the-shape-that-the-query-returns) [#66](https://github.com/ditojs/objection/issues/66)
+- `fetchGraph()` / `$fetchGraph()` and the relations that `skipFetched` skips no longer call the static `beforeFind` hook of the models they already have, as they didn't call `afterFind`: the find hooks are only called for queries that run. See [migration](/release-notes/migration.md#static-hooks-get-the-result-in-the-shape-that-the-query-returns) [#66](https://github.com/ditojs/objection/issues/66)
 - `withGraphFetched()` assigns what a relation query returns to the owners, after the static `afterFind` hook of the related model class and the `runAfter()` callbacks of the relation query, so the related models that they return are assigned, filtered or reordered. Returning other objects than these models throws an error, where 3.x assigned the related models before both and ignored their return values. The same applies to `$relatedQuery()` with `relatedFindQueryMutates`. See [migration](/release-notes/migration.md#withgraphfetched-assigns-what-the-relation-query-returns) [#1963](https://github.com/Vincit/objection.js/issues/1963)
 - `withGraphJoined()` calls the static `afterFind` hooks of the joined related model classes, bottom-up after the query, with the related models of all owners, the `relation` and the owners as `items`, and assigns what they return like `withGraphFetched()` does with the same models, filtered or reordered. `asFindQuery()` and `cancelQuery()` throw in these hooks. See [migration](/release-notes/migration.md#withgraphfetched-assigns-what-the-relation-query-returns) [#1963](https://github.com/Vincit/objection.js/issues/1963)
 - `StaticHookArguments<M, R>` declares `result: R` with `R = undefined`, and the static hooks of `Model` declare the shape of `result` that they get. See [migration](/release-notes/migration.md#static-hooks-get-the-result-in-the-shape-that-the-query-returns) [#66](https://github.com/ditojs/objection/issues/66)
@@ -61,6 +62,8 @@
 
 - `insert()` of several models works on SQLite (3.35 or newer, up to 500 rows) through `returning`, and on all databases when all models have their ids or the model has no `idColumn`, e.g. join rows
 - `BelongsToManyRelation`, a relation where the owner holds an array of the related models' keys, e.g. `posts.tagIds = [1, 4, 7]` for `post.tags`, in a JSON column on all databases or a native array column on PostgreSQL. It supports finding the related models with `$relatedQuery()`, `relatedQuery()`, `withGraphFetched()`, `withGraphJoined()` and `joinRelated()`. `insert()`, `delete()`, `relate()` and `unrelate()` through the relation rewrite the arrays, `insertGraph()` and `upsertGraph()` write them, and `$setRelated()` and `$appendRelated()` keep them in sync. See [relations](/guide/relations.md) [#2667](https://github.com/Vincit/objection.js/issues/2667)
+- `insert()` of several models works on SQLite (3.35 or newer) through `returning`, and on all databases when all models have their ids, e.g. join rows. `insertGraph()` and `upsertGraph()` insert in batches there too
+- `BelongsToManyRelation`, a relation where the owner holds an array of the related models' keys, e.g. `posts.tagIds = [1, 4, 7]` for `post.tags`, in a JSON column on all databases or a native array column on PostgreSQL. It supports finding the related models with `$relatedQuery()`, `relatedQuery()`, `withGraphFetched()`, `withGraphJoined()` and `joinRelated()`. `insert()`, `delete()`, `relate()` and `unrelate()` through the relation rewrite the arrays, `insertGraph()` and `upsertGraph()` write them, and `$setRelated()` and `$appendRelated()` keep them in sync. The reads of the keys that these writes run call no find hooks. See [relations](/guide/relations.md) [#2667](https://github.com/Vincit/objection.js/issues/2667)
 - `Model.jsonColumnFormat` says how the database returns JSON columns: `'auto'` (default) leaves them as they are on PostgreSQL drivers and parses JSON text on others, `'text'` parses them strictly and throws for invalid JSON, `'parsed'` never parses them [#205](https://github.com/ditojs/objection/issues/205)
 - JSON field expressions work on MySQL and SQLite: in `ref()`, e.g. `ref('column:a.b').castInt()` in `select()`, `where()`, `orderBy()`, joins and relation mappings, as keys in `patch()` and `merge()`, e.g. `patch({ 'column:a.b': 1 })`, and in the `whereJson*()` methods, except for `whereJsonSupersetOf()` and `whereJsonSubsetOf()` on SQLite. objection writes the SQL of each database, `JSON_EXTRACT()` and `JSON_SET()` on MySQL and `json_extract()` and `json_set()` on SQLite, and casts to their types. `ref().castJson()` works on both too. See [JSON queries](/recipes/json-queries.md) [#276](https://github.com/Vincit/objection.js/issues/276)
 - `asArray()` is exported, to turn the `result` of static hooks into an array [#66](https://github.com/ditojs/objection/issues/66)

doc/release-notes/migration.md

@@ -209,7 +209,7 @@ class Person extends Model {
 
 The static `after*` hooks run as the last step of the query, after `first()`, `page()`'s models and the fetches of the `*AndFetch()` methods, but before the `runAfter()` callbacks, including the ones in `context()`, `throwIfNotFound()` and `traverse()`. These callbacks now get the result in the shape that the query returns and after the hooks, e.g. the model with `first()` also if `runAfter()` is called before `first()`, and they run before `page()` and `range()` wrap the result in `{ results, total }`.
 
-The outer queries of `upsertGraph()` and `upsertGraphAndFetch()` no longer call the static `beforeFind` and `afterFind` hooks of the root model class with the input models. The queries that run for the models of the graph and their relations call the hooks as before.
+The outer queries of `upsertGraph()` and `upsertGraphAndFetch()` no longer call the static `beforeFind` and `afterFind` hooks of the root model class with the input models. The queries that run for the models of the graph and their relations call the hooks as before. Likewise, `fetchGraph()` / `$fetchGraph()` and the relations that `skipFetched` skips no longer call the static `beforeFind` hook of the models they already have, as they didn't call `afterFind`: the find hooks are only called for queries that run.
 
 TypeScript: `StaticHookArguments<M, R>` declares `result: R`, with `R = undefined` by default, and the static hooks of `Model` declare the shapes that they get, e.g. `StaticHookArguments<any, Model | Model[] | undefined>` for `afterFind`. Overrides that destructure `result` from `StaticHookArguments<Person>` need the type of the result as second type argument, e.g. `StaticHookArguments<Person, Person | Person[] | undefined>`.
 

lib/queryBuilder/operations/FindOperation.js

@@ -5,7 +5,7 @@ import { isObject } from '../../utils/objectUtils.js';
 
 export class FindOperation extends QueryBuilderOperation {
   onBefore2(builder, result) {
-    if (isGraphQuery(builder)) {
+    if (isGraphQuery(builder) || builder.findOptions().dontCallFindHooks) {
       return result;
     } else {
       return afterReturn(callStaticBeforeFind(builder), result);

lib/relations/belongsToMany/BelongsToManyRelation.js

@@ -167,9 +167,13 @@ export class BelongsToManyRelation extends Relation {
   }
 
   // Find the keys of the related rows that `builder` filters, for `unrelate()`
-  // and `delete()`:
+  // and `delete()`. Like all internal reads of the relation, it calls no find
+  // hooks.
   findKeys(builder, owner) {
-    const query = this.relatedModelClass.query().childQueryOf(builder);
+    const query = this.relatedModelClass
+      .query()
+      .childQueryOf(builder)
+      .findOptions({ dontCallFindHooks: true });
 
     return query
       .copyFrom(builder, builder.constructor.JoinSelector)
@@ -181,12 +185,15 @@ export class BelongsToManyRelation extends Relation {
 
   // Add and remove keys in the arrays of the owner rows, and in the owner
   // models. Resolves to the number of owner rows that changed. The arrays are
-  // read and written back by separate queries.
+  // read without the find hooks and written back by separate queries.
   updateKeys(builder, owner, { add = [], remove = [] }) {
     const { ownerModelClass, ownerProp } = this;
     const idProp = ownerModelClass.getIdRelationProperty();
     const ownerIds = owner.getProps(this, idProp);
-    const query = ownerModelClass.query().childQueryOf(builder);
+    const query = ownerModelClass
+      .query()
+      .childQueryOf(builder)
+      .findOptions({ dontCallFindHooks: true });
     const idRefs = idProp.refs(query);
 
     const updateRow = async (row) => {

tests/integration/belongsToMany.js

@@ -450,6 +450,33 @@ export default (session) => {
         expect(await allTags()).toEqual(['1c', '2b', '3a', '4d', '5e']);
       });
 
+      it('relate(), unrelate(), insert() and delete() should not call the find hooks', async () => {
+        const post = await Post.query().findById(2);
+        const hooks = [];
+
+        // Hooks that filter all rows out, e.g. for soft deletes or access control:
+        for (const modelClass of [Post, Tag]) {
+          modelClass.beforeFind = () => hooks.push(`${modelClass.name}.beforeFind`);
+          modelClass.afterFind = () => (hooks.push(`${modelClass.name}.afterFind`), []);
+        }
+
+        try {
+          expect(await post.$relatedQuery('tags').relate(3)).toBe(1);
+          expect(await post.$relatedQuery('tags').unrelate().where('id', 1)).toBe(1);
+          await post.$relatedQuery('tags').insert({ id: 5, name: 'e' });
+          expect(await post.$relatedQuery('tags').delete().where('id', 2)).toBe(1);
+        } finally {
+          for (const modelClass of [Post, Tag]) {
+            delete modelClass.beforeFind;
+            delete modelClass.afterFind;
+          }
+        }
+
+        expect(hooks).toEqual([]);
+        expect(post.tagIds).toEqual([4, 3, 5]);
+        expect(await allTagIds()).toEqual([[3, 1], [4, 3, 5], [], null, [9]]);
+      });
+
       it('delete() and insert() should do nothing if resolved without executing', async () => {
         const post = await Post.query().findById(2);
 

tests/integration/staticHooks.js

@@ -3500,10 +3500,13 @@ export default (session) => {
         ]);
       });
 
-      it('should not call `afterFind` of the root class in `fetchGraph()`', async () => {
+      it('should not call the find hooks of the root class in `fetchGraph()`', async () => {
+        Person.beforeFind = createHookSpy();
+
         const person = await Person.fetchGraph(jennifer.$clone(), 'pets');
 
         expect(person.pets).toHaveLength(2);
+        expect(Person.beforeFind.calls).toHaveLength(0);
         expect(calls).toEqual([{ hook: 'pets.afterFind', result: person.pets }]);
       });