Code review: User Controller and error handling

May 1, 2017

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.