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.76k stars 3.18k forks source link

HasOne/WithOne in TPH with shared column should create multiple indexes #30216

Open Xriuk opened 1 year ago

Xriuk commented 1 year ago

Include your code

Entities

public class Api{
  public int Id {get; set;}

  [...]

  public Ecommerce Ecommerce {get; set;} // One-to-zero-or-one (optional)

  public Logistics Logistics {get; set;} // One-to-zero-or-one (optional)
}

public enum EntityType{
  None,

  Ecommerce,
  Logistics,
  Customer
}

public class Entity{
  public Guid Id {get; set;}

  public EntityType Type {get; set;}

  [...]
}

public class Ecommerce : Entity{
  public Api Api {get; set;} // One-to-one (required)

  [...]
}

public class Logistics : Entity{
  public Api Api {get; set;} // One-to-one (required)

  [...]
}

public class Customer : Entity{
  [...]
}

Fluent API

modelBuilder.Entity<Api>()
  .HasKey(a => a.Id)
  /* ... */;

modelBuilder.Entity<Entity>()
  .HasKey(e => e.Id)
  .HasDiscriminator<EntityType>("Type")
  .HasValue<Entity>(EntityType.None)
  .HasValue<Ecommerce>(EntityType.Ecommerce)
  .HasValue<Logistics>(EntityType.Logistics)
  .HasValue<Customer>(EntityType.Customer)
  /* ... */;

modelBuilder.Entity<Ecommerce>()
  .Property("ApiId").IsRequired();
modelBuilder.Entity<Ecommerce>()
  .HasOne("Api").WithOne("Ecommerce").IsRequired();

modelBuilder.Entity<Logistics>()
  .Property("ApiId").IsRequired();
modelBuilder.Entity<Logistics>()
  .HasOne("Api").WithOne("Logistics").IsRequired();

modelBuilder.Entity<Customer>()
  /* ... */;

This produces the following migration:

[...]

migrationBuilder.CreateTable(
  name: "Entities",
  columns: table => new
  {
    Id = table.Column<Guid>(type: "uniqueidentifier", nullable: false),
    Type = table.Column<int>(type: "int", nullable: false),
    [...]
    ApiId = table.Column<int>(type: "int", nullable: true),
    Logistics_ApiId = table.Column<int>(type: "int", nullable: true)
  },
  constraints: table =>
  {
    table.PrimaryKey("PK_Entities", x => x.Id);
    table.ForeignKey(
      name: "FK_Entities_Api_Logistics_ApiId",
      column: x => x.Logistics_ApiId,
      principalTable: "Api",
      principalColumn: "Id");
    table.ForeignKey(
      name: "FK_Entities_Api_ApiId",
      column: x => x.ApiId,
      principalTable: "Api",
      principalColumn: "Id");
  });

[...]

migrationBuilder.CreateIndex(
  name: "IX_Entities_Logistics_ApiId",
  table: "Entities",
  column: "Logistics_ApiId",
  unique: true,
  filter: "[Logistics_ApiId] IS NOT NULL");

migrationBuilder.CreateIndex(
  name: "IX_Entities_ApiId",
  table: "Entities",
  column: "ApiId",
  unique: true,
  filter: "[ApiId] IS NOT NULL");

Which is fine I guess, because those columns would be null if the entity is not Logistics or Ecommerce but will be correctly populated on those types.

But since I'm using TPH I'd like to save a column by mapping the ApiId to the same column:

modelBuilder.Entity<Ecommerce>()
  .Property("ApiId").IsRequired();
modelBuilder.Entity<Ecommerce>()
  .HasOne("Api").WithOne("Ecommerce").IsRequired().HasColumnName("ApiId");

modelBuilder.Entity<Logistics>()
  .Property("ApiId").IsRequired();
modelBuilder.Entity<Logistics>()
  .HasOne("Api").WithOne("Logistics").IsRequired().HasColumnName("ApiId");

This produces the following migration:

[...]

migrationBuilder.CreateTable(
  name: "Entities",
  columns: table => new
  {
    Id = table.Column<Guid>(type: "uniqueidentifier", nullable: false),
    Type = table.Column<int>(type: "int", nullable: false),
    [...]
    ApiId = table.Column<int>(type: "int", nullable: true)
  },
  constraints: table =>
  {
    table.PrimaryKey("PK_Entities", x => x.Id);
    table.ForeignKey(
      name: "FK_Entities_Api_ApiId",
      column: x => x.ApiId,
      principalTable: "Api",
      principalColumn: "Id");
  });

[...]

migrationBuilder.CreateIndex(
  name: "IX_Entities_ApiId",
  table: "Entities",
  column: "ApiId",
  unique: true,
  filter: "[ApiId] IS NOT NULL");

As you can see the single index is not enough since I have two relationship on different entities types. I think something like this would work:

migrationBuilder.CreateIndex(
  name: "IX_Entities_ApiId_Ecommerce",
  table: "Entities",
  column: "ApiId",
  unique: true,
  filter: "[Type] = 1 AND [ApiId] IS NOT NULL");

migrationBuilder.CreateIndex(
  name: "IX_Entities_ApiId_Logistics",
  table: "Entities",
  column: "ApiId",
  unique: true,
  filter: "[Type] = 2 AND [ApiId] IS NOT NULL");

Maybe this would require some additional change tracking between migrations

Include provider and version information

EF Core version: 6.0.3 Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: .NET 6.0

Xriuk commented 1 year ago

Also adding that the suggested approach should also track derived entities probably, so an operator like IN could be used, also it should handle other discriminator types like strings.

public enum EntityType{
  None, // 0

  Ecommerce, // 1
    MyEcommerce1, // 2
    MyEcommerce2, // 3
  Logistics, // 4
  Customer // 5
}

migrationBuilder.CreateIndex(
  name: "IX_Entities_ApiId_Ecommerce",
  table: "Entities",
  column: "ApiId",
  unique: true,
  filter: "[Type] IN (1, 2, 3) AND [ApiId] IS NOT NULL");
ajcvickers commented 1 year ago

Notes from triage: we should avoid creating any indexes here and warn indicating that uniqueness is not enforced by the database when sharing an FK column in this way. The user can then choose to create indexes manually for whatever their model actually is, rather than us attempting to infer that with a potentially partial discriminator mapping.

stevendarby commented 1 year ago

I think this a duplicate or at least closely related to https://github.com/dotnet/efcore/issues/25391

ajcvickers commented 1 year ago

Note from triage: keeping this issue open to add a warning; https://github.com/dotnet/efcore/issues/25391 in case it ever gets implemented, which seems unlikely.