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

Non-string discriminator properties throw on multiple configuration calls #15553

Closed
jzabroski opened this issue May 1, 2019 · 11 comments
Closed

Comments

@jzabroski
Copy link

jzabroski commented May 1, 2019

Describe what is not working as expected.

Two entities in two separate table-per-hierarchy mappings cannot have the same discriminator name.

Note: In the supplied GitHub project, I have drawn an entity model which provides humanistic view on why this is important and useful.

Workaround (I Hope)

Manually creating unique property names for each table-per-hierarchy mapping works, but is less-than-ideal and worrisome (what have I not uncovered).

If you are seeing an exception, include the full exceptions details (message and stack trace).

Error Message:
 System.InvalidOperationException : The property 'Discriminator' cannot be added to type 'ChildBase' because the type of the corresponding CLR property or field 'ParentChildDiscriminator' does not match the specified type 'string'.
Stack Trace:
   at Microsoft.EntityFrameworkCore.Metadata.Internal.EntityType.AddProperty(String name, Type propertyType, MemberInfo memberInfo, ConfigurationSource configurationSource, Nullable`1 typeConfigurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.EntityType.AddProperty(String name, Type propertyType, ConfigurationSource configurationSource, Nullable`1 typeConfigurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.Property(Property existingProperty, String propertyName, Type propertyType, MemberInfo clrProperty, Nullable`1 configurationSource, Nullable`1 typeConfigurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.Property(String propertyName, Type propertyType, MemberInfo memberInfo, Nullable`1 configurationSource, Nullable`1 typeConfigurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.Property(String propertyName, Type propertyType, ConfigurationSource configurationSource, Nullable`1 typeConfigurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.Property(String propertyName, Type propertyType, ConfigurationSource configurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.RelationalEntityTypeBuilderAnnotations.DiscriminatorBuilder(Func`2 createProperty, Type propertyType)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.DiscriminatorConvention.Apply(InternalEntityTypeBuilder entityTypeBuilder, EntityType oldBaseType)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ImmediateConventionScope.OnBaseEntityTypeChanged(InternalEntityTypeBuilder entityTypeBuilder, EntityType previousBaseType)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.RunVisitor.VisitOnBaseEntityTypeChanged(OnBaseEntityTypeChangedNode node)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ConventionVisitor.VisitConventionScope(ConventionScope node)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ConventionVisitor.VisitConventionScope(ConventionScope node)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ConventionBatch.Run()
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalModelBuilder.Entity(TypeIdentity& type, ConfigurationSource configurationSource, Boolean allowOwned, Boolean throwOnQuery)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalModelBuilder.Entity(Type type, ConfigurationSource configurationSource, Boolean allowOwned, Boolean throwOnQuery)
   at Microsoft.EntityFrameworkCore.ModelBuilder.Entity[TEntity]()
   at Microsoft.EntityFrameworkCore.TablePerHierarchy.ProgramOneTphToOneTph.TestContext.OnModelCreating(ModelBuilder modelBuilder) in D:\source\GitHub\EntityFrameworkMappingTablePerHierarchy\EntityFrameworkMappingTablePerHierarchy\EntityFrameworkMappingTablePerHierarchy\ProgramOneTphToOneTph.cs:line 176
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.CreateModel(DbContext context, IConventionSetBuilder conventionSetBuilder, IModelValidator validator)
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1.CreateValue()
   at Microsoft.EntityFrameworkCore.Internal.DbContextServices.CreateModel()
   at Microsoft.EntityFrameworkCore.Internal.DbContextServices.get_Model()
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScoped(ScopedCallSite scopedCallSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(ConstructorCallSite constructorCallSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScoped(ScopedCallSite scopedCallSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
   at Microsoft.EntityFrameworkCore.DbContext.get_DbContextDependencies()
   at Microsoft.EntityFrameworkCore.DbContext.get_InternalServiceProvider()
   at Microsoft.EntityFrameworkCore.Internal.InternalAccessorExtensions.GetService[TService](IInfrastructure`1 accessor)
   at Microsoft.EntityFrameworkCore.Infrastructure.DatabaseFacade.get_DatabaseCreator()
   at Microsoft.EntityFrameworkCore.Infrastructure.DatabaseFacade.EnsureDeleted()
   at Microsoft.EntityFrameworkCore.TablePerHierarchy.ProgramOneTphToOneTph.One_TablePerHierarchy_To_One_TablePerHierarchy(Boolean applyColumnType) in D:\source\GitHub\EntityFrameworkMappingTablePerHierarchy\EntityFrameworkMappingTablePerHierarchy\EntityFrameworkMappingTablePerHierarchy\ProgramOneTphToOneTph.cs:line 243
Failed   Microsoft.EntityFrameworkCore.TablePerHierarchy.ProgramOneTphToOneTph.One_TablePerHierarchy_To_One_TablePerHierarchy(applyColumnType: False)
Error Message:
 System.InvalidOperationException : The property 'Discriminator' cannot be added to type 'ChildBase' because the type of the corresponding CLR property or field 'ParentChildDiscriminator' does not match the specified type 'string'.
Stack Trace:
   at Microsoft.EntityFrameworkCore.Metadata.Internal.EntityType.AddProperty(String name, Type propertyType, MemberInfo memberInfo, ConfigurationSource configurationSource, Nullable`1 typeConfigurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.EntityType.AddProperty(String name, Type propertyType, ConfigurationSource configurationSource, Nullable`1 typeConfigurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.Property(Property existingProperty, String propertyName, Type propertyType, MemberInfo clrProperty, Nullable`1 configurationSource, Nullable`1 typeConfigurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.Property(String propertyName, Type propertyType, MemberInfo memberInfo, Nullable`1 configurationSource, Nullable`1 typeConfigurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.Property(String propertyName, Type propertyType, ConfigurationSource configurationSource, Nullable`1 typeConfigurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalEntityTypeBuilder.Property(String propertyName, Type propertyType, ConfigurationSource configurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.RelationalEntityTypeBuilderAnnotations.DiscriminatorBuilder(Func`2 createProperty, Type propertyType)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.DiscriminatorConvention.Apply(InternalEntityTypeBuilder entityTypeBuilder, EntityType oldBaseType)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ImmediateConventionScope.OnBaseEntityTypeChanged(InternalEntityTypeBuilder entityTypeBuilder, EntityType previousBaseType)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.RunVisitor.VisitOnBaseEntityTypeChanged(OnBaseEntityTypeChangedNode node)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ConventionVisitor.VisitConventionScope(ConventionScope node)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ConventionVisitor.VisitConventionScope(ConventionScope node)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ConventionBatch.Run()
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalModelBuilder.Entity(TypeIdentity& type, ConfigurationSource configurationSource, Boolean allowOwned, Boolean throwOnQuery)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalModelBuilder.Entity(Type type, ConfigurationSource configurationSource, Boolean allowOwned, Boolean throwOnQuery)
   at Microsoft.EntityFrameworkCore.ModelBuilder.Entity[TEntity]()
   at Microsoft.EntityFrameworkCore.TablePerHierarchy.ProgramOneTphToOneTph.TestContext.OnModelCreating(ModelBuilder modelBuilder) in D:\source\GitHub\EntityFrameworkMappingTablePerHierarchy\EntityFrameworkMappingTablePerHierarchy\EntityFrameworkMappingTablePerHierarchy\ProgramOneTphToOneTph.cs:line 176
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.CreateModel(DbContext context, IConventionSetBuilder conventionSetBuilder, IModelValidator validator)
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
--- End of stack trace from previous location where exception was thrown ---
   at System.Lazy`1.CreateValue()
   at Microsoft.EntityFrameworkCore.Internal.DbContextServices.CreateModel()
   at Microsoft.EntityFrameworkCore.Internal.DbContextServices.get_Model()
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScoped(ScopedCallSite scopedCallSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(ConstructorCallSite constructorCallSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScoped(ScopedCallSite scopedCallSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
   at Microsoft.EntityFrameworkCore.DbContext.get_DbContextDependencies()
   at Microsoft.EntityFrameworkCore.DbContext.get_InternalServiceProvider()
   at Microsoft.EntityFrameworkCore.Internal.InternalAccessorExtensions.GetService[TService](IInfrastructure`1 accessor)
   at Microsoft.EntityFrameworkCore.Infrastructure.DatabaseFacade.get_DatabaseCreator()
   at Microsoft.EntityFrameworkCore.Infrastructure.DatabaseFacade.EnsureDeleted()
   at Microsoft.EntityFrameworkCore.TablePerHierarchy.ProgramOneTphToOneTph.One_TablePerHierarchy_To_One_TablePerHierarchy(Boolean applyColumnType) in D:\source\GitHub\EntityFrameworkMappingTablePerHierarchy\EntityFrameworkMappingTablePerHierarchy\EntityFrameworkMappingTablePerHierarchy\ProgramOneTphToOneTph.cs:line 243

Steps to reproduce

  1. Take latest from https://github.com/jzabroski/EntityFrameworkMappingTablePerHierarchy/tree/aspnet/EntityFrameworkCore%2315553
  2. Run dotnet test --filter ProgramOneTphToOneTph.One_TablePerHierarchy_To_One_TablePerHierarchy
  3. This will fail because the discriminator names are different.
  4. Important Tangent: Note there is some additional weirdness here that suggests the internal modeling engine used by EF is not thread-safe, even if:
    • the contexts are different instances
    • the contexts are different instances and different connection strings
    • the contexts are different classes, each with unique instances, and different connection strings.
  5. Important Tangent: Run dotnet test --filter does not filter xUnit.net Theory Data. Due to above concerns about thread safety, it is recommended to run the tests in Visual Studio where you can manually select individial Theory Data as a test case, and run each Theory Data one-by-one. (I have confirmed both ReSharper and Visual Studio Test Explorer support manually selecting Theory Data items and running one-by-one.)

Further technical details

EF Core version: 2.2.4
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10
IDE: Visual Studio 2017 15.9.11
Target Framework: netcoreapp2.2
Target Architecture: Any CPU

@ajcvickers
Copy link
Contributor

Notes for triage: more minimal repro below. Model builds correctly if there are no relationships between the two hierarchies, but throws exception reported when a relationship is introduced.

using Microsoft.EntityFrameworkCore;

public class ProgramOneTphToOneTph
{
    public static void Main()
    {
        var options = new DbContextOptionsBuilder()
            .UseSqlServer("Server=(localdb)\\mssqllocaldb;Database=Test;Connect Timeout=5")
            .Options;

        using (var db = new TestContext(options))
        {
            var model = db.Model;

            db.Database.EnsureDeleted();
            db.Database.EnsureCreated();
        }
    }

    private enum ParentDiscriminator
    {
        Good = 1,
        Bad = 2
    }

    private enum ParentChildDiscriminator
    {
        Good = 1,
        Bad = 2
    }

    private abstract class ParentBase
    {
        public int Id { get; set; }
        public ParentDiscriminator Discriminator { get; protected set; }
    }

    private class GoodParent : ParentBase
    {
    }

    private class BadParent : ParentBase
    {
        public BadChild Child { get; set; }
    }

    private abstract class ChildBase
    {
        public int Id { get; set; }
        public ParentChildDiscriminator Discriminator { get; protected set; }
    }

    private class GoodChild : ChildBase
    {
    }

    private class BadChild : ChildBase
    {
    }

    public class TestContext : DbContext
    {
        public TestContext(DbContextOptions options) :
            base(options)
        {
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            base.OnModelCreating(modelBuilder);

            modelBuilder.Entity<ParentBase>()
                .HasDiscriminator(x => x.Discriminator)
                .HasValue<GoodParent>(ParentDiscriminator.Good)
                .HasValue<BadParent>(ParentDiscriminator.Bad);

            modelBuilder.Entity<ChildBase>()
                .HasDiscriminator(x => x.Discriminator)
                .HasValue<GoodChild>(ParentChildDiscriminator.Good)
                .HasValue<BadChild>(ParentChildDiscriminator.Bad);
        }
    }
}

@smitpatel
Copy link
Contributor

The derived children are discovered by conventions and added to model (no TPH there yet). But when we add ChildBase we set BaseType for derived children, we are trying to introduce Discriminator property and bug is we assume it is string type.

@jzabroski
Copy link
Author

jzabroski commented May 2, 2019

Good point on the SaveChanges() being mostly (if not entirely) unnecessary. But, see my question #15581 - I got a DbUpdateException after moving past this issue. As discussed by another developer in #14890 , it would probably be ideal to be able to call EnsureCreated() apart from EnsureDeleted(), otherwise I don't see a way to validate in a large code base that things are actually CRUD'ing correctly.

I have more TPH bugs to file, but I have a deadline to get a project done this week and have to optimize what I think are the most valuable issues to file. For example, TPH gets confused with auto-identities. This would not be uncovered with just EnsureCreated and EnsureDeleted

@jzabroski
Copy link
Author

The derived children are discovered by conventions and added to model (no TPH there yet). But when we add ChildBase we set BaseType for derived children, we are trying to introduce Discriminator property and bug is we assume it is string type.

Just my 2 cents - may not be helpful, but having a background as a SQL person who learned C# out of necessity, I think of it as a full outer join and filtering the EF Model against the SQL Model (SELECT * FROM INFORMATION_SCHEMA.COLUMNS, etc). I have no idea if that is what EFCore is doing, but I find SQL to be a very powerful data recon tool. Procedural embedding of knowledge (C#) tends to create strange bugs that are hard to troubleshoot because it is dependent on which code path things took. That is why in #15529 I asked about "FixUp" abbreviation, because coming from a SQL world of ETL troubleshooting, the concept didn't make a lot of sense to me as a way to think about aligning data. In fact, I have often taught people that it's best to troubleshoot ETL issues using inductive reasoning and thinking in reverse - Load, Transform, Extract. Similar ideas have been popularized in computer science literature, cf Phil Wadler: "Threesomes: With or Without Blame" and "Blame For All".

Sorry if this is philosophical drivel, but I believe "perspective is worth 80 IQ points".

@smitpatel
Copy link
Contributor

@jzabroski - My comment was note to the implementer of bug fix. I took a dive in codebase to make sense of stacktrace you posted so those are my findings from brief inspection.

@jzabroski
Copy link
Author

I understand - thanks for your speedy answers!

@jzabroski
Copy link
Author

Repro'd in 3.0.0-preview4

@ajcvickers ajcvickers added this to the 3.0.0 milestone May 3, 2019
@AndriySvyryd AndriySvyryd added the verify-fixed This issue is likely fixed in new query pipeline. label Jun 20, 2019
@roji
Copy link
Member

roji commented Jun 28, 2019

Repro'd again in 4f1f019 (close to preview7).

Ready-to-run repro for QueryBugTests:

#region Bug15553

[ConditionalFact]
public virtual void Discriminator_name_must_be_unique_across_TPH_hierarchies()
{
    using (CreateDatabase15553())
    {
    }
}

private SqlServerTestStore CreateDatabase15553()
    => CreateTestStore(
        () => new MyContext15553(_options),
        context => ClearLog() );

public class MyContext15553 : DbContext
{
    public MyContext15553(DbContextOptions options) : base(options) {}

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<ParentBase15553>()
            .HasDiscriminator(x => x.Discriminator)
            .HasValue<GoodParent15553>(1)
            .HasValue<BadParent15553>(2);

        modelBuilder.Entity<ChildBase15553>()
            .HasDiscriminator(x => x.Discriminator)
            .HasValue<GoodChild15553>(3)
            .HasValue<BadChild15553>(4);
    }
}

private abstract class ParentBase15553
{
    public int Id { get; set; }
    public int Discriminator { get; protected set; }
}

private class GoodParent15553 : ParentBase15553 {}

private class BadParent15553 : ParentBase15553
{
    // Commenting out the following makes the test pass
    public BadChild15553 Child { get; set; }
}

private abstract class ChildBase15553
{
    public int Id { get; set; }
    public int Discriminator { get; protected set; }
}

private class GoodChild15553 : ChildBase15553 {}
private class BadChild15553 : ChildBase15553 {}

#endregion Bug15553

@roji roji removed the verify-fixed This issue is likely fixed in new query pipeline. label Jun 28, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0, Backlog Jun 28, 2019
@AndriySvyryd AndriySvyryd changed the title [BUG] TPH Discriminator names must be unique Non-string discriminator properties throw on multiple configuration calls Aug 31, 2019
@ajcvickers ajcvickers modified the milestones: Backlog, 5.0.0 Nov 20, 2019
@ajcvickers
Copy link
Contributor

Note from servicing meeting: we will fix the simple case here (See #18769) in 5.0 and if the risk is low then port to 3.1.x. Putting in 3.1.x for now so we track it as a potential patch.

@ajcvickers ajcvickers modified the milestones: 5.0.0, 3.1.x Feb 8, 2020
@ajcvickers
Copy link
Contributor

Using #18769 to track the servicing fix.

@ajcvickers ajcvickers modified the milestones: 3.1.x, 5.0.0 Feb 10, 2020
@AndriySvyryd
Copy link
Member

Neither repro fails with 3.1.1

But the discriminator still doesn't use the value converter correctly, see #19650

@AndriySvyryd AndriySvyryd removed this from the 5.0.0 milestone Feb 13, 2020
@AndriySvyryd AndriySvyryd removed their assignment Feb 13, 2020
@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
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