Skip to content

fix: correctly handle other errors from handlers #1033

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.

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
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jpulec
Copy link
Contributor

@jpulec jpulec commented Apr 11, 2025

Code Quality bug fix

Author Description

Title:
This fixes an issue where some unhandled errors thrown by tryPost cause big issues. Due to some loose typing, it was possible for the handlers to return null for a response. This causes bad things as eventually it makes its way to the top of the call chain, and if you return from your last handler in hono, the context itself is returned, which is difficult to understand what is going on.

Title: fix: correctly handle other errors from handlers

🔄 What Changed

  • Added explicit typing for response variable as Response | null
  • Changed error: any to error: unknown for better type safety
  • Added proper type checking for error objects before accessing properties
  • Added a safeguard that throws an error when no response is created

🔍 Impact of the Change

  • Prevents unhandled errors from causing unexpected behavior
  • Ensures that a proper response is always returned or an error is thrown
  • Improves type safety throughout the error handling flow
  • Provides clearer error messages when something goes wrong

📁 Total Files Changed

  • 1 file modified: src/handlers/handlerUtils.ts (12 additions, 3 deletions)

🧪 Test Added

N/A

🔒 Security Vulnerabilities

N/A

Quality Recommendations

  1. Consider adding unit tests to verify the error handling behavior

  2. Add more specific error messages that include details about what went wrong

  3. Consider implementing a more robust error handling strategy with custom error types

@narengogi narengogi requested a review from VisargD April 21, 2025 10:24
@@ -859,7 +859,7 @@ export async function tryTargetsRecursively(
currentJsonPath,
method
);
} catch (error: any) {
} catch (error: unknown) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've just gone through this code, and I think this catch block is meant to set a response object in all cases.
so the correct fix to make sure all cases are handled would be

if (typeof error === 'object' &&
      error &&
          'response' in error &&
          error.response instanceof Response
        ) {
          response = error.response;
        } else if (error instanceof GatewayError) {...} else {response = new Response(500,...)}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@VisargD to take a call here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants