From 616488af548498c32cf1fde45b79fd52490db6f3 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Thu, 25 May 2023 17:12:28 -0500 Subject: [PATCH 01/15] fix: everything but ut/testsetup --- README.md | 2 +- src/config/aliasesConfig.ts | 3 + src/config/configGroup.ts | 2 + src/org/org.ts | 52 +++--- .../accessors/aliasAccessor.ts | 172 ++++++++++++++---- 5 files changed, 172 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index 633b904a17..8e9ed86a3d 100644 --- a/README.md +++ b/README.md @@ -95,7 +95,7 @@ describe('Mocking Aliases', () => { const testData = new MockTestOrgData(); await $$.stubAliases({ myAlias: testData.username }); const alias = (await StateAggregator.getInstance()).aliases.get(testData.username); - strictEqual(alias, 'myAlais'); + strictEqual(alias, 'myAlias'); }); }); diff --git a/src/config/aliasesConfig.ts b/src/config/aliasesConfig.ts index ad655fc64c..2f089a9b59 100644 --- a/src/config/aliasesConfig.ts +++ b/src/config/aliasesConfig.ts @@ -15,6 +15,9 @@ export enum AliasGroup { ORGS = 'orgs', } +/** + * @deprecated. AliasAccessor is very simple now + */ export class AliasesConfig extends ConfigGroup { public static getDefaultOptions(): ConfigGroup.Options { return { ...ConfigGroup.getOptions(AliasGroup.ORGS, 'alias.json'), isGlobal: true, isState: true }; diff --git a/src/config/configGroup.ts b/src/config/configGroup.ts index 6f153eb6a3..a2ee07f79c 100644 --- a/src/config/configGroup.ts +++ b/src/config/configGroup.ts @@ -11,6 +11,8 @@ import { ConfigFile } from './configFile'; import { ConfigContents, ConfigEntry, ConfigValue } from './configStore'; /** + * @deprecated + * * A config file that stores config values in groups. e.g. to store different config * values for different commands, without having manually manipulate the config. * diff --git a/src/org/org.ts b/src/org/org.ts index 55ed85c82d..3667cc0299 100644 --- a/src/org/org.ts +++ b/src/org/org.ts @@ -1211,36 +1211,36 @@ export class Org extends AsyncOptionalCreatable { this.logger.debug(`Removing users associate with org: ${this.getOrgId()}`); const config = await this.retrieveOrgUsersConfig(); this.logger.debug(`using path for org users: ${config.getPath()}`); - const authInfos: AuthInfo[] = await this.readUserAuthFiles(); + + const usernames = (await this.readUserAuthFiles()).map((auth) => auth.getFields().username).filter(isString); await Promise.all( - authInfos - .map((auth) => auth.getFields().username) - .map(async (username) => { - const aliasKeys = (username && stateAggregator.aliases.getAll(username)) ?? []; - stateAggregator.aliases.unsetAll(username as string); - - const orgForUser = - username === this.getUsername() - ? this - : await Org.create({ - connection: await Connection.create({ authInfo: await AuthInfo.create({ username }) }), - }); - - const orgType = this.isDevHubOrg() ? OrgConfigProperties.TARGET_DEV_HUB : OrgConfigProperties.TARGET_ORG; - const configInfo = orgForUser.configAggregator.getInfo(orgType); - const needsConfigUpdate = - (configInfo.isGlobal() || configInfo.isLocal()) && - (configInfo.value === username || aliasKeys.includes(configInfo.value as string)); - - return [ - orgForUser.removeAuth(), - needsConfigUpdate ? Config.update(configInfo.isGlobal(), orgType, undefined) : undefined, - ].filter(Boolean); - }) + usernames.map(async (username) => { + const aliasKeys = (username && stateAggregator.aliases.getAll(username)) ?? []; + stateAggregator.aliases.unsetAll(username); + + const orgForUser = + username === this.getUsername() + ? this + : await Org.create({ + connection: await Connection.create({ authInfo: await AuthInfo.create({ username }) }), + }); + + const orgType = this.isDevHubOrg() ? OrgConfigProperties.TARGET_DEV_HUB : OrgConfigProperties.TARGET_ORG; + const configInfo = orgForUser.configAggregator.getInfo(orgType); + const needsConfigUpdate = + (configInfo.isGlobal() || configInfo.isLocal()) && + (configInfo.value === username || aliasKeys.includes(configInfo.value as string)); + + return [ + orgForUser.removeAuth(), + needsConfigUpdate ? Config.update(configInfo.isGlobal(), orgType, undefined) : undefined, + ].filter(Boolean); + }) ); - await stateAggregator.aliases.write(); + // now that we're dong with all the aliases, we can unset those + await stateAggregator.aliases.unsetAndSave(usernames); } private async removeSandboxConfig(): Promise { diff --git a/src/stateAggregator/accessors/aliasAccessor.ts b/src/stateAggregator/accessors/aliasAccessor.ts index dacff1e2c9..0b0850db42 100644 --- a/src/stateAggregator/accessors/aliasAccessor.ts +++ b/src/stateAggregator/accessors/aliasAccessor.ts @@ -5,17 +5,29 @@ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause */ -import { AsyncOptionalCreatable } from '@salesforce/kit'; +import { join, dirname } from 'node:path'; +import { homedir } from 'node:os'; +import { readFile, writeFile, mkdir } from 'node:fs/promises'; +import { writeFileSync, readFileSync } from 'node:fs'; + +import { AsyncOptionalCreatable, ensureArray } from '@salesforce/kit'; + import { Nullable } from '@salesforce/ts-types'; -import { AliasesConfig } from '../../config/aliasesConfig'; -import { AuthFields, ConfigContents } from '../../exported'; +import { Global } from '../../global'; +import { AuthFields } from '../../org/authInfo'; +import { ConfigContents } from '../../config/configStore'; import { SfError } from '../../sfError'; import { SfToken } from './tokenAccessor'; export type Aliasable = string | (Partial & Partial); +const DEFAULT_GROUP = 'orgs'; +const FILENAME = 'alias.json'; export class AliasAccessor extends AsyncOptionalCreatable { - private config!: AliasesConfig; + // set in init method + private fileLocation!: string; + /** orgs is the default group */ + private aliasStore!: Map; /** * Returns all the aliases for all the values @@ -30,14 +42,14 @@ export class AliasAccessor extends AsyncOptionalCreatable { public getAll(entity?: Aliasable): string[] | ConfigContents { // This will only return aliases under "orgs". This will need to be modified // if/when we want to support more aliases groups. - const all = (this.config.getGroup() ?? {}) as ConfigContents; + if (entity) { - const value = this.getNameOf(entity); - return Object.entries(all) - .filter((entry) => entry[1] === value) - .map((entry) => entry[0]); + const nameFromEntity = getNameOf(entity); + return Array.from(this.aliasStore.entries()) + .filter(([, value]) => nameFromEntity === value) + .map(([alias]) => alias); } else { - return all; + return Object.fromEntries(this.aliasStore.entries()); } } @@ -47,7 +59,7 @@ export class AliasAccessor extends AsyncOptionalCreatable { * @param entity the aliasable entity that you want to get the alias of */ public get(entity: Aliasable): Nullable { - return this.getAll(entity).find((alias) => alias) ?? null; + return this.getAll(entity)[0] ?? null; } /** @@ -56,7 +68,7 @@ export class AliasAccessor extends AsyncOptionalCreatable { * @param alias the alias that corresponds to a value */ public getValue(alias: string): Nullable { - return this.getAll()[alias] ?? null; + return this.aliasStore.get(alias) ?? null; } /** @@ -65,7 +77,7 @@ export class AliasAccessor extends AsyncOptionalCreatable { * @param alias the alias that corresponds to a username */ public getUsername(alias: string): Nullable { - return this.getAll()[alias] ?? null; + return this.aliasStore.get(alias) ?? null; } /** @@ -105,41 +117,92 @@ export class AliasAccessor extends AsyncOptionalCreatable { * * @param usernameOrAlias a string that might be a username or might be an alias */ - public resolveAlias(usernameOrAlias: string): Nullable { - if (this.has(usernameOrAlias)) return usernameOrAlias; - return Object.entries(this.getAll()).find(([, username]) => username === usernameOrAlias)?.[0]; + public resolveAlias(usernameOrAlias: string): string | undefined { + if (this.aliasStore.has(usernameOrAlias)) return usernameOrAlias; + return Array.from(this.aliasStore.entries()).find(([, value]) => value === usernameOrAlias)?.[0]; } /** - * Set an alias for the given aliasable entity + * Set an alias for the given aliasable entity. Writes to the file * + * @deprecated use setAndSave * @param alias the alias you want to set * @param entity the aliasable entity that's being aliased */ public set(alias: string, entity: Aliasable): void { - this.config.set(alias, this.getNameOf(entity)); + // get a very fresh copy to merge with to avoid conflicts + this.readFileToAliasStoreSync(); + this.aliasStore.set(alias, getNameOf(entity)); + this.saveAliasStoreToFileSync(); + } + + /** + * Set an alias for the given aliasable entity. Writes to the file + * + * @deprecated use setAndSave + * @param alias the alias you want to set + * @param entity the aliasable entity that's being aliased + */ + public async setAndSave(alias: string, entity: Aliasable): Promise { + // get a very fresh copy to merge with to avoid conflicts + await this.readFileToAliasStore(); + this.aliasStore.set(alias, getNameOf(entity)); + return this.saveAliasStoreToFile(); } /** - * Unset the given alias. + * Unset the given alias. Writes to the file * */ public unset(alias: string): void { - this.config.unset(alias); + this.readFileToAliasStoreSync(); + this.aliasStore.delete(alias); + this.saveAliasStoreToFileSync(); + } + + /** + * Unset the given alias(es). Writes to the file + * + */ + public async unsetAndSave(alias: string | string[]): Promise { + await this.readFileToAliasStore(); + ensureArray(alias).forEach((a) => this.aliasStore.delete(a)); + return this.saveAliasStoreToFile(); } /** * Unsets all the aliases for the given entity. * + * @deprecated use unsetAllAndSave + * * @param entity the aliasable entity for which you want to unset all aliases */ public unsetAll(entity: Aliasable): void { + this.readFileToAliasStoreSync(); const aliases = this.getAll(entity); - aliases.forEach((alias) => this.unset(alias)); + aliases.forEach((a) => this.aliasStore.delete(a)); + this.saveAliasStoreToFileSync(); } + /** + * Unsets all the aliases for the given entity. + * + * @deprecated use unsetAllAndSave + * + * @param entity the aliasable entity for which you want to unset all aliases + */ + public async unsetAllAndSave(entity: Aliasable): Promise { + await this.readFileToAliasStore(); + const aliases = this.getAll(entity); + aliases.forEach((a) => this.aliasStore.delete(a)); + return this.saveAliasStoreToFile(); + } + + /** + * @deprecated the set/unset methods now write to the file when called. Use (un)setAndSave instead of calling (un)set and then calling write() + */ public async write(): Promise { - return this.config.write(); + return Promise.resolve(this.getAll()); } /** @@ -148,23 +211,68 @@ export class AliasAccessor extends AsyncOptionalCreatable { * @param alias the alias you want to check */ public has(alias: string): boolean { - return this.config.has(alias); + return this.aliasStore.has(alias); } protected async init(): Promise { - this.config = await AliasesConfig.create(AliasesConfig.getDefaultOptions()); + this.fileLocation = join(homedir(), Global.SFDX_STATE_FOLDER, FILENAME); + await this.readFileToAliasStore(); } /** - * Returns the username of given aliasable entity + * go to the fileSystem and read the file, storing a copy in the class's store + * if the file doesn't exist, create it empty */ - // eslint-disable-next-line class-methods-use-this - private getNameOf(entity: Aliasable): string { - if (typeof entity === 'string') return entity; - const aliaseeName = entity.username ?? entity.user; - if (!aliaseeName) { - throw new SfError(`Invalid aliasee, it must contain a user or username property: ${JSON.stringify(entity)}`); + private async readFileToAliasStore(): Promise { + try { + this.aliasStore = fileContentsRawToAliasStore(await readFile(this.fileLocation, 'utf-8')); + } catch (e) { + if (e instanceof Error && 'code' in e && e.code === 'ENOENT') { + this.aliasStore = new Map(); + await mkdir(dirname(this.fileLocation), { recursive: true }); + await this.saveAliasStoreToFile(); + return; + } + throw e; } - return aliaseeName; + } + + private async saveAliasStoreToFile(): Promise { + return writeFile(this.fileLocation, aliasStoreToRawFileContents(this.aliasStore)); + } + + /** provided for the legacy sync set/unset methods */ + private readFileToAliasStoreSync(): void { + // the file is guaranteed to exist because this init method ensures it + this.aliasStore = fileContentsRawToAliasStore(readFileSync(this.fileLocation, 'utf-8')); + } + + /** provided for the legacy sync set/unset methods */ + private saveAliasStoreToFileSync(): void { + writeFileSync(this.fileLocation, aliasStoreToRawFileContents(this.aliasStore)); } } + +/** + * Returns the username of given aliasable entity + */ +const getNameOf = (entity: Aliasable): string => { + if (typeof entity === 'string') return entity; + const aliaseeName = entity.username ?? entity.user; + if (!aliaseeName) { + throw new SfError(`Invalid aliasee, it must contain a user or username property: ${JSON.stringify(entity)}`); + } + return aliaseeName; +}; + +const fileContentsRawToAliasStore = (contents: string): Map => { + const fileContents = JSON.parse(contents) as { + [group: string]: { [alias: string]: string }; + [DEFAULT_GROUP]: { [alias: string]: string }; + }; + + return new Map(Object.entries(fileContents[DEFAULT_GROUP])); +}; + +const aliasStoreToRawFileContents = (aliasStore: Map): string => + JSON.stringify({ [DEFAULT_GROUP]: Object.fromEntries(Array.from(aliasStore.entries())) }); From d351ebb1136cd97d1c2d70f14d29622b4b40e711 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 09:46:07 -0500 Subject: [PATCH 02/15] feat: file locking for aliases --- package.json | 4 +- src/org/org.ts | 9 +-- .../accessors/aliasAccessor.ts | 59 +++++++++++++------ yarn.lock | 12 ++++ 4 files changed, 58 insertions(+), 26 deletions(-) diff --git a/package.json b/package.json index 3c23208172..aeca7e0fd6 100644 --- a/package.json +++ b/package.json @@ -49,6 +49,7 @@ "js2xmlparser": "^4.0.1", "jsforce": "^2.0.0-beta.23", "jsonwebtoken": "9.0.0", + "proper-lockfile": "^4.1.2", "ts-retry-promise": "^0.7.0" }, "devDependencies": { @@ -61,6 +62,7 @@ "@types/debug": "0.0.31", "@types/jsonwebtoken": "9.0.1", "@types/lodash": "^4.14.194", + "@types/proper-lockfile": "^4.1.2", "@types/shelljs": "0.8.12", "@typescript-eslint/eslint-plugin": "^5.58.0", "@typescript-eslint/parser": "^5.57.1", @@ -167,4 +169,4 @@ ] } } -} \ No newline at end of file +} diff --git a/src/org/org.ts b/src/org/org.ts index 3667cc0299..713c780905 100644 --- a/src/org/org.ts +++ b/src/org/org.ts @@ -1216,9 +1216,6 @@ export class Org extends AsyncOptionalCreatable { await Promise.all( usernames.map(async (username) => { - const aliasKeys = (username && stateAggregator.aliases.getAll(username)) ?? []; - stateAggregator.aliases.unsetAll(username); - const orgForUser = username === this.getUsername() ? this @@ -1230,7 +1227,7 @@ export class Org extends AsyncOptionalCreatable { const configInfo = orgForUser.configAggregator.getInfo(orgType); const needsConfigUpdate = (configInfo.isGlobal() || configInfo.isLocal()) && - (configInfo.value === username || aliasKeys.includes(configInfo.value as string)); + (configInfo.value === username || stateAggregator.aliases.get(configInfo.value as string) === username); return [ orgForUser.removeAuth(), @@ -1239,8 +1236,8 @@ export class Org extends AsyncOptionalCreatable { }) ); - // now that we're dong with all the aliases, we can unset those - await stateAggregator.aliases.unsetAndSave(usernames); + // now that we're done with all the aliases, we can unset those + await stateAggregator.aliases.unsetValuesAndSave(usernames); } private async removeSandboxConfig(): Promise { diff --git a/src/stateAggregator/accessors/aliasAccessor.ts b/src/stateAggregator/accessors/aliasAccessor.ts index 0b0850db42..0f37ef8ea7 100644 --- a/src/stateAggregator/accessors/aliasAccessor.ts +++ b/src/stateAggregator/accessors/aliasAccessor.ts @@ -9,6 +9,7 @@ import { join, dirname } from 'node:path'; import { homedir } from 'node:os'; import { readFile, writeFile, mkdir } from 'node:fs/promises'; import { writeFileSync, readFileSync } from 'node:fs'; +import { lock, unlock, lockSync, unlockSync } from 'proper-lockfile'; import { AsyncOptionalCreatable, ensureArray } from '@salesforce/kit'; @@ -21,8 +22,10 @@ import { SfToken } from './tokenAccessor'; export type Aliasable = string | (Partial & Partial); const DEFAULT_GROUP = 'orgs'; -const FILENAME = 'alias.json'; - +export const FILENAME = 'alias.json'; +const lockRetryOptions = { + retries: { retries: 10, maxTimeout: 1000, factor: 2 }, +}; export class AliasAccessor extends AsyncOptionalCreatable { // set in init method private fileLocation!: string; @@ -139,13 +142,12 @@ export class AliasAccessor extends AsyncOptionalCreatable { /** * Set an alias for the given aliasable entity. Writes to the file * - * @deprecated use setAndSave * @param alias the alias you want to set * @param entity the aliasable entity that's being aliased */ public async setAndSave(alias: string, entity: Aliasable): Promise { - // get a very fresh copy to merge with to avoid conflicts - await this.readFileToAliasStore(); + // get a very fresh copy to merge with to avoid conflicts, then lock + await this.readFileToAliasStore(true); this.aliasStore.set(alias, getNameOf(entity)); return this.saveAliasStoreToFile(); } @@ -164,16 +166,16 @@ export class AliasAccessor extends AsyncOptionalCreatable { * Unset the given alias(es). Writes to the file * */ - public async unsetAndSave(alias: string | string[]): Promise { - await this.readFileToAliasStore(); - ensureArray(alias).forEach((a) => this.aliasStore.delete(a)); + public async unsetAndSave(alias: string): Promise { + await this.readFileToAliasStore(true); + this.aliasStore.delete(alias); return this.saveAliasStoreToFile(); } /** * Unsets all the aliases for the given entity. * - * @deprecated use unsetAllAndSave + * @deprecated use unsetValuesAndSave * * @param entity the aliasable entity for which you want to unset all aliases */ @@ -187,14 +189,14 @@ export class AliasAccessor extends AsyncOptionalCreatable { /** * Unsets all the aliases for the given entity. * - * @deprecated use unsetAllAndSave * * @param entity the aliasable entity for which you want to unset all aliases */ - public async unsetAllAndSave(entity: Aliasable): Promise { - await this.readFileToAliasStore(); - const aliases = this.getAll(entity); - aliases.forEach((a) => this.aliasStore.delete(a)); + public async unsetValuesAndSave(aliasees: Aliasable[]): Promise { + await this.readFileToAliasStore(true); + ensureArray(aliasees) + .flatMap((a) => this.getAll(a)) + .map((a) => this.aliasStore.delete(a)); return this.saveAliasStoreToFile(); } @@ -215,7 +217,7 @@ export class AliasAccessor extends AsyncOptionalCreatable { } protected async init(): Promise { - this.fileLocation = join(homedir(), Global.SFDX_STATE_FOLDER, FILENAME); + this.fileLocation = getFileLocation(); await this.readFileToAliasStore(); } @@ -223,8 +225,11 @@ export class AliasAccessor extends AsyncOptionalCreatable { * go to the fileSystem and read the file, storing a copy in the class's store * if the file doesn't exist, create it empty */ - private async readFileToAliasStore(): Promise { + private async readFileToAliasStore(useLock = false): Promise { try { + if (useLock) { + await lock(this.fileLocation, lockRetryOptions); + } this.aliasStore = fileContentsRawToAliasStore(await readFile(this.fileLocation, 'utf-8')); } catch (e) { if (e instanceof Error && 'code' in e && e.code === 'ENOENT') { @@ -233,23 +238,36 @@ export class AliasAccessor extends AsyncOptionalCreatable { await this.saveAliasStoreToFile(); return; } + if (useLock) await unlock(this.fileLocation); throw e; } } private async saveAliasStoreToFile(): Promise { - return writeFile(this.fileLocation, aliasStoreToRawFileContents(this.aliasStore)); + await writeFile(this.fileLocation, aliasStoreToRawFileContents(this.aliasStore)); + try { + await unlock(this.fileLocation); + } catch { + // ignore the error. If it wasn't locked, that's what we wanted + } } - /** provided for the legacy sync set/unset methods */ + /** + * @deprecated use the async version of this method instead + * provided for the legacy sync set/unset methods. */ private readFileToAliasStoreSync(): void { // the file is guaranteed to exist because this init method ensures it + // put a lock in place. This method is only used by legacy set/unset methods. + lockSync(this.fileLocation); this.aliasStore = fileContentsRawToAliasStore(readFileSync(this.fileLocation, 'utf-8')); } - /** provided for the legacy sync set/unset methods */ + /** + * @deprecated use the async version of this method instead + * provided for the legacy sync set/unset methods */ private saveAliasStoreToFileSync(): void { writeFileSync(this.fileLocation, aliasStoreToRawFileContents(this.aliasStore)); + unlockSync(this.fileLocation); } } @@ -276,3 +294,6 @@ const fileContentsRawToAliasStore = (contents: string): Map => { const aliasStoreToRawFileContents = (aliasStore: Map): string => JSON.stringify({ [DEFAULT_GROUP]: Object.fromEntries(Array.from(aliasStore.entries())) }); + +// exported for testSetup mocking +export const getFileLocation = (): string => join(homedir(), Global.SFDX_STATE_FOLDER, FILENAME); diff --git a/yarn.lock b/yarn.lock index b71fcfc557..9a269f1d08 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1197,6 +1197,13 @@ resolved "https://registry.yarnpkg.com/@types/parse-json/-/parse-json-4.0.0.tgz#2f8bb441434d163b35fb8ffdccd7138927ffb8c0" integrity sha512-//oorEZjL6sbPcKUaCdIGlIUeH26mgzimjBB77G6XRgnDl/L5wOnpyBGRe/Mmf5CVW3PwEBE1NjiMZ/ssFh4wA== +"@types/proper-lockfile@^4.1.2": + version "4.1.2" + resolved "https://registry.yarnpkg.com/@types/proper-lockfile/-/proper-lockfile-4.1.2.tgz#49537cee7134055ee13a1833b76a1c298f39bb26" + integrity sha512-kd4LMvcnpYkspDcp7rmXKedn8iJSCoa331zRRamUp5oanKt/CefbEGPQP7G89enz7sKD4bvsr8mHSsC8j5WOvA== + dependencies: + "@types/retry" "*" + "@types/readdir-glob@*": version "1.1.1" resolved "https://registry.yarnpkg.com/@types/readdir-glob/-/readdir-glob-1.1.1.tgz#27ac2db283e6aa3d110b14ff9da44fcd1a5c38b1" @@ -1211,6 +1218,11 @@ dependencies: "@types/node" "*" +"@types/retry@*": + version "0.12.2" + resolved "https://registry.yarnpkg.com/@types/retry/-/retry-0.12.2.tgz#ed279a64fa438bb69f2480eda44937912bb7480a" + integrity sha512-XISRgDJ2Tc5q4TRqvgJtzsRkFYNJzZrhTdtMoGVBttwzzQJkPnS3WWTFc7kuDRoPtPakl+T+OfdEUjYJj7Jbow== + "@types/semver@^7.3.12", "@types/semver@^7.3.13": version "7.3.13" resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.3.13.tgz#da4bfd73f49bd541d28920ab0e2bf0ee80f71c91" From c78956c570ab657e420917e2e3ee4467a7f291c7 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 09:46:40 -0500 Subject: [PATCH 03/15] test: more ut, stubAliases works without configFile --- src/testSetup.ts | 16 +++- .../accessors/aliasAccessorTest.ts | 92 +++++++++++++++++++ 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/src/testSetup.ts b/src/testSetup.ts index 6bf4bf3baa..2d0e27d44e 100644 --- a/src/testSetup.ts +++ b/src/testSetup.ts @@ -9,11 +9,11 @@ /* eslint-disable @typescript-eslint/no-unsafe-assignment */ /* eslint-disable @typescript-eslint/no-unsafe-member-access */ /* eslint-disable @typescript-eslint/no-unsafe-call */ - +import * as fs from 'node:fs'; import { randomBytes } from 'crypto'; import { EventEmitter } from 'events'; import { tmpdir as osTmpdir } from 'os'; -import { basename, join as pathJoin } from 'path'; +import { basename, join as pathJoin, dirname } from 'path'; import * as util from 'util'; import { SinonSandbox, SinonStatic, SinonStub } from 'sinon'; @@ -40,6 +40,7 @@ import { Logger } from './logger'; import { Messages } from './messages'; import { SfError } from './sfError'; import { SfProject, SfProjectJson } from './sfProject'; +import * as aliasAccesorMocks from './stateAggregator/accessors/aliasAccessor'; import { CometClient, CometSubscription, Message, StreamingExtension } from './status/streamingClient'; import { OrgAccessor, StateAggregator } from './stateAggregator'; import { AuthFields, Org, SandboxFields, User, UserFields } from './org'; @@ -122,7 +123,7 @@ export class TestContext { */ public configStubs: { [configName: string]: Optional; - AliasesConfig?: ConfigStub; + // AliasesConfig?: ConfigStub; AuthInfoConfig?: ConfigStub; Config?: ConfigStub; SfProjectJson?: ConfigStub; @@ -377,7 +378,10 @@ export class TestContext { * Stub the aliases in the global aliases config file. */ public stubAliases(aliases: Record, group = AliasGroup.ORGS): void { - this.configStubs.AliasesConfig = { contents: { [group]: aliases } }; + // we don't really "stub" these since they don't use configFile. + // write the fileContents to location + fs.mkdirSync(dirname(getAliasFileLocation()), { recursive: true }); + fs.writeFileSync(getAliasFileLocation(), JSON.stringify({ [group]: aliases })); } /** @@ -527,7 +531,6 @@ export const stubContext = (testContext: TestContext): Record // Turn off the interoperability feature so that we don't have to mock // the old .sfdx config files Global.SFDX_INTEROPERABILITY = false; - const stubs: Record = {}; // Most core files create a child logger so stub this to return our test logger. @@ -635,6 +638,8 @@ export const stubContext = (testContext: TestContext): Record return testContext.fakeConnectionRequest.call(this, request, options as AnyJson); }); + stubMethod(testContext.SANDBOX, aliasAccesorMocks, 'getFileLocation').returns(getAliasFileLocation()); + stubs.configExists = stubMethod(testContext.SANDBOXES.ORGS, OrgAccessor.prototype, 'exists').callsFake( async function (this: OrgAccessor, username: string): Promise { // @ts-expect-error because private member @@ -659,6 +664,7 @@ export const stubContext = (testContext: TestContext): Record return stubs; }; +const getAliasFileLocation = (): string => pathJoin(osTmpdir(), Global.SFDX_DIR, aliasAccesorMocks.FILENAME); /** * Restore a @salesforce/core test context. This is automatically stubbed in the global beforeEach created by * `const $$ = testSetup()` but is useful if you don't want to have a global stub of @salesforce/core and you diff --git a/test/unit/stateAggregator/accessors/aliasAccessorTest.ts b/test/unit/stateAggregator/accessors/aliasAccessorTest.ts index b2744ad782..a60dcbca8e 100644 --- a/test/unit/stateAggregator/accessors/aliasAccessorTest.ts +++ b/test/unit/stateAggregator/accessors/aliasAccessorTest.ts @@ -124,6 +124,35 @@ describe('AliasAccessor', () => { }); }); + describe('setAndSave', () => { + it('should set an alias for a username', async () => { + const stateAggregator = await StateAggregator.getInstance(); + await stateAggregator.aliases.setAndSave('foobar', username1); + const aliases = stateAggregator.aliases.getAll(username1); + expect(aliases).to.include('foobar'); + + // confirm persistence + StateAggregator.clearInstance(); + const stateAggregator2 = await StateAggregator.getInstance(); + const aliases2 = stateAggregator2.aliases.getAll(username1); + expect(aliases2).to.include('foobar'); + }); + + it('should set an alias for an org', async () => { + const stateAggregator = await StateAggregator.getInstance(); + await stateAggregator.aliases.setAndSave('foobar', await org.getConfig()); + const aliases = stateAggregator.aliases.getAll(org.username); + expect(aliases).to.include('foobar'); + }); + + it('should set an alias for a token', async () => { + const stateAggregator = await StateAggregator.getInstance(); + await stateAggregator.aliases.setAndSave('foobar', token); + const aliases = stateAggregator.aliases.getAll(token.user); + expect(aliases).to.include('foobar'); + }); + }); + describe('unsetAll', () => { it('should unset all the aliases for a given username', async () => { const stateAggregator = await StateAggregator.getInstance(); @@ -144,4 +173,67 @@ describe('AliasAccessor', () => { expect(stateAggregator.aliases.has('DOES_NOT_EXIST')).to.be.false; }); }); + + describe('concurrent access', () => { + const quantity = 50; + + describe('synchronous', () => { + it('should not throw an error when setting aliases concurrently', async () => { + const stateAggregator = await StateAggregator.getInstance(); + const testArray = Array(quantity).map((v, i) => i.toString()); + await Promise.all( + testArray.map((i) => Promise.resolve(stateAggregator.aliases.set(i.toString(), i.toString()))) + ); + testArray.forEach((i) => { + expect(stateAggregator.aliases.get(i)).to.equal(i); + }); + + // confirm persistence + StateAggregator.clearInstance(); + const stateAggregator2 = await StateAggregator.getInstance(); + testArray.forEach((i) => { + expect(stateAggregator2.aliases.get(i)).to.equal(i); + }); + }); + + it('should not throw an error when unsetting aliases concurrently', async () => { + const stateAggregator = await StateAggregator.getInstance(); + const testArray = Array(quantity).map((v, i) => i.toString()); + await Promise.all( + testArray.map((i) => Promise.resolve(stateAggregator.aliases.setAndSave(i.toString(), i.toString()))) + ); + await Promise.all(testArray.map((i) => Promise.resolve(stateAggregator.aliases.unsetAndSave(i)))); + testArray.forEach((i) => { + expect(stateAggregator.aliases.get(i)).to.be.undefined; + }); + }); + }); + describe('promises', () => { + it('should not throw an error when setting aliases concurrently', async () => { + const stateAggregator = await StateAggregator.getInstance(); + const testArray = Array(quantity).map((v, i) => i.toString()); + await Promise.all(testArray.map((i) => stateAggregator.aliases.setAndSave(i.toString(), i.toString()))); + testArray.forEach((i) => { + expect(stateAggregator.aliases.get(i)).to.equal(i); + }); + + // confirm persistence + StateAggregator.clearInstance(); + const stateAggregator2 = await StateAggregator.getInstance(); + testArray.forEach((i) => { + expect(stateAggregator2.aliases.get(i)).to.equal(i); + }); + }); + + it('should not throw an error when unsetting aliases concurrently', async () => { + const stateAggregator = await StateAggregator.getInstance(); + const testArray = Array(quantity).map((v, i) => i.toString()); + await Promise.all(testArray.map((i) => stateAggregator.aliases.setAndSave(i.toString(), i.toString()))); + await Promise.all(testArray.map((i) => stateAggregator.aliases.unsetAndSave(i))); + testArray.forEach((i) => { + expect(stateAggregator.aliases.get(i)).to.be.undefined; + }); + }); + }); + }); }); From dd4ad38de341b7b75997e07e2f636f25d84675a9 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 11:11:45 -0500 Subject: [PATCH 04/15] refactor: remove unused superclasses for AliasAccessor --- src/config/aliasesConfig.ts | 30 --- src/config/configGroup.ts | 254 ------------------ .../accessors/aliasAccessor.ts | 7 +- src/testSetup.ts | 9 +- 4 files changed, 9 insertions(+), 291 deletions(-) delete mode 100644 src/config/aliasesConfig.ts delete mode 100644 src/config/configGroup.ts diff --git a/src/config/aliasesConfig.ts b/src/config/aliasesConfig.ts deleted file mode 100644 index 2f089a9b59..0000000000 --- a/src/config/aliasesConfig.ts +++ /dev/null @@ -1,30 +0,0 @@ -/* - * Copyright (c) 2022, 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 { ConfigGroup } from './configGroup'; -import { ConfigContents, ConfigValue } from './configStore'; - -/** - * Different groups of aliases. Currently only support orgs. - */ -export enum AliasGroup { - ORGS = 'orgs', -} - -/** - * @deprecated. AliasAccessor is very simple now - */ -export class AliasesConfig extends ConfigGroup { - public static getDefaultOptions(): ConfigGroup.Options { - return { ...ConfigGroup.getOptions(AliasGroup.ORGS, 'alias.json'), isGlobal: true, isState: true }; - } - - // eslint-disable-next-line class-methods-use-this - protected setMethod(contents: ConfigContents, key: string, value?: ConfigValue): void { - contents[key] = value; - } -} diff --git a/src/config/configGroup.ts b/src/config/configGroup.ts deleted file mode 100644 index a2ee07f79c..0000000000 --- a/src/config/configGroup.ts +++ /dev/null @@ -1,254 +0,0 @@ -/* - * 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 { definiteEntriesOf, definiteValuesOf, Dictionary, getJsonMap, JsonMap, Optional } from '@salesforce/ts-types'; -import { SfError } from '../sfError'; -import { ConfigFile } from './configFile'; -import { ConfigContents, ConfigEntry, ConfigValue } from './configStore'; - -/** - * @deprecated - * - * A config file that stores config values in groups. e.g. to store different config - * values for different commands, without having manually manipulate the config. - * - * **Note:** All config methods are overwritten to use the {@link ConfigGroup.setDefaultGroup}. - * - * ``` - * class MyPluginConfig extends ConfigGroup { - * public static getFileName(): string { - * return 'myPluginConfigFilename.json'; - * } - * } - * const myConfig = await MyPluginConfig.create(ConfigGroup.getOptions('all')); - * myConfig.setDefaultGroup('myCommand'); // Can be set in your command's init. - * myConfig.set('mykey', 'myvalue'); // Sets 'myKey' for the 'myCommand' group. - * myConfig.setInGroup('myKey', 'myvalue', 'all'); // Manually set in another group. - * await myConfig.write(); - * ``` - */ -export class ConfigGroup extends ConfigFile { - protected defaultGroup = 'default'; - /** - * Get ConfigGroup specific options, such as the default group. - * - * @param defaultGroup The default group to use when creating the config. - * @param filename The filename of the config file. Uses the static {@link getFileName} by default. - */ - public static getOptions(defaultGroup: string, filename?: string): ConfigGroup.Options { - const options: ConfigFile.Options = ConfigFile.getDefaultOptions(true, filename); - const configGroupOptions: ConfigGroup.Options = { defaultGroup }; - Object.assign(configGroupOptions, options); - return configGroupOptions; - } - - /** - * Sets the default group for all {@link BaseConfigStore} methods to use. - * **Throws** *{@link SfError}{ name: 'MissingGroupName' }* The group parameter is null or undefined. - * - * @param group The group. - */ - public setDefaultGroup(group: string): void { - if (!group) { - throw new SfError('null or undefined group', 'MissingGroupName'); - } - - this.defaultGroup = group; - } - - /** - * Set a group of entries in a bulk save. Returns The new properties that were saved. - * - * @param newEntries An object representing the aliases to set. - * @param group The group the property belongs to. - */ - public async updateValues(newEntries: Dictionary, group?: string): Promise> { - // Make sure the contents are loaded - await this.read(); - Object.entries(newEntries).forEach(([key, val]) => this.setInGroup(key, val, group ?? this.defaultGroup)); - await this.write(); - return newEntries; - } - - /** - * Set a value on a group. Returns the promise resolved when the value is set. - * - * @param key The key. - * @param value The value. - * @param group The group. - */ - public async updateValue(key: string, value: ConfigValue, group?: string): Promise { - // Make sure the content is loaded - await this.read(); - this.setInGroup(key, value, group ?? this.defaultGroup); - // Then save it - await this.write(); - } - - /** - * Gets an array of key value pairs. - */ - public entries(): ConfigEntry[] { - const group = this.getGroup(); - if (group) { - return definiteEntriesOf(group); - } - return []; - } - - /** - * Returns a specified element from ConfigGroup. Returns the associated value. - * - * @param key The key. - */ - public get(key: string): Optional { - return this.getInGroup(key); - } - - /** - * Returns a boolean if an element with the specified key exists in the default group. - * - * @param {string} key The key. - */ - public has(key: string): boolean { - const group = this.getGroup(); - return !!group && super.has(this.defaultGroup) && !!group[key]; - } - - /** - * Returns an array of the keys from the default group. - */ - public keys(): string[] { - return Object.keys(this.getGroup(this.defaultGroup) ?? {}); - } - - /** - * Returns an array of the values from the default group. - */ - public values(): ConfigValue[] { - return definiteValuesOf(this.getGroup(this.defaultGroup) ?? {}); - } - - /** - * Add or updates an element with the specified key in the default group. - * - * @param key The key. - * @param value The value. - */ - public set(key: string, value: ConfigValue): ConfigContents { - return this.setInGroup(key, value, this.defaultGroup); - } - - /** - * Removes an element with the specified key from the default group. Returns `true` if the item was deleted. - * - * @param key The key. - */ - public unset(key: string): boolean { - const groupContents = this.getGroup(this.defaultGroup); - if (groupContents) { - delete groupContents[key]; - return true; - } - return false; - } - - /** - * Remove all key value pairs from the default group. - */ - public clear(): void { - delete this.getContents()[this.defaultGroup]; - } - - /** - * Get all config contents for a group. - * - * @param {string} [group = 'default'] The group. - */ - public getGroup(group = this.defaultGroup): Optional { - return getJsonMap(this.getContents(), group) ?? undefined; - } - - /** - * Returns the value associated to the key and group, or undefined if there is none. - * - * @param key The key. - * @param group The group. Defaults to the default group. - */ - public getInGroup(key: string, group?: string): Optional { - const groupContents = this.getGroup(group); - if (groupContents) { - return groupContents[key]; - } - } - - /** - * Convert the config object to a json object. - */ - public toObject(): JsonMap { - return this.getContents(); - } - - /** - * Convert an object to a {@link ConfigContents} and set it as the config contents. - * - * @param {object} obj The object. - */ - // eslint-disable-next-line @typescript-eslint/ban-types - public setContentsFromObject(obj: U): void { - const contents = new Map(Object.entries(obj)); - Array.from(contents.entries()).forEach(([groupKey, groupContents]) => { - if (groupContents) { - Object.entries(groupContents).forEach(([contentKey, contentValue]) => { - // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - this.setInGroup(contentKey, contentValue, groupKey); - }); - } - }); - } - - /** - * Sets the value for the key and group in the config object. - * - * @param key The key. - * @param value The value. - * @param group The group. Uses the default group if not specified. - */ - public setInGroup(key: string, value?: ConfigValue, group?: string): ConfigContents { - group = group ?? this.defaultGroup; - - if (!super.has(group)) { - super.set(group, {}); - } - const content = this.getGroup(group) ?? {}; - this.setMethod(content, key, value); - - return content; - } - - /** - * Initialize the asynchronous dependencies. - */ - public async init(): Promise { - await super.init(); - if (this.options.defaultGroup) { - this.setDefaultGroup(this.options.defaultGroup); - } - } -} - -export namespace ConfigGroup { - /** - * Options when creating the config file. - */ - export interface Options extends ConfigFile.Options { - /** - * The default group for properties to go into. - */ - defaultGroup?: string; - } -} diff --git a/src/stateAggregator/accessors/aliasAccessor.ts b/src/stateAggregator/accessors/aliasAccessor.ts index 0f37ef8ea7..cdccc57a45 100644 --- a/src/stateAggregator/accessors/aliasAccessor.ts +++ b/src/stateAggregator/accessors/aliasAccessor.ts @@ -21,9 +21,12 @@ import { SfError } from '../../sfError'; import { SfToken } from './tokenAccessor'; export type Aliasable = string | (Partial & Partial); -const DEFAULT_GROUP = 'orgs'; +export const DEFAULT_GROUP = 'orgs'; export const FILENAME = 'alias.json'; + +const lockOptions = { stale: 10_000 }; const lockRetryOptions = { + ...lockOptions, retries: { retries: 10, maxTimeout: 1000, factor: 2 }, }; export class AliasAccessor extends AsyncOptionalCreatable { @@ -258,7 +261,7 @@ export class AliasAccessor extends AsyncOptionalCreatable { private readFileToAliasStoreSync(): void { // the file is guaranteed to exist because this init method ensures it // put a lock in place. This method is only used by legacy set/unset methods. - lockSync(this.fileLocation); + lockSync(this.fileLocation, lockOptions); this.aliasStore = fileContentsRawToAliasStore(readFileSync(this.fileLocation, 'utf-8')); } diff --git a/src/testSetup.ts b/src/testSetup.ts index 2d0e27d44e..3b3d2bb922 100644 --- a/src/testSetup.ts +++ b/src/testSetup.ts @@ -40,12 +40,11 @@ import { Logger } from './logger'; import { Messages } from './messages'; import { SfError } from './sfError'; import { SfProject, SfProjectJson } from './sfProject'; -import * as aliasAccesorMocks from './stateAggregator/accessors/aliasAccessor'; +import * as aliasAccessorEntireFile from './stateAggregator/accessors/aliasAccessor'; import { CometClient, CometSubscription, Message, StreamingExtension } from './status/streamingClient'; import { OrgAccessor, StateAggregator } from './stateAggregator'; import { AuthFields, Org, SandboxFields, User, UserFields } from './org'; import { SandboxAccessor } from './stateAggregator/accessors/sandboxAccessor'; -import { AliasGroup } from './config/aliasesConfig'; import { Global } from './global'; /** @@ -377,7 +376,7 @@ export class TestContext { /** * Stub the aliases in the global aliases config file. */ - public stubAliases(aliases: Record, group = AliasGroup.ORGS): void { + public stubAliases(aliases: Record, group = aliasAccessorEntireFile.DEFAULT_GROUP): void { // we don't really "stub" these since they don't use configFile. // write the fileContents to location fs.mkdirSync(dirname(getAliasFileLocation()), { recursive: true }); @@ -638,7 +637,7 @@ export const stubContext = (testContext: TestContext): Record return testContext.fakeConnectionRequest.call(this, request, options as AnyJson); }); - stubMethod(testContext.SANDBOX, aliasAccesorMocks, 'getFileLocation').returns(getAliasFileLocation()); + stubMethod(testContext.SANDBOX, aliasAccessorEntireFile, 'getFileLocation').returns(getAliasFileLocation()); stubs.configExists = stubMethod(testContext.SANDBOXES.ORGS, OrgAccessor.prototype, 'exists').callsFake( async function (this: OrgAccessor, username: string): Promise { @@ -664,7 +663,7 @@ export const stubContext = (testContext: TestContext): Record return stubs; }; -const getAliasFileLocation = (): string => pathJoin(osTmpdir(), Global.SFDX_DIR, aliasAccesorMocks.FILENAME); +const getAliasFileLocation = (): string => pathJoin(osTmpdir(), Global.SFDX_DIR, aliasAccessorEntireFile.FILENAME); /** * Restore a @salesforce/core test context. This is automatically stubbed in the global beforeEach created by * `const $$ = testSetup()` but is useful if you don't want to have a global stub of @salesforce/core and you From b770156045af461029184ee904cf89fba9af4ea9 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 11:23:27 -0500 Subject: [PATCH 05/15] style: linter --- src/stateAggregator/accessors/aliasAccessor.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/stateAggregator/accessors/aliasAccessor.ts b/src/stateAggregator/accessors/aliasAccessor.ts index cdccc57a45..5e35aa86b6 100644 --- a/src/stateAggregator/accessors/aliasAccessor.ts +++ b/src/stateAggregator/accessors/aliasAccessor.ts @@ -190,8 +190,7 @@ export class AliasAccessor extends AsyncOptionalCreatable { } /** - * Unsets all the aliases for the given entity. - * + * Unset all the aliases for the given entity. * * @param entity the aliasable entity for which you want to unset all aliases */ From 0a7f1c1f4bd58af303340183b181678767f48e6c Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 11:24:32 -0500 Subject: [PATCH 06/15] test: resolve windows tmpDir for ut --- .github/workflows/test.yml | 4 +++- src/testSetup.ts | 5 +++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f5a38d5182..7d9c4ee43c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -12,9 +12,11 @@ jobs: needs: yarn-lockfile-check uses: salesforcecli/github-workflows/.github/workflows/unitTestsLinux.yml@main windows-unit-tests: - needs: linux-unit-tests + # needs: linux-unit-tests uses: salesforcecli/github-workflows/.github/workflows/unitTestsWindows.yml@main nuts: + # TODO: remove after win UT past + if: false needs: linux-unit-tests uses: salesforcecli/github-workflows/.github/workflows/externalNut.yml@main strategy: diff --git a/src/testSetup.ts b/src/testSetup.ts index bdb5beca1e..1a0580a207 100644 --- a/src/testSetup.ts +++ b/src/testSetup.ts @@ -13,7 +13,7 @@ import * as fs from 'node:fs'; import { randomBytes } from 'crypto'; import { EventEmitter } from 'events'; import { tmpdir as osTmpdir } from 'os'; -import { basename, join as pathJoin, dirname } from 'path'; +import { basename, join as pathJoin, dirname, resolve } from 'path'; import * as util from 'util'; import { SinonSandbox, SinonStatic, SinonStub } from 'sinon'; @@ -662,7 +662,8 @@ export const stubContext = (testContext: TestContext): Record return stubs; }; -const getAliasFileLocation = (): string => pathJoin(osTmpdir(), Global.SFDX_DIR, aliasAccessorEntireFile.FILENAME); +const getAliasFileLocation = (): string => + pathJoin(resolve(osTmpdir()), Global.SFDX_DIR, aliasAccessorEntireFile.FILENAME); /** * Restore a @salesforce/core test context. This is automatically stubbed in the global beforeEach created by * `const $$ = testSetup()` but is useful if you don't want to have a global stub of @salesforce/core and you From 958396e694dab87f9f82ae1af3dab28c1ec6d625 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 11:34:34 -0500 Subject: [PATCH 07/15] test: windows with correct state dir --- src/testSetup.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/testSetup.ts b/src/testSetup.ts index 1a0580a207..2c9aebf246 100644 --- a/src/testSetup.ts +++ b/src/testSetup.ts @@ -13,7 +13,7 @@ import * as fs from 'node:fs'; import { randomBytes } from 'crypto'; import { EventEmitter } from 'events'; import { tmpdir as osTmpdir } from 'os'; -import { basename, join as pathJoin, dirname, resolve } from 'path'; +import { basename, join as pathJoin, dirname } from 'path'; import * as util from 'util'; import { SinonSandbox, SinonStatic, SinonStub } from 'sinon'; @@ -663,7 +663,7 @@ export const stubContext = (testContext: TestContext): Record }; const getAliasFileLocation = (): string => - pathJoin(resolve(osTmpdir()), Global.SFDX_DIR, aliasAccessorEntireFile.FILENAME); + pathJoin(osTmpdir(), Global.SFDX_STATE_FOLDER, aliasAccessorEntireFile.FILENAME); /** * Restore a @salesforce/core test context. This is automatically stubbed in the global beforeEach created by * `const $$ = testSetup()` but is useful if you don't want to have a global stub of @salesforce/core and you From 1bfd23e3fb24c34419039bae5bf0a2dd248016a5 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 11:41:45 -0500 Subject: [PATCH 08/15] test: restore nuts (windows ut pass now) --- .github/workflows/test.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7d9c4ee43c..f5a38d5182 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -12,11 +12,9 @@ jobs: needs: yarn-lockfile-check uses: salesforcecli/github-workflows/.github/workflows/unitTestsLinux.yml@main windows-unit-tests: - # needs: linux-unit-tests + needs: linux-unit-tests uses: salesforcecli/github-workflows/.github/workflows/unitTestsWindows.yml@main nuts: - # TODO: remove after win UT past - if: false needs: linux-unit-tests uses: salesforcecli/github-workflows/.github/workflows/externalNut.yml@main strategy: From 2b1b17a80862548a90705482414de04e5c7ead3c Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 12:42:25 -0500 Subject: [PATCH 09/15] refactor: remove another unused method --- src/stateAggregator/accessors/aliasAccessor.ts | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/stateAggregator/accessors/aliasAccessor.ts b/src/stateAggregator/accessors/aliasAccessor.ts index 5e35aa86b6..e32bfcea17 100644 --- a/src/stateAggregator/accessors/aliasAccessor.ts +++ b/src/stateAggregator/accessors/aliasAccessor.ts @@ -86,20 +86,6 @@ export class AliasAccessor extends AsyncOptionalCreatable { return this.aliasStore.get(alias) ?? null; } - /** - * If the provided string is an alias, it returns the corresponding value. - * If the provided string is not an alias, we assume that the provided string - * is the value and return it. - * - * This method is helpful when you don't know if the string you have is a value - * or an alias. - * - * @param valueOrAlias a string that might be a value or might be an alias - */ - public resolveValue(valueOrAlias: string): string { - return this.getValue(valueOrAlias) ?? valueOrAlias; - } - /** * If the provided string is an alias, it returns the corresponding username. * If the provided string is not an alias, we assume that the provided string @@ -190,7 +176,7 @@ export class AliasAccessor extends AsyncOptionalCreatable { } /** - * Unset all the aliases for the given entity. + * Unset all the aliases for the given array of entity. * * @param entity the aliasable entity for which you want to unset all aliases */ From 4f6d10c6c458889e3ffaa8e896e133bb3089cd23 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 12:42:49 -0500 Subject: [PATCH 10/15] style: remove commented --- src/testSetup.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/testSetup.ts b/src/testSetup.ts index 2c9aebf246..00776b79ea 100644 --- a/src/testSetup.ts +++ b/src/testSetup.ts @@ -121,7 +121,6 @@ export class TestContext { */ public configStubs: { [configName: string]: Optional; - // AliasesConfig?: ConfigStub; AuthInfoConfig?: ConfigStub; Config?: ConfigStub; SfProjectJson?: ConfigStub; From 52d6fe14c66308488bef35694c2b7179394797e1 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 12:47:08 -0500 Subject: [PATCH 11/15] docs: v4 aliases changes --- MIGRATING_V3-V4.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/MIGRATING_V3-V4.md b/MIGRATING_V3-V4.md index bec496435d..606c2a74e5 100644 --- a/MIGRATING_V3-V4.md +++ b/MIGRATING_V3-V4.md @@ -22,7 +22,17 @@ Both of these were empty wrappers around `SfProject` and `SfProjectJson`. Use th ## Connection.deployRecentValidation -Since this was moved to jsforce2, the sfdx-core implementation was an empty wrapper. Use jsforce's Connection.metadata#deployRecentValidation instead. +Since this was moved to jsforce2, the sfdx-core implementation was an empty wrapper. Use jsforce's Connection(eployRecentValidation iunset. + +## StateAggregator.aliases (aliasAccessor) + +There are some deprecated methods (set, unset). They still exist, but you should use the newer async equivalent. They not only set/unset the value in memory, but immediately write to the filesystem. + +`write` on aliases is now deprecated and a no-op. + +AliasAccessor no longer inherits from the entire configFile family of classes, so those methods are no longer available. + +This new version introduces file locks so parallel operations that write to Aliases should not cross-save each other. ## sfdc From 8fa0f0816f7e45b2352096e06238555c30309bb6 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 26 May 2023 12:52:58 -0500 Subject: [PATCH 12/15] docs: typo --- MIGRATING_V3-V4.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MIGRATING_V3-V4.md b/MIGRATING_V3-V4.md index 606c2a74e5..2980615bb8 100644 --- a/MIGRATING_V3-V4.md +++ b/MIGRATING_V3-V4.md @@ -22,7 +22,7 @@ Both of these were empty wrappers around `SfProject` and `SfProjectJson`. Use th ## Connection.deployRecentValidation -Since this was moved to jsforce2, the sfdx-core implementation was an empty wrapper. Use jsforce's Connection(eployRecentValidation iunset. +Since this was moved to jsforce2, the sfdx-core implementation was an empty wrapper. Use jsforce's ConnectionDeployRecentValidation instead. ## StateAggregator.aliases (aliasAccessor) From a71179e6cf29cc36a2df8be4d68bbdb993939681 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Wed, 31 May 2023 16:39:56 -0500 Subject: [PATCH 13/15] refactor: deprecate unset like set --- src/stateAggregator/accessors/aliasAccessor.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/stateAggregator/accessors/aliasAccessor.ts b/src/stateAggregator/accessors/aliasAccessor.ts index e32bfcea17..65f4ddef5d 100644 --- a/src/stateAggregator/accessors/aliasAccessor.ts +++ b/src/stateAggregator/accessors/aliasAccessor.ts @@ -144,6 +144,8 @@ export class AliasAccessor extends AsyncOptionalCreatable { /** * Unset the given alias. Writes to the file * + * @deprecated use unsetAndSave + * */ public unset(alias: string): void { this.readFileToAliasStoreSync(); From 6181899b2ae41f16feae08088a93acd00fd00e8f Mon Sep 17 00:00:00 2001 From: mshanemc Date: Wed, 31 May 2023 17:59:51 -0500 Subject: [PATCH 14/15] refactor: unlock if locked, very specific error --- .../accessors/aliasAccessor.ts | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/stateAggregator/accessors/aliasAccessor.ts b/src/stateAggregator/accessors/aliasAccessor.ts index 65f4ddef5d..e89a9a4a24 100644 --- a/src/stateAggregator/accessors/aliasAccessor.ts +++ b/src/stateAggregator/accessors/aliasAccessor.ts @@ -228,18 +228,14 @@ export class AliasAccessor extends AsyncOptionalCreatable { await this.saveAliasStoreToFile(); return; } - if (useLock) await unlock(this.fileLocation); + if (useLock) return unlockIfLocked(this.fileLocation); throw e; } } private async saveAliasStoreToFile(): Promise { await writeFile(this.fileLocation, aliasStoreToRawFileContents(this.aliasStore)); - try { - await unlock(this.fileLocation); - } catch { - // ignore the error. If it wasn't locked, that's what we wanted - } + return unlockIfLocked(this.fileLocation); } /** @@ -257,7 +253,13 @@ export class AliasAccessor extends AsyncOptionalCreatable { * provided for the legacy sync set/unset methods */ private saveAliasStoreToFileSync(): void { writeFileSync(this.fileLocation, aliasStoreToRawFileContents(this.aliasStore)); - unlockSync(this.fileLocation); + try { + unlockSync(this.fileLocation); + } catch (e) { + // ignore the error. If it wasn't locked, that's what we wanted + if (errorIsNotAcquired(e)) return; + throw e; + } } } @@ -287,3 +289,15 @@ const aliasStoreToRawFileContents = (aliasStore: Map): string => // exported for testSetup mocking export const getFileLocation = (): string => join(homedir(), Global.SFDX_STATE_FOLDER, FILENAME); + +const unlockIfLocked = async (fileLocation: string): Promise => { + try { + await unlock(fileLocation); + } catch (e) { + // ignore the error. If it wasn't locked, that's what we wanted + if (errorIsNotAcquired(e)) return; + throw e; + } +}; + +const errorIsNotAcquired = (e: unknown): boolean => e instanceof Error && 'code' in e && e.code === 'ENOTACQUIRED'; From a6ff1699c7d4fcc309bc69634ef8ed660f471d45 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Wed, 31 May 2023 18:15:55 -0500 Subject: [PATCH 15/15] refactor: narrower catch to just the fs read, plus ut for no file --- .../accessors/aliasAccessor.ts | 6 ++--- .../accessors/aliasAccessorTest.ts | 23 ++++++++++++++++++- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/stateAggregator/accessors/aliasAccessor.ts b/src/stateAggregator/accessors/aliasAccessor.ts index e89a9a4a24..b5701f53f9 100644 --- a/src/stateAggregator/accessors/aliasAccessor.ts +++ b/src/stateAggregator/accessors/aliasAccessor.ts @@ -216,10 +216,10 @@ export class AliasAccessor extends AsyncOptionalCreatable { * if the file doesn't exist, create it empty */ private async readFileToAliasStore(useLock = false): Promise { + if (useLock) { + await lock(this.fileLocation, lockRetryOptions); + } try { - if (useLock) { - await lock(this.fileLocation, lockRetryOptions); - } this.aliasStore = fileContentsRawToAliasStore(await readFile(this.fileLocation, 'utf-8')); } catch (e) { if (e instanceof Error && 'code' in e && e.code === 'ENOENT') { diff --git a/test/unit/stateAggregator/accessors/aliasAccessorTest.ts b/test/unit/stateAggregator/accessors/aliasAccessorTest.ts index a60dcbca8e..77629f7da2 100644 --- a/test/unit/stateAggregator/accessors/aliasAccessorTest.ts +++ b/test/unit/stateAggregator/accessors/aliasAccessorTest.ts @@ -4,9 +4,14 @@ * 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 { join } from 'node:path'; +import { existsSync } from 'node:fs'; +import { rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; import { expect } from 'chai'; -import { StateAggregator } from '../../../../src/stateAggregator'; +import { FILENAME, StateAggregator } from '../../../../src/stateAggregator'; import { MockTestOrgData, TestContext, uniqid } from '../../../../src/testSetup'; +import { Global } from '../../../../src/global'; const username1 = 'espresso@coffee.com'; const username2 = 'foobar@salesforce.com'; @@ -174,6 +179,20 @@ describe('AliasAccessor', () => { }); }); + describe('lockfile concerns', () => { + it('no aliases file, creates empty file', async () => { + const fileLocation = getAliasFileLocation(); + await rm(fileLocation); + expect(existsSync(fileLocation)).to.be.false; + const stateAggregator = await StateAggregator.getInstance(); + const aliases = stateAggregator.aliases.getAll(username1); + expect(aliases).to.deep.equal([]); + const all = stateAggregator.aliases.getAll(); + expect(all).to.deep.equal({}); + expect(existsSync(fileLocation)).to.be.true; + }); + }); + describe('concurrent access', () => { const quantity = 50; @@ -237,3 +256,5 @@ describe('AliasAccessor', () => { }); }); }); + +const getAliasFileLocation = (): string => join(tmpdir(), Global.SFDX_STATE_FOLDER, FILENAME);