Initial change tracking and DetectChanges for complex types #31453

ajcvickers · 2023-08-12T12:59:48Z

Part of #9906

ajcvickers · 2023-08-12T13:02:13Z

@roji There should be enough here for query. Just call IStateManager.StartTrackingFromQuery with the fully materialized entity instance.

roji · 2023-08-12T13:29:30Z

Thanks, I'll experiment with this (and review) tomorrow. I think I have pretty much everything aside from change tracking working at this point.

Part of #9906

roji · 2023-08-13T08:01:58Z

Am integrating this into my query test work, and it seems to be working well!

Just call IStateManager.StartTrackingFromQuery with the fully materialized entity instance

Just to verify, since we don't support tracking complex types on their own (without their containing entity type), no actual materializer change should be necessary here (for change tracking), right? I mean, we already call StartTrackingFromQuery for entity types etc.

roji · 2023-08-13T08:54:50Z

src/EFCore/Metadata/Internal/ClrAccessorFactory.cs

@@ -44,7 +44,7 @@ protected virtual TAccessor Create(MemberInfo memberInfo, IPropertyBase? propert
    {
        var boundMethod = propertyBase != null
            ? GenericCreate.MakeGenericMethod(
-                propertyBase.DeclaringType.ClrType,
+                propertyBase.DeclaringType.ContainingEntityType.ClrType,


FYI this caused some passing tests in my query PR to fail after rebasing on top of this. This seems to mean that the property accessor for a nested property on a complex type requires its top-level containing entity instance rather than its direct container (which is possibly a complex type).

I don't think this will work for query, since there are situations where we have the containing instance (complex type) and we just need to get the property off it. For example:

public virtual Task Contains_over_complex_type(bool async) { var address = new Address { AddressLine1 = "804 S. Lakeshore Road", ZipCode = 38654, Country = new Country { FullName = "United States", Code = "US" } }; return AssertQuery( async, ss => ss.Set<Customer>().Where( c => ss.Set<Customer>().Select(c => c.ShippingAddress).Contains(address))); }

Here a parameterized Address is provided, and we need to get its properties to send as individual parameters to the database; there's no Customer at all...

Where in the code does this happen?

Here.

In the PR that adds complex type querying, this needs to be able to receive a parameter value representing a (possibly nested) complex type, and extract a property value from it.

@roji Pushed an update with GetStructuralTypeClrValue that should work as you need it.

Thanks - I can see the tests passing with this.

We should consider doing it the other way around though - the new code calls GetStructuralTypeClrValue even on an entity type, since it doesn't necessarily know/care whether the property is on an entity or complex type; it feels like the default GetClrValue should just accept an instance of the property's declaring CLR type.

But not a blocker, we can discuss in design later and adjust.

Except GetClrValue accepts a parameter called entity and it is constrained to be a reference type. (Although I think we violate this in some of our code gen.)

Except GetClrValue accepts a parameter called entity and it is constrained to be a reference type. (Although I think we violate this in some of our code gen.)

Do you see a specific problem with removing that constraint (or changing the parameter name)? Currently it seems like we're calling GetStructuralTypeClrValue in all kinds of places where the entity vs. complex type distinction doesn't/shouldn't matter...

Yes. That's when it is called. When it can be any "structural type", as opposed to being an entity type. So one method is "Get the property value given its containing entity instance" and the other is, "Get the property value given an instance of the type on which the property is defined."

OK.

This is purely a naming thing, so we can discuss later in API review. But I'd expect the simple GetClrValue to simply behave like GetStructuralTypeClrValue, and have a specially-named GetClrValueFromContainingEntity for those cases where you want it to do the (considerably more complex) thing of traversing into an arbitrarily deep graph from the containing entity all the way to the leaf property. In any case, there's nothing in the GetClrValue name that suggests it requires the containing entity type etc.

I agree that would be best if we started from scratch. Just trying to avoid a breaking change here if we don't need it.

ajcvickers · 2023-08-13T09:33:34Z

Just to verify, since we don't support tracking complex types on their own (without their containing entity type), no actual materializer change should be necessary here (for change tracking), right? I mean, we already call StartTrackingFromQuery for entity types etc.

Correct.

roji · 2023-08-13T14:15:47Z

@ajcvickers this seems to fail with a complex property on a derived type (any inheritance strategy):

await using var ctx = new BlogContext();
await ctx.Database.EnsureDeletedAsync();
await ctx.Database.EnsureCreatedAsync();

ctx.Blogs.Add(new Blog { Name = "foo" });
ctx.SpecialBlogs.Add(new SpecialBlog { Name = "bar" });
await ctx.SaveChangesAsync();

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }
    public DbSet<SpecialBlog> SpecialBlogs { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer(@"Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false")
            //.UseSqlite("Filename=:memory:")
            // .UseNpgsql(@"Host=localhost;Username=test;Password=test")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<SpecialBlog>().ComplexProperty(b => b.Details);
    }
}

public class Blog
{
    public int Id { get; set; }
    public string? Name { get; set; }
}

public class SpecialBlog : Blog
{
    public BlogDetails Details { get; set; }
}

public class BlogDetails
{
    public string SomeDetail { get; set; }
    public string SomeDetail2 { get; set; }
}

Error:

      System.Diagnostics.UnreachableException: Check.DebugAssert failed: Unexpected null for SpecialBlog.Details#BlogDetails (BlogDetails)
         at Microsoft.EntityFrameworkCore.Utilities.Check.DebugAssert(Boolean condition, String message) in /home/roji/projects/efcore/src/Shared/Check.cs:line 115
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.InternalComplexEntry.ReadPropertyValue(IPropertyBase propertyBase) in /home/roji/projects/efcore/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.InternalComplexEntry.cs:line 596
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.InternalComplexEntry.get_Item(IPropertyBase propertyBase) in /home/roji/projects/efcore/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.InternalComplexEntry.cs:line 784
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.InternalComplexEntry.GetCurrentValue(IPropertyBase propertyBase) in /home/roji/projects/efcore/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.InternalComplexEntry.cs:line 640
         at Microsoft.EntityFrameworkCore.Update.ColumnModification.GetCurrentValue(IUpdateEntry entry, IProperty property) in /home/roji/projects/efcore/src/EFCore.Relational/Update/ColumnModification.cs:line 210
         at Microsoft.EntityFrameworkCore.Update.ColumnModification.get_Value() in /home/roji/projects/efcore/src/EFCore.Relational/Update/ColumnModification.cs:line 156
         at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.AddParameter(IColumnModification columnModification) in /home/roji/projects/efcore/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs:line 303
         at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.AddParameters(IReadOnlyModificationCommand modificationCommand) in /home/roji/projects/efcore/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs:line 281
         at Microsoft.EntityFrameworkCore.SqlServer.Update.Internal.SqlServerModificationCommandBatch.AddCommand(IReadOnlyModificationCommand modificationCommand) in /home/roji/projects/efcore/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs:line 162
         at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.TryAddCommand(IReadOnlyModificationCommand modificationCommand) in /home/roji/projects/efcore/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs:line 112
         at Microsoft.EntityFrameworkCore.SqlServer.Update.Internal.SqlServerModificationCommandBatch.TryAddCommand(IReadOnlyModificationCommand modificationCommand) in /home/roji/projects/efcore/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs:line 146
         at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.CreateCommandBatches(IEnumerable`1 commandSet, Boolean moreCommandSets, Boolean assertColumnModification, ParameterNameGenerator parameterNameGenerator)+MoveNext() in /home/roji/projects/efcore/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs:line 125
         at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.BatchCommands(IList`1 entries, IUpdateAdapter updateAdapter)+MoveNext() in /home/roji/projects/efcore/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs:line 78
         at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.ExecuteAsync(IEnumerable`1 commandBatches, IRelationalConnection connection, CancellationToken cancellationToken) in /home/roji/projects/efcore/src/EFCore.Relational/Update/Internal/BatchExecutor.cs:line 165
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(IList`1 entriesToSave, CancellationToken cancellationToken) in /home/roji/projects/efcore/src/EFCore/ChangeTracking/Internal/StateManager.cs:line 1328
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(StateManager stateManager, Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken) in /home/roji/projects/efcore/src/EFCore/ChangeTracking/Internal/StateManager.cs:line 1427
         at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.ExecuteAsync[TState,TResult](TState state, Func`4 operation, Func`4 verifySucceeded, CancellationToken cancellationToken) in /home/roji/projects/efcore/src/EFCore.SqlServer/Storage/Internal/SqlServerExecutionStrategy.cs:line 79
         at Microsoft.EntityFrameworkCore.DbContext.SaveChangesAsync(Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken) in /home/roji/projects/efcore/src/EFCore/DbContext.cs:line 773
Unhandled exception. System.Diagnostics.UnreachableException: Check.DebugAssert failed: Unexpected null for SpecialBlog.Details#BlogDetails (BlogDetails)

roji · 2023-08-13T14:20:08Z

src/EFCore/Metadata/IEntityType.cs

+    ///     Returns all properties, including those on complex types.
+    /// </summary>
+    /// <returns>The properties.</returns>
+    IEnumerable<IProperty> GetFlattenedProperties()


Move up to ITypeBase

May name it GetAllNestedProperties?

I assume this wouldn't return properties from complex collections

Probably not.

roji · 2023-08-13T14:26:04Z

src/EFCore/Metadata/Internal/ClrAccessorFactory.cs

@@ -44,7 +44,7 @@ protected virtual TAccessor Create(MemberInfo memberInfo, IPropertyBase? propert
    {
        var boundMethod = propertyBase != null
            ? GenericCreate.MakeGenericMethod(
-                propertyBase.DeclaringType.ClrType,
+                propertyBase.DeclaringType.ContainingEntityType.ClrType,


Except GetClrValue accepts a parameter called entity and it is constrained to be a reference type. (Although I think we violate this in some of our code gen.)

Do you see a specific problem with removing that constraint (or changing the parameter name)? Currently it seems like we're calling GetStructuralTypeClrValue in all kinds of places where the entity vs. complex type distinction doesn't/shouldn't matter...

ajcvickers · 2023-08-13T14:29:23Z

InternalComplexEntry

There is no InternalComplexEntry anymore. If you still have it, then that's likely a bad rebase/merge.

roji

🐑 🇮🇹

The two possible concerns are the naming of GetClrValue, and the moving of all the property counts (and related) from structural down to entity. But we can merge and deal with all that later.

roji · 2023-08-13T14:28:03Z

src/EFCore/ChangeTracking/Internal/IInternalEntry.cs

@@ -27,7 +27,7 @@ public interface IInternalEntry
    ///     any release. You should only use it directly in your code with extreme caution and knowing that
    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
    /// </summary>
-    IRuntimeTypeBase StructuralType { get; }
+    IRuntimeEntityType EntityType { get; }


I'm assuming this is because even when we update only properties within a complex type, we still update the table represented by its containing entity type right? It seems to make sense, but I'm guessing @AndriySvyryd had something in mind here?

Complex objects aren't tracked directly, so they never have an entry in the state manager. Therefore, this can only ever be an entity type.

I expect most of this interface to go away to support collections and value types, so any changes to it meanwhile are fine by me

roji · 2023-08-13T14:32:49Z

src/EFCore/Infrastructure/ModelValidator.cs

@@ -181,14 +181,7 @@ void Validate(IConventionTypeBase typeBase)
                        CoreStrings.ComplexPropertyOptional(typeBase.DisplayName(), complexProperty.Name));
                }

-                if (complexProperty.ComplexType.ClrType.IsValueType)


Does this mean that we'll support value types in 8.0?

I think... it's being considered

Right. So far, value types are still in.

roji · 2023-08-13T14:36:21Z

src/EFCore/Metadata/Internal/ClrAccessorFactory.cs

@@ -44,7 +44,7 @@ protected virtual TAccessor Create(MemberInfo memberInfo, IPropertyBase? propert
    {
        var boundMethod = propertyBase != null
            ? GenericCreate.MakeGenericMethod(
-                propertyBase.DeclaringType.ClrType,
+                propertyBase.DeclaringType.ContainingEntityType.ClrType,


OK.

This is purely a naming thing, so we can discuss later in API review. But I'd expect the simple GetClrValue to simply behave like GetStructuralTypeClrValue, and have a specially-named GetClrValueFromContainingEntity for those cases where you want it to do the (considerably more complex) thing of traversing into an arbitrarily deep graph from the containing entity all the way to the leaf property. In any case, there's nothing in the GetClrValue name that suggests it requires the containing entity type etc.

roji · 2023-08-13T14:45:58Z

src/EFCore/Metadata/Internal/IRuntimeTypeBase.cs

-    ///     any release. You should only use it directly in your code with extreme caution and knowing that
-    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
-    /// </summary>
-    int ShadowPropertyCount


This is used in ShapedQueryCompilingExpressionVisitor - shouldn't we be able to get e.g. the shadow property count for a complex count? Most of these seem just as relevant for a complex type as they are for entity types, at least from a pure metadata perspective.

If the counts are needed, then we can add them back. From a change tracking perspective, these are computed as part of computing the snapshotting indexes, and these only exist for entity types since, as pointed out before, complex types don't have entries.

When passing shadow values to the state manager, you should pass all the shadow values for all contained complex types in a single value buffer along with the shadow properties for the entity type itself. The shadow indexes reflect this.

ajcvickers · 2023-08-13T16:35:12Z

@roji The code in your test is invalid, although it should throw a better exception. The complex property on the derived type must be non-null, since it is required. So the code needs to be this:

ctx.Blogs.Add(new Blog { Name = "foo" });
ctx.SpecialBlogs.Add(new SpecialBlog { Name = "bar", Details = new BlogDetails { SomeDetail = "X", SomeDetail2 = "Y" } });

However, I then get this exception:

Cannot insert the value NULL into column 'Details_SomeDetail2', table 'AllTogetherNow.dbo.Blogs'; column does not allow nulls. INSERT fails.
   at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at Microsoft.Data.SqlClient.SqlDataReader.TryHasMoreRows(Boolean& moreRows)
   at Microsoft.Data.SqlClient.SqlDataReader.TryReadInternal(Boolean setTimeout, Boolean& more)
   at Microsoft.Data.SqlClient.SqlDataReader.ReadAsyncExecute(Task task, Object state)
   at Microsoft.Data.SqlClient.SqlDataReader.InvokeAsyncCall[T](SqlDataReaderBaseAsyncCallContext`1 context)

This seems to be #31378.

ajcvickers · 2023-08-13T16:36:24Z

Also, only amateurs call DbSet.Add. Make it shorter: 😆

ctx.Add(new Blog { Name = "foo" });
ctx.Add(new SpecialBlog { Name = "bar", Details = new BlogDetails { SomeDetail = "X", SomeDetail2 = "Y" } });

roji · 2023-08-13T16:38:24Z

Ahahahaha touché... I usually do ctx.Blog.Add with target-typed new() but I forgot this time. I'm just wary of any API which accepts object ;)

roji · 2023-08-13T16:39:34Z

The code in your test is invalid, although it should throw a better exception. The complex property on the derived type must be non-null, since it is required.

Thanks, makes sense! That should mean that at least TPT/TPC tests should be possible.

ajcvickers · 2023-08-13T17:56:20Z

@roji Merged

AndriySvyryd · 2023-08-14T23:56:24Z

src/EFCore/Metadata/ITypeBase.cs

+    ///     Returns all properties that implement <see cref="IProperty"/>, including those on complex types.
+    /// </summary>
+    /// <returns>The properties.</returns>
+    IEnumerable<IProperty> GetFlattenedProperties()


This and GetSnapshottableMembers should be implemented in RuntimeTypeBase in a way that doesn't allocate.

By, "in a way that doesn't allocate", do you mean we should pre-compute this collection?

It can be precomputed or memoized

AndriySvyryd · 2023-08-15T00:03:51Z

src/EFCore/Metadata/Internal/PropertyBase.cs

@@ -426,7 +426,8 @@ public virtual IComparer<IUpdateEntry> CurrentValueComparer
    public static Expression CreateMemberAccess(
        IPropertyBase? property,
        Expression instanceExpression,
-        MemberInfo memberInfo)
+        MemberInfo memberInfo,
+        bool fromStructuralType)


fromContainingType

AndriySvyryd · 2023-08-15T00:06:30Z

src/EFCore/Metadata/Internal/PropertyBase.cs

+                fromStructuralType);
+
+            if (!instanceExpression.Type.IsValueType
+                || instanceExpression.Type.IsNullableValueType())


Isn't there an IsNullable extension method?

Can't find one.

AndriySvyryd · 2023-08-15T00:20:17Z

src/EFCore/Metadata/IClrPropertyGetter.cs

+    /// </summary>
+    /// <param name="complexObject">The complex type instance instance.</param>
+    /// <returns><see langword="true" /> if the property value is the CLR default; <see langword="false" /> it is any other value.</returns>
+    bool HasStructuralTypeSentinelValue(object complexObject);


These two methods will box value types

Filed #31493

Per #31453 (review)

Initial change tracking and DetectChanges for complex types

1a93f44

Part of #9906

ajcvickers requested a review from a team August 12, 2023 12:59

Initial change tracking and DetectChanges for complex types

d194501

Part of #9906

roji reviewed Aug 13, 2023

View reviewed changes

ajcvickers added 2 commits August 13, 2023 10:36

Merge branch '230812_Voidbringer' into 08092023_Bern

a3530c4

Allow reading complex type property value directly from the complex type

280a22f

roji reviewed Aug 13, 2023

View reviewed changes

roji approved these changes Aug 13, 2023

View reviewed changes

Tweaks

4a6a52c

ajcvickers merged commit 0611bee into main Aug 13, 2023

ajcvickers deleted the 08092023_Bern branch August 13, 2023 17:56

AndriySvyryd reviewed Aug 14, 2023

View reviewed changes

AndriySvyryd reviewed Aug 15, 2023

View reviewed changes

ajcvickers mentioned this pull request Aug 17, 2023

Avoid boxing when using IClrPropertyGetter with struct complex types #31493

Open

ajcvickers added a commit that referenced this pull request Aug 17, 2023

Memoize flattened property lookup

68bc8e9

Per #31453 (review)

ajcvickers mentioned this pull request Aug 17, 2023

Memoize flattened property lookup #31494

Merged

ajcvickers added a commit that referenced this pull request Aug 17, 2023

Memoize flattened property lookup

95adcf1

Per #31453 (review)

Initial change tracking and DetectChanges for complex types #31453

Initial change tracking and DetectChanges for complex types #31453

Conversation

ajcvickers commented Aug 12, 2023

ajcvickers commented Aug 12, 2023

roji commented Aug 12, 2023

roji commented Aug 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers commented Aug 13, 2023

roji commented Aug 13, 2023 • edited Loading

roji Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

AndriySvyryd Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers commented Aug 13, 2023

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers commented Aug 13, 2023

ajcvickers commented Aug 13, 2023

roji commented Aug 13, 2023

roji commented Aug 13, 2023 • edited Loading

ajcvickers commented Aug 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers Aug 13, 2023 •

edited

Loading

roji commented Aug 13, 2023 •

edited

Loading

roji Aug 13, 2023 •

edited

Loading

AndriySvyryd Aug 14, 2023 •

edited

Loading

roji commented Aug 13, 2023 •

edited

Loading