Should catching exceptions be part of the business logic such as the Service layer, or should they be caught in the controllers' methods?

For example:

Controller UpdateUser method

    [HttpPut]
    [Route("{id}")]
    [ProducesResponseType(200)]
    [ProducesResponseType(404)]
    public async Task<ActionResult<UserDto>> UpdateUserInfo(int id, UserDto userRequest)
    {
        try
        {
            var user = _userMapper.ConvertToEntity(userRequest);
            var updatedUser = await _userService.UpdateAsync(user, id);
            var result = _userMapper.ConvertToUserDto(updatedUser);

            return Ok(result);
        }
        catch (Exception ex)
        {
            _logger.LogError("Exception caught attempting to update user - Type: {ex}", ex.GetType());
            _logger.LogError("Message: {ex}", ex.Message);
            return StatusCode(500, ex.Message);
        }
    }

The Service Layer

    public async Task<User> UpdateAsync(User user, int id)
    {
        await _repository.UpdateAsync(user, id);
        return user;
    }

So, should the exceptions be caught in the service layer or the controller? Or is it subjective?

CodePudding user response：

It's dependent on the business of your application. maybe in your service you should use a try/catch block to adding a log or do anything when exception occurred. but usually I use a global try/catch in a middleware to get exception and send correct response to the client.

public class AdvancedExceptionHandler
{
    private readonly RequestDelegate _next;
    private readonly ILogger<AdvancedExceptionHandler> _logger;
    private readonly IWebHostEnvironment _env;

    public AdvancedExceptionHandler(RequestDelegate next, ILogger<AdvancedExceptionHandler> logger, IWebHostEnvironment env)
    {
        _next = next;
        _logger = logger;
        _env = env;
    }
    public async Task Invoke(HttpContext context)
    {
        string message = null;
        HttpStatusCode httpStatusCode = HttpStatusCode.InternalServerError;
        try
        {
            await _next(context);
        }
        catch (Exception ex)
        {
            _logger.LogError(ex.Message, ex);

            if (_env.IsDevelopment())
            {
                var dic = new Dictionary<string, string>
                {
                    ["StackTrace"] = ex.StackTrace,
                    ["Exception"] = ex.Message
                };
                message = JsonConvert.SerializeObject(dic);
            }
            else
            {
                message = "an error has occurred";
            }
            await WriteToReponseAsync();
        }
        async Task WriteToReponseAsync()
        {
            if (context.Response.HasStarted)
                throw new InvalidOperationException("The response has already started");
            var exceptionResult = new ExceptionResult(message, (int)httpStatusCode);
            var result = JsonConvert.SerializeObject(exceptionResult);
            context.Response.StatusCode = (int)httpStatusCode;
            context.Response.ContentType = "application/json";
            await context.Response.WriteAsync(result);
        }
    }
}

ExceptionResutl class:

public class ExceptionResult 
{
    public ExceptionResult(string message, int statusCode)
    {
        this.Message = message;
        this.StatusCode = statusCode;
    }
    public string Message { get; set; }
    public int StatusCode { get; set; }
}

public static class ExceptionHandlerMiddlewareExtension
{
    public static void UseAdvancedExceptionHandler(this IApplicationBuilder app)
    {
        app.UseMiddleware<AdvancedExceptionHandler>();
    }
}

Then adding middleware in Configure method

public void Configure(IApplicationBuilder app)
{
    app.UseAdvancedExceptionHandler();//<--NOTE THIS
}

I don't use try/catch block in controllers. (my opinion)

CodePudding user response：

Catching exceptions in your controller will quickly start to violate some clean code principles, like DRY.

If I understand correctly, the example you have written is that you want to log some errors in case any exceptions are thrown in your code. This is reasonable, but if you begin to add more endpoints, you'll notice you have the same try/catch in all your controller methods. The best way to refactor this is to use a middleware that will catch the exception and map it to a response that you want.

Over time as you begin to update your application to have more features you may have a situation where multiple endpoints are throwing similar errors and you want it to be handled in a similar way. For example, in your example, if the user doesn't exist, the application (in your service layer) may throw an UserNotFoundException, and you may have some other endpoints which can throw the same error, too.

You could create another middleware to handle this or even extend your existing middleware.

One of the better approaches I've seen over the years is to use this library https://github.com/khellang/Middleware/tree/master/src/ProblemDetails to handle the boiler plate for you.