Add Code First API to configure the discriminator column #2800
Conversation
| daisy.Relational().DiscriminatorValue = PlantGenus.Daisy; | ||
| modelBuilder.Entity<Plant>().Discriminator(p => p.Genus) | ||
| .HasValue(typeof(Rose), PlantGenus.Rose) | ||
| .HasValue(typeof(Daisy), PlantGenus.Daisy); |
There was a problem hiding this comment.
@rowanmiller What do you think about this API?
There was a problem hiding this comment.
Not sure. A few things spring to mind:
- No generics for the derived types.
- Feels more natural to configure the derived via Entity() but I guess you came up with this to keep strong typing.
E.g.
modelBuilder.Entity<Plant>().DiscriminatorProperty(p => p.Genus)
modelBuilder.Entity<Rose>().DiscriminatorValue(PlantGenus.Rose)
cc @divega, @bricelam, @ajcvickers
There was a problem hiding this comment.
- There's no point for specifying derived type via a generic argument, since there's nothing to deduce it from and there's no API downstream that would benefit from it
- Yes, I prefer the value to be strongly typed in the API. You can still add the value from the derived type:
modelBuilder.Entity<Rose>().Discriminator().HasValue(typeof(Rose), PlantGenus.Rose)
We could add sugar for this to make it look like yours
There was a problem hiding this comment.
- We also do it just because it just looks better and is easier to type. E.g.
modelBuilder.Ignore<Foo>(); - Yeah, your example looks kinda strange because its WET :-)
There was a problem hiding this comment.
It would be good to discuss this all up tomorrow in the design meeting.
FWIW, I don't like the API a lot at first glance but I would like to make sure I understand what we are trying to achieve.
There was a problem hiding this comment.
We'll discuss this in the API review. It wouldn't affect the bulk of the commit though.
AndriySvyryd
force-pushed
the
Discriminator
branch
from
0f11939
to
2a99af4
Compare
| // TODO: remove this when the discriminator convention is added | ||
| modelBuilder.Entity<Animal>().Discriminator() | ||
| .HasValue(typeof(Eagle), "Eagle") | ||
| .HasValue(typeof(Kiwi), "Kiwi"); |
There was a problem hiding this comment.
I'm not sure we need the parameterless overload. Since the user will specify the values perhaps we should force them to specify the type explicitly.
There was a problem hiding this comment.
Is this for the case where we introduce a shadow prop?
There was a problem hiding this comment.
So there is another one that takes the shadow prop name and is generic on the type? If so, then this no-arg one could go away.
AndriySvyryd
force-pushed
the
Discriminator
branch
2 times, most recently
from
1bfacda
to
9bd80dd
Compare
| @@ -167,6 +165,8 @@ private bool InheritsFrom(EntityType entityType) | |||
| return false; | |||
| } | |||
|
|
|||
| public virtual EntityType RootType() => (EntityType)((IEntityType)this).RootType(); | |||
There was a problem hiding this comment.
This is unfortunate but can't think of an easy solution.
|
|
Add API for setting annotations on internal builders that returns success status Part of #1704
AndriySvyryd
force-pushed
the
Discriminator
branch
from
9bd80dd
to
be35af1
Compare
Add API for setting annotations on internal builders that returns success status