shoc-backend/ARCHITECTURE_AND_CODE_QUALITY.md

# Architecture and Code Quality (canonical)

> **Status: canonical.** This document owns the *meaning* of the backend's
> architecture and code-quality rules. On any conflict with the older
> `BACKEND_ARCHITECTURE.md`, **this document governs**; that file is retained
> only as historical reference pending removal.
>
> Companion documents:
> - `QUALITY_GATES.md` — *how* each rule is enforced (commands + CI mapping).
> - `REVIEW_AND_PR_FRAMEWORK.md` — *who* reviews, in what order, with what
>   evidence, and how exceptions are recorded.
> - `AGENTS.md` — precedence and repo-specific agent delta.

## 1. Layering model

The backend is a feature-oriented service boundary over Entity Framework Core:

```text
HTTP controller  ->  I{Feature}Service  ->  I{Feature}DataService  ->  ApplicationDbContext
```

The interfaces are **architectural seams**, not a mandate to wrap every class or
every EF method. Add an interface when it separates HTTP, business,
persistence, or external I/O responsibility *and* enables behavior-focused
testing through the public contract.

| Layer | May depend on | May not depend on |
|-------|---------------|-------------------|
| API controller | `I{Feature}Service`, `ILogger`, framework types | `DbContext`, concrete services, concrete data services, raw SQL, business workflow code |
| Business service (`SeaHaven.Services`) | `I{Feature}DataService`, `IOptions<T>`, `ILogger`, pure ports (`IServicePorts`, `IDispatchEmailPort`, …) | `DbContext`, `IConfiguration`, concrete data services, a generic repository |
| Data service (`SeaHaven.DataServices`) | `ApplicationDbContext`, EF Core | Business decisions, HTTP concerns |
| Composition / infrastructure | `Program.cs`, `*Module.cs`, migrations, Identity | Being depended on by controllers or business services |

### 1.1 Controllers

- Parse HTTP input, authorize the caller, invoke a business-service interface,
  and map the result to the established HTTP contract.
- Depend **only on `I{Feature}Service` interfaces** — never on `DbContext`, EF,
  concrete service implementations, or concrete data-service implementations.
- Contain no persistence queries and no business workflows.
- Map known failures explicitly; let the centralized exception boundary handle
  unexpected failures (see §5).

### 1.2 Business services

- Own validation, business decisions, orchestration, and **transaction intent**
  (the *what* of semantic atomicity — see §4).
- Depend on **feature-specific data/query/command interfaces**, never on a
  generic repository and never on `DbContext`.
- Use `IOptions<T>` for typed configuration. Never inject `IConfiguration`.
- Keep feature responsibilities cohesive. Split a service when it has
  independently changing business reasons — **not** because it crossed an
  arbitrary line count (see §9, "no godfile threshold theater").

### 1.3 Data services

- Own EF Core query shape, persistence, batching, and **transaction mechanics**
  (the *how* of commits — see §4).
- Accept `CancellationToken` on new I/O methods and propagate it (see §6).
- Use `AsNoTracking()` for read-only queries.
- Project only required columns for list/read models; include full graphs only
  when the caller needs them.
- Page unbounded collections at the database.
- Avoid query-in-loop / save-in-loop patterns; prefer set-based reads and
  batched writes **while preserving the workflow's failure and transaction
  semantics**.
- Do **not** introduce a generic repository over EF Core. Feature data services
  expose intent-revealing operations.

### 1.4 Infrastructure ownership and the EF allowlist

EF Core / `DbContext` is owned by approved infrastructure only. The explicit
allowlist of places that may touch EF directly:

- `SeaHaven.DataServices.Implementation.*` (feature data services).
- `Data.SeaHavenIndustries` — the `ApplicationDbContext`, entity configuration,
  and **migrations** (see §7).
- `SeaHaven.DataServices.DependencyInjection.DataServicesModule` — context
  registration and lifetime.
- Identity / authentication composition in `Program.cs` and the DI modules
  (`AddDbContext`, `AddIdentity`, token/options wiring).
- Transaction mechanics inside data services when semantic atomicity requires
  it (see §4).

Anything outside this allowlist that references EF / `DbContext` is an
architecture violation. New allowlist entries require an ADR (see §10).

## 2. Tenant scope and authorization

- **Tenant scope is server-derived.** Filtering by tenant/customer/owner is
  applied in the data access layer from authenticated principal claims, never
  from client-supplied request bodies or query strings.
- **Authorization remains at service entry.** A business service's public method
  is the trust boundary: it authorizes the caller and resolves the effective
  tenant scope before doing work. Controllers express HTTP-level authorization
  (`[Authorize]`, route/policy) and then delegate; they do not re-implement
  service-level authorization.
- Data services receive an already-resolved, server-derived scope; they must not
  invent authorization rules of their own.

## 3. Dependency-direction enforcement (mechanical)

`ArchitectureTests` (in `Api.SeaHavenIndustries.Tests`) enforces mechanical
dependency direction by reflection over constructor parameters:

1. Controllers do not inject `DbContext`, concrete data services, or concrete
   service implementations.
2. Business services do not inject `DbContext` or `IConfiguration`.
3. Business services depend on interfaces for their data/persistence
   dependencies, not on concrete data-service or service implementations.
4. **Discovery must not silently pass on an empty set.** Each invariant asserts
   the discovered type set is non-empty, so the gate **fails** if reflection
   ever returns zero controllers/services (e.g., after a namespace move). A
   passing run therefore means "real types were checked," not "nothing was
   found."

These tests cover *dependency direction*. Behavior lives in behavior tests
through public interfaces. Do **not** add tests merely to prove a linter or
analyzer itself works — test the product behavior the rule protects. Do not
claim full runtime properties from constructor reflection; it proves wiring
shape, not execution behavior.

## 4. Transaction semantics and the one commit convention

- **Services own semantic atomicity.** A business service decides which set of
  operations constitutes a unit of work and expresses that intent.
- **Scoped infrastructure owns transaction mechanics.** The actual commit
  boundary is `DbContext.SaveChangesAsync(...)` and lives **inside a data
  service**. A service must not sprinkle independent `SaveChanges` calls across
  data-service boundaries in a way that breaks the semantic unit.
- **The repo documents one commit convention:** a single
  `SaveChangesAsync(CancellationToken)` inside the owning data service is the
  atomic commit boundary. This is the current convention observed across the
  data-service layer.
- When a unit of work genuinely requires multiple statements to be all-or-
  nothing, use EF Core's transaction (`IDbContextTransaction`) **inside the data
  service**. Explicit transactions are infrastructure (EF allowlist), owned by
  data services, never by controllers or business services.
- Do not "optimize" atomicity away: collapsing several semantically independent
  saves into one final save changes partial-success behavior and is a business
  decision, not mechanical cleanup.

## 5. Error disclosure and structured logging (security)

- **No HTTP response may expose** arbitrary exception messages, stack traces,
  or SQL/provider/path/credential/crypto detail. Known failures are mapped to
  explicit public messages; unexpected failures are funneled through the
  centralized boundary (`SanitizedErrors`), which returns a stable message plus
  a correlation reference and never echoes internal exception text.
- **Correlated structured internal logs** record the original exception at
  `Error` level with a correlation id, so incidents are traceable without
  leaking detail to the client.
- **Secrets are never logged.** Configuration values, tokens, connection
  strings, and credentials are treated as redacted; structured log templates
  must not interpolate secret-bearing fields.
- The `SanitizedErrorsTests` guard the public-facing contract (stable message,
  correlation id present, original exception text absent, original logged at
  `Error`).

## 6. Cancellation

- New async I/O methods accept a `CancellationToken` where the public contract
  permits and **propagate it** to EF Core / downstream I/O.
- **Do not falsely claim propagation.** A `CancellationToken` parameter that is
  ignored, swallowed, or replaced with `default`/`CancellationToken.None` at the
  call site is a defect, not a courtesy. The signature is a contract.
- Enforcement combines **behavior tests** (assert the token reaches the data
  service / EF call for changed I/O paths) and **analyzers** where available.
  Either is acceptable evidence; silence is not. For changed paths, review must
  confirm forwarding, not merely the presence of the parameter.

## 7. Database migrations

Migration gates require **actual-provider** evidence, not reflection or
assumptions:

- **Actual-provider discovery** — the migration provider is the configured
  database provider (e.g., SQL Server / Npgsql), discovered from the runtime
  configuration, not inferred from package references.
- **Snapshot consistency** — `ModelSnapshot` must be consistent with the model
  after the migration set (`dotnet ef ... --output` / script diff with no
  phantom operations).
- **Empty-db apply** — applying the full migration history to an empty database
  must reproduce the current model with no errors.
- **Supported-prior upgrade** — upgrading from each still-supported prior state
  applies cleanly.
- **No untested rollback claims.** Do not assert "down/rollback is supported"
  unless a downgrade is actually exercised against the real provider; otherwise
  state rollback as **not guaranteed**. EF "EnsureDeleted"/re-create is not a
  migration rollback.

## 8. Query performance and complexity

For each changed hot path, document and test:

- input-size variables;
- CPU and memory complexity (Big-O) for any in-memory reconstruction;
- database round trips;
- whether filtering, ordering, and paging execute **in SQL** vs. in memory;
- whether tracking is necessary;
- whether repeated work can be hoisted out of loops;
- the partial-failure and transaction behavior.

Distinguish two classes of finding:

- **Analyzable anti-patterns** — query-in-loop / save-in-loop, unbounded load
  followed by in-memory `Where`, repeated `First`/`Single` scans over a result
  (prefer dictionary/hash-set reconstruction, normally `O(n)` vs `O(n²)`). These
  can be flagged by inspection.
- **SQL-backed evidence** — actual paging/filtering/sort location, query count,
  and plan shape must be backed by provider query logs, `ToQueryString()`,
  store-generated SQL, or a plan/count measurement. Do **not** assert "executes
  in SQL" or "single round trip" from LINQ shape alone.

Do not optimize by weakening correctness: batching all writes into one final
save can change partial-success behavior; adding a transaction can change
locking/retry semantics. Both are business decisions.

## 9. Size, complexity, and "no godfile theater"

- We do **not** enforce arbitrary line-count or file-size thresholds as quality
  gates. A number is not an architecture.
- A file is too large when it has **independently changing responsibilities**
  (low cohesion) or **untestable coupling**, not when it crosses a count.
  Split on seams, not on a counter; describe the seam in the PR.
- Gates that operate on size must name the *consequence* (e.g., "two unrelated
  features in one service") and be backed by review, not by a magic threshold.

## 10. Exceptions and ADRs

Some rules here are hard (EF allowlist, no client exception leakage, server-
derived tenant scope). A deviation from a hard rule requires an **Architecture
Decision Record** under `docs/adr/` (create the path if absent):

- A short `NNNN-title.md` stating context, decision, consequences, and the
  specific rule being excepted.
- The excepted rule and an expiry/review date.
- Reference from the PR that relies on it.

Without an ADR, the rule stands as written. Explicit, documented exceptions
only — **no wildcard suppressions** (no global `[SuppressMessage]`, no
`.editorconfig` `dotnet_diagnostic.*.severity = none` sweeps, no
`#pragma` swaths). A suppression must name the single diagnostic and cite the
ADR/justification.

## 11. Configuration

Configuration is bound once during composition in
`SeaHaven.Services/DependencyInjection/ServicesModule.cs`. Business services
receive typed options (`FrontendOptions`, `JwtOptions`, `ApprovalsOptions`,
`VendorPortalOptions`, …). Defaults must preserve current behavior. Startup
validation is introduced only when every deployed environment is known to
provide the required value.

## 12. Review checklist (quick reference)

- Controller depends only on `I{Feature}Service` abstractions.
- Business logic is in a cohesive feature service.
- EF operations are behind a feature data-service abstraction.
- No raw `IConfiguration` in a business service.
- No unbounded load + in-memory filtering.
- No query/save loop where a set-based operation preserves semantics.
- Read-only EF queries use no tracking.
- New async I/O accepts and **propagates** cancellation where the contract
  permits (verified, not just signed).
- Errors do not leak internal exception details; secrets are not logged.
- Tenant scope is server-derived; authorization enforced at service entry.
- Commit convention respected (single `SaveChangesAsync` in the owning data
  service; transactions only in data services when required).
- Tests cover behavior, authorization, data contracts, ordering, duplicates,
  missing records, and relevant failure boundaries.
- Changed behavior checked against the authoritative ticket board (see
  `REVIEW_AND_PR_FRAMEWORK.md`). Missing board access blocks readiness/merge.