2

I'm trying to improve my MVC.NET Core-fu and I ended up in a bunch of methods in all my controllers looking like this (note the outermost, general, repetitive try-catch).

[HttpPost]
public IActionResult DoSomething([FromBody] Thing thing)
{
  try
  {
    if (...)
      return Ok(thing);
    else
      return NotFound();
  }
  catch (Exception) { return BadRequest(); }
}

The way I see, I want to have the baddy-requesty just in case. Now, that adds a lot of code to otherwise rather simple controller and I have a suspicion that someone before has thought of this and proposed a solution.

I haven't found such thing (and I was informed that filters are obsolete for usage in Core). Possibly it's because I'm not familiar with the right search keys.

DonkeyBanana
  • 3,266
  • 4
  • 26
  • 65
  • You could define a method like `void DoIt(Action<...> callBack)`. Inside `DoIt` you define the exception handling once before calling the delegate/action that is your body per request –  Jan 10 '18 at 02:03
  • Take a quick read at a similar question I answered here https://stackoverflow.com/questions/38025305/best-practice-for-error-handling-with-asp-net-web-api – Nkosi Jan 10 '18 at 02:26
  • This should also be of some interest to you. https://stackoverflow.com/questions/44791206/exception-handling-middleware-and-page – Nkosi Jan 10 '18 at 02:29
  • 3
    Please read the [documentation on error handling](https://learn.microsoft.com/en-us/aspnet/core/fundamentals/error-handling). You shouldn’t catch all exceptions in your actions, that’s what the global error handling is for. Also, the way you do it completely swallows any useful information about the error. Also also, 400 Bad Request is likely a wrong error code for the errors that you encounter. If the server does a mistake and an exception is thrown, then that’s likely a 500 Internal Server Error – which also happens to be the default. – poke Jan 10 '18 at 08:28
  • @poke I agree with your worries but you need worry not - I only wanted to elaborate on my question. The actual handling has logs written to and multiple types of exceptions being caught. And it's mainly the 5xx type we want to hide from the receivers (while investigating them under the back-end hood. The aim of the question is not *proper exception handling* but *implicit jacking in exception handling*. – DonkeyBanana Jan 10 '18 at 15:20
  • @poke In the examples that you link to, the management of exceptions is divided into *developer oriented* and *consumer oriented*, which is great. However, both are simply redirecting to an error page containing (different depth of) information regarding the error. I would like the error cause the application to return something of type *IActionResult*, e.g. *StatusCode(666)* or whatever value I may find confusingly obfuscating. – DonkeyBanana Jan 10 '18 at 15:29
  • You can easily run your own code to handle the exceptions and make the response return whatever you want. Check out [this sample code](https://github.com/aspnet/Diagnostics/blob/rel/2.0.0/samples/ExceptionHandlerSample/Startup.cs). – poke Jan 10 '18 at 15:40
  • @Nkosi I've read through the answers and the linked articles. The way I understand it, those are speaking of filters and/or approach for MVC 6, not Core. I'm looking for something that removes the need for *try-catch* in each method but also doesn't impose me to use filters (as they're apparently not recommended in Core). I've heard that I might want to alter *Configure(...)* or *ConfigureServices(...)* in *Startup* but I haven't figure out how and I haven't been successful finding examples (probably due to my ignorance on the subject or possibly being stupid). Suggestions? – DonkeyBanana Jan 10 '18 at 15:42
  • @DonkeyBanana The approach is transferable. The first link is for core and I am aware the others are for a previous version of MVC. Error handling is cross cutting concern that should be moved into middleware and executed early in the pipeline. – Nkosi Jan 10 '18 at 15:44
  • @Nkosi I get that part. It's the *middleware* concept that confuses me - are we talking about 3rd party product like Nlog etc.? I'd like to see an actual example on how it tranfers to Core on not building a filter but jacking in a service or whatever to pokemon the errors (catch'em all). The first link seems not to be about Core, though. – DonkeyBanana Jan 10 '18 at 15:47
  • @DonkeyBanana then I would suggest research how to properly use middleware and also check to make sure you are not re creating features that you can get out of the box with core or even with 3rd party libraries. I stand corrected it was the second link. – Nkosi Jan 10 '18 at 15:50
  • 1
    Note that filters are *not* discouraged in ASP.NET Core. But they are part of the MVC pipeline, making them specific to MVC. Exception handlers should be globally around in the request pipeline, so they should be implemented as middleware. – As for middleware, make sure you read the [fundamentals on middlewares](https://learn.microsoft.com/en-us/aspnet/core/fundamentals/middleware). – poke Jan 10 '18 at 15:50
  • @poke Hey, mate - you've got me reading for a while now. Awesome link and, embarrassingly, rather new to me. I was ignorant of some fundamental concepts of Core pipeline, which made me ask a rather ill aimed question. It's starting to brighten up, though. So, I'm starting with the dummy middy *app.Use((ctx,nxt)=>nxt());* doing nothing. Then I need to *try-catch* it and figure out what to return if an exception occurs. Apparently, I can't just return an instance of type *IActionResult* (like e.g. *StatusCode(500)* but rather of type *Task*. Are those related? – DonkeyBanana Jan 10 '18 at 16:36
  • @poke I think I'm the point now where I can start progressing by the magic of googling again, so if you feel I'm closing in feel free to tell me to buzz off. Unless you have some useful pointer, in which case, fire away. I need to taskify and action result into a response field in the context, right? – DonkeyBanana Jan 10 '18 at 16:38
  • `IActionResult` is actually an concept of MVC which is processed later in the MVC pipeline to construct the actual HTTP response. When writing middleware, you don’t use these action results (since you are not inside of an MVC *action*) but you directly modify the response. For example, when you return `StatusCode(500)`, you are returning a `StatusCodeResult` which will [eventually just set the `StatusCode` property on the response object](https://github.com/aspnet/Mvc/blob/rel/2.0.0/src/Microsoft.AspNetCore.Mvc.Core/StatusCodeResult.cs#L33). So that’s what you need to do in your middleware. – poke Jan 10 '18 at 17:17
  • @poke All right. I'll figure it out somehow, eventually. It's been an unexpectedly deep but oh-so-awesome learning opportunity! Feel urged to post a short reply so I can accept the answer. Also, I've got a more specific question on middleware [here](https://stackoverflow.com/questions/48193220/cant-implement-invokable-catch-of-try-catch-in-middleware-in-core-pipeline) if you feel like polishing your karma. If not, I'm still very thankful and appreciating. – DonkeyBanana Jan 10 '18 at 17:51

1 Answers1

4

This seems excessive to me. Many methods won't execute instructions that are likely to fail. For those extremely rare cases, a global error handler would be more than sufficient.

For those methods that access the disk, a database, or perform calculations that could raise exceptions (something you should probably be avoiding in the first place), a try...catch block makes sense.

But even in these cases, if a method with a try...catch handler calls another method, there is no reason to put handlers in that other method.

Jonathan Wood
  • 65,341
  • 71
  • 269
  • 466
  • Thanks! I got suggestion from @poke to erad up on the middleware and it turned out that it was my ignorance on how to configure pipeline in AspCore that was the base for the confusion. I got much better understanding for the subject now (after having spent the whole day fighting with it) so it's great. Now I have an actual issue with the pipeline (linked to it from my comment). Rather quiet there so I fear that I might have missed something still, though... – DonkeyBanana Jan 10 '18 at 19:14