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

Allow multiple indexes on the same properties #21089

Closed
ajcvickers opened this issue May 30, 2020 · 5 comments · Fixed by #21115
Closed

Allow multiple indexes on the same properties #21089

ajcvickers opened this issue May 30, 2020 · 5 comments · Fixed by #21115
Assignees
Labels
area-model-building breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@ajcvickers
Copy link
Member

Currently the EF model identifies indexes by an ordered list of properties. This means that only a single index can be defined over that ordered list of properties. This issue is about changing this in the EF model so that indexes are identified by a name. This will allow multiple indexes to be defined over the same ordered list of properties, which in the turn will allow issues such as #4150 to be fixed.

@lajones
Copy link
Contributor

lajones commented May 31, 2020

Just want to record a limitation of this approach. (I do not think this limitation should change the approach mentioned above - I just want to record it).

It will not be possible to define 2 "nameless" indexes (where "nameless" means that we do not assign a name at design time, and EF chooses the name when it needs to create the SQL CREATE INDEX statement) over the same properties which differ only in, for instance, the ASC/DESC or collation mentioned in #4050. So you if you were to do e.g.

entityBuilder.HasIndex(listOfProperties);
entityBuilder.HasIndex(listOfProperties).HasAscendingDescending(listOfAscDesc);

you would not be creating 2 different indexes, instead the 2nd line will update the 1st. This is what we do now for IsUnique() so that's consistent. And it's not a huge problem - if you want to create 2 such indexes you'll just need to do:

entityBuilder.HasIndex(listOfProperties, name1);
entityBuilder.HasIndex(listOfProperties, name2).HasAscendingDescending(listOfAscDesc);

Also, since it is now part of the identifier of an Index, the name should be immutable. If we were doing this from scratch, I'd prefer to remove or Obsolete the HasIndex(propertyList) and HasName(string) APIs in favor of a new HasIndex(listOfProperties, indexName) method. However we cannot do that - we have to keep the old APIs around for backwards compatibility with user code and old snapshot files.

Note: we'll also need to mention that:

[Index("A", "B")]
public class SomeEntity { ... }

followed by

entityBuilderForSomeEntity.HasIndex(new List<Property>(A, B), "SomeIndexName");

will create 2 different indexes.

@ajcvickers ajcvickers added this to the 5.0.0 milestone Jun 1, 2020
@lajones
Copy link
Contributor

lajones commented Jun 1, 2020

Decisions from meeting 6/1/2020:

  • Going ahead with approach above.
  • We can Obsolete the HasName method. In commit 037755e (6/7/17) we updated the migrations code to disable the Obsolete warnings (by outputting #pragma warning disable 612, 618) - so we will not break old migrations code. User code will need updating (to new HasDatabaseName API or to add the same pragma).
  • We still need to keep (i.e. will not Obsolete) HasIndex(listOfProperties).

@lajones
Copy link
Contributor

lajones commented Jun 1, 2020

Linking to #4050 - the change to add [IndexAttribute] - and its PR #21012.

@lajones lajones added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 4, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview7 Jun 22, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview7, 5.0.0 Nov 7, 2020
@danports
Copy link

I noticed after upgrading a project to EF Core 5.0 from 3.1 today I suddenly had 500+ compiler warnings from old migrations. It would be nice if the framework did some minimal cleanup on old migrations. I ran this to fix them:

var path = @"C:\Users\Me\Source\Repos\MyProject\Data\Migrations";
foreach (var file in Directory.EnumerateFiles(path, "*.Designer.cs"))
{
    var filePath = Path.Combine(path, file);
    Console.WriteLine($"Processing {filePath}...");
    var contents = File.ReadAllLines(filePath).ToList();
    var pragmaDisable = "#pragma warning disable 612, 618";
    var pragmaRestore = "#pragma warning restore 612, 618";
    if (!contents.Any(x => x.Contains(pragmaDisable)))
        contents.Insert(contents.FindIndex(x => x.Contains("protected override void BuildTargetModel")) + 2, pragmaDisable);
    if (!contents.Any(x => x.Contains(pragmaRestore)))
        contents.Insert(contents.Count - 3, pragmaRestore);
    File.WriteAllLines(filePath, contents, Encoding.UTF8);
}

@AndriySvyryd
Copy link
Member

@danports #18557 will handle this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-model-building breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants