-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Issue #31819: Fast DetectChanges; Issue #16491: legacy Merge-Option Feature #37812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -215,6 +215,44 @@ public virtual IEnumerable<EntityEntry<TEntity>> Entries<TEntity>() | |||||||
| .Select(e => new EntityEntry<TEntity>(e)); | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Returns tracked entities that are in a given state from a fast cache. | ||||||||
| /// </summary> | ||||||||
| /// <param name="added">Entities in EntityState.Added state</param> | ||||||||
| /// <param name="modified">Entities in Modified.Added state</param> | ||||||||
| /// <param name="deleted">Entities in Modified.Deleted state</param> | ||||||||
| /// <param name="unchanged">Entities in Modified.Unchanged state</param> | ||||||||
| /// <returns>An entry for each entity that matched the search criteria.</returns> | ||||||||
| public virtual IEnumerable<EntityEntry> GetEntriesForState( | ||||||||
| bool added = false, | ||||||||
| bool modified = false, | ||||||||
| bool deleted = false, | ||||||||
| bool unchanged = false) | ||||||||
|
Comment on lines
+226
to
+230
|
||||||||
| { | ||||||||
|
||||||||
| { | |
| { | |
| TryDetectChanges(); |
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this method is incomplete and doesn't follow EF Core documentation conventions. It should include:
- A proper summary describing what the method does and when to use it
- Remarks section with links to documentation using
<see href="https://aka.ms/efcore-docs-*">format - Proper parameter descriptions using
<param>tags - Better description of the state parameters (currently says "Modified.Added" and "Modified.Deleted" which appear to be copy-paste errors - should be "EntityState.Modified" and "EntityState.Deleted")
Compare with the existing Entries() method documentation at lines 183-216 for the expected format.
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this method has copy-paste errors in parameter descriptions. Line 222 says "Entities in Modified.Added state" when it should say "Entities in EntityState.Modified state". Line 223 says "Entities in Modified.Deleted state" when it should say "Entities in EntityState.Deleted state". Line 224 says "Entities in Modified.Unchanged state" when it should say "Entities in EntityState.Unchanged state".
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description claims "Added test for testing ChangeTracker.GetEntries(EntityState)" but no tests for the new GetEntriesForState methods are included in this PR. The new public API methods ChangeTracker.GetEntriesForState() and ChangeTracker.GetEntriesForState() lack test coverage. Tests should be added to verify the behavior of these methods, including:
- Filtering by different entity states (added, modified, deleted, unchanged)
- Filtering by combinations of states
- Generic vs non-generic versions
- Edge cases (no entities, all entities in same state, etc.)
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -680,6 +680,65 @@ public virtual void Reload() | |
| public virtual async Task ReloadAsync(CancellationToken cancellationToken = default) | ||
| => Reload(await GetDatabaseValuesAsync(cancellationToken).ConfigureAwait(false)); | ||
|
|
||
| /// <summary> | ||
| /// Reloads the entity from the database using the specified <see cref="MergeOption" />. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// The behavior of this method depends on the <see cref="MergeOption" /> specified: | ||
| /// </para> | ||
| /// <para> | ||
| /// <see cref="MergeOption.OverwriteChanges" />: Overwrites both current and original property values with values from the database. | ||
| /// The entity will be in the <see cref="EntityState.Unchanged" /> state after calling this method. | ||
| /// </para> | ||
| /// <para> | ||
| /// <see cref="MergeOption.PreserveChanges" />: Updates original property values with values from the database, | ||
| /// but preserves any local modifications to current values. Modified properties remain modified with their current values. | ||
| /// </para> | ||
| /// <para> | ||
| /// If the entity does not exist in the database, the entity will be <see cref="EntityState.Detached" />. | ||
| /// Calling Reload on an <see cref="EntityState.Added" /> entity that does not exist in the database is a no-op. | ||
| /// </para> | ||
| /// <para> | ||
| /// See <see href="https://aka.ms/efcore-docs-entity-entries">Accessing tracked entities in EF Core</see> for more information and | ||
| /// examples. | ||
| /// </para> | ||
| /// </remarks> | ||
| /// <param name="mergeOption">The merge option controlling how database values are applied to the entity.</param> | ||
| public virtual void Reload(MergeOption mergeOption) | ||
| => Reload(GetDatabaseValues(), mergeOption); | ||
|
|
||
| /// <summary> | ||
| /// Reloads the entity from the database using the specified <see cref="MergeOption" />. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// The behavior of this method depends on the <see cref="MergeOption" /> specified: | ||
| /// </para> | ||
| /// <para> | ||
| /// <see cref="MergeOption.OverwriteChanges" />: Overwrites both current and original property values with values from the database. | ||
| /// The entity will be in the <see cref="EntityState.Unchanged" /> state after calling this method. | ||
| /// </para> | ||
| /// <para> | ||
| /// <see cref="MergeOption.PreserveChanges" />: Updates original property values with values from the database, | ||
| /// but preserves any local modifications to current values. Modified properties remain modified with their current values. | ||
| /// </para> | ||
| /// <para> | ||
| /// If the entity does not exist in the database, the entity will be <see cref="EntityState.Detached" />. | ||
| /// Calling Reload on an <see cref="EntityState.Added" /> entity that does not exist in the database is a no-op. | ||
| /// </para> | ||
| /// <para> | ||
| /// See <see href="https://aka.ms/efcore-docs-entity-entries">Accessing tracked entities in EF Core</see> for more information and | ||
| /// examples. | ||
| /// </para> | ||
| /// </remarks> | ||
| /// <param name="mergeOption">The merge option controlling how database values are applied to the entity.</param> | ||
| /// <param name="cancellationToken">A <see cref="CancellationToken" /> to observe while waiting for the task to complete.</param> | ||
| /// <returns>A task that represents the asynchronous operation.</returns> | ||
| /// <exception cref="OperationCanceledException">If the <see cref="CancellationToken" /> is canceled.</exception> | ||
| public virtual async Task ReloadAsync(MergeOption mergeOption, CancellationToken cancellationToken = default) | ||
| => Reload(await GetDatabaseValuesAsync(cancellationToken).ConfigureAwait(false), mergeOption); | ||
|
|
||
| private void Reload(PropertyValues? storeValues) | ||
| { | ||
| if (storeValues == null) | ||
|
|
@@ -697,6 +756,30 @@ private void Reload(PropertyValues? storeValues) | |
| State = EntityState.Unchanged; | ||
| } | ||
| } | ||
| private void Reload(PropertyValues? storeValues, MergeOption mergeOption) | ||
| { | ||
| if (storeValues == null) | ||
| { | ||
| if (State != EntityState.Added) | ||
| { | ||
| State = EntityState.Deleted; | ||
| State = EntityState.Detached; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| foreach (var property in Metadata.GetProperties()) | ||
| { | ||
| var value = storeValues[property]; | ||
| InternalEntry.ReloadValue(property, value, mergeOption, updateEntityState: false); | ||
| } | ||
|
|
||
| if (mergeOption == MergeOption.OverwriteChanges) | ||
| { | ||
| State = EntityState.Unchanged; | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+759
to
+782
|
||
|
|
||
| [field: AllowNull, MaybeNull] | ||
| private IEntityFinder Finder | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -998,6 +998,38 @@ private void DetectChanges(IComplexProperty complexProperty) | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Refreshes the property value with the value from the database | ||
| /// </summary> | ||
| /// <param name="propertyBase">Property</param> | ||
| /// <param name="value">New value from database</param> | ||
| /// <param name="mergeOption">MergeOption</param> | ||
| /// <param name="updateEntityState">Sets the EntityState to Unchanged if MergeOption.OverwriteChanges else calls ChangeDetector to determine changes</param> | ||
| public virtual void ReloadValue(IPropertyBase propertyBase, object? value, MergeOption mergeOption, bool updateEntityState) | ||
| { | ||
| var property = (IProperty)propertyBase; | ||
| EnsureOriginalValues(); | ||
|
Comment on lines
+1008
to
+1011
|
||
| bool isModified = IsModified(property); | ||
| _originalValues.SetValue(property, value, -1); | ||
| if (mergeOption == MergeOption.OverwriteChanges || !isModified) | ||
| { | ||
| SetProperty(propertyBase, value, isMaterialization: true, setModified: false); | ||
| } | ||
|
|
||
| if (updateEntityState) | ||
| { | ||
| if (mergeOption == MergeOption.OverwriteChanges) | ||
| { | ||
| SetEntityState(EntityState.Unchanged); | ||
| } | ||
| else if (StateManager is StateManager stateManager | ||
| && stateManager.ChangeDetector is ChangeDetector changeDetector) | ||
| { | ||
| changeDetector.DetectValueChange(this, property); | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
1001
to
1031
|
||
|
|
||
| private void ReorderOriginalComplexCollectionEntries(IComplexProperty complexProperty, IList? newOriginalCollection) | ||
| { | ||
| Check.DebugAssert(HasOriginalValuesSnapshot, "This should only be called when original values are present"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3099,6 +3099,87 @@ public static IQueryable<TEntity> AsTracking<TEntity>( | |||||||||||
|
|
||||||||||||
| #endregion | ||||||||||||
|
|
||||||||||||
| #region Refreshing | ||||||||||||
|
|
||||||||||||
| internal static readonly MethodInfo RefreshMethodInfo | ||||||||||||
| = typeof(EntityFrameworkQueryableExtensions).GetMethod( | ||||||||||||
| nameof(Refresh), [typeof(IQueryable<>).MakeGenericType(Type.MakeGenericMethodParameter(0)), typeof(MergeOption)])!; | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
|
Comment on lines
+3105
to
+3108
|
||||||||||||
| = typeof(EntityFrameworkQueryableExtensions).GetMethod( | |
| nameof(Refresh), [typeof(IQueryable<>).MakeGenericType(Type.MakeGenericMethodParameter(0)), typeof(MergeOption)])!; | |
| = typeof(EntityFrameworkQueryableExtensions).GetTypeInfo() | |
| .GetDeclaredMethod(nameof(Refresh))! | |
| .GetGenericMethodDefinition(); |
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The <returns> XML doc for Refresh says "annotated with the given tag", which looks like it was copied from TagWith and doesn't describe Refresh. Please update it to describe the refresh behavior instead.
| /// <returns>A new query annotated with the given tag.</returns> | |
| /// <returns>A new query that refreshes already loaded entities according to the specified merge option.</returns> |
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refresh relies on HasNonTrackingOrIgnoreAutoIncludes(source.Expression) to enforce tracking. This only catches explicit AsNoTracking*() calls; it won't catch queries that are non-tracking because ChangeTracker.QueryTrackingBehavior is configured globally, making Refresh a silent no-op instead of throwing.
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for the Refresh method is incomplete and doesn't follow EF Core documentation conventions. It should include:
- A proper remarks section explaining how this method interacts with the change tracker and when to use it
- Links to documentation using the
<see href="https://aka.ms/efcore-docs-*">format (not hardcoded learn.microsoft.com URLs) - An example showing how to use this method
- Better parameter documentation - the current "The MergeOption" description is not helpful
- The return value description says "A new query annotated with the given tag" which is incorrect (copy-pasted from another method) - should describe that it returns a query that will refresh tracked entities when materialized
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HasNonTrackingOrIgnoreAutoIncludes treats IgnoreAutoIncludes() as equivalent to non-tracking and causes Refresh to throw RefreshNonTrackingQuery. Ignoring auto-includes doesn't disable tracking, so this check (or the exception message) should be adjusted.
| || name == nameof(AsNoTrackingWithIdentityResolution) | |
| || name == nameof(IgnoreAutoIncludes)) | |
| || name == nameof(AsNoTrackingWithIdentityResolution)) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,27 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| namespace Microsoft.EntityFrameworkCore; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// The different ways that new objects loaded from the database can be merged with existing objects already in memory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public enum MergeOption | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Will only append new (top level-unique) rows. This is the default behavior. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AppendOnly = 0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// The incoming values for this row will be written to both the current value and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// the original value versions of the data for each column. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| OverwriteChanges = 1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// The incoming values for this row will be written to the original value version | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// of each column. The current version of the data in each column will not be changed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+7
to
+24
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// The different ways that new objects loaded from the database can be merged with existing objects already in memory. | |
| /// </summary> | |
| public enum MergeOption | |
| { | |
| /// <summary> | |
| /// Will only append new (top level-unique) rows. This is the default behavior. | |
| /// </summary> | |
| AppendOnly = 0, | |
| /// <summary> | |
| /// The incoming values for this row will be written to both the current value and | |
| /// the original value versions of the data for each column. | |
| /// </summary> | |
| OverwriteChanges = 1, | |
| /// <summary> | |
| /// The incoming values for this row will be written to the original value version | |
| /// of each column. The current version of the data in each column will not be changed. | |
| /// The different ways that new objects loaded from the database can be merged with existing objects already in memory. | |
| /// </summary> | |
| public enum MergeOption | |
| { | |
| /// <summary> | |
| /// Will only append new (top level-unique) rows. This is the default behavior. | |
| /// </summary> | |
| AppendOnly = 0, | |
| /// <summary> | |
| /// The incoming values for this row will be written to both the current value and | |
| /// the original value versions of the data for each column. | |
| /// </summary> | |
| OverwriteChanges = 1, | |
| /// <summary> | |
| /// The incoming values for this row will be written to the original value version | |
| /// of each column. The current version of the data in each column will not be changed. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XML doc parameter descriptions for
modified,deleted, andunchangedreference non-existent states like "Modified.Added"/"Modified.Deleted". These should refer toEntityState.Modified,EntityState.Deleted, andEntityState.Unchanged(and ideally use<see cref="EntityState.*" />).