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.71k stars 3.17k forks source link

Allow queries for mapped abstract base class with no discriminator #23600

Open Luigi6821 opened 3 years ago

Luigi6821 commented 3 years ago

Hi, I am experiencing an issue while building model using EF Core (EF Core 5.0). Console application (NET5) fails when trying to SaveChanges: "AMOS_ADDRESS is not mapped to a Table" as reported in the attached screenshot. Please can you help me in understanding what I am doing wrong? Thanks

       var loggerFactory = LoggerFactory.Create(builder => {
            builder.AddFilter("Microsoft", LogLevel.Debug)
                   .AddFilter("System", LogLevel.Debug)
                   .AddFilter("ConsoleApp4.Program", LogLevel.Debug)
                   .AddConsole();
        });
        var optionBuilder = new DbContextOptionsBuilder();
        optionBuilder.UseLazyLoadingProxies(true);
        optionBuilder.UseLoggerFactory(loggerFactory);

        var modelBuilder = new ModelBuilder();
        modelBuilder.HasDefaultSchema("AMOS");
        modelBuilder.Entity<AMOS_ADDRESS>(b => b.ToTable("ADDRESS"));
        modelBuilder.Entity<AMOS_ADDRESS>().Property(e => e.CODE).HasColumnName("CODE");
        modelBuilder.Entity<AMOS_ADDRESS>().Property(e => e.ALPHACODE).HasColumnName("ALPHACODE");
        modelBuilder.Entity<AMOS_ADDRESS>().Property(e => e.ADDRESSID).HasColumnName("ADDRESSID");
        modelBuilder.Entity<AMOS_ADDRESS>().HasKey(e => e.ADDRESSID);

        modelBuilder.FinalizeModel();
        optionBuilder.UseModel(modelBuilder.Model);

        //optionBuilder.UseInMemoryDatabase(databaseName: "myDb");
        var connection = Microsoft.Data.SqlClient.SqlClientFactory.Instance.CreateConnection();
        connection.ConnectionString = "Data source=localhost;Initial catalog=ABS10.0.30;User ID=amos;Password=voyager";
        connection.Open(); // Works
        // To prove that table exists and connection works
        using (var command = connection.CreateCommand())
        {
            command.CommandText = "SELECT COUNT(1) from AMOS.ADDRESS";
            var count = (int)command.ExecuteScalar();
            Console.WriteLine($"Address count {count}");
        }
        optionBuilder.UseSqlServer(connection);

        var dbContext = new myDbContext(optionBuilder.Options);
        dbContext.Addresses.Add(new AMOS_ADDRESS() { CODE = "001", ADDRESSID = 1, ALPHACODE = "A001" });
        dbContext.Addresses.Add(new AMOS_ADDRESS() { CODE = "002", ADDRESSID = 2, ALPHACODE = "A002" });
        dbContext.Addresses.Add(new AMOS_ADDRESS() { CODE = "003", ADDRESSID = 3, ALPHACODE = "A003" });
        dbContext.Addresses.Add(new AMOS_ADDRESS() { CODE = "004", ADDRESSID = 4, ALPHACODE = "A004" });
        dbContext.SaveChanges(); // FAILS AMOS_ADDRESS is not mapped to a table

        var addresses = dbContext.Addresses;
        Console.WriteLine(addresses.CountAsync().Result); // Fails AMOS_ADDRESS is not mapped to a table

image

roji commented 3 years ago

Can you provide a bit more context on why you're creating your own ModelBuilder in this way? Normal EF Core usage usually depends on conventions which perform additional configuration (see the warning on the ModelBuilder API docs). Any reason you're not building your model in your DbContext's OnModelCreating, as in the docs?

Luigi6821 commented 3 years ago

Hi @roji, This code is a very simple "extract" of the final solution. The final solution is a web-API project where EF is used for managing data (classes are more than 300). In this final solution the model is built only the first time (there is a framework that scans classes and apply configuration based on property attributes), cached and used in any subsequent calls. I remember that I used it in previous EF core releases and it worked...

Regards Luigi

roji commented 3 years ago

@Luigi6821 when the standard mechanism for building the model is used (i.e. via DbContext's OnModelCreating), the model is also built only the first time and then cached - I don't think what you're doing helps performance in any way.

It's also improbable the the code above worked as-is in previous releases - the model builder has no conventions whatsoever configured, but these are generally necessary for proper model building; note the other constructor of ModelBuilder, which accepts a ConventionSet. Setting up the proper ConventionSet is possible (with conventions from your specific provider, from relational and from core), but this really isn't the recommended way to work with EF Core.

May I suggest trying out the standard mechanism (via OnModelCreating), and seeing whether that works for you?

smitpatel commented 3 years ago

Possible cause of error - Relational model. This may be working in 3.1 release. User is creating a conventionless model builder which excludes the convention to build the relational model. A lot of our stack assumes that relational model will be available.

Luigi6821 commented 3 years ago

@roji The standard way (via OnModelCreating) works and model is cached as you correctly stated. Defining the following

var modelBuilder = new ModelBuilder(SqlServerConventionSetBuilder.Build());

works as expected also using external model definition.

A new one if possible... Suppose a model like the following:

public abstract class AMOS_ADDRESS
{ 
    public decimal ADDRESSID { get; set; }

    public string CODE { get; set; }

    public string ALPHACODE { get; set; }
}

public class ADDRESS : AMOS_ADDRESS
{
    public string ADDRESS1 { get; set; }
}

.. and model definition code

        modelBuilder.HasDefaultSchema("AMOS");

        modelBuilder.Entity<AMOS_ADDRESS>(b => b.ToTable("ADDRESS"));
        modelBuilder.Entity<AMOS_ADDRESS>().HasKey(e => e.ADDRESSID);

        modelBuilder.Entity<AMOS_ADDRESS>().Property(e => e.CODE).HasColumnName("CODE");
        modelBuilder.Entity<AMOS_ADDRESS>().Property(e => e.ALPHACODE).HasColumnName("ALPHACODE");
        modelBuilder.Entity<AMOS_ADDRESS>().Property(e => e.ADDRESSID).HasColumnName("ADDRESSID");

        modelBuilder.Entity<ADDRESS>(b => b.ToTable("ADDRESS"));
        modelBuilder.Entity<ADDRESS>().Property(e => e.ADDRESS1).HasColumnName("ADDRESS1");

While EF supports it EF Core adds Discriminator column when using both methods

        using (var dbContext = new DbContext(optionBuilder.Options))
        {
            var sql = dbContext.Set<AMOS_ADDRESS>().ToQueryString(); 
            // SELECT a.ADDRESSID, a.ALPHACODE, a.CODE, a.Discriminator, a.ADDRESS1
            //FROM AMOS.ADDRESS AS a
            var addresses2 = dbContext.Set<ADDRESS>().ToArrayAsync().Result;
            // SELECT a.ADDRESSID, a.ALPHACODE, a.CODE, a.Discriminator, a.ADDRESS1
            //FROM AMOS.ADDRESS AS a
            Console.ReadKey();
        }

It is an expected behavior or it will be changed in future to adhere to today EF (NET Framework) behavior? Thanks in advance

roji commented 3 years ago

@Luigi6821 yes, the discriminator column is added by-design, and AFAIK has been the EF Core behavior since version 1.0 (I also think EF6 did the same) - you probably want to take a look at our docs on inheritance. Without the discriminator, EF cannot know which entity type is stored in a given database row (is it ADDRESS, or another subclass of AMOS_ADDRESS?). An alternative is to use TPT instead of TPH.

Leaving open in case we want to discuss @smitpatel's comment above.

Luigi6821 commented 3 years ago

@roji I believe that EF Core should work as EF (NET Framework) which does not include discriminator if only one concrete class per abstract. In fact the below code (using NET Framework) works without problem (model is same as EF core example)

class Program
{
    static void Main(string[] args)
    {
        var loggerFactory = LoggerFactory.Create(builder => {
            builder.AddFilter("Microsoft", LogLevel.Debug)
                   .AddFilter("System", LogLevel.Debug)
                   .AddFilter("ConsoleApp4.Program", LogLevel.Debug)
                   .AddConsole();
        });

        var modelBuilder = new DbModelBuilder();
        modelBuilder.HasDefaultSchema("AMOS");

        modelBuilder.Entity<AMOS_ADDRESS>().ToTable("ADDRESS", "AMOS");
        modelBuilder.Entity<AMOS_ADDRESS>().HasKey(e => e.ADDRESSID);

        modelBuilder.Entity<AMOS_ADDRESS>().Property(e => e.CODE).HasColumnName("CODE");
        modelBuilder.Entity<AMOS_ADDRESS>().Property(e => e.ALPHACODE).HasColumnName("ALPHACODE");
        modelBuilder.Entity<AMOS_ADDRESS>().Property(e => e.ADDRESSID).HasColumnName("ADDRESSID");

        modelBuilder.Entity<ADDRESS>().ToTable("ADDRESS", "AMOS");
        modelBuilder.Entity<ADDRESS>().Property(e => e.ADDRESS1).HasColumnName("ADDRESS1");

        var connection = System.Data.SqlClient.SqlClientFactory.Instance.CreateConnection();
        connection.ConnectionString = "Data source=localhost;Initial catalog=ABS10.0.30;User ID=amos;Password=voyager";
        connection.Open(); // Works

        var builtModel = modelBuilder.Build(connection).Compile();
        using (var dbContext = new DbContext(connection, builtModel, true))
        {
            var addressesBase = dbContext.Set<AMOS_ADDRESS>().ToArrayAsync().Result;
            var addressesDerived = dbContext.Set<ADDRESS>().ToArrayAsync().Result;
            Console.WriteLine("Press key to end");
            Console.ReadKey();
        }
    }

    public abstract class AMOS_ADDRESS
    {
        public decimal ADDRESSID { get; set; }

        public string CODE { get; set; }

        public string ALPHACODE { get; set; }
    }

    public class ADDRESS : AMOS_ADDRESS
    {
        public string ADDRESS1 { get; set; }
    }
}

Please can you give me hints on how I can do same with EF Core? Thanks

roji commented 3 years ago

Simply do all your model configuration on ADDRESS - don't do modelBuilder.Entity<AMOS_ADDRESS>().ToTable(...). If you don't explicitly map/configure the base class, EF will create a simple, single table without a discriminator.

Luigi6821 commented 3 years ago

@roji Yes it works but I can't do

dbContext.Set<AMOS_ADDRESS>()

Because of "this type is not included in the model for the context"

and most important is that when using EF methods context.Set<AMOS_ADDRESS>() or context.Set<ADDRESS>() I always get ADDRESS instances.

roji commented 3 years ago

dbContext.Set() [...] I always get ADDRESS instances.

I think you said AMOS_ADDRESS is abstract, no? So why would you need to have dbContext.Set<AMOS_ADDRESS>(), and doesn't it make sense for you to always get ADDRESS instances (since it's the only concrete type)?

Basically, if there's more than one possible concrete type in the hierarchy, a discriminator is needed to distinguish between them in the database. Otherwise, you can just map the concrete class directly (regardless of whether it inherits from an abstract base class or not).

Luigi6821 commented 3 years ago

@roji

I think you said AMOS_ADDRESS is abstract, no? So why would you need to have dbContext.Set<AMOS_ADDRESS>(), and doesn't it make sense for you to always get ADDRESS instances (since it's the only concrete type)?

Yes it makes sense to have always ADDRESS as type even if I use dbContext.Set(). I explain what I am doing if you have patience to read it :-). The basic idea is to offer to our (my company) customers possibility to customize our base classes (they consist of more than 400 tables - maybe not all but a bunch of them) adding new columns for usage in their own customizations. Our SDK is distributed with all our classes defined as "abstract" and when system starts (Web API service) framework can understand if a specific class has been inherited by customer or not. In case of not customized entities (most of them) the framework creates for each of them a concrete class (simple inheritance) and "Emit" it. Of course our base logic doesn't know about concrete classes definition and always works using abstract classes. Customer can expand our base logic and use their own classes for their customizations.. We do this because of if customer inherit from a not abstract class EF tries to use discriminator (as you correctly said)

...EF works in this scenario and having always instance of the concrete class (even if using Set()) makes sense.. Thanks in advance

ajcvickers commented 3 years ago

@Luigi6821 I'm very surprised that EF6 allowed creation of a DbSet for an unmapped type. I will do some investigation into how that worked--would it be possible for you to spin up a simple console application showing how this is working with EF6?

Luigi6821 commented 3 years ago

@ajcvickers Please find attached a running example (both EF and EF Core NET 5 console app) EF Core doesn't work :-) ConsoleAppEFTest.zip

PS: EF allows usage of mapped Type because both ADDRESS and AMOS_ADDRESS are mapped but it does not use discriminator column because the concrete class ADDRESS is the only one derived from AMOS_ADDRESS. By contra EF Core does not allow to map an abstract class and concrete one without adding a discriminator column...

smitpatel commented 3 years ago

This seems like TPC mapping.

ajcvickers commented 3 years ago

@Luigi6821 Thanks for the additional info. /cc @AndriySvyryd

Luigi6821 commented 3 years ago

@smitpatel

This seems like TPC mapping.

Yes but mapped to same table.

ajcvickers commented 3 years ago

@Luigi6821 Thanks for the additional info. Putting this on the backlog to consider supporting this pattern in the future.

Luigi6821 commented 3 years ago

@Luigi6821 Thanks for the additional info. Putting this on the backlog to consider supporting this pattern in the future.

@ajcvickers Thanks

Xriuk commented 1 year ago

I don't know if this is related but abstract base classes would be very handy if the base class itself is not needed to be created but only allow to access derived entities:

// Entities
public abstract class Animal{
  public int Id {get; set;}

  [...]
}

public class Cat : Animal{
  [...]
}

public class Dog : Animal{
  [...]
}

// DbContext
public DbSet<Animal> Animals {get; set;}
public DbSet<Cat> Cats {get; set;}
public DbSet<Dog> Dogs {get; set;}

The Animals DbSet would retrieve all the animals and instantiate them as their correct concrete classes based on the discriminator, which would have only 2 values (Cat and Dog).

ajcvickers commented 1 year ago

@Xriuk This is normal TPH mapping. If this isn't working, then please open a new issue and attach a small, runnable project or post a small, runnable code listing that reproduces what you are seeing so that we can investigate.