Further relax value-generation restrictions on composite derived FKs

[paultobey]

I thought I'd posted this some time ago, but can't find that issue.

I have an EF Core 2.1 preview application which uses three entities. Each entity has a multiple-component primary key, an auto-generated ID and a "CompanyId". One of these entities references another, optionally. The code is roughly like this (I've changed some names because we are very sensitive about sharing code):

Class declarations:

	public abstract partial class FE
	{
		public FE()
		{
		}

		public int Id { get; set; }

		public int CompanyId { get; set; }

		public string Name { get; set; }
	}

	public partial class FB : FE
	{
		public FB()
		{
		}

		public int? TSId { get; set; } = null;

		public virtual S TS { get; set; }
	}

	public partial class S
	{
		public S()
		{
		}

		public int Id { get; set; }

		public int CompanyId { get; set; }

		// Other properties of various types here.
	}

Entity descriptions:

	modelBuilder.Entity<FE>(entity =>
	{
		entity.HasKey(e => new
		{
			e.Id,
			e.CompanyId
		})
				 .HasName("PK_FE");

		entity.Property(e => e.Id)
				 .HasColumnName("ID")
				 .ValueGeneratedOnAdd();

		entity.Property(e => e.CompanyId)
				 .HasColumnName("CompanyID")
				 .HasDefaultValueSql("1");

		entity.Property(e => e.Name)
				 .IsRequired()
				 .HasMaxLength(256);

	});

	// FB is a subclass, in .NET, of FE, so inherits its primary key setup. We are using table-per-hierarchy, 
	// so a Discriminator column is used to identify the type.
	modelBuilder.Entity<FB>(entity =>
	{
		// Here is the reference of interest. Note that it's optional, int?
		entity.Property<int?>("TSId")
				 .HasColumnName("TSId");

		// And the foreign key description. Note that the foreign key uses the same CompanyId
		// value as the FB object. That is, we want this foreign key to refer to "S's" which have the
		// same CompanyId as the FB's which refer to them.
		entity.HasOne(mv => mv.TS)
				 .WithMany()
				 .HasForeignKey(mv => new
				 {
					 mv.TSId,
					 mv.CompanyId
				 })
				 .OnDelete(DeleteBehavior.ClientSetNull);
	});

	modelBuilder.Entity<S>(entity =>
	{
		entity.HasKey(e => new
		{
			e.Id,
			e.CompanyId
		})
				.HasName("PK_S");

		entity.Property(e => e.Id)
				.HasColumnName("ID")
				.ValueGeneratedOnAdd();

		entity.Property(e => e.CompanyId)
				.HasColumnName("CompanyID")
				.HasDefaultValueSql("1");

		//	Other properties of various types here.
	});

The issue occurs when a referenced S is deleted from the S table. We now have a dead reference from one or more FB's which we want to null. Before the delete occurs, I'm assuring that the FB list is loaded into memory for EF Core to use. It seems to be finding the dead reference and trying to null it, but:

System.InvalidOperationException : The property 'CompanyId' on entity type 'FB' is defined to be read-only after it has been saved, but its value has been modified or marked as modified.

So, it might be successfully clearing the SId reference, but, because CompanyId is a portion of the primary key of the FB object, it can't null it (and, of course, I don't want it to). What I hope is for EF Core to null the nullable part of the foreign key and ignore the non-nullable part.

Steps to reproduce

Further technical details

EF Core version: 2.1.0-preview2-final
Database Provider: SQL Server 2016
Operating system: Windows 7/64
IDE: Visual Studio 2017 15.7

[paultobey]

Note also that I can write pre-S-delete code enumerating FB's referencing the to-be-deleted-S and setting each TS navigation property to null. I was expecting the same invalid operation exception for that case, but it seems that explicitly nulling it and nulling it as part of ClientSetNull don't work the same.

[ajcvickers]

Notes for triage:

• I was able to repro this; full code below.
• In 2.0, the model does not build--gives error "The property 'CompanyId' cannot be part of a foreign key on 'FB' because it has value generation enabled and is contained in the key {'Id', 'CompanyId'} defined on a base entity type 'FE'.

public abstract class FE
{
    public int Id { get; set; }
    public int CompanyId { get; set; }
    public string Name { get; set; }
}

public class FB : FE
{
    public int? TSId { get; set; } = null;
    public virtual S TS { get; set; }
}

public class S
{
    public int Id { get; set; }
    public int CompanyId { get; set; }
}

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

            var fb = context.Add(new FB
            {
                Name = "Wubbsy, Inc",
                TS = new S()
            }).Entity;

            context.SaveChanges();
        }

        using (var context = new BloggingContext())
        {
            var fb = context.Set<FB>().Single();
            var s = context.Set<S>().Single();

            Console.WriteLine($"FB.Id = {fb.Id}; FB.CompanyId = {fb.CompanyId}");
            Console.WriteLine($"S.Id = {s.Id}; S.CompanyId = {s.CompanyId}");
            Console.WriteLine($"FB.TS.Id = {fb.TS.Id}; FB.TS.CompanyId = {fb.TS.CompanyId}");

            context.Remove(s);

            context.SaveChanges();
        }
    }
}

public class BloggingContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=Test;ConnectRetryCount=0");

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<FE>(entity =>
        {
            entity.HasKey(e => new
                {
                    e.Id,
                    e.CompanyId
                })
                .HasName("PK_FE");

            entity.Property(e => e.Id)
                .HasColumnName("ID")
                .ValueGeneratedOnAdd();

            entity.Property(e => e.CompanyId)
                .HasColumnName("CompanyID")
                .HasDefaultValueSql("1");

            entity.Property(e => e.Name)
                .IsRequired()
                .HasMaxLength(256);

        });

        // FB is a subclass, in .NET, of FE, so inherits its primary key setup. We are using table-per-hierarchy, 
        // so a Discriminator column is used to identify the type.
        modelBuilder.Entity<FB>(entity =>
        {
            // Here is the reference of interest. Note that it's optional, int?
            entity.Property<int?>("TSId")
                .HasColumnName("TSId");

            // And the foreign key description. Note that the foreign key uses the same CompanyId
            // value as the FB object. That is, we want this foreign key to refer to "S's" which have the
            // same CompanyId as the FB's which refer to them.
            entity.HasOne(mv => mv.TS)
                .WithMany()
                .HasForeignKey(mv => new
                {
                    mv.TSId,
                    mv.CompanyId
                })
                .OnDelete(DeleteBehavior.ClientSetNull);
        });

        modelBuilder.Entity<S>(entity =>
        {
            entity.HasKey(e => new
                {
                    e.Id,
                    e.CompanyId
                })
                .HasName("PK_S");

            entity.Property(e => e.Id)
                .HasColumnName("ID")
                .ValueGeneratedOnAdd();

            entity.Property(e => e.CompanyId)
                .HasColumnName("CompanyID")
                .HasDefaultValueSql("1");

            //	Other properties of various types here.
        });
    }
}

[wmkew]

I just downloaded the 2.1.0 bits, and I am now encountering the same error that @ajcvickers mentioned that was yielded by 2.0: "The property 'CompanyId' cannot be part of a foreign key on 'FB' because it has value generation enabled and is contained in the key {'Id', 'CompanyId'} defined on a base entity type 'FE'." Did I miss something?

[ajcvickers]

@wmkew Two questions:

• What version of EF Core were you using before upgrading to 2.1?
• Do you feel that the exception message is incorrect? If so, in what way?

[wmkew]

@ajcvickers I am currently on EF Core version 2.1.0-preview2-final, and I don't get this error when adding migrations. As soon as I upgraded to 2.1.0, I started seeing this error, even though my models haven't changed. I have a class design similar to the one above; it seems that this should be a valid design, so I'm wondering why I'm even getting an error. If this is indeed a bug, is there a workaround?

[paultobey]

This does seem like a back-step and locks me into EF Core 2.1 preview-2 final, also, I think.

I'm using table-per-hierarchy; I would expect the table structure to be derived from a flattened version of the class hierarchy.

There's only one CompanyId in the FB hierarchy. It's public. I don't see any reason why it can't be both a part of the primary key for FB and part of a foreign key from FB to S.

[ajcvickers]

@paultobey @wmkew What the error is saying is that it doesn't make sense to have value generation for a foreign key property because valid values for that property are defined by the property at the principal end of the relationship. Therefore, any generated values will be wrong as they will not, other than accidentally, conform to the FK constraint.

[AndriySvyryd]

@paultobey @wmkew Before 2.1.0 this wasn't detected consistently. You can workaround this by ignoring the base type or be removing the default value from CompanyId.

We could further relax the restriction by moving this check to key generation and only throwing if no value was provided, but then specifying a default value would be pointless anyway as it would never be used.

[paultobey]

@AndriySvyryd Thanks. So it sounds like you're saying that the default value 'attribute' is causing the message and, if we simply did not specify any automatic value generation all would be well. Am I getting that right?

[AndriySvyryd]

@paultobey Correct

[paultobey]

@ajcvickers OK, I think I can live with that.

[wmkew]

I can confirm that once I remove HasDefaultValueSql from CompanyId in the base classes, I can add migrations now. @ajcvickers I actually was thrown by the exception message, specifically by the phrase "value generation enabled". When I saw that, it made me think of ValueGeneratedOnAdd, which is on Id, but not CompanyId, so initially I was thinking, "wait a minute, CompanyId doesn't have value generation." It didn't occur to me that HasDefaultValueSql also falls into the "value generation" category, so I didn't think to remove it.

[ajcvickers]

@wmkew I was perhaps too hasty in my comments yesterday. I agree the message is confusing when HasDefaultValueSql is used--I don't think we were considering this case when it was written. Also, when the default is a single, fixed value, it may be that we should allow this. I'll chat with @AndriySvyryd and @divega about it.

[divega]

Temporarily clearing milestone so that we remember talking about #11939 (comment) in triage.

[paultobey]

👍