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

EF Core 7 Rev eng: Missing HasForeignKey when referencing a navigation property with a unique constraint with DataAnnotations on #29418

Closed
ErikEJ opened this issue Oct 25, 2022 · 18 comments
Assignees
Labels
area-scaffolding closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression Servicing-approved type-bug
Milestone

Comments

@ErikEJ
Copy link
Contributor

ErikEJ commented Oct 25, 2022

File a bug

Using EF Core 7 SQL Server reverse engineering with "Use Data annotations" enabled, the foreignkey configuration is missing.

See description and link to repro schema here: ErikEJ/EFCorePowerTools#1554

@bricelam

EF Core version: 7.0 RC2
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 6
Operating system: Windows

@bricelam
Copy link
Contributor

bricelam commented Oct 25, 2022

@AndriySvyryd Is it by design that this model creates a new Car.ColorCode1 property (instead of just using the existing ColorCode property) for the relationship?

class TestContext : DbContext
{
    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Car>().HasOne(d => d.Color).WithMany(p => p.Cars)
            .HasPrincipalKey(p => p.ColorCode);
    }
}

class Car
{
    public int Id { get; set; }

    public string? ColorCode { get; set; }
    public virtual Color? Color { get; set; }
}

class Color
{
    public int Id { get; set; }

    public string ColorCode { get; set; } = null!;
    public virtual ICollection<Car> Cars { get; } = new List<Car>();
}

@AndriySvyryd
Copy link
Member

Yes, we only match these patterns:

  • [navigation property name][principal key property name]
  • [navigation property name]Id
  • [principal entity name][principal key property name]
  • [principal entity name]Id

@ajcvickers
Copy link
Contributor

@AndriySvyryd In that case, why do we create a foreign key name by convention (ColorCode--the principal key name) that by default isn't something we match? That seems inconsistent--if we don't match it, we shouldn't create the thing we don't match.

@TimKras
Copy link

TimKras commented Oct 26, 2022

I just wanted to add that this problem only occurs in EF Core 7 (RC).
It is working fine in EF Core 6.

@AndriySvyryd
Copy link
Member

@ajcvickers Because naming it ColorColorCode looked wrong. However, using the same logic for FK discovery might be unexpected. But we could consider using it as long as it doesn't match PK properties.

@ajcvickers
Copy link
Contributor

Notes from triage: ideally, the rules would not result in this behavior. However, rather than making the rules even more complex we decided that it would be better here for scaffolding to respect the existing rules, as it did in EF Core 6.0.l

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Oct 27, 2022

No plan to fix for 7.0??

@ajcvickers
Copy link
Contributor

@ErikEJ We are going to consider patching.

@bricelam
Copy link
Contributor

Stranger and stranger...

Trying to configure it with an attribute doesn't work.

// Still not used. Still get shadow property 'ColorCode1'
public string? ColorCode { get; set; }

[ForeignKey(nameof(ColorCode))]
public virtual Color? Color { get; set; }

@ajcvickers
Copy link
Contributor

What did 6.0 scaffold?

@bricelam
Copy link
Contributor

Scaffold? It never scaffolded data annotations when the principal key is non-primary. It always used fluent API.

But I'm just playing with model building to see which data annotations we can scaffold. I'll just work around the [ForeignKey] bug for now, and we can address it separately.

@bricelam
Copy link
Contributor

Filed #29448

bricelam added a commit to bricelam/efcore that referenced this issue Oct 27, 2022
…nate key

Also adds coverage for more scenarios involving alternate principal keys

Fixes dotnet#29418
@bricelam bricelam added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 27, 2022
bricelam added a commit to bricelam/efcore that referenced this issue Nov 3, 2022
…nate key

Also adds coverage for more scenarios involving alternate principal keys

Fixes dotnet#29418
@ajcvickers ajcvickers removed this from the 8.0.0 milestone Nov 15, 2022
@TimKras
Copy link

TimKras commented Nov 15, 2022

@ajcvickers will this be patched in version 7? If so, is it possible to give an estimation on the timing?

@ajcvickers
Copy link
Contributor

ajcvickers commented Nov 15, 2022

@TimKras Possibly, although you are the only person to have reported it so far, so right now it might not meet the customer impact bar required for a servicing fix.

@ajcvickers
Copy link
Contributor

Note from triage: this issue covers a wider range of cases than initially thought; we should patch.

@bricelam
Copy link
Contributor

bricelam commented Dec 8, 2022

🩹 Workaround

I may be stating the obvious here, but one workaround is to not use data annotations when scaffolding. Or, you can manually add HasForeignKey to the scaffolded code after its generated.

@TimKras
Copy link

TimKras commented Dec 17, 2022

🩹 Workaround

I may be stating the obvious here, but one workaround is to not use data annotations when scaffolding. Or, you can manually add HasForeignKey to the scaffolded code after its generated.

A better temporary workaround is adding the missing HasForeignKey in the OnModelCreatingPartial method, so it not overridden each time the context is scaffold:

partial void OnModelCreatingPartial(ModelBuilder modelBuilder)
{
     modelBuilder.Entity<Car>().HasOne(d => d.Color).WithMany(p => p.Cars)
        .HasForeignKey(p=>p.ColorCode)
        .HasPrincipalKey(p => p.ColorCode);
}

@TimKras
Copy link

TimKras commented Feb 20, 2023

Confirmed fixed in 7.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-scaffolding closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression Servicing-approved type-bug
Projects
None yet
Development

No branches or pull requests

5 participants