Column treated as nullable if its name is <Entity>Id

[stevendarby]

If I create an initial migration for the below context with 8.0.10, EntityId is created as nullable, even though it is marked as required, and the index is created with a NULL filter. If I rename it to XEntityId, it works as expected.

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

await using var context = new MyContext();
await context.Database.MigrateAsync();

public class MyContext : DbContext
{
    public DbSet<Entity> Entities { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .LogTo(Console.WriteLine)
            .UseSqlServer("Data Source=(LocalDb)\\MSSQLLocalDB;Initial Catalog=Migrations;Integrated Security=SSPI");
}

[Index(nameof(EntityId), IsUnique = true)]
public class Entity
{
    [Key]
    [DatabaseGenerated(DatabaseGeneratedOption.None)]
    public int Key { get; set; }

    [Required]
    [MaxLength(50)]
    public required string EntityId { get; set; }
}

[roji]

Confirmed, this also repros on 9.0.0-rc.2:

CREATE TABLE [Entities] (
    [Key] int NOT NULL,
    [EntityId] nvarchar(50) NULL,
    CONSTRAINT [PK_Entities] PRIMARY KEY ([Key])
);

Renaming EntityId indeed makes it

[ChrisJollyAU]

This is actually due to multiple places configuring the property's IsNullable (or rather its private backing field _isNullable)

First off, we have the KeyDiscovererConvention. At the point it runs there are no other annotations or primary key set. At this point it picks up the property called <Entity>Id where <Entity> is the same as the class name. It configures this as the primary key and as part of this configures the property as being required (aka _isNullable = false).

Then we end up configuring the [Required] attribute. This also sets the _isNullable property to false.

Later on we end up configuring the [Key] attribute on the property Key. As part of this it removes the EntityId property from being the key and resets the nullability of the property. This is the key (pun intended) part in that at this point it doesn't have enough info to know that there was another configuration that should take precedence and stay in place. Thus, it ends up clearing the nullability to the default of the clr type for the property. In this case string which is has a default of being nullable

Some comments around where the property is being set refer to #15898 so potentially has been around a while

I did do an experiment to fix it. See this commit ChrisJollyAU@3389369#diff-04ba2d88229db85ef2e15bbe7e1e8dedc22fdb5817920c0898125116b428d3b1

Basically, adds a private variable which tracks if we have configured the _isNullable specifically from the Required attribute. Note configurationSource is too broad as multiple attributes on the property can configure the nullability (at least both Key and Required)

[stevendarby]

@ChrisJollyAU great write up!

It's also my understanding that the Required attribute shouldn't really be needed when nullable reference types are enabled, because it's "string" rather than "string?". When that didn't work, I added the Required attribute, and when that didn't work, I knew there must be a bug.

So, unless I'm mistaken with the NRT thing, there might be another angle to fix.

[AndriySvyryd]

The general fix for this type of issue is #15898, which would preserve configuration instead of overwriting it. And I already made a note that the current solution is suboptimal: https://github.com/dotnet/efcore/blob/main/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs#L349

But I think we could make this scenario work by changing the https://github.com/dotnet/efcore/blob/main/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs#L352 line to be property.Builder.IsRequired(null, ConfigurationSource.Convention);

As to the experimental fix by @ChrisJollyAU - we don't want to change our API surface for something like this

[ChrisJollyAU]

@AndriySvyryd Yes, that will work - just tested it.

But yes, to fix the whole class of this type of issue you would need to fix that other issue

[stevendarby]

@AndriySvyryd would you accept a PR with that fix for the time being?

[AndriySvyryd]

Sure, as long as it includes a test that would probably go into https://github.com/dotnet/efcore/blob/main/test/EFCore.Specification.Tests/DataAnnotationTestBase.cs