Flow unique constraints from reverse engineering to Migrations

[smitpatel]

The logic introduced in PR #8089 is inaccurate.

We should not be scaffolding unique index for unique constraints.
Practically they are same (ref: https://stackoverflow.com/questions/2675474/what-is-the-difference-between-a-unique-constraint-and-a-unique-index)

But metadata wise they are different. If there is unique constraint then we should call HasAlternateKey instead of HasIndex.IsUnique` regardless of it being referenced by relationship or not.

Though the underlying issue is #7956 (comment)

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

At present, we never ask for metadata about the constraints to database. We generate PK based on PrimaryKeyOrdinal of column & we generate AK based on the relationship referenced columns. (therefore we incorrectly identify our unique constraints as unique index atm)

It would preserve #7956 too without complicating our logic. In #7956, there is unique index, we don't scaffold IsRequired for unique index since one null value is allowed.
If there is unique Constraint then we should scaffold IsRequired since SqlServer would make columns non-null too.

[ajcvickers]

@smitpatel We should never generate HasAlternateKey; it is never useful. In EF terms, HasAlternateKey is a way of specifying that something is the principal end of the relationship without saying which relationship that is. But you have to tell us that in the relationship API anyway, so HasAlternateKey has no value; it's a pointless API.

So for something like this:

modelBuilder.Entity<Blog>()
    .HasMany(e => e.Posts)
    .WithOne(e => e.Blog)
    .HasPrincipalKey(e => e.BlogId);

You could also add this:

modelBuilder.Entity<Blog>()
    .HasAlternateKey(e => e.BlogId);

and it would build the same model. But there's really no point since...it builds the same model.

Why this is important is that the key values at the principal end of a relationship cannot be modified. So once something is marked with HasAlternateKey it becomes more restrictive. If it really is the principal end, then fine--it's a restriction we have. But if it has been called for something that isn't the principal end, then the model now has a restriction that it doesn't need to have, and one that historically causes issues for applications.

Maybe now is the time to finally get rid of HasAlternateKey? Or at least obsolete it.

[smitpatel]

Maybe now is the time to finally get rid of HasAlternateKey? Or at least obsolete it.

Perhaps. We can remove that API too, not fan of it. While discovering RevEng today, I realized we did not do anything about constraints. In current codebase, if user creates a model using HasAlternateKey to generate database and then reverse engineer it, we would generate HasIndex calls which is quite confusing. And the reasoning is not obvious.

[ajcvickers]

@smitpatel Yep, agree we should look into what constraints mean and whether they should be mapped in a different way.

[bricelam]

We should never generate HasAlternateKey; it is never useful

I disagree with this. The DDL to interact with indexes is different from that of unique constraints. If you want to "take over" managing a database with Migrations, we need to know if it's a unique constraint or an index. Today, we do this by mapping them to either indexes or alternate keys in the metadata.

Another distinction is that SQL Server can add filters to unique indexes, so NULL (and possibly other values) may be handled differently between the two.

[bricelam]

The distinction may also be important for Update From Database

[ajcvickers]

@bricelam Then should we change the semantics of HasAlternateKey in the EF model? Otherwise people are going to continue to get errors from EF that the property cannot be modified when there is no restriction to modification in the database.

[bricelam]

No; I don't think they're any different from primary keys. HasAlternateKey means we maintain an identity map over them which adds this restriction. This seems normal for an O/RM.

If anyone wants to update the values and isn't using them as the target of a foreign key, they can change it to HasIndex and (optionally) run Update-Database

[divega]

Then should we change the semantics of HasAlternateKey in the EF model? ...

No; I don't think they're any different from primary keys. HasAlternateKey means we maintain an identity map over them which adds this restriction. This seems normal for an O/RM...

From my point of view it would be better that the semantics of HasAlternateKey aligned perfectly with the presence of a unique constraint in the database. Why set up the identity map machinery (and the consequent read-onlyness) even in those cases in which we know we won't use it?

After @smitpatel explained this to me yesterday I had to agree with his original comment in this issue. We are second-guessing the user's intent when we reverse engineer a unique constraint as a unique index. The fact that it doesn't round trip well is a smell.

[ajcvickers]

@divega Let's discuss this in the design meeting tomorrow. I think that we should have something that preserves that a constraint was used. Not sure if it is HasAlternateKey with different semantics, or something else.

[ajcvickers]

Discussed this in triage and we decided to obsolete current HasAlternateKey API (see #8702) and instead do something else that allows the unique constrainst metadata to flow through the stack. This should be relational-only code. It might be an annotation on an Index, or on the property directly, possibly also with fluent API.

[divega]

One of the intermediary conclusions from today is that generating HasAlternateKey (or a similar replacement API) in reverse engineering would be more palatable if we implemented mutable alternate keys (#4073).

[ajcvickers]

Triage: Do something else that allows the unique constraints metadata to flow through the stack. This should be relational-only code. It might be an annotation on an Index, or on the property directly, possibly also with fluent API.