From 2bc9e41634f08c65372c5c4adc4cbde2196bafc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ju=CC=88rg=20Lehni?= Date: Mon, 12 Oct 2026 21:41:39 +0200 Subject: [PATCH] Run the internal reads of BelongsToManyRelation writes without find hooks (#2667) --- doc/release-notes/changelog.md | 3 +++ doc/release-notes/migration.md | 2 +- lib/queryBuilder/operations/FindOperation.js | 2 +- .../belongsToMany/BelongsToManyRelation.js | 15 ++++++++--- tests/integration/belongsToMany.js | 27 +++++++++++++++++++ tests/integration/staticHooks.js | 5 +++- 6 files changed, 47 insertions(+), 7 deletions(-) diff --git a/doc/release-notes/changelog.md b/doc/release-notes/changelog.md index 303a4e84a..50d35cb2e 100644 --- a/doc/release-notes/changelog.md +++ b/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` 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) diff --git a/doc/release-notes/migration.md b/doc/release-notes/migration.md index c48126c1f..3cdb71433 100644 --- a/doc/release-notes/migration.md +++ b/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` declares `result: R`, with `R = undefined` by default, and the static hooks of `Model` declare the shapes that they get, e.g. `StaticHookArguments` for `afterFind`. Overrides that destructure `result` from `StaticHookArguments` need the type of the result as second type argument, e.g. `StaticHookArguments`. diff --git a/lib/queryBuilder/operations/FindOperation.js b/lib/queryBuilder/operations/FindOperation.js index 6d1c0ad77..4eb2ee736 100644 --- a/lib/queryBuilder/operations/FindOperation.js +++ b/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); diff --git a/lib/relations/belongsToMany/BelongsToManyRelation.js b/lib/relations/belongsToMany/BelongsToManyRelation.js index e6b6d5e58..64bea6d1e 100644 --- a/lib/relations/belongsToMany/BelongsToManyRelation.js +++ b/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) => { diff --git a/tests/integration/belongsToMany.js b/tests/integration/belongsToMany.js index 0ee75074e..c66408c4d 100644 --- a/tests/integration/belongsToMany.js +++ b/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); diff --git a/tests/integration/staticHooks.js b/tests/integration/staticHooks.js index f2ad28df4..c16252be4 100644 --- a/tests/integration/staticHooks.js +++ b/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 }]); });