dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.79k stars 3.19k forks source link

Using custom entity Id type results in blocking select query without any error #28040

Closed BenjaminAbt closed 2 years ago

BenjaminAbt commented 2 years ago

File a bug

Include your code

Demo Repo: https://github.com/BenjaminAbt/efcore6-custom-id-type-projection-bug

This example shows entities that use a Guid as a base, but via their own type.

public class TenantEntity
{
    public PlatformTenantId Id { get; set; } = null!;

This type is made known via the conventions.

b.Property(e => e.Id)
   .HasConversion(
     v => v.Value,
     v => new Models.PlatformTenantId(v));

When querying the entire entity, this all works.

var tenants = await context.Tenants.ToListAsync().ConfigureAwait(false);
Console.Write(tenants.Count);

However, when a projection is used, the code blocks permanently without error.

var userProjections = await context.Users
    .Select(u => new
    {
        UserId = u.Id,
        Tenants = u.TenantUserAssociations.Select(ta => new
        {
            TenantId = ta.Tenant.Id,
            TenantName = ta.Tenant.Name
        })
    }).ToListAsync();
Console.Write(userProjections.Count);

The comparison project, which does not use its own types but Guids directly, does not block. https://github.com/BenjaminAbt/efcore6-custom-id-type-projection-bug/tree/main/EFCoreWorks

Include stack traces

None available.

Include provider and version information

EF Core version: 6.0.5 Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: .NET 6 Operating system: Windows 10 IDE: VS Version 17.3.0 Preview 1.0

AndriySvyryd commented 2 years ago

Somehow gets into an infinite loop while populating collections, also in sync.

ajcvickers commented 2 years ago

@BenjaminAbt This is because the PlatformTenantId and PlatformUserId are reference types and hence use reference equality by default. The types should either implement value equality, for example:

    protected bool Equals(PlatformBaseGuid other) => Value.Equals(other.Value);

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (ReferenceEquals(this, obj)) return true;
        if (obj.GetType() != this.GetType()) return false;
        return Equals((PlatformBaseGuid) obj);
    }

    public override int GetHashCode() => Value.GetHashCode();

Or value comparers should be configured, for example:

        b.Property(e => e.Id)
           .HasConversion(
               v => v.Value,
               v => new Models.PlatformUserId(v),
               new ValueComparer<PlatformUserId>(
                   (l, r) => l.Value == r.Value,
                   v => v.Value.GetHashCode(),
                   v => new PlatformUserId(v.Value)));
        b.Property(e => e.Id)
           .HasConversion(
               v => v.Value,
               v => new Models.PlatformTenantId(v),
               new ValueComparer<PlatformTenantId>(
                   (l, r) => l.Value == r.Value,
                   v => v.Value.GetHashCode(),
                   v => new PlatformTenantId(v.Value)));

See Value comparers for more information.

BenjaminAbt commented 2 years ago

@ajcvickers thank you for the detailed clarification.

In principle, I was aware that there was no Equals implemented here; that this triggers the error here I was not aware of. For a better dev experience, can we have at least a warning so that such a potential volatility error is covered?