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

Throw for ambiguous self-referencing navigations #13573

Closed
parxal opened this issue Oct 10, 2018 · 11 comments

Comments

@parxal
Copy link

@parxal parxal commented Oct 10, 2018

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

This comment has been minimized.

Copy link
Contributor

@smitpatel smitpatel commented Oct 10, 2018

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

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Oct 10, 2018

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

This comment has been minimized.

Copy link
Author

@parxal parxal commented Oct 10, 2018

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

@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Oct 10, 2018

@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

This comment has been minimized.

Copy link
Author

@parxal parxal commented Oct 10, 2018

@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

This comment has been minimized.

Copy link
Contributor

@smitpatel smitpatel commented Oct 10, 2018

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

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Oct 10, 2018

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

@davidbaxterbrowne

This comment has been minimized.

Copy link

@davidbaxterbrowne davidbaxterbrowne commented Oct 10, 2018

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

@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Oct 10, 2018

@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

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Oct 15, 2018

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 ajcvickers added this to the 3.0.0 milestone Oct 15, 2018
@ajcvickers ajcvickers added the type-bug label Oct 15, 2018
@AndriySvyryd AndriySvyryd changed the title .Include Error Throw for ambiguous self-referencing navigations May 9, 2019
@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Jul 16, 2019

Poaching...

ajcvickers added a commit that referenced this issue Jul 16, 2019
Verified that this is already fixed in 3.0. (Also verified that it did throw in 2.2, so this is a 3.0 fix.)

Issue #13573
ajcvickers added a commit to aspnet/EntityFramework.Docs that referenced this issue Jul 16, 2019
Specifically:
* New note for aspnet/EntityFrameworkCore#13573
* First attempt at a summary--feedback appreciated. Not sure if a table is right here?
ajcvickers added a commit that referenced this issue Jul 16, 2019
Verified that this is already fixed in 3.0. (Also verified that it did throw in 2.2, so this is a 3.0 fix.)

Issue #13573
ajcvickers added a commit to aspnet/EntityFramework.Docs that referenced this issue Jul 19, 2019
Specifically:
* New note for aspnet/EntityFrameworkCore#13573
* First attempt at a summary--feedback appreciated. Not sure if a table is right here?
ajcvickers added a commit to aspnet/EntityFramework.Docs that referenced this issue Jul 23, 2019
* Updates to breaking-changes.md

Specifically:
* New note for aspnet/EntityFrameworkCore#13573
* Add more usable summary
@ajcvickers ajcvickers closed this Jul 23, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview8 Jul 29, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview8, 3.0.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.