From 7aed0d7c2ef3dcbe6b19e93a7fceae8f04685a21 Mon Sep 17 00:00:00 2001 From: Elliot DeNolf Date: Sun, 6 Oct 2024 13:23:30 -0700 Subject: [PATCH] chore(eslint): payload logger usage (#8578) Payload uses `pino` for a logger. When using the error logger `payload.logger.error` it is possible to pass any number of arguments like this: `payload.logger.error('Some error ocurred', err)`. However, in this scenario, the full error will not be serialized by `pino`. It must be passed as the `err` property inside of an object in order to be properly serialized. This rule ensures that a user is using this function call to properly serialize the error. --- eslint.config.js | 1 + .../proper-payload-logger-usage.js | 89 +++++++++++++++++++ packages/eslint-plugin/index.mjs | 6 +- .../tests/proper-payload-logger-usage.js | 72 +++++++++++++++ 4 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 packages/eslint-plugin/customRules/proper-payload-logger-usage.js create mode 100644 packages/eslint-plugin/tests/proper-payload-logger-usage.js diff --git a/eslint.config.js b/eslint.config.js index 038f4347f..ae45e1d63 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -53,6 +53,7 @@ export const rootEslintConfig = [ 'payload/no-relative-monorepo-imports': 'error', 'payload/no-imports-from-exports-dir': 'error', 'payload/no-imports-from-self': 'error', + 'payload/proper-payload-logger-usage': 'error', }, }, { diff --git a/packages/eslint-plugin/customRules/proper-payload-logger-usage.js b/packages/eslint-plugin/customRules/proper-payload-logger-usage.js new file mode 100644 index 000000000..01817687f --- /dev/null +++ b/packages/eslint-plugin/customRules/proper-payload-logger-usage.js @@ -0,0 +1,89 @@ +export const rule = { + meta: { + type: 'problem', + docs: { + description: 'Disallow improper usage of payload.logger.error', + recommended: 'error', + }, + messages: { + improperUsage: 'Improper logger usage. Pass { msg, err } so full error stack is logged.', + wrongErrorField: 'Improper usage. Use { err } instead of { error }.', + wrongMessageField: 'Improper usage. Use { msg } instead of { message }.', + }, + schema: [], + }, + create(context) { + return { + CallExpression(node) { + const callee = node.callee + + // Function to check if the expression ends with `payload.logger.error` + function isPayloadLoggerError(expression) { + return ( + expression.type === 'MemberExpression' && + expression.property.name === 'error' && // must be `.error` + expression.object.type === 'MemberExpression' && + expression.object.property.name === 'logger' && // must be `.logger` + (expression.object.object.name === 'payload' || // handles just `payload` + (expression.object.object.type === 'MemberExpression' && + expression.object.object.property.name === 'payload')) // handles `*.payload` + ) + } + + // Check if the function being called is `payload.logger.error` or `*.payload.logger.error` + if (isPayloadLoggerError(callee)) { + const args = node.arguments + + // Case 1: Single string is passed as the argument + if ( + args.length === 1 && + args[0].type === 'Literal' && + typeof args[0].value === 'string' + ) { + return // Valid: single string argument + } + + // Case 2: Object is passed as the first argument + if (args.length > 0 && args[0].type === 'ObjectExpression') { + const properties = args[0].properties + + // Ensure no { error } key, only { err } is allowed + properties.forEach((prop) => { + if (prop.key.type === 'Identifier' && prop.key.name === 'error') { + context.report({ + node: prop, + messageId: 'wrongErrorField', + }) + } + + // Ensure no { message } key, only { msg } is allowed + if (prop.key.type === 'Identifier' && prop.key.name === 'message') { + context.report({ + node: prop, + messageId: 'wrongMessageField', + }) + } + }) + return // Valid object, checked for 'err'/'error' keys + } + + // Case 3: Improper usage (string + error or additional err/error) + if ( + args.length > 1 && + args[0].type === 'Literal' && + typeof args[0].value === 'string' && + args[1].type === 'Identifier' && + (args[1].name === 'err' || args[1].name === 'error') + ) { + context.report({ + node, + messageId: 'improperUsage', + }) + } + } + }, + } + }, +} + +export default rule diff --git a/packages/eslint-plugin/index.mjs b/packages/eslint-plugin/index.mjs index dd743b077..c2c8a0391 100644 --- a/packages/eslint-plugin/index.mjs +++ b/packages/eslint-plugin/index.mjs @@ -4,6 +4,7 @@ import noRelativeMonorepoImports from './customRules/no-relative-monorepo-import import noImportsFromExportsDir from './customRules/no-imports-from-exports-dir.js' import noFlakyAssertions from './customRules/no-flaky-assertions.js' import noImportsFromSelf from './customRules/no-imports-from-self.js' +import properPinoLoggerErrorUsage from './customRules/proper-payload-logger-usage.js' /** * @type {import('eslint').ESLint.Plugin} @@ -11,10 +12,13 @@ import noImportsFromSelf from './customRules/no-imports-from-self.js' const index = { rules: { 'no-jsx-import-statements': noJsxImportStatements, - 'no-non-retryable-assertions': noNonRetryableAssertions, 'no-relative-monorepo-imports': noRelativeMonorepoImports, 'no-imports-from-exports-dir': noImportsFromExportsDir, 'no-imports-from-self': noImportsFromSelf, + 'proper-payload-logger-usage': properPinoLoggerErrorUsage, + + // Testing-related + 'no-non-retryable-assertions': noNonRetryableAssertions, 'no-flaky-assertions': noFlakyAssertions, 'no-wait-function': { create: function (context) { diff --git a/packages/eslint-plugin/tests/proper-payload-logger-usage.js b/packages/eslint-plugin/tests/proper-payload-logger-usage.js new file mode 100644 index 000000000..1eccd5123 --- /dev/null +++ b/packages/eslint-plugin/tests/proper-payload-logger-usage.js @@ -0,0 +1,72 @@ +import { RuleTester } from 'eslint' +import rule from '../customRules/proper-payload-logger-usage.js' + +const ruleTester = new RuleTester() + +// Example tests for the rule +ruleTester.run('no-improper-payload-logger-error', rule, { + valid: [ + // Valid: payload.logger.error with object containing { msg, err } + { + code: "payload.logger.error({ msg: 'some message', err })", + }, + // Valid: payload.logger.error with a single string + { + code: "payload.logger.error('Some error message')", + }, + // Valid: *.payload.logger.error with object + { + code: "this.payload.logger.error({ msg: 'another message', err })", + }, + { + code: "args.req.payload.logger.error({ msg: 'different message', err })", + }, + ], + + invalid: [ + // Invalid: payload.logger.error with both string and error + { + code: "payload.logger.error('Some error message', err)", + errors: [ + { + messageId: 'improperUsage', + }, + ], + }, + // Invalid: *.payload.logger.error with both string and error + { + code: "this.payload.logger.error('Some error message', error)", + errors: [ + { + messageId: 'improperUsage', + }, + ], + }, + { + code: "args.req.payload.logger.error('Some error message', err)", + errors: [ + { + messageId: 'improperUsage', + }, + ], + }, + // Invalid: payload.logger.error with object containing 'message' key + { + code: "payload.logger.error({ message: 'not the right property name' })", + errors: [ + { + messageId: 'wrongMessageField', + }, + ], + }, + // Invalid: *.payload.logger.error with object containing 'error' key + { + code: "this.payload.logger.error({ msg: 'another message', error })", + errors: [ + { + messageId: 'wrongErrorField', + }, + ], + }, + ], +})