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

Relational: TPC inheritance mapping pattern #3170

Closed
6 tasks done
Tracked by #24106 ...
rowanmiller opened this issue Sep 18, 2015 · 64 comments · Fixed by #27988
Closed
6 tasks done
Tracked by #24106 ...

Relational: TPC inheritance mapping pattern #3170

rowanmiller opened this issue Sep 18, 2015 · 64 comments · Fixed by #27988
Assignees
Labels
Milestone

Comments

@rowanmiller
Copy link
Contributor

rowanmiller commented Sep 18, 2015

Consider configuring or issuing a warning to help with split identity seeding or shared sequence for the PK
FKs that point to a TPC entity type won't be enforced

  • Model support
  • Migrations support
  • Seed data support
  • SaveChanges support
  • Query support
  • End-to-end tests

Related: #17270 (comment)

@wangkanai
Copy link

Can I make assumption that TPC is now going to be in RC? judged by the following work as expected

public class Program
{
    public void Main(string[] args)
    {
        var context = new InheritanceContext();
        var sarin = new Manager() {Name = "Sarin"};
        var siriphan = new Employee() {Name = "Siriphan", Manager = sarin};
        context.Managers.Add(sarin);
        context.Employees.Add(siriphan);
        context.SaveChanges();
    }
}

public class InheritanceContext : DbContext
{
    public DbSet<Manager> Managers { get; set; }
    public DbSet<Employee> Employees { get; set; }
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlServer(@"Server=(localdb)\MSSQLLocalDB;Database=Ef7Inheritance;Trusted_Connection=True;");
    }
}

public class Person
{
    public int Id { get; set; }
    public string Name { get; set; }
}

public class Manager : Person
{
    public Collection<Employee> Staffs { get; set; } 
}

public class Employee : Person
{
    public Manager Manager { get; set; }
}

tpc
This code is tested EntityFramework 7.0.0-beta8

@jimmymain
Copy link

awesome!

@rowanmiller
Copy link
Contributor Author

@wangkanai you are really just getting two separate entity types with an unmapped base type. They each have a separate key space and you won't be able to run a LINQ query on Person. So it's not really inheritance in EF, it's two separate entity types that happen to share a base type in the CLR for implementation.

@wangkanai
Copy link

If their are in same key space would that work well with sql server auto incremental primary key?

@rowanmiller
Copy link
Contributor Author

@wangkanai not with Identity (since that is per table) but you could use a sequence to generate values. What I really meant is that with true TPC you can't have Manager with Id = 1 and an Employee with Id = 1 since then you would have two instances of Person with Id = 1 (and EF would throw if you every tried to have this). In you setup this is possible since they aren't really in an inheritance hierarchy as far as EF is concerned.

@wangkanai
Copy link

@rowanmiller so if in theory we make a convention to use Id as identity key as Guid (in theory it unique), this should work right?

@weitzhandler
Copy link
Contributor

I'd wanna see TPT before TPC. As @rowanmiller said, @wangkanai example will be used in rare cases when you explicitly wanna keep the entities unmapped or whatever other reason I can't think of now.
But TPT is even more important than TPH, because it can save a bunch of columns generated in each table completely DRYed and less-productive for some purposes.
Lack of TPC made me start a new project in MVC5 and give up the new beta.
Will this be implemented for the RC?

@rowanmiller
Copy link
Contributor Author

@wangkanai yep that would work. Just to be clear though... with the code you listed back at the start there is no need to ensure that the keys are unique between types since EF is treating them as completely separate types. But for true TPC, yes GUID keys is one option, or a sequence for numeric values, etc.

@jimmymain
Copy link

@weitzhandler I agree. Our database teams design with high normal forms in mind. TPT is what's required. I am of the opinion that TPC is a weak compromise compared to TPT. I often need to query base classes, and TPT makes this easy. A classic example of this is Martin Fowlers paper on Roles. Our clients will not be moving to EF until TPT is implemented.

@frogec
Copy link

frogec commented Jun 17, 2017

What is the status on this feature? This turns out to be a really important feature in our case.

@AndriySvyryd
Copy link
Member

The implementation could be simplified if we used a database model #8258

@sthiakos
Copy link

sthiakos commented Nov 2, 2017

+1 For Me - Note to all that you should "thumbs up" the initial post by @rowanmiller to vote up this issue. This and TPT is how DB's are/should be modeled.

@ite-klass
Copy link

The roadmap lists both TPC and TPT as high priority. Sad to not see either of these tickets in the 2.1 or 3.0 milestone.

@ajcvickers
Copy link
Member

Note: see also #10739

@J4S0Nc
Copy link

J4S0Nc commented Feb 19, 2018

Will this ever make it in? 884 days and counting...

https://days.to/18-september/2015

@VocaTinityl
Copy link

Any news about TPC ?

@ajcvickers
Copy link
Member

This issue is in the Backlog milestone. This means that it is not going to happen for the 2.1 release. We will re-assess the backlog following the 2.1 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.

@alexdrl
Copy link

alexdrl commented May 28, 2018

@rowanmiller We're using TPC with something like #3170 (comment) but we're getting stuck trying to get one to one relationships to work. We get the following exception:

System.InvalidOperationException: A key cannot be configured on 'BloodPressure' because it is a derived type. The key must be configured on the root type 'Observation'. If you did not intend for 'Observation' to be included in the model, ensure that it is not included in a DbSet property on your context, referenced in a configuration call to ModelBuilder, or referenced from a navigation property on a type that is included in the model.

One to many relationships Works correctly, even considering that TPC is not supported in EF Core. Any idea for that?

@ajcvickers
Copy link
Member

@alexdrl If you think you are hitting a bug, then can you please open a new issue including a runnable project/solution/repo or complete code listing that exhibits the behavior?

smitpatel added a commit that referenced this issue May 6, 2022
smitpatel added a commit that referenced this issue May 6, 2022
@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 May 10, 2022
AndriySvyryd added a commit that referenced this issue May 10, 2022
…he dependent doesn't use TPH

Don't configure table name for abstract types in TPC that have a corresponding DbSet property
Make `ToTable(b => {})` map the entity type to the default table

Fixes #3170
Fixes #27608
Fixes #27945
Fixes #27947
AndriySvyryd added a commit that referenced this issue May 10, 2022
…he dependent doesn't use TPH

Don't configure table name for abstract types in TPC that have a corresponding DbSet property
Make `ToTable(b => {})` map the entity type to the default table

Fixes #3170
Fixes #27608
Fixes #27945
Fixes #27947
AndriySvyryd added a commit that referenced this issue May 10, 2022
…he dependent doesn't use TPH

Don't configure table name for abstract types in TPC that have a corresponding DbSet property
Make `ToTable(b => {})` map the entity type to the default table

Fixes #3170
Fixes #27608
Fixes #27945
Fixes #27947
AndriySvyryd added a commit that referenced this issue May 11, 2022
…he dependent doesn't use TPH

Don't configure table name for abstract types in TPC that have a corresponding DbSet property
Make `ToTable(b => {})` map the entity type to the default table

Fixes #3170
Fixes #27608
Fixes #27945
Fixes #27947
AndriySvyryd added a commit that referenced this issue May 11, 2022
Don't configure table name for abstract types in TPC that have a corresponding DbSet property
Make ToTable(b => {}) map the entity type to the default table

Fixes #3170
Fixes #27608
Fixes #27945
Fixes #27947
@eraffel-MDSol
Copy link

I haven't been following closely, but since this closed, is TPC now ready to be played with in the nightly builds?

@AndriySvyryd
Copy link
Member

@eraffel-MDSol Yes, starting with tomorrow's build

@Luigi6821
Copy link

Luigi6821 commented May 12, 2022

Hi,
Looking at the new build preview I was playing with TPC to check the following scenario:

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

    public string CODE { get; set; }

    public string ALPHACODE { get; set; }
}

public abstract class AMOS_ADDRESSCONTACT
{
    public decimal ADDRESSCONTACTID { get; set; }

    public AMOS_ADDRESS ADDRESS { get; set; }

    public decimal ADDRESSID { get; set; }
}
#endregion

public class ADDRESS : AMOS_ADDRESS
{ }

public class ADDRESSCONTACT : AMOS_ADDRESSCONTACT
{
}

....
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<AMOS_ADDRESSCONTACT>().ToTable("ADDRESSCONTACT");
modelBuilder.Entity<AMOS_ADDRESSCONTACT>().HasKey(e => e.ADDRESSCONTACTID);
modelBuilder.Entity<AMOS_ADDRESSCONTACT>().Property(e => e.ADDRESSCONTACTID).HasColumnName("ADDRESSCONTACTID");
modelBuilder.Entity<AMOS_ADDRESSCONTACT>().Property(e => e.ADDRESSID).HasColumnName("ADDRESSID");
modelBuilder.Entity<AMOS_ADDRESSCONTACT>().HasOne(e => e.ADDRESS);`

The error I receive when I try to run

var o = dbContext.Set<ADDRESS>().OrderBy(a => a.ADDRESSID).Take(10);

is:

The corresponding CLR type for entity type 'AMOS_ADDRESS' cannot be instantiated, and there is no derived entity type in the model that corresponds to a concrete CLR type.

The same code is perfectly supported by EF6.
Is the TPC implementation still not yet completed ?

Regards
Luigi

@AndriySvyryd
Copy link
Member

AndriySvyryd commented May 12, 2022

@Luigi6821 Don't call modelBuilder.Entity<AMOS_ADDRESS>().ToTable("ADDRESS", "AMOS"); or modelBuilder.Entity<AMOS_ADDRESSCONTACT>().ToTable("ADDRESSCONTACT");. If the entity type is abstract it means that there should be no rows in the corresponding table. Instead call

modelBuilder.Entity<AMOS_ADDRESS>().UseTpcMappingStrategy();
modelBuilder.Entity<AMOS_ADDRESSCONTACT>().UseTpcMappingStrategy();

modelBuilder.Entity<ADDRESS>().ToTable("ADDRESS", "AMOS");
modelBuilder.Entity<ADDRESSCONTACT>().ToTable("ADDRESSCONTACT");

Also, you can't yet configure a different column name for one of the tables, that's coming in #19811

@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-preview5 May 25, 2022
@marchy
Copy link

marchy commented Jun 16, 2022

How are relationships from outside of an TPC hierarchy into any of its entities handled, given that no table is generated for the base/abstract entity?

For example, are any of the following supported?
(assuming the Animals entity hierarchy stated in the announcement post)

// Example 1
class SomeExternalEntity {
    // NOT SUPPORTED in TPC?
    public Animal Animal { get; private set; }
}

// Example 2
class SomeOtherExternalEntity {
    // NOT SUPPORTED in TPC?
    public IList<Animal> Animals { get; private set; }
}

With both TPH and TPT, the following gets generated:

// Example 1
CREATE TABLE [dbo].[SomeExternalEntity](
    [AnimalID] [nvarchar](36) NOT NULL,
) ON [PRIMARY]
ALTER TABLE [dbo].[SomeExternalEntity] ADD  CONSTRAINT [PK_dbo.SomeExternalEntity] PRIMARY KEY CLUSTERED 
(
    [AnimalID] ASC
)
ALTER TABLE [dbo].[SomeExternalEntity]  WITH CHECK ADD  CONSTRAINT [FK_dbo.SomeExternalEntity_dbo.Animals_Id] FOREIGN KEY([Id])
REFERENCES [dbo].[Animals] ([Id])

@ajcvickers
Copy link
Member

@marchy The relationship is handled normally, but no constraint is made in the database. We covered this in the community standup yesterday at around minute 58.

@eraffel-MDSol
Copy link

@ajcvickers With regard to the previous question, how will this work with deletes? Referencing the example given in the video, if I were to delete one of the animals, will the "Human" that references that animal as its favorite be deleted automatically, or would I have to manage preventing "orphan" records like that myself, since there's no FK constraint to enforce that.

@ajcvickers
Copy link
Member

@marchy The database won't perform automatic deletes, but EF will.

@bachratyg
Copy link

@ajcvickers does this mean that TPC uses Bulk (i.e. set-based) update/delete (without loading data into memory) #795 under the hood or that related entities need to be in the change tracker for cascade to work properly?

@ajcvickers
Copy link
Member

@bachratyg They need to be in the change tracker.

@bachratyg
Copy link

That means orphans have to be managed manually.

@ajcvickers
Copy link
Member

@bachratyg That depends on your definition of "manually". If data is appropriately loaded, then EF manages them. But if you are looking for a pattern that works well with database constructs like constraints and Identity keys, then TPC is not for you.

@bachratyg
Copy link

Data being appropriately loaded would mean a read lock on the principal row in all possible transactions modifying the relationship, some clever use of concurrency checks, or a business process that guarantees no concurrent phantom inserts. Either way serious steps should be taken to keep data consistent and that more or less covers my definition of "manually".

Don't get me wrong, I don't feel either way about the implementation, I just want to be sure I fully understand the consequences.

@ajcvickers ajcvickers modified the milestones: 7.0.0-preview5, 7.0.0 Nov 5, 2022
@AndriySvyryd AndriySvyryd removed their assignment Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.