mrc-6085 Error handling #3

EmmaLRussell · 2024-12-09T16:44:10Z

This branch implements an error handling framework. This is based on the framework implemented in WODIN which seems to work quite well. The idea is that any requests which cannot be completed successfully should cause an Error to be thrown. This Error will be dealt with by the handleError method which will ensure that the appropriate JSON error response and status code are returned.

Key changes:

GroutError class. This is for errors which we throw deliberately e.g. in the case of a bad request, or unknown resource requests. Parameters to this class include message, ErrorType and status code. These parameters are used by handleError to build the error Response.
handleError - given a thrown error, builds a "Porcelain-style" response with error, status code and error type. If the error is an instance of GroutError its message is returned in the response. If not, then this is an unexpected error, and we don't want to return its raw error message to the client. so we include a generic message, along with a unique error code. handleError includes message (including error code), type and stack in tokens to be logged by morgan. This means that we can find the stack corresponding to an unexpected error code in the logs if required.
asyncControllerHandler - this allows handleError to be triggered when the error is thrown asynchronously to the initial request call. It does this by catching any error, and ensuring that next is called with the error as parameter. This handler should be used by any controller method which is async.
notFound - utility function for throwing a GroutError for any 404 scenario. Used by both the router and the TileController (for the case where the tile request was for unknown data).
jsonResponse utility for returning JSON error and success responses in our standard Porcelain format. Update to the index response to use this.

In order to test behaviour when a 500 error is thrown, I've also added an "/error-test" endpoint to routes which is only added if GROUT_ERROR_TEST environment variable is set. This means that we can throw an "expected unexpected" error for testing without needing to include it in the production server. The ./docker/run script sets this env var.

You should see that if you run the app using ./docker/run then browsing to http://localhost:5000/error-test will show a 500 "UNEXPECTED_ERROR" response with an error code, and this should also show up in the logs of the grout docker container. The raw error message "Testing error behaviour" should be logged in the container, but not returned in the response.

However, if you run the app outside docker with npm run dev, the env var will not be set, the route should not be added, and browsing to http://localhost:5000/error-test should return a 404 rather than a 500.

EmmaLRussell · 2024-12-17T09:51:46Z

test.html

@@ -79,7 +79,7 @@ <h1>GROUT</h1>
                style: {
                    weight: 1,
                    fill: false,
-                    color: "#000000"
+                    color: "#777"


Tone down garish country outlines on test page!

M-Kusumgar

this is awesome! love the high res data! i will say i think the outlines are too thick at zoomed out levels XD and tooltips stay on when i move my mouse to water, but obviously in development, excited to use this at some point!

I have a couple typescript suggestions but other than that have checked out the endpoints, works well

M-Kusumgar · 2024-12-18T12:08:27Z

src/errors/groutError.ts

+    status: number;
+    errorType: ErrorType;
+
+    constructor(message: string, status: number, errorType: ErrorType) {


do we expect the error types enum to not have a 1 to 1 mapping with the status number? because if so we can just derive the status from the error type enum with a mapping or just changing the enum to reflect that feels easy to remove extra arg

I think at the moment that's true, we're kind of hedging our bets here. Yeah, I'll take it out, we can add it back if we need it.

M-Kusumgar · 2024-12-18T12:25:57Z

src/logging.ts

+interface RequestWithError {
+    errorType: string;
+    errorDetail: string;
+    errorStack: string | undefined;
+}
+export const reqWithError = (req: Request) =>
+    req as unknown as RequestWithError;


hmm i think this is quite weird for 2 reasons:

req is not actually just errorType, errorDetail, errorStack, its much more

using a js function to only satisfy typescript seems weird too, we would have to wrap req with this function every time we want these properties and have it unwrapped in the same block of code where we want to use other properties in Request type

you can just define request with error type as how it is

export type RequestWithError = Request & { errorType?: string; errorDetail?: string; errorStack?: string; };

where Request is the type express exports. Then wherever you use just the request type in function args i.e. arg handleError just replace req: Request with req: RequestWithError, and same thing in all the moken.token callbacks below

that should solve it without you having to wrap req everytime and it will be typed correctly

You're completely right, I'd just blindly copied this from somewhere else (wodin?) - no idea why it had ended up that way there, maybe some no longer relevant history. Yeah, will tidy that up.

M-Kusumgar · 2024-12-18T12:28:01Z

src/logging.ts

+            tokens["error-type"](req, res),
+            tokens["error-detail"](req, res),
+            tokens["error-stack"](req, res)


none of these actually take res, minor point obviously but could just make res optional arg in the type and remove res from here although no harm in leaving this in either

M-Kusumgar · 2024-12-18T12:28:34Z

src/routes.ts

+    // provide an endpoint we can use to test 500 response behaviour by throwing an "unexpected error" - but only if we
+    // are running in a non-production mode indicated by an env var
+    if (process.env.GROUT_ERROR_TEST) {
+        router.get("/error-test", () => {
+            throw Error("Testing error behaviour");
+        });
+    }


nice this is a neat idea, we should definitely use it more!

M-Kusumgar · 2024-12-18T13:34:00Z

Also side note, i could not get the docker/run script to run, i think it is because the wrong tag is pushed on github actions, i believe you cant just do git rev-parse to get the sha on github actions because it does a new merge commit or something like that, you can bake it into github actions, i did this for hint here https://github.com/hivtools/hint/blob/73f5992bdc9d0dee0801a0764f930b8737819255/.github/actions/make-shared-env/action.yml#L39

alternatively to get it working for now i just changed $TAG_SHA to $TAG_BRANCH in docker/run because it was pushed with the correct branchname

EmmaLRussell · 2024-12-18T14:29:27Z

this is awesome! love the high res data! i will say i think the outlines are too thick at zoomed out levels XD and tooltips stay on when i move my mouse to water, but obviously in development, excited to use this at some point!

It is just a test page, guys...

I'll maybe spruce it up in a separate ticket, make the outlines thinner when zoomed out and tidy up the tooltips...

EmmaLRussell · 2024-12-18T14:37:53Z

Also side note, i could not get the docker/run script to run, i think it is because the wrong tag is pushed on github actions, i believe you cant just do git rev-parse to get the sha on github actions because it does a new merge commit or something like that, you can bake it into github actions, i did this for hint here https://github.com/hivtools/hint/blob/73f5992bdc9d0dee0801a0764f930b8737819255/.github/actions/make-shared-env/action.yml#L39

alternatively to get it working for now i just changed $TAG_SHA to $TAG_BRANCH in docker/run because it was pushed with the correct branchname

oops! Good catch. We definitely want to use the sha though, as ./docker/run is used for the integration tests in CI before it pushes the branch tag. Will use your trick.

david-mears-2 · 2024-12-18T15:56:16Z

It is just a test page, guys...

For this reason, I would put some notice on the page saying 'test page' to head off further UI comments

EmmaLRussell · 2024-12-18T16:05:14Z

It is just a test page, guys...

For this reason, I would put some notice on the page saying 'test page' to head off further UI comments

<h1>Deliberately Ugly Test Page</h1>
<p>If you can see the ugly, it means it's working</p>

david-mears-2 · 2024-12-18T15:53:10Z

docker/run

@@ -7,5 +7,6 @@ docker run -d --rm \
    --pull=always \
    -p 5000:5000 \
    -v ./data:/data \
+    -e GROUT_ERROR_TEST=true \


I would have expected a docker/run script to be used for a deployment. Maybe the env var check in src/routes.ts should also check what environment we're in directly.

No, this is just a dev/CI script, we won't be using it for deployment, that'll be a separate tool. I think it's easier to be explicit when we want the testing route to be added (really just when using this script for integration testing locally/on CI).

david-mears-2 · 2024-12-18T16:00:25Z

src/controllers/tileController.ts

+            ErrorType.BAD_REQUEST
+        );
+    }
+    return parseInt(param, 10);


Why not use quinary? Decimal is so 2024.

Banking on retro vibes being in next season.

david-mears-2 · 2024-12-18T16:02:58Z

src/errors/asyncControllerHandler.ts

+    try {
+        await method();
+    } catch (error) {
+        next(error);


What do we use next for in this app, if anything? As far as I can tell from reading about it, it's for falling back to other routes when the route has an error?

It hands on to the next middleware which can handle the error. We need it for handleError to be triggered.

david-mears-2 · 2024-12-18T16:07:29Z

src/errors/groutError.ts

+    status: number;
+    errorType: ErrorType;


Do we need to store both of these? As Mantra said they have a 1-1 mapping?

Or is it that the status can be any valid HTTP status, not just a defined error type status?

Maybe e.g. status should be a getter for the error type, or vice versa. (rather than a property assigned by the constructor)

I do think they're a bit different conceptually, and I think it's ok for GroutError to manage that. There may come a time when it's not a 1-1 mapping, though it is at the moment. OK, will change to getter as that will cover both cases.

M-Kusumgar

nice, have a small optional comment that might make it a bit simpler but otherwise happy to approve!

M-Kusumgar · 2024-12-19T10:44:07Z

src/controllers/tileController.ts

+const parseIntParam = (param: string): number => {
+    // Native parseInt is not strict (ignores whitespace and trailing chars) so test with a regex
+    if (!/^\d+$/.test(param)) {
+        throw new GroutError(
+            `"${param}" is not an integer`,
+            ErrorType.BAD_REQUEST
+        );
+    }
+    return parseInt(param, 10);


dont have to change this but you could use Number(string) which does everything you want without you having to write regex, Number evals the whole string vs parseInt stopping

I don't think Number(string) will help me exclude non-integer numbers though, will it?

What would parseInt(Number(string)) do?

you can do Number.isInteger(Number(string)) for that, so something like

const num = Number(string); if (!Number.IsInteger) { throw... } return num

guess its about the same XD up to you

OK, I might just leave it as it is. Nice idea though, thanks!

codecov · 2024-12-19T12:01:37Z

Codecov Report

Attention: Patch coverage is 97.01493% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.27%. Comparing base (77cac05) to head (bc88234).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
src/routes.ts	50.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##              main       #3      +/-   ##
===========================================
- Coverage   100.00%   98.27%   -1.73%     
===========================================
  Files            7       13       +6     
  Lines           61      116      +55     
  Branches         7       15       +8     
===========================================
+ Hits            61      114      +53     
- Misses           0        2       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

EmmaLRussell · 2024-12-19T16:18:41Z

Disregarding the codecov grumble!

first stab at error handler

0d14148

EmmaLRussell changed the base branch from main to mrc-6064-serve-tile-data December 9, 2024 16:49

EmmaLRussell added 10 commits December 9, 2024 17:13

use async controller handler

4d98f83

comments

dd25f57

return json for all 404s

5894fb9

return 400 on bad z, x or y

3bcbe9a

use jsonResponseSuccess for index response

0310015

unit tests for controller and controller handler

381611c

lint

dfdf1ec

more tests

f6a83f7

added error testing endpoint for running in testing modes

2d5c3c0

error endpoint test, lint, grey country border for test page

ab485a9

EmmaLRussell commented Dec 17, 2024

View reviewed changes

merge serve tile changes

507c75d

EmmaLRussell marked this pull request as ready for review December 17, 2024 14:56

EmmaLRussell requested review from david-mears-2 and M-Kusumgar December 17, 2024 14:56

M-Kusumgar reviewed Dec 18, 2024

View reviewed changes

EmmaLRussell added 2 commits December 18, 2024 15:36

Remove status param from GroutError constructor

9224ee0

fix requestWithError mess

bbb2e95

try git_sha

326efdc

david-mears-2 reviewed Dec 18, 2024

View reviewed changes

EmmaLRussell added 3 commits December 18, 2024 16:24

fix script

4ad0ca4

set pr sh from context

cecd991

make GroutError status a getter

e278c21

EmmaLRussell added 3 commits December 18, 2024 17:07

try to make test page not look like it's trying to be a thing

aabe83c

use RequestWithError as handleError param type

3ea2a0e

lint

fef5ebb

EmmaLRussell requested review from M-Kusumgar and david-mears-2 December 18, 2024 17:21

david-mears-2 approved these changes Dec 18, 2024

View reviewed changes

Merge branch 'main' into mrc-6085-error-handling

e8491fd

EmmaLRussell changed the base branch from mrc-6064-serve-tile-data to main December 19, 2024 10:05

M-Kusumgar approved these changes Dec 19, 2024

View reviewed changes

merge with main

bc88234

EmmaLRussell merged commit b4c775a into main Dec 19, 2024
2 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mrc-6085 Error handling #3

mrc-6085 Error handling #3

EmmaLRussell commented Dec 9, 2024 •

edited

Loading

EmmaLRussell Dec 17, 2024

M-Kusumgar left a comment

M-Kusumgar Dec 18, 2024

EmmaLRussell Dec 18, 2024

M-Kusumgar Dec 18, 2024

EmmaLRussell Dec 18, 2024

M-Kusumgar Dec 18, 2024

M-Kusumgar Dec 18, 2024

M-Kusumgar commented Dec 18, 2024 •

edited

Loading

EmmaLRussell commented Dec 18, 2024

EmmaLRussell commented Dec 18, 2024

david-mears-2 commented Dec 18, 2024

EmmaLRussell commented Dec 18, 2024

david-mears-2 Dec 18, 2024

EmmaLRussell Dec 18, 2024 •

edited

Loading

david-mears-2 Dec 18, 2024

EmmaLRussell Dec 18, 2024

david-mears-2 Dec 18, 2024

EmmaLRussell Dec 18, 2024

david-mears-2 Dec 18, 2024

david-mears-2 Dec 18, 2024

EmmaLRussell Dec 18, 2024

M-Kusumgar left a comment

M-Kusumgar Dec 19, 2024

EmmaLRussell Dec 19, 2024

david-mears-2 Dec 19, 2024

M-Kusumgar Dec 19, 2024

EmmaLRussell Dec 19, 2024

codecov bot commented Dec 19, 2024 •

edited

Loading

EmmaLRussell commented Dec 19, 2024

mrc-6085 Error handling #3

mrc-6085 Error handling #3

Conversation

EmmaLRussell commented Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

M-Kusumgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Kusumgar commented Dec 18, 2024 • edited Loading

EmmaLRussell commented Dec 18, 2024

EmmaLRussell commented Dec 18, 2024

david-mears-2 commented Dec 18, 2024

EmmaLRussell commented Dec 18, 2024

Choose a reason for hiding this comment

EmmaLRussell Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Kusumgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

EmmaLRussell commented Dec 19, 2024

EmmaLRussell commented Dec 9, 2024 •

edited

Loading

M-Kusumgar commented Dec 18, 2024 •

edited

Loading

EmmaLRussell Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading