From 439b18240c5b92563ce7e77ecbd6663027fb32a6 Mon Sep 17 00:00:00 2001 From: Maciej Holyszko <14310995+falkenhawk@users.noreply.github.com> Date: Mon, 6 Feb 2023 22:11:28 +0100 Subject: [PATCH 1/2] allow marking individual models in graph to be unrelated or deleted with `#unrelate` or `#delete` (prop names configurable with `Model.graphUnrelateProp` and `Model.graphDeleteProp`) which override `GraphOptions` i.e. even if `noDelete` or `noUnrelate` is set, specific models marked with those special props can still be unrelated/deleted. This allows partial unrelate/deletes to HasMany and ManyToMany relations without the need of passing the whole list of existing models which should be preserved. --- lib/model/Model.js | 2 + lib/model/graph/ModelGraphNode.js | 8 ++ lib/queryBuilder/graph/GraphFetcher.js | 4 +- lib/queryBuilder/graph/GraphOptions.js | 22 ++++- tests/integration/upsertGraph.js | 130 +++++++++++++++++++++++++ typings/objection/index.d.ts | 4 + 6 files changed, 167 insertions(+), 3 deletions(-) diff --git a/lib/model/Model.js b/lib/model/Model.js index 701ac3ac6..3d84749c4 100644 --- a/lib/model/Model.js +++ b/lib/model/Model.js @@ -729,6 +729,8 @@ Model.uidProp = '#id'; Model.uidRefProp = '#ref'; Model.dbRefProp = '#dbRef'; Model.propRefRegex = /#ref{([^\.]+)\.([^}]+)}/g; +Model.graphUnrelateProp = '#unrelate'; +Model.graphDeleteProp = '#delete'; Model.jsonAttributes = null; Model.cloneObjectAttributes = true; Model.virtualAttributes = null; diff --git a/lib/model/graph/ModelGraphNode.js b/lib/model/graph/ModelGraphNode.js index 80966fd43..7f1f63337 100644 --- a/lib/model/graph/ModelGraphNode.js +++ b/lib/model/graph/ModelGraphNode.js @@ -46,6 +46,14 @@ class ModelGraphNode { return this.obj[this.modelClass.dbRefProp]; } + get toBeUnrelated() { + return this.obj[this.modelClass.graphUnrelateProp] === true; + } + + get toBeDeleted() { + return this.obj[this.modelClass.graphDeleteProp] === true; + } + get parentNode() { if (this.parentEdge) { return this.parentEdge.ownerNode; diff --git a/lib/queryBuilder/graph/GraphFetcher.js b/lib/queryBuilder/graph/GraphFetcher.js index f39e64173..50d2af8e5 100644 --- a/lib/queryBuilder/graph/GraphFetcher.js +++ b/lib/queryBuilder/graph/GraphFetcher.js @@ -141,7 +141,9 @@ function shouldSelectColumn(column, selects, node) { !selects.has(column) && column !== modelClass.propertyNameToColumnName(modelClass.dbRefProp) && column !== modelClass.propertyNameToColumnName(modelClass.uidRefProp) && - column !== modelClass.propertyNameToColumnName(modelClass.uidProp) + column !== modelClass.propertyNameToColumnName(modelClass.uidProp) && + column !== modelClass.propertyNameToColumnName(modelClass.graphUnrelateProp) && + column !== modelClass.propertyNameToColumnName(modelClass.graphDeleteProp) ); } diff --git a/lib/queryBuilder/graph/GraphOptions.js b/lib/queryBuilder/graph/GraphOptions.js index b3152ed99..5faee2a68 100644 --- a/lib/queryBuilder/graph/GraphOptions.js +++ b/lib/queryBuilder/graph/GraphOptions.js @@ -51,6 +51,10 @@ class GraphOptions { // Like `shouldRelate` but ignores settings that explicitly disable relate operations. shouldRelateIgnoreDisable(node, graphData) { + if (node.toBeUnrelated || node.toBeDeleted) { + return false; + } + if (node.isReference || node.isDbReference) { return true; } @@ -71,6 +75,10 @@ class GraphOptions { // Like `shouldInsert` but ignores settings that explicitly disable insert operations. shouldInsertIgnoreDisable(node, graphData) { + if (node.toBeUnrelated || node.toBeDeleted) { + return false; + } + return ( !getCurrentNode(node, graphData) && !this.shouldRelateIgnoreDisable(node, graphData) && @@ -89,6 +97,10 @@ class GraphOptions { // Like `shouldPatch() || shouldUpdate()` but ignores settings that explicitly disable // update or patch operations. shouldPatchOrUpdateIgnoreDisable(node, graphData) { + if (node.toBeUnrelated || node.toBeDeleted) { + return false; + } + if (this.shouldRelate(node, graphData)) { // We should update all nodes that are going to be related. Note that // we don't actually update anything unless there is something to update @@ -121,16 +133,22 @@ class GraphOptions { } shouldUnrelate(currentNode, graphData) { + const node = getNode(currentNode, graphData.graph); + if (node && node.toBeUnrelated) return true; + return ( - !getNode(currentNode, graphData.graph) && + !node && !this._hasOption(currentNode, NO_UNRELATE) && this.shouldUnrelateIgnoreDisable(currentNode) ); } shouldDelete(currentNode, graphData) { + const node = getNode(currentNode, graphData.graph); + if (node && node.toBeDeleted) return true; + return ( - !getNode(currentNode, graphData.graph) && + !node && !this._hasOption(currentNode, NO_DELETE) && !this.shouldUnrelateIgnoreDisable(currentNode) ); diff --git a/tests/integration/upsertGraph.js b/tests/integration/upsertGraph.js index 3453ae145..4e1d0e556 100644 --- a/tests/integration/upsertGraph.js +++ b/tests/integration/upsertGraph.js @@ -1296,6 +1296,136 @@ module.exports = (session) => { }); }); + it('should respect noDelete flag and special #unrelate and #delete model props', () => { + const upsert = { + // the root gets updated because it has an id + id: 2, + model1Prop1: 'updated root 2', + + // unrelate + model1Relation1: null, + + // update idCol=1 + // delete idCol=2 with `#delete: true` special prop + // and insert one new + model1Relation2: [ + { + idCol: 1, + model2Prop1: 'updated hasMany 1', + + // unrelate id=4 with `#unrelate: true` special prop + // don't unrelate id=5 because of `noUnrelate` + // relate id=6 + // and insert one new + model2Relation1: [ + { + id: 4, + '#unrelate': true, + }, + { + // This is the new row. + model1Prop1: 'inserted manyToMany', + }, + { + id: 6, + }, + ], + }, + { + idCol: 2, + '#delete': true, + }, + { + // This is the new row. + model2Prop1: 'inserted hasMany', + }, + ], + }; + + return transaction(session.knex, (trx) => { + return Model1.query(trx) + .upsertGraph(upsert, { + fetchStrategy, + relate: true, + noDelete: true, + unrelate: ['model1Relation1'], + }) + .then((result) => { + // Fetch the graph from the database. + return Model1.query(trx) + .findById(2) + .withGraphFetched( + '[model1Relation1, model1Relation2(orderById).model2Relation1(orderById)]' + ); + }) + .then(omitIrrelevantProps) + .then((result) => { + expect(result).to.eql({ + id: 2, + model1Id: null, + model1Prop1: 'updated root 2', + + model1Relation1: null, + + model1Relation2: [ + { + idCol: 1, + model1Id: 2, + model2Prop1: 'updated hasMany 1', + + model2Relation1: [ + { + id: 5, + model1Id: null, + model1Prop1: 'manyToMany 2', + }, + { + id: 6, + model1Id: null, + model1Prop1: 'manyToMany 3', + }, + { + id: 8, + model1Id: null, + model1Prop1: 'inserted manyToMany', + }, + ], + }, + { + idCol: 3, + model1Id: 2, + model2Prop1: 'inserted hasMany', + model2Relation1: [], + }, + ], + }); + + return Promise.all([trx('Model1'), trx('model2')]).then( + ([model1Rows, model2Rows]) => { + // Row 3 should NOT be deleted. + expect(model1Rows.find((it) => it.id == 3)).to.eql({ + id: 3, + model1Id: null, + model1Prop1: 'belongsToOne', + model1Prop2: null, + }); + + // Row 4 should NOT be deleted. + expect(model1Rows.find((it) => it.id == 4)).to.eql({ + id: 4, + model1Id: null, + model1Prop1: 'manyToMany 1', + model1Prop2: null, + }); + + // Row 2 should be deleted. + expect(model2Rows.find((it) => it.id_col == 2)).to.equal(undefined); + } + ); + }); + }); + }); + it('should relate and unrelate some models if `unrelate` and `relate` are arrays of relation paths', () => { const upsert = { // the root gets updated because it has an id diff --git a/typings/objection/index.d.ts b/typings/objection/index.d.ts index e14784130..8f4398cbb 100644 --- a/typings/objection/index.d.ts +++ b/typings/objection/index.d.ts @@ -1437,6 +1437,8 @@ declare namespace Objection { uidRefProp: string; dbRefProp: string; propRefRegex: RegExp; + graphUnrelateProp: string; + graphDeleteProp: string; pickJsonSchemaProperties: boolean; relatedFindQueryMutates: boolean; relatedInsertQueryMutates: boolean; @@ -1543,6 +1545,8 @@ declare namespace Objection { static uidRefProp: string; static dbRefProp: string; static propRefRegex: RegExp; + static graphUnrelateProp: string; + static graphDeleteProp: string; static pickJsonSchemaProperties: boolean; static relatedFindQueryMutates: boolean; static relatedInsertQueryMutates: boolean; From c339697e634dd34022fa2cd685bcbb20cd64195d Mon Sep 17 00:00:00 2001 From: Maciej Holyszko <14310995+falkenhawk@users.noreply.github.com> Date: Wed, 22 Feb 2023 12:32:43 +0100 Subject: [PATCH 2/2] avoid NotFoundError when trying to unrelate / delete a model with invalid id `NotFoundError: model (id=x) is not a child of model (id=y). If you want to relate it, use the relate option. If you want to insert it with an id, use the insertMissing option` - let upsertGraph ignore those errors when `#delete` or `#unrelate` props are used. --- lib/queryBuilder/graph/GraphUpsert.js | 2 + tests/integration/upsertGraph.js | 93 +++++++++++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/lib/queryBuilder/graph/GraphUpsert.js b/lib/queryBuilder/graph/GraphUpsert.js index 178483de4..2b7039ede 100644 --- a/lib/queryBuilder/graph/GraphUpsert.js +++ b/lib/queryBuilder/graph/GraphUpsert.js @@ -207,6 +207,8 @@ function checkForNotFoundErrors(graphData, builder) { for (const node of graph.nodes) { if ( node.obj.$hasId() && + !node.toBeUnrelated && + !node.toBeDeleted && !graphOptions.shouldInsertIgnoreDisable(node, graphData) && !graphOptions.shouldRelateIgnoreDisable(node, graphData) && !currentGraph.nodeForNode(node) diff --git a/tests/integration/upsertGraph.js b/tests/integration/upsertGraph.js index 4e1d0e556..e2e2a4b8b 100644 --- a/tests/integration/upsertGraph.js +++ b/tests/integration/upsertGraph.js @@ -1426,6 +1426,99 @@ module.exports = (session) => { }); }); + it('should not fail when trying to #unrelate or #delete a model which either does not exist or is linked elsewhere', () => { + const upsert = { + id: 2, + model1Relation2: [ + { + idCol: 1, + model2Relation1: [ + // ignore unrelating/deleting models which are linked to a different base model + // id 6&7 are linked to model1Relation2.idCol=2 + { id: 6, '#unrelate': true }, + { id: 7, '#delete': true }, + // ignore unrelating/deleting non-existing models + { id: 900, '#unrelate': true }, + { id: 901, '#delete': true }, + // add new model + { model1Prop1: 'inserted manyToMany' }, + ], + }, + // ignore delete (non-existing) idCol=3 with `#delete: true` special prop + // ignore unrelate (non-existing) idCol=4 with `#unrelate: true` special prop + { idCol: 3, '#delete': true }, + { idCol: 4, '#unrelate': true }, + ], + }; + + return transaction(session.knex, (trx) => { + return Model1.query(trx) + .upsertGraph(upsert, { + fetchStrategy, + relate: true, + noDelete: true, + }) + .then((result) => { + // Fetch the graph from the database. + return Model1.query(trx) + .findById(2) + .withGraphFetched('[model1Relation2(orderById).model2Relation1(orderById)]'); + }) + .then(omitIrrelevantProps) + .then((result) => { + expect(result).to.eql({ + id: 2, + model1Id: 3, + model1Prop1: 'root 2', + + model1Relation2: [ + { + idCol: 1, + model1Id: 2, + model2Prop1: 'hasMany 1', + + model2Relation1: [ + { + id: 4, + model1Id: null, + model1Prop1: 'manyToMany 1', + }, + { + id: 5, + model1Id: null, + model1Prop1: 'manyToMany 2', + }, + { + id: 8, + model1Id: null, + model1Prop1: 'inserted manyToMany', + }, + ], + }, + { + idCol: 2, + model1Id: 2, + model2Prop1: 'hasMany 2', + + model2Relation1: [ + { + id: 6, + model1Id: null, + model1Prop1: 'manyToMany 3', + }, + { + id: 7, + model1Id: null, + model1Prop1: 'manyToMany 4', + }, + ], + }, + ], + }); + }); + }); + }); + it('should relate and unrelate some models if `unrelate` and `relate` are arrays of relation paths', () => { const upsert = { // the root gets updated because it has an id