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

Update 5.0 required dependent docs to include information about NRTs #3448

Closed
craigbrown opened this issue Sep 25, 2021 · 4 comments · Fixed by #3468
Closed

Update 5.0 required dependent docs to include information about NRTs #3448

craigbrown opened this issue Sep 25, 2021 · 4 comments · Fixed by #3468

Comments

@craigbrown
Copy link

I set up this code-first model with EF Core 3.1, with Nullable Reference Types enabled in my project:

public class Address
{
    [Key]
    public int AddressId { get; set; }

    public string AddressLine1 { get; set; } = null!;

    public string City { get; set; } = null!;
}

public class Company
{
    [Key]
    public int CompanyId { get; set; }

    public string Name { get; set; } = null!;

    public Address Address { get; set; } = null!;
}

When I did Add-Migration, the Address property of Company was correctly recognised as not nullable.

If I upgrade to either EF Core 5 or EF Core 6 Preview, making no other changes, and run Add-Migration again, I get this in the resulting migration class:

protected override void Up(MigrationBuilder migrationBuilder)
{
    ...

    migrationBuilder.AlterColumn<int>(
        name: "AddressId",
        table: "Company",
        type: "int",
        nullable: true,
        oldClrType: typeof(int),
        oldType: "int");

    ...
}

The Address property is now incorrectly recognised as nullable. Even if I add the [Required] attribute to that property, the same thing happens. (This migration also includes some code to alter the foreign key, which I assume is a knock-on effect of Address being recognised as nullable).

Why is this happening? I've read the documentation on Required and Optional Properties and Working with Nullable Reference Types but I can't see anything relevant - maybe I'm missing something?

I see that some changes were made to handle NRT projects in EF Core 5/6, but this seems like a regression.

Minimal Reproducible Example

  1. Clone this repository: EFCoreMigrationTest
  2. Update MyContextFactory with a connection string for a blank SQL Server database.
  3. You can use the git commit history to go back to before the migrations were run so you can run them for yourself.

Provider and version information

EF Core version: 6.0.0-rc.1.21452.10
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 6.0
Operating system: Windows
IDE: Visual Studio 2022 17.0 Preview 4.0

@ajcvickers
Copy link
Member

@AndriySvyryd @roji I am able to reproduce this. The Customer.AddressId FK is required in 3.1.19, but optional in 5.0.10 and 6.0 when NRTs are enabled. When NRTs are disabled, the FK is nullable in both cases.

I suspect it is because of this: Required on the navigation from principal to dependent has different semantics. Address is a navigation from the principal to the dependent. This means that starting with 5.0 we no longer use its requiredness to change the FK requireness. However, we should probably call out in the breaking change documentation that this applies to NRTs inferences as well as attributes.

Code:

public static class Your
{
    public static string ConnectionString = @"Data Source=(LocalDb)\MSSQLLocalDB;Database=SixOh";
}

public class Address
{
    public int AddressId { get; set; }
}

public class Company
{
    public int CompanyId { get; set; }
    public Address Address { get; set; } = null!;
}

public class MyContext : DbContext
{
    public DbSet<Company> Company { get; set; } = null!;

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlServer(Your.ConnectionString);
    }
}

public class Program
{
    static void Main(string[] args)
    {
        using var context = new MyContext();
        Console.WriteLine(context.Model.ToDebugString());
    }
}

3.1 model:

Model:
  EntityType: Address
    Properties:
      AddressId (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
    Keys:
      AddressId PK
  EntityType: Company
    Properties:
      CompanyId (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
      AddressId (no field, int) Shadow Required FK Index
    Navigations:
      Address (Address) ToPrincipal Address
    Keys:
      CompanyId PK
    Foreign keys:
      Company {'AddressId'} -> Address {'AddressId'} ToPrincipal: Address

5.1.10 model:

  EntityType: Address
    Properties:
      AddressId (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
    Keys:
      AddressId PK
  EntityType: Company
    Properties:
      CompanyId (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
      AddressId (no field, Nullable<int>) Shadow FK Index
    Navigations:
      Address (Address) ToPrincipal Address
    Keys:
      CompanyId PK
    Foreign keys:
      Company {'AddressId'} -> Address {'AddressId'} ToPrincipal: Address ClientSetNull
    Indexes:
      AddressId

@AndriySvyryd AndriySvyryd self-assigned this Sep 29, 2021
@ajcvickers
Copy link
Member

Update: This is fixed in RC2. Duplicate issue: dotnet/efcore#23588.

Model:

Model:
  EntityType: Address
    Properties:
      AddressId (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
    Keys:
      AddressId PK
  EntityType: Company
    Properties:
      CompanyId (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
      AddressId (no field, int) Shadow Required FK Index
    Navigations:
      Address (Address) ToPrincipal Address
    Keys:
      CompanyId PK
    Foreign keys:
      Company {'AddressId'} -> Address {'AddressId'} ToPrincipal: Address Cascade
    Indexes:
      AddressId

@ajcvickers
Copy link
Member

@AndriySvyryd Should we still update the docs?

@AndriySvyryd
Copy link
Member

Yep

@ajcvickers ajcvickers changed the title Add-Migration configures non-nullable entity property as nullable after upgrade from EF Core 3.1 Update 5.0 required dependent docs to include information about NRTs Sep 30, 2021
@ajcvickers ajcvickers transferred this issue from dotnet/efcore Sep 30, 2021
@ajcvickers ajcvickers added this to the 6.0.0 milestone Oct 1, 2021
@AndriySvyryd AndriySvyryd removed their assignment Oct 9, 2021
AndriySvyryd added a commit that referenced this issue Oct 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants