Add change tracking support for complex collections #35962
Conversation
d56cde0 to
56d99e6
Compare
56d99e6 to
547b2c5
Compare
547b2c5 to
c103ff1
Compare
e099818 to
cbc8c95
Compare
cbc8c95 to
5a16ed7
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds change tracking support for complex collections by updating property accessor lambdas to include an additional IReadOnlyList indices parameter and by refining snapshot creation logic. Key changes include:
- Updating setter, materialization setter, and accessor lambdas across multiple baseline files to support tracking of nested collection ordinals.
- Renaming local variables (e.g. from “entity” to “structuralType”) in snapshot factory lambdas for improved clarity.
- Adjusting or stubbing out tests for complex type/struct collections in response to known issues.
Reviewed Changes
Copilot reviewed 98 out of 154 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| DataEntityType.cs | Updated accessor lambdas to include indices parameter and refined local variable naming in snapshot creation. |
| PrincipalDerivedEntityType.cs (and similar files) | Updated property accessors and snapshot factory lambdas with the new indices parameter for ordinal tracking. |
| ComplexTypesTrackingSqlServerTest.cs & ModelBuilderTest.ComplexCollections.cs | Added or modified test stubs and skipped tests to reflect unsupported scenarios and document related issues. |
Files not reviewed (1)
- src/EFCore/Properties/CoreStrings.Designer.cs: Language not supported
Comments suppressed due to low confidence (3)
test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/CheckConstraints/DataEntityType.cs:76
- The accessor lambdas have been updated to include an extra 'IReadOnlyList indices' parameter. Please verify that all downstream consumers are updated to pass the correct indices and that the intended usage of this parameter is clearly documented.
(CompiledModelTestBase.Data entity, IReadOnlyList<int> indices, byte[] value) => DataUnsafeAccessors.Blob(entity) = value);
test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/CheckConstraints/DataEntityType.cs:125
- [nitpick] Renaming the local variable to 'structuralType' improves clarity in snapshot creation especially for value types; ensure that any related documentation or internal comments reflect this change.
var structuralType = ((CompiledModelTestBase.Data)(source.Entity));
test/EFCore.SqlServer.FunctionalTests/ComplexTypesTrackingSqlServerTest.cs:101
- Several test methods have been changed to have empty bodies or to return Task.CompletedTask. It would be helpful to add a brief comment in each method explaining that these are stubs due to known limitations (see Issue #36175) so that future maintainers have the necessary context.
// Issue #36175: Complex types with notification change tracking are not supported
There was a problem hiding this comment.
🐑 🇮🇹
@AndriySvyryd I'd have liked to spend more time here and get a deeper understanding of how everything works, but this is the level I got to given the time constraints we have... Most of the comments are either nits or questions for my understanding.
| /// examples. | ||
| /// </remarks> | ||
| /// <value> An entry for the entity that owns this member. </value> | ||
| public virtual EntityEntry EntityEntry |
There was a problem hiding this comment.
If we allow users to navigate to the containing entity entry, should we also allow them to navigate to the immediately-containing complex entry (for nested complex entries)?
There was a problem hiding this comment.
Possibly, but then we'd have to have the naming conversation. For now I am keeping this API minimal, and we can decide later how to make it more useful based on feedback.
| /// See <see href="https://aka.ms/efcore-docs-entity-entries">Accessing tracked entities in EF Core</see> for more information and | ||
| /// examples. | ||
| /// </remarks> | ||
| public virtual IEnumerable<ComplexCollectionEntry> ComplexCollections |
There was a problem hiding this comment.
Re the modeling here: I can see that both ComplexCollectionEntry and ComplexPropertyEntry extend MemberEntry, but that ComplexCollectionEntry doesn't extend ComplexPropertyEntry. Don't we want to treat complex collections as a property that happens to be a collection - just like IComplexProperty in the model represents both? Under that kind of modeling, we'd just have the ComplexProperties above, and some of the entries it returns would happen to be collections etc.
There was a problem hiding this comment.
This is consistent with CollectionEntry/ReferenceEntry and ComplexPropertyBuilder/ComplexCollectionBuilder. For the high-level API we try to avoid inheritance to be able to have members on the singular version that we don't want to expose on the collection version.
| /// examples. | ||
| /// </para> | ||
| /// </remarks> | ||
| public class ComplexEntry : IInfrastructure<InternalComplexEntry> |
There was a problem hiding this comment.
Am I correct in understanding that ComplexEntry is only used for entries with complex collections (i.e. it's returned from the indexer of ComplexCollectionEntry)? In other words, if I interact with non-collections, I'll never see a ComplexEntry?
If that's the case, maybe make that clear in the documentation (and possibly change the name: ComplexElementEntry?). I'm also wondering if it's possible to simplify things a bit, e.g. just have ComplexEntry and ComplexCollectionEntry, with ComplexEntry being used both for the elements of ComplexCollectionEntry and for non-collection complex entries (in other words do away with ComplexPropertyEntry)?
There was a problem hiding this comment.
Yes, we can rename it to ComplexElementEntry. But again, we want to keep them separate as there might be API we want to add to one, but not the other. At this level we are not concerned about logic duplication (there shouldn't be much logic anyway) or polymorphism (these instances are not designed to be passed around)
| @@ -1,8 +1,11 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
| // The .NET Foundation licenses this file to you under the MIT license. | |||
|
|
|||
| using System.Collections.Generic; | |||
| using System.Xml.Linq; | |||
There was a problem hiding this comment.
Say what now? 😨
(just some unused usings)
| /// 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> | ||
| IReadOnlyList<int> GetOrdinals(); |
There was a problem hiding this comment.
In general am just curious why we have the IInternalEntry interface in addition to InternalEntryBase
There was a problem hiding this comment.
I knew this would come up. Technically we don't need it, but that's the type used in property accessors we generate in the compiled model, so it forces us to be more conscious about the members that are exposed on it and how they are modified
| out Expression<Func<TRoot, bool>> hasSentinelExpression, | ||
| out Expression<Func<TDeclaring, TValue>> structuralGetterExpression, | ||
| out Expression<Func<TDeclaring, bool>> hasStructuralSentinelExpression) | ||
| out Expression<Func<TRoot, IReadOnlyList<int>, TValue>> getClrValueUsingContainingEntityExpression, |
There was a problem hiding this comment.
If I understand correctly, the change tracker references the root entity type only, and traverses the tree down every time to actually access nested properties inside the complex sub-document - is that right? If so, I'm assuming this is done in order to support value types, which can't be (safely) referenced by the change tracker (is that right?).
But do you think it makes sense to reference complex nested types when they're a reference type, as an optimization, to avoid having to traverse the tree every time? Are we not doing that because of the added complexity, or is there some other reason?
There was a problem hiding this comment.
Time constraints. Tracked in #36041
5a16ed7 to
773e8fa
Compare
Part of #31237
Overview
The implementation introduces change-tracking support for complex type collections that enables EF Core to detect modifications, additions, and deletions within (nested) complex type collections. This design leverages ordinal-based tracking and runtime-generated property accessors to efficiently manage state changes in complex type collections.
Change tracking entries
This change introduces
InternalComplexEntry, likeInternalEntityEntryit directly tracks the original values and changes to the contained properties and non-collection complex properties. But unlike the latter, it doesn't contain a reference to the tracked object as it could be of a value type.To access nested complex entries for a particular complex type collection the current or original ordinal for the corresponding collection needs to be used.
The change-tracking system employs a dual-ordinal approach to maintain element positioning:
This dual-ordinal system enables the change tracker to correlate elements between their original and current states, facilitating accurate detection of positional changes and element migrations within collections.
Value-Type Support for Observable Collections
The architecture is designed with forward compatibility for value-type collections. The ordinal-based tracking mechanism abstracts away the reference semantics typically required for change detection, allowing the same infrastructure to support both reference-type complex objects and value-type collections. This is achieved through the indices array system that provides positional addressing regardless of the underlying element type.
DetectChanges Algorithm
The
DetectChangesprocess for complex type collections implements a matching algorithm that relies on snapshots:Property Accessor Architecture
The property accessor system employs runtime code generation to create efficient access patterns for nested complex type collections:
Indices Array System
Each complex type collection maintains an indices array that contains index values for each level of nesting. For a deeply nested structure like
Order.Details[i].LineItems[j].Properties[k], the indices array would contain[i, j, k], representing the position at each collection level.