Best place to map Model got in ActionMethod

Question

I'm not sure this question matches this forum's purpose, but I didn't think it should belong to the stackoverflow one either, so here it goes:

I created a model binder that makes one mapping so "clean" by putting it in the model binder itself, but now I wanted to do it again in another action method and I was just wondering if there would be a better way to do it, since I'm not convinced that's the right place to do so.

My action method looks like this:

[HttpPost("Register")]
public async Task<IActionResult> Register(
    [FromQuery] UserToRegister userToRegister,
    User user /*This property is never used from the body request, since I set it in my custom Model Binder*/)
{
    var response = await _userService.RegisterAsync(user);
    return Ok(response);
}

I created a custom Model Binder, where I map the properties from userToRegister into the user param. So this custom binder looks like this:

public Task BindModelAsync(ModelBindingContext bindingContext)
{
    var values = bindingContext.ValueProvider;
    User user = new User()
    {
        Id = Guid.NewGuid(),
        Name = values.GetValue("Name").FirstValue,
        Password = /*password encrypted*/,
        ...
    };
    bindingContext.Result = ModelBindingResult.Success(user);
    return Task.CompletedTask;
}

So do you find it a good solution? Would it better to get the User object in the request so I could modify it later? Should I do this modification (hashing the password, creating a new Id, etc) in the ModelBinder?

Answer 1

The custom model binder is taking the place of a use case in your application. While this technically works, it would be surprising to most people to find this logic buried deep in the architecture of the UI layer. Furthermore you are not able to leverage the automatic data conversion you get with the default model binder.

Your current use case only seems to deal with strings, so no big deal here. When the use case needs numbers or dates, then you are stuck writing manual conversion code, forcing you to write a pile of boiler plate code that you don't need to write when utilizing the default model binder.

I've always been a little suspect about this sort of thing. It starts to become chicken voodoo magic, because it is not obvious from the controller code how the use case is being executed. The Principal of Least Astonishment applies here. I would not expect a business use case to be executed here. Common places for this to exist are:

• The controller action method
• A "service" class that the controller method calls
• A "unit of work" class that the controller calls
• A "use case" class that the controller calls, if applying something like hexagonal architecture

I'm not making a judgement about whether any of the above are best practices. Those are just common places to find this behavior.

Basically I expect the control flow to go like this:

Browser -> HTTP Request -> MVC framework -> controller -> ??? -> business class

Where you replace ??? with something you can trace back to a method call initiated by the controller.

Answer 2

Unless the logic is very complicated, there's no reason the controller can't do the mapping.

[HttpPost("Register")]
public async Task<IActionResult> Register([FromQuery] UserToRegister userToRegister)
{
    var user = new User {
        Name = userToRegister.Name,
        Email = userToRegister.Email
    };
    var response = await _userService.RegisterAsync(user);
    return Ok(response);
}

This code is type-safe and easily understood and keeps the code for the mapping logic near where the mapping takes place. So it's a good default option.

If the logic is very complicated, you can write a utility class or utility methods to do the mapping; for example, you could give the UserToRegister class a ToUser() method. Or you could use a tool like AutoMapper. But I wouldn't bother unless your mapping logic causes your action method to become too large to be easily understood.