From c179f3067c7cb231dd7e183efa1ee1b55eee63d9 Mon Sep 17 00:00:00 2001 From: Michael Richardson Date: Wed, 22 Jul 2020 17:30:02 +0100 Subject: [PATCH 1/9] test(ReplicateAttributes): Add tests for deleting field in update --- test/functions/index.js | 26 ++++++++++++ test/functions/integrify.rules.js | 18 ++++++++ test/unit.test.js | 69 ++++++++++++++++++++++++++++++- 3 files changed, 112 insertions(+), 1 deletion(-) diff --git a/test/functions/index.js b/test/functions/index.js index 07ee03a..bf4e03f 100644 --- a/test/functions/index.js +++ b/test/functions/index.js @@ -39,6 +39,32 @@ module.exports.replicateMasterToDetail = integrify({ }, }); +module.exports.replicateMasterDeleteWhenEmpty = integrify({ + rule: 'REPLICATE_ATTRIBUTES', + source: { + collection: 'master', + }, + targets: [ + { + collection: 'detail1', + foreignKey: 'tempId', + attributeMapping: { + masterDetail1: 'foreignDetail1', + masterDetail2: 'foreignDetail2', + }, + deleteMissing: true, + }, + ], + hooks: { + pre: (change, context) => { + setState({ + change, + context, + }); + }, + }, +}); + module.exports.deleteReferencesToMaster = integrify({ rule: 'DELETE_REFERENCES', source: { diff --git a/test/functions/integrify.rules.js b/test/functions/integrify.rules.js index 0970319..2869c6b 100644 --- a/test/functions/integrify.rules.js +++ b/test/functions/integrify.rules.js @@ -27,6 +27,24 @@ module.exports = [ }, ], }, + { + rule: 'REPLICATE_ATTRIBUTES', + name: 'replicateMasterDeleteWhenEmpty', + source: { + collection: 'master', + }, + targets: [ + { + collection: 'detail1', + foreignKey: 'tempId', + attributeMapping: { + masterDetail1: 'foreignDetail1', + masterDetail2: 'foreignDetail2', + }, + deleteMissing: true, + }, + ], + }, { rule: 'DELETE_REFERENCES', name: 'deleteReferencesToMaster', diff --git a/test/unit.test.js b/test/unit.test.js index ed1e35b..bc8ab3d 100644 --- a/test/unit.test.js +++ b/test/unit.test.js @@ -61,6 +61,9 @@ testsuites.forEach(testsuite => { testDeleteReferences(sut, t, name)); test(`[${name}] test maintain count`, async t => testMaintainCount(sut, t)); + test(`[${name}] test replicate attributes delete when field is not there`, async t => + testReplicateAttributesDeleteEmpty(sut, t, name)); + test(`[${name}] test delete with masterId in target reference`, async t => testDeleteParamReferences(sut, t, name)); test(`[${name}] test delete with snapshot fields in target reference`, async t => @@ -163,7 +166,9 @@ async function testReplicateAttributes(sut, t, name) { // Call trigger to replicate attributes from master const beforeSnap = fft.firestore.makeDocumentSnapshot( - {}, + { + masterField5: 'missing', + }, `master/${masterId}` ); const afterSnap = fft.firestore.makeDocumentSnapshot( @@ -226,6 +231,68 @@ async function testReplicateAttributes(sut, t, name) { await t.pass(); } +async function testReplicateAttributesDeleteEmpty(sut, t, name) { + // Add a couple of detail documents to follow master + const masterId = makeid(); + await db.collection('detail1').add({ + tempId: masterId, + foreignDetail1: 'foreign_detail_1', + foreignDetail2: 'foreign_detail_2', + }); + + // Call trigger to replicate attributes from master + const beforeSnap = fft.firestore.makeDocumentSnapshot( + { + masterDetail1: 'after1', + masterDetail2: 'after2', + }, + `master/${masterId}` + ); + const afterSnap = fft.firestore.makeDocumentSnapshot( + { + masterDetail2: 'after3', + }, + `master/${masterId}` + ); + const change = fft.makeChange(beforeSnap, afterSnap); + const wrapped = fft.wrap(sut.replicateMasterDeleteWhenEmpty); + setState({ + change: null, + context: null, + }); + await wrapped(change, { + params: { + masterId: masterId, + }, + }); + + // Assert pre-hook was called (only for rules-in-situ) + if (name === 'rules-in-situ') { + const state = getState(); + t.truthy(state.change); + t.truthy(state.context); + t.is(state.context.params.masterId, masterId); + } + + // Assert that attributes get replicated to detail documents + await assertQuerySizeEventually( + db + .collection('detail1') + .where('tempId', '==', masterId) + .where('foreignDetail1', '==', 'foreign_detail_1'), + 0 + ); + await assertQuerySizeEventually( + db + .collection('detail1') + .where('tempId', '==', masterId) + .where('foreignDetail2', '==', 'after3'), + 1 + ); + + await t.pass(); +} + async function testDeleteReferences(sut, t, name) { // Create some docs referencing master doc const masterId = makeid(); From 2ed10ac435a501bfcf9fb84f7f2d8411a0ca4f5f Mon Sep 17 00:00:00 2001 From: Michael Richardson Date: Wed, 22 Jul 2020 17:30:57 +0100 Subject: [PATCH 2/9] feat(ReplicateAttributes): Delete field not set by setting flag --- src/rules/replicateAttributes.ts | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/rules/replicateAttributes.ts b/src/rules/replicateAttributes.ts index c2ad586..94aeec5 100644 --- a/src/rules/replicateAttributes.ts +++ b/src/rules/replicateAttributes.ts @@ -1,4 +1,6 @@ import { Config, Rule } from '../common'; +import { firestore } from 'firebase-admin'; +const FieldValue = firestore.FieldValue; export interface ReplicateAttributesRule extends Rule { source: { @@ -11,6 +13,7 @@ export interface ReplicateAttributesRule extends Rule { [sourceAttribute: string]: string; }; isCollectionGroup?: boolean; + deleteMissing?: boolean; }[]; hooks?: { pre?: Function; @@ -80,19 +83,30 @@ export function integrifyReplicateAttributes( return null; } - // Loop over each target specification to replicate atributes + // Loop over each target specification to replicate attributes const db = config.config.db; rule.targets.forEach(target => { const targetCollection = target.collection; const update = {}; + let shouldDelete = false; - // Create "update" mapping each changed attribute from source => target - Object.keys(newValue).forEach(changedAttribute => { - if (target.attributeMapping[changedAttribute]) { + if (target.deleteMissing) { + shouldDelete = target.deleteMissing; + } + + // Create "update" mapping each changed attribute from source => target, + // if delete is set delete field + Object.keys(target.attributeMapping).forEach(changedAttribute => { + if (newValue[changedAttribute]) { update[target.attributeMapping[changedAttribute]] = newValue[changedAttribute]; + } else if (shouldDelete) { + update[ + target.attributeMapping[changedAttribute] + ] = FieldValue.delete(); } }); + console.log( `integrify: On collection ${ target.isCollectionGroup ? 'group ' : '' From 0c7494ca4daaf9c59fe6b9f5300c2d399891e3ce Mon Sep 17 00:00:00 2001 From: Michael Richardson Date: Thu, 23 Jul 2020 10:35:10 +0100 Subject: [PATCH 3/9] test(replicateAttributes): Add test for primary key --- test/functions/index.js | 26 ++++++++- test/functions/integrify.rules.js | 18 +++++- test/unit.test.js | 93 +++++++++++++++++++++++++++---- 3 files changed, 125 insertions(+), 12 deletions(-) diff --git a/test/functions/index.js b/test/functions/index.js index bf4e03f..48bcb23 100644 --- a/test/functions/index.js +++ b/test/functions/index.js @@ -42,7 +42,7 @@ module.exports.replicateMasterToDetail = integrify({ module.exports.replicateMasterDeleteWhenEmpty = integrify({ rule: 'REPLICATE_ATTRIBUTES', source: { - collection: 'master', + collection: 'master/{primaryKey}', }, targets: [ { @@ -65,6 +65,30 @@ module.exports.replicateMasterDeleteWhenEmpty = integrify({ }, }); +module.exports.replicateReferencesWithMissingKey = integrify({ + rule: 'REPLICATE_ATTRIBUTES', + source: { + collection: 'master/{masterId}', + }, + targets: [ + { + collection: 'detail1', + foreignKey: 'randomId', + attributeMapping: { + masterDetail1: 'foreignDetail1', + }, + }, + ], + hooks: { + pre: (snap, context) => { + setState({ + snap, + context, + }); + }, + }, +}); + module.exports.deleteReferencesToMaster = integrify({ rule: 'DELETE_REFERENCES', source: { diff --git a/test/functions/integrify.rules.js b/test/functions/integrify.rules.js index 2869c6b..5f73532 100644 --- a/test/functions/integrify.rules.js +++ b/test/functions/integrify.rules.js @@ -31,7 +31,7 @@ module.exports = [ rule: 'REPLICATE_ATTRIBUTES', name: 'replicateMasterDeleteWhenEmpty', source: { - collection: 'master', + collection: 'master/{primaryKey}', }, targets: [ { @@ -45,6 +45,22 @@ module.exports = [ }, ], }, + { + rule: 'REPLICATE_ATTRIBUTES', + name: 'replicateReferencesWithMissingKey', + source: { + collection: 'master/{masterId}', + }, + targets: [ + { + collection: 'detail1', + foreignKey: 'randomId', + attributeMapping: { + masterDetail1: 'foreignDetail1', + }, + }, + ], + }, { rule: 'DELETE_REFERENCES', name: 'deleteReferencesToMaster', diff --git a/test/unit.test.js b/test/unit.test.js index bc8ab3d..b329b79 100644 --- a/test/unit.test.js +++ b/test/unit.test.js @@ -17,7 +17,7 @@ admin.initializeApp({ const db = admin.firestore(); async function clearFirestore() { - const collections = ['detail1', 'detail2', 'somecoll']; + const collections = ['detail1', 'detail2', 'detail3', 'somecoll']; for (const collection of collections) { const { docs } = await admin .firestore() @@ -55,14 +55,19 @@ testsuites.forEach(testsuite => { testPrimaryKey(sut, t, name)); test(`[${name}] test target collection parameter swap`, async t => testTargetVariableSwap(sut, t, name)); + + // Standard functionality test(`[${name}] test replicate attributes`, async t => testReplicateAttributes(sut, t, name)); test(`[${name}] test delete references`, async t => testDeleteReferences(sut, t, name)); test(`[${name}] test maintain count`, async t => testMaintainCount(sut, t)); + // Added by GitLive test(`[${name}] test replicate attributes delete when field is not there`, async t => testReplicateAttributesDeleteEmpty(sut, t, name)); + test(`[${name}] test replicate attributes with missing primary key in source reference`, async t => + testReplicateMissingSourceCollectionKey(sut, t, name)); test(`[${name}] test delete with masterId in target reference`, async t => testDeleteParamReferences(sut, t, name)); @@ -75,7 +80,6 @@ testsuites.forEach(testsuite => { test(`[${name}] test delete all sub-collections in target reference`, async t => testDeleteAllSubCollections(sut, t, name)); - test(`[${name}] test delete missing arguments error`, async t => testDeleteMissingArgumentsError(sut, t, name)); }); @@ -233,9 +237,9 @@ async function testReplicateAttributes(sut, t, name) { async function testReplicateAttributesDeleteEmpty(sut, t, name) { // Add a couple of detail documents to follow master - const masterId = makeid(); + const primaryKey = makeid(); await db.collection('detail1').add({ - tempId: masterId, + tempId: primaryKey, foreignDetail1: 'foreign_detail_1', foreignDetail2: 'foreign_detail_2', }); @@ -246,13 +250,13 @@ async function testReplicateAttributesDeleteEmpty(sut, t, name) { masterDetail1: 'after1', masterDetail2: 'after2', }, - `master/${masterId}` + `master/${primaryKey}` ); const afterSnap = fft.firestore.makeDocumentSnapshot( { masterDetail2: 'after3', }, - `master/${masterId}` + `master/${primaryKey}` ); const change = fft.makeChange(beforeSnap, afterSnap); const wrapped = fft.wrap(sut.replicateMasterDeleteWhenEmpty); @@ -262,7 +266,7 @@ async function testReplicateAttributesDeleteEmpty(sut, t, name) { }); await wrapped(change, { params: { - masterId: masterId, + primaryKey: primaryKey, }, }); @@ -271,21 +275,21 @@ async function testReplicateAttributesDeleteEmpty(sut, t, name) { const state = getState(); t.truthy(state.change); t.truthy(state.context); - t.is(state.context.params.masterId, masterId); + t.is(state.context.params.primaryKey, primaryKey); } // Assert that attributes get replicated to detail documents await assertQuerySizeEventually( db .collection('detail1') - .where('tempId', '==', masterId) + .where('tempId', '==', primaryKey) .where('foreignDetail1', '==', 'foreign_detail_1'), 0 ); await assertQuerySizeEventually( db .collection('detail1') - .where('tempId', '==', masterId) + .where('tempId', '==', primaryKey) .where('foreignDetail2', '==', 'after3'), 1 ); @@ -293,6 +297,75 @@ async function testReplicateAttributesDeleteEmpty(sut, t, name) { await t.pass(); } +async function testReplicateMissingSourceCollectionKey(sut, t, name) { + // Create some docs referencing master doc + const randomId = makeid(); + await db.collection('detail1').add({ + tempId: randomId, + foreignDetail1: 'foreign_detail_1', + foreignDetail2: 'foreign_detail_2', + }); + + // Trigger function to delete references + const beforeSnap = fft.firestore.makeDocumentSnapshot( + { + masterDetail1: 'after1', + masterDetail2: 'after2', + }, + `master/${randomId}` + ); + const afterSnap = fft.firestore.makeDocumentSnapshot( + { + masterDetail2: 'after3', + }, + `master/${randomId}` + ); + const change = fft.makeChange(beforeSnap, afterSnap); + const wrapped = fft.wrap(sut.replicateReferencesWithMissingKey); + setState({ + snap: null, + context: null, + }); + + const error = await t.throwsAsync(async () => { + await wrapped(change, { + params: { + randomId: randomId, + }, + }); + }); + + t.is( + error.message, + 'integrify: Missing a primary key [masterId] in the source params' + ); + + // Assert pre-hook was called (only for rules-in-situ) + if (name === 'rules-in-situ') { + const state = getState(); + t.is(state.snap, null); + t.is(state.context, null); + } + + // Assert referencing docs were not deleted + await assertQuerySizeEventually( + db + .collection('detail1') + .where('tempId', '==', randomId) + .where('foreignDetail1', '==', 'foreign_detail_1'), + 1 + ); + await assertQuerySizeEventually( + db + .collection('detail1') + .where('tempId', '==', randomId) + .where('foreignDetail2', '==', 'foreign_detail_2'), + 1 + ); + + t.pass(); +} + async function testDeleteReferences(sut, t, name) { // Create some docs referencing master doc const masterId = makeid(); From 091f54ecdd136c3a04f97ee297538ff673519ee2 Mon Sep 17 00:00:00 2001 From: Michael Richardson Date: Thu, 23 Jul 2020 10:36:36 +0100 Subject: [PATCH 4/9] feat(replicateAttributes): Add optional primary key --- src/rules/replicateAttributes.ts | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/rules/replicateAttributes.ts b/src/rules/replicateAttributes.ts index 94aeec5..66045de 100644 --- a/src/rules/replicateAttributes.ts +++ b/src/rules/replicateAttributes.ts @@ -1,4 +1,4 @@ -import { Config, Rule } from '../common'; +import { Config, Rule, getPrimaryKey } from '../common'; import { firestore } from 'firebase-admin'; const FieldValue = firestore.FieldValue; @@ -43,6 +43,11 @@ export function integrifyReplicateAttributes( }); }); + const { hasPrimaryKey, primaryKey } = getPrimaryKey(rule.source.collection); + if (!hasPrimaryKey) { + rule.source.collection = `${rule.source.collection}/{${primaryKey}}`; + } + // Create map of master attributes to track for replication const trackedMasterAttributes = {}; rule.targets.forEach(target => { @@ -52,12 +57,18 @@ export function integrifyReplicateAttributes( }); return functions.firestore - .document(`${rule.source.collection}/{masterId}`) + .document(rule.source.collection) .onUpdate((change, context) => { - const masterId = context.params.masterId; + // Get the last {...} in the source collection + const primaryKeyValue = context.params[primaryKey]; + if (!primaryKeyValue) { + throw new Error( + `integrify: Missing a primary key [${primaryKey}] in the source params` + ); + } const newValue = change.after.data(); console.log( - `integrify: Detected update in [${rule.source.collection}], id [${masterId}], new value:`, + `integrify: Detected update in [${rule.source.collection}], id [${primaryKeyValue}], new value:`, newValue ); @@ -124,7 +135,7 @@ export function integrifyReplicateAttributes( } promises.push( whereable - .where(target.foreignKey, '==', masterId) + .where(target.foreignKey, '==', primaryKeyValue) .get() .then(detailDocs => { detailDocs.forEach(detailDoc => { From ddc065087786624b117355d25737813e9df9e4e2 Mon Sep 17 00:00:00 2001 From: Michael Richardson Date: Thu, 23 Jul 2020 11:07:40 +0100 Subject: [PATCH 5/9] fix(readme): Update readme --- README.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 2592b9f..73fa8ec 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,11 @@ integrify({ config: { functions, db } }); module.exports.replicateMasterToDetail = integrify({ rule: 'REPLICATE_ATTRIBUTES', source: { - collection: 'master', + source: { + collection: 'master', // <-- This will append {masterId} + // OR + collection: 'master/{masterId}', // <-- Can be any string as in Firebase + }, }, targets: [ { @@ -35,6 +39,7 @@ module.exports.replicateMasterToDetail = integrify({ masterField1: 'detail1Field1', masterField2: 'detail1Field2', }, + deleteMissing: true, // Optional: Delete missing fields on update, defaults to false }, { collection: 'detail2', From e3340ce3ef902e5def1b506bbdc05caad54c4895 Mon Sep 17 00:00:00 2001 From: Michael Richardson Date: Thu, 23 Jul 2020 11:35:56 +0100 Subject: [PATCH 6/9] fix(readme): Update readme --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index 73fa8ec..eff78b1 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,6 @@ module.exports.replicateMasterToDetail = integrify({ masterField1: 'detail1Field1', masterField2: 'detail1Field2', }, - deleteMissing: true, // Optional: Delete missing fields on update, defaults to false }, { collection: 'detail2', From 4f2d6772532b8f5f9599d7d87e46259c447359b1 Mon Sep 17 00:00:00 2001 From: Michael Richardson Date: Thu, 23 Jul 2020 11:38:35 +0100 Subject: [PATCH 7/9] fix(replicateAttributes): Always delete missing fields --- README.md | 2 +- src/rules/replicateAttributes.ts | 16 ++-------------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index eff78b1..420b51a 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ module.exports.replicateMasterToDetail = integrify({ collection: 'detail1', foreignKey: 'masterId', attributeMapping: { - masterField1: 'detail1Field1', + masterField1: 'detail1Field1', // If an field is missing after the update, the field will be deleted masterField2: 'detail1Field2', }, }, diff --git a/src/rules/replicateAttributes.ts b/src/rules/replicateAttributes.ts index 66045de..e6330e1 100644 --- a/src/rules/replicateAttributes.ts +++ b/src/rules/replicateAttributes.ts @@ -13,7 +13,6 @@ export interface ReplicateAttributesRule extends Rule { [sourceAttribute: string]: string; }; isCollectionGroup?: boolean; - deleteMissing?: boolean; }[]; hooks?: { pre?: Function; @@ -99,23 +98,12 @@ export function integrifyReplicateAttributes( rule.targets.forEach(target => { const targetCollection = target.collection; const update = {}; - let shouldDelete = false; - - if (target.deleteMissing) { - shouldDelete = target.deleteMissing; - } // Create "update" mapping each changed attribute from source => target, // if delete is set delete field Object.keys(target.attributeMapping).forEach(changedAttribute => { - if (newValue[changedAttribute]) { - update[target.attributeMapping[changedAttribute]] = - newValue[changedAttribute]; - } else if (shouldDelete) { - update[ - target.attributeMapping[changedAttribute] - ] = FieldValue.delete(); - } + update[target.attributeMapping[changedAttribute]] = + newValue[changedAttribute] || FieldValue.delete(); }); console.log( From 7b12ec76fd5324b9e32f417f068fd39ea6e53d51 Mon Sep 17 00:00:00 2001 From: Michael Richardson Date: Thu, 23 Jul 2020 11:39:40 +0100 Subject: [PATCH 8/9] fix(replicateAttributes): Remove deleteMissing flag --- test/functions/index.js | 1 - test/functions/integrify.rules.js | 1 - 2 files changed, 2 deletions(-) diff --git a/test/functions/index.js b/test/functions/index.js index 48bcb23..e63dc1d 100644 --- a/test/functions/index.js +++ b/test/functions/index.js @@ -52,7 +52,6 @@ module.exports.replicateMasterDeleteWhenEmpty = integrify({ masterDetail1: 'foreignDetail1', masterDetail2: 'foreignDetail2', }, - deleteMissing: true, }, ], hooks: { diff --git a/test/functions/integrify.rules.js b/test/functions/integrify.rules.js index 5f73532..b9ae26c 100644 --- a/test/functions/integrify.rules.js +++ b/test/functions/integrify.rules.js @@ -41,7 +41,6 @@ module.exports = [ masterDetail1: 'foreignDetail1', masterDetail2: 'foreignDetail2', }, - deleteMissing: true, }, ], }, From 74fb12161d60017285f1b4a1e340bf05e3e72f58 Mon Sep 17 00:00:00 2001 From: Michael Richardson Date: Thu, 23 Jul 2020 12:05:04 +0100 Subject: [PATCH 9/9] chore(package): Updated to version 4.0.0 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 782dc19..131b0c9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "integrify", - "version": "3.0.1", + "version": "4.0.0", "description": "Enforce referential integrity in Firestore using Cloud Functions", "keywords": [ "firebase",