fix: handle wildcard '*' in --context parameter for UpdateDatabase an…#38269
fix: handle wildcard '*' in --context parameter for UpdateDatabase an…#38269BrunoSync wants to merge 7 commits into
Conversation
…d MigrationsBundle
There was a problem hiding this comment.
Pull request overview
This PR adds wildcard --context * handling to migration database updates so EF Core tooling can attempt to update all discovered DbContext types.
Changes:
- Adds a wildcard branch in
MigrationsOperations.UpdateDatabase. - Adds a test intended to cover wildcard update behavior across multiple contexts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/EFCore.Design/Design/Internal/MigrationsOperations.cs |
Adds CreateAllContexts() iteration for contextType == "*". |
test/EFCore.Design.Tests/Design/Internal/MigrationsOperationsTest.cs |
Adds wildcard update test coverage. |
- Implement wildcard support in UpdateDatabase method - Throw OperationException when no contexts found (consistency with non-wildcard path) - MigrationsBundle covered by this fix (calls UpdateDatabase internally) - Remove weak test coverage (no existing tests for UpdateDatabase in codebase)
Implementation Notes for ReviewHi @dotnet/efcore-team, I've addressed the wildcard Changes Made
Design Decision: Why AddMigration is Excluded
Testing ApproachI noticed
Please let me know if you'd like me to approach testing differently or if there's additional context needed. Thanks for reviewing! |
That would be a big change, so it should be done separately - #38278 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- src/EFCore.Design/Properties/DesignStrings.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
src/EFCore.Design/Design/Internal/MigrationsOperations.cs:244
CreateAllContexts()is an iterator that creates aDbContextas it is enumerated, so callingAny()here constructs the first context and then abandons it undisposed before theforeachcreates a second instance. Use a single enumeration pattern (for example, thecontextOptimizedflag used byDbContextOperations.Optimize) to avoid leaking the first context and duplicating its side effects.
var contexts = _contextOperations.CreateAllContexts();
if (!contexts.Any())
{
throw new OperationException(DesignStrings.NoContext(_assembly.GetName().Name));
}
foreach (var item in contexts)
src/EFCore.Design/Properties/DesignStrings.resx:517
- The
<value>element is indented differently from the rest of this.resxfile (nearby resource values use four spaces under<data>). Please align it with the existing XML formatting.
<data name="WildcardNotSupported" xml:space="preserve">
<value>The wildcard '*' is not supported for this command.</value>
</data>
| if (contextType == "*") | ||
| { | ||
| throw new OperationException(DesignStrings.WildcardNotSupported); |
| string? targetMigration, | ||
| string? connectionString, | ||
| string? contextType) |
|
Thanks for the detailed review! Addressing all three points: GetContextInfo wildcard: Good catch. Since MigrationsBundleCommand calls GetContextInfo internally to get the context type for Program.cs generation, throwing there would break the bundle scenario. My proposal is to remove the wildcard check from GetContextInfo and move it directly into MigrationsBundleCommand.Execute instead, keeping each command responsible for its own validation. Does that sound right? Tests: I tried adding tests for the wildcard path in UpdateDatabase but ran into difficulties since it requires real IMigration implementations and a database connection. How does the project usually handle testing this kind of integration flow? Happy to follow whatever approach is standard here. |
Add it to
The tools are currently just tested manually |
Fix wildcard '*' support in --context parameter
Fixes #38254
What was done
UpdateDatabase— when--context *is passed, the operation now callsCreateAllContexts()and iterates over all found contextsMigrationsBundleis covered by this fix since it internally callsUpdateDatabaseUpdateDatabase_with_wildcard_context_runs_for_all_contextsto verify the fixPending
AddMigrationalso needs wildcard support, but since it returnsMigrationFiles, iterating over multiple contexts raises a design question about the return value. Looking for guidance on how to handle this before implementing.