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

Shadow FK created by replicating the property type from principal key are non-nullable #4895

Closed
divega opened this issue Mar 23, 2016 · 5 comments
Assignees
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 23, 2016

I tried applying [ForeignKey(name)] on a reference navigation property that didn't have a corresponding FK property in the CLR type to pick a column name that was different from the one that the convention would produce.

This had the unexpected side effect that it made the FK non-nullable (which in turns activated cascade deletes, caused queries with Include() on that navigation property to translate to INNER JOINs, etc).

None of these things happened if the name I specified in the attribute matched what the convention would pick.

I haven't dug into the implementation but I wonder if this affects more than just the nullability of the property, and if the relationship builder should produce nullable FKs by default.

Repro

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

namespace ForeignKeyName
{
    public class Tests
    {
        [Fact]
        private static void FKAttribute_should_not_change_nullability_if_it_renames_property()
        {
            using (var context = new BloggingContext())
            {
                var et = context.Model.FindEntityType(typeof(Post));
                var fk = et.FindProperty("AuthorId");
                Assert.True(fk.IsNullable); // fails
            }
        }

        [Fact]
        private static void FKAttribute_should_not_change_nullability_if_it_does_not_rename_property()
        {
            using (var context = new BloggingContext())
            {
                var et = context.Model.FindEntityType(typeof(Post));
                var fk = et.FindProperty("ModifiedById");
                Assert.True(fk.IsNullable); // passes
            }
        }
    }

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

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

    public class Post
    {
        public int PostId { get; set; }
        public string Body { get; set; }
        [ForeignKey("AuthorId")] // does not match by convention name
        public User PostedBy { get; set; }
        [ForeignKey("ModifiedById")] // matches by convention name
        public User ModifiedBy { get; set; }
    }

    public class User
    {
        public int Id { get; set; }
        public string Name { get; set; }
    }
}
@smitpatel
Copy link
Contributor

Small observation: If the property name matches by convention then ForeignKeyPropertyDiscoveryConvention will also set the fk properties.

@smitpatel
Copy link
Contributor

Same test results with following models too.
Seems issue arises when fk properties set by some source matches with what convention would find. Not long ago we started matching shadow properties for fk. (See #4591 )

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

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

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Post>(b =>
            {
                b.HasOne(e => e.PostedBy).WithMany().HasForeignKey("AuthorId");
                b.HasOne(e => e.ModifiedBy).WithMany().HasForeignKey("ModifiedById");
            });
    }
}

public class Post
{
    public int PostId { get; set; }
    public string Body { get; set; }
    public User PostedBy { get; set; }
    public User ModifiedBy { get; set; }
}

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

@smitpatel smitpatel changed the title ForeignKeyAttribute changes the nullability of foreign keys in shadow state ForeignKeyDiscoveryConvention changes the nullability of foreign keys in shadow state Mar 25, 2016
@smitpatel smitpatel changed the title ForeignKeyDiscoveryConvention changes the nullability of foreign keys in shadow state Shadow FK created by replicating the property type from principal key are non-nullable Mar 25, 2016
@smitpatel
Copy link
Contributor

Found the issue
When we create shadow property by replicating type from the principal key properties, we use the same type hence the property is non-nullable.
If the property name matches with conventional name then the convention will create property which will use nullable type, hence we use the same property instead of creating one by replicating type. Therefore it works.

@smitpatel
Copy link
Contributor

@AndriySvyryd - Should we just use nullable type while copying from principal key? 😄

@AndriySvyryd
Copy link
Member

@smitpatel Yes, by default FKs are not required and thus should create nullable shadow properties

@rowanmiller rowanmiller added this to the 1.0.0-rc2 milestone Mar 25, 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

5 participants