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

Log a warning with using a non-nullable property by convention for an explicitly optional FK #19958

Open
Tracked by #22952
quixoticaxis opened this issue Feb 18, 2020 · 2 comments

Comments

@quixoticaxis
Copy link

quixoticaxis commented Feb 18, 2020

When we have two entities:

public class Container
{
    public long InnerId { get; set; }

    public List<Item> Items { get; set; } = default!; // still set to not null during the context initialization
}

public class Item
{
    public long InnerId { get; set; }

    // public long ContainerId { get; set; }

    public Container? Container { get; set; }
}

if we configure the model as follows:

modelBuilder.Entity<Container>
    .HasKey(container => container.InnerId);
modelBuilder.Entity<Item>
    .HasKey(item => item.InnerId);
modelBuilder.Entity<Item>
    .HasOne(item => item.Container)
    .WithMany(container => container!.Items);

we expect the model to allow one or less Container to be linked to a single Item, which is explicitly defined by the nullable reference type of Item.Container property. This works, the nullable integer attribute CountainerInnerId is generated in the database to support the desired relation.

If we uncomment public long ContainerId { get; set; }, EF Core finds out that the not nullable property with the "conventional" (<principal entity name>Id) name exists and generates the model that links any Item to exactly one container. Even adding IsRequired(false) does not prevent EF from doing it.

I wasn't able to understand whether it is the by-design behavior or a bug by reading the documentation, but, IMHO, it seems very counter-intuitive that the existence of conventionally named property completely "overrides" manual fluent configuration. I was expecting the shadow foreign key to be introduced to satisfy the desired nullable semantics.

EF Core version: 3.1.1
Database provider: tested it with SQLite (Microsoft.EntityFrameworkCore.Sqlite 3.1.1)
Operating system: Windows 10 Pro 64 bit
IDE: VSCode

@AndriySvyryd
Copy link
Member

@quixoticaxis Do you want to actually use ContainerId as the foreign key? If so it would be impossible to assign it a null value to indicate that there's no associated Container. Otherwise you would need to explicitly specify the property you wish to use for the foreign key.

We could consider logging a warning for this case.

@quixoticaxis
Copy link
Author

quixoticaxis commented Feb 19, 2020

@AndriySvyryd no, I was expecting EFCore to introduce a shadow foreign key, because none of the existing properties can support the desired relation (exactly as it would do when ContainerId is commented out). It was a surprise that EF silently ignores the desired relation semantics defined by Has/With methods and nullable types.
Although I personally would obviously prefer shadow foreign key generation, the warning you suggest would also be great. Something along the lines The foreign key that was found by following the naming convention cannot support the desired relation: Item.Container is of nullable type Container?, but Item.ContainerId is of non-nullable type System.Int64. I can understand that it silently ignores the nullable reference type as the language feature is still young and the uniform behavior (for both the projects that utilize nullable reference types and for the projects that do not) seems reasonable, but I was really surprised that even with explicit IsRequired(false) EF does not raise any warnings.

@AndriySvyryd AndriySvyryd changed the title Code First model prefers "conventional" properties to explicitely defined relations Log a warning with using a non-nullable property by convention for an explicitly optional FK Feb 19, 2020
@ajcvickers ajcvickers added this to the Backlog milestone Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants