Reputation: 99
I'm new in DDD previously I was working more with services and transaction scripts. Now I'm working with a team on a DDD project and from my point of view there is too much unnecessary data is loading and easy to forget about some data to load. I hope someone can help me to understand if there something wrong in this DDD project or it's me who just doesn't get it.
Assuming that we domain model and entity Order
with relation. To change order status should I load all its relations and then all nested relations? (example 1)
If I load only the relation that I need isn't then my Order not fully created object that can cause some issue later?(example 2)
Also if I do not load every relation (because there are a lot of nested relations) operation may throw exception or just wrong result, but how I can possibly know which relation are need without checking method implementation? (example 3)
public class Application
{
SomeDbContext _context;
public Application(SomeDbContext context)
{
_context = context;
}
public void RunSomeLogic()
{
///example 1
var orders = _context.Orders
.Include(x => x.Invoices).ThenInclude(x => x.Payments)
.Include(x => x.Customer)
.Include(x => x.VerifyByUser).Where(x => x.Status == OrderStatus.Paid).ToList();
foreach (var order in orders)
order.RefreshStatus();
_context.SaveChanges();
///example 2
var orders2 = _context.Orders
.Include(x => x.Invoices).ThenInclude(x => x.Payments)
.Include(x => x.VerifyByUser).Where(x => x.Status != OrderStatus.Paid).ToList();
foreach (var order in orders)
order.RefreshStatus();
_context.SaveChanges();
///example 3
var orders3 = _context.Orders
.Include(x => x.Invoices)//.ThenInclude(x => x.Payments)
.Include(x => x.Customer)
.Include(x => x.VerifyByUser).Where(x => x.Status == OrderStatus.Paid).ToList();
foreach (var order in orders)
order.RefreshStatus();
_context.SaveChanges();
}
}
public enum OrderStatus
{
New,
Veryfied,
Paid
}
public class User
{ }
public class Order
{
public ICollection<Invoice> Invoices { get; set; }
public OrderStatus Status { get; set; }
public User VerifyByUser { get; set; }
public Customer Customer { get; set; }
public DateTime OrderDater { get; set; }
public Guid Id { get; set; }
public void RefreshStatus()
{
if (Status == OrderStatus.New && VerifyByUser != null)
Status = OrderStatus.Veryfied;
else if (Status == OrderStatus.Veryfied )
{
foreach (var invoice in Invoices)
invoice.CheckPayments();
if( Invoices.All(x => x.IsPaid))
Status = OrderStatus.Paid;
}
}
}
public class Invoice
{
public Order Order { get; set; }
public int OrderId { get; set; }
public ICollection<Payment> Payments { get; set; }
public bool IsPaid { get; set; }
public decimal Value { get; set; }
public void CheckPayments()
{
if (Payments?.Sum(x => x.Value) >= Value)
IsPaid = true;
else
IsPaid = false;
}
}
public class Payment
{
public Invoice Invoice { get;set; }
public int InvoiceId { get; set; }
public decimal Value { get; set; }
///code
}
public class Customer
{
///code
}
Upvotes: 1
Views: 297
Reputation: 64121
You (or your team before you joined) are confusing DDD Entities and EF Core entities.
In DDD you don't make any assumptions of the persistence layer at all. Ideally there shouldn't even be Id
properties (except on the aggregate) on most domain objects, because that's mostly a property of relational databases. Other type of databases (document databases) don't have or require these (only for the document itself).
First, in EF Core entities are any complex objects which are not defined as "owned types" (Microsofts first attempt on addressing value types in EF Core) and have an id.
Taking an your order and Invoice as example, nothing there is an "entity" in DDD sense, except for Invoice.Order
property. Invoice
and Order
are aggregates, they are the transactional boundary.
All operations saved on an order, need to be done in a single transaction, hence all of the properties need to be loaded at the time the aggregate is loaded.
On the invoice, its a bit different. The Invoice (=aggregate) is referencing Order
(=another aggregate).
This should and must be modeled in a way that there is no direct reference to the Order
, because an invoice can't be loading another aggregate. Instead, it would have to be public OrderId Order { get; }
, in other words: Just a reference.
If you load an order and need to do any modifications on the order, you would need to create a domain service, that loads the Invoice
aggregate and then load the Order
aggregate from the id).
For example, if you want the status of the order to change, when an invoice is paid you would create an domain service (or a command handler/notification if you are using messaging) like
public class PaymentService
{
public Task InvoicePaid(InvoiceId invoiceId, IEnumerable<Payments> payments)
{
Invoice invoice = await unitOfWork.Invoices.GetById(invoiceId);
Order order = await unitOfWork.Orders.GetById(invoice.OrderId);
invoice.MakePayments(payments);
order.Completed();
await unitOfWork.SaveChangesAsync();
}
}
That's what domain services are for, to coordinate interaction between multiple aggregates and wrap it into one transaction.
That being said, DDD works better with document databases, as it better represents the "aggregate" as document.
Also the naming and encapsulation of your aggregates is questionable at best. You are setting IsPaid
somewhere then calling CheckPayment()
. This is very error prone, as its easy for a developer to modify your domain model or forget to call CheckPayment()
.
Instead you'd want to encapsulate your model
Instead of
public class Invoice
{
public Order Order { get; set; }
public int OrderId { get; set; }
public ICollection<Payment> Payments { get; set; }
public bool IsPaid { get; set; }
public decimal Value { get; set; }
public void CheckPayments()
{
if (Payments?.Sum(x => x.Value) >= Value)
IsPaid = true;
else
IsPaid = false;
}
}
you would prefer something like
public class Invoice
{
public OrderId OrderId { get; }
public IEnumerable<Payment> Payments { get; }
public bool IsPaid { get; }
public MonetaryAmount TotalSum { get; }
public void MakePayment(IEnumerable<Payment> paymentsDone)
{
// do some basic validation on payments
if(IsPaid)
{
throw new InvalidOperationException("Invoice is already paid!");
}
payments.AddRange(paymentsDone);
IsPaid = Payments.Sum(x => x.Value) >= TotalSum;
Status = OrderStatus.Paid;
}
}
Now, your domain model is protected. No one can set IsPaid
or Payments
from outside the class. They MUST use MakePayment
method and pass the payments. The method will validate the input, add the payments to the order and update the IsPaid
and Status
properties.
And this model is very easy to unit test. And since you can't modify the Invoice
, there is less testing required in the services (as they only coordinate)
Upvotes: 4
Reputation: 34653
Polling loops to do something like update a status /w a DDD approach is bound to get expensive. One option to help cut back the cost of eager loading all of that data would be to leverage a Split Query.
A DDD approach for something like this would be instead to ensure that an Invoice has a method to "MakePayment" which the the only way that the system allows a Payment entity to to be added to the invoice, refreshing the invoice's status, and then signalling to it's order if it's status has changed. Even then you will need to know what entity relationships a DDD operation (MakePayment) is going to need and either have those eager loaded or accept the cost of lazy loading them. Ultimately this is best done at the time you are making changes to a single aggregate root rather than polling through a set of aggregate roots.
If the system needs to accommodate other services etc. adding payment records to the database, then you need to consider an approach and a supporting data structure that can easily identify these new payment records, load them with their respective invoice & order they apply to, and proceed to use the same logic to refresh those statuses. This would involve looking at something like a CreatedAt DateTime and likely a indicator of some kind on the payment to mark rows that were inserted by a alternate system and need additional processing.
Upvotes: 1