From 4add4875877dc27a8251a8e40c9b0601473eda70 Mon Sep 17 00:00:00 2001 From: Hweinstock <42325418+Hweinstock@users.noreply.github.com> Date: Thu, 7 Nov 2024 17:51:36 -0500 Subject: [PATCH] build(lint): printf/console.log format specifiers must match args #5895 ## Problem currently `logger.error(msg, err)` silently drops `err` if it doesn't map to a `%x` format specifier in msg. we want to either prevent that mistake, or enhance our logger so that it prints useful parts of err instead of silently dropping it. Also, there are cases like: `getLogger().error('Certificate path {0} already exists', certPath)`, but this silently drops the arg `certPath`. Pretty sure this syntax is from Java IIRC and not supported by Node. ## Solution Provide a custom lint rule to ensure the number of extra args matches the number of format specifiers. ### Note About Alternatives - As an alternative, we could have transformed the extra `meta` items to strings and appended them to the log message or add them as separate log messages. This would allow usage of the form `getLogger().error('hit error', e)`. - If we add separate log messages, we break the expectation that each `logger.error` statement corresponds to one message in the log. - Conversely, if we append to the log message, this is somewhat of a hidden feature not exposed to the consumer. By forcing them to add a '%O' format specifier, we are making them clearly aware of how the error (or other object) will show up in the log, ensuring the effect does not "surprise" the user. ## Notes Relevant: code used to format errors: https://github.com/aws/aws-toolkit-vscode/blob/f806ae04f5d56b872282e42d5f765b2b1a9e28dd/packages/core/src/shared/errors.ts#L329-L334 which should now actually be used since errors won't be dropped. --- .eslintrc.js | 1 + .../emitTelemetryMessageHandler.ts | 2 +- .../appBuilder/explorer/nodes/deployedNode.ts | 4 +- .../appBuilder/explorer/samProject.ts | 2 +- .../src/awsService/iot/commands/createCert.ts | 6 +- .../redshift/explorer/redshiftNode.ts | 2 +- .../src/awsService/s3/commands/uploadFile.ts | 2 +- packages/core/src/codewhisperer/activation.ts | 2 +- .../core/src/codewhisperer/client/agent.ts | 2 +- .../commands/startSecurityScan.ts | 2 +- .../commands/startTransformByQ.ts | 2 +- .../core/src/codewhisperer/util/zipUtil.ts | 2 +- .../vue/configEditor/samInvokeBackend.ts | 2 +- .../lambda/vue/remoteInvoke/invokeLambda.ts | 4 +- .../src/lambda/wizards/samDeployWizard.ts | 2 +- .../webview/vue/toolkit/backend_toolkit.ts | 2 +- packages/core/src/shared/env/resolveEnv.ts | 2 +- .../core/src/shared/filesystemUtilities.ts | 2 +- packages/core/src/shared/sam/activation.ts | 2 +- .../src/shared/sam/cli/samCliLocalInvoke.ts | 2 +- packages/core/src/shared/ui/picker.ts | 2 +- .../core/src/shared/utilities/zipStream.ts | 2 +- .../ssmDocument/commands/deleteDocument.ts | 2 +- .../ssmDocument/commands/openDocumentItem.ts | 2 +- .../commands/updateDocumentVersion.ts | 2 +- .../awsService/appBuilder/samProject.test.ts | 2 +- .../vue/remoteInvoke/invokeLambda.test.ts | 2 +- .../test/lambda/vue/samInvokeBackend.test.ts | 2 +- .../explorer/nodes/deployedNode.test.ts | 8 +-- plugins/eslint-plugin-aws-toolkits/index.ts | 2 + .../lib/rules/no-json-stringify-in-log.ts | 2 +- .../lib/rules/no-printf-mismatch.ts | 62 +++++++++++++++++++ .../test/rules/no-printf-mismatch.test.ts | 32 ++++++++++ 33 files changed, 133 insertions(+), 36 deletions(-) create mode 100644 plugins/eslint-plugin-aws-toolkits/lib/rules/no-printf-mismatch.ts create mode 100644 plugins/eslint-plugin-aws-toolkits/test/rules/no-printf-mismatch.test.ts diff --git a/.eslintrc.js b/.eslintrc.js index a57d304c341..5d196c59541 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -163,6 +163,7 @@ module.exports = { 'aws-toolkits/no-string-exec-for-child-process': 'error', 'aws-toolkits/no-console-log': 'error', 'aws-toolkits/no-json-stringify-in-log': 'error', + 'aws-toolkits/no-printf-mismatch': 'error', 'no-restricted-imports': [ 'error', { diff --git a/packages/core/src/applicationcomposer/messageHandlers/emitTelemetryMessageHandler.ts b/packages/core/src/applicationcomposer/messageHandlers/emitTelemetryMessageHandler.ts index 9ca92ea445b..9b0506b9196 100644 --- a/packages/core/src/applicationcomposer/messageHandlers/emitTelemetryMessageHandler.ts +++ b/packages/core/src/applicationcomposer/messageHandlers/emitTelemetryMessageHandler.ts @@ -67,7 +67,7 @@ export function emitTelemetryMessageHandler(message: EmitTelemetryMessage) { return } } catch (e) { - getLogger().error('Could not log telemetry for App Composer', e) + getLogger().error('Could not log telemetry for App Composer %O', e) } } diff --git a/packages/core/src/awsService/appBuilder/explorer/nodes/deployedNode.ts b/packages/core/src/awsService/appBuilder/explorer/nodes/deployedNode.ts index 70ac56bb1f6..913cdd067e0 100644 --- a/packages/core/src/awsService/appBuilder/explorer/nodes/deployedNode.ts +++ b/packages/core/src/awsService/appBuilder/explorer/nodes/deployedNode.ts @@ -96,7 +96,7 @@ export async function generateDeployedNode( .Configuration as Lambda.FunctionConfiguration newDeployedResource = new LambdaFunctionNode(lambdaNode, regionCode, configuration) } catch (error: any) { - getLogger().error('Error getting Lambda configuration') + getLogger().error('Error getting Lambda configuration %O', error) throw ToolkitError.chain(error, 'Error getting Lambda configuration', { code: 'lambdaClientError', }) @@ -153,7 +153,7 @@ export async function generateDeployedNode( } default: newDeployedResource = new DeployedResourceNode(deployedResource) - getLogger().info('Details are missing or are incomplete for:', deployedResource) + getLogger().info('Details are missing or are incomplete for: %O', deployedResource) return [ createPlaceholderItem( localize('AWS.appBuilder.explorerNode.noApps', '[This resource is not yet supported.]') diff --git a/packages/core/src/awsService/appBuilder/explorer/samProject.ts b/packages/core/src/awsService/appBuilder/explorer/samProject.ts index fdb4b8e2117..fd571cd6be8 100644 --- a/packages/core/src/awsService/appBuilder/explorer/samProject.ts +++ b/packages/core/src/awsService/appBuilder/explorer/samProject.ts @@ -42,7 +42,7 @@ export async function getStackName(projectRoot: vscode.Uri): Promise { } catch (error: any) { switch (error.code) { case SamConfigErrorCode.samNoConfigFound: - getLogger().info('No stack name or region information available in samconfig.toml', error) + getLogger().info('No stack name or region information available in samconfig.toml: %O', error) break case SamConfigErrorCode.samConfigParseError: getLogger().error(`Error getting stack name or region information: ${error.message}`, error) diff --git a/packages/core/src/awsService/iot/commands/createCert.ts b/packages/core/src/awsService/iot/commands/createCert.ts index c3b65cf886d..22db3136831 100644 --- a/packages/core/src/awsService/iot/commands/createCert.ts +++ b/packages/core/src/awsService/iot/commands/createCert.ts @@ -115,14 +115,14 @@ async function saveCredentials( const publicKeyExists = await fileExists(publicKeyPath) if (certExists) { - getLogger().error('Certificate path {0} already exists', certPath) + getLogger().error('Certificate path %s already exists', certPath) void vscode.window.showErrorMessage( localize('AWS.iot.createCert.error', 'Failed to create certificate. Path {0} already exists.', certPath) ) return false } if (privateKeyExists) { - getLogger().error('Key path {0} already exists', privateKeyPath) + getLogger().error('Key path %s already exists', privateKeyPath) void vscode.window.showErrorMessage( localize( 'AWS.iot.createCert.error', @@ -133,7 +133,7 @@ async function saveCredentials( return false } if (publicKeyExists) { - getLogger().error('Key path {0} already exists', publicKeyPath) + getLogger().error('Key path %s already exists', publicKeyPath) void vscode.window.showErrorMessage( localize( 'AWS.iot.createCert.error', diff --git a/packages/core/src/awsService/redshift/explorer/redshiftNode.ts b/packages/core/src/awsService/redshift/explorer/redshiftNode.ts index 4d923192f22..52eb470283b 100644 --- a/packages/core/src/awsService/redshift/explorer/redshiftNode.ts +++ b/packages/core/src/awsService/redshift/explorer/redshiftNode.ts @@ -127,7 +127,7 @@ export class RedshiftNode extends AWSTreeNodeBase implements LoadMoreNode { newServerlessToken = response.nextToken ?? '' } } catch (error) { - getLogger().error("Serverless workgroup operation isn't supported or failed:", error) + getLogger().error("Serverless workgroup operation isn't supported or failed: %O", error) // Continue without interrupting the provisioned cluster loading } } diff --git a/packages/core/src/awsService/s3/commands/uploadFile.ts b/packages/core/src/awsService/s3/commands/uploadFile.ts index 4bfcc91e027..ca77617af4e 100644 --- a/packages/core/src/awsService/s3/commands/uploadFile.ts +++ b/packages/core/src/awsService/s3/commands/uploadFile.ts @@ -415,7 +415,7 @@ export async function promptUserForBucket( try { allBuckets = await s3client.listAllBuckets() } catch (e) { - getLogger().error('Failed to list buckets from client', e) + getLogger().error('Failed to list buckets from client %O', e) void vscode.window.showErrorMessage( localize('AWS.message.error.promptUserForBucket.listBuckets', 'Failed to list buckets from client') ) diff --git a/packages/core/src/codewhisperer/activation.ts b/packages/core/src/codewhisperer/activation.ts index ff78d8a99ef..a9a74a67e5f 100644 --- a/packages/core/src/codewhisperer/activation.ts +++ b/packages/core/src/codewhisperer/activation.ts @@ -618,6 +618,6 @@ export async function enableDefaultConfigCloud9() { await editorSettings.update('acceptSuggestionOnEnter', 'on', vscode.ConfigurationTarget.Global) await editorSettings.update('snippetSuggestions', 'top', vscode.ConfigurationTarget.Global) } catch (error) { - getLogger().error('amazonq: Failed to update user settings', error) + getLogger().error('amazonq: Failed to update user settings %O', error) } } diff --git a/packages/core/src/codewhisperer/client/agent.ts b/packages/core/src/codewhisperer/client/agent.ts index d825e94d8d1..52152d2a616 100644 --- a/packages/core/src/codewhisperer/client/agent.ts +++ b/packages/core/src/codewhisperer/client/agent.ts @@ -82,6 +82,6 @@ export function initializeNetworkAgent(): void { } } catch (error) { // Log any errors in the patching logic - getLogger().error('Failed to patch http agent', error) + getLogger().error('Failed to patch http agent %O', error) } } diff --git a/packages/core/src/codewhisperer/commands/startSecurityScan.ts b/packages/core/src/codewhisperer/commands/startSecurityScan.ts index 58eaee2bf40..5bd0b1552cf 100644 --- a/packages/core/src/codewhisperer/commands/startSecurityScan.ts +++ b/packages/core/src/codewhisperer/commands/startSecurityScan.ts @@ -225,7 +225,7 @@ export async function startSecurityScan( logger.verbose(`Security scan completed.`) } catch (error) { - getLogger().error('Security scan failed.', error) + getLogger().error('Security scan failed. %O', error) if (error instanceof CodeScanStoppedError) { codeScanTelemetryEntry.result = 'Cancelled' } else { diff --git a/packages/core/src/codewhisperer/commands/startTransformByQ.ts b/packages/core/src/codewhisperer/commands/startTransformByQ.ts index edf48ed00c5..7acbf04f32e 100644 --- a/packages/core/src/codewhisperer/commands/startTransformByQ.ts +++ b/packages/core/src/codewhisperer/commands/startTransformByQ.ts @@ -142,7 +142,7 @@ export async function validateSQLMetadataFile(fileContents: string, message: any `CodeTransformation: Parsed .sct file with source DB: ${sourceDB}, target DB: ${targetDB}, source host name: ${sourceServerName}, and schema names: ${Array.from(schemaNames)}` ) } catch (err: any) { - getLogger().error('CodeTransformation: Error parsing .sct file.', err) + getLogger().error('CodeTransformation: Error parsing .sct file. %O', err) transformByQState.getChatMessenger()?.sendUnrecoverableErrorResponse('error-parsing-sct-file', message.tabID) return false } diff --git a/packages/core/src/codewhisperer/util/zipUtil.ts b/packages/core/src/codewhisperer/util/zipUtil.ts index b0c22eed885..2d41da94f98 100644 --- a/packages/core/src/codewhisperer/util/zipUtil.ts +++ b/packages/core/src/codewhisperer/util/zipUtil.ts @@ -259,7 +259,7 @@ export class ZipUtil { language: this._language, } } catch (error) { - getLogger().error('Zip error caused by:', error) + getLogger().error('Zip error caused by: %O', error) throw error } } diff --git a/packages/core/src/lambda/vue/configEditor/samInvokeBackend.ts b/packages/core/src/lambda/vue/configEditor/samInvokeBackend.ts index 9e3eed9980b..643ea4631e2 100644 --- a/packages/core/src/lambda/vue/configEditor/samInvokeBackend.ts +++ b/packages/core/src/lambda/vue/configEditor/samInvokeBackend.ts @@ -269,7 +269,7 @@ export class SamInvokeWebview extends VueWebview { selectedFile: this.getFileName(fileLocations[0].fsPath), } } catch (e) { - getLogger().error('readFileSync: Failed to read file at path %O', fileLocations[0].fsPath, e) + getLogger().error('readFileSync: Failed to read file at path %s %O', fileLocations[0].fsPath, e) throw ToolkitError.chain(e, 'Failed to read selected file') } } diff --git a/packages/core/src/lambda/vue/remoteInvoke/invokeLambda.ts b/packages/core/src/lambda/vue/remoteInvoke/invokeLambda.ts index 36ea36a55c5..501304c1a94 100644 --- a/packages/core/src/lambda/vue/remoteInvoke/invokeLambda.ts +++ b/packages/core/src/lambda/vue/remoteInvoke/invokeLambda.ts @@ -128,7 +128,7 @@ export class RemoteInvokeWebview extends VueWebview { selectedFile: this.getFileName(fileLocations[0].fsPath), } } catch (e) { - getLogger().error('readFileSync: Failed to read file at path %O', fileLocations[0].fsPath, e) + getLogger().error('readFileSync: Failed to read file at path %s %O', fileLocations[0].fsPath, e) throw ToolkitError.chain(e, 'Failed to read selected file') } } @@ -151,7 +151,7 @@ export class RemoteInvokeWebview extends VueWebview { selectedFile: this.getFileName(fileLocation.fsPath), } } catch (e) { - getLogger().error('readFileSync: Failed to read file at path %O', fileLocation.fsPath, e) + getLogger().error('readFileSync: Failed to read file at path %s %O', fileLocation.fsPath, e) throw ToolkitError.chain(e, 'Failed to read selected file') } } diff --git a/packages/core/src/lambda/wizards/samDeployWizard.ts b/packages/core/src/lambda/wizards/samDeployWizard.ts index 0f0ef4d9cf7..a45e07b0b74 100644 --- a/packages/core/src/lambda/wizards/samDeployWizard.ts +++ b/packages/core/src/lambda/wizards/samDeployWizard.ts @@ -992,7 +992,7 @@ async function populateS3QuickPick( }) } } catch (e) { - getLogger().error('Recent bucket JSON not parseable.', e) + getLogger().error('Recent bucket JSON not parseable: %O', e) } if (isCloud9() && recent !== cloud9Bucket) { diff --git a/packages/core/src/login/webview/vue/toolkit/backend_toolkit.ts b/packages/core/src/login/webview/vue/toolkit/backend_toolkit.ts index ffdad51dfa8..aa81329948f 100644 --- a/packages/core/src/login/webview/vue/toolkit/backend_toolkit.ts +++ b/packages/core/src/login/webview/vue/toolkit/backend_toolkit.ts @@ -96,7 +96,7 @@ export class ToolkitLoginWebview extends CommonAuthWebview { await setContext('aws.explorer.showAuthView', false) await this.showResourceExplorer() } catch (e) { - getLogger().error('Failed submitting credentials', e) + getLogger().error('Failed submitting credentials %O', e) return { id: this.id, text: e as string } } } diff --git a/packages/core/src/shared/env/resolveEnv.ts b/packages/core/src/shared/env/resolveEnv.ts index e7cb1e6c3e6..5c6db4bc2c1 100644 --- a/packages/core/src/shared/env/resolveEnv.ts +++ b/packages/core/src/shared/env/resolveEnv.ts @@ -303,7 +303,7 @@ async function doResolveUnixShellEnv(timeout: Timeout): Promise { // give quickpick item an error message // we should not blow away the existing items, they should still be viable const err = e as Error - getLogger().error('Error while loading items for IteratingQuickPickController:', err) + getLogger().error('Error while loading items for IteratingQuickPickController: %O', err) resolve({ value: [ { diff --git a/packages/core/src/shared/utilities/zipStream.ts b/packages/core/src/shared/utilities/zipStream.ts index f4d9648957a..a1389676b75 100644 --- a/packages/core/src/shared/utilities/zipStream.ts +++ b/packages/core/src/shared/utilities/zipStream.ts @@ -135,7 +135,7 @@ export class ZipStream { while (!finished) { finished = await new Promise((resolve) => { setTimeout(() => { - getLogger().verbose('success is', this._numberOfFilesSucceeded, '/', this._numberOfFilesToStream) + getLogger().verbose(`success is ${this._numberOfFilesSucceeded}/${this._numberOfFilesToStream}`) onProgress?.(Math.floor((100 * this._numberOfFilesSucceeded) / this._numberOfFilesToStream)) resolve(this._numberOfFilesToStream <= this._numberOfFilesSucceeded) }, 1000) diff --git a/packages/core/src/ssmDocument/commands/deleteDocument.ts b/packages/core/src/ssmDocument/commands/deleteDocument.ts index eb30e1d24ad..993413b6761 100644 --- a/packages/core/src/ssmDocument/commands/deleteDocument.ts +++ b/packages/core/src/ssmDocument/commands/deleteDocument.ts @@ -60,7 +60,7 @@ export async function deleteDocument(node: DocumentItemNodeWriteable) { } catch (err) { result = 'Failed' const error = err as Error - logger.error('Error on deleting document: %0', error) + logger.error('Error on deleting document: %O', error) void showViewLogsMessage( localize( 'AWS.message.error.ssmDocument.deleteDocument.could_not_delete', diff --git a/packages/core/src/ssmDocument/commands/openDocumentItem.ts b/packages/core/src/ssmDocument/commands/openDocumentItem.ts index 434ca7627b8..c794e56ff71 100644 --- a/packages/core/src/ssmDocument/commands/openDocumentItem.ts +++ b/packages/core/src/ssmDocument/commands/openDocumentItem.ts @@ -44,7 +44,7 @@ export async function openDocumentItem(node: DocumentItemNode, awsContext: AwsCo } catch (err) { result = 'Failed' const error = err as Error - logger.error('Error on opening document: %0', error) + logger.error('Error on opening document: %O', error) void showViewLogsMessage( localize( 'AWS.message.error.ssmDocument.openDocument.could_not_open', diff --git a/packages/core/src/ssmDocument/commands/updateDocumentVersion.ts b/packages/core/src/ssmDocument/commands/updateDocumentVersion.ts index 04a467422f7..576f38a3e75 100644 --- a/packages/core/src/ssmDocument/commands/updateDocumentVersion.ts +++ b/packages/core/src/ssmDocument/commands/updateDocumentVersion.ts @@ -70,7 +70,7 @@ export async function updateDocumentVersion(node: DocumentItemNodeWriteable, aws node.documentName ) ) - logger.error('Error on updating document version: %0', error) + logger.error('Error on updating document version: %O', error) } finally { telemetry.ssm_updateDocumentVersion.emit({ result: result }) } diff --git a/packages/core/src/test/awsService/appBuilder/samProject.test.ts b/packages/core/src/test/awsService/appBuilder/samProject.test.ts index 1b60c06e0ae..1d884ddd694 100644 --- a/packages/core/src/test/awsService/appBuilder/samProject.test.ts +++ b/packages/core/src/test/awsService/appBuilder/samProject.test.ts @@ -78,7 +78,7 @@ describe('samProject', () => { // simulate error when no samconfig.toml file in directory const result = await getStackName(projectRoot) assert.deepStrictEqual(result, {}) - assertLogsContain('No stack name or region information available in samconfig.toml', true, 'info') + assertLogsContain('No stack name or region information available in samconfig.toml: %O', true, 'info') }) it('returns empty object give error parsing samconfig file', async () => { diff --git a/packages/core/src/test/lambda/vue/remoteInvoke/invokeLambda.test.ts b/packages/core/src/test/lambda/vue/remoteInvoke/invokeLambda.test.ts index 823472bfb68..5878dca3bad 100644 --- a/packages/core/src/test/lambda/vue/remoteInvoke/invokeLambda.test.ts +++ b/packages/core/src/test/lambda/vue/remoteInvoke/invokeLambda.test.ts @@ -198,7 +198,7 @@ describe('RemoteInvokeWebview', () => { new Error('Failed to read selected file') ) assert.strictEqual(loggerErrorStub.calledOnce, true) - assert.strictEqual(loggerErrorStub.firstCall.args[0], 'readFileSync: Failed to read file at path %O') + assert.strictEqual(loggerErrorStub.firstCall.args[0], 'readFileSync: Failed to read file at path %s %O') assert.strictEqual(loggerErrorStub.firstCall.args[1], fileUri.fsPath) assert(loggerErrorStub.firstCall.args[2] instanceof Error) } finally { diff --git a/packages/core/src/test/lambda/vue/samInvokeBackend.test.ts b/packages/core/src/test/lambda/vue/samInvokeBackend.test.ts index 8f3eeff2e21..25b3d1eac8f 100644 --- a/packages/core/src/test/lambda/vue/samInvokeBackend.test.ts +++ b/packages/core/src/test/lambda/vue/samInvokeBackend.test.ts @@ -446,7 +446,7 @@ describe('SamInvokeWebview', () => { new Error('Failed to read selected file') ) assert.strictEqual(loggerErrorStub.calledOnce, true) - assert.strictEqual(loggerErrorStub.firstCall.args[0], 'readFileSync: Failed to read file at path %O') + assert.strictEqual(loggerErrorStub.firstCall.args[0], 'readFileSync: Failed to read file at path %s %O') assert.strictEqual(loggerErrorStub.firstCall.args[1], fileUri.fsPath) assert(loggerErrorStub.firstCall.args[2] instanceof Error) } finally { diff --git a/packages/core/src/test/shared/applicationBuilder/explorer/nodes/deployedNode.test.ts b/packages/core/src/test/shared/applicationBuilder/explorer/nodes/deployedNode.test.ts index 8579d90fdbb..07e7ea04bae 100644 --- a/packages/core/src/test/shared/applicationBuilder/explorer/nodes/deployedNode.test.ts +++ b/packages/core/src/test/shared/applicationBuilder/explorer/nodes/deployedNode.test.ts @@ -223,8 +223,8 @@ describe('generateDeployedNode', () => { lambdaDeployedNodeInput.resourceTreeEntity ) - assert(loggerErrorStub.calledOnceWith('Error getting Lambda configuration')) - assert(loggerErrorStub.neverCalledWith('Error getting Lambda V3 configuration')) + assert(loggerErrorStub.calledOnceWith('Error getting Lambda configuration %O')) + assert(loggerErrorStub.neverCalledWith('Error getting Lambda V3 configuration %O')) assert(deployedResourceNodes.length === 1) // Check placeholder propertries @@ -375,10 +375,10 @@ describe('generateDeployedNode', () => { unsupportTypeInput.resourceTreeEntity ) - assert(loggerInfoStub.calledOnceWith('Details are missing or are incomplete for:')) + assert(loggerInfoStub.calledOnceWith('Details are missing or are incomplete for: %O')) // Check deployedResourceNodes array propertries - assert(loggerErrorStub.neverCalledWith('Error getting Lambda V3 configuration')) + assert(loggerErrorStub.neverCalledWith('Error getting Lambda V3 configuration %O')) // Check deployedResourceNodes array propertries assert(deployedResourceNodes.length === 1) diff --git a/plugins/eslint-plugin-aws-toolkits/index.ts b/plugins/eslint-plugin-aws-toolkits/index.ts index df8ab4d2026..87ee111c095 100644 --- a/plugins/eslint-plugin-aws-toolkits/index.ts +++ b/plugins/eslint-plugin-aws-toolkits/index.ts @@ -10,6 +10,7 @@ import NoOnlyInTests from './lib/rules/no-only-in-tests' import NoStringExecForChildProcess from './lib/rules/no-string-exec-for-child-process' import NoConsoleLog from './lib/rules/no-console-log' import noJsonStringifyInLog from './lib/rules/no-json-stringify-in-log' +import noPrintfMismatch from './lib/rules/no-printf-mismatch' const rules = { 'no-await-on-vscode-msg': NoAwaitOnVscodeMsg, @@ -19,6 +20,7 @@ const rules = { 'no-string-exec-for-child-process': NoStringExecForChildProcess, 'no-console-log': NoConsoleLog, 'no-json-stringify-in-log': noJsonStringifyInLog, + 'no-printf-mismatch': noPrintfMismatch, } export { rules } diff --git a/plugins/eslint-plugin-aws-toolkits/lib/rules/no-json-stringify-in-log.ts b/plugins/eslint-plugin-aws-toolkits/lib/rules/no-json-stringify-in-log.ts index dd6696f00c5..92a9cd75a35 100644 --- a/plugins/eslint-plugin-aws-toolkits/lib/rules/no-json-stringify-in-log.ts +++ b/plugins/eslint-plugin-aws-toolkits/lib/rules/no-json-stringify-in-log.ts @@ -34,7 +34,7 @@ function isTemplateWithStringifyCall(node: TSESTree.CallExpressionArgument): boo * Check if node is representing syntax of the form getLogger().f(msg) for some f and msg or * if it is doing so indirectly via a logger variable. */ -function isLoggerCall(node: TSESTree.CallExpression): boolean { +export function isLoggerCall(node: TSESTree.CallExpression): boolean { return ( node.callee.type === AST_NODE_TYPES.MemberExpression && (isGetLoggerCall(node.callee.object) || isDisguisedGetLoggerCall(node.callee.object)) && diff --git a/plugins/eslint-plugin-aws-toolkits/lib/rules/no-printf-mismatch.ts b/plugins/eslint-plugin-aws-toolkits/lib/rules/no-printf-mismatch.ts new file mode 100644 index 00000000000..41485540a04 --- /dev/null +++ b/plugins/eslint-plugin-aws-toolkits/lib/rules/no-printf-mismatch.ts @@ -0,0 +1,62 @@ +/*! + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ +import { AST_NODE_TYPES, ESLintUtils, TSESTree } from '@typescript-eslint/utils' +import { isLoggerCall } from './no-json-stringify-in-log' +import { Rule } from 'eslint' + +/** + * reuse solution done in logger itself: https://github.com/winstonjs/winston/blob/195e55c7e7fc58914ae4967ea7b832c9e0ced930/lib/winston/logger.js#L27 + */ +function countSubTokens(literalNode: TSESTree.StringLiteral) { + const formatRegExp = /%[scdjifoO%]/g + return literalNode.value.match(formatRegExp)?.length || 0 +} +/** + * Form the error message using templates or actual values. + * Allows us to avoid copy pasting message into test file. + */ +export function formErrorMsg(substitutionTokens: string | number, numArgs: string | number): string { + return `printf-style (console.log) call has ${substitutionTokens} format specifiers, but ${numArgs} arguments.` +} + +export default ESLintUtils.RuleCreator.withoutDocs({ + meta: { + docs: { + description: 'ensure string substitution args and templates match', + recommended: 'recommended', + }, + messages: { + errMsg: formErrorMsg('{{ substitutionTokens }}', '{{ args }}'), + }, + type: 'problem', + fixable: 'code', + schema: [], + }, + defaultOptions: [], + create(context) { + return { + CallExpression(node: TSESTree.CallExpression) { + if ( + isLoggerCall(node) && + node.arguments[0].type === AST_NODE_TYPES.Literal && + typeof node.arguments[0].value === 'string' + ) { + const numSubTokens = countSubTokens(node.arguments[0]) + const numExtraArgs = node.arguments.length - 1 + if (numSubTokens !== numExtraArgs) { + return context.report({ + node: node, + data: { + substitutionTokens: numSubTokens, + args: numExtraArgs, + }, + messageId: 'errMsg', + }) + } + } + }, + } + }, +}) as unknown as Rule.RuleModule diff --git a/plugins/eslint-plugin-aws-toolkits/test/rules/no-printf-mismatch.test.ts b/plugins/eslint-plugin-aws-toolkits/test/rules/no-printf-mismatch.test.ts new file mode 100644 index 00000000000..160f6dc505a --- /dev/null +++ b/plugins/eslint-plugin-aws-toolkits/test/rules/no-printf-mismatch.test.ts @@ -0,0 +1,32 @@ +/*! + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +import { rules } from '../../index' +import { formErrorMsg } from '../../lib/rules/no-printf-mismatch' +import { getRuleTester } from '../testUtil' + +getRuleTester().run('no-string-sub-mismatch', rules['no-printf-mismatch'], { + valid: [ + 'getLogger().debug("this is a string %s and a number %d", "s", 2)', + 'getLogger().debug("this is a number %d", 2)', + 'getLogger().debug("this has no substitutions")', + 'getLogger().debug("1 %s 2 %d 3 %O 4 %o 5 %s", arg1, arg2, arg3, arg4, arg5)', + 'getLogger().debug("not real a sub-token %z")', + ], + invalid: [ + { + code: 'getLogger().debug("this is a string %s and a number %d", "s")', + errors: [formErrorMsg(2, 1)], + }, + { + code: 'getLogger().debug("this is a string %s a string %s a string %s")', + errors: [formErrorMsg(3, 0)], + }, + { + code: 'getLogger().debug("this is a string", err)', + errors: [formErrorMsg(0, 1)], + }, + ], +})