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

Using ForeignKeyAttribute to control the name of a FK property unexpectedly swaps principal and dependent sides and sets cardinality to 1:1 #4909

Closed
divega opened this issue Mar 25, 2016 · 6 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@divega
Copy link
Contributor

divega commented Mar 25, 2016

I introduced [ForeignKey("UserId")] in a model like the one below with the sole purpose of affecting the name of the foreign key for Post.Author.

Instead EF Core went off to find that "UserId" matched the name of the principal key on User, and so decided to create a 1:1 identifying relationship between Post.PostId and User.UserId.

    public class Post
    {
        public int PostId { get; set; }
        public string Body { get; set; }
        [ForeignKey("UserId")]
        public User Author { get; set; }
    }

    public class User
    {
        public int UserId { get; set; }
        public string Name { get; set; }
    }

    public class BloggingContext : DbContext
    {
        public DbSet<Post> Posts { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseInMemoryDatabase();
        }
    }

This is probably by design right now but I believe it is very confusing. In fact I find it strange that using ForeignKeyAttribute on a reference navigation property would turn a relationship around and make the reference point from principal to dependent.

I only realized that something was wrong when the navigation property started to be populated with completely arbitrary entities.

Additional repro code:

using System.ComponentModel.DataAnnotations.Schema;
using System.Linq;
using Microsoft.EntityFrameworkCore;
using Xunit;

namespace ForeignKeyName
{
    public class Tests
    {
        [Fact]
        private static void ForeignKeyAtribute_should_rename_fk_for_reference()
        {
            using (var context = new BloggingContext())
            {
                var postForeignKeys = context.Model.FindEntityType(typeof (Post)).GetForeignKeys();
                Assert.True(
                    postForeignKeys 
                        .Any(fk=>fk.Properties
                            .Any(p=>p.Name=="UserId")));
            }
        }
        [Fact]
        private static void ForeignKeyAtribute_should_not_swap_principal_side()
        {
            using (var context = new BloggingContext())
            {
                var userForeignKeys = context.Model.FindEntityType(typeof(User)).GetForeignKeys();
                Assert.False(
                    userForeignKeys
                        .Any(fk => fk.PrincipalKey.Properties
                            .Any(p => p.Name == "PostId")));
            }
        }
    }
@divega divega changed the title Using ForeignKeyAttribute to control the name of a FK property unexpectedly changes the principal side and cardinality to 1:1 Using ForeignKeyAttribute to control the name of a FK property unexpectedly swaps principal and dependent sides and sets cardinality to 1:1 Mar 25, 2016
@rowanmiller rowanmiller added this to the 1.0.0 milestone Mar 25, 2016
@ajcvickers
Copy link
Contributor

I'm wondering if the data annotations shouldn't have exactly the same semantics that they did in EF6? Isn't this inherit to their nature as independent from any given technology? If ForeginKey doesn't have a single set of semantics, then how do other technologies that want to consume the annotation decide what it means?

@AndriySvyryd
Copy link
Member

@ajcvickers Are you saying the current behavior is the desired one or not?
In EF6 the FKA configured the properties iff the dependent side was configured.

@divega
Copy link
Contributor Author

divega commented Mar 28, 2016

Actually for this particular scenario in EF6 you would get the following exception:

"An unhandled exception of type 'System.InvalidOperationException' occurred in EntityFramework.dll

Additional information: The ForeignKeyAttribute on property 'Author' on type 'ConsoleApplication43.Post' is not valid. The foreign key name 'UserId' was not found on the dependent type 'ConsoleApplication43.Post'. The Name value should be a comma separated list of foreign key property names."

Note that FKs in shadow state were not supported.

Regardless I do agree with @ajcvickers that backwards compatibility (or maintaining consistent semantics of how data annotations are interpreted) is important. Although I believe if there is a case in which the old behavior was very broken we should seriously consider changing it in the new stack. In particular I don't believe there are many things out there that try to reason about ForeignKeyAttribute.

@divega
Copy link
Contributor Author

divega commented Mar 28, 2016

In particular I don't believe there are many things out there that try to reason about ForeignKeyAttribute .

Actually there might be things out there (other than EF6) that do reason about ForeignKeyAttribute. I would be surprise if they ever conclude that the name specified for an FK on a reference can be referring on the FK on the other side of the relationship.

@ajcvickers
Copy link
Contributor

@AndriySvyryd It was more a general comment. There have been a few issues filed recently about the way data annotations work and I was saying maybe in general we should be making sure we don't change the semantics.

@divega If you make the relationship explicitly 1:1 (add another nav property) does it still throw with the old stack?

@divega
Copy link
Contributor Author

divega commented Mar 28, 2016

If you add the navigation property on the other side you get this:

"An unhandled exception of type 'System.InvalidOperationException' occurred in EntityFramework.dll

Additional information: Unable to determine the principal end of an association between the types 'ConsoleApplication43.User' and 'ConsoleApplication43.Post'. The principal end of this association must be explicitly configured using either the relationship fluent API or data annotations."

If you configure the relationship explicit I believe the ForeignKeyAttribute is ignored, so I don't think it is relevant, but I might be missing something.

AndriySvyryd added a commit that referenced this issue Mar 30, 2016
…side when specified on a reference navigation property.

Fixes #4909
Fixes #4885
AndriySvyryd added a commit that referenced this issue Mar 30, 2016
…side when specified on a reference navigation property.

Fixes #4909
Fixes #4885
@AndriySvyryd AndriySvyryd modified the milestones: 1.0.0-rc2, 1.0.0 Apr 1, 2016
@AndriySvyryd AndriySvyryd removed their assignment Apr 1, 2016
@ajcvickers ajcvickers removed this from the 1.0.0-rc2 milestone Oct 15, 2022
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 15, 2022
@ajcvickers ajcvickers added this to the 1.0.0 milestone Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

4 participants