Conversation
haileymck
force-pushed
the
dev/hmckelvie/fix-2941391
branch
from
43db3ef to
5a35564
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new scaffolding step to ensure a missing DbSet<T> is inserted into a pre-existing ApplicationDbContext (or other selected context) during CRUD scaffolding, addressing a build-breaking scenario when the context already exists.
Changes:
- Introduces
AddDbSetToExistingContextStepthat delegates insertion toCodeModificationStepvia an in-memory JSON config. - Updates
ClassAnalyzers.GetDbContextInfoto populate aNewDbSetStatementwhen an existing context is missing the model’sDbSet. - Adds/updates unit tests covering the new
NewDbSetStatementbehavior and the new step’s configuration/no-op paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet-scaffolding/dotnet-scaffold.Tests/AspNet/ScaffoldSteps/AddDbSetToExistingContextStepTests.cs | New unit tests for the new scaffold step’s no-op behavior and JSON config generation. |
| test/dotnet-scaffolding/dotnet-scaffold.Tests/AspNet/Common/ClassAnalyzersTests.cs | Adds tests for NewDbSetStatement generation; updates Roslyn compilation references to support EF Core types. |
| src/dotnet-scaffolding/dotnet-scaffold/AspNet/ScaffoldSteps/AddDbSetToExistingContextStep.cs | New scaffold step that applies a DbSet insertion to an existing DbContext via CodeModificationStep. |
| src/dotnet-scaffolding/dotnet-scaffold/AspNet/Extensions/ScaffolderBuilderAspNetExtensions.cs | Adds WithAddDbSetStep() builder extension to wire the new step into pipelines. |
| src/dotnet-scaffolding/dotnet-scaffold/AspNet/Common/ClassAnalyzers.cs | Populates NewDbSetStatement when an existing DbContext is missing the relevant DbSet. |
| src/dotnet-scaffolding/dotnet-scaffold/AspNet/AspNetCommandService.cs | Registers the new step type and inserts it into multiple AspNet scaffolder command pipelines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Only act when: | ||
| // The target file already exists on disk (existing context — new contexts are | ||
| // handled by WithDbContextStep via its T4 template which already includes the DbSet). | ||
| // A DbSet statement was produced (only set when GetDbContextInfo found no existing DbSet). | ||
| // The project path is available to initialise the Roslyn workspace. | ||
| if (string.IsNullOrEmpty(dbContextPath) || !File.Exists(dbContextPath) || string.IsNullOrEmpty(dbSetStatement) || string.IsNullOrEmpty(ProjectPath)) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The guard File.Exists(dbContextPath) does not actually distinguish “existing context” from “new context created by WithDbContextStep”, because the new context file will exist by the time this step runs (it’s executed after WithDbContextStep). This means the step can run in new-context scenarios, doing an extra Roslyn workspace modification pass and logging “Adding DbSet…” even when the property is already present. Consider adding a fast pre-check (e.g., parse the file or use Roslyn to detect an existing matching DbSet property) and return true without invoking CodeModificationStep when it’s already there, and/or store a flag in context during DbContext creation to reliably skip for newly created contexts.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Added a DbSetAlreadyPresent pre-check in commit c0a5946. After the file-existence guard, ExecuteAsync now reads the file content and returns true immediately (without invoking CodeModificationStep) when a DbSet<TypeName> for the same model type is already present. This reliably skips the redundant Roslyn workspace pass when WithDbContextStep has just created a new context file that already includes the DbSet property. Five new unit tests (scenarios 11–15) cover the new behavior.
…olderBuilderAspNetExtensions.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…tep for new contexts Agent-Logs-Url: https://github.com/dotnet/Scaffolding/sessions/dfdc66ac-1e84-476a-9c5e-c918f79aae5c Co-authored-by: haileymck <111816896+haileymck@users.noreply.github.com>
Fixes Azdo Bug
This error is occuring when there is a pre-existing
ApplicationDbContexttheDbSet<[Model]>is not added to theApplicationDbContextduring scaffolding (it is added when there is a newApplicationDbContextcreated). This change ensures that theApplicationDbContextis edited during the scaffolding process to add theDbSetand allows for the project to build after scaffolding in this scenario. To ensure this code change, I create a new step, called theAddDbSetToExistingContextStepthat does this edit if needed. Furthermore, more tests are added to handle this scenario.