From 13bbec18757824bdab82d0bc18dbfed0344fee42 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Fri, 29 Mar 2024 19:22:01 -0700 Subject: [PATCH] refactor(daemon): Fix naming issues in multimap interface Addresses some naming issues in the multimap's interface and adds missing test cases. --- packages/daemon/src/multimap.js | 25 ++-- packages/daemon/src/pet-store.js | 18 +-- packages/daemon/src/types.d.ts | 24 ++-- packages/daemon/test/test-multimap.js | 159 ++++++++++++++++---------- 4 files changed, 130 insertions(+), 96 deletions(-) diff --git a/packages/daemon/src/multimap.js b/packages/daemon/src/multimap.js index 07749a57e1..08e30b2abd 100644 --- a/packages/daemon/src/multimap.js +++ b/packages/daemon/src/multimap.js @@ -35,7 +35,7 @@ const internalMakeMultimap = mapConstructor => { get: key => map.get(key)?.keys().next().value, - getAll: key => Array.from(map.get(key) ?? []), + getAllFor: key => Array.from(map.get(key) ?? []), }; }; @@ -58,12 +58,9 @@ export const makeWeakMultimap = () => { */ export const makeBidirectionalMultimap = () => { /** - * @type {import('./types.js').Multimap} + * @type {import('./types.js').Multimap} */ const keyForValues = internalMakeMultimap(Map); - /** - * @type {Map} - */ const valueForKey = new Map(); return { @@ -79,8 +76,8 @@ export const makeBidirectionalMultimap = () => { ); } - valueForKey.set(value, key); keyForValues.add(key, value); + valueForKey.set(value, key); }, delete: (key, value) => { @@ -89,24 +86,20 @@ export const makeBidirectionalMultimap = () => { }, deleteAll: key => { - for (const value of keyForValues.getAll(key)) { + for (const value of keyForValues.getAllFor(key)) { valueForKey.delete(value); } return keyForValues.deleteAll(key); }, - hasValue: value => { - return valueForKey.has(value); - }, + hasValue: value => valueForKey.has(value), - get: value => valueForKey.get(value), + get: key => keyForValues.get(key), - getValue: key => keyForValues.get(key), + getKey: value => valueForKey.get(value), - getAllValues: () => { - return [...valueForKey.keys()]; - }, + getAll: () => [...valueForKey.keys()], - getAllValuesFor: key => keyForValues.getAll(key), + getAllFor: key => keyForValues.getAllFor(key), }; }; diff --git a/packages/daemon/src/pet-store.js b/packages/daemon/src/pet-store.js index fdeeca3f22..732692ef4d 100644 --- a/packages/daemon/src/pet-store.js +++ b/packages/daemon/src/pet-store.js @@ -51,7 +51,7 @@ export const makePetStoreMaker = (filePowers, locator) => { /** @type {import('./types.js').PetStore['identifyLocal']} */ const identifyLocal = petName => { assertValidName(petName); - return idsToPetNames.get(petName); + return idsToPetNames.getKey(petName); }; /** @type {import('./types.js').PetStore['write']} */ @@ -60,7 +60,7 @@ export const makePetStoreMaker = (filePowers, locator) => { assertValidId(formulaIdentifier); if (idsToPetNames.hasValue(petName)) { - const oldFormulaIdentifier = idsToPetNames.get(petName); + const oldFormulaIdentifier = idsToPetNames.getKey(petName); if (oldFormulaIdentifier === formulaIdentifier) { return; } @@ -89,7 +89,7 @@ export const makePetStoreMaker = (filePowers, locator) => { * @returns {import('./types.js').IdRecord} */ const formulaIdentifierRecordForName = petName => { - const formulaIdentifier = idsToPetNames.get(petName); + const formulaIdentifier = idsToPetNames.getKey(petName); if (formulaIdentifier === undefined) { throw new Error(`Formula does not exist for pet name ${q(petName)}`); } @@ -97,12 +97,12 @@ export const makePetStoreMaker = (filePowers, locator) => { }; /** @type {import('./types.js').PetStore['list']} */ - const list = () => harden(idsToPetNames.getAllValues().sort()); + const list = () => harden(idsToPetNames.getAll().sort()); /** @type {import('./types.js').PetStore['follow']} */ const follow = async function* currentAndSubsequentNames() { const changes = nameChangesTopic.subscribe(); - for (const name of idsToPetNames.getAllValues().sort()) { + for (const name of idsToPetNames.getAll().sort()) { const formulaIdentifierRecord = formulaIdentifierRecordForName(name); yield /** @type {{ add: string, value: import('./types.js').IdRecord }} */ ({ add: name, @@ -115,7 +115,7 @@ export const makePetStoreMaker = (filePowers, locator) => { /** @type {import('./types.js').PetStore['remove']} */ const remove = async petName => { assertValidName(petName); - const formulaIdentifier = idsToPetNames.get(petName); + const formulaIdentifier = idsToPetNames.getKey(petName); if (formulaIdentifier === undefined) { throw new Error( `Formula does not exist for pet name ${JSON.stringify(petName)}`, @@ -138,8 +138,8 @@ export const makePetStoreMaker = (filePowers, locator) => { if (fromName === toName) { return; } - const formulaIdentifier = idsToPetNames.get(fromName); - const overwrittenId = idsToPetNames.get(toName); + const formulaIdentifier = idsToPetNames.getKey(fromName); + const overwrittenId = idsToPetNames.getKey(toName); if (formulaIdentifier === undefined) { throw new Error( `Formula does not exist for pet name ${JSON.stringify(fromName)}`, @@ -174,7 +174,7 @@ export const makePetStoreMaker = (filePowers, locator) => { /** @type {import('./types.js').PetStore['reverseIdentify']} */ const reverseIdentify = formulaIdentifier => { assertValidId(formulaIdentifier); - const formulaPetNames = idsToPetNames.getAllValuesFor(formulaIdentifier); + const formulaPetNames = idsToPetNames.getAllFor(formulaIdentifier); if (formulaPetNames === undefined) { return harden([]); } diff --git a/packages/daemon/src/types.d.ts b/packages/daemon/src/types.d.ts index 25c2bd22ab..db52136bab 100644 --- a/packages/daemon/src/types.d.ts +++ b/packages/daemon/src/types.d.ts @@ -900,7 +900,7 @@ export type Multimap = { * @param key - The key whose values to retrieve. * @returns An array of all values associated with the key. */ - getAll(key: K): V[]; + getAllFor(key: K): V[]; }; /** @@ -924,37 +924,37 @@ export type BidirectionalMultimap = { delete(key: K, value: V): boolean; /** - * @param key - The key whose values to delete + * @param key - The key whose values to delete. * @returns `true` if the key was found and its values were deleted, `false` otherwise. */ deleteAll(key: K): boolean; /** - * @param value - The value whose existence to check for. - * @returns `true` if the value was found and `false` otherwise. + * @param value - The value whose presence to check for. + * @returns `true` if the value is present and `false` otherwise. */ hasValue(value: V): boolean; /** - * @param value - The value whose key to retrieve - * @returns The key associated with the value. + * @param key - The key whose first value to retrieve. + * @returns The first value associated with the key. */ - get(value: V): K | undefined; + get(key: K): V | undefined; /** - * @param key - The key whose first value to retrieve - * @returns The first value associated with the key. + * @param value - The value whose key to retrieve. + * @returns The key associated with the value. */ - getValue(key: K): V | undefined; + getKey(value: V): K | undefined; /** * @returns An array of all values, for all keys. */ - getAllValues(): V[]; + getAll(): V[]; /** * @param key - The key whose values to retrieve. * @returns An array of all values associated with the key. */ - getAllValuesFor(key: K): V[]; + getAllFor(key: K): V[]; }; diff --git a/packages/daemon/test/test-multimap.js b/packages/daemon/test/test-multimap.js index d6c8d2b21a..2dd75fc781 100644 --- a/packages/daemon/test/test-multimap.js +++ b/packages/daemon/test/test-multimap.js @@ -8,9 +8,9 @@ import { [ [makeMultimap, 'multimap'], - [makeWeakMultimap, 'weakMultimap'], + [makeWeakMultimap, 'weak multimap'], ].forEach(([multimapConstructor, mapName]) => { - test(`${mapName}: add and get`, t => { + test(`${mapName}: add`, t => { const multimap = multimapConstructor(); const key = {}; const value = 'foo'; @@ -23,7 +23,7 @@ import { t.is(multimap.get(key), value); }); - test(`${mapName}: add and get with multiple keys`, t => { + test(`${mapName}: get`, t => { const multimap = multimapConstructor(); const key1 = {}; const key2 = {}; @@ -35,25 +35,28 @@ import { multimap.add(key2, value1); t.is(multimap.get(key1), value1); - t.deepEqual(multimap.getAll(key1), [value1, value2]); + t.deepEqual(multimap.getAllFor(key1), [value1, value2]); t.is(multimap.get(key2), value1); - t.deepEqual(multimap.getAll(key2), [value1]); + t.deepEqual(multimap.getAllFor(key2), [value1]); }); test(`${mapName}: getAll`, t => { const multimap = multimapConstructor(); - const key = {}; + const key1 = {}; + const key2 = {}; const value1 = 'foo'; const value2 = 'bar'; - multimap.add(key, value1); - multimap.add(key, value2); - t.deepEqual(multimap.getAll(key), [value1, value2]); + multimap.add(key1, value1); + multimap.add(key1, value2); + t.deepEqual(multimap.getAllFor(key1), [value1, value2]); + t.deepEqual(multimap.getAllFor(key2), []); // Adding a value for a key should be idempotent. - multimap.add(key, value1); - multimap.add(key, value2); - t.deepEqual(multimap.getAll(key), [value1, value2]); + multimap.add(key1, value1); + multimap.add(key1, value2); + t.deepEqual(multimap.getAllFor(key1), [value1, value2]); + t.deepEqual(multimap.getAllFor(key2), []); }); test(`${mapName}: delete`, t => { @@ -81,7 +84,7 @@ import { multimap.add(key, value1); multimap.add(key, value2); - t.deepEqual(multimap.getAll(key), [value1, value2]); + t.deepEqual(multimap.getAllFor(key), [value1, value2]); t.is(multimap.deleteAll(key), true); t.is(multimap.get(key), undefined); @@ -91,120 +94,135 @@ import { }); }); -test('bimap: add and getValue', t => { +test('multi-bimap: add', t => { const bimap = makeBidirectionalMultimap(); const key = 'foo'; const value = {}; bimap.add(key, value); - t.is(bimap.getValue(key), value); + t.is(bimap.get(key), value); + t.is(bimap.getKey(value), key); // Adding a value for the same key should be idempotent. bimap.add(key, value); - t.is(bimap.getValue(key), value); + t.is(bimap.get(key), value); + t.is(bimap.getKey(value), key); }); -test('bimap: getAllValuesFor', t => { +test('multi-bimap: get', t => { const bimap = makeBidirectionalMultimap(); - const key = 'foo'; + const key1 = 'foo'; + const key2 = 'bar'; + const key3 = 'baz'; const value1 = {}; const value2 = {}; - bimap.add(key, value1); - bimap.add(key, value2); - t.deepEqual(bimap.getAllValuesFor(key), [value1, value2]); + bimap.add(key1, value1); + bimap.add(key2, value2); - // Adding a value for the same key should be idempotent. - bimap.add(key, value1); - bimap.add(key, value2); - t.deepEqual(bimap.getAllValuesFor(key), [value1, value2]); + t.is(bimap.get(key1), value1); + t.is(bimap.get(key2), value2); + t.is(bimap.get(key3), undefined); }); -test('bimap: hasValue', t => { +test('multi-bimap: get (multiple values per key)', t => { const bimap = makeBidirectionalMultimap(); const key1 = 'foo'; const key2 = 'bar'; const value1 = {}; const value2 = {}; const value3 = {}; - const value4 = {}; bimap.add(key1, value1); bimap.add(key1, value2); bimap.add(key2, value3); - t.is(bimap.hasValue(value1), true); + t.is(bimap.get(key1), value1); + t.is(bimap.get(key2), value3); t.is(bimap.hasValue(value2), true); - t.is(bimap.hasValue(value3), true); - t.is(bimap.hasValue(value4), false); }); -test('bimap: add and get', t => { +test('multi-bimap: getAll', t => { const bimap = makeBidirectionalMultimap(); - const key = 'foo'; - const value = {}; + const key1 = 'foo'; + const value1 = {}; + const value2 = {}; - bimap.add(key, value); - t.is(bimap.get(value), key); + t.deepEqual(bimap.getAll(), []); - // Adding a value for the same key should be idempotent. - bimap.add(key, value); - t.is(bimap.get(value), key); + bimap.add(key1, value1); + bimap.add(key1, value2); + + t.deepEqual(bimap.getAll(), [value1, value2]); }); -test('bimap: add and get with multiple values', t => { +test('multi-bimap: getAllFor', t => { const bimap = makeBidirectionalMultimap(); const key1 = 'foo'; + const key2 = 'bar'; const value1 = {}; const value2 = {}; bimap.add(key1, value1); bimap.add(key1, value2); + t.deepEqual(bimap.getAllFor(key1), [value1, value2]); + t.deepEqual(bimap.getAllFor(key2), []); - t.is(bimap.get(value1), key1); - t.is(bimap.get(value2), key1); + // Adding a value for the same key should be idempotent. + bimap.add(key1, value1); + bimap.add(key1, value2); + t.deepEqual(bimap.getAllFor(key1), [value1, value2]); + t.deepEqual(bimap.getAllFor(key2), []); }); -test('bimap: key remapping', t => { +test('multi-bimap: getKey', t => { const bimap = makeBidirectionalMultimap(); const key1 = 'foo'; - const key2 = 'bar'; const value1 = {}; const value2 = {}; + const value3 = {}; bimap.add(key1, value1); bimap.add(key1, value2); - t.is(bimap.get(value1), key1); - t.is(bimap.get(value2), key1); - t.throws(() => bimap.add(key2, value1), { - message: `May not remap key "foo" of existing value to new key "bar". Delete the original mapping first.`, - }); + t.is(bimap.getKey(value1), key1); + t.is(bimap.getKey(value2), key1); + t.is(bimap.getKey(value3), undefined); +}); - bimap.delete(key1, value2); +test('multi-bimap: hasValue', t => { + const bimap = makeBidirectionalMultimap(); + const key1 = 'foo'; + const key2 = 'bar'; + const value1 = {}; + const value2 = {}; + const value3 = {}; + + bimap.add(key1, value1); bimap.add(key2, value2); - t.is(bimap.get(value1), key1); - t.is(bimap.get(value2), key2); + t.is(bimap.hasValue(value1), true); + t.is(bimap.hasValue(value2), true); + t.is(bimap.hasValue(value3), false); }); -test('bimap: delete', t => { +test('multi-bimap: delete', t => { const bimap = makeBidirectionalMultimap(); const key = 'foo'; const value = {}; bimap.add(key, value); - t.is(bimap.getValue(key), value); + t.is(bimap.get(key), value); t.is(bimap.delete(key, value), true); - t.is(bimap.getValue(key), undefined); + t.is(bimap.get(key), undefined); // Deleting should be idempotent. t.is(bimap.delete(key, value), false); - t.is(bimap.getValue(key), undefined); + t.is(bimap.get(key), undefined); }); -test('bimap: deleteAll', t => { +test('multi-bimap: deleteAll', t => { const bimap = makeBidirectionalMultimap(); const key = 'foo'; const value1 = {}; @@ -213,11 +231,34 @@ test('bimap: deleteAll', t => { bimap.add(key, value1); bimap.add(key, value2); - t.deepEqual(bimap.getAllValuesFor(key), [value1, value2]); + t.deepEqual(bimap.getAllFor(key), [value1, value2]); t.is(bimap.deleteAll(key), true); - t.is(bimap.getValue(key), undefined); + t.is(bimap.get(key), undefined); // Deleting should be idempotent. t.is(bimap.deleteAll(key), false); - t.is(bimap.getValue(key), undefined); + t.is(bimap.get(key), undefined); +}); + +test('multi-bimap: key remapping', t => { + const bimap = makeBidirectionalMultimap(); + const key1 = 'foo'; + const key2 = 'bar'; + const value1 = {}; + const value2 = {}; + + bimap.add(key1, value1); + bimap.add(key1, value2); + t.is(bimap.getKey(value1), key1); + t.is(bimap.getKey(value2), key1); + + t.throws(() => bimap.add(key2, value1), { + message: `May not remap key "foo" of existing value to new key "bar". Delete the original mapping first.`, + }); + + bimap.delete(key1, value2); + bimap.add(key2, value2); + + t.is(bimap.getKey(value1), key1); + t.is(bimap.getKey(value2), key2); });