Throw for ambiguous self-referencing navigations

[parxal]

Include Method are Generating Wrong Sql Join

SELECT [p].[Id], [p].[CreatedById], [p].[Name], [p].[UpdatedById], [p.UpdatedBy].[Id], [p.UpdatedBy].[CreatedById], [p.UpdatedBy].[Name], [p.UpdatedBy].[UpdatedById], [p.CreatedBy].[Id], [p.CreatedBy].[CreatedById], [p.CreatedBy].[Name], [p.CreatedBy].[UpdatedById]
FROM [Users] AS [p]
LEFT JOIN [Users] AS [p.UpdatedBy] ON [p].[Id] = [p.UpdatedBy].[CreatedById]
INNER JOIN [Users] AS [p.CreatedBy] ON [p].[CreatedById] = [p.CreatedBy].[Id]

Steps to reproduce

SQL

CREATE TABLE [dbo].[Users]
(
    [CreatedById] [UNIQUEIDENTIFIER] NOT NULL,
    [UpdatedById] [UNIQUEIDENTIFIER] NULL,
    [Id] [UNIQUEIDENTIFIER] NOT NULL,
    [Name] [NVARCHAR](150) NOT NULL,

    CONSTRAINT [PK_Users] 
        PRIMARY KEY CLUSTERED ([Id] ASC)
) ON [PRIMARY]
GO

ALTER TABLE [dbo].[Users]  WITH CHECK 
    ADD CONSTRAINT [FK_Users_Users_CreatedById] 
        FOREIGN KEY([CreatedById]) REFERENCES [dbo].[Users] ([Id])
GO

ALTER TABLE [dbo].[Users] WITH CHECK 
    ADD CONSTRAINT [FK_Users_Users_UpdatedById] 
        FOREIGN KEY([UpdatedById]) REFERENCES [dbo].[Users] ([Id])
GO

INSERT [dbo].[Users] ([CreatedById], [UpdatedById], [Id], [Name]) 
VALUES (N'2cfc1025-cf96-4929-a6a4-e2ed77a6ae12', N'2cfc1025-cf96-4929-a6a4-e2ed77a6ae12', N'2cfc1025-cf96-4929-a6a4-e2ed77a6ae12', N'Admin'),
       (N'2cfc1025-cf96-4929-a6a4-e2ed77a6ae12', N'60ca72c4-db2d-4cea-9981-c10d6942a11e', N'60ca72c4-db2d-4cea-9981-c10d6942a11e', N'User 01'),
       (N'60ca72c4-db2d-4cea-9981-c10d6942a11e', NULL, N'40719114-f53e-4d3f-9a1b-49f63b18002b', N'New User')

GO

[Table("Users")]
public partial class User 
{
        [Required]
        public Guid Id { get; set; }
        [Required]
        [MaxLength(150)]
        public string Name { get; set; }
        [Required]
        public User CreatedBy { get; set; }
        public User UpdatedBy { get; set; }
        [Required]
        public Guid CreatedById { get; set; }
        public Guid? UpdatedById { get; set; }
}

public class ApplicationDbContext : DbContext
{
    public DbSet<User> Users { get; set; }

    public ApplicationDbContext(DbContextOptions<ApplicationDbContext> options) : base(options) {}
}

class Program
{
    private static ApplicationDbContext CreateContext()
    {
         return new ApplicationDbContext(new DbContextOptionsBuilder<ApplicationDbContext>().UseSqlServer("Server=...").Options);
    }

    static void Main(string[] args)
    {
        using (var dbContext = CreateContext())
        {
            var test = dbContext.Set<User>().Include(p => p.CreatedBy).Include(p => p.UpdatedBy).ToList();
            Console.WriteLine(test.Count);
        }
   }
}

The same instructions in Entity Framework 6.2 works fine. (this code only returns 3 records)
In entityframeworkcore sql generate join is "strange", and returns 4 records.

Further technical details

EF Core version: 2.1.4
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system:
IDE: Visual Studio 2017 15.8.5

[smitpatel]

Share your model configuration.
It seems like your model is configured incorrectly to use CreatedById as FK for UpdatedBy navigation property. Which is incorrect since, it should be using UpdatedById.

[ajcvickers]

Note that this was also posted on Stack Overflow and David was unable to reproduce the issue.

@parxal Based on the responses from David and Smit, can you please provide a runnable project/solution or complete code listing that demonstrates the behavior you are seeing?

[parxal]

@ajcvickers just upload my example https://github.com/parxal/EntityFrameworkCore , @smitpatel check my user model class

[ajcvickers]

@parxal Thanks!

Note for triage: the model created is making CreatedBy and UpdatedBy inverses of each other:

Model: 
  EntityType: User
    Properties: 
      Id (Guid) Required PK AfterSave:Throw ValueGenerated.OnAdd 0 0 0 -1 0
      CreatedById (Guid) Required FK Index 1 1 1 -1 1
      Name (string) Required MaxLength150 2 2 -1 -1 -1
      UpdatedById (Nullable<Guid>) 3 3 -1 -1 -1
    Navigations: 
      CreatedBy (<CreatedBy>k__BackingField, User) ToPrincipal User Inverse: UpdatedBy 0 -1 2 -1 -1
      UpdatedBy (<UpdatedBy>k__BackingField, User) ToDependent User Inverse: CreatedBy 1 -1 3 -1 -1
    Keys: 
      Id PK
    Foreign keys: 
      User {'CreatedById'} -> User {'Id'} Unique ToDependent: UpdatedBy ToPrincipal: CreatedBy

@parxal We will discuss if this is the correct model to build by convention. You should be able to fix your issue by explicitly configuring the two relationships as separate. For example:

 modelBuilder
     .Entity<User>()
     .HasOne(e => e.CreatedBy)
     .WithMany()
     .OnDelete(DeleteBehavior.ClientSetNull); // To prevent cascade cycles on SQL Server
 
 modelBuilder
     .Entity<User>()
     .HasOne(e => e.UpdatedBy)
     .WithMany();

[parxal]

@ajcvickers thanks, i just fixed it with: modelBuilder.Entity().HasOne(e => e.CreatedBy).WithMany();
And now all it's ok. This project was created with code first and my final question is : Should this line be Required when creating the model? Isn't this a bug?

[smitpatel]

2 navigations between a pair of entity (same entity in this case), are always paired of if it is not ambiguous. If you want to declare multiple navigations, you would need to manually configure them.

[ajcvickers]

@parxal "Should this line be Required when creating the model?" is something we need to discuss.

[davidbaxterbrowne]

I would think that unless CreatedById is unique, that UpdatedBy cannot be the inverse navigation property of CreatedBy.

[ajcvickers]

@davidbaxterbrowne My guess is we currently pivot on matching navigations before attempting to look for FKs, and hence CreatedById is not even matched as an FK.

[ajcvickers]

Notes from triage: This:

public class User 
{
        public Guid Id { get; set; }
        public User CreatedBy { get; set; }
        public User UpdatedBy { get; set; }
        public Guid CreatedById { get; set; }
        public Guid? UpdatedById { get; set; }
}

is essentially the same with a self-referencing relationships as this is without them:

public class Bar
{
        public Guid Id { get; set; }
        public Foo UpdatedBy { get; set; }
        public Guid? UpdatedById { get; set; }
}
public class Foo
{
        public Guid Id { get; set; }
        public Bar CreatedBy { get; set; }
        public Guid CreatedById { get; set; }
}

This throws for ambiguity, and so should the self-referencing example.

[ajcvickers]

Poaching...