Code review: User Controller and error handling



This is the first code review where I showcase some real-world code example and nitpick suggest improvements in it. If you want to learn more about this new format, check out this post. You can also request a review yourself. To do that, use the form on this page.

Code review: User Controller and error handling

The code in question is a UserController class with this Create method:

public IActionResult Create([FromBody] UserCreateModel user)

{

    try

    {

        var man = new UserManager();

        ControllerAction res = man.ValidateUser(user, ModelState);

 

        if (res.IsSuccess)

        {

            bool userExists = _userRepository.GetUsers()

                .Any(u => u.FirstName == user.FirstName && u.LastName == user.LastName);

            res = man.ValidateUserAfterExists(userExists);

        }

 

        if (res.IsSuccess)

        {

            var userToAdd = Mapper.Map<User>(user);

 

            _userRepository.Add(userToAdd);

 

            if (!_userRepository.SubmitChanges())

            {

                return StatusCode(500, D4GErrorMessage.DatabaseSavingError);

            }

        }

 

        return res.Action;

    }

    catch (Exception exception)

    {

        _logger.LogCritical($“Failed to create user\n{exception});

        return BadRequest(“Failed to create user”);

    }

}

It uses UserManager to validate the incoming user data:

public class UserManager

{

    public ControllerAction ValidateUser(

        UserCreateModel user, ModelStateDictionary modelState)

    {

        if (user == null)

            return new ControllerAction(

                new BadRequestObjectResult($“Input {nameof(UserCreateModel)} is null”),

                “”,

                LogType.None,

                false);

 

        if (!modelState.IsValid)

            return new ControllerAction(

                new BadRequestObjectResult(modelState),

                “Error while validating user”,

                LogType.Error,

                false);

 

        return new ControllerAction(

            new CreatedAtRouteResult(“default”, user),

            string.Empty,

            LogType.None,

            true);

    }

 

    public ControllerAction ValidateUserAfterExists(bool userExists)

    {

        if (userExists)

            return new ControllerAction(

                new BadRequestObjectResult($”User already exists”),

                “”,

                LogType.None,

                false);

    }

}

Which relies on this wrapper to return information about validation errors:

public struct ControllerAction

{

    public readonly IActionResult Action;

    public readonly bool IsSuccess;

    public readonly string LogMessage;

    public readonly LogType LogType;

 

    public ControllerAction(

        IActionResult action, string logMessage, LogType logType, bool isSuccess)

    {

        Action = action;

        LogMessage = logMessage;

        LogType = logType;

        IsSuccess = isSuccess;

    }

}

Alright, so the first thing I’d like to pay attention to is the try-catch block in the Create method:

public IActionResult Create([FromBody] UserCreateModel user)

{

    try

    {

        /* … */

    }

    catch (Exception exception)

    {

        _logger.LogCritical($“Failed to create user\n{exception});

        return BadRequest(“Failed to create user”);

    }

}

There are two issues with it. First, it’s a try-catch which doesn’t comply with a guideline I described in my Functional C# course (also wrote about it here: Exceptions for flow control). That is, you can either catch exceptions at the lowest level of your execution stack possible, when you want to convert some exception from a 3rd party library into a Result instance, or you can catch them at the highest level possible to log the debug information and return the user some generic apology. Everything in between falls into the trap of using exceptions to control the program flow which has the effect similar to that of the goto statement.

The second issue is that the exception gets converted into a 400 response (bad request) whereas, semantically, it’s a 500 one (internal server error). I wrote about it in more detail here: REST API response codes: 400 vs 500. In short: never mask server failures behind 400 responses. The generic exception is clearly a server failure and should be depicted as such.

To overcome these two problems, we need to introduce a middleware:

public async Task Invoke(HttpContext context)

{

    try

    {

        await _next(context);

    }

    catch (Exception ex)

    {

        await HandleExceptionAsync(context, ex);

    }

}

 

private Task HandleExceptionAsync(HttpContext context, Exception exception)

{

    string result = JsonConvert.SerializeObject(new { error = exception.Message });

    context.Response.ContentType = “application/json”;

    context.Response.StatusCode = (int)HttpStatusCode.InternalServerError;

    return context.Response.WriteAsync(result);

}

that would handle all uncaught exceptions from your controllers by returning an internal server error response. This is the “highest level possible” I mentioned earlier with regards to ASP.NET Core applications. In “Classical” ASP.NET MVC, that would be global.asax.

Note that it’s a good idea to create an application-wide envelope to wrap your responses with and not just use an anonymous type like in the above sample. Here’s an example of one: link.

Now that we have introduced this logic to the application, it applies to all our controllers. We can remove the try-catch block:

public IActionResult Create([FromBody] UserCreateModel user)

{

    var man = new UserManager();

    ControllerAction res = man.ValidateUser(user, ModelState);

 

    if (res.IsSuccess)

    {

        bool userExists = _userRepository.GetUsers()

            .Any(u => u.FirstName == user.FirstName && u.LastName == user.LastName);

        res = man.ValidateUserAfterExists(userExists);

    }

 

    if (res.IsSuccess)

    {

        var userToAdd = Mapper.Map<User>(user);

 

        _userRepository.Add(userToAdd);

 

        if (!_userRepository.SubmitChanges())

        {

            return StatusCode(500, D4GErrorMessage.DatabaseSavingError);

        }

    }

 

    return res.Action;

}

So far so good. The next issue I’d like to address lies in here:

if (!_userRepository.SubmitChanges())

{

    return StatusCode(500, D4GErrorMessage.DatabaseSavingError);

}

Note that the code returns a 500 response which is good – inability to save the user to the database is clearly a server’s fault, not the client’s one. But look at the way it is handled. The SubmitChanges method returns a boolean which signalizes success of the operation. Presumably, there’s a try-catch block inside this method that converts all exceptions from Entity Framework into a return value.

It might look like a legit use of a try-catch statement. After all, didn’t I just mention that exceptions should be caught at the lowest level of the code’s execution stack possible? There’s another problem with it, however. As I described in this post, all failures can be categorized into two buckets:

  • Errors we know how to deal with.
  • Errors we don’t know how to deal with.

The first one should be addressed with return values, and the second one – with exceptions. Inability to save something to the database falls into the second bucket: it is not a failure we can work around, and thus we should not be using return values to handle them.

The code also violates another guideline. Which is: never return 500 errors intentionally. Let the global exception handler take care of all of them.

So, how to address these problems? Just remove the try-catch statement from the repository’s SubmitChanges and let all DB-related exceptions propagate straight to the middleware we just created. Therefore, the if statement checking for the result of the operation can be removed as well:

public IActionResult Create([FromBody] UserCreateModel user)

{

    var man = new UserManager();

    ControllerAction res = man.ValidateUser(user, ModelState);

 

    if (res.IsSuccess)

    {

        bool userExists = _userRepository.GetUsers()

            .Any(u => u.FirstName == user.FirstName && u.LastName == user.LastName);

        res = man.ValidateUserAfterExists(userExists);

    }

 

    if (res.IsSuccess)

    {

        var userToAdd = Mapper.Map<User>(user);

 

        _userRepository.Add(userToAdd);

 

        // All exceptions thrown here will be caught in the middleware

        _userRepository.SubmitChanges();

    }

 

    return res.Action;

}

Now, let’s look at the wrapper we are using to denote a validation result:

public struct ControllerAction

{

    public readonly IActionResult Action;

    public readonly bool IsSuccess;

    public readonly string LogMessage;

    public readonly LogType LogType;

 

    public ControllerAction(

        IActionResult action, string logMessage, LogType logType, bool isSuccess)

    {

        Action = action;

        LogMessage = logMessage;

        LogType = logType;

        IsSuccess = isSuccess;

    }

}

A good thing about it is that it’s a struct. On the other side, it conflates an ASP.NET MVC concern – IActionResult – with the application concern – whether or not the validation succeeded. These should be separated: let your application services layer (in our case, the controller) do its job and then just transform the outcome into an ASP.NET action result.

Another issue here is the LogMessage and IsSuccess properties. No need to keep both, one can be derived from the other.

This is what the struct looks like after the refactoring:

public struct ValidationResult

{

    public bool IsSuccess => string.IsNullOrEmpty(LogMessage);

    public readonly string LogMessage;

    public readonly LogType LogType;

 

    public ValidationResult(string logMessage, LogType logType)

    {

        LogMessage = logMessage;

        LogType = logType;

    }

}

Note that I also renamed it into ValidationResult because it better conveys the meaning behind it.

Now, let’s look at the validation itself. Here’s the original version of the UserManager class once again:

public class UserManager

{

    public ControllerAction ValidateUser(

        UserCreateModel user, ModelStateDictionary modelState)

    {

        if (user == null)

            return new ControllerAction(

                new BadRequestObjectResult($“Input {nameof(UserCreateModel)} is null”),

                “”,

                LogType.None,

                false);

 

        if (!modelState.IsValid)

            return new ControllerAction(

                new BadRequestObjectResult(modelState),

                “Error while validating user”,

                LogType.Error,

                false);

 

        return new ControllerAction(

            new CreatedAtRouteResult(“default”, user),

            string.Empty,

            LogType.None,

            true);

    }

 

    public ControllerAction ValidateUserAfterExists(bool userExists)

    {

        if (userExists)

            return new ControllerAction(

                new BadRequestObjectResult($”User already exists”),

                “”,

                LogType.None,

                false);

    }

}

Having a separate class just for validation purposes is a code smell. Either move all application (non-domain) logic to it, or keep it in the controller itself. In the former case, you are extracting a new application services layer and the controller becomes just a thin wrapper on top of it. In the latter – you treat the controller as such a layer. Both solutions are good. The existing implementation, however, falls right in the middle between them.

So, let’s merge UserManager into UserController:

public IActionResult Create([FromBody] UserCreateModel user)

{

    ValidationResult res = ValidateUser(user, ModelState);

 

    if (res.IsSuccess)

    {

        bool userExists = _userRepository.GetUsers()

            .Any(u => u.FirstName == user.FirstName && u.LastName == user.LastName);

        res = ValidateUserAfterExists(userExists);

    }

 

    if (res.IsSuccess)

    {

        var userToAdd = Mapper.Map<User>(user);

 

        _userRepository.Add(userToAdd);

        _userRepository.SubmitChanges();

    }

 

    return res.IsSuccess

        ? (IActionResult)CreatedAtRoute(“default”, user)

        : BadRequest(res.LogMessage);

}

 

private ValidationResult ValidateUser(

    UserCreateModel user, ModelStateDictionary modelState)

{

    if (user == null)

        return new ValidationResult($“Input is null”, LogType.None);

 

    if (!modelState.IsValid)

        return new ValidationResult(“Error while validating user”, LogType.Error);

 

    return new ValidationResult(string.Empty, LogType.None);

}

 

public ValidationResult ValidateUserAfterExists(bool userExists)

{

    if (userExists)

        return new ValidationResult(“User already exists”, LogType.None);

}

In fact, even these methods are not needed here. We can apply a little bit of Early Exit magic and vualá:

public IActionResult Create([FromBody] UserCreateModel user)

{

    if (user == null)

        return BadRequest($“Input {nameof(UserCreateModel)} is null”);

 

    if (!ModelState.IsValid)

        return BadRequest(ModelState);

 

    bool userExists = _userRepository.GetUsers()

        .Any(u => u.FirstName == user.FirstName && u.LastName == user.LastName);

 

    if (userExists)

        return BadRequest(“User already exists”);

 

    var userToAdd = Mapper.Map<User>(user);

 

    _userRepository.Add(userToAdd);

    _userRepository.SubmitChanges();

 

    return CreatedAtRoute(“default”, user);

}

The next issue here is these two lines:

bool userExists = _userRepository.GetUsers()

    .Any(u => u.FirstName == user.FirstName && u.LastName == user.LastName);

What they do is they pull the whole list of users from the database and then check for matches in the memory. That’s a significant performance breaker. Instead, we need to pass the two parameters inside the repository method so that, at any given time, it retrieves no more than one record:

public IActionResult Create([FromBody] UserCreateModel user)

{

    if (user == null)

        return BadRequest($“Input {nameof(UserCreateModel)} is null”);

 

    if (!ModelState.IsValid)

        return BadRequest(ModelState);

 

    Maybe<User> existingUser = _userRepository.GetUser(user.FirstName, user.LastName);

    if (existingUser.HasValue)

        return BadRequest(“User already exists”);

 

    var userToAdd = Mapper.Map<User>(user);

 

    _userRepository.Add(userToAdd);

    _userRepository.SubmitChanges();

 

    return CreatedAtRoute(“default”, user);

}

Note that the method returns a Maybe of User. This way, we make it honest about its possible outcomes: it returns either a user or nothing, depending on the data currently existing in the database.

Finally, take a look at this line:

var userToAdd = Mapper.Map<User>(user);

This is not a good use of AutoMapper and I wrote about it here: On AutoMappers. In short: never use automappers to map incoming DTOs to domain classes because it completely obliterates the encapsulation of the latter. In order to make this mapping work, you need to introduce public setters to all the properties in your domain classes and turn off any invariant checks you might otherwise have.

An alternative? Just instantiate your domain classes manually:

var userToAdd = new User(user.FirstName, user.LastName);

Not only does it keep the User entity encapsulated, it also brings explicitness regarding what fields are needed in order to create that entity. And you can keep all property setters in your entities private, as you should by default.

Here’s how the end result looks like after all our refactorings:

// UserController

public IActionResult Create([FromBody] UserCreateModel user)

{

    if (user == null)

        return BadRequest($“Input {nameof(UserCreateModel)} is null”);

 

    if (!ModelState.IsValid)

        return BadRequest(ModelState);

 

    Maybe<User> existingUser = _userRepository.GetUser(user.FirstName, user.LastName);

    if (existingUser.HasValue)

        return BadRequest(“User already exists”);

 

    var userToAdd = new User(user.FirstName, user.LastName);

 

    _userRepository.Add(userToAdd);

    _userRepository.SubmitChanges();

 

    return CreatedAtRoute(“default”, user);

}

// Middleware

public async Task Invoke(HttpContext context)

{

    try

    {

        await _next(context);

    }

    catch (Exception ex)

    {

        await HandleExceptionAsync(context, ex);

    }

}

 

private Task HandleExceptionAsync(HttpContext context, Exception exception)

{

    string result = JsonConvert.SerializeObject(new { error = exception.Message });

    context.Response.ContentType = “application/json”;

    context.Response.StatusCode = (int)HttpStatusCode.InternalServerError;

    return context.Response.WriteAsync(result);

}

Conclusion

Okay, that’s it for this code review session. If you want me to review your own code, send it over using the submit form on the right side bar or on this page.

Reference list:

Share




  • Amir Shitrit

    I like this format. It’s very informative.
    Perhaps it would also be nice to embed a quiz which let’s the reader test his/her coding skills just before reading the answer. You could also see where most people got it right/wrong.
    Thanks!

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      Thanks, glad that you liked it 🙂
      That’s an interesting idea, I’ll see if I could find a way to implement it.

  • Kirk Larkin

    I’ve been waiting for the first code review to see exactly how this format works (perhaps I’m too scared to submit my own code) and based on this post alone, I think it’s going to be very valuable. Having read a great number of posts on here, I’ve often felt like I “get it” but then sometimes have difficulty applying the concepts to my own code. These real-world code examples will help reinforce the material presented in your posts and I look forward to seeing more.

    The inline references are, for me, the most useful aspects of this format, giving a sense of an almost context-sensitive help system and adding depth to the Enterprise Craftsmanship site as a whole.

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      Glad that you found it valuable, thanks! And don’t hesitate to submit your own code, that is an interesting learning experience for everyone (including me).

  • Peter

    “Having a separate class just for validation purposes is a code smell. Either move all application (non-domain) logic to it, or keep it in the controller itself. ”

    Maybe it’s just me but I don’t really like private helper methods in controllers (or any other method which is not an actionmethod). After a while there will be countless public AND private methods and it will be hard to tell who uses what (and those private methods will start calling each other).
    I don’t see any problem with extracting validation functionality (control logic) if we keep that code in the same abstraction as the controller is on. (handling modelstate, passing parameters to domain layers, converting back, etc)

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      I would say that if a controller gets too bloated, it’s a good idea to split it. Or, alternatively, you could keep the single controller but extract everything (not only validation) out of it into a separate application services layer (possibly consisting of multiple classes).

      Although, it indeed may be helpful to extract some validation rules in order to make them more declarative. Something like FluentValidator could help here. In other cases, separating validation from other operations generally worsen the cohesion.

  • Piotr Pasieka

    In review you mentioned that using Envelope to wrap response is good idea. Some time ago I would agree with that in 100% but lately I build my apps responses based on HTTP status codes.

    When:
    * everything is ok then I respond with 200 + some serialized object
    * validation went wrong then I respond wit 400 + error messages for specific fields + general message
    * server error occurs then I respond with 500 (just like you suggested through high level catch)

    What are true benefits of using Envelope in compassion to HTTP codes in case where Envelope has only result and error massage fields?

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      One benefit in using Envelope is that its strongly-typed structure protects you from accidentally breaking the format of the output message. For instance, you can’t mistype the `Error` property. Whereas, with anonymous types, it is a possibility.

      Another benefit is that it’s generally easier for clients to process responses when they have the same structure, regardless of the output.

      And by the way, I usually still use HTTP codes. It’s just the content of the response always have the same set of fields regardless of them (non-relevant fields are set to nulls).

  • Jedielson Nakonieczni

    I would remove the validation to test if UserCreateModel is null too.
    Because of Asp.Net Mvc data-binding, the properties inside this object must have public set. So, unless this class has no empty constructors, this object will not be null.

    The properties inside of it could be null, but in this case, you can make use of data-annotations for validation and the ModelState.IsValid is enough.

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      Could point. Indeed, this validation looks unnecessary here.