Skip to content

Protects against logger failure #555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
w3nl wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Protects against logger failure #555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a5703c
Protects against logger failure
pieterwigboldus-visma Feb 19, 2026
09ef6a8
Update lock file
pieterwigboldus-visma Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 57 additions & 50 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 26 additions & 16 deletions src/express-callback.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@
/**
* Make an express callback for the controller
* @param {object} params
* @param {Function} params.controller

Check warning on line 15 in src/express-callback.js

View workflow job for this annotation

GitHub Actions / build 

Prefer a more specific type to `Function`
* @param {object} params.specification
* @param {boolean=} params.errorDetails
* @param {Logger=} params.logger
* @param {object=} params.meta
* @param {boolean=} params.mock
* @returns {Function}

Check warning on line 21 in src/express-callback.js

View workflow job for this annotation

GitHub Actions / build 

Prefer a more specific type to `Function`
*/
export const makeExpressCallback
= ({ controller, specification, errorDetails, logger, meta, mock }) =>
Expand All @@ -28,9 +28,9 @@
* @param {Context} context
* @param {Request} request
* @param {Response} response
* @returns {Promise<any>}

Check warning on line 31 in src/express-callback.js

View workflow job for this annotation

GitHub Actions / build 

Prefer a more specific type to `any`
*/
async (context, request, response) => {

Check failure on line 33 in src/express-callback.js

View workflow job for this annotation

GitHub Actions / build 

Async arrow function has a complexity of 25. Maximum allowed is 20

Check warning on line 33 in src/express-callback.js

View workflow job for this annotation

GitHub Actions / build 

Async arrow function has too many statements (27). Maximum allowed is 20
const startTime = hrtime()
try {
const allParameters = {
Expand Down Expand Up @@ -67,27 +67,37 @@
const responseBody = await controller(feedback)
const responseTime = hrtime(startTime)[1] / 1000000 // convert to milliseconds

logger.debug({
url,
parameters,
post: request.body,
response: responseBody,
method,
ip,
userAgent,
responseTime,
statusCode: response.statusCode || 200,
message: 'access'
})
try {
logger?.debug({
url,
parameters,
post: request.body,
response: responseBody,
method,
ip,
userAgent,
responseTime,
statusCode: response.statusCode || 200,
message: 'access'
})
} catch (logError) {
// Prevent logging errors from affecting the request
console.error('Logger failed:', logError)

Check warning on line 85 in src/express-callback.js

View workflow job for this annotation

GitHub Actions / build 

Unexpected console statement
Comment on lines +83 to +85
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using console.error as a fallback when the logger fails creates a circular issue: if the logger failed due to a general logging infrastructure problem, console.error might also fail or be unavailable in certain environments. Additionally, this creates inconsistent logging output (some logs go through the configured logger, others through console), making log aggregation and monitoring more difficult. Consider either: 1) silently swallowing the error if resilience is the primary goal, or 2) using a more robust fallback mechanism.

Copilot uses AI. Check for mistakes.
}
Comment on lines +70 to +86
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent null-safety pattern with logger parameter. The code uses optional chaining (logger?.debug) inside a try-catch block, but optional chaining already prevents errors if logger is null/undefined. Either remove the optional chaining operator (since the try-catch will handle any errors), or remove the try-catch block (since optional chaining prevents the error). The current implementation provides double protection that is redundant.

Copilot uses AI. Check for mistakes.
Comment on lines +70 to +86
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new error handling behavior for logger failures is not covered by tests. Since express-callback.test.js exists and tests other aspects of this function, consider adding test cases to verify that: 1) the application continues to work correctly when the logger throws an error, and 2) the fallback console.error is called when the logger fails. This ensures the resilience behavior works as intended.

Copilot uses AI. Check for mistakes.
Comment on lines +70 to +86
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider whether this defensive error handling is necessary. Most production-grade logging libraries (Winston, Pino, Bunyan) handle errors internally and don't throw synchronous exceptions. If the logger is throwing errors, it typically indicates a configuration issue or bug that should be fixed rather than silenced. The added try-catch blocks increase code complexity and may hide underlying problems. Consider documenting which specific logger failure scenarios this protects against, or whether the logger interface requirements should be documented instead.

Copilot uses AI. Check for mistakes.

return responseBody
} catch (error) {
const errorCodeStatus = getStatusByError(error)

if (errorCodeStatus >= 500) {
logger.error(error)
} else {
logger.warn(error)
try {
if (errorCodeStatus >= 500) {
logger?.error(error)
} else {
logger?.warn(error)
}
} catch (logError) {
// Prevent logging errors from affecting the request
console.error('Logger failed:', logError)
Comment on lines +98 to +100
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using console.error as a fallback when the logger fails creates a circular issue: if the logger failed due to a general logging infrastructure problem, console.error might also fail or be unavailable in certain environments. Additionally, this creates inconsistent logging output (some logs go through the configured logger, others through console), making log aggregation and monitoring more difficult. Consider either: 1) silently swallowing the error if resilience is the primary goal, or 2) using a more robust fallback mechanism.

Copilot uses AI. Check for mistakes.
}
Comment on lines +92 to 101
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent null-safety pattern with logger parameter. The code uses optional chaining (logger?.error and logger?.warn) inside a try-catch block, but optional chaining already prevents errors if logger is null/undefined. Either remove the optional chaining operator (since the try-catch will handle any errors), or remove the try-catch block (since optional chaining prevents the error). The current implementation provides double protection that is redundant.

Copilot uses AI. Check for mistakes.
Comment on lines +92 to 101
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new error handling behavior for logger failures in the error handling path is not covered by tests. Since express-callback.test.js exists and tests error scenarios, consider adding test cases to verify that: 1) the application continues to handle errors correctly when the logger throws an error, and 2) the fallback console.error is called when the logger fails during error logging. This ensures the resilience behavior works as intended even in error scenarios.

Copilot uses AI. Check for mistakes.

response.status(errorCodeStatus)
Expand Down
19 changes: 12 additions & 7 deletions src/handlers/response-validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,18 @@ export default (logger, validateResponse) => (context, request, response) => {
: null
if (valid?.errors) {
if (logger) {
logger.error({
message: 'Response validation failed',
errors: valid.errors,
operation: context.operation,
statusCode: response.statusCode,
response: context.response
})
try {
logger.error({
message: 'Response validation failed',
errors: valid.errors,
operation: context.operation,
statusCode: response.statusCode,
response: context.response
})
} catch (logError) {
// Prevent logging errors from affecting the response
console.error('Logger failed:', logError)
Comment on lines +31 to +32
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using console.error as a fallback when the logger fails creates a circular issue: if the logger failed due to a general logging infrastructure problem, console.error might also fail or be unavailable in certain environments. Additionally, this creates inconsistent logging output (some logs go through the configured logger, others through console), making log aggregation and monitoring more difficult. Consider either: 1) silently swallowing the error if resilience is the primary goal, or 2) using a more robust fallback mechanism.

Suggested change
// Prevent logging errors from affecting the response
console.error('Logger failed:', logError)
// Intentionally ignore logging errors to avoid interfering with the response

Copilot uses AI. Check for mistakes.
}
Comment on lines 21 to +33
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent logger null-safety approach between files. In response-validation.js, the code uses if (logger) check followed by a try-catch, while in express-callback.js, it uses optional chaining (logger?.) with try-catch. For consistency and clarity, the same pattern should be used throughout the codebase. Consider standardizing on one approach across both files.

Copilot uses AI. Check for mistakes.
}
if (!response.headersSent) {
return response.status(502).json({
Expand Down
Loading