diff --git a/docs/refactor-backend-findings.md b/docs/refactor-backend-findings.md new file mode 100644 index 0000000..1ee176a --- /dev/null +++ b/docs/refactor-backend-findings.md @@ -0,0 +1,108 @@ +# Backend architecture refactor — findings + +Tracking doc for `refactor/backend-architecture-cleanup`. Each item is verified +against the actual code (not just a grep hit) before being listed here. +Checked off as fixed, with tests, on this branch. + +## 1. Layering / dependency-injection violations + +- [ ] `AssetController` injects `UserManager` via constructor + but never uses it anywhere in the file — dead dependency, remove it. +- [ ] `ContactController` depends on both `IContactService` **and** + `ILocationService` and implements full Location CRUD (`GetAllLocationsAsync`, + `CreateLocationAsync`, `UpdateLocationAsync`, `DeleteLocationAsync`, + `GetLocationsPagedAsync`) even though a dedicated `LocationController` + with the same `ILocationService` already exists. Move the Location + endpoints out of `ContactController` into `LocationController`; a + Contact controller should depend only on `IContactService`. +- [ ] `ArchitectureTests` (G2) currently passes with 0 failures — dependency + *direction* is otherwise clean (no controller/service injects + `DbContext`/`IConfiguration`/a concrete class). Re-run after each fix + above to confirm it stays green. + +## 2. Validation coverage gap + +Established pattern: `I{Action}{Feature}Validation : IValidator<{Dto}>` + +`{Action}{Feature}Validation : AbstractValidator<{Dto}>` in +`SeaHaven.Services/Validation/`, injected into the owning business service +(see `LocationValidation.cs` / `LocationService`). Confirmed: **validation +stays in the service layer** (not moved to data services) — only extending +coverage to features that lack it. + +Have a validator today: Account, Asset, Contact, Dispatch, Employee, FollowUp, +Location, PMSchedule, Vendor, WorkOrder, WorkOrderBoardCreate. + +Confirmed missing (real external Create/Update DTO, no validator): +- [ ] `CalendarService.CreateEventAsync` / `UpdateEventAsync` +- [ ] `CompletionDocTemplateService.CreateAsync` / `UpdateAsync` +- [ ] `QuotesService.CreateQuoteAsync` / `UpdateQuoteAsync` +- [ ] `ServicesRegistryService.CreateAsync` / `UpdateAsync` +- [ ] `TaskListTemplateService.CreateAsync` / `UpdateAsync` +- [ ] `TeamMemberInviteService.CreateAsync` +- [ ] `TeamMemberService.CreateAsync` / `UpdateAsync` +- [ ] `VendorCompanyRosterService.CreateRosterAsync` +- [ ] `VendorOperationsService.UpdateAvailabilityAsync` / `UpdateCommercialStatusAsync` +- [ ] `VendorPortalService.UpdateChecklistItemAsync` +- [ ] `WorkOrderCommentService.UpdateCommentAsync` +- [ ] `WorkOrderDispatchService.UpdateDispatchAsync` / `UpdateChecklistItemAsync` +- [ ] `WorkOrderMediaService.UpdateMediaCategoryAsync` +- [ ] `WorkOrderPocService.UpdatePocAsync` +- [ ] `WorkOrderUpliftService.CreateAsync` +- [ ] `DropdownOptionsService.CreateAsync` / `UpdateAsync` +- [ ] `AuthenticationService.UpdateProfileAsync` + +Ruled out (internal/no external DTO, not a validator candidate): +`AuthenticationService.CreateSessionAsync`, `VendorPortalTokenService.GetOrCreateActiveTokenAsync`, +`WorkOrderAccountResolver` (no real Create/Update method; grep false positive). + +## 3. N+1 / query-shape issues (verified, not grep-only) + +- [ ] **`WorkOrderService.DeleteWorkOrderCascadeAsync`** loads all comments + then calls `_commentDataService.DeleteAsync(comment.Id)` once per + comment. `CommentDataService.DeleteAsync` does its own `GetByIdAsync` + (1 SELECT) + `SaveChangesAsync` (1 commit) **per call** — N comments = + 2N round trips and N separate commits, breaking the one-commit + convention (§4) as well as being slow. Fix: add a batch delete + (`DeleteAllForWorkOrderAsync` via `ExecuteDeleteAsync`, or load once + + `RemoveRange` + one `SaveChangesAsync`) and call it once. +- [ ] **`WorkOrderService.SaveAttachmentsAndPhotos`** loops over uploaded + attachments calling `_workOrderDataService.AddWorkOrderAttachmentAsync` + per file; that method does `AddAsync` + `SaveChangesAsync` **per call** — + N attachments = N separate commits. Fix: collect the `WorkOrderAttachments` + entities in the loop (file storage save can stay per-file — that's I/O, + not DB), then add a batch method that calls `AddRangeAsync` + one + `SaveChangesAsync`. +- [ ] **`VendorCompanyRosterDataService`** (3 call sites: roster update, + create, and a third) loads `existingTechnicians` once via a single + query, then does `existingTechnicians.FirstOrDefault(e => e.Id == ...)` + **inside** a loop over the incoming roster — O(n·m) in-memory scan. + Fix: build `existingTechnicians.ToDictionary(e => e.Id)` once before the + loop and `TryGetValue` inside it — O(n+m). + +## Ruled out during the sweep (false positives / legitimate design) + +- `WorkOrderAuditDataService.PersistAsync`: the `foreach` detaches conflicting + entries from a caught `DbUpdateException`; the retry `SaveChangesAsync` is + after the loop, not inside it. Not an issue. +- `VendorPortalTokenService.RotateAsync` / `RevokeAllAsync`: the `foreach` + only mutates already-tracked entities in memory (`t.RevokedAt = now`); the + single `SaveChangesAsync` is after the loop. Not an issue. +- `WorkOrderWeekRolledService`: calls `TryProcessWeekRolledAsync` once per + work order inside a batch-job loop, each with its own try/catch. This is + intentional per-item fault isolation for a background job — collapsing it + into one transaction would change the failure semantics (one bad work order + would abort the whole batch). Not an issue; do not "optimize" this away. +- Legacy `SeaHavenIndustries` project (old Blazor admin app) has its own + ad-hoc `Service` classes touching `DbContext` directly, violating the + layering model wholesale — but it is being actively sunset (see commit + `64ca9e0`, "add legacy deprecation middleware and Blazor WO sunset guard"). + Out of scope: refactoring code on its way out is wasted effort. + +## Still to audit + +- Cancellation-token forwarding (§6) on data-service methods — not yet swept. +- Error disclosure (§5) — spot-check only so far (`VendorCompanyRosterController` + catches `DbUpdateConcurrencyException` directly; check whether that + duplicates the existing `ConcurrencyExceptionFilter`). +- Tenant-scope derivation (§2) — not yet swept. +- `AsNoTracking()` coverage on read-only data-service queries — not yet swept.