Fix for 21089. Allow multiple indexes on the same properties. #21115
Conversation
src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs
Outdated
Show resolved
Hide resolved
|
@AndriySvyryd Overall I'm worried about separating the single concept of Our reason for doing this is solely historical - we originally created indexes as unique on the list of properties they represented - this seemed like a good idea because that's what's important for helping speed up queries, the name was just a handy way of referring to it. Now you're suggesting separating by name because SQL databases don't allow you to create multiple indexes with the same name on the same table. But, as I mentioned before, if we have Entity-Splitting it's quite possible that they could have the same name on different tables, despite the fact that both indexes would be on the same EntityType. And non-SQL databases can do whatever they want. But I think customers will continue to see them as one concept and we run the risk that the split will seem artificial and illogical - after all they always had names on the database - so "unnamed" is solely an EF concept. I'd like to make sure that we can write understandable docs on how to use this. |
|
@lajones At the fluent API level no separation is needed. The problem is in Metadata API where it's not as clear that |
Design meeting decision 6/3/2020 - we decided not to rename |
| @@ -19,34 +19,38 @@ namespace Microsoft.EntityFrameworkCore | |||
| public static class RelationalIndexExtensions | |||
| { | |||
| /// <summary> | |||
| /// Returns the name for this index. | |||
| /// Returns the name of the index on the database. | |||
There was a problem hiding this comment.
in the database or on the database?
| /// </returns> | ||
| IConventionIndexBuilder HasIndex( | ||
| [NotNull] IReadOnlyList<IConventionProperty> properties, | ||
| [NotNull] string name, |
There was a problem hiding this comment.
HasIndex everywhere else takes CanBeNull name.
| => AddIndex( | ||
| new[] { property }, configurationSource); | ||
| new[] { property }, name, configurationSource); |
There was a problem hiding this comment.
This pass null value to a parameter which is marked as NotNull.
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| public Index( | ||
| [NotNull] IReadOnlyList<Property> properties, |
|
What is overall decision on HasIndex which takes name? Is that name nullable or not? The changes in this PR has mismatch in various APIs over that parameter value. |
|
Name should not be nullable in any API |
Agree with @AndriySvyryd. They were originally, deliberately |
|
PR #21138 addresses the comments above. |
| /// <returns> An object that can be used to configure the index. </returns> | ||
| public new virtual IndexBuilder<TEntity> HasIndex( | ||
| [NotNull] string[] propertyNames, | ||
| [CanBeNull] string name) |
| /// <returns> An object that can be used to configure the index. </returns> | ||
| public virtual IndexBuilder<TEntity> HasIndex( | ||
| [NotNull] Expression<Func<TEntity, object>> indexExpression, | ||
| [CanBeNull] string name) |
| /// <returns> An object that can be used to configure the index. </returns> | ||
| public virtual IndexBuilder HasIndex( | ||
| [NotNull] string[] propertyNames, | ||
| [CanBeNull] string name) |
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| public virtual Index FindDeclaredIndex([NotNull] string name) | ||
| => _namedIndexes.TryGetValue(name, out var index) |
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| public virtual IEnumerable<Index> FindIndexesInHierarchy([NotNull] string name) | ||
| => ToEnumerable(FindIndex(name)).Concat(FindDerivedIndexes(name)); |
Fixes #21089.
Name is now part of the identifier for an index. This allows multiple indexes over the same properties so long as they have different names.
Added suitable overloads for creating such an index, and added
FindIndexoverloads for finding them.Indexno longer allowsHasNameto manipulate that identifying name, but we do have aHasDatabaseNameextension API which stores the name by which the user wants this index to be referred to in SQL as an annotation on theIndex. This is helpful in situations such as an anonymous index has been created when creating a foreign key and the user wants to provide a name for that index in the database different from the one which would be generated automatically.One issue: in
EntityTypeBuilderthe APIs for creating a nameless and namedIndexfor a list of property names are contrasted below:Note that the
propertyNamesparameter is aparamsparameter for nameless indexes, but cannot be so for named ones. The same issue affectsEntityTypeBuilder<T>, however there it is more natural to use expressions to define the index properties anyway.