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

Fix message for unknown API errors #22379

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

Fix message for unknown API errors #22379

wants to merge 8 commits into from
+93 −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

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: 0e5a045

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

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.

api/src/middleware/error-handler.ts
Comment on lines 49 to 52
extensions: {
// Expose error code under error's extensions data
code: error.code,
...(error.extensions ?? {}),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extensions: {
// Expose error code under error's extensions data
code: error.code,
...(error.extensions ?? {}),
extensions: {
...(error.extensions ?? {}),
code: error.code,

Inverted the order to ensure the error.extensions spreading cannot overwrite the set code.

api/src/middleware/error-handler.ts
});

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

Choose a reason for hiding this comment

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

Suggested change
return (err: any, req: Request, res: Response, next: NextFunction) =>
// Note: keep all 4 parameters here. That's how Express recognizes it's the error handler, even if
// we don't use next
return (err: any, req: Request, res: Response, next: NextFunction) =>

Lets not forget adding back the original comment here

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