[WIP] - Shared-Type Entities Update Pipeline #14057

lajones · 2018-12-01T00:21:26Z

DO NOT COMMIT THIS - work in progress.

Wanted to get some feedback on the work on Shared-Type Entity Types (#9914). So far I've updated the Model to have a new AddSharedEntityType() method to be used during OnModelCreating(); updated DbContext to allow specifying a Shared-Type-DbSet which uses that EntityType; and using that you can insert, update and delete those entities (Note: query not implemented yet).

As part of this I created a new service ISharedTypeEntityFinder. This is used by the StateManager, when given an entity instance, to associate that instance with a Shared-Type-EntityType defined during OnModelCreating(). At the moment there is only one implementation of this service - which identifies the instance by expecting it to have a property-indexer (see #13610) which takes a specific (but configurable) argument and which returns the name of the EntityType which it represents; however the service is replaceable - so other implementations are possible.

Added and updated some tests. More functional tests will be needed but wanted to get feedback first.

…able. Can do insert, update and delete but not select yet.

lajones · 2018-12-01T00:24:58Z

src/EFCore/ChangeTracking/Internal/ISharedTypeEntityFinder.cs

+        /// </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);
+    }


Wondering about making this a Chain-Of-Responsibility pattern by adding a NextFinder getter/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.

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.

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.

lajones · 2018-12-01T01:09:17Z

src/EFCore/ChangeTracking/Internal/SelfDescribingIndexPropertyEntityFinder.cs

+                : Model.FindEntityType(entityTypeName);
+
+            return entityType != null && entityType.IsSharedType ? entityType : null;
+        }


Could add a FindSharedTypeEntityType(string name) method on Model and use that instead of the above?

src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs

AndriySvyryd · 2018-12-02T09:00:23Z

src/EFCore/ChangeTracking/Internal/SelfDescribingIndexPropertyEntityFinder.cs

+        ///     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; }


This doesn't need to be writable

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.

One would need to do that in the constructor, allowing to do it afterwards could easily lead to inconsistency in the model.

Also since it's overridable derived classes can just replace it with a different value.

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.

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 DbContextOptions

AndriySvyryd · 2018-12-02T09:04:16Z

src/EFCore/ChangeTracking/Internal/StateManager.cs

@@ -155,7 +157,8 @@ public virtual InternalEntityEntry GetOrCreateEntry(object entity)
            {
                _trackingQueryMode = TrackingQueryMode.Multiple;

-                var entityType = _model.FindRuntimeEntityType(entity.GetType());
+                var entityType = _sharedTypeEntityFinder.FindSharedTypeEntityType(entity)
+                    ?? _model.FindRuntimeEntityType(entity.GetType());


_model.FindRuntimeEntityType(entity.GetType()) can be moved into the SharedTypeEntityFinder implementation and it could be renamed to EntityTypeFinder.
Are you planning to implement other type finding strategies? Is there a tracking issue that lists them?

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 Model which do that in various different ways. Did you want to encapsulate all of that somehow?

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.

You could also implement the strategy that uses a normal property and have the name passed in the constructor or a overridable readonly property.

AndriySvyryd · 2018-12-03T06:43:13Z

src/EFCore/Internal/InternalSharedTypeDbSet.cs

+    ///     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 InternalSharedTypeDbSet<TEntity> : InternalDbSet<TEntity>


You could just add EntityTypeName to InternalDbSet, no need to add another type.

You mean add EntityTypeName to InternalDbSet, then add a constructor that takes the name, assign to EntityTypeName in that constructor and later use the fact that EntityTypeName is set to distinguish when trying to find the EntityType? That would be possible. It's true I didn't like having to make various things protected. to make this work But this seemed cleaner to me - during e.g. debugging I can immediately see what this is representing - but I'm open to the possibility.

Again, I just want to reduce special casing and handling this in one class would result in less code and less code means there are fewer places to introduce bugs.

AndriySvyryd · 2018-12-03T06:52:02Z

src/EFCore/Metadata/Internal/EntityType.cs

+        ///     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 bool IsSharedType { get; set; }


This should be readonly

This was just following the same pattern as used for IsQueryType. See Model.AddQueryType() and then AddSharedTypeEntityType() immediately below it.

Yes, we would need to change IsQueryType too. Changing the value for either after the type is already in the model could leave it in an incosistent state, since it would avoid the checks done in AddSharedType and AddQueryType

Can create a shared-type DbSet so long as the target type is instanti…

662d25e

…able. Can do insert, update and delete but not select yet.

lajones requested review from divega, ajcvickers and AndriySvyryd December 1, 2018 00:21

lajones commented Dec 1, 2018

View reviewed changes

src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs Show resolved Hide resolved

AndriySvyryd reviewed Dec 2, 2018

View reviewed changes

AndriySvyryd reviewed Dec 3, 2018

View reviewed changes

lajones self-assigned this Mar 27, 2019

ajcvickers closed this May 10, 2019

smitpatel deleted the 181130_SharedTypeEntities_07 branch September 9, 2019 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] - Shared-Type Entities Update Pipeline #14057

[WIP] - Shared-Type Entities Update Pipeline #14057

lajones commented Dec 1, 2018 •

edited

lajones Dec 1, 2018 •

edited

AndriySvyryd Dec 2, 2018

lajones Dec 3, 2018

lajones Dec 1, 2018

AndriySvyryd Dec 2, 2018

lajones Dec 3, 2018 •

edited

AndriySvyryd Dec 3, 2018

AndriySvyryd Dec 3, 2018

lajones Dec 3, 2018

AndriySvyryd Dec 4, 2018

AndriySvyryd Dec 2, 2018

lajones Dec 3, 2018 •

edited

AndriySvyryd Dec 3, 2018

AndriySvyryd Dec 3, 2018

AndriySvyryd Dec 3, 2018

lajones Dec 3, 2018

AndriySvyryd Dec 3, 2018

AndriySvyryd Dec 3, 2018

lajones Dec 3, 2018

AndriySvyryd Dec 3, 2018

[WIP] - Shared-Type Entities Update Pipeline #14057

[WIP] - Shared-Type Entities Update Pipeline #14057

Conversation

lajones commented Dec 1, 2018 • edited

lajones Dec 1, 2018 • edited

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

lajones Dec 3, 2018 • edited

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

lajones Dec 3, 2018 • edited

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

lajones commented Dec 1, 2018 •

edited

lajones Dec 1, 2018 •

edited

lajones Dec 3, 2018 •

edited

lajones Dec 3, 2018 •

edited