-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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 TPT support to model building, migrations and update pipeline #20938
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
20% done 🗡️
src/EFCore.Relational/Extensions/RelationalEntityTypeExtensions.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Extensions/RelationalEntityTypeExtensions.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Looked through the Migrations changes)
@@ -914,21 +907,23 @@ private static bool EntityTypePathEquals(IEntityType source, IEntityType target, | |||
return true; | |||
} | |||
|
|||
if (!string.Equals(source.Name, target.Name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this break renaming entity types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is only used for column diffing, so as long as the type is not renamed together with a column everything should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But renaming the type and a column is a scenario we should cover since the PK column (and others) frequently includes the type name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would also need to rename a different column to what the PK column was before to hit this
src/EFCore.Relational/Extensions/RelationalEntityTypeExtensions.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Extensions/RelationalForeignKeyExtensions.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Extensions/RelationalForeignKeyExtensions.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Metadata/Conventions/EntityTypeMappingConvention.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Metadata/Conventions/SharedTableConvention.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
75/95
src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Metadata/Conventions/RelationalValueGenerationConvention.cs
Show resolved
Hide resolved
test/EFCore.Relational.Specification.Tests/Query/TPTRelationshipsQueryTestBase.cs
Show resolved
Hide resolved
src/EFCore.Relational/Extensions/RelationalEntityTypeExtensions.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Specification.Tests/Query/NorthwindFunctionsQueryTestBase.cs
Outdated
Show resolved
Hide resolved
It would have been better if langword changes happened in a different PR. |
I feel like somebody attached a "langword" rider to the TPT bill. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve for the extensive change tracking changes.
src/EFCore.Relational/Metadata/Conventions/EntityTypeHierarchyMappingConvention.cs
Show resolved
Hide resolved
src/Microsoft.Data.Sqlite.Core/SqliteConnectionStringBuilder.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Specification.Tests/Query/NorthwindFunctionsQueryTestBase.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Added a few minor comments.
@bricelam Can you take a look at the last commit? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Part of #2266