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

Allow specifying different column names per table in TPT, TPC or entity splitting #19811

Closed
Tracked by #24106 ...
AndriySvyryd opened this issue Feb 6, 2020 · 10 comments · Fixed by #28156
Closed
Tracked by #24106 ...
Labels
area-model-building area-relational-mapping closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. ef6-parity punted-for-6.0 type-enhancement
Milestone

Comments

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Feb 6, 2020

E.g. int Id could be mapped to int Id in People and to string CustomerId in Customers

TPC:

    modelBuilder.Entity<Person>().ToTable("People")
        .InheritPropertyMapping(false);  
    modelBuilder.Entity<Customer>().ToTable("Customers", t =>
        {
            t.Property(c => c.Id).HasColumnName("CustomerId");
        });

Entity splitting:

    modelBuilder.Entity<Customer>(cb =>
    {
        cb.ToTable("Customers");
        cb.SplitToTable("CustomerDetails", t =>
        {
            t.Property(c => c.Id).HasColumnName("CustomerId");
        })
    };

Also allow to map a base property to the derived table in TPT

TPT:

    modelBuilder.Entity<Person>().ToTable("People");  
    modelBuilder.Entity<Customer>().ToTable("Customers", t =>
        {
            t.Property(c => c.Id).HasColumnName("CustomerId");
        });

Related: #17270 (comment)

@ajcvickers ajcvickers added this to the Backlog milestone Feb 7, 2020
@AndriySvyryd AndriySvyryd changed the title Allow specifying different column facets per table in TPC Allow specifying different column facets per table in TPC or entity splitting Feb 21, 2020
@smitpatel
Copy link
Member

A possible side-effect in query, that if you are writing query like context.People.Where(e => e.Id == 5) then translating that could be complicated, since now you have to apply the predicate to both the tables separately before generating Union. Which could easily go downhill when the Id start being used in complex query operators like join conditions etc.

@smitpatel
Copy link
Member

Just different column names or just different but database implicit convertible column types should be ok.

@AndriySvyryd AndriySvyryd changed the title Allow specifying different column facets per table in TPC or entity splitting Allow specifying different facets per table in TPC or entity splitting Aug 21, 2020
@AndriySvyryd
Copy link
Member Author

Test for SQL Server FKs being uniquified correctly:

            [ConditionalFact]
            public virtual void TPT_inherited_FK_is_uniquified()
            {
                var modelBuilder = CreateModelBuilder();
                modelBuilder.Entity<BigMak>()
                    .Ignore(b => b.Bun)
                    .Ignore(b => b.Pickles);
                modelBuilder.Entity<Ingredient>(b =>
                {
                    b.ToTable("Ingredients");
                    b.HasOne(i => i.BigMak).WithOne().HasForeignKey<Ingredient>("BigMakId")
                        .HasConstraintName("FK_Buns_Ingredients_Id");
                });
                modelBuilder.Entity<Bun>(b =>
                {
                    b.ToTable("Buns");
                });
                modelBuilder.Entity<Pickle>(b =>
                {
                    b.ToTable("Pickles");
                });

                var model = modelBuilder.FinalizeModel();

                var principalType = model.FindEntityType(typeof(BigMak));
                Assert.Empty(principalType.GetForeignKeys());
                Assert.Empty(principalType.GetIndexes());

                var ingredientType = model.FindEntityType(typeof(Ingredient));

                var bunType = model.FindEntityType(typeof(Bun));
                var bunFk = bunType.GetDeclaredForeignKeys().Single();
                Assert.True(bunFk.IsBaseLinking());
                Assert.Equal("FK_Buns_Ingredients_Id", bunFk.GetConstraintName());
                Assert.Equal("FK_Buns_Ingredients_Id1", bunFk.GetConstraintName(
                    StoreObjectIdentifier.Create(bunType, StoreObjectType.Table).Value,
                    StoreObjectIdentifier.Create(ingredientType, StoreObjectType.Table).Value));
                Assert.Equal(bunType.GetForeignKeys().Count() - 1, bunType.GetIndexes().Count());
                Assert.True(bunFk.DeclaringEntityType.FindIndex(bunFk.Properties).IsUnique);

                var pickleType = model.FindEntityType(typeof(Pickle));
                var pickleFk = pickleType.GetDeclaredForeignKeys().Single();
                Assert.True(pickleFk.IsBaseLinking());
                Assert.Equal("FK_Pickle_Ingredients_Id", pickleFk.GetConstraintName());
                Assert.Equal("FK_Pickle_Ingredients_Id", pickleFk.GetConstraintName(
                    StoreObjectIdentifier.Create(pickleType, StoreObjectType.Table).Value,
                    StoreObjectIdentifier.Create(bunType, StoreObjectType.Table).Value));
                Assert.Equal(pickleType.GetForeignKeys().Count() - 1, pickleType.GetIndexes().Count());
                Assert.True(pickleFk.DeclaringEntityType.FindIndex(pickleFk.Properties).IsUnique);
            }

@ajcvickers
Copy link
Member

Not to implementer: see scenario in #22753.

@AndriySvyryd AndriySvyryd changed the title Allow specifying different facets per table in TPT, TPC or entity splitting Allow specifying different column names per table in TPT, TPC or entity splitting May 7, 2022
AndriySvyryd added a commit that referenced this issue May 24, 2022
Allow specifying different column names per table in TPT, TPC or entity splitting

Part of #620
Fixes #19811
AndriySvyryd added a commit that referenced this issue Jun 1, 2022
Allow specifying different column names per table in TPT, TPC or entity splitting

Part of #620
Fixes #19811
@twenzel
Copy link
Contributor

twenzel commented Jun 1, 2022

Any process or workaround for this issue?

@overdunne
Copy link

We ended up creating a view for one of the tables in TPT, that renames the PK column to match the other, then mapping to the view as if it were the table.

AndriySvyryd added a commit that referenced this issue Jun 4, 2022
Allow specifying different column names per table in TPT, TPC or entity splitting
Move EntityTypeBuilder properties on store object builders to explicit implementation
Change TableValuedFunctionBuilder to follow the pattern
Introduce IReadOnlyStoreObjectDictionary to abstract table-specific configuration storage
Rename GetColumnBaseName to GetColumnName

Part of #620
Fixes #19811
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 4, 2022
@AndriySvyryd AndriySvyryd removed their assignment Jun 4, 2022
AndriySvyryd added a commit that referenced this issue Jun 7, 2022
Allow specifying different column names per table in TPT, TPC or entity splitting
Move EntityTypeBuilder properties on store object builders to explicit implementation
Change TableValuedFunctionBuilder to follow the pattern
Introduce IReadOnlyStoreObjectDictionary to abstract table-specific configuration storage
Rename GetColumnBaseName to GetColumnName

Part of #620
Fixes #19811
AndriySvyryd added a commit that referenced this issue Jun 14, 2022
Allow specifying different column names per table in TPT, TPC or entity splitting
Move EntityTypeBuilder properties on store object builders to explicit implementation
Change TableValuedFunctionBuilder to follow the pattern
Introduce IReadOnlyStoreObjectDictionary to abstract table-specific configuration storage
Rename GetColumnBaseName to GetColumnName

Part of #620
Fixes #19811
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-preview6 Jun 20, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-preview6, 7.0.0 Nov 5, 2022
@FransBouma
Copy link

FransBouma commented Nov 7, 2022

@AndriySvyryd :

The fix for the issue in the OP which I reported for EF Core 6 now has apparently a 'solution' but it's very inconvenient:
https://learn.microsoft.com/en-us/ef/core/what-is-new/ef-core-7.0/whatsnew#properties-mapped-to-different-column-names

Manager inherits from Employee in a TPT scenario

Instead of:

config.ToTable("Manager");
config.Property(t => t.EmployeeId).HasColumnName("ManagerID");
config.Property(t => t.ManagesDepartmentId).HasColumnName("ManagesDepartmentID");
config.HasOne(t => t.Department).WithMany(t => t.Managers).HasForeignKey(t => t.ManagesDepartmentId);

I now have to do:

config.ToTable("Manager", tb=>tb.Property(t => t.EmployeeId).HasColumnName("ManagerID"));
config.Property(t => t.ManagesDepartmentId).HasColumnName("ManagesDepartmentID");
config.HasOne(t => t.Department).WithMany(t => t.Managers).HasForeignKey(t => t.ManagesDepartmentId);

which forces me to adjust my code generator for this particular scenario (and generate clumsy code for compound PKs too). While that's of course doable, it begs the question why it wasn't solved behind the scenes: the target table is different, Manager inherits from Employee in the model, the field name is different in the pk field mapping for Manager, it maps the inherited field EmployeeId to a field that's named differently in the mapping than in Employee... gee, could it be it might be it remaps the pk field!?

@AndriySvyryd
Copy link
Member Author

which forces me to adjust my code generator for this particular scenario (and generate clumsy code for compound PKs too). While that's of course doable, it begs the question why it wasn't solved behind the scenes: the target table is different, Manager inherits from Employee in the model, the field name is different in the pk field mapping for Manager, it maps the inherited field EmployeeId to a field that's named differently in the mapping than in Employee... gee, could it be it might be it remaps the pk field!?

In general, we err on the side of having simpler rules instead of trying to guess user's intent. This wouldn't help you in particular, but the users can now write their own conventions to handle situations like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-model-building area-relational-mapping closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. ef6-parity punted-for-6.0 type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants