Add support for index sort order (descending)#27210
Conversation
If IsDescending is specified, it must explicitly specify all values for all properties.
Are all indexable columns orderable? Couldn't there be some type for which specifying the order could be detrimental, like an encrypted column, for example? |
Throw |
That's an interesting question, although note that in the migration SQL we still generate nothing (just as before this feature) unless you use the new API to specify "descending". In other words, the PR forces the user to specify true/false for all columns, but at the migration level we generate only DESC, never ASC (which is the universal default). So if there's some issue with indexes over encrypted columns, that issue already exists today.
Yeah, I tried that but RelationalModel accesses that IIndex.IsDescending property when building TableIndex - that seems to be a difference between having it as a real property vs. annotation. |
Calculate |
|
And don't forget to update |
|
|
||
| if (index.IsDescending.Any(desc => desc)) | ||
| { | ||
| indexBuilder.IsDescending(index.IsDescending.ToArray()); |
There was a problem hiding this comment.
Keep indexBuilder updated; Fluent API doesn't guarantee that old builders remain valid.
There was a problem hiding this comment.
I was wondering about that - saw some other places where it wasn't updated. May be worth doing a pass to check.
test/EFCore.Design.Tests/Migrations/Design/CSharpMigrationOperationGeneratorTest.cs
Show resolved
Hide resolved
|
Add a test to |
test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs
Show resolved
Hide resolved
6012e3e to
acd885d
Compare
src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs
Outdated
Show resolved
Hide resolved
…r.cs Co-authored-by: Andriy Svyryd <AndriySvyryd@users.noreply.github.com>
test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs
Outdated
Show resolved
Hide resolved
test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| [DebuggerStepThrough] | ||
| public virtual string DisplayName() | ||
| => Name is null ? Properties.Format() : $"'{Name}'"; |
There was a problem hiding this comment.
You can call ((IReadOnlyIndex)this).DisplayName() instead of overriding
There was a problem hiding this comment.
The problem is that recurses into the same call: the public DisplayName implements the interface on IReadOnlyIndex...
Compare EntityType.DisplayName which happens to be private - there it's safe to do it (and I get an IDE warning with Non-public method 'DisplayName' hides method with default implementation in interface IReadOnlyTypeBase). But with Index we want to call DisplayName() from EntityType.cs, so it needs to be public...
We could probably work around this with something fancy, but is it worth it...
There was a problem hiding this comment.
The problem is that recurses into the same call: the public DisplayName implements the interface on IReadOnlyIndex...
That's why I said "instead of overriding", i.e. don't have this method on Index at all.
Compare EntityType.DisplayName which happens to be private
It's private by design 😃
There was a problem hiding this comment.
Ah I see... So this would have to be done here, but also in EntityType.cs where we call this twice... I dunno, overriding seems a bit nicer even if it duplicates the logic - it's very unlikely to change etc.
/cc @AndriySvyryd would appreciate a good review as this touches upon lots of metadata/model stuff, and is non-annotation array metadata property.
Closes #4150