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.

Sign up for GitHub

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

Jump to bottom

Refactor MigrationsModelDiffer to use the database model. #20305

Merged
merged 1 commit into from
Mar 24, 2020
Merged

Refactor MigrationsModelDiffer to use the database model. #20305

merged 1 commit into from
Mar 24, 2020

Conversation

AndriySvyryd
Copy link
Member

Change IMigrationsAnnotationProvider to use the database model and rename to IRelationalAnnotationProvider
Add IRelationalModel
Add more shared objects validation

Part of #2266

@AndriySvyryd AndriySvyryd force-pushed the RefactorDiffer branch 3 times, most recently from d40c737 to 9eef47b Compare March 18, 2020 17:42
@AndriySvyryd AndriySvyryd mentioned this pull request Mar 19, 2020
Closed
@AndriySvyryd AndriySvyryd requested a review from a team March 20, 2020 00:00
@AndriySvyryd AndriySvyryd linked an issue Mar 20, 2020 that may be closed by this pull request
Revisit model snapshots and the finalized model #17205
Closed
bricelam
bricelam approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

😍 It's so beautiful!

src/EFCore.Relational/Metadata/ITable.cs Outdated Show resolved Hide resolved
src/EFCore.Relational/Metadata/RelationalAnnotationProvider.cs Show resolved Hide resolved
src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs Show resolved Hide resolved
src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs Outdated Show resolved Hide resolved
src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs
?? typeMapping.ClrType).UnwrapNullableType();

columnOperation.ColumnType = property.GetConfiguredColumnType();
Copy link
Contributor

Choose a reason for hiding this comment

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

There was some subtlety around GetConfiguredColumnType that I can't remember. But I probably wrote a test to cover it, so as long as no test fails... 😬

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 think that was from when we tried to avoid putting the store type in the snapshot

src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs Show resolved Hide resolved
test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs Show resolved Hide resolved
test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs Show resolved Hide resolved
src/EFCore.Relational/Metadata/IRelationalAnnotationProvider.cs Show resolved Hide resolved
src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs Show resolved Hide resolved
@AndriySvyryd AndriySvyryd force-pushed the RefactorDiffer branch 2 times, most recently from 7a51371 to 20b4b8a Compare March 24, 2020 00:25
@AndriySvyryd
Refactor MigrationsModelDiffer to use the database model.
3eec55e 
Change IMigrationsAnnotationProvider to use the database model and split a part to IRelationalAnnotationProvider
Add IRelationalModel
Add more shared objects validation

Part of #2266
@AndriySvyryd AndriySvyryd merged commit 3eec55e into master Mar 24, 2020
@AndriySvyryd AndriySvyryd deleted the RefactorDiffer branch March 24, 2020 01:34
@ajcvickers
Copy link
Member

@AndriySvyryd This is the PR that broke the error page, right? Do we need to document some of the breaking changes here?

@AndriySvyryd
Copy link
Member Author

@ajcvickers The offending changes were in the signature of IMigrationsModelDiffer.Has(Get)Differences. Personally I don't think many users would be affected.

cc @bricelam

@ajcvickers
Copy link
Member

@AndriySvyryd Not 100% convinced. It's not uncommon to get questions on model diffing.
Would like to know what @bricelam thinks.

@bricelam
Copy link
Contributor

I've shown a lot of people how to call those over the years. We should at least document

@AndriySvyryd AndriySvyryd mentioned this pull request Mar 26, 2020
Closed
@ajcvickers ajcvickers mentioned this pull request Mar 27, 2020
Closed
roji added a commit to roji/efcore.pg that referenced this pull request Mar 28, 2020
@roji
Changes for new relational model
c32ef76 
Following dotnet/efcore#20305
roji added a commit to roji/efcore.pg that referenced this pull request Mar 28, 2020
@roji
Changes for new relational model
3af904e 
Following dotnet/efcore#20305
roji added a commit to roji/efcore.pg that referenced this pull request Mar 29, 2020
@roji
Changes for new relational model
c13fe99 
Following dotnet/efcore#20305
roji added a commit to npgsql/efcore.pg that referenced this pull request Mar 29, 2020
@roji
Changes for new relational model
4c9fea7 
Following dotnet/efcore#20305
@bricelam bricelam mentioned this pull request Aug 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bricelam bricelam bricelam approved these changes

Assignees
No one assigned
Labels
providers-beware
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revisit model snapshots and the finalized model
3 participants
@AndriySvyryd @ajcvickers @bricelam