[WIP] - Shared-Type Entities Update Pipeline #14057
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using JetBrains.Annotations; | ||
| using Microsoft.EntityFrameworkCore.Metadata; | ||
|
|
||
| namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal | ||
| { | ||
| /// <summary> | ||
| /// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
| /// directly from your code. This API may change or be removed in future releases. | ||
| /// </summary> | ||
| public interface ISharedTypeEntityFinder | ||
| { | ||
| /// <summary> | ||
| /// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
| /// directly from your code. This API may change or be removed in future releases. | ||
| /// </summary> | ||
| /// <returns> the shared-type EntityType corresponding to this instance if found in the model, or null if not. </returns> | ||
| IEntityType FindSharedTypeEntityType([NotNull] object entityInstance); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Linq; | ||
| using System.Linq.Expressions; | ||
| using System.Reflection; | ||
| using JetBrains.Annotations; | ||
| using Microsoft.EntityFrameworkCore.Metadata; | ||
| using Microsoft.EntityFrameworkCore.Utilities; | ||
|
|
||
| namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal | ||
| { | ||
| /// <summary> | ||
| /// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
| /// directly from your code. This API may change or be removed in future releases. | ||
| /// </summary> | ||
| public class SelfDescribingIndexPropertyEntityFinder : ISharedTypeEntityFinder | ||
| { | ||
| public const string DefaultEntityTypeNamePropertyName = "__EntityTypeName__"; | ||
|
|
||
| /// <summary> | ||
| /// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
| /// directly from your code. This API may change or be removed in future releases. | ||
| /// </summary> | ||
| public SelfDescribingIndexPropertyEntityFinder([NotNull] IModel model) | ||
| { | ||
| Check.NotNull(model, nameof(model)); | ||
|
|
||
| Model = model; | ||
| EntityTypeNamePropertyName = DefaultEntityTypeNamePropertyName; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
| /// directly from your code. This API may change or be removed in future releases. | ||
| /// </summary> | ||
| public virtual string EntityTypeNamePropertyName { get; [NotNull]set; } | ||
|
There was a problem hiding this comment. This doesn't need to be writable There was a problem hiding this comment. I actually thought was a nice feature, It just allows you to pick a different argument to the indexer - other than "__EntityTypeName__" - which I kind of picked at random. There was a problem hiding this comment. One would need to do that in the constructor, allowing to do it afterwards could easily lead to inconsistency in the model. There was a problem hiding this comment. Also since it's overridable derived classes can just replace it with a different value. There was a problem hiding this comment. It is true that users could get themselves in trouble if they changed the value half way through. I was just trying to make it simple - so they didn't have to write a whole new class just to change that one value. There was a problem hiding this comment. If we really want to make this simple to configure we should add sugar API that doesn't require messing with services at all, perhaps something on |
||
|
|
||
| /// <summary> | ||
| /// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
| /// directly from your code. This API may change or be removed in future releases. | ||
| /// </summary> | ||
| public virtual IModel Model { get; } | ||
|
|
||
| /// <summary> | ||
| /// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
| /// directly from your code. This API may change or be removed in future releases. | ||
| /// </summary> | ||
| /// <returns> the shared-type EntityType corresponding to this instance if found in the model, or null if not. </returns> | ||
| public virtual IEntityType FindSharedTypeEntityType(object entityInstance) | ||
| { | ||
| Check.NotNull(entityInstance, nameof(entityInstance)); | ||
|
|
||
| var efIndexerPropInfo = entityInstance.GetType() | ||
| .GetRuntimeProperties().FirstOrDefault(p => p.IsEFIndexerProperty()); | ||
| if (efIndexerPropInfo == null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| string entityTypeName = null; | ||
| try | ||
| { | ||
| entityTypeName = efIndexerPropInfo | ||
| .GetValue(entityInstance, new[] { EntityTypeNamePropertyName }) as string; | ||
| } | ||
| catch | ||
| { | ||
| // exceptions indicate the indexer does not have the | ||
| // EntityTypeNamePropertyName key etc. | ||
| } | ||
|
|
||
| var entityType = entityTypeName == null | ||
| ? null | ||
| : Model.FindEntityType(entityTypeName); | ||
|
|
||
| return entityType != null && entityType.IsSharedType ? entityType : null; | ||
| } | ||
|
There was a problem hiding this comment. Could add a |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ public class StateManager : IStateManager | |
| private readonly IModel _model; | ||
| private readonly IDatabase _database; | ||
| private readonly IConcurrencyDetector _concurrencyDetector; | ||
| private readonly ISharedTypeEntityFinder _sharedTypeEntityFinder; | ||
|
|
||
| /// <summary> | ||
| /// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
|
|
@@ -72,6 +73,7 @@ public StateManager([NotNull] StateManagerDependencies dependencies) | |
|
|
||
| UpdateLogger = dependencies.UpdateLogger; | ||
| _changeTrackingLogger = dependencies.ChangeTrackingLogger; | ||
| _sharedTypeEntityFinder = dependencies.SharedTypeEntityFinder; | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -155,7 +157,8 @@ public virtual InternalEntityEntry GetOrCreateEntry(object entity) | |
| { | ||
| _trackingQueryMode = TrackingQueryMode.Multiple; | ||
|
|
||
| var entityType = _sharedTypeEntityFinder.FindSharedTypeEntityType(entity) | ||
| ?? _model.FindRuntimeEntityType(entity.GetType()); | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. Not sure I'm following. This service is meant to find Shared-Type EntiyTypes only. I did have an idea for another one of those - which is for a CLR-type which is shared, and has an indexer for the other properties, but could have a known, fixed-name CLR-property for the EntityType name. But I hadn't discussed that with @ajcvickers or @divega yet and I seem to remember one of them had a 3rd idea on how we should do this. Basically I can see people wanting to identify Shared-Type entities in various different ways and wanted to make it as easy as possible to write your own service if you wanted to. But I did not intend to change or make into a service the existing functionality to find EntityTypes using their CLR-type. There are 6 methods already existing on There was a problem hiding this comment. In general I would like to avoid having to special case Shared type entity types as much as possible and here this can by accomplished by making the service look for non-shared types as well. There was a problem hiding this comment. You could also implement the strategy that uses a normal property and have the name passed in the constructor or a overridable readonly property. |
||
| if (entityType == null) | ||
| { | ||
| if (_model.HasEntityTypeWithDefiningNavigation(entity.GetType())) | ||
|
|
||
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.
Wondering about making this a Chain-Of-Responsibility pattern by adding a
NextFindergetter/setter. That way if you are using multiple different patterns of identifying a Shared-Type-EntityType from instance in your context you can set up multiple of these chained together in the order you want to apply them.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.
I don't think that would be necessary, this logic shouldn't be replaced or extended and it shouldn't need to change often. Furthermore it's in a perf-sensitive part of the code and keeping all the patterns for finding the EntityType in one class will be slightly faster.
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.
I can see your point. Don't agree that it shouldn't be replaced or extended. I'd be really surprised if different users of this functionality are completely happy with just the one way of doing it. It was not even agreed among us that the index property was the only way to go. That said I take your point on perf (though we do cache the result of course) and maybe we could wait on Chain-Of-Responsibility til we get feedback from customers.