Re-platform SampleMvcWebApp to ASP.NET Core MVC on .NET 10 (EF Core + EfCore.GenericServices) - #27
Conversation
…has (feature: Phase 0 research) Co-Authored-By: Parker Duff <pwjduff@gmail.com>
Convert the three class libraries to SDK-style net10.0 projects. DataLayer: EF Core 10 DbContext (options-only ctor); SaveChanges/SaveChangesAsync port of the change-tracking logic (fixes the early-return bug -> continue); unique Tag.Slug index + friendly pre-save duplicate check; Blog/Post + Post/Tag relationships. Remove EfConfiguration and EF6 initializers; DataLayerInitialise keeps ResetBlogs using ILogger. Seed XML kept as embedded resources. ServiceLayer: replace GenericServices 1.0.9 with EfCore.GenericServices. DTOs are now POCOs implementing ILinkToEntity<T>; DetailPost*Dto use PerDtoConfig to ignore UI-only members. New PostCrudHelper (IPostCrudHelper) does dropdown/multiselect setup and Post create/update, replacing the removed SetupSecondaryData/CreateDataFromDto hooks. DI: replace Autofac modules with AddDataLayer/AddBizLayer/AddServiceLayer IServiceCollection extensions; AddServiceLayer wires GenericServicesSimpleSetup. Scope: does not touch SampleWebApp or Tests. Co-Authored-By: Parker Duff <pwjduff@gmail.com>
- Convert SampleWebApp.csproj to SDK-style Microsoft.NET.Sdk.Web (net10.0) - Replace Global.asax/App_Start/Autofac/OWIN/log4net with Program.cs (minimal hosting) - Move config to appsettings.json/appsettings.Development.json - Port all controllers to ICrudServices/ICrudServicesAsync/IPostCrudHelper with [FromServices] - Move static assets to wwwroot; drop SignalR/ActionRunner/Modernizr/Respond - Add _ViewImports, fix _Layout/views bundles and MVC5-only helpers Co-Authored-By: Parker Duff <pwjduff@gmail.com>
…factory Co-Authored-By: Parker Duff <pwjduff@gmail.com>
Co-Authored-By: Parker Duff <pwjduff@gmail.com>
…-in DI Convert the Tests project to an SDK-style net10.0 project and update every test to the migrated EF Core / EfCore.GenericServices / Microsoft.Extensions.DependencyInjection APIs. - SDK-style csproj with NUnit 4, NUnit3TestAdapter, Microsoft.NET.Test.Sdk, Moq, Microsoft.EntityFrameworkCore.Sqlite, EfCore.GenericServices; drops SampleWebApp ref. - New TestDbContext helper builds a SampleWebAppDb over an open in-memory SQLite connection. - Group01/03/06 tests ported to EF Core, MS DI, and AspNetCore ModelStateDictionary. - bug: DataLayer set GenerateAssemblyInfo=false, which silently dropped the <InternalsVisibleTo Include="Tests"> item; replaced with an explicit AssemblyInfo.cs so internal types (LoadDbDataFromXml) are visible to the Tests assembly again. Co-Authored-By: Parker Duff <pwjduff@gmail.com>
Co-Authored-By: Parker Duff <pwjduff@gmail.com>
…OTES Co-Authored-By: Parker Duff <pwjduff@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Null-safe DetailPostDto/DetailPostDtoAsync TagNames so ASP.NET Core model validation no longer throws ArgumentNullException on Create/Edit POST (Tags is not model-bound and was null). Wire a GenericServices BeforeSaveChanges hook that reports duplicate Tag.Slug as a status/validation error instead of letting the data layer ValidationException escape as an unhandled HTTP 500. Add regression tests for both. Co-Authored-By: Parker Duff <pwjduff@gmail.com>
…ion notes Co-Authored-By: Parker Duff <pwjduff@gmail.com>
Runtime verification (Phase 2) — passed ✅Ran the migrated app in the VM ( Two runtime defects found on the first pass were fixed on this branch and re-verified:
Results
EvidenceDuplicate slug handled gracefully (no 500): Post created via EfCore.GenericServices: Post details — blogger + both tags persisted: Post edited — title + tag set updated: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| public IActionResult Edit(int id, [FromServices] ICrudServices service, [FromServices] IPostCrudHelper crudHelper) | ||
| { | ||
| return View(service.GetOriginal<DetailPostDto>(id).Result); | ||
| var dto = service.ReadSingle<DetailPostDto>(id); | ||
| crudHelper.SetupSecondaryData(dto); | ||
| return View(dto); | ||
| } |
There was a problem hiding this comment.
🟡 Editing a post with an unknown or deleted id crashes with an unhandled error
When a post is opened for editing with an id that no longer exists, the post is looked up and comes back as nothing, then the code immediately tries to fill its dropdown/tag lists (crudHelper.SetupSecondaryData(dto) at SampleWebApp/Controllers/PostsController.cs:67) on that missing item, throwing an unhandled error.
Impact: Requesting the Post edit page (sync or async) for a stale/invalid id returns a server error page instead of a graceful "not found" response.
Null-reference from ReadSingle returning null before SetupSecondaryData
ICrudServices.ReadSingle<DetailPostDto>(id) returns null (and records an error on the service) when no matching row exists. The controller does not check this before calling crudHelper.SetupSecondaryData(dto). In ServiceLayer/PostServices/PostCrudHelper.cs:83-88, SetupSecondaryData dereferences dto.Bloggers / dto.UserChosenTags, causing a NullReferenceException. The same pattern exists in the async controller (SampleWebApp/Controllers/PostsAsyncController.cs:56-60, SetupSecondaryDataAsync). The previous EF6 implementation returned View(null) without touching the DTO in the controller.
Prompt for agents
In PostsController.Edit(int id, ...) and PostsAsyncController.Edit(int id, ...), ICrudServices.ReadSingle/ReadSingleAsync can return null when the post id does not exist, and the subsequent call to crudHelper.SetupSecondaryData(dto)/SetupSecondaryDataAsync(dto) dereferences the null dto (dto.Bloggers etc.) causing a NullReferenceException / HTTP 500. Add a null check on the returned dto and return NotFound() (or redirect to Index with an error message) before calling SetupSecondaryData. Consider doing the same anywhere else a ReadSingle result is passed into PostCrudHelper.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed and fixed in b4a8fd9 — this was a real regression.
ICrudServices.ReadSingle<DetailPostDto>(id) returns null for a missing key, and PostCrudHelper.SetupSecondaryData immediately dereferences dto.Bloggers / dto.UserChosenTags, so a stale id gave an unhandled NullReferenceException instead of the old View(null). Both the sync and async Edit GETs now guard:
var dto = service.ReadSingle<DetailPostDto>(id);
if (dto == null)
return NotFound();
crudHelper.SetupSecondaryData(dto);Added Tests/UnitTests/Group06Mvc/Test03PostsControllerEdit.cs (3 tests: unknown id → NotFoundResult with SetupSecondaryData never called, known id → ViewResult with secondary data set up, and the async equivalent). This needed a Tests → SampleWebApp project reference, which is new in this commit.
I verified the test genuinely covers it: with the guard removed the new test fails, with it in place it passes.
I checked the other ReadSingle call sites (Posts/PostsAsync Details, Tags/TagsAsync Details+Edit, Blogs Edit) — those pass the result straight to View(...) without dereferencing it, which matches the pre-migration behaviour, so I left them alone rather than broadening the change.
| @@ -64,13 +63,5 @@ public class SimplePostDto : EfGenericDto<Post, SimplePostDto> | |||
| public DateTime LastUpdatedUtc { get { return DateTime.SpecifyKind(LastUpdated, DateTimeKind.Utc); } } | |||
|
|
|||
| public string TagNames { get { return string.Join(", ", Tags.Select(x => x.Name)); } } | |||
There was a problem hiding this comment.
🔍 SimplePostDto/SimplePostDtoAsync TagNames left without null guard unlike DetailPostDto
The migration explicitly null-guarded DetailPostDto.TagNames/DetailPostDtoAsync.TagNames (because those DTOs are model-bound on POST, where Tags is null and every getter is visited during validation, per MIGRATION_NOTES §Phase2). ServiceLayer/PostServices/SimplePostDto.cs:65 and SimplePostDtoAsync keep string.Join(", ", Tags.Select(x => x.Name)) with no null guard. This is safe only as long as these list DTOs are never model-bound (they are read-only, populated by ReadManyNoTracked<> projection which materializes Tags) and the AutoMapper/GenericServices projection actually populates the Tags collection. If a future controller ever posts a SimplePostDto, or if the projection does not fill Tags, the getter will throw an NRE the same way DetailPostDto did. Worth confirming the projection fills Tags and keeping the guard consistent.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Your reasoning is right on both counts, and I've made it consistent in b4a8fd9 — SimplePostDto and SimplePostDtoAsync now use the same Tags == null ? string.Empty : ... guard as the Detail DTOs.
To your "worth confirming the projection fills Tags" point: I added Test17CrudServicesReadProjectsTagsCollection to Test11DiExtensions, which goes through the real DI graph and asserts Tags is non-null on ReadManyNoTracked<SimplePostDto>() and non-null/non-empty on ReadSingle<DetailPostDto>(), with TagNames non-empty. So the projection does populate it today; the guard is defence-in-depth in case one of these is ever model-bound.
| public class DetailPostDtoConfig : PerDtoConfig<DetailPostDto, Post> | ||
| { | ||
| public override Action<IMappingExpression<Post, DetailPostDto>> AlterReadMapping | ||
| { | ||
|
|
||
| var blogId = Bloggers.SelectedValueAsInt; | ||
| if (blogId == null) | ||
| return "The blogger was not selected. You must do that before the post can be saved."; | ||
|
|
||
| var blogger = db.Blogs.Find((int)blogId); | ||
| if (blogger == null) | ||
| return "Could not find the blogger you selected. Did another user delete it?"; | ||
|
|
||
| BlogId = (int)blogId; //will be copied over to database entity by AutoMapper | ||
| return null; | ||
| get | ||
| { | ||
| return cfg => cfg | ||
| .ForMember(d => d.Bloggers, o => o.Ignore()) | ||
| .ForMember(d => d.UserChosenTags, o => o.Ignore()); | ||
| } | ||
| } | ||
|
|
||
| private string ChangeTagsBasedOnMultiSelectList(SampleWebAppDb db, Post post = null) | ||
| public override Action<IMappingExpression<DetailPostDto, Post>> AlterSaveMapping | ||
| { | ||
| var requiredTagIds = UserChosenTags.GetFinalSelectionAsInts(); | ||
| if (!requiredTagIds.Any()) | ||
| return "You must select at least one tag for the post."; | ||
|
|
||
| if (requiredTagIds.Any(x => db.Tags.Find(x) == null)) | ||
| return "Could not find one of the tags. Did another user delete it?"; | ||
|
|
||
| if (post != null) | ||
| //This is an update so we need to load the tags | ||
| db.Entry(post).Collection(p => p.Tags).Load(); | ||
|
|
||
| var newTagsForPost = db.Tags.Where(x => requiredTagIds.Contains(x.TagId)).ToList(); | ||
| Tags = newTagsForPost; //will be copied over to database entity by AutoMapper | ||
|
|
||
| return null; | ||
| get | ||
| { | ||
| return cfg => cfg | ||
| .ForMember(d => d.Tags, o => o.Ignore()) | ||
| .ForMember(d => d.Blogger, o => o.Ignore()) | ||
| .ForMember(d => d.LastUpdated, o => o.Ignore()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 ProjectTo of entity collection (Tags) into read DTOs depends on an implicit Tag->Tag map
DetailPostDto/SimplePostDto expose ICollection<Tag> Tags and the read mappings only ignore Bloggers/UserChosenTags, so GenericServices' AutoMapper ProjectTo must project Post.Tags (entity) into dto.Tags (same entity type). AutoMapper generally requires an explicit map for Tag->Tag to project a collection of full entities; if GenericServices does not auto-register identity maps for entities referenced by DTOs, ReadSingle/ReadManyNoTracked could throw a mapping-configuration error at runtime. The PR states CRUD flows were verified end-to-end, so this presumably works, but it relies on GenericServices behavior that isn't obvious from the diff and is worth a targeted check (list + details pages that render TagNames).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Verified — it does work, and I've pinned it down with a test rather than leaving it resting on the runtime pass.
Test17CrudServicesReadProjectsTagsCollection (in Test11DiExtensions) builds the same DI graph as Program.cs, seeds via ResetBlogs, and asserts that the projection populates the collection:
var simple = service.ReadManyNoTracked<SimplePostDto>().Single(x => x.PostId == postId);
var detail = service.ReadSingle<DetailPostDto>(postId);
ClassicAssert.NotNull(simple.Tags);
ClassicAssert.NotNull(detail.Tags);
ClassicAssert.IsTrue(detail.Tags.Any());
ClassicAssert.IsFalse(string.IsNullOrEmpty(detail.TagNames));So GenericServices' AutoMapper config does handle projecting Post.Tags into the DTO's ICollection<Tag> without an explicitly declared Tag→Tag map. Agreed it isn't obvious from the diff — the test now documents the dependency and will fail loudly if a package upgrade changes that behaviour.
I also added the null guard to SimplePostDto/SimplePostDtoAsync per your related comment, so TagNames is safe regardless.
| { | ||
| var firstError = GetSlugUniquenessErrors().FirstOrDefault(); | ||
| if (firstError != null) | ||
| throw new ValidationException(firstError); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns a user-friendly error message for every added/modified Tag whose Slug collides with | ||
| /// another Tag's Slug. Empty when all Slugs are unique. Used both by the throwing | ||
| /// <see cref="CheckForUniqueSlugs"/> and by GenericServices' BeforeSaveChanges hook so a duplicate | ||
| /// Slug is reported as a validation error rather than an unhandled exception. | ||
| /// </summary> | ||
| public IReadOnlyList<string> GetSlugUniquenessErrors() | ||
| { | ||
| var errors = new List<string>(); | ||
| var changedTags = ChangeTracker.Entries<Tag>() | ||
| .Where(e => e.State == EntityState.Added || e.State == EntityState.Modified) | ||
| .Select(e => e.Entity) | ||
| .ToList(); | ||
|
|
||
| foreach (var tagToCheck in changedTags) | ||
| { | ||
| if (Tags.Any(x => x.TagId != tagToCheck.TagId && x.Slug == tagToCheck.Slug)) | ||
| errors.Add(string.Format("The Slug on tag '{0}' must be unique and is already being used.", tagToCheck.Name)); | ||
| } | ||
|
|
||
| return errors; | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Duplicate-slug protection is enforced in two places with different failure modes
SampleWebAppDb.CheckForUniqueSlugs throws ValidationException on direct SaveChanges, while the GenericServices path relies on the BeforeSaveChanges hook in ServiceLayerServiceCollectionExtensions returning an invalid IStatusGeneric so SaveChanges is never reached. GetSlugUniquenessErrors compares x.TagId != tagToCheck.TagId; for multiple newly-added Tags in the same save all have TagId==0, so duplicate slugs among not-yet-persisted rows in a single SaveChanges are NOT detected by this check and would instead surface as the DB unique-index exception. This matches the old EF6 per-entity behavior and the seed data has no duplicates, so it is not a regression, but the two-layer scheme (hook + throw + DB index) means a duplicate introduced outside the GenericServices path still throws rather than returning a friendly status.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Accurate read of the design, and the TagId == 0 observation is correct. Leaving the behaviour as-is deliberately; recording the reasoning:
- Two failure modes is intentional. The
BeforeSaveChangeshook is the graceful path for everything that goes through GenericServices (i.e. all the controllers).CheckForUniqueSlugsthrowing on a directSaveChangesis a backstop for tests and any future non-GenericServices caller — it preserves the EF6ValidateEntitycontract, where a direct save also threw. Making it return a status instead would silently drop the error for callers that don't inspect one. - Multiple new Tags in one save is not reachable from the UI — every Tag create/edit action saves exactly one Tag — and as you note the DB unique index (
IX_Tags_Slug) still catches it. That matches the old per-entity EF6 behaviour, so it isn't a regression.
Happy to unify these (e.g. group unsaved tags by slug in GetSlugUniquenessErrors, and have the DbContext surface a status rather than throw) if you'd prefer the stricter contract — just say the word, as it's a behaviour change beyond the scope of the re-platform.
| private IStatusGeneric BuildAndSave(int postId, string title, string content, | ||
| DropDownListType bloggers, MultiSelectListType userChosenTags, bool isUpdate) | ||
| { | ||
| var status = new StatusGenericHandler(); | ||
|
|
||
| var post = ResolvePost(postId, isUpdate, status); | ||
| if (status.HasErrors) | ||
| return status; | ||
|
|
||
| var blogId = ResolveBlogId(bloggers, status); | ||
| var tags = ResolveTags(userChosenTags, status); | ||
| if (status.HasErrors) | ||
| return status; | ||
|
|
||
| ApplyToPost(post, title, content, blogId.Value, tags, isUpdate); | ||
|
|
||
| status.CombineStatuses(ValidateEntity(post)); | ||
| if (status.HasErrors) | ||
| return status; | ||
|
|
||
| _db.SaveChanges(); | ||
| status.Message = isUpdate ? "Successfully updated the post." : "Successfully created the post."; | ||
| return status; | ||
| } |
There was a problem hiding this comment.
📝 Info: PostCrudHelper adds a new Post to the tracked context before validation on the create path
In BuildAndSave/BuildAndSaveAsync (create path) the new Post is added to the context (ResolvePost / _db.Posts.Add) before blogger/tag resolution and ValidateEntity. When validation fails the method returns without calling SaveChanges, leaving an Added-but-unsaved Post in the scoped change tracker. Because the DbContext is per-request scoped and no further SaveChanges runs on that request, nothing is persisted, and the subsequent SetupSecondaryData only queries Blogs/Tags. So this is benign today, but it is a latent footgun: any later SaveChanges on the same request/scope (e.g. if ICrudServices and IPostCrudHelper are both used and a save happens) would persist the partially-built Post.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fair point — fixed in b4a8fd9 rather than left as a latent footgun, since it was a small change.
ResolvePost no longer calls _db.Posts.Add for the create path; the new Post stays untracked until after blogger/tag resolution and ValidateEntity succeed:
status.CombineStatuses(ValidateEntity(post));
if (status.HasErrors)
return status;
if (!isUpdate)
_db.Posts.Add(post);
_db.SaveChanges();Same in BuildAndSaveAsync. Attaching after validation is still correct for the many-to-many write, because the Tag entities resolved from _db.Tags are already tracked, so EF just creates the PostTag join rows.
Covered by Test14PostCrudHelperTracking, which asserts a failed create leaves no Added Post in the change tracker and that a subsequent SaveChanges on the same scope persists nothing. Confirmed it fails against the previous implementation.
| public IActionResult Delete(int id, [FromServices] ICrudServices service) | ||
| { | ||
|
|
||
| var response = service.Delete<Post>(id); | ||
| if (response.IsValid) | ||
| TempData["message"] = response.SuccessMessage; | ||
| service.DeleteAndSave<Post>(id); | ||
| if (service.IsValid) | ||
| TempData["message"] = service.Message; | ||
| else | ||
| //else errors, so send back an error message | ||
| TempData["errorMessage"] = new MvcHtmlString(response.ErrorsAsHtml()); | ||
| TempData["errorMessage"] = service.GetAllErrors(); |
There was a problem hiding this comment.
📝 Info: Delete-error message rendering changed from HTML (
) to plain joined string
The controllers now set TempData["errorMessage"] = service.GetAllErrors(); (a plain string, typically newline-joined) and the views render it with @Html.Raw(...). The old code stored MvcHtmlString(response.ErrorsAsHtml()) which produced <br/>-separated markup. Multi-error delete messages will now render on a single line (newlines collapsed by HTML) instead of separate lines. Purely cosmetic, but a behavior change from the original. Also note @Html.Raw on GetAllErrors() output would render any HTML in an error string unescaped — fine for these fixed internal messages, but keep in mind if error text ever becomes data-derived.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Both points addressed in b4a8fd9.
On the XSS half (same root cause as your Views/Posts/Index.cshtml comment): the views no longer use @Html.Raw. It mattered more than "fixed internal messages" suggested, because the Slug-uniqueness error interpolates a persisted Tag name.
On the cosmetic half: rather than reintroducing raw HTML to get <br/> back, the container now sets white-space: pre-line, so the newline-joined output of GetAllErrors() renders one error per line while staying HTML-encoded — same visual result as the old ErrorsAsHtml() output, without the injection surface.
| "ConnectionStrings": { | ||
| "SampleWebAppDb": "" | ||
| } |
There was a problem hiding this comment.
📝 Info: Production appsettings.json ships an empty connection string
SampleWebApp/appsettings.json sets ConnectionStrings:SampleWebAppDb to "". Program.cs passes this straight to UseSqlServer(...), and MigrateAndSeed runs on startup, so a non-Development run without an override (env var / appsettings.Production.json) will fail at startup when EF tries to open an empty connection string. This is intended (the README documents supplying the string), but it means the app cannot start out-of-the-box outside Development.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, and intended — but you're right that the failure mode was poor, so b4a8fd9 improves it.
Program.cs now validates the connection string up front instead of passing "" into UseSqlServer and failing somewhere inside MigrateAndSeed:
var connectionString = builder.Configuration.GetConnectionString("SampleWebAppDb");
if (string.IsNullOrWhiteSpace(connectionString))
throw new InvalidOperationException(
"No connection string named 'SampleWebAppDb' was found. Set it via " +
"`dotnet user-secrets set \"ConnectionStrings:SampleWebAppDb\" \"...\"` or the " +
"ConnectionStrings__SampleWebAppDb environment variable. See README.md.");Note appsettings.Development.json is now also empty (the SA credentials there were flagged separately), so this applies in Development too — the README documents user secrets and the env var as the two supported ways to supply it.
| @if (TempData["errorMessage"] != null) | ||
| { | ||
| <div class="text-danger"><strong>@TempData["errorMessage"]</strong></div> | ||
| <div class="text-danger"><strong>@Html.Raw(TempData["errorMessage"])</strong></div> |
There was a problem hiding this comment.
🟨 Error messages rendered unescaped with Html.Raw may allow stored XSS
The delete/error path stores service.GetAllErrors() (and other status messages) in TempData["errorMessage"] and the Index views now render it with @Html.Raw(TempData["errorMessage"]) (e.g. SampleWebApp/Views/Posts/Index.cshtml:15, Views/Tags/Index.cshtml:15, Views/TagsAsync/Index.cshtml:15, Views/PostsAsync/Index.cshtml:15, Views/Blogs/Index.cshtml:12). Some of these error strings interpolate persisted, user-controlled entity data (for example the Slug-uniqueness error embeds a Tag's Name: "The Slug on tag '{0}' must be unique..."). If an attacker stores a Tag/Blog name containing HTML/script, the message is emitted without HTML-encoding, allowing script injection when the error is later displayed.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — fixed in b4a8fd9.
You're right that this is exploitable in principle: the Slug-uniqueness message interpolates a persisted Tag name ("The Slug on tag '{0}' must be unique..."), so a tag named <script>...</script> would be emitted unescaped. Html.Raw was also simply wrong here now — the controllers store service.GetAllErrors(), which is a plain string, not markup. All five Index views now use:
<div class="text-danger" style="white-space: pre-line"><strong>@TempData["errorMessage"]</strong></div>@TempData[...] HTML-encodes, and white-space: pre-line renders the newline-separated multi-error string across lines — which also restores the <br/>-separated layout you flagged separately in the PostsController delete-path comment.
| var connectionString = | ||
| Environment.GetEnvironmentVariable("SAMPLEWEBAPPDB_CONNECTION") | ||
| ?? "Server=localhost,1433;Database=SampleWebAppDb;User Id=sa;Password=Design_Time_Only!;TrustServerCertificate=True;MultipleActiveResultSets=True"; |
There was a problem hiding this comment.
🟨 Hardcoded SQL Server credentials committed in source
A design-time fallback connection string with a hardcoded SA password is committed in DataLayer/DesignTimeDbContextFactory.cs:20 (Password=Design_Time_Only!), and SampleWebApp/appsettings.Development.json:9 ships a connection string with User Id=sa;Password=ChangeMe_Passw0rd. Committing credentials—even sample/design-time ones—normalizes checking secrets into source control and can be copied into real deployments.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in b4a8fd9 — agreed, no credentials should be in source even as samples.
DesignTimeDbContextFactory: the fallback is now credential-free (Trusted_Connection=True). It is only enough for offline tooling such asdotnet ef migrations add, which builds the model without opening a connection;dotnet ef database updaterequiresSAMPLEWEBAPPDB_CONNECTION, and the XML doc comment now says so.appsettings.Development.json: the SA connection string is gone (empty value + a_commentpointing at the right mechanism).- Added a
UserSecretsIdtoSampleWebApp.csprojsodotnet user-secretsis the documented local path, and updated the README with both user-secrets and env-var options. Program.csnow fails fast with an actionable message instead of handing an empty string toUseSqlServer, which also addresses the separate note aboutappsettings.jsonshipping an empty connection string.
…rom PR review Addresses the Devin Review findings on PR #27: - bug: Posts/PostsAsync Edit(GET) returned 500 for an unknown or deleted id because ICrudServices.ReadSingle returns null and the result was passed straight to IPostCrudHelper.SetupSecondaryData; now returns NotFound(). - bug: TempData[errorMessage] was rendered with @Html.Raw in the five Index views while the controllers now store plain text from GetAllErrors(), so persisted user-controlled data (e.g. a Tag name in the Slug-uniqueness message) could inject markup. Rendered HTML-encoded, with white-space:pre-line restoring the multi-error line breaks the old MvcHtmlString/<br/> output had. - bug: removed the hardcoded SA credentials from DesignTimeDbContextFactory and appsettings.Development.json; the connection string now comes from user secrets or the ConnectionStrings__SampleWebAppDb env var, and Program.cs fails fast with a clear message when it is missing. - bug: PostCrudHelper no longer adds a new Post to the change tracker before validation, so a failed create cannot leave an Added entity for a later SaveChanges. - Null-guarded SimplePostDto/SimplePostDtoAsync TagNames to match DetailPostDto. Tests: 57 pass (7 new). The Edit-NotFound and create-tracking tests were confirmed to fail without their fixes. Test17 asserts the GenericServices read projection really populates the Tags collection on the Post DTOs. Co-Authored-By: Parker Duff <pwjduff@gmail.com>
Runtime verification of the review fixesRan the migrated app in the VM (.NET 10 + SQL Server in Docker, seeded Two caveats up front:
Results
Primary fix: stale Post edit id now 404s instead of throwing a NullReferenceExceptionCreated a throwaway Post (id 1002), deleted it, then hit its now-stale edit URL. Previously
Control — valid id still renders the fully populated form, so the guard doesn't reject legitimate requests: Post create regression (validate-before-Add reorder)Deliberate failure first — title with Then corrected — created with the right blogger and both tags ( Duplicate Tag slug + delete error bannerSlug Tag list unchanged at 8 — nothing inserted:
DB state after the run was back to the clean seeded baseline: Session: https://app.devin.ai/sessions/3685003c30a74180b63a714e6149d960 |
Summary
Re-platforms the whole solution from ASP.NET MVC 5 / .NET Framework 4.5.1 / EF6 / GenericServices 1.0.9 / Autofac / OWIN to ASP.NET Core MVC / .NET 10 / EF Core 10 / EfCore.GenericServices 10 / built-in DI. This is a re-platform, not a version bump: every project is now SDK-style
net10.0,System.Web/Global.asax/OWIN are gone, and the data access is EF Core with code-first migrations. Full research of the gotchas is inMIGRATION_NOTES.md.Work was split across three parallel child sessions (Data/Service/Biz → PR #24, Web app → PR #25, Tests → PR #26) and merged here, then finalized (EF Core migration, README, solution references).
dotnet build SampleWebApp.sln→ 0 errors;dotnet test→ 57/57 pass; the app runs on .NET 10 against SQL Server and the Blogs/Posts/Tags CRUD flows work end-to-end (verified in-VM, recording linked in the session).Data layer (EF6 → EF Core)
SampleWebAppDbnow takesDbContextOptions<SampleWebAppDb>(name-based ctor gone).IGenericServicesDbContext,EfConfiguration, and EF6 database initializers removed.ValidateEntity/DbEntityValidationResultdon't exist in EF Core, soTag.Sluguniqueness is enforced two ways:HandleChangeTracking(TrackUpdate.LastUpdated) ported into overriddenSaveChanges/SaveChangesAsync; the EF6 early-returnbug (aborted on the first non-TrackUpdateentity) is fixed tocontinue.InitialCreatemigration (Blogs,Tagsw/ uniqueSlug,Posts→BlogFK,PostTagjoin) +DesignTimeDbContextFactorysodotnet efdoesn't run app startup.DataLayerInitialise.ResetBlogskeeps the XML seeding, now via EF CoreSaveChanges.Service layer (GenericServices → EfCore.GenericServices)
EfGenericDto<TEntity,TDto>to POCOs implementingILinkToEntity<TEntity>; reads go throughICrudServices/ICrudServicesAsync(ReadManyNoTracked<T>,ReadSingle<T>,DeleteAndSave<T>).SetupSecondaryData(blogger dropdown + tags multiselect) has no equivalent in EfCore.GenericServices, so it's moved into a hand-writtenIPostCrudHelper. Post create/update run there (notICrudServices.CreateAndSave) to preserve the many-to-many Tag resolution, blogger selection, andIValidatableObjectrules.IServiceCollectionextensions:AddDataLayer(),AddServiceLayer()(wiresGenericServicesSimpleSetup<SampleWebAppDb>+IPostCrudHelper),AddBizLayer().Web app (MVC 5 → ASP.NET Core MVC)
Global.asax+App_Start/*+ OWIN +DiModelBinder/WebUiInitialise/AutofacDireplaced by a single minimal-hostingProgram.cs(AddControllersWithViews,AddDbContext<SampleWebAppDb>(UseSqlServer),AddServiceLayer/AddBizLayer, endpoint routing; migrates + seeds on startup).Microsoft.AspNetCore.Mvc; the custom action-parameter injection replaced with[FromServices].Web.config→appsettings.json/appsettings.Development.json(connection string keySampleWebAppDb).Content/+Scripts/+fonts/→wwwroot/with plain<link>/<script>tags (bundling removed);_ViewImports.cshtmladded.MIGRATION_NOTES.md§4/§5).Tests
net10.0; NUnit 4 / Moq 4.20 / Test.Sdk 17.11; in-memory SQLiteSampleWebAppDb(honors the Slug unique index). Autofac-module tests rebuilt as DI-extension tests;ModelStateTesterre-implemented on ASP.NET Core validation.InternalsVisibleTo("Tests")restored viaDataLayer/Properties/AssemblyInfo.cs.Testsnow also referencesSampleWebAppso controller actions can be unit-tested (Group06Mvc/Test03PostsControllerEdit.cs).Review fixes (commit b4a8fd9)
Addressing the Devin Review findings; each is replied to in its own thread.
EditGET crashed on a stale id.ICrudServices.ReadSinglereturnsnullfor a missing key, and the result was handed straight toIPostCrudHelper.SetupSecondaryData, which dereferences it → unhandledNullReferenceException. Both sync and async nowreturn NotFound()first. Regression-tested (confirmed the test fails without the guard).TempData["errorMessage"]with@Html.Raw, but the controllers now store plain text fromGetAllErrors()— and the Slug-uniqueness message interpolates a persisted Tag name. Now HTML-encoded, withwhite-space: pre-linepreserving the multi-error line breaks the oldMvcHtmlString/<br/>output produced.DesignTimeDbContextFactory's fallback is credential-free (only sufficient for offlinedotnet ef migrations add;database updateneedsSAMPLEWEBAPPDB_CONNECTION), andappsettings.Development.jsonno longer carries an SA password. Added aUserSecretsId, documented user-secrets + env-var in the README, andProgram.csnow fails fast instead of passing""toUseSqlServer.PostCrudHelperno longer tracks before validating. A newPostwasAdded to the scoped context before blogger/tag resolution and validation, so a failed create left anAddedentity that a laterSaveChangeson the same request could persist. TheAddmoved to after validation; attaching late is still correct for the many-to-many write because the resolvedTagentities are already tracked.SimplePostDto/SimplePostDtoAsyncTagNamesnull-guarded to matchDetailPostDto, plusTest17CrudServicesReadProjectsTagsCollectionwhich asserts the GenericServices read projection really does populate theTagscollection (the implicitTag→Tagmap the reviewer flagged as unverified).Notes / follow-ups
NU1903build warnings (documented with rationale inMIGRATION_NOTES.md§13):AutoMapper 13.0.1(pinned by EfCore.GenericServices 10; the DoS needs ~25k-deep self-referential graphs, impossible here) and design-time-onlySystem.Security.Cryptography.Xml 9.0.0(transitive via EF Core Design,PrivateAssets=all, not shipped).BeforeSaveChangesstatus and a throwing backstop on directSaveChanges(preserving the EF6ValidateEntitycontract). Two brand-new Tags sharing a slug in a single save are caught by the DB unique index rather than the pre-save check — same as the EF6 behaviour, and unreachable from the UI.Link to Devin session: https://app.devin.ai/sessions/3685003c30a74180b63a714e6149d960
Devin Review