Separating the service layer from the validation layer

Question

I currently have a service layer based on the article Validating with a service layer from the ASP.NET site.

According to this answer, this is a bad approach because the service logic is mixed with the validation logic which violates the single responsibility principle.

I really like the alternative that is supplied but during re-factoring of my code I have come across a problem that I am unable to solve.

Consider the following service interface:

interface IPurchaseOrderService
{
    void CreatePurchaseOrder(string partNumber, string supplierName);
}

with the following concrete implementation based on the linked answer:

public class PurchaseOrderService : IPurchaseOrderService
{
    public void CreatePurchaseOrder(string partNumber, string supplierName)
    {
        var po = new PurchaseOrder
        {
            Part = PartsRepository.FirstOrDefault(p => p.Number == partNumber),
            Supplier = SupplierRepository.FirstOrDefault(p => p.Name == supplierName),
            // Other properties omitted for brevity...
        };

        validationProvider.Validate(po);
        purchaseOrderRepository.Add(po);
        unitOfWork.Savechanges();
    }
}

The PurchaseOrder object that is passed to the validator also requires two other entities, Part and Supplier (let's assume for this example that a PO only has a single part).

Both the Part and Supplier objects could be null if the details supplied by the user do not correspond to entities in the database which would require the validator to throw an exception.

The problem I have is that at this stage the validator has lost the contextual information (the part number and the supplier name) so is unable to report an accurate error to the user. The best error I can supply is along the lines of "A purchase order must have an associated part" which would not make sense to the user because they did supply a part number (it just does not exist in the database).

Using the service class from the ASP.NET article I am doing something like this:

public void CreatePurchaseOrder(string partNumber, string supplierName)
{
    var part = PartsRepository.FirstOrDefault(p => p.Number == partNumber);
    if (part == null)
    {
        validationDictionary.AddError("", 
            string.Format("Part number {0} does not exist.", partNumber);
    }

    var supplier = SupplierRepository.FirstOrDefault(p => p.Name == supplierName);
    if (supplier == null)
    {
        validationDictionary.AddError("",
            string.Format("Supplier named {0} does not exist.", supplierName);
    }

    var po = new PurchaseOrder
    {
        Part = part,
        Supplier = supplier,
    };

    purchaseOrderRepository.Add(po);
    unitOfWork.Savechanges();
}

This allows me to provide much better validation information to the user but means that the validation logic is contained directly in the service class, violating the single responsibility principle (code is also duplicated between service classes).

Is there a way of getting the best of both worlds? Can I separate the service layer from the validation layer whilst still providing the same level of error information?

Answer 1

Short answer:

You are validating the wrong thing.

Very long answer:

You are trying to validate a PurchaseOrder but that is an implementation detail. Instead what you should validate is the operation itself, in this case the partNumber and supplierName parameters.

Validating those two parameters by themselves would be awkward, but this is caused by your design—you're missing an abstraction.

Long story short, the problem is with your IPurchaseOrderService interface. It shouldn't take two string arguments, but rather one single argument (a Parameter Object). Let's call this Parameter Object CreatePurchaseOrder:

public class CreatePurchaseOrder
{
    public string PartNumber;
    public string SupplierName;
}

With the altered IPurchaseOrderService interface:

interface IPurchaseOrderService
{
    void CreatePurchaseOrder(CreatePurchaseOrder command);
}

The CreatePurchaseOrder Parameter Object wraps the original arguments. This Parameter Object is a message that describes the intend of the creation of a purchase order. In other words: it's a command.

Using this command, you can create an IValidator<CreatePurchaseOrder> implementation that can do all the proper validations including checking the existence of the proper parts supplier and reporting user friendly error messages.

But why is the IPurchaseOrderService responsible for the validation? Validation is a cross-cutting concern and you should prevent mixing it with business logic. Instead you could define a decorator for this:

public class ValidationPurchaseOrderServiceDecorator : IPurchaseOrderService
{
    private readonly IValidator<CreatePurchaseOrder> validator;
    private readonly IPurchaseOrderService decoratee;

    ValidationPurchaseOrderServiceDecorator(
        IValidator<CreatePurchaseOrder> validator,
        IPurchaseOrderService decoratee)
    {
        this.validator = validator;
        this.decoratee = decoratee;
    }

    public void CreatePurchaseOrder(CreatePurchaseOrder command)
    {
        this.validator.Validate(command);
        this.decoratee.CreatePurchaseOrder(command);
    }
}

This way you can add validation by simply wrapping a real PurchaseOrderService:

var service =
    new ValidationPurchaseOrderServiceDecorator(
        new CreatePurchaseOrderValidator(),
        new PurchaseOrderService());

Problem, of course, with this approach is that it would be really awkward to define such decorator class for each service in the system. That would cause severe code publication.

But the problem is caused by a flaw. Defining an interface per specific service (such as the IPurchaseOrderService) is typically problematic. You defined the CreatePurchaseOrder and, therefore, already have such a definition. You can now define one single abstraction for all business operations in the system:

public interface ICommandHandler<TCommand>
{
    void Handle(TCommand command);
}

With this abstraction you can now refactor PurchaseOrderService to the following:

public class CreatePurchaseOrderHandler : ICommandHandler<CreatePurchaseOrder>
{
    public void Handle(CreatePurchaseOrder command)
    {
        var po = new PurchaseOrder
        {
            Part = ...,
            Supplier = ...,
        };

        unitOfWork.Savechanges();
    }
}

With this design, you can now define one single generic decorator to handle all validations for every business operation in the system:

public class ValidationCommandHandlerDecorator<T> : ICommandHandler<T>
{
    private readonly IValidator<T> validator;
    private readonly ICommandHandler<T> decoratee;

    ValidationCommandHandlerDecorator(
        IValidator<T> validator, ICommandHandler<T> decoratee)
    {
        this.validator = validator;
        this.decoratee = decoratee;
    }

    void Handle(T command)
    {
        var errors = this.validator.Validate(command).ToArray();

        if (errors.Any())
        {
            throw new ValidationException(errors);
        }

        this.decoratee.Handle(command);
    }
}

Notice how this decorator is almost the same as the previously defined ValidationPurchaseOrderServiceDecorator, but now as a generic class. This decorator can be wrapped around your new service class:

var service =
    new ValidationCommandHandlerDecorator<PurchaseOrderCommand>(
        new CreatePurchaseOrderValidator(),
        new CreatePurchaseOrderHandler());

But since this decorator is generic, you can wrap it around every command handler in your system. Wow! How's that for being DRY?

This design also makes it really easy to add cross-cutting concerns later on. For instance, your service currently seems responsible for calling SaveChanges on the unit of work. This can be considered a cross-cutting concern as well and can easily be extracted to a decorator. This way your service classes become much simpler with less code left to test.

The CreatePurchaseOrder validator could look as follows:

public sealed class CreatePurchaseOrderValidator : IValidator<CreatePurchaseOrder>
{
    private readonly IRepository<Part> partsRepository;
    private readonly IRepository<Supplier> supplierRepository;

    public CreatePurchaseOrderValidator(
        IRepository<Part> partsRepository,
        IRepository<Supplier> supplierRepository)
    {
        this.partsRepository = partsRepository;
        this.supplierRepository = supplierRepository;
    }

    protected override IEnumerable<ValidationResult> Validate(
        CreatePurchaseOrder command)
    {
        var part = this.partsRepository.GetByNumber(command.PartNumber);

        if (part == null)
        {
            yield return new ValidationResult("Part Number", 
                $"Part number {command.PartNumber} does not exist.");
        }

        var supplier = this.supplierRepository.GetByName(command.SupplierName);

        if (supplier == null)
        {
            yield return new ValidationResult("Supplier Name", 
                $"Supplier named {command.SupplierName} does not exist.");
        }
    }
}

And your command handler like this:

public class CreatePurchaseOrderHandler : ICommandHandler<CreatePurchaseOrder>
{
    private readonly IUnitOfWork uow;

    public CreatePurchaseOrderHandler(IUnitOfWork uow)
    {
        this.uow = uow;
    }

    public void Handle(CreatePurchaseOrder command)
    {
        var order = new PurchaseOrder
        {
            Part = this.uow.Parts.Get(p => p.Number == partNumber),
            Supplier = this.uow.Suppliers.Get(p => p.Name == supplierName),
            // Other properties omitted for brevity...
        };

        this.uow.PurchaseOrders.Add(order);
    }
}

Note that command messages will become part of your domain. There is a one-to-one mapping between use cases and commands and instead of validating entities, those entities will be an implementation detail. The commands become the contract and will get validation.

Note that it will probably make your life much easier if your commands contain as much IDs as possible. So your system would could benefit from defining a command as follows:

public class CreatePurchaseOrder
{
    public int PartId;
    public int SupplierId;
}

When you do this you won't have to check if a part by the given name does exist. The presentation layer (or an external system) passed you an ID, so you don't have to validate the existence of that part anymore. The command handler should of course fail when there's no part by that ID, but in that case there is either a programming error or a concurrency conflict. In either case no need to communicate expressive user friendly validation errors back to the client.

This does, however, moves the problem of getting the right IDs to the presentation layer. In the presentation layer, the user will have to select a part from a list for us to get the ID of that part. But still I experienced the this to make the system much easier and scalable.

It also solves most of the problems that are stated in the comments section of the article you are referring to, such as:

• The problem with entity serialization goes away, because commands can be easily serialized and model bind.
• DataAnnotation attributes can be applied easily to commands and this enables client side (Javascript) validation.
• A decorator can be applied to all command handlers that wraps the complete operation in a database transaction.
• It removes the circular reference between the controller and the service layer (via the controller's ModelState), removing the need for the controller to new the service class.

If you want to learn more about this type of design, you should absolutely check out this article.