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

Add index on foreign key properties by convention #3764

Merged
merged 1 commit into from
Nov 17, 2015
Merged

Conversation

smitpatel
Copy link
Member

Implements #700

foreach (var index in Metadata.GetIndexes().Where(i => i.Properties.Contains(property)).ToList())
{
var removed = RemoveIndex(index, configurationSource);
Debug.Assert(removed.HasValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this assert fail if the application creates an index explicitly on a property created by convention?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just moved the code block here so that we detach relationships before removing indexes because detaching relationship will remove FK and conventional index defined for that FK.
This assert should never fail. This is RemoveProperty function. When we add any component in the model, we update the configuration source (CS) of the property to have highest CS of all component defined using this property. At the start of this function we check if given CS is able to remove this property. If it can remove the property then all components defined using this property should get removed. Hence assert should always pass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I didn't know we did this: "we update the configuration source (CS) of the property to have highest CS of all component defined using this property."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajcvickers Yes, we do that for any referenced member of the model. However we don't downgrade the CS if the referencing member is removed, that's something that would be fixed with #2869

@ajcvickers
Copy link
Member

:shipit:

@@ -49,5 +50,7 @@ public virtual ConfigurationSource UpdateConfigurationSource(ConfigurationSource

[UsedImplicitly]
private string DebuggerDisplay => Property.Format(Properties);

public virtual bool IsInUse() => DeclaringEntityType.FindForeignKeys(Properties).Any();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to call FindForeignKeysInHierarchy() instead to account for FKs on derived types.

@AndriySvyryd
Copy link
Member

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants