Efcore: Handle name resolution using explicit PropertyInfos in builder APIs

[raffaeler]

When building dynamically the model, the overloads taking the expressions are fundamental.
Unfortunately, the modeling APIs exposing Expressions are all typed forcing to use reflection over the methods taking the lambda Expressions:
For example: HasOne(Expression<Func<TEntity, TRelatedEntity>>)

It would be extremely helpful if the builder could expose another overload just taking the untyped Expression (which of course will be validated at runtime.
This would enable the construction of a model every time the types are not known at compile time.

EF Core version: 2.1

[smitpatel]

@raffaeler - The strong type checking is mainly to provide intellisense while writing code inside an IDE. Since you are generating model dynamically, is there any issue in using string based APIs? It would make dynamic code generation easier too.

[raffaeler]

@smitpatel In the specific case of the Expression, there is no really any help in giving the strong type version. Unless you want to explicitly call "Compile" (which is of course not the case in EF), the only reason for strong typed expressions is to verify the lambda types, but I don't see why you should use Expressions when you are in full control of the model.
On the intellisense side, there is really no help at all.

Anyway, in my case I want to use Expressions to avoid any ambiguity over the members I will find on the type at runtime. For example you can find a type with both implicit and explicit interface implementation or other edge cases.

Finally, I am not asking to change the current signatures, but just to add the untyped Expression overload so that it's easier to add dynamic mappings, that's all.

[smitpatel]

I don't think either of your statements are true.
Since the parameter for HasOne is of type Expression<Func<TEntity, TRelatedEntity>>, when you call the API on EntityTypeBuilder<TEntity>, you can member completion in intellisense from TEntity type. Without typed expression, that information is not available. _Intellisense needs typed expression._
Further, compiler infers TRelatedEntity from expression, which makes the ReferenceNavigationBuilder of strong type, which allows to chain further call with intellisense the same way. Also the strong type tells EF what is the target type of the relationship.

In the absence of strongly typed expression,

• intellisense won't show anything useful. Remember, intellisense needs static inference at compile time. Runtime inference to correct type has no benefit to intellisense.
• You would need to provide target type when configuring relationships. That is the reason why all string based overloads for relationship takes type argument.

Adding overloads may or may not work based on how intellisense works. Probably needs to verify API first. See https://github.com/aspnet/EntityFrameworkCore/issues/4117 for more details about having overloads with ambiguity and user experience issues it creates. We don't have anything against adding overloads as long as it doesn't ruin UX for others as most of the users of code first are writing code in VS rather than dynamically generating.

[raffaeler]

@smitpatel The typed overload is usefuel when you are writing a lambda, not the expression:

a => a.B

In the case of the expressions you have to write something like this:

var member = sourceType.GetProperty(propertyName);
var parameter = Expression.Parameter(sourceType, "source");
var body = Expression.MakeMemberAccess(parameter, member);
var lambda = Expression.Lambda(body, parameter);

In this last case you don't know sourceType and propertyName, so the intellisense is just for the "Expression" type, but not for the "sourceType" which you can't know.

Again, all what you said is correct if you know the model. But when you build a model dynamically (because you don't know the model at compile time), the game is totally different.

[smitpatel]

The current state of API is due to most users writing lambda rather than building Expression tree.

[raffaeler]

Indeed, and this is why I asked a new overload.
While many users have the luck to know everything at compile time, this is never the case in the domain of my customer.

[divega]

@raffaeler in case it helps, there are three different types of APIs you can use for building the model:

• Fluent overloads that take typed lambda expressions
• Fluent overloads that take strings
• Metatada API

The idea has always been that if you are doing something dynamically you would want to use the string overloads. Moreover, if you are comfortable enough going low level to build your own expressions you may consider using the metadata API directly.

Having untyped expression (or even MemeberInfo) overloads was always a possiblity but it seemed that given the other options available it would be unnecessary.

We really don't want to have more versions of these API unless it serves a specific purpose. It is possible that we have missed string overloads of some specific methods, or that we simply haven't run into a scenario in which really having untyped expression overloads would be more compelling.

I just want to make sure you have tried the options we already have and if it really doesn't work for you we can spend more time trying to understand why.

[raffaeler]

Thank you @divega

• Lambda Expressions overloads are the one I am using via reflection but, as they are typed, it is a huge work
• Strings. I appreciated the fact that now you can access even non-public properties but there is the risk not to be able to disambiguate edge cases. I would prefer avoiding this risk.
• Metadata API. I am not familiar with the metadata API. _Can you point me to docs/examples building a dynamic model with this API?_ With regards to Expressions, I am very comfortable with them, let's give it a try!

I undertstand the current typed Expression overload is a great solution to let the compiler give you the Expression when typing a a lambda in the code. In any case, having untyped Expressions overloads in addition to the current ones would definitely unlock the standard API to any customization.

[divega]

but there is the risk not to be able to disambiguate edge case

@raffaeler can you please elaborate on this?

[raffaeler]

@divega My research started from a type that have both the explicit and implicit interface implementations.
When defining the relationship with the string overload of HasOne from entity A to entity B (which is implemented both implicitly and explicitly), the builder returns this obscure error:

System.InvalidOperationException: 'The navigation property 'B' cannot be added to the entity type 'A' because the target entity type 'B' is defined in shadow state and navigations properties cannot point to shadow state entities.'

I temporarily workarounded the problem by using the overload taking (type, string) but I am afraid to fall into other ambiguities with, eventually, base classes. (Again, the model is not under my full control).

Having the possibility to point to the correct property with an (untyped) expression, totally eliminates any ambiguity and a lot of headaches in understanding the origin of the error.

[AndriySvyryd]

@raffaeler We don't have docs on Metatada API as it's for advanced use cases only and so far we haven't seen many scenarios where the normal "Fluent API" wasn't enough.

For your case you could do something like this:

C# var foreignKey = entity.HasOne(otherEntity).WithOne().Metadata; foreignKey.HasDependentToPrincipal(navPropertyInfo); foreignKey.HasPrincipalToDependent(otherNavPropertyInfo);

You could configure everything using modelBuilder.Model, but this API is very verbose, so it's best to avoid it if possible.

[raffaeler]

@AndriySvyryd no docs, no fun :) Now I get why I din't know about that.

Since the model will be auto-generated, the verbosiness is not a problem. The main goal is to avoid any possible ambiguity.
Thanks a lot for the snippet, I will give a try.

[raffaeler]

@AndriySvyryd I am trying to use the Metadata API but when I set HasPrincipalToDependent, I get an exception for duplicate foreign key.
Questions:

• is there any convention already applying FKs?
• if yes, how can I disable it?
• can you explain me a bit better how can I use this API? (should I model one to many, many to one, both?)... what is the correct strategy?

[raffaeler]

@ajcvickers I see you removed the 2.2.0 label. Does this mean my request will not be addressed or what? Thanks.

[ajcvickers]

@raffaeler This was marked for investigation in 2.2, but it's not clear to me what action is planned beyond this, so I removed it from the milestone to re-triage.

[AndriySvyryd]

@raffaeler There's currently no easy way of removing conventions, this is tracked by https://github.com/aspnet/EntityFrameworkCore/issues/214.
You could do it by replacing one of our services in the service provider. If you are using SQL Server you would need to derive from SqlServerConventionSetBuilder and register your class as a singleton IConventionSetBuilder. Then override AddConventions and remove the RelationshipDiscoveryConvention from all convention lists it's in, see CoreConventionSetBuilder.

Alternatively you can wrap your code in
```C#
using (modelBuilder.Model.ConventionDispatcher.StartBatch())
{
...
}

This prevents the conventions from running until the end of the using block. Inside you can remove the conflicting FK:
```C#
var conflictingFk = entityType.FindNavigation(navigationName)?.ForeignKey;
if(conflictingFk != null)
{
    conflictingFk.DeclaringEntityType.RemoveForeignKey(conflictingFk.Properties, conflictingFk.PrincipalKey, conflictingFk.PrincipalEntityType);
}

[raffaeler]

Thank you @AndriySvyryd I will give a try and let you know.
With regards to the metadata API, do you have any sample/link/other showing how to model complex relationships? I want to understand if the problem derives from a mistake of mine or the default conventions.

[raffaeler]

@ajcvickers My request can be simplified with an overload taking the PropertyInfo, if this works better for you. Otherwise an untyped Expression should be simple to expose as well.
I had the same problem today when I was trying to distinguish a number of explicitly implemented interface properties.
Thanks

[AndriySvyryd]

@raffaeler The closest would be ForeignKeyTest.cs. Usually it's the conventions doing something you don't expect/want.

[ajcvickers]

Notes from triage:

• We decided to fix the bug whereby the property name, rather than the MemberInfo, is used to determine if the configured property is changing. Assigning to @AndriySvyryd for this.
• At this time, we are not going to add additional API surface here as it's not clear the additional APIs add enough value, especially when compared with adding more decision points in the API surface. We could revisit this based on more feedback from other people.
• If we do add API surface in the future, it would likely be to expose PropertyInfo overloads such that the exact property can be explicitly specified without creating an expression tree. For anyone who really needs these now they can already be implemented fairly easily as extension methods. For example:
  ```C#
  public static ReferenceNavigationBuilder HasOne(
  this EntityTypeBuilder entityTypeBuilder,
  Type relatedType,
  PropertyInfo navigation)
  where TEntity : class
  {
  var parameter = Expression.Parameter(typeof(TEntity));
  var navigationExpression = Expression.Lambda(
  Expression.Property(parameter, navigation),
  parameter);

var builderMethod = entityTypeBuilder.GetType().GetMethods()
    .Single(m => m.Name == nameof(EntityTypeBuilder.HasOne)
                 && m.IsGenericMethodDefinition
                 && m.GetParameters().Length == 1
                 && m.GetParameters()[0].ParameterType.IsGenericType);

builderMethod = builderMethod.MakeGenericMethod(relatedType);

return (ReferenceNavigationBuilder)builderMethod
    .Invoke(entityTypeBuilder, new object[] { navigationExpression });

}
```

[raffaeler]

@ajcvickers Thanks, but in reality the snippet you posted should be far more complex to be usable in this scenario.
Since, for obvious reasons, you are returning ReferenceNavigationBuilder instead of ReferenceNavigationBuilder<TPrincipal, TDependent>, the HasForeignKey and other methods do not take the PropertyInfo, you have to treat everything with Reflection otherwise you end up in declaring the foreign key with the string, which fails to find the right one.
In the untyped API, the ability to specify the PropertyInfo should be everywhere otherwise you are stuck.

[ajcvickers]

@raffaeler Here is a more complete example that shows configuring both navigations and an FK for a one-to-many relationship.
```C#
public static class PropertyInfoExtensions
{
public static CollectionNavigationBuilder HasMany(
this EntityTypeBuilder entityTypeBuilder,
Type relatedType,
PropertyInfo navigation)
where TEntity : class
{
var parameter = Expression.Parameter(typeof(TEntity));
var navigationExpression = Expression.Lambda(
Expression.Convert(
Expression.Property(parameter, navigation),
typeof(IEnumerable<>).MakeGenericType(relatedType)),
parameter);

    var builderMethod = entityTypeBuilder.GetType().GetMethods()
        .Single(m => m.Name == nameof(EntityTypeBuilder.HasMany)
                     && m.IsGenericMethodDefinition
                     && m.GetParameters().Length == 1
                     && m.GetParameters()[0].ParameterType.IsGenericType);

    builderMethod = builderMethod.MakeGenericMethod(relatedType);

    return (CollectionNavigationBuilder)builderMethod
        .Invoke(entityTypeBuilder, new object[] { navigationExpression });
}

public static ReferenceCollectionBuilder WithOne(
    this CollectionNavigationBuilder collectionNavigationBuilder,
    Type relatedType,
    PropertyInfo navigation)
{
    var entityType = collectionNavigationBuilder.GetType().GenericTypeArguments[1];

    var parameter = Expression.Parameter(entityType);
    var navigationExpression = Expression.Lambda(
        Expression.Property(parameter, navigation),
        parameter);

    var builderMethod = collectionNavigationBuilder.GetType().GetMethods()
        .Single(m => m.Name == nameof(CollectionNavigationBuilder.WithOne)
                     && m.GetParameters().Length == 1
                     && m.GetParameters()[0].ParameterType.IsGenericType);

    return (ReferenceCollectionBuilder)builderMethod
        .Invoke(collectionNavigationBuilder, new object[] { navigationExpression });
}

public static ReferenceCollectionBuilder HasForeignKey(
    this ReferenceCollectionBuilder referenceCollectionBuilder,
    Type dependentEntityType, 
    PropertyInfo foreignKeyProperty)
{
    var parameter = Expression.Parameter(dependentEntityType);
    var fkExpression = Expression.Lambda(
        Expression.Convert(
            Expression.Property(parameter, foreignKeyProperty),
            typeof(object)),
        parameter);

    var builderMethod = referenceCollectionBuilder.GetType().GetMethods()
        .Single(m => m.Name == nameof(ReferenceReferenceBuilder.HasForeignKey)
                     && m.GetParameters().Length == 1
                     && m.GetParameters()[0].ParameterType.IsGenericType);

    return (ReferenceCollectionBuilder)builderMethod
        .Invoke(referenceCollectionBuilder, new object[] { fkExpression });
}

}

public class BloggingContext : DbContext
{
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
{
optionsBuilder
.UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=Test;ConnectRetryCount=0");
}

protected override void OnModelCreating(ModelBuilder builder)
{
    var navigationToPosts = typeof(Blog).GetProperty(nameof(Blog.Posts));
    var navigationToBlog = typeof(Post).GetProperty(nameof(Post.Blog));
    var fk = typeof(Post).GetProperty(nameof(Post.BlogId));

    builder
        .Entity<Blog>()
        .HasMany(typeof(Post), navigationToPosts)
        .WithOne(typeof(Blog), navigationToBlog)
        .HasForeignKey(typeof(Post), fk);
}

}
```

[raffaeler]

Thank you @ajcvickers, the snippet is a very useful hint.
I will refactor the extension methods in order to use a different approach in searching the correct overloads as future overloads could retrieve the wrong one.

[raffaeler]

@ajcvickers I made a little step until I faced a similar problems in defining owned types which are more complex. The following EF docs sample resembles my need, but I have to do via reflection and expression of course ...

modelBuilder.Entity<Order>().OwnsOne(
    o => o.ShippingAddress,
    sa =>
        {
            sa.Property(p=>p.Street).HasColumnName("ShipsToStreet");
            sa.Property(p=>p.City).HasColumnName("ShipsToCity");
        });

[raffaeler]

BTW I could create a new extension method to configure each property, but I always have this error:

The entity of type 'abc' is sharing the table 'dbo.abc' with entities of type 'xyz',
but there is no entity of this type with the same key value
 '{Id: d567c047-f7f9-403b-ba53-6e73780f3f48}' that has been marked as 'Added'.'

I did not specify ToTable on the owned type. I can't understand whether the error is saying that data is splitted on different tables (which is not what I want nor I asked) or whatelse.
Thanks

[raffaeler]

A side note. While EF is validating the model, the error messages do not necessarily catch the exact reason of the problem.
For example I was looking for a navigation property conflicting with the owned types, but then I discovered the problem was the call to "HasIndex" that i removed. This solved the problem of removing the default convention but wasted a lot of time
Debugging with sourcelink the EF Core sources, I am afraid my last question is just a misleading error message, as the code throws when it is searching for inherited types which is not the case I am trying to solve.
@AndriySvyryd do you have any hint? Thank you

[AndriySvyryd]

@raffaeler
Instead of
```C#
modelBuilder.Entity().OwnsOne(
o => o.ShippingAddress,
sa =>
{
sa.Property(p=>p.Street).HasColumnName("ShipsToStreet");
sa.Property(p=>p.City).HasColumnName("ShipsToCity");
});

you can use:

```C#
var sa = modelBuilder.Entity<Order>().OwnsOne(o => o.ShippingAddress);
sa.Property(p => p.Street).HasColumnName("ShipsToStreet");
sa.Property(p => p.City).HasColumnName("ShipsToCity");

By default owned types are mapped to the same table as the owner, so you would need to call ToTable on them to map to a separate table.

[raffaeler]

@AndriySvyryd Thank you but this is what I did by writing a new extension method that models the "Property" with untyped Expression.
The problem, as I wrote in the other message, is this one:

The entity of type 'abc' is sharing the table 'dbo.abc' with entities of type 'xyz',
but there is no entity of this type with the same key value
 '{Id: d567c047-f7f9-403b-ba53-6e73780f3f48}' that has been marked as 'Added'.'

• I didn't call ToTable() when defining the owned type, so I don't get why EF should care about the key (that doesn't even exist in the 'xyz' owned type).
• the data should be saved in the same table as I described using the API that you just mentioned (but with my own version that takes a PropertyInfo
• Before saving the entity, all the field of the owned type has been given a value

It looks like the error is just wrong, but maybe you have a better idea...

[AndriySvyryd]

@raffaeler All entity types have a primary key.

When inserting an entity you must also insert all of its owned entities.

If you are still having problems attach a small repro project so I can give a more concrete response.

[raffaeler]

I am trying to get a small repro with no luck.
AFAIK the owned type does not need the PK, but the entity has the PK (which is the guid of the error message I posted).
For example In the docs, the Address model is a class with no PK. As they should share the same table, I don't expect a key is required too.

Are these assumptions wrong? @AndriySvyryd

[AndriySvyryd]

Owned entity types get a PK in shadow state by convention defined on the same properties as the FK to the owner. When mapped to the same table as the owner these keys are not created in the database, but are still present in the model.

[raffaeler]

@AndriySvyryd I changed the state of the owned property to added and it worked.
Please add this (non obvious) detail to the documentation. I could not find anything suggesting this behavior.
Now other exceptions are waiting for me in this painful migration, but thanks!

[AndriySvyryd]

@raffaeler Why wasn't the owned entity in Added state? How did you add the owner to the context?

[raffaeler]

The app has an intermediate layer creating entities, attaching them to the context, setting the state and committing. This layer is working nicely with EF6. I just can see from the tests that this behavior changed with EFCore but didn't investigate the details.
My goal is to create the new data layer without having to change anything on the public facing side of the app.

[AndriySvyryd]

I see, then this is the issue tracked by https://github.com/aspnet/EntityFrameworkCore/issues/10551

[raffaeler]

Let me add a note. While it generally makes sense to start with EF Core on new projects, there are tons of apps that will need to migrate from EF6 or die.
For those apps, it would be extremely important to have a bit of help from your team:

• a guidance on the migration
• some helpers to ease the most common tasks
• a well documented number of things to change to avoid painful surprises
• preserve the EF6 behavior whenever possible (I mean without sacrificying any feature)

The issue on the owned type state could be solved with one of the last two points.

[smitpatel]

We decided to fix the bug whereby the property name, rather than the MemberInfo, is used to determine if the configured property is changing. Assigning to @AndriySvyryd for this.

Fixed in #13611