Warn/throw when saving an optional dependent with all null properties when table splitting

[AndriySvyryd]

No description provided.

[umitkavala]

Hi @AndriySvyryd,
I'd like to take this issue.

[AndriySvyryd]

@umitkavala Ok

[umitkavala]

public class Order
{
    public int Id { get; set; }
    public OrderStatus? Status { get; set; }
    public DetailedOrder DetailedOrder { get; set; }
}

public class DetailedOrder
{
    public int Id { get; set; }
    public OrderStatus? Status { get; set; }
    public string? BillingAddress { get; set; }
    public string? ShippingAddress { get; set; }
}

modelBuilder.Entity<DetailedOrder>(
    dob =>
    {
        dob.ToTable("Orders");
        dob.Property(o => o.Status).HasColumnName("Status");
    });

modelBuilder.Entity<Order>(
    ob =>
    {
        ob.ToTable("Orders");
        ob.Property(o => o.Status).HasColumnName("Status");
        ob.HasOne(o => o.DetailedOrder).WithOne()
            .HasForeignKey<DetailedOrder>(o => o.Id);
    });


using (var context = new TableSplittingContext())
{
    context.Database.EnsureDeleted();
    context.Database.EnsureCreated();

    context.Add(
        new Order
        {
            Status = OrderStatus.Pending,
        });

    context.SaveChanges();
}

@AndriySvyryd Is this the use case? (From https://docs.microsoft.com/en-us/ef/core/modeling/table-splitting#optional-dependent-entity)
I'm trying to save principal entity without setting optional dependent entity?

[AndriySvyryd]

@umitkavala Not quite.

The use case is more like

using (var context = new TableSplittingContext())
{
    context.Database.EnsureDeleted();
    context.Database.EnsureCreated();

    context.Add(
        new Order
        {
            DetailedOrder = new DetailedOrder()
        });

    context.SaveChanges();
}

Then if you do

using (var context = new TableSplittingContext())
{
    var order = context.Set<Order>().Include(o => o.DetailedOrder).OrderBy(o => o.Id).First();

    // order.DetailedOrder is null even though it wasn't null when saved
}

[umitkavala]

@AndriySvyryd I'm just not sure where to check properties values. First I thought it will be at RelationalModelValidator but we need to check during save not model validation.

efcore/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs

Line 1316 in 3b38e3c

public virtual InternalEntityEntry PrepareToSave()

Is this the correct place to check null properties? Are we going to throw InvalidOperationException?

[AndriySvyryd]

@umitkavala No, you'd need to do it in

efcore/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs

Lines 161 to 164 in ac2bb48

	protected virtual IEnumerable<ModificationCommand> CreateModificationCommands(
	IList<IUpdateEntry> entries,
	IUpdateAdapter updateAdapter,
	Func<string> generateParameterName)

That's where we have enough information. Perhaps a helper method in SharedTableEntryMap would be needed to determine whether a given entry is an optional dependent.

Note that this isn't straightforward, so don't feel pressured to submit a PR.

[umitkavala]

When I try to save optional dependent with null properties;

 context.Add(
        new Order
        {
            DetailedOrder = new DetailedOrder()
        });

below warning thrown before code reach CreateModificationCommands

efcore/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs

Lines 330 to 358 in 6d4b03f

	var requiredNonSharedColumnFound = false;
	foreach (var property in entityType.GetProperties())
	{
	if (property.IsPrimaryKey()
	|| property.IsNullable)
	{
	continue;
	}
	var columnName = property.GetColumnName(table);
	if (columnName != null)
	{
	if (!principalColumns.Contains(columnName))
	{
	requiredNonSharedColumnFound = true;
	break;
	}
	}
	}
	if (!requiredNonSharedColumnFound)
	{
	if (entityType.GetReferencingForeignKeys().Select(e => e.DeclaringEntityType).Any(t => mappedTypes.Contains(t)))
	{
	throw new InvalidOperationException(
	RelationalStrings.OptionalDependentWithDependentWithoutIdentifyingProperty(entityType.DisplayName()));
	}
	logger.OptionalDependentWithoutIdentifyingPropertyWarning(entityType);
	}

[AndriySvyryd]

@umitkavala Yes, that's a warning logged at model validation. We need another warning logged when the user actually tries to insert an optional dependent with all null properties when table splitting. We don't want to throw an exception in this case as there might be working applications doing this.

[umitkavala]

@AndriySvyryd problem is WarningBehavior set to Throw for this message. So it throws an exception and execution stop there. I'll commit my test code as draft for this PR #24856.