Reverse engineering: Scaffold-DbContext can generate IsRequired() on nullable unique column if it is also an FK

[sonphnt]

Hi Everyone,

I have a column with unique index like this.

Name (nvarchar(100), null)
CREATE UNIQUE NONCLUSTERED INDEX [IX_Account_Name_unique] ON [dbo].[Account]
(
	[Name] ASC
)

The class generated by Scaffolding look like this

entity.HasIndex(e => e.Name)
                    .HasName("IX_Account_Name_unique")
                    .IsUnique();

entity.Property(e => e.Name)
                    .IsRequired()
                    .HasMaxLength(100);

Why it added IsRequired() into Name property?
Is it a bug? And is it possible to do a unique nullable column in Entity Framework?

Thanks

[sonphnt]

If I set like this
Name (nvarchar(100), not null)

There is no IsRequired() added. It looks weird. It should be the other way around.

[ErikEJ]

What is the value of the "nullable" column for that table/column when you run this:

https://github.com/aspnet/EntityFramework/blob/dev/src/EFCore.SqlServer.Design/Internal/SqlServerDatabaseModelFactory.cs#L322

[sonphnt]

Hi @ErikEJ What do you mean. How can I run what you mean in your link?

[ErikEJ]

In SQL Server management studio against your database

[sonphnt]

Name column nullable value = 1 in SSMS

[ErikEJ]

So it is currently ?

Name (nvarchar(100), null)

[sonphnt]

Yes, exactly
Name (nvarchar(100), null)

[sonphnt]

Did you find anything @ErikEJ ?
Thanks

[ajcvickers]

We discussed this in triage and decided that we should reverse engineer what the schema says even if it is not something we would generate by default. We should also look at whether or not this should generate warnings in model validation.

[sonphnt]

Hi @ajcvickers .

Yes the flow should be reversed with SQL schema. Column allow null then it generated IsRequired(), Column "not null" then there was no IsRequired() specification.

For now after scaffolding I have to change DbContext file manually to make EF querying correctly.

Thanks

[lajones]

Note: this doesn't happen if you don't create the index; then Name is correctly not assigned the IsRequired() fluent API.

What's going on here is that in RelationalScaffoldingModelFactory.VisitIndex() the scaffolder is correctly calling HasAlternateKey() on the EntityType to indicate the unique index. However this sets all the key properties to Required at InternalEntityTypeBuilder.L177.

Need to have a look at that code and maybe allow a new API HasAlternateKey(string propertyNames, bool allowsNulls) which does not automatically set all of its properties to Required?

[ajcvickers]

@lajones Just because there is a unique index does not mean that there should be an alternate key. There should only be an alternate key if the the properties are at the principal end of a foreign key constraint.

[lajones]

OK. Then we should be calling HasIndex() instead of HasAlternateKey(). I've assigned this to myself to update.

[lajones]

I've been looking at this offline. It's OK to not call HasAlternateKey() and it turns out we were already calling HasIndex().

If the index is just a separate independent (i.e. not supporting an FK) index then this causes no problems. But if the index is on the principal end of an FK (and includes a nullable column) then we see an exception from the Scaffolder:

A key on entity type 'TestEntity' cannot contain the property 'TestProperty' because it is nullable/optional. All properties on which a key is declared must be marked as non-nullable/required

This is because the Scaffolder tries to add an alternate key on the Entity where the principal end of the FK is located and the property has been set up as nullable (previously it wasn't nullable because the index, through the EntityBuilder, was setting all such properties to Required). Keys in EF are expected to be on non-nullable properties. So the question becomes what do we scaffold in the above situation.

Options:

1. Exclude such FKs (and any calls they make to set up alternate keys) from the generated model.
2. Force the property to be Required in the model even though it’s not on the database – but causes problems if the runtime encounters a NULL from the database.
3. Do not add the alternate key, but model the FK anyway - I’m pretty sure this is legal i.e. will produce code which compiles – but, as above, will cause problems if the runtime ever encounters a NULL.

In all cases I think we should log a warning from the Scaffolder saying it has encountered the situation. For options 2 and 3 we should consider also warning in model validation.

@ajcvickers - you asked what Scaffolding would do if it came across a primary key column which is nullable. Firstly, this is not possible to set up in a SQL database as of SQL-92 (and maybe earlier) - the CREATE/CONSTRAINT statements don't allow it. But secondly if you fake it using the TableModel, ColumnModel classes directly then we end up calling EntityTypeBuilder.HasKey() on the set of generated properties which at InternalEntityTypeBuilder.cs#L177 sets the generated properties to be Required (so like option 2 above).

[ajcvickers]

@lajones That's what I thought would happen. I think option #2 is the correct thing to do for alternate keys as well. I think this should also generate a warning because, as you say, if there are nulls in the data then there will be problems when using this at runtime.

[lajones]

OK. Will go ahead with that. @sonphnt - the effect this will have is that the property Name should come out as not Required except if you use it as the principal end of a foreign key.

[divega]

@ajcvickers @lajones FWIW, nullable alternate keys (i.e. nullable columns in a unique constraint) are legal in the SQL standard and seems to be supported in at least some databases. Even in SQL Server it is legal, although the behavior does not adhere to the standard in the sense that NULLs values are not completely ignored. The idea is something like "I sometimes don't know the value of this column, but when I do, it uniquely identifies the row".

My 👍 is because I agree it should be ok to restrict properties in alternate keys to be required in EF Core unless or until we have evidence that it is important to customers to remove this restriction (I am assuming this is what we have right now, and it would be somewhat expensive to change it, but I wanted you to keep this in mind).

[popcatalin81]

@divega @ajcvickers Please don't force IsRequired on nullable unique columns.

I can give a clear example where this would create a blocking issue for the app I'm working on.

There is an Users table with (ID pk, UserName NOT NULL Unique, EMail NULL Unique). In this design the email is optional but also required to be unique. Also null in the email column is used to drive different business processes (IE: different password reset process) and String Empty won't work (due to uniqueness constraint) and filling in random unique values to please EF would be a terrible workaround.

[sonphnt]

@popcatalin81 I agree. There are some cases we need nullable unique columns in business logic. Somethings like Either we dont have a value but if it does, it should be unique. Your example has reflected exactly a business logic in enterprise application

I think nullable unique columns is legal point in SQL standard. If not Microsoft SQL server should not allow users creating unique index like that in database.

@lajones The second option, what does it mean? I dont get it. My example is on Name property column but we can have a foreign key id. By making this column nullable unique that will make the column become a "one-zero,one" relationship. And second thing Required() on a column that will make EF generate a query with INNER JOIN between 2 tables base on that Required column. We expect a "LEFT JOIN" here in this scenario due to Nullable unique column.

One of many examples between 2 tables about that:
SecurityCard(Id, Code, CreatedDate, ExpireDate, so on...)
Employee (Id, Name, Age, Gender, SecurityCardId, so on...)

If we do not set Nullable unique column on SecurityCardId

var employee = dbContext.Employee.Include(x=>x.SecurityCard).FirstOrDefault(x=>x.Id == 123);
// To get information of SecurityCard we have to do => employee.SecurityCard[0].XXX

The associate/parttime employees do not have SecurityCardId. but an official employee of company will have one security card and the card must be unique for everyone.

[ajcvickers]

@sonphnt @popcatalin81 We are not intending to disallow nullable unique columns. Nor is it about nullable unique columns that are part of a foreign key. The conversation in the recent posts is only about cases where there is a foreign key constraint where the constraint references columns that are not the primary key. Example:

• Blog

  • Id int PK
  • BlogName string Not PK

• Post

  • Id int PK
  • OwnerBlogName string FK

FK constraint from OwnerBlogName on Post (the FK) to BlogName on Blog (not the PK)

In this case EF cannot handle nulls in BlogName on Blog. Nulls in the OwnerBlogName (the FK) on Post are allowed.

@divega The state manager treats PKs and alternate keys in the same way--no nulls and values cannot be changed. Both of these restrictions could be relaxed. We just haven't done the work to do it. Also, we would have to understand the meaning of null, but if it just means that no FK will ever match it, then that should be fine and not difficult to implement.

[lajones]

@sonphnt @popcatalin81 As @ajcvickers says above - it is only nullable, unique columns on the prinicipal end of the FK that are affected. Nullable, unique columns on the dependent end are perfectly normal and are unaffected by the plan above. Both of your situations are describing the dependent end.

[lajones]

Fixed by PR #8089.

[divega]

@divega The state manager treats PKs and alternate keys in the same way--no nulls and values cannot be changed. Both of these restrictions could be relaxed. We just haven't done the work to do it. Also, we would have to understand the meaning of null, but if it just means that no FK will ever match it, then that should be fine and not difficult to implement.

@ajcvickers Good to know. I see relaxing those two restrictions (nullability and mutability) on alternate keys as orthogonal improvements. And yes, I believe a null alternate key value could not participate in relationships. I tested it in SQL Server and null values on the principal and dependent keys do not form a relationship. Do you think it is worth creating an issue for this?

[ajcvickers]

@divega Yes. We should have issues for both relaxations, although we might have one for the mutability already on the backlog.

[divega]

@ajcvickers FYI, nullability is already covered in #4415 and mutability in #4073.