Better exception when using a stale builder

[ajcvickers]

Moved from #12954 filed by @raffaeler

@raffaeler commented 2 days ago
@smitpatel At the end the problem was a mix of two issues:

the message coming from the exception is not clear enough to understand the real origin of the problem

@AndriySvyryd commented a day ago
@raffaeler Calling ToTable on the owned types is a workaround for your issue, it shouldn't be needed, that's why it's not documented.

@raffaeler commented a day ago
@AndriySvyryd ok. But without it I could not make it work. I am not sure whether @smitpatel test cover the case when PhysicalSchema was initially specified by hand to the entity (and thus not propagated to the Owned Type)

@smitpatel commented a day ago
@raffaeler - If you look at the code I posted, I have configured the owner type to map to different schema using ToTable call and yet it is propagated properly to owned entities. The code in SharedTableConvention also does processing for table and schema both. Hence the assumption that schema is not getting propagated is incorret.
At this stage we will need repro to investigate further else there does not seem to be any issue in codebase or documentation.

@raffaeler commented 12 hours ago
@smitpatel I see but there is a huge difference between my code and yours.
I have to manually specify all the mappings because in my case I don't know the entities. Therefore I have to use reflection to map them.
Beyond the fact I created new extension methods to use reflection objects such as PropertyInfo, I manually add every single property without letting the conventions decide.

That said, I tried to add to your code the manual mappings, but I get an internal NullReferenceException:

var entityBuilder = modelBuilder.Entity<Blog>().ToTable("Blogging", "dbo");
var ownershipBuilder1 = modelBuilder.Entity<Blog>().OwnsOne(e => e.HomeAddress);
var ownershipBuilder2 = modelBuilder.Entity<Blog>().OwnsOne(e => e.WorkAddress);
var propertyBuilder = ownershipBuilder1.Property<Address>("City"); // explodes internally in EF

Let's start from this ... do you see that?
Thanks

@smitpatel commented 3 hours ago
var propertyBuilder = ownershipBuilder1.Property("City")
Don't forget that the type of Property should match up.
The cause of issue is more likely that you are specifying table manually but not the schema when configuring owned entities.

@raffaeler commented 2 hours ago •
@smitpatel I am now using your example and I did not modify the entities you defined, so string is correct.
I also tried this statement:

var propertyBuilder = ownershipBuilder1.Property("City");

but I still get a NullReferenceException. At home I did not configure the source code stepping so I am not understanding why this exception get thrown. (In any case I don't expect EFCore throwing a NullReferenceException).

For clarity this is the OnModelCreating method:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    // Configure model
    var entityBuilder = modelBuilder.Entity<Blog>().ToTable("Blogging", "Custom");
    var ownershipBuilder1 = modelBuilder.Entity<Blog>().OwnsOne(e => e.HomeAddress);
    var ownershipBuilder2 = modelBuilder.Entity<Blog>().OwnsOne(e => e.WorkAddress);

    var propertyBuilder = ownershipBuilder1.Property<Address>("City"); // throws NullReferenceException internally
    propertyBuilder.HasColumnName("City");

    //propertyBuilder = ownershipBuilder1.Property(typeof(Guid), $"Id.Shadow");
    //propertyBuilder.Metadata.IsPrimaryKey();
    //ownershipBuilder1.HasForeignKey($"Id.Shadow");
}

[ajcvickers]

@raffaeler We discussed this in triage and we would love to be able to reproduce what you are seeing but so far we have not been able to. Could you please file a project/solution or complete code listing that reproduces the behavior you are seeing, and also the full stack trace of the exception.

[raffaeler]

With regards to the NullReferenceException, I already documented the changes I made to @smitpatel sample.
In any case this is the complete listing.
I installed sqlite provider just because it's easier to setup.

program.cs:

using System;
using System.Linq;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;


// https://github.com/aspnet/EntityFrameworkCore/issues/12954#issuecomment-427083836

namespace TestSchema
{
    public class Program
    {
        public static void Main(string[] args)
        {
            using (var db = new MyContext())
            {
                // Recreate database
                db.Database.EnsureDeleted();
                db.Database.EnsureCreated();

                // Seed database


                db.SaveChanges();
            }

            using (var db = new MyContext())
            {
                // Run queries
                var query = db.Blogs.ToList();
            }

            Console.WriteLine("Program finished.");
        }
    }


    public class MyContext : DbContext
    {
        private static ILoggerFactory LoggerFactory => new LoggerFactory().AddConsole(LogLevel.Trace);

        // Declare DBSets
        public DbSet<Blog> Blogs { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            // Select 1 provider
            optionsBuilder
                //.UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=_ModelApp;Trusted_Connection=True;Connect Timeout=5;ConnectRetryCount=0")
                .UseSqlite("filename=_modelApp.db")
                //.UseInMemoryDatabase(databaseName: "_modelApp")
                .EnableSensitiveDataLogging()
                .UseLoggerFactory(LoggerFactory);
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            // Configure model
            var entityBuilder = modelBuilder.Entity<Blog>().ToTable("Blogging", "Custom");
            var ownershipBuilder1 = modelBuilder.Entity<Blog>().OwnsOne(e => e.HomeAddress);
            var ownershipBuilder2 = modelBuilder.Entity<Blog>().OwnsOne(e => e.WorkAddress);

            var propertyBuilder = ownershipBuilder1.Property<Address>("City"); // exception here
            propertyBuilder.HasColumnName("City");

            //propertyBuilder = ownershipBuilder1.Property(typeof(Guid), $"Id.Shadow");
            propertyBuilder.Metadata.IsPrimaryKey();
            ownershipBuilder1.HasForeignKey($"Id.Shadow");

        }
    }

    public class Blog
    {
        public int Id { get; set; }
        public Address HomeAddress { get; set; }
        public Address WorkAddress { get; set; }
    }

    public class Address
    {
        public string City { get; set; }
    }
}

csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.1</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="2.1.4" />
    <PackageReference Include="Microsoft.Extensions.Logging.Console" Version="2.1.1" />
  </ItemGroup>

</Project>

exception stack trace:

   at Microsoft.EntityFrameworkCore.Metadata.Builders.ReferenceOwnershipBuilder.Property[TProperty](String propertyName)
   at TestSchema.MyContext.OnModelCreating(ModelBuilder modelBuilder) in H:\dev.net\ZZZ-Delete\efcore\TestSchema\TestSchema\Program.cs:line 63
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelCustomizer.Customize(ModelBuilder modelBuilder, DbContext context)
   at Microsoft.EntityFrameworkCore.Infrastructure.RelationalModelCustomizer.Customize(ModelBuilder modelBuilder, DbContext context)
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.CreateModel(DbContext context, IConventionSetBuilder conventionSetBuilder, IModelValidator validator)
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.<>c__DisplayClass5_0.<GetModel>b__1()
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)

[ajcvickers]

@raffaeler This line:

var propertyBuilder = ownershipBuilder1.Property<Address>("City")

says configure the property called City which is of type Address. The City property is of type string, not Address.

[raffaeler]

@ajcvickers sorry, my fault after the tests I made on the other thread.
Specifying string or even no generic type at all, does not change the result:

[ajcvickers]

The issue is holding on to the ownership builder, configuring a second ownership (which in turn makes the type weak) followed by further configuration on the original builder, which now tries to access the weak type as a normal type. Smaller repo:

namespace TestSchema
{
    public class Program
    {
        public static void Main(string[] args)
        {
            using (var db = new MyContext())
            {
                db.Database.EnsureDeleted();
                db.Database.EnsureCreated();
            }
        }
    }

    public class MyContext : DbContext
    {
        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) 
            => optionsBuilder.UseSqlite("filename=_modelApp.db");

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            var ownershipBuilder1 = modelBuilder
                .Entity<Blog>()
                .OwnsOne(e => e.HomeAddress);
            
            modelBuilder
                .Entity<Blog>()
                .OwnsOne(e => e.WorkAddress);

            ownershipBuilder1
                .Property<string>("City"); // exception here
        }
    }

    public class Blog
    {
        public int Id { get; set; }
        public Address HomeAddress { get; set; }
        public Address WorkAddress { get; set; }
    }

    public class Address
    {
        public string City { get; set; }
    }
}

Changing the model building code to this makes it pass:

            modelBuilder
                .Entity<Blog>()
                .OwnsOne(e => e.HomeAddress)
                .Property<string>("City");
            
            modelBuilder
                .Entity<Blog>()
                .OwnsOne(e => e.WorkAddress);

[raffaeler]

This is a nightmare in my use-case because I don't have the model and I have to cycle all the properties by reflection and instruct the model step by step.
For this reason I already created additional extension methods taking propertyinfo objects to configure the model.

[ajcvickers]

Triage: we will generate a better exception for this case that indicates that builders should not be held onto like this and that a new builder should be created when coming back to work on the same part of configuration again.

[AndriySvyryd]

Related to #11108.