Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notifying EF that child entity should not be added #13974

Closed
skyflyer opened this issue Nov 16, 2018 · 5 comments
Closed

Notifying EF that child entity should not be added #13974

skyflyer opened this issue Nov 16, 2018 · 5 comments

Comments

@skyflyer
Copy link

A lot of related issues: #4424, #6907, #9185, #10644

I am using an "enumeration" pattern for static values, which are also mapped to DB. An example of such entity is OrderStatus.

You can see how it is mapped to DB here.

Normally, you would seed the database with:

foreach(var status in OrderStatus.List()) {
  if (!db.OrderStatuses.Any(x => x.Id == status.Id)) {
    db.OrderStatuses.Add(status);
  }
}

and that works fine. But when you're modifying the order and want to set a status:

public class Order {
public void Ship() {
  // ...
  Status = OrderStatus.Shipped;
}
}

and you save the order (db.Orders.Add(order) for new orders or db.SaveChangesAsync for tracked entities) EF would want to save the disconnected entity OrderStatus as well.

Since it is better to not mix the DB logic with domain logic, I would not want to expose db context to order and I'd also not want to use OrderStatusId to set the order status, as that implies "problems" with the DB layer. If possible, I'd like to avoid explicit OrderStatusId property on the entity itself (though shadow property is obviously fine and required).

I was trying to figure out how to detect a situation where OrderStatus is added explicitly through it's DbSet<OrderStatus> or when it is implicitly added via change detector. I could not find a way, so please point me in the right direction, if it exists.

I came up with this thus far though, and I'd like your opinion (the obvious drawback is that extra queries are executed against the DB every time this enumeration is assigned to a property of an entity:

        public override Task<int> SaveChangesAsync(CancellationToken cancellationToken = default(CancellationToken))
        {
            foreach(var e in ChangeTracker.Entries()) {
                if (e.Entity is Enumeration) {
                    var dbvalues = e.GetDatabaseValues();
                    if (dbvalues != null) {
                        e.State = EntityState.Unchanged;
                    }
                }
            }

            return base.SaveChangesAsync(cancellationToken);
        }

I'm checking e.GetDatabaseValues() for a changed entity if it is derived from Enumeration and resetting the EntityState in case it is present in the DB already. This works in practice, as far as I have tested.

Let me know if you need a full repro, I'll provide one.

Is there a better way to handle this scenario? Something like

modelBuilder.Entity<Order>(conf =>
  conf.Property(x => x.Status).DoNotTrack();
});

perhaps? :) I would want the shadow OrderStatusId to be set, of course.

@ajcvickers
Copy link
Member

@skyflyer This looks like a request for read-only entities to act as always-existing reference data--this is being tracked by issue #7586. A different way to work around this is to explicitly attach this reference data whenever a context instance is created. This means that instances of the enumeration will always been in the Unchanged state. This can be combined with EF Core data seeding to see the database with the known values in migrations/EnsureCreated. For example, using your OrderStatus class:

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

public class BloggingContext : DbContext
{
    public BloggingContext()
    {
        AttachRange(OrderStatus.List());
    }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder
            .UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=Test;ConnectRetryCount=0");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Order>();
        modelBuilder.Entity<OrderStatus>(
            b =>
            {
                b.HasKey(o => o.Id);

                b.Property(o => o.Id)
                    .HasDefaultValue(1)
                    .ValueGeneratedNever()
                    .IsRequired();

                b.Property(o => o.Name)
                    .HasMaxLength(200)
                    .IsRequired();

                b.HasData(OrderStatus.List().ToArray());
            });
    }
}

public class Program
{
    public static void Main()
    {
        using (var context = new BloggingContext())
        {
            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();
        }

        using (var context = new BloggingContext())
        {
            context.Add(new Order {OrderStatus = OrderStatus.Submitted});

            context.SaveChanges();
        }

        using (var context = new BloggingContext())
        {
            var order = context.Set<Order>().FirstOrDefault();

            order.OrderStatus = OrderStatus.Cancelled;

            context.SaveChanges();
        }
    }
}

@skyflyer
Copy link
Author

I'm already using seed, so this makes perfect sense. The approach with annotations (outlined in #7586) is also interesting and I expect it to be much more preformant
The reason I implemented checks is because even though I'm using custom seed logic, SaveChanges gets called. That's why I had to check the DB. I will test the suggested approach (also if HasData() avoids SaveChanges logic). Thanks a lot!

@skyflyer
Copy link
Author

skyflyer commented Nov 24, 2018

@ajcvickers, this approach works very well, thank you. I noticed that the HasData is only run when the table is created and not checked subsequently - I suppose that's by design for performance reasons in order to avoid checking the DB on every DbContext construction. All I had to do was implement my own "seeding" strategy for scenarios where enums are being added (I did notice that if the new enum is referenced by an entity (say an order in the above example), then it is implicitly created in the OrderStatuses table).

Upon program startup, where I run my seed logic, I added:

var dbStatuses = await db.OrderStatuses.ToListAsync();
var missingStatuses =  OrderStatus.List().Except(dbStatuses);
db.OrderStatuses.AddRange(missingStatuses);

and that works very nicely. Thanks again!

@ajcvickers
Copy link
Member

@skyflyer HasData should take care of that for you when you add a new value to the Enumeration. (Specifically, when creating the migration, EF should see the new value in the seed data compared to the last time a database update was made and should scaffold an appropriate insert command. Likewise, if a value is removed from the Enumeration, then the migration will contain the code to delete it.

@skyflyer
Copy link
Author

Oh, that's very nice. I was under the impression it was only created the first time around, but that's because I wasn't creating migrations when changing it. Wonderful! Thanks again for the tips to work around it! I can remove the seeds now.

@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants