EF Core 5 generates duplicate FK from model

[davidbaxterbrowne]

File a bug

The follosing model generates two foreign keys on the same column, but only if the FK column is not configured as an alternate key.

Use triple-tick code fences for any posted code. For example:

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

using (var db = new Db())
{
    db.Database.EnsureDeleted();
    db.Database.EnsureCreated();

}

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

public class User : Entity
{
    public string Name { get; set; }
    public Person Person { get; set; }
}

public class Person : Entity
{
    public string FirstName { get; set; }
    public string LastName { get; set; }

    public int UserId { get; set; }
    public User User { get; set; }
    //public ICollection<PersonSchool> PersonSchool { get; set; }
}
public class Db : DbContext
{

    public Db() : base()
    {

    }

    private static readonly ILoggerFactory loggerFactory = LoggerFactory.Create(builder =>
    {
        builder.AddFilter((category, level) =>
           category == DbLoggerCategory.Database.Command.Name
           && level == LogLevel.Debug).AddConsole();
    });

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        var constr = "Server=localhost; database=efcore5test; integrated security = true; TrustServerCertificate=true";

        optionsBuilder.UseLoggerFactory(loggerFactory)
                      .UseSqlServer(constr, o => o.UseRelationalNulls());

        base.OnConfiguring(optionsBuilder);
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<User>()
            .ToTable("User")
            .HasKey(c => c.Id);

        modelBuilder.Entity<User>().Property(c => c.Name).IsRequired(true);

        modelBuilder.Entity<Person>()
            .ToTable("Person")
            .HasKey(c => c.Id);

        modelBuilder.Entity<Person>().Property(c => c.FirstName).IsRequired(true);

        modelBuilder.Entity<Person>().Property(c => c.LastName).IsRequired(true);

        //modelBuilder.Entity<Person>().HasAlternateKey(p => p.UserId);

        modelBuilder.Entity<Person>().HasOne(i => i.User)
           .WithOne()
           .HasForeignKey<Person>(rl => rl.UserId)
           .OnDelete(DeleteBehavior.Restrict);

        base.OnModelCreating(modelBuilder);
    }

}

Creates this

      CREATE TABLE [Person] (
          [Id] int NOT NULL IDENTITY,
          [FirstName] nvarchar(max) NOT NULL,
          [LastName] nvarchar(max) NOT NULL,
          [UserId] int NOT NULL,
          [UserId1] int NULL,
          CONSTRAINT [PK_Person] PRIMARY KEY ([Id]),
          CONSTRAINT [FK_Person_User_UserId] FOREIGN KEY ([UserId]) REFERENCES [User] ([Id]) ON DELETE NO ACTION,
          CONSTRAINT [FK_Person_User_UserId1] FOREIGN KEY ([UserId1]) REFERENCES [User] ([Id]) ON DELETE NO ACTION
      );

With the alternate key enabled it creates:

      CREATE TABLE [Person] (
          [Id] int NOT NULL IDENTITY,
          [FirstName] nvarchar(max) NOT NULL,
          [LastName] nvarchar(max) NOT NULL,
          [UserId] int NOT NULL,
          CONSTRAINT [PK_Person] PRIMARY KEY ([Id]),
          CONSTRAINT [AK_Person_UserId] UNIQUE ([UserId]),
          CONSTRAINT [FK_Person_User_UserId] FOREIGN KEY ([UserId]) REFERENCES [User] ([Id]) ON DELETE NO ACTION
      );

Include provider and version information

EF Core version: 5.0.0 Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: .NET 5.0 Operating system: Windows IDE: Visual Studio 2019

[ajcvickers]

@davidbaxterbrowne What is the intention when calling HasAlternateKey in this case?

Note for triage: I think this is by-design, but @AndriySvyryd should confirm. When the alternate key is defined, then it acts as a candidate principal key for the second relationship, which then switches the principal and dependent end and removes the need to create a shadow FK property.

Models (slightly simplified) for without the AK:

Model: 
  EntityType: Person
    Properties: 
      Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
      FirstName (string)
      LastName (string)
      UserId (int) Required FK Index
      UserId1 (no field, Nullable<int>) Shadow FK Index
    Navigations: 
      User (User) ToPrincipal User
    Keys: 
      Id PK
    Foreign keys: 
      Person {'UserId'} -> User {'Id'} Unique ToPrincipal: User Restrict
      Person {'UserId1'} -> User {'Id'} Unique ToDependent: Person ClientSetNull
    Indexes: 
      UserId  Unique
      UserId1  Unique
  EntityType: User
    Properties: 
      Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
      Name (string)
    Navigations: 
      Person (Person) ToDependent Person
    Keys: 
      Id PK

And with AK:

Model: 
  EntityType: Person
    Properties: 
      Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
      FirstName (string)
      LastName (string)
      UserId (int) Required FK AlternateKey AfterSave:Throw
    Navigations: 
      User (User) ToPrincipal User
    Keys: 
      Id PK
      UserId
    Foreign keys: 
      Person {'UserId'} -> User {'Id'} Unique ToPrincipal: User Restrict
  EntityType: User
    Properties: 
      Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
      Name (string)
      PersonId (no field, Nullable<int>) Shadow FK Index
    Navigations: 
      Person (Person) ToPrincipal Person
    Keys: 
      Id PK
    Foreign keys: 
      User {'PersonId'} -> Person {'Id'} ToPrincipal: Person ClientSetNull
    Indexes: 
      PersonId 

[smitpatel]

I find model with AK bit suspicious. Since both reference navigations are in different relationship, the unconfigured 'Person' navigation should create one-to-many relationship with Person being navigation to principal rather than to dependent. Not sure what causes it to be one-to-one without any user configuration.

[davidbaxterbrowne]

What is the intention when calling HasAlternateKey in this case?

Simply to configure a 1-1 relationship between the two tables. And it's what I expected to happen when configuring the model as

        modelBuilder.Entity<Person>().HasOne(i => i.User)
           .WithOne()
           .HasForeignKey<Person>(rl => rl.UserId)
           .OnDelete(DeleteBehavior.Restrict);

Why is a shadow FK created when the relationship is configured with HasForeignKey.

[davidbaxterbrowne]

Got it. I totally overlooked the additional navigation property. This should simply be

        modelBuilder.Entity<Person>().HasOne(i => i.User)
           .WithOne(u => u.Person)
           .HasForeignKey<Person>(rl => rl.UserId)
           .OnDelete(DeleteBehavior.Restrict);

[ajcvickers]

Discussed in triage and agreed that this scenario will be helped by #20435 since a warning will be generated that the FK name was uniquified. We don't believe anything else is needed here. But @smitpatel wants to investigate more, so keeping this open.

[smitpatel]

public class User
    {
        public int Id { get; set; }
        public Person Person { get; set; }
    }

    public class Person
    {
        public int Id { get; set; }
        //public int UserId { get; set; }
        //public User User { get; set; }
    }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<Person>();
            modelBuilder.Entity<User>();
            // Configure model
            //modelBuilder.Entity<Person>().HasOne(i => i.User)
            //   .WithOne()
            //   .HasForeignKey<Person>(rl => rl.UserId);
        }

Generates model

Model:
  EntityType: Person
    Properties:
      Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
    Keys:
      Id PK
  EntityType: User
    Properties:
      Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
      PersonId (no field, Nullable<int>) Shadow FK Index
    Navigations:
      Person (Person) ToPrincipal Person
    Keys:
      Id PK
    Foreign keys:
      User {'PersonId'} -> Person {'Id'} ToPrincipal: Person ClientSetNull
    Indexes:
      PersonId

Uncommenting the commented code and adding another FK in the model which does not change anything with existing FK. Generated model

Model:
  EntityType: Person
    Properties:
      Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
      UserId (int) Required FK Index
      UserId1 (no field, Nullable<int>) Shadow FK Index
    Navigations:
      User (User) ToPrincipal User
    Keys:
      Id PK
    Foreign keys:
      Person {'UserId'} -> User {'Id'} Unique ToPrincipal: User Cascade
      Person {'UserId1'} -> User {'Id'} Unique ToDependent: Person ClientSetNull
    Indexes:
      UserId  Unique
      UserId1  Unique
  EntityType: User
    Properties:
      Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
    Navigations:
      Person (Person) ToDependent Person
    Keys:
      Id PK

This converts a conventionally identified one-to-many navigation to one-to-one on different side. This seems like a confusing behavior and possible bug in convention somewhere. cc: @AndriySvyryd

[AndriySvyryd]

This happens because we invert the relationship and try to use UserId for the FK. If the other relationships would've been found by convention as well we'd throw saying that it's ambiguous. To fix this we'd need to avoid creating ambiguous relationships altogether and store them in an annotation instead, similarly to how the RelationshipDiscoveryConvention works.

This would be a somewhat significant change that could start throwing for models that are currently considered valid, so I don't think this is a good fit for a patch.

[AndriySvyryd]

The foreign keys could also becomes non-ambiguous when a better FK candidate property is added. Implementing a fix that would handle this in a robust way would be fairly expensive (also in perf impact). It could be done in a simpler way when https://github.com/dotnet/efcore/issues/15898 is implemented

[ajcvickers]

Note from triage: backlog this to do have #15898 is implemented.