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

Shared column with different CLR types gets projected out twice #25272

Open
roji opened this issue Jul 16, 2021 · 4 comments
Open

Shared column with different CLR types gets projected out twice #25272

roji opened this issue Jul 16, 2021 · 4 comments

Comments

@roji
Copy link
Member

roji commented Jul 16, 2021

In a TPH hierarchy, if two properties map to the same column, but have differing CLR types, the column gets projected twice.

Minimal repro
await using var ctx = new BlogContext();
await ctx.Database.EnsureDeletedAsync();
await ctx.Database.EnsureCreatedAsync();

_ = ctx.Blogs.ToList();

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }

    static ILoggerFactory ContextLoggerFactory
        => LoggerFactory.Create(b => b.AddConsole().AddFilter("", LogLevel.Information));

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer(@"Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0")
            .EnableSensitiveDataLogging()
            .UseLoggerFactory(ContextLoggerFactory);

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<SpecialBlog1>().Property(b => b.Foo)
            .HasColumnName("foo")
            .HasColumnType("int");
        modelBuilder.Entity<SpecialBlog2>().Property(b => b.Foo)
            .HasColumnName("foo")
            .HasColumnType("int");
    }
}

public class Blog
{
    public int Id { get; set; }
}

public class SpecialBlog1 : Blog
{
    public int Id { get; set; }
    public long Foo { get; set; }
}

public class SpecialBlog2 : Blog
{
    public int Id { get; set; }
    public short Foo { get; set; }
}

SQL:

SELECT [b].[Id], [b].[Discriminator], [b].[foo], [b].[foo]
FROM [Blogs] AS [b]

Originally flagged by @jeuxjeux20 in npgsql/efcore.pg#1897 regarding PG JSON POCOs mapped to the same database jsonb column.

@zejji
Copy link

zejji commented Dec 24, 2022

Is this issue on the roadmap for EF7?

We are using TPH with a hierarchy of classes inheriting from a BaseEvent abstract class for the purposes of event logging. There are a large number of derived classes, and the specific content for each event type is stored in a single JSONB Content column shared among all the derived classes (something that was broken in EF 7 and due to be fixed by #29859). This situation looks similar in principle to that mentioned in issue npgsql/efcore.pg#1897.

Unfortunately, it looks as though for every additional derived event type, the Content column is being selected an additional time, which is not ideal!

Here is a simplified example of the Event hierarchy to make things clearer - note how (i) each concrete child Event class has a different type mapped to the Content property; and (ii) in the OnModelCreating method, these types are all mapped to a single column:

public abstract class Event
{
    public Guid Id { get; set; }
    public DateTime Created { get; set; }
}

public class StockPurchaseEvent : Event
{
    public class Payload
    {
        public Guid StockId { get; set; }
        public int Quantity { get; set; }
        public decimal Price { get; set; }
        public decimal Commission { get; set; }
    }
    public required Payload Content { get; set; }
}

public class StockDividendEvent : Event
{
    public class Payload
    {
        public Guid StockId { get; set; }
        public decimal DividendPerShare { get; set; }
    }
    public required Payload Content { get; set; }
}

// .. more event classes

public class AppDbContext : DbContext
{
    public AppDbContext(DbContextOptions<AppDbContext> options)
        : base(options)
    {
    }

    public DbSet<Event> Events { get; set; } = default!;

    protected override void OnModelCreating(ModelBuilder builder)
    {
        builder.Entity<StockDividendEvent>()
            .Property(e => e.Content)
            .HasColumnType("jsonb")
            .HasColumnName("Content");
        builder.Entity<StockPurchaseEvent>()
            .Property(e => e.Content)
            .HasColumnType("jsonb")
            .HasColumnName("Content");
       // ... more event config

        base.OnModelCreating(builder);
    }
}

Fetching the Events DbSet produces the following SQL:

SELECT e."Id", e."Created", e."Discriminator", e."Content", e."Content", e."Content", e."Content", ... -- more duplicated columns
FROM "Events" AS e

@ajcvickers
Copy link
Member

@zejji

Is this issue on the roadmap for EF7?

EF7 is released, so no. Also, while I can see that this is more of a problem is in your case than for most, this issue still only has two votes (👍) overall. We will re-assess the backlog following the this release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources. Make sure to vote (👍) for this issue if it is important to you.

@defteame
Copy link

defteame commented Jul 6, 2024

Is there a workaround for this? It seems like inheritance in EF Core is always treated as an afterthought, never given the appropriate level of testing or support. To try and fix this, I looked into the newer ToJson functionality — surprise, it doesn't support TPH/discriminators either. This isn't a feature request; it's a glaring bug in existing functionality. The feature was released with documentation, and on paper, it should work without such blatant issues.
As it stands, JSON polymorphism is effectively broken in EF Core. This should be clearly noted in the documentation to prevent anyone from making the mistake of using it in a production environment.

@roji
Copy link
Member Author

roji commented Jul 8, 2024

it's a glaring bug in existing functionality

Although EF shouldn't be projecting the same column multiple times, that's not a bug - at most a performance issue that's going to be pretty minor for most people; note that the issue has received only 8 votes since it was opened in 2021, which is one good reason we haven't prioritized it. I'm definitely surprised a duplicate projection serious enough to consider switching to ToJson() just because of that.

As it stands, JSON polymorphism is effectively broken in EF Core.

It's not broken - it's not supported yet (that's tracked by #34184). But you're right that we should be validating and throwing an informative error if the user tries to do this, and also improve our docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants