diff --git a/.coderabbit.yaml b/.coderabbit.yaml index fdbe946c..d4aba0c0 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -41,6 +41,17 @@ reviews: HandleAsync (NOT Handle). Private instance fields prefixed with _; constants and static readonly use PascalCase. Sync-over-async patterns (.Result, .Wait(), .GetAwaiter().GetResult()) are banned at build time — flag any sneak-through. + + MODEL NAMING (intent-based, by role). Flag a generic `*Dto` suffix on a + REQUEST model — name inbound CQRS messages `*Command` (writes) / `*Query` + (reads), gRPC contracts `*Request`/`*Response`. `*Dto` is reserved for + read-side projections (the shape a query handler projects to, e.g. + OrderSummaryDto) and is acceptable ONLY there. So: a new `CreateXDto` or + `XRequestDto` on a command/query is a Should-consider (rename to + `*Command`/`*Query`); a `*Dto` on a query-result projection is fine. Don't + recommend renaming existing `*Dto` read projections to `*Response` — + they're query contracts, not HTTP-response models. See CLAUDE.md + "Coding Standards → Model names are intent-based, by role". Interfaces earn their keep through consumer substitution (test mock or 2+ impls today, OR concrete near-term roadmap) — flag speculative interfaces. Repository wrappers around EF Core (IFooRepository + FooRepository) are explicitly NOT diff --git a/CLAUDE.md b/CLAUDE.md index 373bc535..ebfc44ca 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -112,6 +112,11 @@ project's lesson, not an inconsistency to clean up. - Private *instance* fields prefixed with `_` (camelCase). Constants and `static readonly` fields use PascalCase per .NET convention — do NOT prefix with `_` (e.g. `OrdersPlaced`, `Carriers`, `TraceIdKey`). The `.editorconfig` enforces this split via separate naming rules - Async methods suffixed with `Async` - Interfaces prefixed with `I` +- **Model names are intent-based, by role — prefer a specific suffix over a generic `Dto` on a request model.** A generic suffix like `CreateOrderDto` only says "data transfer object"; it doesn't say what the type *does* or where it belongs. Name by role instead: + - **CQRS messages:** `*Command` for writes (`PlaceOrderCommand`, `ProcessPaymentCommand`), `*Query` for reads (`GetOrderByIdQuery`, `SearchProductsQuery`). These are *more* intent-revealing than a generic `*Request` — they say write-vs-read *and* that the type flows the Wolverine message pipeline. This is the canonical inbound shape; do NOT introduce a `*Dto`/`*Request` request model where a Command/Query fits. + - **gRPC contracts:** `*Request` / `*Response` pairs (`GetProductRequest` / `ProductResponse`) — the gRPC idiom, already used in CatalogService. + - **Read projections:** `*Dto` (`OrderSummaryDto`, `ProductDto`) — the query-result shape a query handler projects to. `Dto` is acceptable *here only*, where it correctly signals "read-side projection / transfer shape" in a CQRS context; it is NOT coupled to HTTP, so don't rename it to `*Response`. + - The rule in one line: **generic `*Dto` on a request model hides intent — name it by what it does; reserve `*Dto` for read projections.** - Use `var` when type is apparent - TreatWarningsAsErrors is enabled - zero warnings allowed - Static analyzers: Meziantou, SonarAnalyzer, Roslynator @@ -208,6 +213,7 @@ These are always-on. Deeper guidance (modern EF features, transactions, caching - **Cache invalidation in the write path**: if a handler mutates a cached entity, it must invalidate or update the cache in the same handler — not "later" or "via TTL". - **Migrations are immutable once applied**: never edit a migration that has run anywhere (dev included). Destructive changes (drop column/table, rename, NOT NULL on existing column) need a multi-step plan, not a single migration. - **Measure before optimizing**: don't add caching, compiled queries, `ValueTask`, or `AsSplitQuery()` on intuition. Use BenchmarkDotNet for code paths, `dotnet-counters`/k6 for system behavior, `ToQueryString()` for EF. +- **`AsSpan` over `Substring` for zero-allocation slicing — but only on profiled hot paths, and mind the async constraint.** `Substring(...)` allocates a new `string` per call; `AsSpan(...)` returns a `ReadOnlySpan` view over the original with no allocation, and `string.Concat`, `int.Parse`, etc. have span overloads. This is a real win in *synchronous, hot, string-heavy loops* — parsers, formatters, tokenizers, bulk ID/field manipulation. **Two guardrails make it a narrow tool, not a default:** (1) it's a micro-optimization governed by "Measure before optimizing" above — apply it where profiling shows string-allocation pressure, not reflexively; (2) **`Span`/`ReadOnlySpan` is a `ref struct` — stack-only, can't cross an `await` boundary or be captured in a lambda/field**, so it's rarely usable inside NextAurora's async-everywhere request handlers (the compiler will stop you). NextAurora has no such hot path today — its string work is incidental (log templates, IDs), and the bottleneck is always I/O (EF, gRPC, HTTP), never `Substring`. The rule is preventative: *if* a synchronous string-crunching hot path appears and profiling justifies it, reach for `AsSpan`; until then, `Substring` is fine and clearer. - **Dapper is the sanctioned escape hatch from EF**, not a peer abstraction. Reach for it only when (a) the SQL is provider-specific and doesn't translate cleanly, (b) profiling proves EF is the bottleneck on a hot path, or (c) the query is a SQL aggregation where LINQ obscures intent. Always use `ctx.Database.GetDbConnection()` so Dapper shares the EF connection and any ambient transaction — never open a separate `NpgsqlConnection`/`SqlConnection`. Writes always go through aggregates + EF (Dapper bypasses concurrency tokens, domain validation, and the outbox). Full rationale: [docs/performance-and-data-correctness.md "Decision: when to reach past EF Core (Dapper escape hatch)"](docs/performance-and-data-correctness.md#decision-when-to-reach-past-ef-core-dapper-escape-hatch). ## Testing