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

Improve exception for One-to-many from Keyless Entity Type #24283

Closed
optiks opened this issue Feb 26, 2021 · 13 comments
Closed

Improve exception for One-to-many from Keyless Entity Type #24283

optiks opened this issue Feb 26, 2021 · 13 comments
Assignees
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@optiks
Copy link

optiks commented Feb 26, 2021

Background

Hi team (long time no see!),

We're trying to configure a one-to-many from a Keyless Entity Type -> regular entity, however we're running into a NullReferenceException during model creation.

According to Keyless Entity Types,

They can only contain reference navigation properties pointing to regular entities.

Relationships defines a reference navigation property as,

A navigation property that holds a reference to a single related entity.

In this case our Keyless Entity Type is trying to map a collection navigation property, so our assumption is that's the cause.


Expected Result

If is this is an explicit design decision not to support collection navigation properties on Keyless Entity Types, then the messaging should be improved to state what the problem is (with references to the applicable documentation). Errors like this can take quite a while to investigate, and good messaging greatly improves the customer experience. I note that #20292 was closed, which looks to be of a similar concern.

More broadly:

  • Has the reasoning behind not supporting this use case been documented or discussed anywhere?
  • Is there a plan to support it moving forwards?
  • Is there a suggested workaround?

Many thanks, and looking forward to hearing your thoughts :)

Classes

public class vwContainerSource // Keyless Entity Type
{
    public vwContainerSource()
    {
        ShippingLineContainerTariff = new HashSet<ShippingLineContainerTariff>();
    }

    public int CompanyShippingId { get; set; }
    
    [Required]
    [StringLength(50)]
    public string ContainerNumber { get; set; }
    
    public virtual Company CompanyShipping { get; set; }
    
    public virtual ICollection<ShippingLineContainerTariff> ShippingLineContainerTariff { get; set; }
}

public class ShippingLineContainerTariff // Regular entity
{
    public int ShippingLineContainerTariffId { get; set; }
    
    public int CompanyShippingId { get; set; }
    
    [Required]
    [StringLength(50)]
    public string ContainerNumber { get; set; }
    
    public virtual Company CompanyShipping { get; set; }
}

Mapping

modelBuilder.Entity<vwContainerSource>(entity =>
    entity
        .HasNoKey()
        .ToView("vwContainerSource")
        .HasMany(d => d.ShippingLineContainerTariff)
        .WithOne()
        .HasForeignKey(d => new { d.CompanyShippingId, d.ContainerNumber })
        .HasPrincipalKey(d => new { d.CompanyShippingId, d.ContainerNumber })
    );

Result

System.NullReferenceException: 'Object reference not set to an instance of an object.'
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.SetOrAddForeignKey(ForeignKey foreignKey, InternalEntityTypeBuilder principalEntityTypeBuilder, IReadOnlyList`1 dependentProperties, Key principalKey, String navigationToPrincipalName, Nullable`1 isRequired, Nullable`1 configurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.CreateForeignKey(InternalEntityTypeBuilder principalEntityTypeBuilder, IReadOnlyList`1 dependentProperties, Key principalKey, String navigationToPrincipalName, Nullable`1 isRequired, ConfigurationSource configurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.HasRelationshipInternal(EntityType targetEntityType, Key principalKey, ConfigurationSource configurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.HasRelationship(EntityType principalEntityType, ConfigurationSource configurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Builders.EntityTypeBuilder`1.HasMany[TRelatedEntity](Expression`1 navigationExpression)
   at Redacted.<>c.<OnModelCreatingViews>b__348_0(EntityTypeBuilder`1 entity) in Redacted\Entities.cs:line 1374
   at Microsoft.EntityFrameworkCore.ModelBuilder.Entity[TEntity](Action`1 buildAction)
   at Redacted.OnModelCreatingViews(ModelBuilder modelBuilder) in Redacted\Entities.cs:line 1373
   at Redacted.OnModelCreating(ModelBuilder modelBuilder) in Redacted\Entities.cs:line 1365
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelCustomizer.Customize(ModelBuilder modelBuilder, DbContext context)
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.CreateModel(DbContext context, IConventionSetBuilder conventionSetBuilder)
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.GetModel(DbContext context, IConventionSetBuilder conventionSetBuilder)
   at Microsoft.EntityFrameworkCore.Internal.DbContextServices.CreateModel()
   at Microsoft.EntityFrameworkCore.Internal.DbContextServices.get_Model()
   at Microsoft.EntityFrameworkCore.Infrastructure.EntityFrameworkServicesBuilder.<>c.<TryAddCoreServices>b__7_3(IServiceProvider p)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitFactory(FactoryCallSite factoryCallSite, RuntimeResolverContext context)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScopeCache(ServiceCallSite singletonCallSite, RuntimeResolverContext context)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(ConstructorCallSite constructorCallSite, RuntimeResolverContext context)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScopeCache(ServiceCallSite singletonCallSite, RuntimeResolverContext context)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite callSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine.<>c__DisplayClass1_0.<RealizeService>b__0(ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngine.GetService(Type serviceType, ServiceProviderEngineScope serviceProviderEngineScope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
   at Microsoft.EntityFrameworkCore.DbContext.get_DbContextDependencies()
   at Microsoft.EntityFrameworkCore.DbContext.get_InternalServiceProvider()
   at Microsoft.EntityFrameworkCore.DbContext.get_DbContextDependencies()
   at Microsoft.EntityFrameworkCore.DbContext.get_Model()
   at Microsoft.EntityFrameworkCore.Internal.InternalDbSet`1.get_EntityType()
   at Microsoft.EntityFrameworkCore.Internal.InternalDbSet`1.CheckState()
   at Microsoft.EntityFrameworkCore.Internal.InternalDbSet`1.get_EntityQueryable()
   at Microsoft.EntityFrameworkCore.Internal.InternalDbSet`1.System.Linq.IQueryable.get_Provider()
   at System.Linq.Queryable.Where[TSource](IQueryable`1 source, Expression`1 predicate)
   at Redacted
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)

Include provider and version information

EF Core version: 3.1.5 [this also repros in 3.1.12]
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3
Operating system: Windows 10
IDE: Visual Studio 2019 16.5

@anranruye
Copy link

anranruye commented Feb 26, 2021

There is a conflict between .HasNoKey() and .HasPrincipalKey()(which will define an alternate key).

@optiks
Copy link
Author

optiks commented Feb 26, 2021

There is a conflict between .HasNoKey() and .HasPrincipalKey()(which will define an alternate key).

Thanks for your reply, but that's not the problem: it's the call to .HasMany() that fails (it doesn't even get to the .HasPrincipalKey()).

@anranruye
Copy link

anranruye commented Feb 26, 2021

@optiks
Yes, as the documentation says, keyless entity can only be the independent side in a relationship. Just notice that a child entity can not find its parent if the parent has no key, can't it?

I want to point that, you should use regular entity for vwContainerSource class since it actually has key properties:

modelBuilder.Entity<vwContainerSource>(entity =>
    entity
        .HasKey(d => new { d.CompanyShippingId, d.ContainerNumber })
        //.HasNoKey()
        .ToTable("vwContainerSource")  //vwContainerSource is a database view
        .HasMany(d => d.ShippingLineContainerTariff)
        .WithOne()
        .HasForeignKey(d => new { d.CompanyShippingId, d.ContainerNumber })
        //.HasPrincipalKey(d => new { d.CompanyShippingId, d.ContainerNumber })
    );

Also, you should change your migration file manully if you use migration. Remove the table and foreign key creation code. And create foreign keys for underlying database tables of vwContainerSource view.

@optiks
Copy link
Author

optiks commented Feb 26, 2021

@anangaur Thanks again for your reply.

Yes, as the documentation, keyless entity can only be the independent side in a relationship. Just notice that a child entity can not find its parent if the parent has no key, can't it?

As I mentioned in Expected Result, the bug isn't that this scenario isn't supported, it's that the result of invoking the unsupported path is a NullReferenceException -- a terrible experience which then leads to unnecessary time being spent trying to work out what's going on. I'm sure the EF team is aware of this complaint, as bugs frequently manifest as obscure exceptions.

I want to point that, you should use regular entity for vwContainerSource class since it actually has key properties. Also, you should change your migration file manully if you use migration, by removing the table and foriegn key creation code.

We're already using "regular entities" as a workaround, but it feels like a hack, to me. We want the standard Keyless Entity Type benefits (changes aren't tracked, not discovered by convention, HasNoKey), and I don't really see why having a primary key defined (which isn't even used in the relationship) is necessary?

@lakeman
Copy link

lakeman commented Feb 26, 2021

An sql query that returns both parent and child properties, must be able to step through the flat result set, recognise which rows in the result set map to the same parent object, materialise the child objects and link them to that parent. This is impossible without a key on the parent. Sure, a better error would be good though.

@optiks
Copy link
Author

optiks commented Feb 26, 2021

An sql query that returns both parent and child properties, must be able to step through the flat result set, recognise which rows in the result set map to the same parent object, materialise the child objects and link them to that parent. This is impossible without a key on the parent. Sure, a better error would be good though.

Thanks, good reply. I see the problem, but I also see two potential solutions (although admittedly I haven't thought them through that deeply...):

  1. Two queries (one for the parent, one for the children). I believe EF Core used to use this strategy at times?
  2. Flat query, but uniquely identify the parent at the SQL level (explored below*)

vwContainerSource (P)

CompanyShippingId ContainerNumber Type
1 ABCD1234567 A
1 ABCD1234567 B
1 ABCD1111111 A

ShippingLineContainerTariff (C)

ShippingLineContainerTariffId (PK) CompanyShippingId ContainerNumber Type
1 1 ABCD1234567 Detention
2 1 ABCD1234567 Demurrage

After joining on CompanyShippingId, ContainerNumber:

SELECT P.*, C.*
FROM (
    SELECT (SELECT NEWID()) AS RowGroup, *
    FROM vwContainerSource
) P
LEFT OUTER JOIN ShippingLineContainerTariff C ON P.CompanyShippingId = C.CompanyShippingId AND P.ContainerNumber = C.ContainerNumber
RowGroup P.CompanyShippingId P.ContainerNumber P.Type C.ShippingLineContainerId C.CompanyShippingId C.ContainerNumber C.Type
1 1 ABCD1234567 A 1 1 ABCD1234567 Detention
1 1 ABCD1234567 A 2 1 ABCD1234567 Demurrage
2 1 ABCD1234567 B 1 1 ABCD1234567 Detention
2 1 ABCD1234567 B 2 1 ABCD1234567 Demurrage
3 1 ABCD1111111 A

During materialisation, I'd expect:

  • vwContainerSource/RowGroup1 => [ ShippingLineContainerTariff/1, ShippingLineContainerTariff/2 ]
  • vwContainerSource/RowGroup2 => [ ShippingLineContainerTariff/1, ShippingLineContainerTariff/2 ] // Share the same tracked ShippingLineContainerTariff entities
  • vwContainerSource/RowGroup3 => [ ]

Wouldn't this work?

@ajcvickers
Copy link
Member

@optiks Nice to talk to you again. :-) First, the NullReferenceException is a bug. We should be detecting the situation and throwing a better exception message.

Second, EF Core needs to be able to uniquely identity the parent entity. If such a set of columns exist, then they should be defined as the key for the parent type. It doesn't matter if their is such a key in the database or not; it just needs to provide a unique identifier for each row read.

Once the principal type is no longer keyless, then a relationship can be defined to the child in the normal way. Again, it doesn't matter if that relationship is defined in the database or not.

@optiks
Copy link
Author

optiks commented Feb 26, 2021

@ajcvickers Thanks - better messaging would be greatly appreciated.

Second, EF Core needs to be able to uniquely identity the parent entity. If such a set of columns exist, then they should be defined as the key for the parent type. It doesn't matter if their is such a key in the database or not; it just needs to provide a unique identifier for each row read.

I get that, but by switching to regular entities it means taking on additional behaviours that we don't really want (primarily entity tracking).

  1. Did you see my (no doubt extremely naive) suggestion above?
  2. Is there a way of opting out of the additional behaviours that get tacked on when moving from a Keyless to regular entity? Making vwContainerSource read-only would probably be good enough. Feature request: read only entities #7586

@ajcvickers
Copy link
Member

@optiks If you don't want the entities to be tracked, then use a no-tracking query. We don't recommend using keyless entity types just so they are not tracked.

@optiks
Copy link
Author

optiks commented Feb 26, 2021

@ajcvickers Okie - I'll let it go :-).

The reality is that the view result is not an entity (in fact, for us it represents the unification of three different entities) - so it's more of an ideological complaint than a practical one. Pushing no-tracking to the caller is less-than-ideal, but it works.

@ajcvickers
Copy link
Member

@optiks There is inherently no difference between a CLR typed mapped to a view and a CLR type mapped to a table. Either could be updatable or read-only. Either could have a primary key defined, or not have a primary key defined. And so on. Therefore, we ultimately decided to call them all "entity types", because otherwise people read too much semantics into the naming. For example, "view types" where assumed to be read-only because people usually use read-only views. Likewise, people assumed views could not be mapped to types with keys because views don't usually have keys.

@ajcvickers ajcvickers reopened this Mar 6, 2021
@AndriySvyryd AndriySvyryd changed the title One-to-many from Keyless Entity Type -> regular entity throws a NullReferenceException Improve exception for One-to-many from Keyless Entity Type Sep 17, 2021
@smitpatel smitpatel assigned smitpatel and unassigned AndriySvyryd Sep 17, 2021
@smitpatel
Copy link
Contributor

Error message in current 6.0 branch
System.InvalidOperationException : The keyless entity type 'KeylessCollectionNavigation' cannot be on the principal end of the relationship between 'KeylessCollectionNavigation.Stores' and 'Store'. The principal entity type must have a key.

smitpatel added a commit that referenced this issue Sep 17, 2021
@smitpatel smitpatel added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed poachable labels Sep 17, 2021
@smitpatel
Copy link
Contributor

Adding regression test to main branch. Leaving issue in 6.0 since the bug is fixed in 6.0 already.

@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-rc2 Sep 18, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-rc2, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

No branches or pull requests

6 participants