Split the 28-method AppRepository god-trait along its aggregates #30

New issue

Closed

opened 2026-07-01 14:49:31 +00:00 by rosa · 1 comment

rosa commented

2026-07-01 14:49:31 +00:00

Owner

Problem

AppRepository (crates/domain/src/ports.rs:24-159) is a single trait spanning five unrelated aggregates — Users, Confirmations, Posts, Access Tokens, Authorization Codes — with 28 methods. It is deep per method (each Postgres impl hides real SQL), but wide as an interface: a caller or test author must learn, or stub, all 28 to work with any one.

The cost lands hardest on tests. A service test that only creates a Post still instantiates a MemoryAppRepo (crates/domain/src/mocks.rs, 604 lines) carrying users, indexes, confirmations, tokens, and codes. There is no way to say "this behaviour depends only on the Post store." And AppService is generic over exactly one R: AppRepository, so its type signature claims a dependency on all five aggregates even for the authorization flow, which never touches a Post.

Deletion test (sibling rule): splitting isn't a deletion, so apply "one adapter is a hypothetical seam, two is a real one." Here it inverts: there are two adapters but one seam bundling five independent responsibilities that never vary together. That bundling is the smell.

Solution

Split the god-trait along the aggregates already named in CONTEXT.md:

UserRepository
PostRepository
TokenRepository (access tokens + authorization codes)
ConfirmationRepository

PostgresAppRepository keeps implementing all of them (nothing changes at the infra seam); each service method depends on only the trait it needs. Mocks shrink to the surface a given test exercises.

Benefits

Leverage / navigability: a reader of the auth flow sees it needs TokenRepository + UserRepository — the type states the real dependency surface instead of "everything."
Testability: a Post test stubs ~7 methods, not 28; a focused fake becomes a large-adapter / small-implementation the compiler helps keep minimal.
Locality of change: adding a token query stops widening the interface every existing test must satisfy.

Counter-weight (grill before committing)

Vernier is deliberately single-context (one CONTEXT.md, one binary), and one wide trait keeps wiring trivial. Splitting adds trait bounds to AppService. Grill whether the test-ergonomics win beats that cost — this may be a "later, when a second Post adapter or a read-model appears" call rather than now.

Architecture review candidate 3 of 4 (Worth exploring). Sequence after candidates 1 and 2.

## Problem `AppRepository` (`crates/domain/src/ports.rs:24-159`) is a single trait spanning five unrelated aggregates — Users, Confirmations, Posts, Access Tokens, Authorization Codes — with 28 methods. It is *deep per method* (each Postgres impl hides real SQL), but **wide as an interface**: a caller or test author must learn, or stub, all 28 to work with any one. The cost lands hardest on tests. A service test that only creates a Post still instantiates a `MemoryAppRepo` (`crates/domain/src/mocks.rs`, 604 lines) carrying users, indexes, confirmations, tokens, and codes. There is no way to say "this behaviour depends only on the Post store." And `AppService` is generic over exactly one `R: AppRepository`, so its type signature claims a dependency on all five aggregates even for the authorization flow, which never touches a Post. **Deletion test (sibling rule):** splitting isn't a deletion, so apply "one adapter is a hypothetical seam, two is a real one." Here it inverts: there are two adapters but **one seam bundling five independent responsibilities** that never vary together. That bundling is the smell. ## Solution Split the god-trait along the aggregates already named in `CONTEXT.md`: - `UserRepository` - `PostRepository` - `TokenRepository` (access tokens + authorization codes) - `ConfirmationRepository` `PostgresAppRepository` keeps implementing all of them (nothing changes at the infra seam); each service method depends on only the trait it needs. Mocks shrink to the surface a given test exercises. ## Benefits - **Leverage / navigability:** a reader of the auth flow sees it needs `TokenRepository + UserRepository` — the type states the real dependency surface instead of "everything." - **Testability:** a Post test stubs ~7 methods, not 28; a focused fake becomes a large-adapter / small-implementation the compiler helps keep minimal. - **Locality of change:** adding a token query stops widening the interface every existing test must satisfy. ## Counter-weight (grill before committing) Vernier is deliberately single-context (one `CONTEXT.md`, one binary), and one wide trait keeps wiring trivial. Splitting adds trait bounds to `AppService`. Grill whether the test-ergonomics win beats that cost — this may be a "later, when a second Post adapter or a read-model appears" call rather than now. _Architecture review candidate 3 of 4 (Worth exploring). Sequence after candidates 1 and 2._

rosa added the

wontfix

label

2026-07-01 16:28:57 +00:00

rosa commented

2026-07-01 16:28:57 +00:00

Author

Owner

Resolution: deferring — the premise doesn't hold yet

Grilled the split before committing. Three findings, each undercutting a stated benefit:

1. The real boundary is 3-way, not 4-way. Confirmation isn't an independent
aggregate — it's a subordinate entity in the user/identity lifecycle, and CONTEXT.md
never names it as an aggregate. Every confirmation write happens atomically inside a
user-lifecycle transaction:

create_user writes users + confirmations in one transaction
(postgres.rs:274-327).
update_user_password writes users + confirmations + sessions/user_sessions
in one transaction (postgres.rs:618-659).

A standalone ConfirmationRepository would only create a "which trait owns this
cross-aggregate transaction?" problem. If we ever split, it's User (incl.
confirmations) / Post / Token.

2. A single AppService delivers no leverage/navigability benefit. AppService
touches all three aggregate groups, so its bound becomes the union
UserRepository + PostRepository + TokenRepository — the signature still advertises
"everything." The headline "a reader of the auth flow sees it needs TokenRepository + UserRepository" is only delivered by also decomposing AppService, which this issue
explicitly scopes out.

3. The testability benefit is illusory in this codebase. The issue assumes per-test
stubbing ("a Post test stubs ~7 methods, not 28"). But there are exactly two
impl AppRepository blocks — PostgresAppRepository and one shared real in-memory
adapter MemoryAppRepo — and all 33 test construction sites just call
MemoryAppRepo::new(). No test stubs anything; a Post-only test pays zero incremental
cost for the machinery it never calls. Splitting the trait shrinks no existing test —
it only enables a hypothetical focused fake (MemoryPostRepo) that nothing needs. The
issue's own deletion-test rule actually cuts against the split here: two adapters over
one seam, where the fake is a real implementation, not a stub, so the fake-surface pain
the argument assumes never materializes.

Revisit when a genuine second Post adapter, a read-model, or a caching decorator
lands — that's the real driver that would make the seam earn its keep. Until then the
one wide trait keeps the wiring trivial (the counter-weight the issue itself raised).

Full write-up: thoughts/shared/research/004_split_apprepository_god_trait.md.

## Resolution: deferring — the premise doesn't hold yet Grilled the split before committing. Three findings, each undercutting a stated benefit: **1. The real boundary is 3-way, not 4-way.** Confirmation isn't an independent aggregate — it's a subordinate entity in the user/identity lifecycle, and CONTEXT.md never names it as an aggregate. Every confirmation write happens *atomically inside* a user-lifecycle transaction: - `create_user` writes `users` + `confirmations` in one transaction (`postgres.rs:274-327`). - `update_user_password` writes `users` + `confirmations` + `sessions`/`user_sessions` in one transaction (`postgres.rs:618-659`). A standalone `ConfirmationRepository` would only create a "which trait owns this cross-aggregate transaction?" problem. If we ever split, it's User (incl. confirmations) / Post / Token. **2. A single `AppService` delivers no leverage/navigability benefit.** `AppService` touches all three aggregate groups, so its bound becomes the *union* `UserRepository + PostRepository + TokenRepository` — the signature still advertises "everything." The headline "a reader of the auth flow sees it needs `TokenRepository + UserRepository`" is only delivered by *also* decomposing `AppService`, which this issue explicitly scopes out. **3. The testability benefit is illusory in this codebase.** The issue assumes per-test stubbing ("a Post test stubs ~7 methods, not 28"). But there are exactly two `impl AppRepository` blocks — `PostgresAppRepository` and one shared *real* in-memory adapter `MemoryAppRepo` — and all 33 test construction sites just call `MemoryAppRepo::new()`. No test stubs anything; a Post-only test pays zero incremental cost for the machinery it never calls. Splitting the trait shrinks no existing test — it only enables a hypothetical focused fake (`MemoryPostRepo`) that nothing needs. The issue's own deletion-test rule actually cuts against the split here: two adapters over one seam, where the fake is a real implementation, not a stub, so the fake-surface pain the argument assumes never materializes. **Revisit when** a genuine second Post adapter, a read-model, or a caching decorator lands — that's the real driver that would make the seam earn its keep. Until then the one wide trait keeps the wiring trivial (the counter-weight the issue itself raised). Full write-up: `thoughts/shared/research/004_split_apprepository_god_trait.md`.

rosa closed this issue

2026-07-01 16:28:57 +00:00

rosa referenced this issue from a commit

2026-07-01 16:32:48 +00:00

docs: Record AppRepository split research and its supersession