-1

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?

2
  • Why does the default MVC model binder not work? May 28, 2020 at 17:06
  • @GregBurghardt As you can see in my custom binder, I'm mapping from the UserToRegister to the User object, so I'm getting some extra properties, such as the user's Id, or the hashed password...
    – Ferran R.
    May 29, 2020 at 6:28

2 Answers 2

0

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.

0

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.

2
  • I get it and it sounds quite reasonable to me, I just don't really know where I should do this mapping now, but thanks, you made me understand why it shouldn't be in the custom model binder!
    – Ferran R.
    May 30, 2020 at 9:47
  • @Ferran.R: I had the same question. May 30, 2020 at 13:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.