From 837f574469954aed10f54d7254adb7909e995efd Mon Sep 17 00:00:00 2001 From: peternhale Date: Mon, 5 Aug 2024 15:28:41 -0600 Subject: [PATCH] fix: strip ansi color control chars from json (#5708) * fix: strip ansi color control chars from json @W-16375638@ * chore: remove strip-ansi * chore: add tests * chore: fix call chain * test: new unit tests for utils.test.ts * chore: better handle command errors * chore: fix comments for failures --------- Co-authored-by: Daphne Yang Co-authored-by: Daphne Yang <139700604+daphne-sfdc@users.noreply.github.com> --- package-lock.json | 1 - .../salesforcedx-utils-vscode/package.json | 1 - .../src/cli/commandOutput.ts | 16 +++- .../src/commands/channelService.ts | 2 +- .../src/helpers/utils.ts | 37 +++++++ .../test/jest/helpers/utils.test.ts | 96 ++++++++++++++----- .../src/cli/commandOutput.ts | 19 +++- .../salesforcedx-utils/src/helpers/index.ts | 1 + .../salesforcedx-utils/src/helpers/utils.ts | 39 ++++++++ .../test/jest/cli/commandOutput.test.ts | 41 +++++++- 10 files changed, 217 insertions(+), 36 deletions(-) create mode 100644 packages/salesforcedx-utils/src/helpers/utils.ts diff --git a/package-lock.json b/package-lock.json index a1debc2658..e7b0645143 100644 --- a/package-lock.json +++ b/package-lock.json @@ -33393,7 +33393,6 @@ "applicationinsights": "1.0.7", "cross-spawn": "7.0.3", "rxjs": "^5.4.1", - "strip-ansi": "^5.2.0", "tree-kill": "^1.1.0" }, "devDependencies": { diff --git a/packages/salesforcedx-utils-vscode/package.json b/packages/salesforcedx-utils-vscode/package.json index d86153ead9..a8f7e90d92 100644 --- a/packages/salesforcedx-utils-vscode/package.json +++ b/packages/salesforcedx-utils-vscode/package.json @@ -16,7 +16,6 @@ "applicationinsights": "1.0.7", "cross-spawn": "7.0.3", "rxjs": "^5.4.1", - "strip-ansi": "^5.2.0", "tree-kill": "^1.1.0" }, "devDependencies": { diff --git a/packages/salesforcedx-utils-vscode/src/cli/commandOutput.ts b/packages/salesforcedx-utils-vscode/src/cli/commandOutput.ts index 283d18c7c0..6005d822d8 100644 --- a/packages/salesforcedx-utils-vscode/src/cli/commandOutput.ts +++ b/packages/salesforcedx-utils-vscode/src/cli/commandOutput.ts @@ -5,6 +5,7 @@ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause */ +import { stripAnsiInJson } from '../helpers/utils'; import { CommandExecution } from './commandExecutor'; export class CommandOutput { @@ -12,6 +13,7 @@ export class CommandOutput { private stderrBuffer = ''; public async getCmdResult(execution: CommandExecution): Promise { + const hasJsonEnabled = execution.command.args?.some(arg => arg === '--json'); execution.stdoutSubject.subscribe(realData => { this.stdoutBuffer += realData.toString(); }); @@ -23,9 +25,19 @@ export class CommandOutput { (resolve: (result: string) => void, reject: (reason: string) => void) => { execution.processExitSubject.subscribe(data => { if (data !== undefined && String(data) === '0') { - return resolve(this.stdoutBuffer); + return resolve(stripAnsiInJson(this.stdoutBuffer, hasJsonEnabled)); } else { - return reject(this.stderrBuffer || this.stdoutBuffer); + // Is the command is sf cli - if so, just use stdoutBuffer before stderrBuffer + if (execution.command.command === 'sf') { + return reject( + stripAnsiInJson(this.stdoutBuffer, hasJsonEnabled) || + stripAnsiInJson(this.stderrBuffer, hasJsonEnabled) + ); + } + return reject( + stripAnsiInJson(this.stderrBuffer, hasJsonEnabled) || + stripAnsiInJson(this.stdoutBuffer, hasJsonEnabled) + ); } }); } diff --git a/packages/salesforcedx-utils-vscode/src/commands/channelService.ts b/packages/salesforcedx-utils-vscode/src/commands/channelService.ts index ee2b1e1908..a8fcedb5fe 100644 --- a/packages/salesforcedx-utils-vscode/src/commands/channelService.ts +++ b/packages/salesforcedx-utils-vscode/src/commands/channelService.ts @@ -5,9 +5,9 @@ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause */ -import stripAnsi from 'strip-ansi'; import { OutputChannel, window } from 'vscode'; import { CommandExecution } from '../cli'; +import { stripAnsi } from '../helpers/utils'; import { nls } from '../messages'; import { SettingsService } from '../settings'; diff --git a/packages/salesforcedx-utils-vscode/src/helpers/utils.ts b/packages/salesforcedx-utils-vscode/src/helpers/utils.ts index 741a907e75..fb7dc0a010 100644 --- a/packages/salesforcedx-utils-vscode/src/helpers/utils.ts +++ b/packages/salesforcedx-utils-vscode/src/helpers/utils.ts @@ -85,3 +85,40 @@ export const fileUtils = { flushFilePath, extractJsonObject }; + +export const stripAnsiInJson = (str: string, hasJson: boolean): string => { + return str && hasJson ? stripAnsi(str) : str; +}; + +export const stripAnsi = (str: string): string => { + return str ? str.replaceAll(ansiRegex(), '') : str; +}; + +/* +Copied from https://github.com/sindresorhus/strip-ansi/blob/master/index.js + +MIT License +Copyright (c) Sindre Sorhus (https://sindresorhus.com) + +Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated +documentation files (the "Software"), to deal in the Software without restriction, including without +limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies +of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED +TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. +IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE +OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +*/ + +export const ansiRegex = ({ onlyFirst = false } = {}): RegExp => { + const pattern = [ + '[\\u001B\\u009B][[\\]()#;?]*(?:(?:(?:(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]+)*|[a-zA-Z\\d]+(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]*)*)?\\u0007)', + '(?:(?:\\d{1,4}(?:;\\d{0,4})*)?[\\dA-PR-TZcf-nq-uy=><~]))' + ].join('|'); + + return new RegExp(pattern, onlyFirst ? undefined : 'g'); +}; diff --git a/packages/salesforcedx-utils-vscode/test/jest/helpers/utils.test.ts b/packages/salesforcedx-utils-vscode/test/jest/helpers/utils.test.ts index 068b117416..ab9fff64ac 100644 --- a/packages/salesforcedx-utils-vscode/test/jest/helpers/utils.test.ts +++ b/packages/salesforcedx-utils-vscode/test/jest/helpers/utils.test.ts @@ -4,33 +4,79 @@ * 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 { extractJsonObject } from '../../../src/helpers/utils'; - -describe('extractJsonObject unit tests', () => { - const initialValue = { - how: 'does', - it: true, - get: 5, - handled: false - }; - const jsonString = JSON.stringify(initialValue); - - it('Should be able to parse a json string.', () => { - const result = extractJsonObject(jsonString); - expect(result).toStrictEqual(initialValue); - }); +import { extractJsonObject, stripAnsiInJson } from '../../../src/helpers/utils'; + +describe('utils tests', () => { + describe('extractJsonObject unit tests', () => { + const initialValue = { + how: 'does', + it: true, + get: 5, + handled: false + }; + const jsonString = JSON.stringify(initialValue); + + it('Should be able to parse a json string.', () => { + const result = extractJsonObject(jsonString); + expect(result).toStrictEqual(initialValue); + }); - it('Should throw error if argument is a simple text', () => { - const invalidJson = initialValue.how; - expect(() => extractJsonObject(invalidJson)).toThrow( - 'The string "does" is not a valid JSON string.' - ); + it('Should throw error if argument is a simple text', () => { + const invalidJson = initialValue.how; + expect(() => extractJsonObject(invalidJson)).toThrow( + 'The string "does" is not a valid JSON string.' + ); + }); + + it('Should throw error if argument is invalid JSON string', () => { + const invalidJson = jsonString.substring(10); + expect(() => extractJsonObject(invalidJson)).toThrow( + `The string "${invalidJson}" is not a valid JSON string.` + ); + }); }); + describe('stripAnsiInJson', () => { + it('should return the original string if hasJson is false', () => { + const input = 'some string'; + const result = stripAnsiInJson(input, false); + expect(result).toBe(input); + }); + + it('should return the stripped string if hasJson is true', () => { + const input = '\u001b[4msome string\u001b[0m'; + const result = stripAnsiInJson(input, true); + expect(result).toBe('some string'); + }); + + it('should return the original string even when it contains ANSI if hasJson is false', () => { + const input = '\u001b[4msome string\u001b[0m'; + const result = stripAnsiInJson(input, false); + expect(result).toBe('\u001b[4msome string\u001b[0m'); + }); + + it('should return the original string if hasJson is true and the string does not contain ANSI', () => { + const input = 'some string'; + const result = stripAnsiInJson(input, true); + expect(result).toBe(input); + }); + + it('should return the original JSON string if hasJson is false', () => { + const input = '{"key": "value"}'; + const result = stripAnsiInJson(input, false); + expect(result).toBe(input); + }); + + it('should return the stripped JSON string if hasJson is true', () => { + const input = '{"key": "\u001b[4mvalue\u001b[0m"}'; + const result = stripAnsiInJson(input, true); + expect(result).toBe('{"key": "value"}'); + }); - it('Should throw error if argument is invalid JSON string', () => { - const invalidJson = jsonString.substring(10); - expect(() => extractJsonObject(invalidJson)).toThrow( - `The string "${invalidJson}" is not a valid JSON string.` - ); + it('should handle complex JSON with ANSI codes', () => { + const input = + '{"key1": "\u001b[31mvalue1\u001b[0m", "key2": "\u001b[32mvalue2\u001b[0m"}'; + const result = stripAnsiInJson(input, true); + expect(result).toBe('{"key1": "value1", "key2": "value2"}'); + }); }); }); diff --git a/packages/salesforcedx-utils/src/cli/commandOutput.ts b/packages/salesforcedx-utils/src/cli/commandOutput.ts index 7dcc6f8394..15403e1a8d 100644 --- a/packages/salesforcedx-utils/src/cli/commandOutput.ts +++ b/packages/salesforcedx-utils/src/cli/commandOutput.ts @@ -5,13 +5,18 @@ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause */ +import { stripAnsiInJson } from '../helpers'; import { CommandExecution } from '../types'; +import { JSON_FLAG } from './commandBuilder'; export class CommandOutput { private stdoutBuffer = ''; private stderrBuffer = ''; public async getCmdResult(execution: CommandExecution): Promise { + const hasJsonEnabled = execution.command?.args?.some( + arg => arg === JSON_FLAG + ); execution.stdoutSubject.subscribe(realData => { this.stdoutBuffer += realData.toString(); }); @@ -23,9 +28,19 @@ export class CommandOutput { (resolve: (result: string) => void, reject: (reason: string) => void) => { execution.processExitSubject.subscribe(data => { if (data !== undefined && String(data) === '0') { - return resolve(this.stdoutBuffer); + return resolve(stripAnsiInJson(this.stdoutBuffer, hasJsonEnabled)); } else { - return reject(this.stderrBuffer || this.stdoutBuffer); + // Is the command is sf cli - if so, just use stdoutBuffer before stderrBuffer + if (execution.command.command === 'sf') { + return reject( + stripAnsiInJson(this.stdoutBuffer, hasJsonEnabled) || + stripAnsiInJson(this.stderrBuffer, hasJsonEnabled) + ); + } + return reject( + stripAnsiInJson(this.stderrBuffer, hasJsonEnabled) || + stripAnsiInJson(this.stdoutBuffer, hasJsonEnabled) + ); } }); } diff --git a/packages/salesforcedx-utils/src/helpers/index.ts b/packages/salesforcedx-utils/src/helpers/index.ts index a5e1b9b982..fd7207cccb 100644 --- a/packages/salesforcedx-utils/src/helpers/index.ts +++ b/packages/salesforcedx-utils/src/helpers/index.ts @@ -6,3 +6,4 @@ */ export { extractJsonObject } from './extractJsonObject'; export { ensureCurrentWorkingDirIsProjectPath } from './ensureCurrentWorkingDirIsProjectPath'; +export { stripAnsiInJson } from './utils'; diff --git a/packages/salesforcedx-utils/src/helpers/utils.ts b/packages/salesforcedx-utils/src/helpers/utils.ts new file mode 100644 index 0000000000..93d0351afe --- /dev/null +++ b/packages/salesforcedx-utils/src/helpers/utils.ts @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2024, 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 + */ + +export const stripAnsiInJson = (str: string, hasJson: boolean): string => { + return hasJson ? str.replaceAll(ansiRegex(), '') : str; +}; + +/* +Copied from https://github.com/sindresorhus/strip-ansi/blob/master/index.js + +MIT License +Copyright (c) Sindre Sorhus (https://sindresorhus.com) + +Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated +documentation files (the "Software"), to deal in the Software without restriction, including without +limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies +of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED +TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. +IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE +OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +*/ + +const ansiRegex = ({ onlyFirst = false } = {}): RegExp => { + const pattern = [ + '[\\u001B\\u009B][[\\]()#;?]*(?:(?:(?:(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]+)*|[a-zA-Z\\d]+(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]*)*)?\\u0007)', + '(?:(?:\\d{1,4}(?:;\\d{0,4})*)?[\\dA-PR-TZcf-nq-uy=><~]))' + ].join('|'); + + return new RegExp(pattern, onlyFirst ? undefined : 'g'); +}; diff --git a/packages/salesforcedx-utils/test/jest/cli/commandOutput.test.ts b/packages/salesforcedx-utils/test/jest/cli/commandOutput.test.ts index d590795c3a..8134e3c854 100644 --- a/packages/salesforcedx-utils/test/jest/cli/commandOutput.test.ts +++ b/packages/salesforcedx-utils/test/jest/cli/commandOutput.test.ts @@ -28,6 +28,9 @@ describe('CommandOutput Unit Tests.', () => { }, processExitSubject: { subscribe: jest.fn() + }, + command: { + command: 'sf' } }; commandOutput = new CommandOutput(); @@ -77,7 +80,8 @@ describe('CommandOutput Unit Tests.', () => { }); }); - it('Should have data from stderr on failure.', async () => { + it('Should have data from stderr on failure for not command sf.', async () => { + fakeExecution.command.command = 'notsf'; const stdoutCallback = fakeExecution.stdoutSubject.subscribe.mock.calls[0][0]; stdoutCallback(goodOutput); @@ -86,23 +90,52 @@ describe('CommandOutput Unit Tests.', () => { stderrCallback(badOutput); const exitCallback = fakeExecution.processExitSubject.subscribe.mock.calls[0][0]; - // Call the exit callback with a 0 response to indicate success + // Call the exit callback with a 1 response to indicate failure exitCallback(failCode); result.catch(outValue => { expect(outValue).toEqual(badOutput); }); }); - it('Should have data from stdout on failure if there is no stderr.', async () => { + it('Should have data from stdout on failure if there is no stderr when command not sf.', async () => { + fakeExecution.command.command = 'notsf'; const stdoutCallback = fakeExecution.stdoutSubject.subscribe.mock.calls[0][0]; stdoutCallback(goodOutput); const exitCallback = fakeExecution.processExitSubject.subscribe.mock.calls[0][0]; - // Call the exit callback with a 0 response to indicate success + // Call the exit callback with a 1 response to indicate failure exitCallback(failCode); result.catch(outValue => { expect(outValue).toEqual(goodOutput); }); }); + it('Should have data from stdout on failure for sf command.', async () => { + const stdoutCallback = + fakeExecution.stdoutSubject.subscribe.mock.calls[0][0]; + stdoutCallback(badOutput); + const stderrCallback = + fakeExecution.stderrSubject.subscribe.mock.calls[0][0]; + stderrCallback(goodOutput); + const exitCallback = + fakeExecution.processExitSubject.subscribe.mock.calls[0][0]; + // Call the exit callback with a 1 response to indicate failure + exitCallback(failCode); + result.catch(outValue => { + expect(outValue).toEqual(badOutput); + }); + }); + + it('Should have data from stderr on failure if there is no stdout when sf command.', async () => { + const stderrCallback = + fakeExecution.stderrSubject.subscribe.mock.calls[0][0]; + stderrCallback(badOutput); + const exitCallback = + fakeExecution.processExitSubject.subscribe.mock.calls[0][0]; + // Call the exit callback with a 1 response to indicate failure + exitCallback(failCode); + result.catch(outValue => { + expect(outValue).toEqual(badOutput); + }); + }); });