Skip to content
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

Jump to bottom

Error handler does not support InvalidInput, InvalidActor, or InvalidState errors #9405

Open
timolegros opened this issue Oct 2, 2024 · 3 comments · May be fixed by #9418
Open

Error handler does not support InvalidInput, InvalidActor, or InvalidState errors #9405

timolegros opened this issue Oct 2, 2024 · 3 comments · May be fixed by #9418
Assignees
@Rotorsoft
Labels
1 1-2 hour task bug Something isn't working

Comments

@timolegros
Copy link
Collaborator

Description

In the commands/query framework we throw 3 types of errors:

  • InvalidInput: equivalent to AppError
  • InvalidState: equivalent to ServerError
  • InvalidActor: authentication error (no explicit equivalent just errors thrown by passport)

These errors are not properly supported by setupErrorHandlers.ts. For example, if you set up a command that throws an InvalidInput error and integrate it using the express adapter, the error will be returned as an HTTP 500 error. We need to map the above errors to relevant error codes in setupErrorHandlers.ts

Additional context
@timolegros timolegros added bug Something isn't working needs estimate 1 1-2 hour task and removed needs estimate labels Oct 2, 2024
@Rotorsoft
Copy link
Contributor

We have error middleware in /libs/adapters/express/middleware.ts, which can be added to the integration routers using the express adapter.... or do we need to merge this with the legacy error handlers?

@timolegros
Copy link
Collaborator Author

timolegros commented Oct 3, 2024 •
edited
Loading

We have error middleware in /libs/adapters/express/middleware.ts, which can be added to the integration routers using the express adapter.... or do we need to merge this with the legacy error handlers?

I hadn't seen that middleware. The middleware that currently applies is setupErrorHandlers because it handles errors globally across all routes on the express app. Is there a reason this error handling middleware is not applied to any of the new express routers?

Also it does seem that we need to modify it because logging is incorrect in that middleware. It is using console.log when it should be using log.error for all 500 errors.

@Rotorsoft
Copy link
Contributor

yep... it was never wired to any router. I can take this ticket and basically merge it to the legacy setupErrorHandlers.

@Rotorsoft Rotorsoft linked a pull request Oct 3, 2024 that will close this issue
Open
@timolegros timolegros removed their assignment Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Rotorsoft Rotorsoft

Labels
1 1-2 hour task bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Adds new errors to global express error handler hicommonwealth/commonwealth
2 participants
@Rotorsoft @timolegros