kgrzybek / sample-dotnet-core-cqrs-api

Sample .NET Core REST API CQRS implementation with raw SQL and DDD using Clean Architecture.
https://www.kamilgrzybek.com/design/simple-cqrs-implementation-with-raw-sql-and-ddd/
MIT License
2.88k stars 646 forks source link

DDD EF Core Remove issue #4

Closed macgyver76 closed 5 years ago

macgyver76 commented 5 years ago

Let's say we have a classes:

public class Order 
{
    public int Id { get; set; }
    public List<OrderItem> OrderItems { get; set; }
    // other properties
}
public class OrderItem
{
    public int Id { get; set; }
    public int OrderId { get; set; }
    public Order Order { get; set; }
    // other properties
}

Order has primary key on Id property, OrderItem as well. OrderItem has foreign key to Order with OrderId property. Simple as that. Both are autoincremented.

Is there any way to remove OrderItem from Order with RemoveOrderItem defined method in Order class with using Entity Framework Core and not setting primary key on two fields (Id, OrderId) in OrderItem class (or any different key breaking changes workarounds)?

SO

kgrzybek commented 5 years ago

I don't have example in this repository but I checked other production codebase and there is no problem with this, maybe the reason is:

In this repository I have only "soft" deletes:

internal void Remove()
{
      this._isRemoved = true;
}

I prefer "soft" deletes rather than "hard" because it gives me some kind of history and trace about deleted entity and you can save deletion date if you want to. Of course with this approach you have to handle this flag for example in your queries to not return deleted entities.

macgyver76 commented 5 years ago

Removing property Order from OrderItem does not help.

I was assuming that you prefer "soft" deletes rather than "hard" due to similar issues with Entity Framework Core. In this specific (I would say common) situation - Order, OrderItem case - there is no simple solution - Google, SO says. Maybe I am wrong.

Could you provide an example of working DDD style hard deletes?

kgrzybek commented 5 years ago

I created example. As you see I even don't have OrderId in OrderItem class because I don't need it, I use EF Core shadow properties feature.

public class Order
    {
        public int Id { get; set; }

        private string _name;

        private List<OrderItem> _orderItems;

        private Order()
        {
            // For EF.
        }

        public void AddOrderItem(string name)
        {
            _orderItems = _orderItems ?? new List<OrderItem>();

            _orderItems.Add(new OrderItem(name));
        }

        public void RemoveOrderItem(int orderItemId)
        {
            var orderItem = _orderItems.Single(x => x.Id == orderItemId);

            _orderItems.Remove(orderItem); // hard delete
        }

    }

    public class OrderItem
    {
        public int Id { get; set; }

        private string _name;

        private OrderItem()
        {
            // For EF.
        }

        public OrderItem(string name)
        {
            _name = name;
        }
    }

    internal class OrderEntityTypeConfiguration : IEntityTypeConfiguration<Order>
    {
        public void Configure(EntityTypeBuilder<Order> builder)
        {
            builder.ToTable("Orders");

            builder.HasKey(b => b.Id);  

            builder.Property<string>("_name").HasColumnName("Name");

            builder.OwnsMany<OrderItem>("_orderItems", x =>
            {
                x.ToTable("OrderItems");          
                x.HasKey(p => p.Id);
                x.HasForeignKey("OrderId");

                x.Property<string>("_name").HasColumnName("Name");
            });
        }
    }

    public class OrdersContext : DbContext
    {
        public DbSet<Order> Orders { get; set; }

        public OrdersContext(DbContextOptions options) : base(options)
        {

        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.ApplyConfiguration(new OrderEntityTypeConfiguration());
        }
    }

    [Route("api/[controller]")]
    [ApiController]
    public class OrdersController : ControllerBase
    {
        private readonly OrdersContext _context;

        public OrdersController(OrdersContext context)
        {
            _context = context;
        }

        [Route("{orderId}/orderItems")]
        [HttpPost]
        public async Task<IActionResult> AddOrderItem(int orderId)
        {
            var order = _context.Orders.Single(x => x.Id == orderId);
            order.AddOrderItem("sampleName");

            await _context.SaveChangesAsync();

            return Ok();
        }

        [HttpDelete("{orderId}/orderItems/{orderItemId}")]
        public async Task<IActionResult> RemoveOrderItem(int orderId, int orderItemId)
        {
            var order = _context.Orders.Single(x => x.Id == orderId);

            order.RemoveOrderItem(orderItemId);

            await _context.SaveChangesAsync();

            return Ok();
        }
    }

Database:

CREATE TABLE dbo.Orders 
(
    Id INT NOT NULL IDENTITY(1, 1),
    [Name] NVARCHAR(200) NOT NULL,
    CONSTRAINT [PK_Orders_Id] PRIMARY KEY ([Id] ASC)
)
GO

CREATE TABLE dbo.OrderItems
(
    Id INT NOT NULL IDENTITY(1, 1),
    OrderId INT NOT NULL,
    [Name] NVARCHAR(200) NOT NULL,
    CONSTRAINT [PK_OrderItems_Id] PRIMARY KEY ([Id] ASC),
    CONSTRAINT [FK_OrderItems_Orders_OrderId] FOREIGN KEY ([OrderId]) REFERENCES dbo.Orders([Id]),
)
GO

INSERT INTO dbo.Orders VALUES ('SampleOrder')
GO

Requests: POST https://localhost:5001/api/orders/1/orderItems POST https://localhost:5001/api/orders/1/orderItems DELETE https://localhost:5001/api/orders/1/orderItems/2

macgyver76 commented 5 years ago

Thank you for example. I found root of the problem. It is foreign key restriction on deleting OrderItem. Code below:

internal class OrderEntityTypeConfiguration : IEntityTypeConfiguration<Order>
    {
        public void Configure(EntityTypeBuilder<Order> builder)
        {
            builder.ToTable("Orders");

            builder.HasKey(b => b.Id);

            builder.Property<string>("_name").HasColumnName("Name");

            builder.OwnsMany<OrderItem>("_orderItems", x =>
            {
                x.ToTable("OrderItems");
                x.HasKey(p => p.Id);
                x.HasForeignKey("OrderId").OnDelete(DeleteBehavior.Restrict);

                x.Property<string>("_name").HasColumnName("Name");
            });
        }
    }

This configuration gives me an exception: InvalidOperationException: The association between entity types 'Order' and 'OrderItem' has been severed but the relationship is either marked as 'Required' or is implicitly required because the foreign key is not nullable. If the dependent/child entity should be deleted when a required relationship is severed, then setup the relationship to use cascade deletes. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the key values.

What if want DeleteBehavior set to Restrict? In my situation it is global setting for each entity. What's your thougts / suggestions?

kgrzybek commented 5 years ago

I don't use any of "Cascade Delete" EF Core functionality because I treat it as anti-pattern.

From Domain-Driven Design perspective I think it is even more anti-pattern because AggregateRoot should be responsible for logic of all its children. Using EF Core to do that we are moving responsibility of deleting to infrastructure what is very bad idea.

I suggest you start to get rid of these OnDelete declarations one by one.

macgyver76 commented 5 years ago

I don't use any of "Cascade Delete" EF Core functionality because I treat it as anti-pattern.

I agree. Hovewer OrderEntityTypeConfiguration class you provide sets delete rule to cascade when using migrations.

By the way. Here is how I set DeleteBehavior to Restrict globally to avoid cascade.

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    foreach (var relationship in modelBuilder.Model.GetEntityTypes().SelectMany(e => e.GetForeignKeys()))
    {
        relationship.DeleteBehavior = DeleteBehavior.Restrict;
    }
}
kgrzybek commented 5 years ago

Hovewer OrderEntityTypeConfiguration class you provide sets delete rule to cascade when using migrations.

I don't use EF migrations so this something new to me I need to say. I don't want to have auto generated SQL migration scripts and this behavior which you described is very good proof to not to do it. I prefer to have full control about my migration scripts so I use database project and DbUp library. Here you can read about my approach if you like - http://www.kamilgrzybek.com/database/using-database-project-and-dbup-for-database-management/

macgyver76 commented 5 years ago

I came up with workaround. If your existing relationship do not use cascade - solution is to set DeleteBehavior to Cascade and then use Add-Migration command to generate migration files and update snapshot model but before running Update-Database you need to commend out Up method in migration file in order to not set cascade to database. Indeed, we have cascade in EF but not in database.

@kgrzybek - thanks for your commitment and I am looking forward to new DDD stuff in this repo.