feat: make error handling configurable #592

routmoute · 2021-12-30T12:03:02Z

add Simple ErrorService possibility

See Issue #591

crookse · 2021-12-31T00:25:20Z

hey @routmoute ! i like the idea behind this, but the current implementation is part of Drash's "core" code as opposed to being a separate service that can be plugged in through the services config. what i mean by that is:

This Pull Request

const server = new Server({
  protocol: "http",
  hostname: "localhost",
  port: 3000,
  resources: [],
  error_service: new MyErrorService() // <--- should be in services config
});

Expected Implementation

const server = new Drash.Server({
  protocol: "http",
  hostname: "localhost",
  port: 3000,
  resources: [],
  services: [ new MyErrorService() ] // <--- see here
});

reason we don't introduce much code into Drash's core is because we don't know for sure that developers want "built-in" features. the way Drash is currently built is so that developers are free to create their own services as well as a custom error service that meets their needs. however, we do feel that Drash should have an error handling layer that can be used easily, which is what you're introducing in this pull request. also, thank you for writing the documentation! much appreciated :).

so, all this being said, could you refactor your error service so that it can be plugged into the services config? if you run into any issues, feel free to message back and we'll respond asap. another option is we can merge your pull request in a feature branch, refactor some code, make a pull request to that feature branch, and have you review it. either option is fine with us, but we'd love your thoughts. let us know! thanks again!

routmoute · 2021-12-31T02:33:29Z

I think I see, don't create another class but add the runOnError function directly on the services. I will refactor that, it will be better, thank you.

Revert "ErrorService tests: add assert status" This reverts commit 3bc90f3. Revert "ErrorService: change function name" This reverts commit 21fdacb. Revert "add ErrorService tests" This reverts commit eebd9d6. Revert "add ErrorService feature" This reverts commit 58237b3.

…rors

crookse · 2021-12-31T20:57:40Z

hey @routmoute, so i had a talk with the core team and basically we think drash should have a built-in error handler and not an "error service". so your approach was mostly correct. sorry for misguiding you. the end goal would be a Drash.ErrorHandler class or something similar. that class would be instantiated and executed in server.ts' getHandler() method inside the catch block.

routmoute · 2022-01-01T17:38:57Z

Arf, but, I think the second approach is better because developers can work the errors differently if they are thrown on a particular method, path and then on the server.
On the first approach, we can only work at the server level.

crookse · 2022-01-01T18:14:00Z

true. maybe we stick with the error service for now and see how that goes and if it needs to be changed we can always do that later and make another release

routmoute · 2022-01-01T18:23:10Z

Actualy, I've just add a function runOnError() in Drash.Service class, this function is called in server.ts getHandler() catch block.

crookse · 2022-01-02T14:13:44Z

hey @routmoute , so i pulled down your branch locally and tested it -- keeping developer user experience and maintainability in mind -- and during my testing, i realized using an "error service" will get us back to what we had in v1 (and we don't want to go back to what we had in v1).

in v1, the error handling got really messy and was hard to maintain because we had to run middleware during the error handling process. every time we came back to the code, we forgot how it worked and there were edge cases with it that we kept finding. in your branch, you basically have what we had in v1 (https://github.com/routmoute/drash/blob/errorService/src/http/server.ts#L354-L385). your code isn't wrong. it's exactly what's needed to have an "error service" in drash, but it's also something that we removed when going from v1 to v2.

@Guergeiro suggested that there be an error handling layer in drash and that it be a built-in feature that can be extended by developers and i disagreed because i thought having an "error service" would be best suited for configurability. after testing your code locally, i feel that @Guergeiro is correct and we should not use an "error service," but instead have an error handling layer that can be extended and configured by developers (if they choose to do so).

crookse · 2022-01-02T14:24:58Z

feel free to hop in discord to discuss btw. we're pretty active in it and respond faster there. here's the link in case you're interested: https://discord.gg/DmjhWJt5

Revert "refactoring serviceError" This reverts commit 2ea504c. Revert "oops: clean" This reverts commit ab734e6. Revert "add basic ErrorsToJsonService with test" This reverts commit 16f603a. Revert "rename service_error to service_runOnError tests" This reverts commit fcd6765. Revert "add runBeforeResource() and runAfterResource() errors tests" This reverts commit 7a2fb22.

routmoute · 2022-01-02T17:02:21Z

I will retry something

routmoute · 2022-01-02T18:56:47Z

I've created an ExceptionLayer class can be exetended and declared on server 'exception' var.

what do you think about that ?

…esource for test

crookse · 2022-01-02T23:35:56Z

src/http/server.ts

+
+        if (exception) {
+          try {
+            await exception.catch(e, originalRequest, response);


this isn't documented in the codebase, but we try to instantiate things during compile time so that

the user doesn't have to do it

saves the server from having to build a new object for every request

could you make it so that the exception doesn't have to be instantiated by the user? that's the only feedback i have. id like to get thoughts on the naming of the class from @ebebbington and @Guergeiro

as far as implementation, LGTM. thanks @routmoute !

You mean the new Drash.ExceptionLayer().catch(...)?

yea, but we instantiate it once and reuse it just like we do with resources

I agree with it. Let's avoid generating garbage for the garbage collector to collect 👍🏿

ok, last commit instantiate ExceptionLayer on server.ts

src/http/server.ts

Guergeiro · 2022-01-02T23:52:50Z

src/interfaces.ts

@@ -141,6 +141,7 @@ export interface IServerOptions {
  protocol: "http" | "https";
  resources: typeof Resource[];
  services?: Service[];
+  exception?: ExceptionLayer;


This should be the IExceptionLayer. With that, I can create my own exception layer that does not extend the Drash layer, but rather completely overrides it.

A simple test with this would be a nice touch 😃

in last commit, I use typeof ExceptionLayer (like 'typeof Resource').

I added a test, I don't know if it's that you want.

@Guergeiro , i tested using IExceptionLayer as the expected type in the TS playground. seems to require new MyExceptionClass() to be used. see playground here.

Yes. It's supposed to. Here's a link with the same code, but the correct options signature.

ahh constructor. ok

… the user

…tends

ebebbington

Also in regards to #592 (comment), i think eric means, instantiate the exception class in the server constructor:

#exception: CustomException|null = null
constructor(...) {
  if (options.exception) {
    this.#exception = new options.exception()
  }
}
...

const exception = this.#exception
return function () {
  exception.catch(...)
}

But that wouldn't be a problem if the catch method was static anyways

src/http/server.ts

…ault error handler

…t and native Request

Guergeiro

In general, it's good for me. Logic wise.

This is some stuff I would prefer to simplify stuff. Basically, if no option for an ErrorHandler is passed, just automatically assign a default one. Then we can be assured it exists, otherwise it would fail in server setup.

Guergeiro · 2022-01-06T09:38:28Z

src/http/server.ts

+    if (options.error_handler) {
+      this.#error_handler = new options.error_handler();
+    }


No need for condition. Always apply the default if none exist.

this.#error_handler = new (options.error_handler() || Drash.ErrorHandler);

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgBIQDYYPbIN4BQyyAFpjgBQCUAXMgG7bAAmA3AQL4EEIZwDO-ZADFsuYAFsADhggSI4IeizZCxKQFcARhmAJS5bNXxFiyBNhD9ssgHQ4A5hQBEMMc6qmuXHn0HIAITgoZEkZOQUwJUMTdW1dfTIVYzUzCysbCHtsJ2ctYI8vTm4YDRAEMGBLZC0NYAxmaAoEAH46EAgAd2QKWz7gh346OBAATwBtAF0qZABeAD40QxnU9P4wBjnkDu7m5AAffcDgqnZielskylPiglr6xqhqdnuGptFsKiA

Guergeiro · 2022-01-06T09:38:50Z

src/http/server.ts

@@ -173,6 +182,9 @@ export class Server {
  #getHandler(): (r: Request, connInfo: ConnInfo) => Promise<Response> {
    const resources = this.#resources;
    const serverServices = this.#options.services ?? [];
+    const errorHandler = this.#error_handler;
+    const defaultErrorHandler = new Drash.ErrorHandler();


No need for this.

Guergeiro · 2022-01-06T09:45:01Z

src/http/server.ts

+        if (errorHandler) {
+          try {
+            await errorHandler.catch(e, originalRequest, response);
+          } catch (e) {
+            await defaultErrorHandler.catch(e, originalRequest, response);
+          }
+        } else {
+          await defaultErrorHandler.catch(e, originalRequest, response);
        }
-        return new Response(e.stack, {
-          status: e.code,
-          headers: response.headers,
-        });
+
+        return new Response(response.body, response);


try { // stuff } catch (e) { await this.#error_handler.catch(e, originalRequest, response); }

we use defaultErrorHandler when this.#error_handler.catch() or errorHandler.catch() (it's the same const errorHandler = this.#error_handler;) thrown an error

src/http/error_handler.ts

routmoute · 2022-01-06T16:57:02Z

tests/integration/error_handler_test.ts

+    }
+    response.json({ error: error.message });
+  }
+}


why error is Error here and is HttpError in default ErrorHandler ?
if is simple Error thrown, this ErrorHandler return 200 ?

I think use Error everywhere and set status to 500 if not an HttpError, see e805f2a and bcb8f18, it's better for me.

the answer to the second question is yes, it's bad. fixed on this commits

So there's two approaches here:

Catch on every error. This means the server will never stop working, regardless of the error. If an error is thrown just send 500.

Catch only HttpError. This can make the server crash if, for instance, a database errors and the developer doesn't worry about surrounding external calls with try...catch.

My opinion is that option 1 would be the most interesting. Ideally the server never goes down. If an error occurs that was not expected, and Internal Server Error Exception shall be sent to the user.

Same opinion, that's exactly what I did.

Guergeiro

I approve the architecture of this PR. Good job for me.

crookse · 2022-01-08T13:39:13Z

thank you @routmoute :)

routmoute added 3 commits December 30, 2021 12:54

feat: add ErrorService feature

8336d94

test: add ErrorService tests

3c9c113

feat(ErrorService): change function name

a35e1b0

routmoute mentioned this pull request Dec 30, 2021

add ErrorService tutorial drashland/website-v2#79

Merged

test(ErrorService): add assert status

9c9f060

routmoute added 2 commits December 31, 2021 06:56

test(ErrorService): add runBeforeResource() and runAfterResource() er…

9d0a5e6

…rors

routmoute added 3 commits January 1, 2022 20:40

test(ErrorService): rename service_error to service_runOnError

5225c5c

feat(ErrorService): add basic ErrorsToJsonService with test

a6c8b14

fix(ErrorsToJsonService): oops, clean

a892719

crookse linked an issue Jan 2, 2022 that may be closed by this pull request

ErrorService to handle HttpErrors / introduce custom error handling #591

Closed

3 tasks

crookse mentioned this pull request Jan 2, 2022

Throw JSON Response #594

Closed

routmoute added 3 commits January 2, 2022 19:41

refactor(ErrorService): rework ExceptionLayer

e980a25

fix(http/server): fix if no error code on second catch

bbfd2fa

test(integration/exception_test): fix bad implement

a9ab31d

test(integration/exception_test): rename testCases + delete useless r…

8c8493d

…esource for test

crookse reviewed Jan 2, 2022

View reviewed changes

Guergeiro requested changes Jan 2, 2022

View reviewed changes

fix(ExceptionLayer): the exception doesn't have to be instantiated by…

f8b69d0

… the user

test(integration/exception_test): own ExceptionLayer class without ex…

029e143

…tends

ebebbington reviewed Jan 3, 2022

View reviewed changes

src/http/server.ts Outdated Show resolved Hide resolved

routmoute added 2 commits January 3, 2022 19:28

fix(http/server): instantiate ExceptionLayer in constructor

bceaf7b

fix(http/server): add comment

95ff784

routmoute commented Jan 3, 2022

View reviewed changes

src/http/server.ts Outdated Show resolved Hide resolved

crookse mentioned this pull request Jan 4, 2022

e.code check in request handler doesn't account for non-HTTP error objects with code property #595

Closed

crookse changed the title ~~add ErrorService~~ feat: make error handling configurable Jan 4, 2022

crookse added the Type: Minor Merging this pull request results in a minor version increment label Jan 4, 2022

crookse and others added 5 commits January 5, 2022 07:53

refactor(http/error_handler): use term "Error" instead of "Exception"

f617412

refactor(http/error_handler): put setting of 500 status code into def…

05879ad

…ault error handler

fix(http/error_handler): clashing "Request" name between Drash.Reques…

8c41ac4

…t and native Request

test(http/error_handler): fix tests i broke

87cdabe

test(integration/error_handler): rename ExceptionLayer to ErrorHandler

b546e27

Guergeiro requested changes Jan 6, 2022

View reviewed changes

routmoute added 3 commits January 6, 2022 11:03

fix(http/server): this.#error_handler cannot be null

8dd0cbb

deno fmt

20effb0

test(integration/error_handler): add Async ErrorHandler test

4072ec7

routmoute commented Jan 6, 2022

View reviewed changes

routmoute added 2 commits January 6, 2022 18:36

fix(http/error_handler): return 500 if simple Error thrown

e805f2a

test(integration/error_handler): add simple Error thrown test

bcb8f18

Guergeiro approved these changes Jan 7, 2022

View reviewed changes

crookse approved these changes Jan 8, 2022

View reviewed changes

crookse merged commit fd1fffc into drashland:main Jan 8, 2022

routmoute deleted the errorService branch January 8, 2022 13:54

crookse mentioned this pull request Jan 9, 2022

feat: Drash Error Handling tutorials drashland/website-v2#83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make error handling configurable #592

feat: make error handling configurable #592

routmoute commented Dec 30, 2021

crookse commented Dec 31, 2021

routmoute commented Dec 31, 2021

crookse commented Dec 31, 2021

routmoute commented Jan 1, 2022 •

edited

Loading

crookse commented Jan 1, 2022

routmoute commented Jan 1, 2022 •

edited

Loading

crookse commented Jan 2, 2022

crookse commented Jan 2, 2022

routmoute commented Jan 2, 2022

routmoute commented Jan 2, 2022

crookse Jan 2, 2022

Guergeiro Jan 2, 2022

crookse Jan 2, 2022

Guergeiro Jan 2, 2022

routmoute Jan 3, 2022

Guergeiro Jan 2, 2022

routmoute Jan 3, 2022

crookse Jan 4, 2022

Guergeiro Jan 4, 2022

crookse Jan 5, 2022

ebebbington left a comment

Guergeiro left a comment

Guergeiro Jan 6, 2022

Guergeiro Jan 6, 2022

Guergeiro Jan 6, 2022

Guergeiro Jan 6, 2022

routmoute Jan 6, 2022

routmoute Jan 6, 2022

routmoute Jan 6, 2022 •

edited

Loading

Guergeiro Jan 7, 2022

routmoute Jan 7, 2022

Guergeiro left a comment

crookse commented Jan 8, 2022

feat: make error handling configurable #592

feat: make error handling configurable #592

Conversation

routmoute commented Dec 30, 2021

crookse commented Dec 31, 2021

This Pull Request

Expected Implementation

routmoute commented Dec 31, 2021

crookse commented Dec 31, 2021

routmoute commented Jan 1, 2022 • edited Loading

crookse commented Jan 1, 2022

routmoute commented Jan 1, 2022 • edited Loading

crookse commented Jan 2, 2022

crookse commented Jan 2, 2022

routmoute commented Jan 2, 2022

routmoute commented Jan 2, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebebbington left a comment

Choose a reason for hiding this comment

Guergeiro 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

routmoute Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guergeiro left a comment

Choose a reason for hiding this comment

crookse commented Jan 8, 2022

routmoute commented Jan 1, 2022 •

edited

Loading

routmoute commented Jan 1, 2022 •

edited

Loading

routmoute Jan 6, 2022 •

edited

Loading