Skip to content

Move error handling to middleware #143

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

Merged
merged 18 commits into from
Jul 6, 2020
Merged

Move error handling to middleware #143

merged 18 commits into from
Jul 6, 2020

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Jul 2, 2020

EDIT: Removed the sanitize numbers middleware. No monkey patching express functions for now.

Closes #138

This PR is a combination of two related features that both affect the implementation of get and post. This is in preparation for merging #134 as that PR does not use a generic wrapper around express app.get and app.post.'

Motivation

Middleware functions are modular, can be put on routes/routers with fine grain control, and are individually tested-able.

  • With our current setup of error handling we must have duplicate code across get and post.

  • The handling of different errors is intertwined and number sanitization is all done in one function, making good unit testing difficult.

  • When adding new types of error formats or phasing out old error formats there is a high cognitive over head with the current setup. Using middleware specific to an error formats will allow us to easily add or remove error formats from our codebase

  • For number sanitization, having the functionality at the middleware level allows us to move it from the application level to the router level if we choose. The current layout I propose is at the application level as that mimics our current API

What to look for while reviewing

  • Make sure there are no hanging responses. Express does not natively catch errors thrown by async functions, so it is up to the try/catch wrappers in get and post to catch them.
  • All error responses are the same as before. I tried to ensure that we handle all error formats the same way, but good to double check
  • The modification of res.send is reasonable. Implementing this was tricking because
    1. I needed to ensure the correct binding of res to the original send and
    2. I had to do some generic typing that got a bit finicky since some of the types from @types/express had several any defaults and sanitizeNumbers is all anys

@emostov emostov requested review from danforbes and joepetrowski July 2, 2020 22:21
Remove unused import

Remove unused log
@emostov emostov force-pushed the zeke-ware-err-sanitize branch from 62b13aa to ec44d28 Compare July 2, 2020 22:49
@emostov emostov changed the title Move error handling and number sanitization to middleware Move error handling to middleware Jul 6, 2020
@emostov emostov merged commit edc93dc into master Jul 6, 2020
@emostov emostov deleted the zeke-ware-err-sanitize branch July 6, 2020 15:26
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.

Use middleware to catch errors and sanitize response
3 participants