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

ModelBuilder.Entity does not return generic instance. #31626

Closed
sjb-sjb opened this issue Sep 4, 2023 · 6 comments
Closed

ModelBuilder.Entity does not return generic instance. #31626

sjb-sjb opened this issue Sep 4, 2023 · 6 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@sjb-sjb
Copy link

sjb-sjb commented Sep 4, 2023

Classifying this as a bug rather than a feature request because, arguably, the current behavior is unintuitive.

ModelBuilder.Entity<T>() returns an EntityTypeBuilder<T>, while ModelBuilder.Entity( typeof(T)) returns an EntityTypeBuilder that is not an EntityTypeBuilder<T>. It would make far more sense that both of them return an instance of EntityTypeBuilder<T>.

Because of this, if one does not know T at compile time then it is difficult to get an instance of EntityTypeBuilder<T>. This actually is a live issue for me as I would like to use reflection to call a method similar to IEntityTypeConfiguration<TEntity>.Configure and pass the generic instance. The reason for doing this myself instead of calling ApplyConfigurationsFromAssembly is primarily that ApplyConfigurationsFromAssembly doesn't call base classes before derived classes and doesn't pass the ModelBuilder.

It is not very easy to get the Entity<T> method by reflection. The workaround is:

IEnumerable<MethodInfo> entityMethods = modelBuilder.GetType().GetMember(nameof(ModelBuilder.Entity),BindingFlags.Public|BindingFlags.Instance).Where( m => m is MethodInfo).Cast<MethodInfo>();
MethodInfo entityMethod = entityMethods.Single(em => em.IsGenericMethod && em.GetParameters().Count() == 0 && em.GetGenericArguments().Count() == 1).MakeGenericMethod(clrType);
EntityTypeBuilder entityTypeBuilder = entityMethod.Invoke(modelBuilder, null) as EntityTypeBuilder ?? throw new InvalidOperationException();
Debug.Assert( entityTypeBuilder.GetType().IsAssignableTo( typeof(EntityTypeBuilder<>).MakeGenericType(clrType)));
@roji
Copy link
Member

roji commented Sep 4, 2023

I might be misunderstanding what you're asking, if the Entity() method isn't generic (i.e. it accepts a Type parameter and has no generic type parameters), then it cannot return an EntityTypeBuilder in its signature. It could return a non-generic EntityTypeBuilder in its signature, and in actuality return a generic EntityTypeBuilder which extends the non-generic EntityTypeBuilder, but the consumer would then have to perform the down-cast from EntityTypeBuilder to EntityTypeBuilder. And at that point, why isn't the consumer just calling the generic Entity() method in the first place?

Can you please provide the actual use-case scenario where you want to call a non-generic Entity() - passing a Type argument - and need a generic EntityTypeBuilder to be returned?

@sjb-sjb
Copy link
Author

sjb-sjb commented Sep 5, 2023

Hi @roji

Correct, although the return signature is EntityTypeBuilder I believe it should return an instance of EntityTypeBuilder<TEntity>. This would be useful because you can then call by reflection a method whose parameter has type EntityTypeBuilder<T>. An example would be calling a method implementing IEntityTypeConfiguration<TEntity>.Configure( EntityTypeBuilder<TEntity>). You can obtain and call that method easily by reflection.

In fact my actual case scenario is that I am calling a static method OnModelCreating( ModelBuilder mb, EntityTypeBuilder<TEntity>) that is in each entity class. I get the OnModelCreating method by reflection, passing typeof(EntityTypeBuilder<>).MakeGenericType( entityType) for the 2nd parameter type.

This is very much in the spirit of calling IEntityTypeConfiguration<TEntity>.Configure except that I want to do it in base-class-first order and I want to pass in the ModelBuilder. The model builder is needed in some cases where I need to fiddle with the relationships on the entity. I want to do it in base-first order because otherwise there is no predictability in whether the base model has been configured or not at the time of configuring the derived class.

If IEntityTypeConfiguration<TEntity>.Configure were being called in base-class-first order, and if the ModelBuilder could be obtained from EntityTypeBuilder<TEntity> then all of this would be unnecessary. Those changes could be made without breaking anything, I believe.

Steve

@ajcvickers
Copy link
Contributor

Note from triage: this is something we could do in many places, and we have discussed doing so before, but ultimately the result is almost always still using Reflection at some point. It might be that you have a special case where that's not true, but it's a buig change with a perf impact, so we're not going to change the behavior for your case.

@sjb-sjb
Copy link
Author

sjb-sjb commented Sep 8, 2023

Understandable. @ajcvickers maybe an optional flag parameter that the user can pass to ApplyConfigurationsFromAssembly that would cause it to sort the Configure calls in base-class-first order?

About getting the model builder from the entity type builder, I’m not familiar with the internal structure but it seems plausible to have a pointer back to the model builder from the entity type builder.

@AndriySvyryd
Copy link
Member

Understandable. @ajcvickers maybe an optional flag parameter that the user can pass to ApplyConfigurationsFromAssembly that would cause it to sort the Configure calls in base-class-first order?

See #22035. That should allow you to implement the base-class-first order.

About getting the model builder from the entity type builder, I’m not familiar with the internal structure but it seems plausible to have a pointer back to the model builder from the entity type builder.

See #15258. The API design is not final, but it should enable your scenario.

@ajcvickers
Copy link
Contributor

Note from triage: the issues Andriy linked cover the scenario in a more compelling way than returning a generic instance here.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 5, 2023
@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

4 participants