If View Model property has value with certain length, replace it with query from database.Do it in Controller, Validation Attribute or somewhere else?

Question

My View Model looks like this:

public class ProductVM
{
       public string SerialNumber { get; set; }

       // Other properties left out for brevity
}

In the form that fills this model, users can supply the SerialNumber of the Product or they can supply the ProductId. The SerialNumber is always 100 characters long and the ProductId is always 50 characters long.

My current solution is to get the SerialNumber from database if ProductId is supplied, inside the Action method of the Controller, after checking if ModelState.IsValid. From my view of thinking, this is bad because of the following reasons:

All validation logic on the parameters data passed to the action method(same goes for View Models) should be done when calling ModelState.IsValid.
It makes the controller 'Fat' which is viewed as an anti-pattern, as it makes it harder to understand the flow of the process.

What I want to know is:

Should I leave the code as is?
Should I move this check/transformation outside the controller action method, and if so, where?
Should I make new Service Query to fetch the needed data, but instead of querying by SerialNumber, to query by ProductId? (This solution looks to me like repetition of code for most of the part)

Currently my Action method looks like this:

[HttpPost]
public ActionResult GenerateReport(ProductVM vm)
{
    if(ModelState.IsValid == false)
    {
        vm.Rehydrate();
        return View(vm);
    }

    // If Length is 100 then ProductId has been supplied, query database to find SerialNumber
    if(vm.SerialNumber.Length == 100)
    {
        vm.SerialNumber = _db.GetSerialNumber(vm.SerialNumber);'

        // This is the thing that I don't like.
        // Multiple places where data validation of View Model is being done inside Controller.
        if(string.IsNullOrEmpty(vm.SerialNumber))
        {
            ModelState.AddModelError("", "No Product has been found with the given ProductId!");
            return View(vm);
            
        }
    }

    var reportData = _productDomainService.GetReportData(vm.SerialNumber);
    var report = _infrastructure.GenerateReport(reportData);

    // Rest of code left out for brevity
    
}

Antonio Ferreira · Accepted Answer · 2024-08-25 16:57:35Z

2

My suggestion is to refactor the code to improve separation of concerns. While it's valid to use ModelState.IsValid for standard validations such as Required and MaxLength, more complex validation that involves database interaction, such as verifying the SerialNumber or ProductId, should be handled in a dedicated service.

Here’s how you can refactor the code:

1. Create a Service for Validation and Transformation

Create a service that handles the logic of transforming and validating the SerialNumber and ProductId. This will keep your controller focused on orchestrating actions rather than managing detailed validation.

Service Implementation Example:

public interface IProductService
{
    string TransformProductIdentifier(string identifier, out string errorMessage);
}

public class ProductService : IProductService
{
    private readonly YourDbContext _db;

    public ProductService(YourDbContext db)
    {
        _db = db;
    }

    public string TransformProductIdentifier(string identifier, out string errorMessage)
    {
        if (identifier.Length == 100) // Assuming this is a ProductId
        {
            var serialNumber = _db.GetSerialNumber(identifier);
            if (string.IsNullOrEmpty(serialNumber))
            {
                errorMessage = "No Product has been found with the given ProductId!";
                return null;
            }
            errorMessage = null;
            return serialNumber;
        }

        errorMessage = null;
        return identifier; // Assume it is already a valid SerialNumber
    }
}

2. Update the Controller to Use the Service

Refactor the controller to delegate validation and transformation to the service. This approach adheres to the principle of Single Responsibility and keeps the controller simpler.

Controller Implementation Example:

public class ReportController : Controller
{
    private readonly IProductService _productService;
    private readonly IProductDomainService _productDomainService;
    private readonly IInfrastructure _infrastructure;

    public ReportController(IProductService productService, 
                            IProductDomainService productDomainService, 
                            IInfrastructure infrastructure)
    {
        _productService = productService;
        _productDomainService = productDomainService;
        _infrastructure = infrastructure;
    }

    [HttpPost]
    public ActionResult GenerateReport(ProductVM vm)
    {
        if (!ModelState.IsValid)
        {
            vm.Rehydrate();
            return View(vm);
        }

        // Use the service to transform and validate the identifier
        var serialNumber = 
          _productService.TransformProductIdentifier(vm.SerialNumber, out var 
          errorMessage);

        if (!string.IsNullOrEmpty(errorMessage))
        {
            ModelState.AddModelError("", errorMessage);
            return View(vm);
        }

        vm.SerialNumber = serialNumber;

        var reportData = _productDomainService.GetReportData(vm.SerialNumber);
        var report = _infrastructure.GenerateReport(reportData);

        //more code
   }
}

answered Aug 25, 2024 at 16:57

Antonio Ferreira

563 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Nikola Over a year ago

what kind of a service is IProductService? Is it Application Service?

Nikola Over a year ago

If IProductService is indeed Application Layer Service, than should'nt the validation and transformation be implicit and not explicit as in the given answer? Should it not wrap up the generation of the report data also, that I do inside _productDomainService.GetReportData() and only expose a public interface to the Use Case as a whole instead of the validation and transformation, which in this case would be made implicit and it's use not known to the Controller Action method?

Antonio Ferreira Over a year ago

Yes, it is a good approach to keep your public interface for the Use Case simple and cohesive. By having _productDomainService.GetReportData() handle the validation and transformation internally, and exposing only a clean and focused public interface to the Use Case, you encapsulate the complexity and keep your Controller Action method clean and straightforward. This way, the Controller doesn't need to know about the underlying validation and transformation details, leading to better separation of concerns and more maintainable code.

Mark Seemann · Accepted Answer · 2024-09-01 09:04:34Z

2

As a general rule, I find it helpful to distinguish between validation and business rules. The validation rules that ASP.NET ModelState.IsValid typically check mostly coincide with what I, too, call validation. Attributes and other self-contained checks that don't require a snapshot of the entire system.

The rule in the OP about ProductId and SerialNumber is really an existence check. It does, indeed, require a snapshot of the entire system, because you need to query a database whether you have a row with that number.

This indicates to me that the code shown in the OP lacks at least two other branches. The current code only looks in the database if SerialNumber.Length == 100. Shouldn't it also check in the database if SerialNumber.Length == 50? Just because the correct number of characters were supplied, you can't conclude that a row with that number exists.

Furthermore, what should happen if SerialNumber.Length isn't either 50 or 100? This latter case you might already have encoded as a validation rule that can be caught by ModelState.IsValid, but the two lookups are better modelled as a business rule. Thus, there's nothing wrong with it being implemented outside of ASP.NET's validation system.

You can leave it as is, or adopt code similar to that suggested by Antônio.

answered Sep 1, 2024 at 9:04

Mark Seemann

234k51 gold badges448 silver badges778 bronze badges

2 Comments

Nikola Over a year ago

Very good explanation in the article. Correct me if I am wrong, the pre-condition validation checks like SerialNumber length and the transformation from ProductId to SerialNumber should go Application Layer and after that there should be existence check(or more complex business logic check) that is done by the Domain Layer. Does that imply that ModelState should be avoided and all validation checks be moved in upper layers?

Mark Seemann Over a year ago

@Nikola You can keep checking ModelState if it's useful to you. I mostly do REST APIs, and my knowledge of the MVC details are a little rusty, but AFAICT the ModelState API serves a purpose related to model binding and UI (forms, etc.).

Collectives™ on Stack Overflow

If View Model property has value with certain length, replace it with query from database.Do it in Controller, Validation Attribute or somewhere else?

2 Answers 2

1. Create a Service for Validation and Transformation

2. Update the Controller to Use the Service

3 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1. Create a Service for Validation and Transformation

2. Update the Controller to Use the Service

3 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related