[FIX] Passing HttpException from Exception handler

[geoperez]

From SO question at https://stackoverflow.com/questions/58718698/how-to-setup-an-exception-handler-in-embedio-web-api

[codecov[bot]]

Codecov Report

Merging #427 into master will increase coverage by 0.07%. The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #427      +/-   ##
==========================================
+ Coverage   60.02%   60.09%   +0.07%     
==========================================
  Files         168      168              
  Lines        6852     6857       +5     
  Branches     1233     1236       +3     
==========================================
+ Hits         4113     4121       +8     
+ Misses       2393     2390       -3     
  Partials      346      346

Impacted Files	Coverage Δ
src/EmbedIO/WebModuleBase.cs	72.72% <100%> (+6.81%)	:arrow_up:
src/EmbedIO/WebServerBase`1.cs | 80% <100%> (ø)	:arrow_up:
src/EmbedIO/ExceptionHandler.cs	84.9% <71.42%> (-0.52%)	:arrow_down:
src/EmbedIO/HttpException-Shortcuts.cs	46.15% <0%> (+7.69%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a77ebaa...2bceae5. Read the comment docs.

[geoperez]

@rdeado, I saw your response in SO, did you test that code? Because I first tried throwing HttpException from HandleUnhandledException callback, but this always returns 404.

[geoperez]

One issue is that you can only set a proper handler for the HttpException in the module's parent. Like this:

            var server = new WebServer(o => o
                    .WithUrlPrefix(url)
                    .WithMode(HttpListenerMode.EmbedIO)).WithWebApi("/api", o =>
            {
                o.WithController<TestController>();

                o.OnUnhandledException = (ctx, ex) => throw HttpException.BadRequest();
            });

            server.OnHttpException = async (ctx, ex) => await ctx.SendDataAsync(ex);

The following code, the proper way, is not working:

            var server = new WebServer(o => o
                    .WithUrlPrefix(url)
                    .WithMode(HttpListenerMode.EmbedIO)).WithWebApi("/api", o =>
            {
                o.WithController<TestController>();

                o.OnHttpException = async (ctx, ex) => await ctx.SendDataAsync(ex);
                o.OnUnhandledException = (ctx, ex) => throw HttpException.BadRequest();
            });

[rdeago]

I have to admit that the code I posted on SO was only submitted to the looks-legit-may even-compile™️ test. Anyway, the behavior you describe looks more like a bug to me than a need for an additional catch clause.

[geoperez]

A bug because it is not passing to the OnHttpException handler?

[geoperez]

@rdeago I changed the signature of ExceptionHandler, what do you think?

[rdeago]

About being only able to handle the resulting HTTP exception at an outer level: you are right, but think about it: next time someone wants to throw an exception from an HTTP exception handler, what do we do inside the catch clause you just added? Catch exceptions and pass them to the exception handler, which in turn can throw another HTTP exception?

Either we loop, handling exceptions and catching exceptions thrown by handlers until no further exception is thrown, or we handle exceptions thrown by handlers at an outer level; after all, structured exception handling got its name for a reason.

Should the need arise, in some particular setup, to throw HTTP exceptions from an exception handler and have them handled differently from HTTP exceptions generated in other modules, just wrap the module in a ModuleGroup, thus introducing a further handling layer.

[geoperez]

I understand what are you trying to explain here. I know a HttpExceptionHandler can throw an unexpected exception, but in this scenario, you are in control of the handler.

In the user story of this PR, you might not have control of what are you connecting to your WebAPI, and it's really hard to translate from a common exception to one using IHttpException. The only solution it's having a try/catch in all your endpoints. Kinda messy.

I may recommend keep this patch, and open the discussion if we need a more complex exception handler.

[rdeago]

My browser keeps crashing. I'm moving to Slack to try and continue the conversation.

Can you defer merging for now, so we can see if there's a cleaner way to make everyone happy?