diff --git a/doc/guide/query-examples.md b/doc/guide/query-examples.md index 13c047b56..7645ad99d 100644 --- a/doc/guide/query-examples.md +++ b/doc/guide/query-examples.md @@ -1558,7 +1558,7 @@ The values that a strict graph gives but doesn't write have to equal the stored The keys of `relations` are relation expressions that match relation paths like the expressions of the other options above: `'*'` matches all relations at any depth, and the root models are never matched. The entries are applied in their order, and later entries override the settings of earlier ones, setting by setting. Above, `movies` has `removed: 'preserve'` from `'*'`, and the settings of its own entry. A relation named in a key that its model doesn't have throws a `ValidationError` of type `InvalidGraph`, so a misspelled relation doesn't fall back to the defaults. -[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. +[insertGraph](/api/query-builder/mutate-methods.html#insertgraph) takes `strict`, `root` and `relations` too, with only the `added` and `created` settings, since it never 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. `'relate'` throws a `NotFoundError` for ids that don't exist, before anything is written. 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. diff --git a/doc/release-notes/changelog.md b/doc/release-notes/changelog.md index 251250e8d..fe52f3145 100644 --- a/doc/release-notes/changelog.md +++ b/doc/release-notes/changelog.md @@ -1,5 +1,11 @@ # Changelog +## 4.3.1 + +### Fixes + +- Strict `insertGraph()` with `added: 'relate'` throws a `NotFoundError` for related models whose ids don't exist, before anything is written, like `upsertGraph()` does. Since 4.3.0, it related them unchecked, writing keys that point to no row unless a foreign key constraint rejected them [#538](https://github.com/ditojs/objection/issues/538) + ## 4.3.0 ### Changes diff --git a/lib/queryBuilder/graph/GraphLoader.js b/lib/queryBuilder/graph/GraphLoader.js index 6f72da5b4..7975613e2 100644 --- a/lib/queryBuilder/graph/GraphLoader.js +++ b/lib/queryBuilder/graph/GraphLoader.js @@ -110,8 +110,44 @@ export async function loadCurrentGraph(builder, plan, readKeys = null) { // The other related models only need to exist, except for `#ref` models, // which are the models that they refer to, and the rows of the current graph // (#372): - const nodes = findRelatedNodes((node, rowKeys) => !node.isReference && !rowKeys.has(node.rowKey)); + await markMissingModels( + builder, + graphOptions, + findRelatedNodes((node, rowKeys) => !node.isReference && !rowKeys.has(node.rowKey)), + ); +} + +// `insertGraph()` loads no current graph, but in strict mode, it checks that +// the models that it relates by id exist, like `upsertGraph()` does (#538). +export async function loadRelatedIds(builder, plan) { + await markMissingModels( + builder, + plan.graphOptions, + plan.nodes.filter( + (node) => + node.obj.$hasId() && + !node.isReference && + !hasIdReference(node) && + plan.actionFor(node) === 'relate' && + isInWrittenBranch(plan, node), + ), + ); +} + +// Return true if the graph writes the branches of all the ancestors of `node`. +// `insertGraph()` writes nothing below related models (#217). +function isInWrittenBranch(plan, node) { + for (let parent = node.parent; parent; parent = parent.parent) { + if (!plan.writesBranchOf(parent)) { + return false; + } + } + + return true; +} +// Mark the models of `nodes` whose rows don't exist as missing. +async function markMissingModels(builder, graphOptions, nodes) { for (const nodesOfClass of groupBy(nodes, (node) => node.modelClass).values()) { const models = await fetchModels(builder, graphOptions, nodesOfClass, { onlyIds: true }); const idKeys = new Set(models.map((model) => model.$idKey())); diff --git a/lib/queryBuilder/graph/GraphUpsert.js b/lib/queryBuilder/graph/GraphUpsert.js index 0b704fdd1..fd38359ea 100644 --- a/lib/queryBuilder/graph/GraphUpsert.js +++ b/lib/queryBuilder/graph/GraphUpsert.js @@ -4,7 +4,7 @@ import { GraphPlan } from './GraphPlan.js'; import { GraphKeys } from './graphKeys.js'; import { GraphSchedule } from './GraphSchedule.js'; import { GraphWriter } from './GraphWriter.js'; -import { getReadKeys, loadCurrentGraph } from './GraphLoader.js'; +import { getReadKeys, loadCurrentGraph, loadRelatedIds } from './GraphLoader.js'; import { checkChangedIds, checkGraph, @@ -80,6 +80,8 @@ export class GraphUpsert { const load = async (readKeys) => { if (isUpsert) { await loadCurrentGraph(builder, plan, readKeys); + } else if (graphOptions.isStrict()) { + await loadRelatedIds(builder, plan); } plan.plan(); diff --git a/lib/queryBuilder/graph/graphChecks.js b/lib/queryBuilder/graph/graphChecks.js index 2439a4184..a1aae155e 100644 --- a/lib/queryBuilder/graph/graphChecks.js +++ b/lib/queryBuilder/graph/graphChecks.js @@ -47,7 +47,8 @@ export function checkMissingModels(builder, plan) { const key = node.hasId ? `id=${node.obj.$id()}` : node.relation.relatedProp.props.map((prop) => `${prop}=${node.obj[prop]}`).join(', '); - const added = node.isMissing ? 'relateOrInsert' : 'insert'; + // `insertGraph()` has no `relateOrInsert`: + const added = node.isMissing && !graphOptions.isInsertOnly() ? 'relateOrInsert' : 'insert'; const relateHint = `If you want to relate it, ${getHint(node, 'relate', 'relate')}. `; const insertHint = `If you want to insert it with ${node.hasId ? 'an id' : 'its key'}, ${getHint(node, 'insertMissing', added)}`; diff --git a/tests/integration/upsertGraphStrict.js b/tests/integration/upsertGraphStrict.js index 41a3c17e0..fd03d74dc 100644 --- a/tests/integration/upsertGraphStrict.js +++ b/tests/integration/upsertGraphStrict.js @@ -515,6 +515,37 @@ export default (session) => { expect(joinRows.map((row) => [row.model1Id, row.model2Id])).toEqual([[inserted.id, 1]]); }); + for (const [relationName, related, dataPath] of [ + ['model1Relation1', { id: 1000 }, ['model1Relation1']], + ['model1Relation3', [{ idCol: 1 }, { idCol: 1000 }], ['model1Relation3', 1]], + ]) { + it(`should throw before writing anything in insertGraph() if a model related through ${relationName} does not exist (#538)`, async () => { + const error = await Model1.query(session.knex) + .insertGraph( + { model1Prop1: 'new', [relationName]: related }, + { strict: true, relations: { [relationName]: { added: 'relate' } } }, + ) + .catch((err) => err); + + expect(error).toBeInstanceOf(NotFoundError); + expect(error.message).toBe( + `model (id=1000) does not exist. If you want to insert it with an id, set added: 'insert' for ${relationName} in the relations option`, + ); + expect(error.data.dataPath).toEqual(dataPath); + expect(await getRows('Model1', 'id')).toEqual([1, 2, 3, 4]); + }); + } + + it('should not check the models below related models in insertGraph(), which it ignores (#538)', async () => { + const inserted = await Model1.query(session.knex).insertGraph( + { model1Prop1: 'new', model1Relation3: [{ idCol: 1, model2Relation1: [{ id: 1000 }] }] }, + { strict: true, relations: { '*': { added: 'relate' } } }, + ); + + const joinRows = await session.knex('Model1Model2'); + expect(joinRows.map((row) => [row.model1Id, row.model2Id])).toEqual([[inserted.id, 1]]); + }); + it("should throw for rows with ids in insertGraph() with added: 'throw'", async () => { const insert = (graph, options) => Model1.query(session.knex)