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

Flow unique constraints from reverse engineering to Migrations #8645

Closed
smitpatel opened this Issue May 31, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@smitpatel
Copy link
Contributor

smitpatel commented May 31, 2017

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

This comment has been minimized.

Copy link
Member

ajcvickers commented May 31, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

smitpatel commented May 31, 2017

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

This comment has been minimized.

Copy link
Member

ajcvickers commented May 31, 2017

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

@bricelam

This comment has been minimized.

Copy link
Member

bricelam commented May 31, 2017

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

This comment has been minimized.

Copy link
Member

bricelam commented May 31, 2017

The distinction may also be important for Update From Database

@ajcvickers

This comment has been minimized.

Copy link
Member

ajcvickers commented May 31, 2017

@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

This comment has been minimized.

Copy link
Member

bricelam commented May 31, 2017

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

This comment has been minimized.

Copy link
Member

divega commented May 31, 2017

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

This comment has been minimized.

Copy link
Member

ajcvickers commented May 31, 2017

@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 ajcvickers changed the title RevEng: Generate HasAlternateKey for Unique Constraints Flow unique constraints from reverse engineering to Migrations Jun 2, 2017

@ajcvickers

This comment has been minimized.

Copy link
Member

ajcvickers commented Jun 2, 2017

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

This comment has been minimized.

Copy link
Member

divega commented Jun 3, 2017

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).

@smitpatel smitpatel removed this from the Backlog milestone Jun 3, 2017

@ajcvickers ajcvickers added this to the Backlog milestone Jun 5, 2017

@ajcvickers ajcvickers closed this Sep 7, 2017

@ajcvickers

This comment has been minimized.

Copy link
Member

ajcvickers commented Sep 7, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.