-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Add runtime migration creation and application #37415
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
base: main
Are you sure you want to change the base?
Conversation
a15611a to
9a35a9b
Compare
62018f1 to
5c41f2f
Compare
Note on SQL Server Integration TestsThe Why these tests are skipped in CI:
Test coverage is still maintained:
This is consistent with how other complex migration infrastructure tests handle CI limitations. |
test/EFCore.SqlServer.FunctionalTests/Migrations/RuntimeMigrationSqlServerTest.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Design/DesignTimeServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Migrations/Design/IDynamicMigrationsAssembly.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Migrations/Design/IDynamicMigrationsAssembly.cs
Outdated
Show resolved
Hide resolved
|
Thank you for the thorough review @AndriySvyryd! I've addressed all your feedback:
All tests pass locally (SQLite: 26 tests, SQL Server: 7 tests, CSharpMigrationCompiler: 4 tests, MigrationsOperations: 2 tests). |
src/EFCore.Relational/Extensions/RelationalDatabaseFacadeExtensions.cs
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
test/EFCore.Design.Tests/Design/Internal/MigrationsOperationsTest.cs
Outdated
Show resolved
Hide resolved
This comment was marked as off-topic.
This comment was marked as off-topic.
- Move migration name validation before context creation in AddMigration and AddAndApplyMigration to ensure proper error messages when name is empty - Use Single() instead of First() in Migration_preserves_existing_data test to avoid FirstWithoutOrderByAndFilterWarning
Replace First() with Single() to avoid FirstWithoutOrderByAndFilterWarning
Close connection before migrator.Migrate("0") call and reopen after,
since the migrator manages its own connection state internally.
I agree that it's very unlikely, keep NonCapturingLazyInitializer and remove the lock from AddMigrations |
test/EFCore.Relational.Specification.Tests/RuntimeMigrationTestBase.cs
Outdated
Show resolved
Hide resolved
| public void Can_scaffold_migration() | ||
| { | ||
| using var context = CreateContext(); | ||
| CleanDatabase(context); |
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 should be able to call Fixture.ReseedAsync() instead.
You can also call it from IAsyncLifetime.InitializeAsync to avoid repeating the call in each method
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.
Sorry, I forgot that ReseedAsync also creates tables. Change InitializeAsync to
{
using var context = CreateContext();
return Fixture.TestStore.CleanAsync(context, createTables: false);
}Then add the bool createTables = true parameter to TestStore.CleanAsync, SqlServerDatabaseFacadeExtensions.EnsureClean, SqliteDatabaseFacadeTestExtensions.EnsureClean and RelationalDatabaseCleaner.Clean
| => "RuntimeMigration"; | ||
|
|
||
| protected override bool UsePooling | ||
| => false; |
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.
Why did you have to specify false here?
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.
Re: UsePooling => false in RuntimeMigrationFixtureBase
Connection pooling is disabled because these tests dynamically alter the database schema (creating/dropping tables, applying/reverting migrations). With pooling enabled, pooled connections might hold stale schema information or cached state that conflicts with the schema changes made during migration operations.
However, if you think pooling should work fine for these tests, I can remove the override and test it.
This property controls DbContext pooling, not connection pooling, so leaving the default value (true) should be fine for these tests
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.
I see now, if you use context pooling then the tests will get a context instance that still contains the runtime migrations assembly from the previous test, making it fail in most cases. So in this case, disabling DbContext pooling is the right choice
Per review: race condition is very unlikely since AddMigrations is only called during design-time operations, not concurrently with Migrations access.
|
Re: Connection pooling is disabled because these tests dynamically alter the database schema (creating/dropping tables, applying/reverting migrations). With pooling enabled, pooled connections might hold stale schema information or cached state that conflicts with the schema changes made during migration operations. However, if you think pooling should work fine for these tests, I can remove the override and test it. |
- Implement IAsyncLifetime and call Fixture.ReseedAsync() in InitializeAsync instead of manually calling CleanDatabase in each test - Use context.Database.OpenConnection/CloseConnection instead of direct connection.Open/Close calls - Move database cleanup logic to fixture's CleanAsync override - Add GetTableNamesAsync to fixtures for async cleanup
This comment was marked as resolved.
This comment was marked as resolved.
- Simplify CSharpMigrationCompiler.GetMetadataReferences to use cached references plus context assembly, removing explicit Assembly.Load calls - Remove duplicate name validation from AddMigration/AddAndApplyMigration since PrepareForMigration already validates - Remove UsePooling override (controls DbContext pooling, not connection pooling)
|
Addressed the remaining unresolved comments:
Commit: e7b6329 |
The validation must happen BEFORE CreateContext() because: 1. Tests use mock contexts that can't be created without a database 2. PrepareForMigration runs AFTER context creation 3. Without early validation, context creation fails with a confusing error instead of the expected "A migration name must be specified" message This validation is not truly duplicated - it's needed at this specific point in the call sequence to ensure proper error messages.
The simplified version wasn't loading the context assembly's referenced assemblies explicitly. Static references may not be loaded into memory when the cache is built, causing compilation failures when those assemblies are needed.
The IAsyncLifetime + ReseedAsync approach was causing CI failures. Reverting to the original CleanDatabase method approach that was working in commit cd324fd. This will need to be addressed differently based on reviewer feedback.
- Implement IAsyncLifetime on test class, call Fixture.ReseedAsync() - Move GetTableNames and CleanDatabase to fixture classes - Use context.Database.OpenConnection/CloseConnection instead of direct connection calls - Remove per-test CleanDatabase calls (handled by ReseedAsync)
The SharedStoreFixtureBase.ReseedAsync() calls CleanAsync which calls Clean. Override Clean directly to properly integrate with the base class cleanup flow.
| if (string.IsNullOrWhiteSpace(name)) | ||
| { | ||
| throw new OperationException(DesignStrings.MigrationNameRequired); | ||
| } |
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.
Ok, then remove this check since it's already checked before this call
ReseedAsync pattern is for restoring seeded data after tests modify it. Our tests need a clean database with no tables before each test. Call CleanDatabase directly which drops all tables.
The IAsyncLifetime + ReseedAsync pattern doesn't fit the runtime migration test use case. Runtime migrations need a completely empty database (no tables) before each test, while ReseedAsync is designed for restoring seeded data after tests modify it. Changes: - Remove IAsyncLifetime from test base class - Move CleanDatabase to virtual method in test class, called per-test - Keep GetTableNames in fixture as protected internal abstract - Use context.Database.OpenConnection()/CloseConnection() pattern
When overriding a protected internal member from a different assembly, only protected is visible, so the override must use protected (not protected internal).
EF Core now throws an error for First() without OrderBy. Since we know there's exactly one entity, use Single() instead which is also more semantically correct.
The EnsureCreated() call was creating tables that were immediately dropped, which could cause timing issues in CI. The database is already created by the fixture initialization, so EnsureCreated() is unnecessary.
Migration tests need to start with an empty database (no tables) so they can test creating and applying migrations from scratch. The default test stores call EnsureCreatedResilientlyAsync() which creates all tables, causing "table already exists" errors when tests try to run migrations. This fix creates custom test store factories that: - SQLite: Uses SqliteTestStore.GetExisting() which sets seed=false - SQL Server: Creates a custom SqlServerTestStore subclass that overrides InitializeAsync to skip table creation Each test still calls CleanDatabase() to drop any existing tables before running, ensuring a clean state for each test.
Summary
Implements #37342: Allow creating and applying migrations at runtime without recompiling.
This adds support for creating and applying migrations at runtime using Roslyn compilation, enabling scenarios like .NET Aspire and containerized applications where recompilation isn't possible.
CLI Usage
The
-o/--output-dir,-n/--namespace, and--jsonoptions require--addto be specified.PowerShell Usage
Architecture
IMigrationCompiler/CSharpMigrationCompilerIMigrationsAssembly.AddMigrations(Assembly)MigrationsOperations.AddAndApplyMigration()Design Decisions
IMigrationsScaffolderfor scaffolding andIMigratorfor applying, adding only the newIMigrationCompilerserviceIMigrationsAssemblyinterface to accept additional assemblies containing runtime-compiled migrationsAddMigration, files are always saved to enable source control and future recompilationIMigrationCompilerandCSharpMigrationCompilerare in the.Internalnamespace as they require design work for public APIMigrationsAssemblyuses locking to protect against race conditions when adding migrations concurrentlyWorkflow
Robustness Features
AddAndApplyMigrationwraps the save-compile-register-apply chain in try-catch, deleting saved files on failure to prevent orphansPrepareForMigrationensures the DbContext is disposed if validation or service building failsMigrationsAssemblyuses locking to protect shared state (migrations dictionary, model snapshot, additional assemblies list)Limitations
[RequiresDynamicCode]Test plan
CSharpMigrationCompilerMigrationsOperations.AddAndApplyMigrationRuntimeMigrationTestBase(SQLite and SQL Server implementations)Fixes #37342