Conversation
…onger necessary if all entity classes in the model are defined with a change-tracking strategy other than "ChangeTrackingStrategy.Snapshot." This is controlled in the ProcessModelFinalizing method of the ChangeTrackingStrategyConvention class. DetectChanges is therefore not called when saving because change tracking is performed using the EntityReferenceMap (_entityReferenceMap property), which essentially corresponds to the previous caching mechanism using EntityState dictionaries from EF4/5/6. However, querying this cache was not enabled; instead, users were forced to use ChangeTracker.Entries(), which returned all objects and led to performance problems with large long-term contexts. This change now enables fast access to changed objects using the new GetEntriesForState method in the ChangeTracker class. This, in turn, calls EntityReferenceMap.GetEntriesForState(), which already exists. For fast change tracking, implement INotifyProperty-Changed on your entity classes and activate this strategy in the Model Builder using ChangeTrackingStrategy.ChangedNotifications via HasChangeTrackingStrategy(). dotnet#31819
|
This needs to target |
|
@dotnet-policy-service agree |
|
As this seems to be a new feature, there's very little chance we'll be backporting this to 9.0. In fact, the window for introducing new features into 10.0 is also closing very soon, so this will most likely be considered for 11.0 at the earliest. |
Do I need to do anything else on my side, or can you handle the remaining work yourself so it can be included in release 10 or 11? (I'm asking because I'm not familiar with GitHub workflows or merging into the main branch.) |
|
As @cincuranet wrote above, you need retarget this PR against main - it's currently targeting release/9.0. And as I wrote above, this will almost certainly not going into EF 10 (or 9). |
|
@cincuranet How can I "retarget to main"? Do you have any instructions? |
|
Rebase your changes on top of |
…onger necessary if all entity classes in the model are defined with a change-tracking strategy other than "ChangeTrackingStrategy.Snapshot." This is controlled in the ProcessModelFinalizing method of the ChangeTrackingStrategyConvention class. DetectChanges is therefore not called when saving because change tracking is performed using the EntityReferenceMap (_entityReferenceMap property), which essentially corresponds to the previous caching mechanism using EntityState dictionaries from EF4/5/6. However, querying this cache was not enabled; instead, users were forced to use ChangeTracker.Entries(), which returned all objects and led to performance problems with large long-term contexts. This change now enables fast access to changed objects using the new GetEntriesForState method in the ChangeTracker class. This, in turn, calls EntityReferenceMap.GetEntriesForState(), which already exists. For fast change tracking, implement INotifyProperty-Changed on your entity classes and activate this strategy in the Model Builder using ChangeTrackingStrategy.ChangedNotifications via HasChangeTrackingStrategy(). dotnet#31819
DamirLisak
requested review from
AndriySvyryd and
dotnet-maestro-bot
as code owners
There was a problem hiding this comment.
This needs a significant number of new tests to be added.
- Some tests for
GetEntriesForStateto exercise each parameter - A new test class for
Refresh-test\EFCore.Specification.Tests\MergeOptionTestBase.cs- It should cover the following cases:
- Existing entries in all states
- Unchanged entries with original value set to something that doesn't match the database state
- Modified entries with properties marked as modified, but with the original value set to something that matches the database state
- Owned entity that was replaced by a different instance, so it's tracked as both Added and Deleted
- A derived entity that was replaced by a base entity with same key value
- Different terminating operators:
ToList,FirstOrDefault, etc... - Streaming (non-buffering) query that's consumed one-by-one
- Queries with
Include,Includewith filter andThenInclude - Projecting a related entity in
SelectwithoutInclude - Creating a new instance of the target entity in
Selectwith calculated values that are going to be client-evaluated - Projecting an entity multiple times in
Selectwith same key, but different property values - Lazy-loading proxies with navigations in loaded and unloaded states
- Non-tracking queries should throw
- Multiple
Refreshwith different values in the same query should throw
- The test model should incorporate the following features:
- Collection and non-collection owned types
- Collection and non-collection complex properties of value and reference types
- Many-to-many relationships without an explicit join type
- Global query filters
- Shadow and non-shadow properties
- Properties mapped to computed columns
- Properties with value converters
- Primitive collections
- Table-sharing with shared non-key columns
- It should cover the following cases:
There was a problem hiding this comment.
Pull request overview
This PR implements the legacy MergeOption feature from EF4/5/6, enabling fast access to changed objects and database refresh functionality. The implementation adds a Refresh extension method that allows queries to overwrite or preserve tracked entity changes when reloading from the database, and introduces GetEntriesForState methods to the ChangeTracker for efficient state-based entity retrieval.
Key Changes:
- Added
MergeOptionenum with AppendOnly, OverwriteChanges, and PreserveChanges options - Implemented
Refresh<T>LINQ extension method for query-level merge control - Added
GetEntriesForStatemethods toChangeTrackerfor efficient filtered entity access
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/tech_stack_review.md | Comprehensive EF Core learning guide covering database fundamentals, EF Core 9 concepts, and test contexts |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/RefreshFromDb_ValueConverters_SqlServer_Test.cs | Tests for value converter refresh scenarios (enum-to-string, JSON collections, DateTime, Guid, custom value objects) |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/RefreshFromDb_TableSharing_SqlServer_Test.cs | Tests for table sharing scenarios where multiple entity types map to the same database table |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/RefreshFromDb_ShadowProperties_SqlServer_Test.cs | Tests for shadow property refresh including audit fields and shadow foreign keys |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/RefreshFromDb_PrimitiveCollections_SqlServer_Test.cs | Tests for primitive collection refresh (List, List, List) stored as JSON |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/RefreshFromDb_Northwind_SqlServer_Test.cs | Comprehensive refresh tests using Northwind database covering entity states, query patterns, and Include operations |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/RefreshFromDb_ManyToMany_SqlServer_Test.cs | Tests for many-to-many relationship refresh including bidirectional updates and collection navigation |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/RefreshFromDb_GlobalFilters_SqlServer_Test.cs | Tests for global query filter behavior during refresh (multi-tenancy and soft delete patterns) |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/RefreshFromDb_ComputedColumns_SqlServer_Test.cs | Tests for computed column refresh when base column values change |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/RefreshFromDb_ComplexTypes_SqlServer_Test.cs | Tests for complex type and owned entity refresh scenarios |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/NorthwindMergeOptionFeatureFixture.cs | Fixture for seeding test data across all MergeOption feature tests |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/NorthwindMergeOptionFeatureContext.cs | DbContext with configurations for all test scenarios (complex types, computed columns, global filters, etc.) |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/MergeOptionFeature_Test_Guide.md | Detailed guide explaining all test scenarios and their purposes |
| test/EFCore.SqlServer.FunctionalTests/MergeOptionFeature/MergeOptionFeatureTask.md | Task description document in Croatian detailing implementation requirements |
| src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs | Core implementation of entity refresh logic during query materialization |
| src/EFCore/Query/QueryCompilationContext.cs | Added RefreshMergeOption property to compilation context |
| src/EFCore/Query/Internal/QueryableMethodNormalizingExpressionVisitor.cs | Processing of Refresh method calls during query normalization |
| src/EFCore/Properties/CoreStrings.resx | Added error messages for refresh validation |
| src/EFCore/MergeOption.cs | Enum defining merge options for entity refresh |
| src/EFCore/Extensions/EntityFrameworkQueryableExtensions.cs | Public Refresh extension method with validation |
| src/EFCore/ChangeTracking/Internal/InternalEntryBase.cs | ReloadValue method for updating entity values with merge option support |
| src/EFCore/ChangeTracking/ChangeTracker.cs | GetEntriesForState methods for efficient state-based entity retrieval |
Files not reviewed (1)
- src/EFCore/Properties/CoreStrings.Designer.cs: Language not supported
| @@ -0,0 +1,47 @@ | |||
| ## MergeOptionFeature Tasks | |||
There was a problem hiding this comment.
The entire MergeOptionFeatureTask.md file is written in Croatian, which is inconsistent with the project's English documentation standards. This file should either be translated to English or removed from the codebase if it's only meant for internal/temporary use.
| private readonly bool _queryStateManager = | ||
| queryTrackingBehavior is QueryTrackingBehavior.TrackAll or QueryTrackingBehavior.NoTrackingWithIdentityResolution; | ||
|
|
||
| private readonly MergeOption _MergeOption = mergeOption; |
There was a problem hiding this comment.
The field '_MergeOption' uses PascalCase instead of the expected camelCase naming convention for private fields. It should be renamed to '_mergeOption' to follow C# naming conventions.
| private readonly MergeOption _MergeOption = mergeOption; | |
| private readonly MergeOption _mergeOption = mergeOption; |
| bool isNotTracked = false; | ||
|
|
||
| string[] expressionNames = source.Expression.ToString().Split('.'); | ||
| if ( | ||
| expressionNames.Any(c => c.Contains(nameof(EntityFrameworkQueryableExtensions.AsNoTracking))) | ||
| || expressionNames.Any(c => c.Contains(nameof(EntityFrameworkQueryableExtensions.AsNoTrackingWithIdentityResolution))) | ||
| || expressionNames.Any(c => c.Contains(nameof(EntityFrameworkQueryableExtensions.IgnoreAutoIncludes))) | ||
| ) | ||
| { | ||
| isNotTracked = true; | ||
| } | ||
|
|
||
| if (isNotTracked) | ||
| { | ||
| throw new InvalidOperationException(CoreStrings.RefreshNonTrackingQuery); | ||
| } | ||
|
|
||
| MergeOption[] otherMergeOptions = Enum.GetValues<MergeOption>().Where(v => v != mergeOption).ToArray(); | ||
|
|
||
| bool anyOtherMergeOption = expressionNames.Any(c => otherMergeOptions.Any(o => c.Contains(o.ToString()))); | ||
| if (anyOtherMergeOption) |
There was a problem hiding this comment.
Checking for tracking behavior by parsing expression strings is fragile and error-prone. Consider using the query compilation context or a more robust mechanism to determine tracking behavior rather than string parsing.
| bool isNotTracked = false; | |
| string[] expressionNames = source.Expression.ToString().Split('.'); | |
| if ( | |
| expressionNames.Any(c => c.Contains(nameof(EntityFrameworkQueryableExtensions.AsNoTracking))) | |
| || expressionNames.Any(c => c.Contains(nameof(EntityFrameworkQueryableExtensions.AsNoTrackingWithIdentityResolution))) | |
| || expressionNames.Any(c => c.Contains(nameof(EntityFrameworkQueryableExtensions.IgnoreAutoIncludes))) | |
| ) | |
| { | |
| isNotTracked = true; | |
| } | |
| if (isNotTracked) | |
| { | |
| throw new InvalidOperationException(CoreStrings.RefreshNonTrackingQuery); | |
| } | |
| MergeOption[] otherMergeOptions = Enum.GetValues<MergeOption>().Where(v => v != mergeOption).ToArray(); | |
| bool anyOtherMergeOption = expressionNames.Any(c => otherMergeOptions.Any(o => c.Contains(o.ToString()))); | |
| if (anyOtherMergeOption) | |
| static bool HasNonTrackingOrIgnoreAutoIncludes(Expression expression) | |
| { | |
| Expression? current = expression; | |
| while (current is MethodCallExpression call) | |
| { | |
| var method = call.Method; | |
| if (method.DeclaringType == typeof(EntityFrameworkQueryableExtensions)) | |
| { | |
| var name = method.Name; | |
| if (name == nameof(AsNoTracking) | |
| || name == nameof(AsNoTrackingWithIdentityResolution) | |
| || name == nameof(IgnoreAutoIncludes)) | |
| { | |
| return true; | |
| } | |
| } | |
| current = call.Arguments.Count > 0 | |
| ? call.Arguments[0] | |
| : null; | |
| } | |
| return false; | |
| } | |
| static bool HasOtherMergeOptions(Expression expression, MergeOption[] otherMergeOptions) | |
| { | |
| Expression? current = expression; | |
| while (current is MethodCallExpression call) | |
| { | |
| var method = call.Method; | |
| if (method.DeclaringType == typeof(EntityFrameworkQueryableExtensions) | |
| && method.Name == nameof(Refresh) | |
| && call.Arguments.Count >= 2 | |
| && call.Arguments[1] is ConstantExpression constant | |
| && constant.Type == typeof(MergeOption) | |
| && constant.Value is MergeOption option | |
| && otherMergeOptions.Contains(option)) | |
| { | |
| return true; | |
| } | |
| current = call.Arguments.Count > 0 | |
| ? call.Arguments[0] | |
| : null; | |
| } | |
| return false; | |
| } | |
| var expression = source.Expression; | |
| if (HasNonTrackingOrIgnoreAutoIncludes(expression)) | |
| { | |
| throw new InvalidOperationException(CoreStrings.RefreshNonTrackingQuery); | |
| } | |
| MergeOption[] otherMergeOptions = Enum.GetValues<MergeOption>().Where(v => v != mergeOption).ToArray(); | |
| if (HasOtherMergeOptions(expression, otherMergeOptions)) |
not for review
roji
force-pushed
the
main
branch
2 times, most recently
from
249ae47 to
6b86657
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 11 comments.
Files not reviewed (1)
- src/EFCore/Properties/CoreStrings.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
src/EFCore/Properties/CoreStrings.Designer.cs:3493
CoreStringsdefinesRefreshNonTrackingQuerytwice in this file (another definition exists earlier around line ~2919). This will cause a compile error due to duplicate members; remove the duplicate and regenerate the designer from the .resx if needed.
src/EFCore/Properties/CoreStrings.Designer.cs:3499CoreStringsdefinesRefreshMultipleMergeOptionstwice in this file (another definition exists earlier around line ~2913). This will cause a compile error due to duplicate members; remove the duplicate and regenerate the designer from the .resx if needed.
| entityType); | ||
|
|
||
| /// <summary> | ||
| /// Navigation '{1_entityType}.{0_navigationName}' was not found. Please add the navigation to the entity type using HasOne, HasMany, or OwnsOne/OwnsMany methods before configuring it. |
There was a problem hiding this comment.
The XML doc comment for CanOnlyConfigureExistingNavigations contains two different sentences for the same message (the new one and the original one). This looks like an accidental duplication/merge artifact; keep only one consistent wording.
| /// Navigation '{1_entityType}.{0_navigationName}' was not found. Please add the navigation to the entity type using HasOne, HasMany, or OwnsOne/OwnsMany methods before configuring it. |
| // converting the collection to an array if it isn't already one to ensure consistent caching. Fixes #37112. | ||
| // #37212 may be a possible future solution providing broader capabilities around parameterizing collections. | ||
| arguments: [source.Expression, Expression.Constant(filterKeys is string[] ? filterKeys : filterKeys.ToArray())])) | ||
| arguments: [source.Expression, Expression.Constant(filterKeys)])) |
There was a problem hiding this comment.
IgnoreQueryFilters now captures filterKeys directly in a constant expression. Since ExpressionEqualityComparer only does structural equality for IStructuralEquatable (e.g., arrays), passing a List/HashSet will cause cache-key fragmentation (and can reintroduce the issue mentioned in the removed comment). Consider normalizing to a string array (e.g., filterKeys as array or filterKeys.ToArray()) before creating the constant.
| arguments: [source.Expression, Expression.Constant(filterKeys)])) | |
| arguments: | |
| [ | |
| source.Expression, | |
| Expression.Constant( | |
| filterKeys is string[] filterKeysArray | |
| ? filterKeysArray | |
| : filterKeys.ToArray()) | |
| ])) |
| /// Specifies that the current Entity Framework LINQ query should refresh already loaded objects with the specified merge option. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type of entity being queried.</typeparam> | ||
| /// <param name="source">The source query.</param> | ||
| /// <param name="mergeOption">The MergeOption</param> | ||
| /// <returns>A new query annotated with the given tag.</returns> |
There was a problem hiding this comment.
The XML docs for Refresh appear to be copy/pasted from tagging: the <returns> text says "annotated with the given tag" and the parameter docs are very terse. Please update the docs to accurately describe the operator and its semantics.
| /// Specifies that the current Entity Framework LINQ query should refresh already loaded objects with the specified merge option. | |
| /// </summary> | |
| /// <typeparam name="T">The type of entity being queried.</typeparam> | |
| /// <param name="source">The source query.</param> | |
| /// <param name="mergeOption">The MergeOption</param> | |
| /// <returns>A new query annotated with the given tag.</returns> | |
| /// Configures the current Entity Framework LINQ query to refresh already loaded entities in the context using the specified | |
| /// <see cref="MergeOption" /> when the query is executed. | |
| /// </summary> | |
| /// <typeparam name="T">The type of entity being queried.</typeparam> | |
| /// <param name="source">The source query on which the refresh behavior is being configured.</param> | |
| /// <param name="mergeOption">The merge option that determines how values from the database are applied to tracked entities.</param> | |
| /// <returns>A new query that will refresh tracked entities according to the specified merge option when materialized.</returns> |
| /// 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 IEnumerable<EntityEntry> GetEntriesForState( | ||
| bool added = false, | ||
| bool modified = false, | ||
| bool deleted = false, | ||
| bool unchanged = false) | ||
| { |
There was a problem hiding this comment.
The XML param docs for these new APIs contain multiple incorrect state references (e.g., "Modified.Added" / "Modified.Deleted") and the docs don't mention that this does not call DetectChanges (unlike Entries()), so results can be stale under snapshot tracking unless changes are otherwise detected. Please fix the state names and clarify the DetectChanges behavior (or call TryDetectChanges() for consistency).
| /// 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 IEnumerable<EntityEntry> GetEntriesForState( | |
| bool added = false, | |
| bool modified = false, | |
| bool deleted = false, | |
| bool unchanged = false) | |
| { | |
| /// Returns tracked entities that are in a given state from a fast cache. | |
| /// </summary> | |
| /// <param name="added">Entities in <see cref="EntityState.Added" /> state.</param> | |
| /// <param name="modified">Entities in <see cref="EntityState.Modified" /> state.</param> | |
| /// <param name="deleted">Entities in <see cref="EntityState.Deleted" /> state.</param> | |
| /// <param name="unchanged">Entities in <see cref="EntityState.Unchanged" /> state.</param> | |
| /// <returns>An entry for each entity that matches the specified state flags.</returns> | |
| public IEnumerable<EntityEntry> GetEntriesForState( | |
| bool added = false, | |
| bool modified = false, | |
| bool deleted = false, | |
| bool unchanged = false) | |
| { | |
| TryDetectChanges(); |
| && typeof(string[]).Equals(factory.GetParameters()[0].ParameterType); | ||
| } | ||
|
|
||
| // Used by EF tooling without any Hosting references. Looses some return type safety checks. |
There was a problem hiding this comment.
Correct spelling in comment: "Looses" should be "Loses".
| // Used by EF tooling without any Hosting references. Looses some return type safety checks. | |
| // Used by EF tooling without any Hosting references. Loses some return type safety checks. |
| <value>Unable to refresh query with multiple merge options!</value> | ||
| </data> | ||
| <data name="RefreshNonTrackingQuery" xml:space="preserve"> | ||
| <value>Unable to refresh non-tracking query!</value> |
There was a problem hiding this comment.
These resource messages are user-facing but are very generic and include an exclamation point, which is inconsistent with other EF Core exception messages and makes them less actionable. Consider rewording to explain the exact invalid usage (e.g., Refresh cannot be used on non-tracking queries / cannot be applied more than once).
| <value>Unable to refresh query with multiple merge options!</value> | |
| </data> | |
| <data name="RefreshNonTrackingQuery" xml:space="preserve"> | |
| <value>Unable to refresh non-tracking query!</value> | |
| <value>The query cannot be refreshed because it specifies multiple merge options.</value> | |
| </data> | |
| <data name="RefreshNonTrackingQuery" xml:space="preserve"> | |
| <value>Refresh cannot be used on non-tracking queries.</value> |
| @@ -0,0 +1,367 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
| // The .NET Foundation licenses this file to you under the MIT license. | |||
|
|
|||
There was a problem hiding this comment.
This file currently only imports Microsoft.EntityFrameworkCore.Internal and Microsoft.Extensions.Hosting, but it uses many types from System, System.Linq, System.Reflection, System.Diagnostics, and System.Threading. As written, it will not compile (e.g., Assembly, BindingFlags, Debugger, DiagnosticListener, Thread are undefined). Add the missing using directives (or fully-qualify the types) and remove any unused usings.
| using System; | |
| using System.Diagnostics; | |
| using System.Linq; | |
| using System.Reflection; | |
| using System.Threading; |
| /// <returns>The total number of rows updated in the database.</returns> | ||
| [DynamicDependency( | ||
| "ExecuteUpdate``1(System.Linq.IQueryable{``0},System.Collections.Generic.IReadOnlyList{System.Runtime.CompilerServices.ITuple})", | ||
| "ExecuteUpdate``1(System.Linq.IQueryable{``1},System.Collections.Generic.IReadOnlyList{ITuple})", |
There was a problem hiding this comment.
The DynamicDependency signature string must exactly match the target method. Here it uses the wrong generic parameter index (1 instead of 0) and drops the System.Runtime.CompilerServices namespace for ITuple, which can break trimming/AOT scenarios. Update the string to match the actual private ExecuteUpdate``1(IQueryable{``0}, IReadOnlyList{System.Runtime.CompilerServices.ITuple}) overload.
| "ExecuteUpdate``1(System.Linq.IQueryable{``1},System.Collections.Generic.IReadOnlyList{ITuple})", | |
| "ExecuteUpdate``1(System.Linq.IQueryable{``0},System.Collections.Generic.IReadOnlyList{System.Runtime.CompilerServices.ITuple})", |
| public IEnumerable<EntityEntry> GetEntriesForState( | ||
| bool added = false, | ||
| bool modified = false, | ||
| bool deleted = false, | ||
| bool unchanged = false) | ||
| { | ||
| return StateManager.GetEntriesForState(added, modified, deleted, unchanged) | ||
| .Select(e => new EntityEntry(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 IEnumerable<EntityEntry<TEntity>> GetEntriesForState<TEntity>( | ||
| bool added = false, | ||
| bool modified = false, | ||
| bool deleted = false, | ||
| bool unchanged = false) | ||
| where TEntity : class | ||
| { | ||
| return StateManager.GetEntriesForState(added, modified, deleted, unchanged) | ||
| .Where(e => e.Entity is TEntity) | ||
| .Select(e => new EntityEntry<TEntity>(e)); | ||
| } |
There was a problem hiding this comment.
These new GetEntriesForState* methods are public APIs but are not declared virtual, unlike the other public query APIs on ChangeTracker in this file (e.g., Entries() / Entries<TEntity>()). Consider making them virtual to match the existing extensibility surface.
| /// 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. |
There was a problem hiding this comment.
XML doc formatting in this new public enum doesn't match the established style in this repo (indentation with /// ... and line wrapping as seen in e.g. AutoTransactionBehavior). Please align the documentation formatting to keep public API docs consistent.
| /// 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. |
| <value>Unable to refresh query with multiple merge options!</value> | ||
| </data> | ||
| <data name="RefreshNonTrackingQuery" xml:space="preserve"> | ||
| <value>Unable to refresh non-tracking query!</value> |
There was a problem hiding this comment.
These user-facing error messages use exclamation points and are fairly terse/inaccurate (e.g. RefreshNonTrackingQuery is also used when IgnoreAutoIncludes is present). Please reword to match existing CoreStrings style (sentence case, no exclamation) and accurately describe the invalid query shape.
| <value>Unable to refresh query with multiple merge options!</value> | |
| </data> | |
| <data name="RefreshNonTrackingQuery" xml:space="preserve"> | |
| <value>Unable to refresh non-tracking query!</value> | |
| <value>The query used for refresh specifies multiple merge options. Configure the query to use a single merge option.</value> | |
| </data> | |
| <data name="RefreshNonTrackingQuery" xml:space="preserve"> | |
| <value>The query used for refresh must be a tracking query and cannot use 'IgnoreAutoIncludes'.</value> |
| internal static readonly MethodInfo IgnoreQueryFiltersMethodInfo | ||
| = typeof(EntityFrameworkQueryableExtensions).GetTypeInfo().GetDeclaredMethods(nameof(IgnoreQueryFilters)) | ||
| .Where(info => info.GetParameters().Length == 1) | ||
| .First(); | ||
|
|
||
| internal static readonly MethodInfo IgnoreNamedQueryFiltersMethodInfo | ||
| = typeof(EntityFrameworkQueryableExtensions).GetTypeInfo().GetDeclaredMethods(nameof(IgnoreQueryFilters)) | ||
| .Where(info => info.GetParameters().Length == 2) | ||
| .First(); | ||
|
|
||
| /// <summary> | ||
| /// Specifies that the current Entity Framework LINQ query should not have any model-level entity query filters applied. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// See <see href="https://aka.ms/efcore-docs-query-filters">EF Core query filters</see> for more information and examples. | ||
| /// </remarks> | ||
| /// <typeparam name="TEntity">The type of entity being queried.</typeparam> | ||
| /// <param name="source">The source query.</param> | ||
| /// <returns>A new query that will not apply any model-level entity query filters.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="source" /> is <see langword="null" />.</exception> |
There was a problem hiding this comment.
The indentation/formatting of this entire block is inconsistent with the surrounding file (extra leading spaces, misaligned XML docs, misindented #endregion). Please reformat to match the project’s standard 4-space indentation and alignment.
| internal static readonly MethodInfo IgnoreQueryFiltersMethodInfo | |
| = typeof(EntityFrameworkQueryableExtensions).GetTypeInfo().GetDeclaredMethods(nameof(IgnoreQueryFilters)) | |
| .Where(info => info.GetParameters().Length == 1) | |
| .First(); | |
| internal static readonly MethodInfo IgnoreNamedQueryFiltersMethodInfo | |
| = typeof(EntityFrameworkQueryableExtensions).GetTypeInfo().GetDeclaredMethods(nameof(IgnoreQueryFilters)) | |
| .Where(info => info.GetParameters().Length == 2) | |
| .First(); | |
| /// <summary> | |
| /// Specifies that the current Entity Framework LINQ query should not have any model-level entity query filters applied. | |
| /// </summary> | |
| /// <remarks> | |
| /// See <see href="https://aka.ms/efcore-docs-query-filters">EF Core query filters</see> for more information and examples. | |
| /// </remarks> | |
| /// <typeparam name="TEntity">The type of entity being queried.</typeparam> | |
| /// <param name="source">The source query.</param> | |
| /// <returns>A new query that will not apply any model-level entity query filters.</returns> | |
| /// <exception cref="ArgumentNullException"><paramref name="source" /> is <see langword="null" />.</exception> | |
| internal static readonly MethodInfo IgnoreQueryFiltersMethodInfo | |
| = typeof(EntityFrameworkQueryableExtensions).GetTypeInfo().GetDeclaredMethods(nameof(IgnoreQueryFilters)) | |
| .Where(info => info.GetParameters().Length == 1) | |
| .First(); | |
| internal static readonly MethodInfo IgnoreNamedQueryFiltersMethodInfo | |
| = typeof(EntityFrameworkQueryableExtensions).GetTypeInfo().GetDeclaredMethods(nameof(IgnoreQueryFilters)) | |
| .Where(info => info.GetParameters().Length == 2) | |
| .First(); | |
| /// <summary> | |
| /// Specifies that the current Entity Framework LINQ query should not have any model-level entity query filters applied. | |
| /// </summary> | |
| /// <remarks> | |
| /// See <see href="https://aka.ms/efcore-docs-query-filters">EF Core query filters</see> for more information and examples. | |
| /// </remarks> | |
| /// <typeparam name="TEntity">The type of entity being queried.</typeparam> | |
| /// <param name="source">The source query.</param> | |
| /// <returns>A new query that will not apply any model-level entity query filters.</returns> | |
| /// <exception cref="ArgumentNullException"><paramref name="source" /> is <see langword="null" />.</exception> |
| => Reload(GetDatabaseValues(), mergeOption); | ||
|
|
There was a problem hiding this comment.
Reload(MergeOption) accepts any enum value, but the implementation only has special handling for OverwriteChanges and otherwise falls back to PreserveChanges-like behavior. Please validate mergeOption (throw ArgumentOutOfRangeException for unsupported values such as an undefined cast) to avoid surprising behavior.
| => Reload(GetDatabaseValues(), mergeOption); | |
| { | |
| if (mergeOption != MergeOption.OverwriteChanges | |
| && mergeOption != MergeOption.PreserveChanges) | |
| { | |
| throw new ArgumentOutOfRangeException(nameof(mergeOption)); | |
| } | |
| Reload(GetDatabaseValues(), mergeOption); | |
| } |
| /// <param name="concreteEntityTypeVariable">The variable representing the concrete entity type.</param> | ||
| /// <param name="materializationContextVariable">The materialization context variable.</param> | ||
| /// <param name="shaper">The structural type shaper expression.</param> | ||
| /// <returns>An expression that updates the existing entity with database values.</returns> | ||
| private Expression UpdateExistingEntityWithDatabaseValues( | ||
| ParameterExpression entryVariable, | ||
| ParameterExpression concreteEntityTypeVariable, |
There was a problem hiding this comment.
The parameter concreteEntityTypeVariable is never used inside this method. Please remove it from the signature/call sites (or use it if it’s required) to reduce noise and avoid confusion.
| /// <param name="concreteEntityTypeVariable">The variable representing the concrete entity type.</param> | |
| /// <param name="materializationContextVariable">The materialization context variable.</param> | |
| /// <param name="shaper">The structural type shaper expression.</param> | |
| /// <returns>An expression that updates the existing entity with database values.</returns> | |
| private Expression UpdateExistingEntityWithDatabaseValues( | |
| ParameterExpression entryVariable, | |
| ParameterExpression concreteEntityTypeVariable, | |
| /// <param name="materializationContextVariable">The materialization context variable.</param> | |
| /// <param name="shaper">The structural type shaper expression.</param> | |
| /// <returns>An expression that updates the existing entity with database values.</returns> | |
| private Expression UpdateExistingEntityWithDatabaseValues( | |
| ParameterExpression entryVariable, |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Microsoft.EntityFrameworkCore.Internal; | ||
| using Microsoft.Extensions.Hosting; |
There was a problem hiding this comment.
These using directives appear unused in this file. Removing them would also allow reconsidering whether the Microsoft.Extensions.Hosting package reference is necessary at all.
| using Microsoft.Extensions.Hosting; |
| <PackageReference Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" /> | ||
| <PackageReference Include="Microsoft.Extensions.DependencyModel" /> | ||
| <PackageReference Include="Microsoft.Extensions.HostFactoryResolver.Sources" PrivateAssets="All" /> | ||
| <PackageReference Include="Microsoft.Extensions.Hosting" /> |
There was a problem hiding this comment.
This PackageReference adds a new dependency on Microsoft.Extensions.Hosting to EFCore.Design. Given the repo uses centrally-managed Microsoft.Extensions.* preview versions, please confirm this dependency is required (the new HostFactoryResolver currently doesn’t use any types from this package) and, if needed, ensure it aligns with the repo’s versioning strategy to avoid conflicts.
| <PackageReference Include="Microsoft.Extensions.Hosting" /> |
| /// <summary> | ||
| /// Manually added: This is not a part of EF! | ||
| /// https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs | ||
| /// </summary> | ||
| internal sealed class HostFactoryResolver | ||
| { |
There was a problem hiding this comment.
This internal type in a *.Internal namespace is missing the standard internal-API documentation warning comment used throughout the codebase. Please add the internal API doc block to the type (and any exposed members) to match other internal infrastructure types.
| public static IQueryable<T> Refresh<T>( | ||
| this IQueryable<T> source, | ||
| [NotParameterized] MergeOption mergeOption) | ||
| { |
There was a problem hiding this comment.
Refresh is missing the standard null-check pattern used throughout this class (e.g. throwing ArgumentNullException when source is null). Please add the same input validation used by other queryable extensions.
| { | |
| { | |
| if (source is null) | |
| { | |
| throw new ArgumentNullException(nameof(source)); | |
| } |
…EntityFrameworkCore.ApiConsistencyTest.Public_inheritable_apis_should_be_virtual
…peline Microsoft.EntityFrameworkCore.ApiConsistencyTest.Public_inheritable_apis_should_be_virtual
|
@AndriySvyryd the Azure Pipeline checks failed. We don't know why, as these error messages are unrelated to our changes or pull request. Our tests compile and run successfully on our local system without any errors. Could you please investigate and resolve the issue? 🙏 |
|
@DamirLisak It's failing because you added another |
|
@AndriySvyryd where can I find information stating that HostFactoryResolver.cs is the problem? I can't find it anywhere when clicking on the CI links. Committing HostFactoryResolver.cs and the SLN files was an oversight on our part, something we were trying to avoid. But without HostFactoryResolver.cs, ef core isn't compilable at all! Why is it designed this way? I think the best solution would be to create another fork and add all our changes there, even if we can't compile and test it locally. Then we would create a NEW pull request. Wouldn't that be the simpler solution? |
It would have been the next error if you removed the .sln file.
Yes, a new PR would be best. However, I would like to understand why it doesn't compile for you locally when you use build.cmd or test.cmd |
Issue #31819 "DetectChanges slow in long-term contexts" is no longer necessary if all entity classes in the model are defined with a change-tracking strategy other than "ChangeTrackingStrategy.Snapshot." This is controlled in the ProcessModelFinalizing method of the ChangeTrackingStrategyConvention class. DetectChanges is therefore not called when saving because change tracking is performed using the EntityReferenceMap (_entityReferenceMap property), which essentially corresponds to the previous caching mechanism using EntityState dictionaries from EF4/5/6. However, querying this cache was not enabled; instead, users were forced to use ChangeTracker.Entries(), which returned all objects and led to performance problems with large long-term contexts. This change now enables fast access to changed objects using the new GetEntriesForState method in the ChangeTracker class. This, in turn, calls EntityReferenceMap.GetEntriesForState(), which already exists.
Issue #16491: Implemented legacy Merge-Option Feature from EF/4/5/6