diff --git a/doc/api/query-builder/mutate-methods.md b/doc/api/query-builder/mutate-methods.md index 640246041..724f36bb4 100644 --- a/doc/api/query-builder/mutate-methods.md +++ b/doc/api/query-builder/mutate-methods.md @@ -132,7 +132,7 @@ queryBuilder = queryBuilder.insertGraph(graph, options); See the [section about graph inserts](/guide/query-examples.html#graph-inserts). -With `strict: true`, the `root` and `relations` options say per relation whether models with ids are related, inserted or throw (`added`, by default `'insert'`), and whether models without ids are inserted or throw (`created`, by default `'insert'`). See [strict mode](/guide/query-examples.html#strict-mode) in the section about graph upserts. +With `strict: true`, the `root` and `relations` options say per relation whether models with ids are related, inserted or throw (`added`, by default `'insert'`), and whether models without ids are inserted or throw (`created`, by default `'insert'`). Strict calls are atomic: they run in the transaction of the query, or else in their own, so hooks that run queries need to run them in `context.transaction`. See [strict mode](/guide/query-examples.html#strict-mode) in the section about graph upserts. ::: warning [onConflict()](/api/query-builder/mutate-methods.html#onconflict), [ignore()](/api/query-builder/mutate-methods.html#ignore) and [merge()](/api/query-builder/mutate-methods.html#merge) aren't supported by `insertGraph` (or `upsertGraph`), and make the query throw an error. A graph is inserted using multiple insert queries, one for each table and batch, so there is no single query to apply a conflict target to. Applying it to the root table only isn't safe either: rows skipped by `ignore()` aren't returned from the database, so the related rows would be inserted without the correct foreign keys. Insert the rows that may conflict using a separate [insert()](/api/query-builder/mutate-methods.html#insert) query and relate them in the graph, for example using `#dbRef` or the `relate` option. @@ -482,7 +482,7 @@ queryBuilder = queryBuilder.upsertGraph(graph, options); See the [section about graph upserts](/guide/query-examples.html#graph-upserts) -With `strict: true`, the `root` and `relations` options replace the options per relation path: each relation expression in `relations` maps to the settings `removed`, `added`, `kept` and `created`, one for each case of the difference between the graph and the stored relation. Entries are applied in their order, later ones overriding earlier ones setting by setting, and `'*'` matches all relations. `removed` has no default and has to be set for every relation that the graph gives, and `added` defaults to `'throw'`. `added: 'relate'` lets the graph relate any row by its id, so limit the relations with [allowGraph](/api/query-builder/eager-methods.html#allowgraph) for graphs from untrusted input. See [strict mode](/guide/query-examples.html#strict-mode) for the settings and their defaults. +With `strict: true`, the `root` and `relations` options replace the options per relation path: each relation expression in `relations` maps to the settings `removed`, `added`, `kept` and `created`, one for each case of the difference between the graph and the stored relation. Entries are applied in their order, later ones overriding earlier ones setting by setting, and `'*'` matches all relations. `removed` has no default and has to be set for every relation that the graph gives, and `added` defaults to `'throw'`. `added: 'relate'` lets the graph relate any row by its id, so limit the relations with [allowGraph](/api/query-builder/eager-methods.html#allowgraph) for graphs from untrusted input. Strict calls are atomic: they run in the transaction of the query, or else in their own, so hooks that run queries need to run them in `context.transaction`. See [strict mode](/guide/query-examples.html#strict-mode) for the settings and their defaults. In addition to the `#id`, `#ref` and `#dbRef` special properties supported by [insertGraph](/api/query-builder/mutate-methods.html#insertgraph), related models in the graph can be marked with `'#unrelate': true` or `'#delete': true` to unrelate or delete them individually, regardless of the `unrelate`, `noUnrelate` and `noDelete` options. Like `#ref`, these special properties need to be enabled with the `allowMarkers` option, since they let whoever provides the graph unrelate or delete related models. See [this section](/guide/query-examples.html#graph-upserts) for details. diff --git a/doc/guide/query-examples.md b/doc/guide/query-examples.md index e7af0997c..13c047b56 100644 --- a/doc/guide/query-examples.md +++ b/doc/guide/query-examples.md @@ -911,7 +911,7 @@ See the [allowGraph](/api/query-builder/eager-methods.html#allowgraph) method if If you are using Postgres the inserts are done in batches for maximum performance. On other databases the rows need to be inserted one at a time. This is because postgresql is the only database engine that returns the identifiers of all inserted rows and not just the first or the last one. -[insertGraph](/api/query-builder/mutate-methods.html#insertgraph) operation is **not** atomic by default! You need to start a transaction and pass it to the query using any of the supported ways. See the section about [transactions](/guide/transactions.html) for more information. +[insertGraph](/api/query-builder/mutate-methods.html#insertgraph) operation is **not** atomic by default, unless in [strict mode](#strict-mode)! You need to start a transaction and pass it to the query using any of the supported ways. See the section about [transactions](/guide/transactions.html) for more information. You can read more about graph inserts from [this blog post](https://www.vincit.fi/en/blog/nested-eager-loading-and-inserts-with-objection-js/). @@ -1135,7 +1135,7 @@ The [upsertGraph](/api/query-builder/mutate-methods.html#upsertgraph) method wor [upsertGraph](/api/query-builder/mutate-methods.html#upsertgraph) uses [insertGraph](/api/query-builder/mutate-methods.html#insertgraph) under the hood for inserts. That means that you can insert object graphs for relations and use all [insertGraph](/api/query-builder/mutate-methods.html#insertgraph) features like `#ref` references. -[upsertGraph](/api/query-builder/mutate-methods.html#upsertgraph) operation is **not** atomic by default! You need to start a transaction and pass it to the query using any of the supported ways. See the section about [transactions](/guide/transactions.html) for more information. +[upsertGraph](/api/query-builder/mutate-methods.html#upsertgraph) operation is **not** atomic by default, unless in [strict mode](#strict-mode)! You need to start a transaction and pass it to the query using any of the supported ways. See the section about [transactions](/guide/transactions.html) for more information. See the [allowGraph](/api/query-builder/eager-methods.html#allowgraph) method if you need to limit which relations can be modified using [upsertGraph](/api/query-builder/mutate-methods.html#upsertgraph) method to avoid security issues. @@ -1560,6 +1560,8 @@ The keys of `relations` are relation expressions that match relation paths like [insertGraph](/api/query-builder/mutate-methods.html#insertgraph) takes `strict`, `root` and `relations` too, with only the `added` and `created` settings, since it never loads, updates or removes rows. There, `added` defaults to `'insert'` and takes `'relate'`, `'insert'` and `'throw'`, which makes models with ids throw a `NotFoundError` instead of inserting them. The props of the rows that it relates are neither written nor compared with the stored rows. +Strict graphs are written atomically: strict `insertGraph` and `upsertGraph` calls, and their `AndFetch` variants, run in the transaction of the query, passed to it or bound to its model, or else in their own. The transaction covers the whole call, including the loads and the fetch, so that a database error halfway, e.g. a `UniqueViolationError`, leaves nothing written. Hooks that run queries need to run them in `context.transaction`: queries through another connection wait for the transaction, and on SQLite with its single connection, they deadlock. + ::: warning `added: 'relate'` and `'relateOrInsert'` let whoever provides the graph relate any row by its id, and move it away from its current owner, also when set through `'*'`. Set them only for the relations that need them when the graph comes from a request body, and use [allowGraph](/api/query-builder/eager-methods.html#allowgraph) to limit the relations the graph can contain. ::: diff --git a/doc/release-notes/changelog.md b/doc/release-notes/changelog.md index 70734f72d..3bcbed797 100644 --- a/doc/release-notes/changelog.md +++ b/doc/release-notes/changelog.md @@ -8,6 +8,7 @@ - In strict mode, `created: 'throw'` throws for models without ids, where `noInsert` skips them, and `added: 'throw'`, the default of `upsertGraph()`, throws a `NotFoundError` for models with ids, or with the key that relates them, that aren't in their relation, naming the `added` setting to use [#429](https://github.com/ditojs/objection/issues/429) - In strict mode, a key prop that a relation of the same model sets throws a `ValidationError` of type `InvalidGraph` when it differs from the relation's value, except for `#ref{}` templates, which the relation's value still overrides. Without strict mode, the relation's value overrides all of them [#429](https://github.com/ditojs/objection/issues/429) - In strict mode, values that the graph gives but doesn't write, under `kept: 'preserve'`, including the extras of join rows and the rows that `added` relates, or at `#ref` models that only place their model, throw a `ValidationError` of type `InvalidGraph` when they differ from the stored rows, compared so that fetched graphs sent back unchanged pass on all databases, but without the conversions of `==`: `''` and `false` don't equal `0`. Repeated models are compared the same way [#429](https://github.com/ditojs/objection/issues/429) +- Strict `insertGraph()` and `upsertGraph()` are atomic: they run in the transaction of the query, or else in their own, which covers the whole call, including the loads and the fetch of the `AndFetch` variants. Hooks that run queries need to run them in `context.transaction` [#429](https://github.com/ditojs/objection/issues/429) ## 4.2.0 diff --git a/lib/queryBuilder/QueryBuilder.js b/lib/queryBuilder/QueryBuilder.js index 240d9ed9e..f649603ea 100644 --- a/lib/queryBuilder/QueryBuilder.js +++ b/lib/queryBuilder/QueryBuilder.js @@ -636,9 +636,21 @@ export class QueryBuilder extends QueryBuilderBase { const builder = this.clone(); try { - await beforeExecute(builder); - const result = await doExecute(builder); - return await afterExecute(builder, result); + if (builder.has((op) => op.isAtomic()) && !builder.knex().isTransaction) { + // `Object.entries()` leaves out the symbol that binds the context to + // this builder, so that `context.transaction` is the transaction: + return await builder.knex().transaction((trx) => + executeQuery( + builder + .clone() + .knex(trx) + .clearContext() + .context(Object.fromEntries(Object.entries(builder.context()))), + ), + ); + } else { + return await executeQuery(builder); + } } catch (error) { return await handleExecuteError(builder, error); } @@ -1426,6 +1438,12 @@ function findQueryExecutorOperation(builder) { return builder.findOperation((op) => op.hasQueryExecutor()); } +async function executeQuery(builder) { + await beforeExecute(builder); + const result = await doExecute(builder); + return await afterExecute(builder, result); +} + function beforeExecute(builder) { let promise = Promise.resolve(); diff --git a/lib/queryBuilder/operations/DelegateOperation.js b/lib/queryBuilder/operations/DelegateOperation.js index 6c123b837..8e9acb691 100644 --- a/lib/queryBuilder/operations/DelegateOperation.js +++ b/lib/queryBuilder/operations/DelegateOperation.js @@ -138,6 +138,10 @@ export class DelegateOperation extends QueryBuilderOperation { return this.onError !== DelegateOperation.prototype.onError || this.delegate.hasOnError(); } + isAtomic() { + return this.delegate.isAtomic(); + } + toFindOperation(builder) { return this.delegate.toFindOperation(builder); } diff --git a/lib/queryBuilder/operations/InsertGraphOperation.js b/lib/queryBuilder/operations/InsertGraphOperation.js index a6a77c7c0..497eedd41 100644 --- a/lib/queryBuilder/operations/InsertGraphOperation.js +++ b/lib/queryBuilder/operations/InsertGraphOperation.js @@ -45,6 +45,10 @@ export class InsertGraphOperation extends DelegateOperation { return retVal; } + isAtomic() { + return this.upsert.graphOptions.isStrict(); + } + onBefore1(builder, result) { GraphUpsert.checkForOnConflict(builder, this.name); return result; diff --git a/lib/queryBuilder/operations/QueryBuilderOperation.js b/lib/queryBuilder/operations/QueryBuilderOperation.js index 51b8b907e..25d964604 100644 --- a/lib/queryBuilder/operations/QueryBuilderOperation.js +++ b/lib/queryBuilder/operations/QueryBuilderOperation.js @@ -214,6 +214,12 @@ export class QueryBuilderOperation { return this.onError !== QueryBuilderOperation.prototype.onError; } + // Return true if the whole query must run in a transaction: its own, unless + // the query already runs in one. + isAtomic() { + return false; + } + // Returns the "find" equivalent of this operation. // // For example an operation that finds an item and updates it diff --git a/lib/queryBuilder/operations/UpsertGraphOperation.js b/lib/queryBuilder/operations/UpsertGraphOperation.js index 71b3a3e7f..e33f46e53 100644 --- a/lib/queryBuilder/operations/UpsertGraphOperation.js +++ b/lib/queryBuilder/operations/UpsertGraphOperation.js @@ -38,6 +38,10 @@ export class UpsertGraphOperation extends QueryBuilderOperation { return true; } + isAtomic() { + return this.upsert.graphOptions.isStrict(); + } + onBefore1(builder, result) { GraphUpsert.checkForOnConflict(builder, this.name); return result; diff --git a/tests/integration/upsertGraphStrict.js b/tests/integration/upsertGraphStrict.js index daca85819..41a3c17e0 100644 --- a/tests/integration/upsertGraphStrict.js +++ b/tests/integration/upsertGraphStrict.js @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach } from 'vitest'; -import { NotFoundError } from 'objection'; +import { NotFoundError, UniqueViolationError } from 'objection'; import { FetchStrategy } from '../../lib/queryBuilder/graph/GraphOptions.js'; export default (session) => { @@ -381,8 +381,9 @@ export default (session) => { ) .catch((err) => err); + // Leave out the statements of the transaction that the query runs in: const queries = []; - const onQuery = ({ sql }) => queries.push(sql); + const onQuery = ({ sql }) => !/^(begin|rollback)\b/i.test(sql) && queries.push(sql); session.knex.on('query', onQuery); const error = await upsert({ model1Relation3: { removed: 'delete' } }); session.knex.off('query', onQuery); @@ -1000,5 +1001,164 @@ export default (session) => { expect(joinRow.extra1).toBe('changed'); }); }); + + describe('transactions', () => { + beforeEach(() => { + return session.populate([ + { id: 1, model1Prop1: 'root 1' }, + { + id: 2, + model1Prop1: 'root 2', + model1Relation2: [{ idCol: 1, model2Prop1: 'hasMany 1' }], + }, + ]); + }); + + // The new Model1 rows are inserted before their hasMany rows, whose id is + // taken: + const insertGraph = (query, options) => + query + .insertGraph( + { model1Prop1: 'new', model1Relation2: [{ idCol: 1, model2Prop1: 'taken' }] }, + options, + ) + .catch((err) => err); + + const upsertGraph = (query, options) => + query + .upsertGraph( + { + id: 1, + model1Relation1: { + model1Prop1: 'new', + model1Relation2: [{ idCol: 1, model2Prop1: 'taken' }], + }, + }, + options, + ) + .catch((err) => err); + + const getProps = async (knex = session.knex) => + (await knex('Model1').orderBy('id')).map((row) => row.model1Prop1); + + it('should roll back a strict insertGraph() that fails at the database', async () => { + const error = await insertGraph(Model1.query(session.knex), { strict: true }); + expect(error).toBeInstanceOf(UniqueViolationError); + expect(await getProps()).toEqual(['root 1', 'root 2']); + }); + + it('should keep the rows of a non-strict insertGraph() that fails at the database', async () => { + const error = await insertGraph(Model1.query(session.knex)); + expect(error).toBeInstanceOf(UniqueViolationError); + expect(await getProps()).toEqual(['root 1', 'root 2', 'new']); + }); + + it('should roll back a strict upsertGraph() that fails at the database', async () => { + const error = await upsertGraph(Model1.query(session.knex), { + strict: true, + relations: { '*': { added: 'insert', removed: 'preserve' } }, + }); + expect(error).toBeInstanceOf(UniqueViolationError); + expect(await getProps()).toEqual(['root 1', 'root 2']); + }); + + it('should keep the rows of a non-strict upsertGraph() that fails at the database', async () => { + const error = await upsertGraph(Model1.query(session.knex), { insertMissing: true }); + expect(error).toBeInstanceOf(UniqueViolationError); + expect(await getProps()).toEqual(['root 1', 'root 2', 'new']); + }); + + it('should roll back a strict insertGraphAndFetch() that fails at the database', async () => { + const error = await Model1.query(session.knex) + .insertGraphAndFetch( + { model1Prop1: 'new', model1Relation2: [{ idCol: 1, model2Prop1: 'taken' }] }, + { strict: true }, + ) + .catch((err) => err); + expect(error).toBeInstanceOf(UniqueViolationError); + expect(await getProps()).toEqual(['root 1', 'root 2']); + }); + + it('should call onError() after the rollback', async () => { + const result = await insertGraph( + Model1.query(session.knex).onError((error, builder) => getProps(builder.knex())), + { strict: true }, + ); + expect(result).toEqual(['root 1', 'root 2']); + }); + + it('should pass the transaction to the hooks', async () => { + const transactions = []; + class HookModel1 extends Model1 { + $beforeInsert(context) { + transactions.push(context.transaction); + } + } + + await HookModel1.query(session.knex) + .runBefore((result, builder) => { + transactions.push(builder.context().transaction); + }) + .insertGraph({ model1Prop1: 'new' }, { strict: true }); + + expect(transactions).toHaveLength(2); + expect(transactions[0].isTransaction).toBe(true); + expect(transactions[1]).toBe(transactions[0]); + }); + + // Run `callback` in a transaction that is rolled back, and return the + // transaction, the one in the context of the query that `callback` + // returns, and whether a savepoint was created: + async function rollBack(callback) { + let trx = null; + let contextTransaction = null; + let hasSavepoint = false; + const onQuery = (query) => { + hasSavepoint ||= /savepoint/i.test(query.sql); + }; + + session.knex.on('query', onQuery); + + try { + const error = await session.knex + .transaction(async (transaction) => { + trx = transaction; + await callback(trx).runBefore((result, builder) => { + contextTransaction = builder.context().transaction; + }); + throw new Error('rollback'); + }) + .catch((err) => err); + + expect(error.message).toBe('rollback'); + } finally { + session.knex.off('query', onQuery); + } + + return { trx, contextTransaction, hasSavepoint }; + } + + it('should use the transaction that the query runs in', async () => { + const { trx, contextTransaction, hasSavepoint } = await rollBack((trx) => + Model1.query(trx).insertGraph({ model1Prop1: 'new' }, { strict: true }), + ); + + expect(contextTransaction).toBe(trx); + expect(hasSavepoint).toBe(false); + expect(await getProps()).toEqual(['root 1', 'root 2']); + }); + + it('should use the transaction that the model is bound to', async () => { + const { trx, contextTransaction, hasSavepoint } = await rollBack((trx) => + Model1.bindKnex(trx) + .query() + .upsertGraph({ id: 1, model1Prop1: 'changed' }, { strict: true }), + ); + + expect(contextTransaction).toBe(trx); + expect(hasSavepoint).toBe(false); + expect(await getProps()).toEqual(['root 1', 'root 2']); + }); + }); }); };