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

EF8 core: filtering by enum doesn't work with TPH and OfType #32865

Open
karczk-dnv opened this issue Jan 19, 2024 · 2 comments · May be fixed by #32878
Open

EF8 core: filtering by enum doesn't work with TPH and OfType #32865

karczk-dnv opened this issue Jan 19, 2024 · 2 comments · May be fixed by #32878
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression Servicing-consider type-bug
Milestone

Comments

@karczk-dnv
Copy link

karczk-dnv commented Jan 19, 2024

Code

context.Technologies has TPH, discriminator is based on TechnologyType enum (8 different values)

public static class ContextExtensions
{
    public static IQueryable<TechnologyDto> StorageTechnologiesQuery(this AppDbContext context) =>
        context.Technologies
            .Include(x => x.Manufacturer)
            .Where(x => (x.TechnologyType == TechnologyType.StorageA
                    || x.TechnologyType == TechnologyType.StorageB
                    || x.TechnologyType == TechnologyType.StorageC
                    || x.TechnologyType == TechnologyType.StorageD));

    public static IQueryable<TechnologyDto> StorageTechnologyWithDetailsQuery(this AppDbContext context) =>
        StorageTechnologiesQuery(context)
            .Include(x => ((StorageATechnologyDto)x).SomeDetailsX)
            .Include(x => ((StorageATechnologyDto)x).SomeDetailsY)
            .Include(x => ((StorageATechnologyDto)x).SomeDetailsZ)
            .Include(x => ((StorageCTechnologyDto)x).SomeDetailsX)
            .Include(x => ((StorageCTechnologyDto)x).SomeDetailsY)
            .Include(x => ((StorageCTechnologyDto)x).SomeDetailsZ)
            .Include(x => ((StorageDTechnologyDto)x).SomeDetailsY)
                .ThenInclude(x => x.AssociatedData)
                    .ThenInclude(x => x.Manufacturer)
            .Include(x => ((StorageDTechnologyDto)x).SomeDetailsX)
            .Include(x => ((StorageDTechnologyDto)x).SomeDetailsZ)
            .Include(x => ((StorageDTechnologyDto)x).SomeDetailsX)
            .Include(x => ((StorageDTechnologyDto)x).SomeDetailsY);
}

Case:

var entity = await _context.StorageTechnologyWithDetailsQuery()
    .OfType<StorageDTechnologyDto>()
    .FirstOrDefaultAsync(x => x.Id == request.Id, cancellationToken);

Include verbose output

.NET 8 & EF core 8.0.1 produces WHERE 0 = 1

SELECT TOP(1) [t].[Id], [t].[CreatedAt], [t].[CreatedBy], [t].[Description], ...other columns
FROM [Technologies] AS [t]
INNER JOIN [Manufacturers] AS [m] ON [t].[ManufacturerId] = [m].[Id]
LEFT JOIN [StorageDTechnologySomeDetailsY] AS [t0] ON [t].[Id] = [t0].[TechnologyId]
LEFT JOIN [StorageDTechnologySomeDetailsX] AS [t1] ON [t].[Id] = [t1].[TechnologyId]
WHERE 0 = 1

It worked in .NET 7 & EF7

Workaround

Replace

.Where(x => (x.TechnologyType == TechnologyType.StorageA
    || x.TechnologyType == TechnologyType.StorageB
    || x.TechnologyType == TechnologyType.StorageC
    || x.TechnologyType == TechnologyType.StorageD));

by

.Where(x => (x is StorageATechnologyDto
    || x is StorageBTechnologyDto
    || x is StorageCTechnologyDto
    || x is StorageDTechnologyDto));

Include provider and version information

EF Core version: core 8.0.1
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 8.0
Operating system: Windows & Linux
IDE: Visual Studio 2022 17.8.4

@ajcvickers
Copy link
Member

Note for triage: still repros on the latest daily build; regression from EF7.

Daily generates:

SELECT [s].[Id], [s].[TechnologyType]
FROM [StorageDTechnologyDto] AS [s]
WHERE [s].[TechnologyType] = 2 AND [s].[TechnologyType] IN (0, 1, 2, 3)

EF7 generates:

SELECT [s].[Id], [s].[TechnologyType]
FROM [StorageDTechnologyDto] AS [s]
WHERE [s].[TechnologyType] = 2 AND [s].[TechnologyType] IN (0, 1, 2, 3)

Code:

using (var context = new SomeDbContext())
{
    await context.Database.EnsureDeletedAsync();
    await context.Database.EnsureCreatedAsync();

    await context.SaveChangesAsync();
}

using (var context = new SomeDbContext())
{
    var results = context.Set<StorageDTechnologyDto>()
        .OfType<StorageDTechnologyDtoC>()
        .Where(x => (x.TechnologyType == TechnologyType.StorageA
                     || x.TechnologyType == TechnologyType.StorageB
                     || x.TechnologyType == TechnologyType.StorageC
                     || x.TechnologyType == TechnologyType.StorageD))
        .ToList();
}

public class SomeDbContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer(@"Data Source=localhost;Database=One;Integrated Security=True;TrustServerCertificate=true")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<StorageDTechnologyDto>()
            .HasDiscriminator(e => e.TechnologyType)
            .HasValue<StorageDTechnologyDtoA>(TechnologyType.StorageA)
            .HasValue<StorageDTechnologyDtoB>(TechnologyType.StorageB)
            .HasValue<StorageDTechnologyDtoC>(TechnologyType.StorageC)
            .HasValue<StorageDTechnologyDtoD>(TechnologyType.StorageD);
    }
}

public abstract class StorageDTechnologyDto
{
    public int Id { get; set; }
    public TechnologyType  TechnologyType  { get; set; }
}

public class StorageDTechnologyDtoA : StorageDTechnologyDto
{
}

public class StorageDTechnologyDtoB : StorageDTechnologyDto
{
}

public class StorageDTechnologyDtoC : StorageDTechnologyDto
{
}

public class StorageDTechnologyDtoD : StorageDTechnologyDto
{
}

public enum TechnologyType
{
    StorageA,
    StorageB,
    StorageC,
    StorageD
}

@roji roji added type-bug regression area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. labels Jan 22, 2024
@roji
Copy link
Member

roji commented Jan 22, 2024

The regression was introduced in #31046, where SqlExpressionSimplifyingExpressionVisitor was changed to compare SqlConstantExpressions instead of bare .NET values, making it more sensitive to subtle differences between expression nodes.

The problem here is that the OfType() code path creates SqlConstantExpressions for discriminator columns based on the pre-converted value (so enum instead of underlying int (here), whereas in other places in the query pipeline (e.g. regular Where comparison) the SqlConstantExpression gets the underlying type instead; the Type property on the nodes mismatch, leading to the bug.

The root cause is that we don't apply the discriminator's value converter to the (model) values stored in the model; we enabled value converters on discriminator columns in #19650 back in 5.0, but didn't modify OfType() accordingly.

roji added a commit to roji/efcore that referenced this issue Jan 22, 2024
@roji roji linked a pull request Jan 22, 2024 that will close this issue
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jan 22, 2024
roji added a commit to roji/efcore that referenced this issue Jan 22, 2024
@ajcvickers ajcvickers added this to the 8.0.x milestone Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression Servicing-consider type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants