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

Clean-up error handler & fix message for unknown API errors #22379

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

Clean-up error handler & fix message for unknown API errors #22379

wants to merge 12 commits into from
+328 −65

Conversation

paescuj
Copy link
Member

@paescuj paescuj commented May 3, 2024 •
edited

Scope

  • Fix broken logic to get error message of unknown errors (e.g. from a Flow) for admin users
    • Brackets were missing, which resulted in the error object itself being used as the message
    • Example Flow
      Before
      After
  • Clean-up of error handler
    • Wrap in async handler, similar to what we're using in other middleware/handlers - makes it easier to read and allows us to easily catch possible errors in the handler itself (unexpected, but would leak filesystem structure info if uncaught)
    • Add comments to reveal some of the magic happening here 🪄
    • Reusable fallback error
    • Prevent adding undefined extensions data / stack traces to error data (in dev mode)
    • Don't use toArray util to transform errors - would split string errors by commas
    • Better typing
  • Shared InternalServerError
  • Add tests

Potential Risks / Drawbacks

None

Review Notes / Questions

None

Fixes #22420, partially addresses #22416

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented May 3, 2024 •
edited

🦋 Changeset detected

Latest commit: d76629c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@directus/api Patch
@directus/errors Minor
directus Patch
@directus/extensions-registry Patch
@directus/memory Patch
@directus/validation Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

hanneskuettner
hanneskuettner reviewed May 3, 2024
View reviewed changes
Copy link
Contributor

@hanneskuettner hanneskuettner left a comment

Choose a reason for hiding this comment

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

One minor nitpick, otherwise it looks super clean

api/src/middleware/error-handler.ts Outdated Show resolved Hide resolved
@paescuj @hanneskuettner
Slightly more readable stack check
8df1a9f 
Co-authored-by: Hannes Küttner <4376726+hanneskuettner@users.noreply.github.com>
@paescuj paescuj force-pushed the enhance-api-error-handler branch from ffb7eea to 21c19cd Compare May 3, 2024 10:55
@paescuj paescuj requested a review from br41nslug May 3, 2024 14:56
This was referenced May 7, 2024
Open
Open
@alexchopin alexchopin added this to the Next Minor Release milestone May 17, 2024
br41nslug
br41nslug reviewed May 28, 2024
View reviewed changes
Copy link
Member

@br41nslug br41nslug left a comment •
edited

Choose a reason for hiding this comment

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

Looking good, some comments I feel are unnecessary or overkill but they're not hurting anyone so I dont think those matter and would just add noise in the comments 😄

api/src/middleware/error-handler.ts Outdated

function asyncErrorHandler(fn: ErrorRequestHandler) {
return (err: any, req: Request, res: Response, next: NextFunction) =>
Promise.resolve(fn(err, req, res, next)).catch((error) => {
Copy link
Member

Choose a reason for hiding this comment

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

Lets add a comment here to make it very clear that this Promise.resolve is here to support both regular and async functions as fn(). I personally think thats not directly clear from just the code/types and may be a footgun for future refactoring.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good thought! The function has initially been adapted from

directus/api/src/utils/async-handler.ts

Lines 3 to 4 in 7e6980d

const asyncHandler = (fn: RequestHandler) => (req: Request, res: Response, next: NextFunction) =>
Promise.resolve(fn(req, res, next)).catch(next);
but since we only use it here in this file we know that we receive an async function, I've now dropped the Promise.resolve wrapping.

api/src/middleware/error-handler.ts Outdated Show resolved Hide resolved
api/src/middleware/error-handler.ts Outdated Show resolved Hide resolved
paescuj and others added 4 commits May 29, 2024 12:35
@paescuj @br41nslug
Ensure error.code cannot be overwritten
f2d8038 
Co-authored-by: Brainslug <tim@brainslug.nl>
@paescuj
Catch case where headers have already been sent
e80671c
@paescuj
Enhance asyncErrorHandler signature & handler call
d07bed1 
- Omit Promise.resolve as we know we receive a promise in this case
- Use args from ErrorRequestHandler
@paescuj
Add tests
d76629c
@paescuj paescuj force-pushed the enhance-api-error-handler branch from f13e167 to d76629c Compare May 31, 2024 13:30
@paescuj paescuj changed the title Fix message for unknown API errors Clean-up error handler & fix message for unknown API errors May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@br41nslug br41nslug br41nslug left review comments

@hanneskuettner hanneskuettner Awaiting requested review from hanneskuettner

At least 1 approving review is required to merge this pull request.

Assignees

@hanneskuettner hanneskuettner

Labels
🍰 Bug 📁 REST
Projects
Software Roadmap
Status: 👀 In review
Milestone
Next Minor Release
Development

Successfully merging this pull request may close these issues.

Error message missing in the popup dialog
4 participants
@paescuj @hanneskuettner @br41nslug @alexchopin