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;
public 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 another shortcoming. 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();
}
}
IMPORTANT: ICommandHandler<CreatePurchaseOrder> now replaces IPurchaseOrderService interface and CreatePurchaseOrderHandler replaces the PurchaseOrderService implementation. Consumers will now not longer depend on IPurchaseOrderService (as that will no longer exist) but instead depend on (get injected with) an ICommandHandler<CreatePurchaseOrder>.
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.
