From ed756b942a13fb8ca196429d26ea86880821746d Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Tue, 15 Aug 2023 14:54:55 -0600 Subject: [PATCH 1/5] feat!: component set components are now DecodableMaps --- src/collections/componentSet.ts | 90 +++++++++++++++++++++++++++++---- 1 file changed, 80 insertions(+), 10 deletions(-) diff --git a/src/collections/componentSet.ts b/src/collections/componentSet.ts index 6bb3e28130..62fdbf0f49 100644 --- a/src/collections/componentSet.ts +++ b/src/collections/componentSet.ts @@ -43,6 +43,78 @@ export type RetrieveSetOptions = Omit; const KEY_DELIMITER = '#'; +/** + * This is an extension of the Map class that treats keys as the same by matching first normally, + * then decoded. Decoding the key before comparing can solve some edge cases in component fullNames + * such as Layouts. See: https://github.com/forcedotcom/cli/issues/1683 + * + * Examples: + * + * Given a map with entries: + * ```javascript + * 'layout#Layout__Broker__c-v1%2E1 Broker Layout' : {...} + * 'layout#Layout__Broker__c-v9.2 Broker Layout' : {...} + * ``` + * + * `decodeableMap.has('layout#Layout__Broker__c-v1.1 Broker Layout')` --> returns `true` + * `decodeableMap.has('layout#Layout__Broker__c-v9%2E2 Broker Layout')` --> returns `true` + */ +class DecodeableMap extends Map { + /** + * boolean indicating whether an element with the specified key (matching decoded) exists or not. + */ + public has(key: K): boolean { + return super.has(key) || this.hasDecoded(key); + } + + /** + * Returns a specified element from the Map object. If the value that is associated to + * the provided key (matching decoded) is an object, then you will get a reference to + * that object and any change made to that object will effectively modify it inside the Map. + */ + public get(key: K): V | undefined { + return super.get(key) ?? this.getDecoded(key); + } + + /** + * Adds a new element with a specified key and value to the Map. If an element with the + * same key (matching decoded) already exists, the element will be updated. + */ + public set(key: K, value: V): this { + const sKey = this.getExistingKey(key) ?? key; + return super.set(sKey, value); + } + + /** + * true if an element in the Map existed (matching decoded) and has been removed, or false + * if the element does not exist. + */ + public delete(key: K): boolean { + const sKey = this.getExistingKey(key) ?? key; + return super.delete(sKey); + } + + // Returns true if the passed `key` matches an existing key entry when both keys are decoded. + private hasDecoded(key: string): boolean { + return !!this.getExistingKey(key); + } + + // Returns the value of an entry matching on decoded keys. + private getDecoded(key: string): V | undefined { + const existingKey = this.getExistingKey(key); + return existingKey ? super.get(existingKey) : undefined; + } + + // Returns the key as it is in the map, matching on decoded keys. + private getExistingKey(key: string): string | undefined { + for (const compKey of this.keys()) { + if (decodeURIComponent(compKey) === decodeURIComponent(key)) { + return compKey; + } + } + } +} + /** * A collection containing no duplicate metadata members (`fullName` and `type` pairs). `ComponentSets` * are a convenient way of constructing a unique collection of components to perform operations such as @@ -73,14 +145,14 @@ export class ComponentSet extends LazyCollection { public forceIgnoredPaths?: Set; private logger: Logger; private registry: RegistryAccess; - private components = new Map>(); + private components = new DecodeableMap>(); // internal component maps used by this.getObject() when building manifests. private destructiveComponents = { - [DestructiveChangesType.PRE]: new Map>(), - [DestructiveChangesType.POST]: new Map>(), + [DestructiveChangesType.PRE]: new DecodeableMap>(), + [DestructiveChangesType.POST]: new DecodeableMap>(), }; - private manifestComponents = new Map>(); + private manifestComponents = new DecodeableMap>(); private destructiveChangesType = DestructiveChangesType.POST; @@ -485,7 +557,7 @@ export class ComponentSet extends LazyCollection { public add(component: ComponentLike, deletionType?: DestructiveChangesType): void { const key = simpleKey(component); if (!this.components.has(key)) { - this.components.set(key, new Map()); + this.components.set(key, new DecodeableMap()); } if (!(component instanceof SourceComponent)) { @@ -502,12 +574,12 @@ export class ComponentSet extends LazyCollection { this.logger.debug(`Marking component for delete: ${component.fullName}`); const deletions = this.destructiveComponents[deletionType]; if (!deletions.has(key)) { - deletions.set(key, new Map()); + deletions.set(key, new DecodeableMap()); } deletions.get(key)?.set(sourceKey(component), component); } else { if (!this.manifestComponents.has(key)) { - this.manifestComponents.set(key, new Map()); + this.manifestComponents.set(key, new DecodeableMap()); } this.manifestComponents.get(key)?.set(sourceKey(component), component); } @@ -542,10 +614,8 @@ export class ComponentSet extends LazyCollection { * @returns `true` if the component is in the set */ public has(component: ComponentLike): boolean { - // Compare the component key as is and decoded. Decoding the key before comparing can solve some edge cases - // in component fullNames such as Layouts. See: https://github.com/forcedotcom/cli/issues/1683 const key = simpleKey(component); - if (this.components.has(key) || this.components.has(decodeURIComponent(key))) { + if (this.components.has(key)) { return true; } From 9d24d82273ae5bf42ed6ef265dd841537a18c563 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Tue, 15 Aug 2023 17:32:46 -0600 Subject: [PATCH 2/5] fix: move DecodeableMap to a new file --- src/collections/componentSet.ts | 77 ++----------------------------- src/collections/decodeableMap.ts | 78 ++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 74 deletions(-) create mode 100644 src/collections/decodeableMap.ts diff --git a/src/collections/componentSet.ts b/src/collections/componentSet.ts index 62fdbf0f49..bea09a5346 100644 --- a/src/collections/componentSet.ts +++ b/src/collections/componentSet.ts @@ -34,6 +34,7 @@ import { PackageTypeMembers, } from './types'; import { LazyCollection } from './lazyCollection'; +import { DecodeableMap } from './decodeableMap'; Messages.importMessagesDirectory(__dirname); const messages = Messages.loadMessages('@salesforce/source-deploy-retrieve', 'sdr'); @@ -43,78 +44,6 @@ export type RetrieveSetOptions = Omit; const KEY_DELIMITER = '#'; -/** - * This is an extension of the Map class that treats keys as the same by matching first normally, - * then decoded. Decoding the key before comparing can solve some edge cases in component fullNames - * such as Layouts. See: https://github.com/forcedotcom/cli/issues/1683 - * - * Examples: - * - * Given a map with entries: - * ```javascript - * 'layout#Layout__Broker__c-v1%2E1 Broker Layout' : {...} - * 'layout#Layout__Broker__c-v9.2 Broker Layout' : {...} - * ``` - * - * `decodeableMap.has('layout#Layout__Broker__c-v1.1 Broker Layout')` --> returns `true` - * `decodeableMap.has('layout#Layout__Broker__c-v9%2E2 Broker Layout')` --> returns `true` - */ -class DecodeableMap extends Map { - /** - * boolean indicating whether an element with the specified key (matching decoded) exists or not. - */ - public has(key: K): boolean { - return super.has(key) || this.hasDecoded(key); - } - - /** - * Returns a specified element from the Map object. If the value that is associated to - * the provided key (matching decoded) is an object, then you will get a reference to - * that object and any change made to that object will effectively modify it inside the Map. - */ - public get(key: K): V | undefined { - return super.get(key) ?? this.getDecoded(key); - } - - /** - * Adds a new element with a specified key and value to the Map. If an element with the - * same key (matching decoded) already exists, the element will be updated. - */ - public set(key: K, value: V): this { - const sKey = this.getExistingKey(key) ?? key; - return super.set(sKey, value); - } - - /** - * true if an element in the Map existed (matching decoded) and has been removed, or false - * if the element does not exist. - */ - public delete(key: K): boolean { - const sKey = this.getExistingKey(key) ?? key; - return super.delete(sKey); - } - - // Returns true if the passed `key` matches an existing key entry when both keys are decoded. - private hasDecoded(key: string): boolean { - return !!this.getExistingKey(key); - } - - // Returns the value of an entry matching on decoded keys. - private getDecoded(key: string): V | undefined { - const existingKey = this.getExistingKey(key); - return existingKey ? super.get(existingKey) : undefined; - } - - // Returns the key as it is in the map, matching on decoded keys. - private getExistingKey(key: string): string | undefined { - for (const compKey of this.keys()) { - if (decodeURIComponent(compKey) === decodeURIComponent(key)) { - return compKey; - } - } - } -} - /** * A collection containing no duplicate metadata members (`fullName` and `type` pairs). `ComponentSets` * are a convenient way of constructing a unique collection of components to perform operations such as @@ -184,11 +113,11 @@ export class ComponentSet extends LazyCollection { return size; } - public get destructiveChangesPre(): Map> { + public get destructiveChangesPre(): DecodeableMap> { return this.destructiveComponents[DestructiveChangesType.PRE]; } - public get destructiveChangesPost(): Map> { + public get destructiveChangesPost(): DecodeableMap> { return this.destructiveComponents[DestructiveChangesType.POST]; } diff --git a/src/collections/decodeableMap.ts b/src/collections/decodeableMap.ts new file mode 100644 index 0000000000..86edacb720 --- /dev/null +++ b/src/collections/decodeableMap.ts @@ -0,0 +1,78 @@ +/* + * Copyright (c) 2023, salesforce.com, inc. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ + +/** + * This is an extension of the Map class that treats keys as the same by matching first normally, + * then decoded. Decoding the key before comparing can solve some edge cases in component fullNames + * such as Layouts. See: https://github.com/forcedotcom/cli/issues/1683 + * + * Examples: + * + * Given a map with entries: + * ```javascript + * 'layout#Layout__Broker__c-v1%2E1 Broker Layout' : {...} + * 'layout#Layout__Broker__c-v9.2 Broker Layout' : {...} + * ``` + * + * `decodeableMap.has('layout#Layout__Broker__c-v1.1 Broker Layout')` --> returns `true` + * `decodeableMap.has('layout#Layout__Broker__c-v9%2E2 Broker Layout')` --> returns `true` + */ +export class DecodeableMap extends Map { + /** + * boolean indicating whether an element with the specified key (matching decoded) exists or not. + */ + public has(key: K): boolean { + return super.has(key) || this.hasDecoded(key); + } + + /** + * Returns a specified element from the Map object. If the value that is associated to + * the provided key (matching decoded) is an object, then you will get a reference to + * that object and any change made to that object will effectively modify it inside the Map. + */ + public get(key: K): V | undefined { + return super.get(key) ?? this.getDecoded(key); + } + + /** + * Adds a new element with a specified key and value to the Map. If an element with the + * same key (matching decoded) already exists, the element will be updated. + */ + public set(key: K, value: V): this { + const sKey = this.getExistingKey(key) ?? key; + return super.set(sKey, value); + } + + /** + * true if an element in the Map existed (matching decoded) and has been removed, or false + * if the element does not exist. + */ + public delete(key: K): boolean { + const sKey = this.getExistingKey(key) ?? key; + return super.delete(sKey); + } + + // Returns true if the passed `key` matches an existing key entry when both keys are decoded. + private hasDecoded(key: string): boolean { + return !!this.getExistingKey(key); + } + + // Returns the value of an entry matching on decoded keys. + private getDecoded(key: string): V | undefined { + const existingKey = this.getExistingKey(key); + return existingKey ? super.get(existingKey) : undefined; + } + + // Returns the key as it is in the map, matching on decoded keys. + private getExistingKey(key: string): string | undefined { + for (const compKey of this.keys()) { + if (decodeURIComponent(compKey) === decodeURIComponent(key)) { + return compKey; + } + } + } +} From acec124a03dd5ceb9d77d0d9ba78c54ac1be0581 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Wed, 16 Aug 2023 15:18:33 -0600 Subject: [PATCH 3/5] chore: add tests for DecodeableMap --- test/collections/decodeableMap.test.ts | 168 +++++++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 test/collections/decodeableMap.test.ts diff --git a/test/collections/decodeableMap.test.ts b/test/collections/decodeableMap.test.ts new file mode 100644 index 0000000000..743f3c91b4 --- /dev/null +++ b/test/collections/decodeableMap.test.ts @@ -0,0 +1,168 @@ +/* + * Copyright (c) 2020, salesforce.com, inc. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ +import { expect } from 'chai'; +import * as sinon from 'sinon'; +import { DecodeableMap } from '../../src/collections/decodeableMap'; + +describe('DecodeableMap', () => { + let dMap: DecodeableMap; + const ENCODED_KEY = 'encodedKey'; + const DECODED_KEY = 'decodedKey'; + + const sandbox = sinon.createSandbox(); + let hasDecodedSpy: sinon.SinonSpy; + let getDecodedSpy: sinon.SinonSpy; + let hasMapSpy: sinon.SinonSpy; + let getMapSpy: sinon.SinonSpy; + let setMapSpy: sinon.SinonSpy; + let deleteMapSpy: sinon.SinonSpy; + + beforeEach(() => { + dMap = new DecodeableMap([ + ['Layout-v1%2E1 Layout', ENCODED_KEY], + ['Layout-v9.2 Layout', DECODED_KEY], + ]); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + hasDecodedSpy = sandbox.spy(dMap, 'hasDecoded' as any); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getDecodedSpy = sandbox.spy(dMap, 'getDecoded' as any); + hasMapSpy = sandbox.spy(Map.prototype, 'has'); + getMapSpy = sandbox.spy(Map.prototype, 'get'); + setMapSpy = sandbox.spy(Map.prototype, 'set'); + deleteMapSpy = sandbox.spy(Map.prototype, 'delete'); + }); + + afterEach(() => { + sandbox.restore(); + }); + + describe('has()', () => { + it('should match on exact key without decoding', () => { + expect(dMap.has('Layout-v1%2E1 Layout')).to.be.true; + expect(hasMapSpy.called).to.be.true; + expect(hasDecodedSpy.called).to.be.false; + }); + + it('should match encoded key with decoded value', () => { + expect(dMap.has('Layout-v1.1 Layout')).to.be.true; + expect(hasMapSpy.called).to.be.true; + expect(hasDecodedSpy.called).to.be.true; + }); + + it('should match decoded key with encoded value', () => { + expect(dMap.has('Layout-v9%2E2 Layout')).to.be.true; + expect(hasMapSpy.called).to.be.true; + expect(hasDecodedSpy.called).to.be.true; + }); + + it('should not match on no existing key', () => { + expect(dMap.has('Layout-MISSING Layout')).to.be.false; + expect(hasMapSpy.called).to.be.true; + expect(hasDecodedSpy.called).to.be.true; + }); + }); + + describe('get()', () => { + it('should get value with exact key without decoding', () => { + expect(dMap.get('Layout-v1%2E1 Layout')).to.equal(ENCODED_KEY); + expect(getMapSpy.calledOnce).to.be.true; + expect(getDecodedSpy.called).to.be.false; + }); + + it('should get value of encoded key using decoded key', () => { + expect(dMap.get('Layout-v1.1 Layout')).to.equal(ENCODED_KEY); + expect(getMapSpy.calledTwice).to.be.true; + expect(getDecodedSpy.calledOnce).to.be.true; + }); + + it('should get value of decoded key using encoded key', () => { + expect(dMap.get('Layout-v9%2E2 Layout')).to.equal(DECODED_KEY); + expect(getMapSpy.calledTwice).to.be.true; + expect(getDecodedSpy.calledOnce).to.be.true; + }); + + it('should return undefined on no existing key', () => { + expect(dMap.get('Layout-MISSING Layout')).to.be.undefined; + expect(getMapSpy.calledOnce).to.be.true; + expect(getDecodedSpy.called).to.be.true; + }); + }); + + describe('set()', () => { + const NEW_VALUE = 'new value from set'; + + it('should set value with exact key', () => { + expect(dMap.set('Layout-v1%2E1 Layout', NEW_VALUE)).to.equal(dMap); + expect(setMapSpy.called).to.be.true; + expect(setMapSpy.lastCall.args[0]).to.equal('Layout-v1%2E1 Layout'); + expect(setMapSpy.lastCall.args[1]).to.equal(NEW_VALUE); + expect(dMap.size).to.equal(2); + expect(dMap.get('Layout-v1%2E1 Layout')).to.equal(NEW_VALUE); + }); + + it('should set value of encoded key using decoded key', () => { + expect(dMap.set('Layout-v1.1 Layout', NEW_VALUE)).to.equal(dMap); + expect(setMapSpy.called).to.be.true; + expect(setMapSpy.lastCall.args[0]).to.equal('Layout-v1%2E1 Layout'); + expect(setMapSpy.lastCall.args[1]).to.equal(NEW_VALUE); + expect(dMap.size).to.equal(2); + expect(dMap.get('Layout-v1%2E1 Layout')).to.equal(NEW_VALUE); + }); + + it('should set value of decoded key using encoded key', () => { + expect(dMap.set('Layout-v9%2E2 Layout', NEW_VALUE)).to.equal(dMap); + expect(setMapSpy.called).to.be.true; + expect(setMapSpy.lastCall.args[0]).to.equal('Layout-v9.2 Layout'); + expect(setMapSpy.lastCall.args[1]).to.equal(NEW_VALUE); + expect(dMap.size).to.equal(2); + expect(dMap.get('Layout-v9.2 Layout')).to.equal(NEW_VALUE); + }); + + it('should set new entry on no existing key', () => { + expect(dMap.set('Layout-MISSING Layout', NEW_VALUE)).to.equal(dMap); + expect(setMapSpy.called).to.be.true; + expect(setMapSpy.lastCall.args[0]).to.equal('Layout-MISSING Layout'); + expect(setMapSpy.lastCall.args[1]).to.equal(NEW_VALUE); + expect(dMap.size).to.equal(3); + expect(dMap.get('Layout-MISSING Layout')).to.equal(NEW_VALUE); + }); + }); + + describe('delete()', () => { + it('should delete using exact key', () => { + expect(dMap.delete('Layout-v1%2E1 Layout')).to.be.true; + expect(deleteMapSpy.calledOnce).to.be.true; + expect(deleteMapSpy.firstCall.args[0]).to.equal('Layout-v1%2E1 Layout'); + expect(dMap.size).to.equal(1); + expect(dMap.has('Layout-v1%2E1 Layout')).to.be.false; + }); + + it('should delete the encoded key using decoded value', () => { + expect(dMap.delete('Layout-v1.1 Layout')).to.be.true; + expect(deleteMapSpy.calledOnce).to.be.true; + expect(deleteMapSpy.firstCall.args[0]).to.equal('Layout-v1%2E1 Layout'); + expect(dMap.size).to.equal(1); + expect(dMap.has('Layout-v1.1 Layout')).to.be.false; + }); + + it('should delete the decoded key using encoded value', () => { + expect(dMap.delete('Layout-v9%2E2 Layout')).to.be.true; + expect(deleteMapSpy.calledOnce).to.be.true; + expect(deleteMapSpy.firstCall.args[0]).to.equal('Layout-v9.2 Layout'); + expect(dMap.size).to.equal(1); + expect(dMap.has('Layout-v9%2E2 Layout')).to.be.false; + }); + + it('should not delete on no existing key', () => { + expect(dMap.delete('Layout-MISSING Layout')).to.be.false; + expect(deleteMapSpy.calledOnce).to.be.true; + expect(deleteMapSpy.firstCall.args[0]).to.equal('Layout-MISSING Layout'); + expect(dMap.size).to.equal(2); + expect(dMap.has('Layout-MISSING Layout')).to.be.false; + }); + }); +}); From ec7ec72165ebee3543ce8e18edbfb27673666fbd Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Wed, 11 Oct 2023 11:30:16 -0600 Subject: [PATCH 4/5] fix: use internal map of decoded to encoded keys --- src/collections/decodeableMap.ts | 80 +++++---- test/collections/decodeableMap.test.ts | 221 ++++++++++++++++--------- 2 files changed, 196 insertions(+), 105 deletions(-) diff --git a/src/collections/decodeableMap.ts b/src/collections/decodeableMap.ts index 86edacb720..9eeae8cf24 100644 --- a/src/collections/decodeableMap.ts +++ b/src/collections/decodeableMap.ts @@ -6,27 +6,39 @@ */ /** - * This is an extension of the Map class that treats keys as the same by matching first normally, - * then decoded. Decoding the key before comparing can solve some edge cases in component fullNames - * such as Layouts. See: https://github.com/forcedotcom/cli/issues/1683 + * This is an extension of the Map class that can match keys whether they are encoded or decoded. + * Decoding the key can solve some edge cases in component fullNames such as Layouts and Profiles. + * See: https://github.com/forcedotcom/cli/issues/1683 * * Examples: * * Given a map with entries: * ```javascript - * 'layout#Layout__Broker__c-v1%2E1 Broker Layout' : {...} - * 'layout#Layout__Broker__c-v9.2 Broker Layout' : {...} + * 'layout#Layout__Broker__c-v1.1 Broker Layout' : {...} + * 'layout#Layout__Broker__c-v9%2E2 Broker Layout' : {...} * ``` * - * `decodeableMap.has('layout#Layout__Broker__c-v1.1 Broker Layout')` --> returns `true` - * `decodeableMap.has('layout#Layout__Broker__c-v9%2E2 Broker Layout')` --> returns `true` + * `decodeableMap.has('layout#Layout__Broker__c-v1%2E1 Broker Layout')` --> returns `true` + * `decodeableMap.has('layout#Layout__Broker__c-v9.2 Broker Layout')` --> returns `true` */ export class DecodeableMap extends Map { + // Internal map of decoded keys to encoded keys. + // E.g., { 'foo-v1.3 Layout': 'foo-v1%2E3 Layout' } + // This is initialized in the `keysMap` getter. + private internalkeysMap!: Map; + + private get keysMap(): Map { + if (!this.internalkeysMap) { + this.internalkeysMap = new Map(); + } + return this.internalkeysMap; + } + /** * boolean indicating whether an element with the specified key (matching decoded) exists or not. */ public has(key: K): boolean { - return super.has(key) || this.hasDecoded(key); + return !!this.getExistingKey(key); } /** @@ -35,43 +47,49 @@ export class DecodeableMap extends Map { * that object and any change made to that object will effectively modify it inside the Map. */ public get(key: K): V | undefined { - return super.get(key) ?? this.getDecoded(key); + const existingKey = this.getExistingKey(key); + return existingKey ? super.get(existingKey) : undefined; } /** * Adds a new element with a specified key and value to the Map. If an element with the - * same key (matching decoded) already exists, the element will be updated. + * same key (encoded or decoded) already exists, the element will be updated. */ public set(key: K, value: V): this { - const sKey = this.getExistingKey(key) ?? key; - return super.set(sKey, value); + return super.set(this.getExistingKey(key, true) ?? key, value); } /** - * true if an element in the Map existed (matching decoded) and has been removed, or false - * if the element does not exist. + * true if an element in the Map existed (matching encoded or decoded key) and has been + * removed, or false if the element does not exist. */ public delete(key: K): boolean { - const sKey = this.getExistingKey(key) ?? key; - return super.delete(sKey); - } - - // Returns true if the passed `key` matches an existing key entry when both keys are decoded. - private hasDecoded(key: string): boolean { - return !!this.getExistingKey(key); - } - - // Returns the value of an entry matching on decoded keys. - private getDecoded(key: string): V | undefined { const existingKey = this.getExistingKey(key); - return existingKey ? super.get(existingKey) : undefined; + return existingKey ? super.delete(existingKey) : false; } - // Returns the key as it is in the map, matching on decoded keys. - private getExistingKey(key: string): string | undefined { - for (const compKey of this.keys()) { - if (decodeURIComponent(compKey) === decodeURIComponent(key)) { - return compKey; + // Optimistically looks for an existing key. If the key is not found, detect if the + // key is encoded. If encoded, try using the decoded key. If decoded, look for an + // encoded entry in the internal map to use for the lookup. + private getExistingKey(key: K, setInKeysMap = false): string | undefined { + if (super.has(key)) { + return key; + } else { + const decodedKey = decodeURIComponent(key); + if (key !== decodedKey) { + // The key is encoded; If this is part of a set operation, + // set the { decodedKey : encodedKey } in the internal map. + if (setInKeysMap) { + this.keysMap.set(decodedKey, key); + } + if (super.has(decodedKey)) { + return decodedKey; + } + } else { + const encodedKey = this.keysMap.get(decodedKey); + if (encodedKey && super.has(encodedKey)) { + return encodedKey; + } } } } diff --git a/test/collections/decodeableMap.test.ts b/test/collections/decodeableMap.test.ts index 743f3c91b4..94dd1ea5ef 100644 --- a/test/collections/decodeableMap.test.ts +++ b/test/collections/decodeableMap.test.ts @@ -10,12 +10,16 @@ import { DecodeableMap } from '../../src/collections/decodeableMap'; describe('DecodeableMap', () => { let dMap: DecodeableMap; - const ENCODED_KEY = 'encodedKey'; - const DECODED_KEY = 'decodedKey'; + const layout1_key_encoded = 'Layout-v1%2E1 Layout'; + const layout1_key_decoded = 'Layout-v1.1 Layout'; + const layout1_value = 'layout1.1-value'; + const layout9_key_encoded = 'Layout-v9%2E2 Layout'; + const layout9_key_decoded = 'Layout-v9.2 Layout'; + const layout9_value = 'layout9.2-value'; + const nonExistent_key_decoded = 'Layout-v3.3-MISSING Layout'; + const nonExistent_key_encoded = 'Layout-v3%2E3-MISSING Layout'; const sandbox = sinon.createSandbox(); - let hasDecodedSpy: sinon.SinonSpy; - let getDecodedSpy: sinon.SinonSpy; let hasMapSpy: sinon.SinonSpy; let getMapSpy: sinon.SinonSpy; let setMapSpy: sinon.SinonSpy; @@ -23,13 +27,9 @@ describe('DecodeableMap', () => { beforeEach(() => { dMap = new DecodeableMap([ - ['Layout-v1%2E1 Layout', ENCODED_KEY], - ['Layout-v9.2 Layout', DECODED_KEY], + [layout1_key_encoded, layout1_value], + [layout9_key_decoded, layout9_value], ]); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - hasDecodedSpy = sandbox.spy(dMap, 'hasDecoded' as any); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - getDecodedSpy = sandbox.spy(dMap, 'getDecoded' as any); hasMapSpy = sandbox.spy(Map.prototype, 'has'); getMapSpy = sandbox.spy(Map.prototype, 'get'); setMapSpy = sandbox.spy(Map.prototype, 'set'); @@ -41,128 +41,201 @@ describe('DecodeableMap', () => { }); describe('has()', () => { - it('should match on exact key without decoding', () => { - expect(dMap.has('Layout-v1%2E1 Layout')).to.be.true; - expect(hasMapSpy.called).to.be.true; - expect(hasDecodedSpy.called).to.be.false; + it('should match an encoded key with a decoded key', () => { + expect(dMap.has(layout1_key_decoded)).to.be.true; + expect(hasMapSpy.calledWith(layout1_key_decoded)).to.be.true; + expect(hasMapSpy.calledWith(layout1_key_encoded)).to.be.true; }); - it('should match encoded key with decoded value', () => { - expect(dMap.has('Layout-v1.1 Layout')).to.be.true; - expect(hasMapSpy.called).to.be.true; - expect(hasDecodedSpy.called).to.be.true; + it('should match an encoded key with an encoded key', () => { + expect(dMap.has(layout1_key_encoded)).to.be.true; + expect(hasMapSpy.calledWith(layout1_key_encoded)).to.be.true; + expect(hasMapSpy.calledWith(layout1_key_decoded)).to.be.false; }); - it('should match decoded key with encoded value', () => { - expect(dMap.has('Layout-v9%2E2 Layout')).to.be.true; - expect(hasMapSpy.called).to.be.true; - expect(hasDecodedSpy.called).to.be.true; + it('should match a decoded key with a decoded key', () => { + expect(dMap.has(layout9_key_decoded)).to.be.true; + expect(hasMapSpy.calledWith(layout9_key_decoded)).to.be.true; + expect(hasMapSpy.calledWith(layout9_key_encoded)).to.be.false; }); - it('should not match on no existing key', () => { - expect(dMap.has('Layout-MISSING Layout')).to.be.false; - expect(hasMapSpy.called).to.be.true; - expect(hasDecodedSpy.called).to.be.true; + it('should match a decoded key with an encoded key', () => { + expect(dMap.has(layout9_key_encoded)).to.be.true; + expect(hasMapSpy.calledWith(layout9_key_encoded)).to.be.true; + expect(hasMapSpy.calledWith(layout9_key_decoded)).to.be.true; + }); + + it('should not match on decoded nonexistent key', () => { + expect(dMap.has(nonExistent_key_decoded)).to.be.false; + expect(hasMapSpy.calledWith(nonExistent_key_decoded)).to.be.true; + expect(hasMapSpy.calledWith(nonExistent_key_encoded)).to.be.false; + }); + + it('should not match on encoded nonexistent key', () => { + expect(dMap.has(nonExistent_key_encoded)).to.be.false; + expect(hasMapSpy.calledWith(nonExistent_key_encoded)).to.be.true; + expect(hasMapSpy.calledWith(nonExistent_key_decoded)).to.be.true; }); }); describe('get()', () => { - it('should get value with exact key without decoding', () => { - expect(dMap.get('Layout-v1%2E1 Layout')).to.equal(ENCODED_KEY); - expect(getMapSpy.calledOnce).to.be.true; - expect(getDecodedSpy.called).to.be.false; + it('should get value from an encoded key with a decoded key', () => { + expect(dMap.get(layout1_key_decoded)).to.equal(layout1_value); + expect(getMapSpy.calledWith(layout1_key_decoded)).to.be.true; + expect(getMapSpy.calledWith(layout1_key_encoded)).to.be.true; + }); + + it('should get value from an encoded key with an encoded key', () => { + expect(dMap.get(layout1_key_encoded)).to.equal(layout1_value); + expect(getMapSpy.calledWith(layout1_key_encoded)).to.be.true; + expect(getMapSpy.calledWith(layout1_key_decoded)).to.be.false; }); - it('should get value of encoded key using decoded key', () => { - expect(dMap.get('Layout-v1.1 Layout')).to.equal(ENCODED_KEY); - expect(getMapSpy.calledTwice).to.be.true; - expect(getDecodedSpy.calledOnce).to.be.true; + it('should get value from a decoded key with a decoded key', () => { + expect(dMap.get(layout9_key_decoded)).to.equal(layout9_value); + expect(getMapSpy.calledWith(layout9_key_decoded)).to.be.true; + expect(getMapSpy.calledWith(layout9_key_encoded)).to.be.false; }); - it('should get value of decoded key using encoded key', () => { - expect(dMap.get('Layout-v9%2E2 Layout')).to.equal(DECODED_KEY); - expect(getMapSpy.calledTwice).to.be.true; - expect(getDecodedSpy.calledOnce).to.be.true; + it('should get value from a decoded key with an encoded key', () => { + expect(dMap.get(layout9_key_encoded)).to.equal(layout9_value); + expect(getMapSpy.calledWith(layout9_key_encoded)).to.be.false; + expect(getMapSpy.calledWith(layout9_key_decoded)).to.be.true; }); - it('should return undefined on no existing key', () => { - expect(dMap.get('Layout-MISSING Layout')).to.be.undefined; - expect(getMapSpy.calledOnce).to.be.true; - expect(getDecodedSpy.called).to.be.true; + it('should return undefined on decoded nonexistent key', () => { + expect(dMap.get(nonExistent_key_decoded)).to.be.undefined; + // This is true since it gets from the internal map. + expect(getMapSpy.calledWith(nonExistent_key_decoded)).to.be.true; + expect(getMapSpy.calledWith(nonExistent_key_encoded)).to.be.false; + }); + + it('should return undefined on encoded nonexistent key', () => { + expect(dMap.get(nonExistent_key_encoded)).to.be.undefined; + expect(getMapSpy.calledWith(nonExistent_key_encoded)).to.be.false; + expect(getMapSpy.calledWith(nonExistent_key_decoded)).to.be.false; }); }); describe('set()', () => { const NEW_VALUE = 'new value from set'; - it('should set value with exact key', () => { - expect(dMap.set('Layout-v1%2E1 Layout', NEW_VALUE)).to.equal(dMap); + it('should update value of decoded key using decoded key', () => { + expect(dMap.set(layout9_key_decoded, NEW_VALUE)).to.equal(dMap); + expect(setMapSpy.called).to.be.true; + expect(setMapSpy.lastCall.args[0]).to.equal(layout9_key_decoded); + expect(setMapSpy.lastCall.args[1]).to.equal(NEW_VALUE); + expect(dMap.size).to.equal(2); + expect(dMap.get(layout9_key_decoded)).to.equal(NEW_VALUE); + // @ts-ignore testing private map. expect 1 for initial map creation + expect(dMap.keysMap.size).to.equal(1); + }); + + it('should update value of decoded key using encoded key', () => { + expect(dMap.set(layout9_key_encoded, NEW_VALUE)).to.equal(dMap); expect(setMapSpy.called).to.be.true; - expect(setMapSpy.lastCall.args[0]).to.equal('Layout-v1%2E1 Layout'); + expect(setMapSpy.lastCall.args[0]).to.equal(layout9_key_decoded); expect(setMapSpy.lastCall.args[1]).to.equal(NEW_VALUE); expect(dMap.size).to.equal(2); - expect(dMap.get('Layout-v1%2E1 Layout')).to.equal(NEW_VALUE); + expect(dMap.get(layout9_key_encoded)).to.equal(NEW_VALUE); + // @ts-ignore testing private map. expect 2 for initial map creation and addition + expect(dMap.keysMap.size).to.equal(2); + // @ts-ignore testing private map + expect(dMap.keysMap.get(layout9_key_decoded)).to.equal(layout9_key_encoded); }); - it('should set value of encoded key using decoded key', () => { - expect(dMap.set('Layout-v1.1 Layout', NEW_VALUE)).to.equal(dMap); + it('should update value of encoded key using encoded key', () => { + expect(dMap.set(layout1_key_encoded, NEW_VALUE)).to.equal(dMap); expect(setMapSpy.called).to.be.true; - expect(setMapSpy.lastCall.args[0]).to.equal('Layout-v1%2E1 Layout'); + expect(setMapSpy.lastCall.args[0]).to.equal(layout1_key_encoded); expect(setMapSpy.lastCall.args[1]).to.equal(NEW_VALUE); expect(dMap.size).to.equal(2); - expect(dMap.get('Layout-v1%2E1 Layout')).to.equal(NEW_VALUE); + expect(dMap.get(layout1_key_encoded)).to.equal(NEW_VALUE); + // @ts-ignore testing private map. expect 1 for initial map creation + expect(dMap.keysMap.size).to.equal(1); }); - it('should set value of decoded key using encoded key', () => { - expect(dMap.set('Layout-v9%2E2 Layout', NEW_VALUE)).to.equal(dMap); + it('should update value of encoded key using decoded key', () => { + expect(dMap.set(layout1_key_decoded, NEW_VALUE)).to.equal(dMap); expect(setMapSpy.called).to.be.true; - expect(setMapSpy.lastCall.args[0]).to.equal('Layout-v9.2 Layout'); + expect(setMapSpy.lastCall.args[0]).to.equal(layout1_key_encoded); expect(setMapSpy.lastCall.args[1]).to.equal(NEW_VALUE); expect(dMap.size).to.equal(2); - expect(dMap.get('Layout-v9.2 Layout')).to.equal(NEW_VALUE); + expect(dMap.get(layout1_key_encoded)).to.equal(NEW_VALUE); + // @ts-ignore testing private map. expect 1 for initial map creation + expect(dMap.keysMap.size).to.equal(1); + }); + + it('should set new entry on decoded nonexistent key', () => { + expect(dMap.set(nonExistent_key_decoded, NEW_VALUE)).to.equal(dMap); + expect(setMapSpy.called).to.be.true; + expect(setMapSpy.lastCall.args[0]).to.equal(nonExistent_key_decoded); + expect(setMapSpy.lastCall.args[1]).to.equal(NEW_VALUE); + expect(dMap.size).to.equal(3); + expect(dMap.get(nonExistent_key_decoded)).to.equal(NEW_VALUE); + // @ts-ignore testing private map. expect 1 for initial map creation + expect(dMap.keysMap.size).to.equal(1); }); - it('should set new entry on no existing key', () => { - expect(dMap.set('Layout-MISSING Layout', NEW_VALUE)).to.equal(dMap); + it('should set new entry on encoded nonexistent key', () => { + expect(dMap.set(nonExistent_key_encoded, NEW_VALUE)).to.equal(dMap); expect(setMapSpy.called).to.be.true; - expect(setMapSpy.lastCall.args[0]).to.equal('Layout-MISSING Layout'); + expect(setMapSpy.lastCall.args[0]).to.equal(nonExistent_key_encoded); expect(setMapSpy.lastCall.args[1]).to.equal(NEW_VALUE); expect(dMap.size).to.equal(3); - expect(dMap.get('Layout-MISSING Layout')).to.equal(NEW_VALUE); + expect(dMap.get(nonExistent_key_encoded)).to.equal(NEW_VALUE); + // @ts-ignore testing private map. expect 2 for initial map creation and addition + expect(dMap.keysMap.size).to.equal(2); + // @ts-ignore testing private map + expect(dMap.keysMap.get(nonExistent_key_decoded)).to.equal(nonExistent_key_encoded); }); }); describe('delete()', () => { - it('should delete using exact key', () => { - expect(dMap.delete('Layout-v1%2E1 Layout')).to.be.true; + it('should delete an encoded key with a decoded key', () => { + expect(dMap.delete(layout1_key_decoded)).to.be.true; expect(deleteMapSpy.calledOnce).to.be.true; - expect(deleteMapSpy.firstCall.args[0]).to.equal('Layout-v1%2E1 Layout'); + expect(deleteMapSpy.firstCall.args[0]).to.equal(layout1_key_encoded); expect(dMap.size).to.equal(1); - expect(dMap.has('Layout-v1%2E1 Layout')).to.be.false; + expect(dMap.has(layout1_key_decoded)).to.be.false; }); - it('should delete the encoded key using decoded value', () => { - expect(dMap.delete('Layout-v1.1 Layout')).to.be.true; + it('should delete an encoded key with an encoded key', () => { + expect(dMap.delete(layout1_key_encoded)).to.be.true; expect(deleteMapSpy.calledOnce).to.be.true; - expect(deleteMapSpy.firstCall.args[0]).to.equal('Layout-v1%2E1 Layout'); + expect(deleteMapSpy.firstCall.args[0]).to.equal(layout1_key_encoded); expect(dMap.size).to.equal(1); - expect(dMap.has('Layout-v1.1 Layout')).to.be.false; + expect(dMap.has(layout1_key_encoded)).to.be.false; }); - it('should delete the decoded key using encoded value', () => { - expect(dMap.delete('Layout-v9%2E2 Layout')).to.be.true; + it('should delete a decoded key with a decoded key', () => { + expect(dMap.delete(layout9_key_decoded)).to.be.true; expect(deleteMapSpy.calledOnce).to.be.true; - expect(deleteMapSpy.firstCall.args[0]).to.equal('Layout-v9.2 Layout'); + expect(deleteMapSpy.firstCall.args[0]).to.equal(layout9_key_decoded); expect(dMap.size).to.equal(1); - expect(dMap.has('Layout-v9%2E2 Layout')).to.be.false; + expect(dMap.has(layout9_key_decoded)).to.be.false; }); - it('should not delete on no existing key', () => { - expect(dMap.delete('Layout-MISSING Layout')).to.be.false; + it('should delete a decoded key with an encoded key', () => { + expect(dMap.delete(layout9_key_encoded)).to.be.true; expect(deleteMapSpy.calledOnce).to.be.true; - expect(deleteMapSpy.firstCall.args[0]).to.equal('Layout-MISSING Layout'); + expect(deleteMapSpy.firstCall.args[0]).to.equal(layout9_key_decoded); + expect(dMap.size).to.equal(1); + expect(dMap.has(layout9_key_encoded)).to.be.false; + }); + + it('should not delete a decoded nonexistent key', () => { + expect(dMap.delete(nonExistent_key_decoded)).to.be.false; + expect(deleteMapSpy.called).to.be.false; + expect(dMap.size).to.equal(2); + expect(dMap.has(nonExistent_key_decoded)).to.be.false; + }); + + it('should not delete an encoded nonexistent key', () => { + expect(dMap.delete(nonExistent_key_encoded)).to.be.false; + expect(deleteMapSpy.called).to.be.false; expect(dMap.size).to.equal(2); - expect(dMap.has('Layout-MISSING Layout')).to.be.false; + expect(dMap.has(nonExistent_key_encoded)).to.be.false; }); }); }); From 8f9e7f52da0d3e26e0aefb44c69dce943aa729a0 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Thu, 12 Oct 2023 09:45:46 -0600 Subject: [PATCH 5/5] chore: update comment for internal map initialization --- src/collections/decodeableMap.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/collections/decodeableMap.ts b/src/collections/decodeableMap.ts index 9eeae8cf24..c73701d0b1 100644 --- a/src/collections/decodeableMap.ts +++ b/src/collections/decodeableMap.ts @@ -24,7 +24,9 @@ export class DecodeableMap extends Map { // Internal map of decoded keys to encoded keys. // E.g., { 'foo-v1.3 Layout': 'foo-v1%2E3 Layout' } - // This is initialized in the `keysMap` getter. + // This is initialized in the `keysMap` getter due to the constructor calling + // `super` before the initialization happens, and it needs to be initialized + // before `this.set()` is called or `TypeErrors` will be thrown. private internalkeysMap!: Map; private get keysMap(): Map {