mirror of
https://github.com/Sea-Haven-Industries/shoc-backend.git
synced 2026-10-06 17:02:12 +00:00
d01f847a4d
AssetController and ContactController had [Authorize] commented out with "Removed for testing", leaving every asset and contact endpoint reachable without a token. Restore the attribute. Also drop AssetController's unused UserManager dependency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
125 lines
7.2 KiB
Markdown
125 lines
7.2 KiB
Markdown
# 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.
|
|
|
|
## 0. Security finding (not an architecture item, fixed anyway — user confirmed)
|
|
|
|
- [x] **`AssetController` and `ContactController` both had `[Authorize]`
|
|
commented out** (`//[Authorize] // Removed for testing`), so every
|
|
endpoint on both — asset CRUD, contact CRUD, and (via the next item)
|
|
Location CRUD — was reachable with no authentication. Git history shows
|
|
it landed via a commit titled "fix" / "backend changes" — looks like
|
|
forgotten debug code, not an intentional exception. Confirmed with the
|
|
user before fixing (changes production auth behavior). **Fixed:**
|
|
restored `[Authorize]` on both.
|
|
|
|
## 1. Layering / dependency-injection violations
|
|
|
|
- [x] `AssetController` injected `UserManager<ApplicationUser>` via constructor
|
|
but never used it anywhere in the file. **Fixed:** removed the dead
|
|
dependency and its now-unused `using` statements.
|
|
- [ ] `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)
|
|
|
|
- [x] **`WorkOrderService.DeleteWorkOrderCascadeAsync`** loaded all comments
|
|
then called `_commentDataService.DeleteAsync(comment.Id)` once per
|
|
comment (2N round trips, N separate commits — broke the one-commit
|
|
convention, §4). **Fixed:** added `ICommentDataService.DeleteAllForWorkOrderAsync`
|
|
(single `ExecuteDeleteAsync`), caller now calls it once.
|
|
- [x] **`WorkOrderService.SaveAttachmentsAndPhotos`** looped over uploaded
|
|
attachments calling `_workOrderDataService.AddWorkOrderAttachmentAsync`
|
|
per file (N separate commits). **Fixed:** added
|
|
`AddWorkOrderAttachmentsAsync` (mirrors the existing `AddAuditLogsAsync`
|
|
batch pattern already in this interface) — `AddRangeAsync` + one
|
|
`SaveChangesAsync`; file-storage save stays per-file (I/O, not DB).
|
|
Also fixed the same class of issue in the before/after/signoff-photo
|
|
block right below it: up to 3 separate `GetByIdAsync` + `UpdateAsync`
|
|
(each its own commit) when all three photos are submitted together,
|
|
now 1 `GetByIdAsync` + 1 `UpdateAsync`.
|
|
- [x] **`VendorCompanyRosterDataService.SaveRosterAsync`** (the roster-update
|
|
method only — checked the other two call sites and they don't do this:
|
|
`CreateRosterAsync` is a pure additive create with nothing to match
|
|
against, and the third occurrence only does a uniform update over all
|
|
existing technicians, no per-item lookup) loads `existingTechnicians`
|
|
once via a single query, then did
|
|
`existingTechnicians.FirstOrDefault(e => e.Id == ...)` **inside** a loop
|
|
over the incoming roster — O(n·m) in-memory scan. **Fixed:** built
|
|
`existingTechniciansById = existingTechnicians.ToDictionary(v => v.Id)`
|
|
once before the loop and used `TryGetValue` inside it — O(n+m).
|
|
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.
|