Check related ids in strict insertGraph() like upsertGraph() does (#538)

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.
 

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

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()));

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();

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)}`;
 

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)