Support cascade updates for owned types/aggregates

[nickverschueren]

When I attach an entity that has an owned type attached to it and I change the state of that entity using the ChangeTracker to EntityState.Added, the owned type should also get the EntityState.Added state, but it doesn't.

Exception message: The entity of 'Entity' is sharing the table 'Entity' with 'Entity.OwnedType#OwnedType', but there is no entity of this type with the same key value that has been marked as 'Added'. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the key values.'
Stack trace:    at Microsoft.EntityFrameworkCore.Update.Internal.ModificationCommandIdentityMap.Validate(Boolean sensitiveLoggingEnabled)
   at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.CreateModificationCommands(IReadOnlyList`1 entries, Func`1 generateParameterName)
   at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.<BatchCommands>d__8.MoveNext()
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(Tuple`2 parameters)
   at Microsoft.EntityFrameworkCore.Storage.Internal.SqlServerExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
   at Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.Execute[TState,TResult](IExecutionStrategy strategy, TState state, Func`2 operation)
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(IEnumerable`1 commandBatches, IRelationalConnection connection)
   at Microsoft.EntityFrameworkCore.Storage.RelationalDatabase.SaveChanges(IReadOnlyList`1 entries)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(IReadOnlyList`1 entriesToSave)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges()
   at EfCoreState.Program.Main(String[] args) in c:\users\nickv\source\repos\projects\EfCoreState\EfCoreState\Program.cs:line 19

Steps to reproduce

    class Program
    {
        static void Main(string[] args)
        {
            var entity = new Entity();
            entity.OwnedType.SomeText = "Hello world!";

            var context = new TestContext();
            context.Database.EnsureCreated();

            context.Attach(entity);
            context.ChangeTracker.Entries<Entity>().First().State = EntityState.Added;
            context.SaveChanges();

            context.Database.EnsureDeleted();
        }
    }


    public class Entity
    {
        public Entity()
        {
            Id = Guid.NewGuid();
            OwnedType = new OwnedType();
        }

        public Guid Id { get; set; }

        public OwnedType OwnedType { get; set; }
    }

    public class OwnedType
    {
        public string SomeText { get; set; }
    }

    public class TestContext : DbContext
    {
        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlServer("Server=(localdb)\\mssqllocaldb;Database=Test;Trusted_Connection=True;MultipleActiveResultSets=true");

            base.OnConfiguring(optionsBuilder);
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            var entity = modelBuilder.Entity<Entity>();
            entity.HasKey(e => e.Id);
            entity.OwnsOne(e => e.OwnedType);

            base.OnModelCreating(modelBuilder);
        }
    }

Further technical details

EF Core version: 2.0.1
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10 (1703)
IDE: Visual Studio 2017 15.5.1

[ajcvickers]

@nickverschueren We will discuss this, but a quick question: why does the code call Attach first and then change the state instead of calling Add?

[nickverschueren]

@ajcvickers This is just to illustrate the problem. We actually use our own self-tracking entities.

[ajcvickers]

@nickverschueren I am re-purposing this issue into a feature for cascade updates that would behave similar to cascade deletes in that it would allow the states of owned child entites to update based on the state of the parent in a similar way to cascade delete does now.

[FrederickBrier]

Wasn't this feature committed back in October? The discussion below seemed to indicated you had decided Owned types do follow the state of their owning entity.

#7985

How does the owned state follow? Does it follow if you make changes via the TrackGraph? Or just via a context Add(). BTW, I have been fighting Nick's error all day. I can't add an entity with an owned type. Is it me, or is the feature not quite fully working?

[ajcvickers]

@FrederickBrier This issue is about cascade updates in the database. EF Core does not support this yet, which is why this issue is in the Backlog milestone. In general, as I said in another comment, without more details (i.e. code to reproduce what you are seeing) it is very hard to know exactly what you are asking and whether or not you are hitting a bug.

[FrederickBrier]

@ajcvickers I will put together an example, but it will probably be next week. For the time being, added the owned type's members to the owning entity, plus the owned type, tagged as NotMapped, with get/set members to write to individual persisted properties. Not great, but the tests pass, the database schema is the same, and we will figure out :). Oh, and thank you for the help. All your discussions on these issues is very helpful.

[javiercampos]

I'd +1 this but not specifically for owned types, but for one-to-one relationships. Having an UPDATE CASCADE constraint on the FK would be helpful, specially if you use direct SQL queries (you have to update the FK on the other table manually). Not sure if EF does update the FK (it probably does) but it definitely doesn't add a UPDATE CASCADE constraint.

Thing is, this seems to be supported on Migrations... I can manually add it on the ForeignKey method:

constraints: table =>
{
    table.PrimaryKey("PK_Products", x => x.Id);
    table.ForeignKey(
        name: "FK_Products_EanUpcs_EanUpcId",
        column: x => x.EanUpcId,
        principalSchema: "om",
        principalTable: "EanUpcs",
        principalColumn: "Id",
        onDelete: ReferentialAction.Restrict,
	onUpdate: ReferentialAction.Cascade // <-- i can add this manually
    );
});

But there doesn't seem to be a fluent API to configure it

[reloaded]

+1

[SilverioMiranda]

5 years in backlog

[smitpatel]

Still 6m11d to go in 5th birthday.

[ru-mertsaloff]

and another one year without this feature

[reloaded]

Perhaps by the time we inhabit Mars, the EF Core team will address this issue.

[roji]

Everyone, this issue has 22 votes in total - since 2017 - so there has been very little user interest in it. I understand that's no comfort if this is affecting you, but we prioritize our work based on how many users would be affected by it (as well as other factors).

[ru-mertsaloff]

@roji

I understand your priorities perfectly and have nothing against it. It just seems strange to me that the design of the same cascade deletion (or another type of it) is provided for configuration in ModelBuilder, but not for updating. The vendor itself supports this function when deploying a new database, why not here too? It just looks kind of incomplete, that the deletion setting is possible, but the update is not. And you have to manage to configure this with a native SQL request to change the update limit. I kindly ask you to introduce this feature without looking to the end.

P.S.

It seems to me that you could have added such an obvious thing yourself without discussing the problem of this topic, it just looks absurd that “there is one without the other”

[piskov]

@roji

Everyone, this issue has 22 votes in total - since 2017

I would vote if I knew how and where :-) Or are you talking about thumbs-up under the comment? Never have I ever thought about it as a voting tool. Also @ajcvickers changed the issue in mid-air with “re-purposing this issue” so what comment on this thread should one 👍 to vote? :-)

—

Regardless of the silly talk above: I have just a real-world use-case for this feature. We’re migrating our dozen-years-old DBs from MS SQL Server to PostgreSQL.

Please bear with me (this is a story not just about cascade updates).

We’ve always used DB-first approach with carefully constructed DBs (default values, cacade updates, clustered indices, etc.).

Now, it was an obvious choice to use EF to help us with migration to postgres: generate creation scripts for databases.

First, we encountered that DbContext.Database.GenerateScripts() doesn’t actually generate a DB creation script (only table creation scripts). Well, that’s a bummer. I understand reasoning like “we don’t know permission”, yet any db creation placeholder is probably better than non at all. Than again, how does EnsureCreated — many use for tests — works: it fails if there’s no DB? Ok, note to self to deal with the actual db creation later.

Second, we see stuff like bit not null default(0)in DB translates to bool without default value in EF. And after GenerateScripts it translates back to just bit not null in table creation scripts. There, we’ve just lost the default(0) which was originally present in the MS SQL. And this would break some sql scripts relying on that default value. Also finding about out this “vanished” kind of things is a heck of a pain in the ass. Just try to compare EF script with SQL management studio generated scripts (everything is out of order, EF doesn’t have square brackets, etc.).

Third, we see no cascade updates in EF scripts. And no option to configure them. Yes, we would never-ever try to update those IDs via code. But that IDs are string IDs (something like, “QuestionnaireId” in our case, and we may change one of those IDs once in while in a couple of years when ID itself may become not good — descriptive, specific — enough). Or may be you need some two duplicate entries merged when instead of changing a lot of foreign keys, you just update a primary key on the top level and delete the other “duplicate”.

I mean, I dunno, there wouldn’t be a cascade update option in SQL it it wasn’t useful sometimes :-). If we were not to have cascade updates set in DB, we would need to manually update entries on dozen of other tables.

So there we a have it: also a bummer (which is more tedious to fix as we need to add that cascade update to dozens of tables manually later). List could go on and on (dbo schema is not picked up while generating classes from DB, so then EF generated table names in scripts would miss it; indices are not marked as clustered, etc., etc.).

—

I completely understand that not a lot of people migrate between database engines (like I’ve said, we were running perfectly fine since 2011 with MS SQL, yet now we need Postgres). And that EF is not s LINQ-to-SQL (which, if I remember correctly, was solely focused on MS SQL), hence you need to keep in mind dozens of others.

So I totally understand this may not be a priority, and appreciate your focus on data-centric stuff, optimizing the heck out of queries, etc. For example, that new contains / in with json doc, so we can now literally have thousands of in items, is just a chef's kiss.

Yet, may be a lot more people would love to drop and recreate DBs (for tests maybe? and so that it would replicate the production DB as much as possible). So that EF is not used only as DB-first but code-first approach as well (and not just for adding a couple of new properties to the existing table). So supporting things like cascade updates would be nice. Just for DB-sake side of things as not everything is done just from C# code.

Also thanks about any other DB-first improvements like PrimaryKey with multiple fields data annotations (same for ForeginKey wuold be nice) — you work is truly doesn’t go unnoticed on that field.

—

P. S. While I have your attention, may a have a little off-topic question please? I’ve customized some stuff in the T4 creation scripts to have required inserted, regions, and lazy init for collection navigations. Do you think it’s worth it to try to avoid allocations like that or is it just a wasted time as EF may be still create some empty lists under the hood?

[PrimaryKey(nameof(NameId), nameof(QuestionnaireId), nameof(LocaleId))]
[Table("QuestionnaireNorm")]
public sealed class QuestionnaireNorm
{
    // ** regions, nullable backing fields are all created automatically via t4 **
    #region Fields
    private ICollection<LocalizedNormDescription>? _localizedNormDescriptions;  // **nullable**
    #endregion

    #region DB properties
    [StringLength(maximumLength: 100)]
    public required string NameId { get; set; } // **required is inserted automatically as well if not null in db**
    #endregion

    #region Navigation properties
    [ForeignKey(nameof(LocaleId))]
    public Locale Locale { get; set; } = null!; // sic `null!` per your docs for navigation properties

    // ** does this reduce allocations? **
    public ICollection<LocalizedNormDescription> LocalizedNormDescriptions =>
        _localizedNormDescriptions ??= new List<LocalizedNormDescription>();
    #endregion
}

[roji]

I would vote if I knew how and where :-) Or are you talking about thumbs-up under the comment?

Yes - thumbs-up on the top-most post. Github allows sorting by votes, and we use that list when we plan for features.

The rest is mostly off-topic to this issue, but here are some answers anyway.

First, we encountered that DbContext.Database.GenerateScripts() doesn’t actually generate a DB creation script (only table creation scripts). Well, that’s a bummer. I understand reasoning like “we don’t know permission”, yet any db creation placeholder is probably better than non at all. Than again, how does EnsureCreated — many use for tests — works: it fails if there’s no DB? Ok, note to self to deal with the actual db creation later.

That's simply how PostgreSQL works. In PG, you're always connected to a specific database, and cannot change the database you're connected to once you're connected (unlike in e.g. SQL Server). This means that a script cannot both create a database and create tables into it. EnsureCreated isn't a SQL script - it's a programming API; so it can create your database, with subsequent EF invocations connecting to the database which was created.

Second, we see stuff like bit not null default(0)in DB translates to bool without default value in EF. And after GenerateScripts it translates back to just bit not null in table creation scripts. There, we’ve just lost the default(0) which was originally present in the MS SQL.

SQL Server also does not add a default when creating a table with a new bool column; try it, and you'll see that it created bit NOT NULL. The default you have is very likely because you added a non-nullable bool column to an existing table at some point; since a non-nullable can't be added without a default to an existing table (there are existing rows), EF adds the default. Since for PG you're creating a single new migration for your schema, there's no addition of a column to an existing table - just the creation of the table.

Third, we see no cascade updates in EF scripts. And no option to configure them.

Yes, this is what this issue tracks.

I mean, I dunno, there wouldn’t be a cascade update option in SQL it it wasn’t useful sometimes

Nobody said it wasn't useful - in fact, that's why this issue is open and in the backlog (otherwise we'd have closed it).

List could go on and on (dbo schema is not picked up while generating classes from DB, so then EF generated table names in scripts would miss it; indices are not marked as clustered, etc., etc.).

The dbo schema (which is the SQL Server default) is - by design - not included in the model. I'm not sure what you're referring to with indices not being marked as clustered; I just tested this and EF definitely marks clustered indexes as clustered when reverse-engineering an existing database. Maybe you're referring to the fact that primary keys aren't explicitly marked as clustered; that's because they're clustered by default and there's no need to do that.

While I have your attention, may a have a little off-topic question please? I’ve customized some stuff in the T4 creation scripts to have required inserted, regions, and lazy init for collection navigations. Do you think it’s worth it to try to avoid allocations like that or is it just a wasted time as EF may be still create some empty lists under the hood?

Have you done a benchmark to measure what the impact of this is on your final, end-to-end application performance? I'd be very surprised if you saw one, making this a case of premature optimization, adding complexity/brittleness for no reason. If you do see a benefit to your application, I'd be interested in hearing about it.

To summarize, most of the complaints above don't seem to be accurate... If you do feel that EF should be behaving differently in one or more of these cases, please open a separate issue - one for each problem - rather than continuing to post here.

[piskov]

@roji thank you for your answers, much appreciated.

Nobody said it wasn't useful - in fact, that's why this issue is open and in the backlog

My “rant” was an aggregate of my attempts to solve this issue and reading many other pages on the same issue with some comments like “why would you ever want that”. So that was just me giving it some context, when this kind of things are useful.

—

The stuff below is off-topic (just to answer some things you’ve said; I will later search for existing issues, or create new ones, otherwise).

This means that a script cannot both create a database and create tables into it.

That’s what was surprising as EF can create databases in the end (“programmatical API” or not) and SQL Management Studio generates one single script for everything.

SQL Server also does not add a default when creating a table with a new bool column

All stuff in our DB is created manually via SQL (not through EF). So we had that bool property with explicit default value set. And for some reason EF doesn’t pick it up when generating C# classes from the DB = it doesn’t generate .HasDefaultValue('0') when it’s explicitly present in the DB. Which in turn results in missing this default in sql scripts with reverse GenerateScripts() operation.

adding complexity/brittleness for no reason

It’s a T4 and generated code after all (we don’t use EF classes as some kind of DTOs or domain objects — we have separate classes for that). So I would think about it just as free reduced allocations, unless EF always sets the backing fields to empty lists under-the-hood even if navigations are not included in the query (which I don’t know and hence was the question). Premature or not, that’s the general theme in .NET these days, no? (ReadOnlyCollection.Empty, ReadOnlyDictionary.Empty, cached struct enumerators for empty collections for foreach, etc.).

[ajcvickers]

@piskov Please keep in mind that both reverse engineering from an existing database to an EF model and creating a database from an EF model are lossy operations. This is because the database can have configuration that EF can't represent, and the EF model can have configuration that the database can't represent. This means that reverse engineering an existing database and then using that model to create a new database will not result in an identical database. This is true even when the database provider and the underlying database system do not change. When moving from one database system to another, the scaffolded model is, at best, a starting point if you want to start using migrations going forward.