Skip to content
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

Allow more flexible conditional mapping for TPH and similar #10140

Open
Tracked by #24106 ...
Tarig0 opened this issue Oct 23, 2017 · 20 comments
Open
Tracked by #24106 ...

Allow more flexible conditional mapping for TPH and similar #10140

Tarig0 opened this issue Oct 23, 2017 · 20 comments

Comments

@Tarig0
Copy link

Tarig0 commented Oct 23, 2017

I have the following model

public class BaseType
    {
        public string Code {get;set;}
        public string Type {get;set;}
}
public class ChildType:BaseType
    {
        public ChildType()=> Type = "ExplicitChild"
        public string AdditionalData {get;set;}
}

I am able to create a BaseType with a type value as "Implicit Child" and save it to the database, but when retrieving the information I can not get a reference to the "Implicit Child."

db.Set<BaseType>().ToList();

This is caused by the discriminator column being present in the where clause at all times.

Where [e].[Type] IN (N'ExplicitChild', N'BaseType')

I would expect one of the following

  • Prevent saving of arbitrary discriminator.
  • Don't check the discriminator column at all when using the base set without any constraints. <== preferred for my case

Further technical details

EF Core version: 2.0.0
Database Provider: SqlLite, MSSql
Operating system: Windows 10
IDE:Visual Studio 2017

@ajcvickers
Copy link
Member

@Tarig0 The normal pattern for TPH mapping in EF creates the discriminator property as a shadow property, and also marks it as read-only-after-save, both of which mean that to get to a place where the value can be modified arbitrarily requires some deviation from the norm. This is balanced with EF Core not blocking certain things that can be useful if you know what you are doing--for example, it's sometimes useful to be able to save a different discriminator value to cause a type change, but it's easy to shoot yourself in the foot if you're not careful.

To your first point, we could do some extra validation to prevent arbitrary values being saved, probably with some way to override this if needed, but at this point we don't think there is enough return on investment and it could also have perf implications for the update pipeline.

To your second point, we have talked a lot in the past about making the discriminator pattern just a specific case of more general conditional mapping, which would allow things like null/not null to be used. Since we don't seem to have an issue for this in the backlog I will re-purpose this issue for those cases.

Couple of questions for you:

  • Would an explicit list of discriminator values, instead of just one, work for you instead of the wildcard? In other words, you could say that values "A", "B", and "C" will all result in creation of the same type "X".
  • In such a case, when encountering type "X", which discriminator value would you expect EF to choose when none is set?

@ajcvickers ajcvickers reopened this Oct 23, 2017
@ajcvickers ajcvickers changed the title TPH: Doesn't allow for wildcard discriminator on select Allow more flexible conditional mapping for TPH and similar Oct 23, 2017
@ajcvickers ajcvickers added this to the Backlog milestone Oct 23, 2017
@Tarig0
Copy link
Author

Tarig0 commented Oct 23, 2017

The case I was stretching for was to allow the end user to create new "types" at runtime which would act as the base type, until I could extend it later.

I would most likely end up using an enum, if I have to give a defined list at compile time, and have the base class be the default type. This would restrict me from doing things accidentally, and my previous case but I'm OK with adding a single line of code to a release/patch.

possible syntax for explicit list of discriminators.

typeEnum Type {get;set}

modelBuilder.Entity<BaseClass>().HasDiscriminator(e=>e.Type)
   .HasValue<ExplicitChild>(typeEnum.ExplicitChild)
   .HasValue<BaseClass>(new [] {typeEnum.Child1,typeEnum.Child2}) // or params

@Tarig0
Copy link
Author

Tarig0 commented Oct 23, 2017

on your point on what would I expect when creating a base type, I would expect the discriminator to have to be explicitly defined, before attaching the entity.

Another option is to add a fluent function to the hasdiscriminator path, something like "HasDefaultDiscriminator" or have hasvalue have the overload that takes an ienumerable require a T param to be passed for the default

typeEnum Type {get;set}

modelBuilder.Entity<BaseClass>().HasDiscriminator(e=>e.Type)
   .HasValue<ExplicitChild>(typeEnum.ExplicitChild)
   .HasValue<BaseClass>(typeEnum.Child1,typeEnum.Child2)
   .HasDefaultDiscriminator<BaseClass>(typeEnum.Child1);

modelBuilder.Entity<BaseClass>().HasDiscriminator(e=>e.Type)
   .HasValue<ExplicitChild>(typeEnum.ExplicitChild)
   .HasValue<BaseClass>(default:typeEnum.Child1, typeEnum.Child1,typeEnum.Child2)

@Tarig0
Copy link
Author

Tarig0 commented Apr 9, 2018

I just realized there is nothing stopping me from generating the explicit list at dbcontext construct time, or at program start time.

I would have to gather the data either from configuration or manually query the database.

Either way it would allow me to do things with wild cards out side of EFcores engine.

@Gazzo100uk
Copy link

Would this allow a discriminator to be configured across multiple columns, i.e. a composite discriminator?

@Tarig0
Copy link
Author

Tarig0 commented Sep 24, 2018

Not with how it's been discussed, think that would be in an issue for type conversion.

@ajcvickers
Copy link
Member

Consider the case in #14601 when working on this.

@ajcvickers
Copy link
Member

See also multiple discriminator values in #15365

@schrufygroovy
Copy link

Is there some timeline when this is going to be added?

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Oct 24, 2019

@schrufygroovy If an issue is in the backlog it means that it hasn't been considered for any release yet.

Related: #2594 and #18106

A possible implementation is to have the discriminator typed as Expression<Func<ValueBuffer, IEntityType>>
Also we should store the discriminator condition on the entity type directly.

@gojanpaolo
Copy link

Are there any workarounds to assign multiple discriminator values for a single derived type?

@Tarig0
Copy link
Author

Tarig0 commented Feb 14, 2020

@gojanpaolo The only "work around" is to create the classes for each discriminator value.

@smitpatel
Copy link
Member

Are there any workarounds to assign multiple discriminator values for a single derived type?

During a team meeting today we came up with a work-around. You can use FromSql to remap your table in a way to make it look like known values only.

context.BaseType.FromSql(
"Select Id,... /*other columns */, 
    CASE 
        WHEN Discriminator = N'UnknownValue' THEN N'DerivedTypeValue' 
        ELSE Discriminator
    END AS Dicriminator
FROM BaseTypeTable");

@Tarig0
Copy link
Author

Tarig0 commented Mar 19, 2020

👍 nice fromsql find better yet you can move the sql to a sproc to be able to do updates without updating the code

@alfred-zaki
Copy link

@AndriySvyryd how about something like this cosmos-specific-model-customization HasNoDiscriminator

i tried to use this on my case "SecurityUser" model but got the following exception

The entity type 'SecurityUser' is part of a hierarchy, but does not have a discriminator property configured.

@AndriySvyryd
Copy link
Member

@alfred-zaki If you are using TPH you need some discriminator

@alfred-zaki
Copy link

@AndriySvyryd is there any workaround for my situation using 3.1.7? Seems like this feature not going to be worked on anytime soon if ever!

@SuricateCan
Copy link

@alfred-zaki unfortunately no. You need to declare different classes for each discriminator.
The full feature is implemented in net 5.

@AndriySvyryd
Copy link
Member

@alfred-zaki You could map only one class to the table that will contain all the corresponding properties. Alternatively you could use table sharing and map the extra columns to an owned type.

@alfred-zaki
Copy link

@AndriySvyryd Alternatively you could use table sharing and map the extra columns to an owned type. can you please guide me towards what that might be or look like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants