dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.82k stars 3.2k forks source link

Support inheritance for owned/property bag/shared-type entity types #9630

Open ajcvickers opened 7 years ago

ajcvickers commented 7 years ago

For example, this from #9536:

public class Friend
{
    public int Id { get; set; }
    public string Name { get; set; }

    public FullAddress Address { get; set; }
}

public class LessThanFriend
{
    public int Id { get; set; }
    public string Name { get; set; }

    public CityAddress Address { get; set; }
}

public class CityAddress
{
    public string Cap { get; set; }
    public string City { get; set; }
}

public class FullAddress : CityAddress
{
    public string Street { get; set; }
}

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<Friend>().OwnsOne(e => e.Address);
    modelBuilder.Entity<LessThanFriend>().OwnsOne(e => e.Address);
}

This is ambiguous as to whether LessThanFriend.CityAddress should be mapped to allow inheritance such that either a CityAddress or a FullAddress could be persisted. Typically, we only map inheritance when both types are in the model. However, having both types in the model as Owned types on different entities perhaps does not have the same semantics--especially when thinking of them like complex types.

Based on triage discussion from #9536, we think we want to support both the simple mapping (just persisting given concrete type, which is what the request on #9536 is for) and the inheritance mapping, which would require, for TPH, and additional discriminator column in the table. We did not come to a final decision on which should be the default, or what the new API will look like to switch between the two.

AndriySvyryd commented 6 years ago

We would need API on ReferenceOwnershipBuilder to declare the derived types. Related to https://github.com/dotnet/efcore/issues/10140

andre-f-paggi commented 6 years ago

It took me so much time to get to this issue.

I am having a problem in which a owned type generates a Discriminator property, even though there is no need for it, since the class that has this property is not derived anywhere else.

Can this be the same issue?

I tried removing it from migration and from the snapshot to test, but it generates a wrong sql statement when acessing the DbContext on that Set.

Is there any way to get around this, without generating the property on the database?

ajcvickers commented 6 years ago

@andre-f-paggi Please file a new issue and include a runnable project/solution or complete code listing that demonstrates the behavior you are seeing.

chris-stormideas commented 5 years ago

@ajcvickers I can look at supplying a runnable solution if it will assist getting it resolved quicker

ajcvickers commented 5 years ago

@chris-stormideas We understand what this issue is about--i.e. inheritance mapping into a shared table is not supported. The feature is currently on the backlog, and not considered very high priority. The best way to influence getting it resolved is probably to make a case for why this specific mapping is so important.

davidroth commented 4 years ago

@ajcvickers

What about collections of owned types, where the owned-collection is polymorphic? For example:

public class Order
{
    public List<Position> Positions { get; set; } // Owned collection
}

public abstract class Position
{ }

public class FancyPosition : Position
{ }

public class StandardPosition : Position
{ }

public class SampleContext : DbContext
{
    public virtual DbSet<Order> Orders { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        modelBuilder.Entity<Order>().OwnsMany(p => p.Positions);
    }
}

Is this implicitly tracked by this issue or would you recommend creating a new issue for this?

ajcvickers commented 4 years ago

@AndriySvyryd Thoughts on polymorphic owned collections?

AndriySvyryd commented 4 years ago

Polymorphic owned collections will be covered by this, but similar to non-collections all derived types will need to be configured explicitly.

digitalpowers commented 4 years ago

Polymorphic owned collections do provide the ability to compose our object model which is really quite nice.
From my perspective having to define all the derived types explicitly isn't a problem at all. Do you guys have an idea when this issue might bubble up in the backlog?

I have a project I have been waiting to migrate over to efcore that currently uses Horrible InHouse Awful ORM (tm) that I wrote years ago much to today me's regret. This is the only feature left that I cant really support with efcore.

AndriySvyryd commented 4 years ago

@digitalpowers For now we can only say that it won't happen this year.

davidroth commented 4 years ago

Thanks for the feedback. If polymorphic owned collections are covered by this issue, I would recommend updating the issue title+description so that users can find this. ATM it is not really clear and looks like it only covers inline owned types. Thanks!

Lobosque commented 4 years ago

@AndriySvyryd Does this issue also cover the case where I explicitly want to set a HasDiscriminator under an owned entity?

Example:


            builder.OwnsOne<AlertPattern>("Pattern", b =>
            {
                b.HasDiscriminator<int>("type")
                    .HasValue<PatternOne>(1)
                    .HasValue<PatternTwo>(2)
                    .HasValue<PatternThree>(3)
                    .HasValue<PatternFour>(4)
                    .HasValue<PatternFive>(5);
            });

In this particular example, AlertPattern is an abstract class and PatternOne and friends inherit it.

AndriySvyryd commented 4 years ago

@Lobosque Yes

drdamour commented 4 years ago

this would be awesome when using Cosmos NoSQL as one of it's advertised selling points is that embedded documents don't have to follow a specific structure/single type. as it is now i have to do weird things like serialize the collection as json with $type magic which feels very dirty

erikrenaud commented 3 years ago

I just hit this problem using the CosmosDB provider. My type has an "Actions" collections which can be "ActionCall", "ActionEmail" types...

Any suggestions to keep my "Cosmosdb document" coherent with what will be possible when this feature comes out ? migarting a cosmosdb is not on the fun side of things...

erikrenaud commented 3 years ago

cific structure/single type. as it is now i have to do weird things like serialize the collection as json with $type magic which feels very dirty

Anyway you can share the code you use to serialize the collection to json with $type magic ?

drdamour commented 3 years ago

Anyway you can share the code you use to serialize the collection to json with $type magic ?

private static readonly JsonSerializerSettings serializerWithType = new JsonSerializerSettings()
{
    TypeNameHandling = TypeNameHandling.Auto
};

//In ON model creating

//Super hack here, json in json is bad
//but EF Core doesn't support owned type hierarchies
e.Property(e => e.Activities)
    .HasConversion(
        v => JsonConvert.SerializeObject(v, serializerWithType),
        v => JsonConvert.DeserializeObject<List<BaseActitity>>(v, serializerWithType)   
    )
    ;
erikrenaud commented 3 years ago

Anyway you can share the code you use to serialize the collection to json with $type magic ?

private static readonly JsonSerializerSettings serializerWithType = new JsonSerializerSettings()
{
    TypeNameHandling = TypeNameHandling.Auto
};

//In ON model creating

//Super hack here, json in json is bad
//but EF Core doesn't support owned type hierarchies
e.Property(e => e.Activities)
    .HasConversion(
        v => JsonConvert.SerializeObject(v, serializerWithType),
        v => JsonConvert.DeserializeObject<List<BaseActitity>>(v, serializerWithType)   
    )
    ;

Thanks DrDAmour, it confirms that you are doing json within json... Because HasConversion ultimatly serializes to-from a string...

Anybody from the EF team know if we could have easily develop (as this issues is not in the tagged for the dotnet 6 timeframe as of right now) a "HasConversion" that generates json elements for the cosmosdb backend, which could resolve to a string version of that json for "string based storage" backends ?

i.e. .HasConversion( v => GetJsonDOMFor(v), v => CreateObjectsFromJsonDOM(v) )

for cosmosdb, it nests the json into the parent json. For sql, it converts the json dom to a string and puts that in the string column...

Does this make any sens ? i am really trying to us EF over CosmosDB without having to revert to using native CosmosDB apis for certain entities...

dstj commented 3 years ago

Sorry for butting in, but I'm following this because it sort of looks similar to the issue I created in the efcore.pg repo where I was reporting that the JSON serialization did not work properly with the TypeHandling.All. Could they related in some way?

Maybe not, and if not, sorry :)

roji commented 3 years ago

@dstj your problem doesn't seem related to this thread - I've answered in https://github.com/npgsql/efcore.pg/issues/1553#issuecomment-773200018.

jon-peel commented 3 years ago

@Lobosque This is exactly what I am trying to do right now. Were you able to find a way to do or achieve this?

Does this issue also cover the case where I explicitly want to set a HasDiscriminator under an owned entity?

Example:


            builder.OwnsOne<AlertPattern>("Pattern", b =>
            {
                b.HasDiscriminator<int>("type")
                    .HasValue<PatternOne>(1)
                    .HasValue<PatternTwo>(2)
                    .HasValue<PatternThree>(3)
                    .HasValue<PatternFour>(4)
                    .HasValue<PatternFive>(5);
            });

In this particular example, AlertPattern is an abstract class and PatternOne and friends inherit it.

erikrenaud commented 3 years ago

Hi @Thorocaine ,

We had too many small problems with EF for CosmosDB dealing with polymorhpism and owned types which were not prioritized for EF 6 so we switched to using the Azure CosmosDB SDK v3 directly and things are great. We did incorporate https://github.com/dahomey-technologies/Dahomey.Json in order to manage system.text.json polymorphism correctly and configured the CosmosDB SDK to use System.text.json.

Weak points (that we lost because of not using EF):

The good things:

In our case, we access documents by their ID 90% of the time, and are not querying much, so we haven`t needed to invest too much in query code.

If ever EF comes back with better support for CosmosDB, polymorphism and owned types, i`ll probably give it a shot again !

Good luck!

jon-peel commented 3 years ago

Thank you @tuath I am using SQL Server for the current project, but I don't think it changes the fact regarding type inheritance in OwnedTypes.

Two options I can think of are:

This is still in the early stages of Dev, so my whole entity structure is likely to change, so that's okay for now.

drdamour commented 3 years ago

@Thorocaine im doing the coverted string, its fine unless you want to linq where that nested structure...poly is needed..patiently waiting

reinux commented 2 years ago
  • (and what I have done) Switch from Owned to Has. This is annoying because I then need to .Inclue it on selects.

Also because it forces you to tightly couple unrelated parts of your code in some common situations.

AndiHahn commented 2 years ago

Any update on this?

Especially with Cosmos Db Provider it's very likely to need inheritance on owned collection types.

ajcvickers commented 2 years ago

@AndiHahn This issue is in the Backlog milestone. This means that it is not planned for the next release (EF Core 7.0). We will re-assess the backlog following the this release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources. Make sure to vote (👍) for this issue if it is important to you.

darthg8r commented 2 years ago

Eeeshhh, this still isn't on the board? It seems like such a basic feature for any kind of non anemic modeling. I constantly bump up against this, search to see if there's an update, and find this issue on GitHub. I've even unvoted and voted for it multiple times. Come on guys, this stinks. Somehow, stored procedure support in EF has gotten more attention, even when it's perfectly viable to drop to ADO.NET to execute.

ComptonAlvaro commented 2 years ago

I would ask in https://github.com/dotnet/efcore/issues/29252.

In sumary, I wanted to have a value object status for an order. I want to follow the state pattern.

Then to map in EF Core I need to configure the status as owned entity. But I need to configure also the derived classes, but I don't know how to do.

Then my question is. Is it possible to configure EF Core for state pattern when the state is a value object? If not, perhaps the another option would be to use the state as entity, but then the domain is affected by the database, and I would like to avoid this option if it would be possible.

Thanks.

ajcvickers commented 2 years ago

@ComptonAlvaro I don't think it's currently possible to do what you are asking.

ComptonAlvaro commented 2 years ago

@ajcvickers Thanks for you help. Then if actually is not possible, perhaps in the future?

And now, which could be an alternative?

1.- Don't use state pattern in the domain 2.- Use state pattern and create one table for each derived classes. But in this case each table would have only one record. 3.- Use state pattern and use state as entity instead of value object? 4.- Use state pattern in the domain, use state as value object in the domain, in the database to have a table for the states and in the application layer convert the state from database to the value object that will be used by the domain. 5.- Perhaps another option.

Thanks.

ajcvickers commented 2 years ago

@ComptonAlvaro Hard to say, since it's your code and your architecture, but my initial impression is to just keep things simple, which probably means not using the state pattern.

marchy commented 1 year ago

Just wanted to post up and example that I think showcases how much benefit having inheritance specified on owned types could have.

For a two-case discriminated union that allows setting two different types, each of which refers to a different entity (referential relation in this case):

public abstract record Target {
    public record ReferenceTarget( EntityReference Reference ) : Target; // style 1: linked entity only
    public record ContentTarget( string ContentID, Content Content ) : Target; // style 2: linked entity and foreign key
}

that would ideally only have the following property defined in the POCO model:

public class SomeDomainModel {
    ...
    public Target Target { get; private set; } // if owned type inheritance was supported
    ...
}

we currently have to work around this by mapping all the properties to DAL-only properties and use a property wrapper that switches on the model and nulls out all the non-shared properties/columns between them.

public class SomeDomainModel {
    ...
    public Target Target {
        get => TargetType switch {
            nameof(Target.ReferenceTarget) => new Target.ReferenceTarget( Reference! ),
            nameof(Target.ContentTarget) => new Target.ContentTarget( ContentID!, Content! ),
            _ => throw new NotSupportedException( $"Unknown target type '{TargetType}'" ),
        };
        set {
            (string, EntityType?, string?, string?, EntityReference?, string?, Content?) persistedValue = value switch {
                Target.ReferenceTarget referenceTarget => (
                    TargetType: nameof(Target.ReferenceTarget),
                    ReferenceType: referenceTarget.Reference.Type,
                    ReferenceProvider: referenceTarget.Reference.Provider,
                    ReferenceID: referenceTarget.Reference.ID,
                    Reference: referenceTarget.Reference,
                    // NOTE: we null out non-common properties of other sub-classes
                    ContentID: null,
                    Content: null
                ),
                Target.ContentTarget contentTarget => (
                    TargetType: nameof(Target.ContentTarget),
                    // NOTE: we null out non-common properties of other sub-classes
                    ReferenceType: null,
                    ReferenceProvider: null,
                    ReferenceID: null,
                    Reference: null,
                    ContentID: contentTarget.ContentID,
                    Content: contentTarget.Content
                ),
                _ => throw new NotSupportedException( $"Unknown identity type '{value.GetType().Name}'" ),
            };
            (TargetType, ReferenceType, ReferenceProvider, ReferenceID, Reference, ContentID, Content) = persistedValue;
        }
    }
    /*DAL*/internal string TargetType { get; private set; }
    // NOTE: composite foreign key
    /*DAL*/internal EntityType? ReferenceType { get; private set; }
    /*DAL*/internal string? ReferenceProvider { get; private set; }
    /*DAL*/internal string? ReferenceID { get; private set; }
    /*DAL*/internal EntityReference? Reference { get; private set; }
    /*DAL*/internal string? ContentID { get; private set; }
    /*DAL*/internal Content? Content { get; private set; }
    ...
}

Note that ReferenceType, ReferenceProvider and ReferenceID are all foreign keys into a composite key of the linked entity, while ContentID is a simple single-column foreign key.

Ideally the EF configuration could be set up so that the underlying columns that get mapped are still a flattened set of the combined sub-class columns similar to what happens for TPH on a non-owned entity:

This would require letting EF know above the known sub-classes (or it discovering them) so that it can "flatten" the hierarchy into nullable columns and add in the discriminator column.

Please note that the code above is required for a single property of our main model (Target), and in practice we have multiple of these in the model alongside other regular properties/columns, and our domain is filled with these patterns where you need a type hierarchy (ie: discriminated union) to accurately model.

For other domain models, we have similar patterns but are okay with them being JSON blobs rather than flattened columns – in which case unfortunately EF doesn't support that either (https://github.com/dotnet/efcore/issues/27779)

edwickensrhenus commented 1 year ago

@ajcvickers

What about collections of owned types, where the owned-collection is polymorphic? For example:

public class Order
{
    public List<Position> Positions { get; set; } // Owned collection
}

public abstract class Position
{ }

public class FancyPosition : Position
{ }

public class StandardPosition : Position
{ }

public class SampleContext : DbContext
{
    public virtual DbSet<Order> Orders { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        modelBuilder.Entity<Order>().OwnsMany(p => p.Positions);
    }
}

Is this implicitly tracked by this issue or would you recommend creating a new issue for this?

I just wanted to register my interest in support for this exact scenario

digitalpowers commented 9 months ago

is any input needed to help decide if this makes the cut for the current release? It is still the only thing preventing me from migrating to efcore and I very much want to :)

chrisc-ona commented 1 month ago

What's the chance we might see this in EF Core 9?

ErikEJ commented 1 month ago

@chrisc-ona None - 9 is done

chrisc-ona commented 1 month ago

@ErikEJ well hopefully we'll have it in 10 then