#65 — Tech Debt: `TodoListDto` Construction Is Hand-Rolled in Two Query Handlers #65

New issue

Closed

opened 2026-08-18 13:13:25 +02:00 by lena · 1 comment

lena commented

2026-08-18 13:13:25 +02:00

(Migrated from git.butzei.de)

Tech Debt: `TodoListDto` Construction Is Hand-Rolled in Two Query Handlers

Reported by: Backend Engineer, spotted during #62's code review, 2026-07-22

Symptom

GetTodoListQueryHandler.cs and GetTodoListsOfCurrentUserQueryHandler.cs each hand-build a
TodoListDto positionally inside a LINQ .Select(...):

.Select(x => new TodoListDto(
    x.Id,
    x.Title,
    x.Description,
    x.Users.Where(u => u.UserId == userId).Select(u => u.Role).FirstOrDefault(),
    x.IsArchived,
    x.Color,
    x.Icon))

duplicated verbatim across both files. TodoMappings.cs and UserMappings.cs already solve the
equivalent "entity to DTO" projection problem with a Mapperly ProjectToDto(this IQueryable<TEntity> entity) extension (see CqsTodo/Features/Todos/GetTodosOfCurrentUserQueryHandler.cs
for a caller); TodoListMappings.cs has no equivalent, so this is the one remaining place in the
TodoLists feature area without that safety net.

If TodoListDto gains a new optional/defaulted member, both hand-rolled .Select(...)
projections would keep compiling while silently leaving the new field at its constructor default —
the same "silently defaults on the wire" failure mode that #62 fixed for
TodoListDto -> TodoListBroadcastDto, just one layer upstream (entity -> TodoListDto).

Why this wasn't folded into `#62`

#62 was scoped to the TodoListBroadcastDto mapping specifically. TodoListDto.CurrentUserRole
is computed from the requesting user's membership row (x.Users.Where(u => u.UserId == userId)...), which is external, per-request context — not a value derivable from TodoListEntity
alone. A generic ProjectToDto(this IQueryable<TodoListEntity> entities) extension (matching the
TodoMappings/UserMappings precedent exactly) can't compute this field without also taking
userId as a parameter, which is a different shape of Mapperly usage than the existing
precedents (TodoMappings.ProjectToDto and UserMappings.ProjectToDto have no such external
dependency). Whoever picks this up should evaluate whether Mapperly supports a clean way to
express that (e.g. a partial method with an extra parameter, or a [MapProperty] reference to a
helper), or whether the two hand-rolled projections should be de-duplicated a simpler way (e.g. a
shared private static expression) without adopting the ProjectToDto pattern verbatim.

Expected behaviour

Either a Mapperly-based projection (if a clean parameterized [Mapper] shape is found) or a
shared, single hand-written projection expression used by both GetTodoListQueryHandler and
GetTodoListsOfCurrentUserQueryHandler, so a new TodoListDto field only needs to be added in one
place.

Acceptance criteria

The TodoListDto construction in GetTodoListQueryHandler.cs and
GetTodoListsOfCurrentUserQueryHandler.cs is de-duplicated into a single definition
Adding a field to TodoListDto without updating that single definition either fails the
build (preferred, if a Mapperly shape is found) or is at minimum only one place to fix by hand
No behavior change to either query's existing output
A unit test exists asserting the projection's field-for-field shape (see
CqsTodo.Tests/Mappings/TodoListMappingsTests.cs from #62 for the pattern of testing a mapping
directly without Docker/Testcontainers)

Agents involved

Backend Engineer — owns evaluating the Mapperly-shape question and implementing the fix

Blockers

None.

# Tech Debt: `TodoListDto` Construction Is Hand-Rolled in Two Query Handlers **Reported by:** Backend Engineer, spotted during `#62`'s code review, 2026-07-22 ## Symptom `GetTodoListQueryHandler.cs` and `GetTodoListsOfCurrentUserQueryHandler.cs` each hand-build a `TodoListDto` positionally inside a LINQ `.Select(...)`: ```csharp .Select(x => new TodoListDto( x.Id, x.Title, x.Description, x.Users.Where(u => u.UserId == userId).Select(u => u.Role).FirstOrDefault(), x.IsArchived, x.Color, x.Icon)) ``` duplicated verbatim across both files. `TodoMappings.cs` and `UserMappings.cs` already solve the equivalent "entity to DTO" projection problem with a Mapperly `ProjectToDto(this IQueryable<TEntity> entity)` extension (see `CqsTodo/Features/Todos/GetTodosOfCurrentUserQueryHandler.cs` for a caller); `TodoListMappings.cs` has no equivalent, so this is the one remaining place in the `TodoLists` feature area without that safety net. If `TodoListDto` gains a new optional/defaulted member, both hand-rolled `.Select(...)` projections would keep compiling while silently leaving the new field at its constructor default — the same "silently defaults on the wire" failure mode that `#62` fixed for `TodoListDto` -> `TodoListBroadcastDto`, just one layer upstream (entity -> `TodoListDto`). ## Why this wasn't folded into `#62` `#62` was scoped to the `TodoListBroadcastDto` mapping specifically. `TodoListDto.CurrentUserRole` is computed from the requesting user's membership row (`x.Users.Where(u => u.UserId == userId)...`), which is external, per-request context — not a value derivable from `TodoListEntity` alone. A generic `ProjectToDto(this IQueryable<TodoListEntity> entities)` extension (matching the `TodoMappings`/`UserMappings` precedent exactly) can't compute this field without also taking `userId` as a parameter, which is a different shape of Mapperly usage than the existing precedents (`TodoMappings.ProjectToDto` and `UserMappings.ProjectToDto` have no such external dependency). Whoever picks this up should evaluate whether Mapperly supports a clean way to express that (e.g. a partial method with an extra parameter, or a `[MapProperty]` reference to a helper), or whether the two hand-rolled projections should be de-duplicated a simpler way (e.g. a shared private static expression) without adopting the `ProjectToDto` pattern verbatim. ## Expected behaviour Either a Mapperly-based projection (if a clean parameterized `[Mapper]` shape is found) or a shared, single hand-written projection expression used by both `GetTodoListQueryHandler` and `GetTodoListsOfCurrentUserQueryHandler`, so a new `TodoListDto` field only needs to be added in one place. ## Acceptance criteria - [ ] The `TodoListDto` construction in `GetTodoListQueryHandler.cs` and `GetTodoListsOfCurrentUserQueryHandler.cs` is de-duplicated into a single definition - [ ] Adding a field to `TodoListDto` without updating that single definition either fails the build (preferred, if a Mapperly shape is found) or is at minimum only one place to fix by hand - [ ] No behavior change to either query's existing output - [ ] A unit test exists asserting the projection's field-for-field shape (see `CqsTodo.Tests/Mappings/TodoListMappingsTests.cs` from `#62` for the pattern of testing a mapping directly without Docker/Testcontainers) ## Agents involved - **Backend Engineer** — owns evaluating the Mapperly-shape question and implementing the fix ## Blockers None.

lena commented

2026-08-18 13:13:25 +02:00

(Migrated from git.butzei.de)

design (65_todolist_dto_projection_hand_rolled_design.md)

`#65` — Design Note

Decision: hand-written shared expression, not a Mapperly `ProjectToDto`

Evaluated the Mapperly-based approach the story's own "why this wasn't folded into #62" section
raised as an open question. TodoMappings.ProjectToDto/UserMappings.ProjectToDto's existing
precedent is parameterless (this IQueryable<TEntity> entity) because none of their target DTOs
need external, per-request context. TodoListDto.CurrentUserRole/CurrentUserEmailNotificationMode
both need the requesting userId, which TodoListEntity alone doesn't carry — Mapperly can
support additional mapping-method parameters, but confirming it generates a correct,
EF-translatable expression for a member that isn't a direct property copy (it needs a
.Where(u => u.UserId == userId).Select(...).FirstOrDefault() sub-query referencing the extra
parameter) isn't something this sandbox can verify empirically — no Docker/Postgres available to
confirm the generated SQL actually works, and Mapperly's exact codegen behavior for this shape
wasn't something to guess at for a live per-request query path.

Chose the story's own explicitly-offered fallback instead: a single hand-written
TodoListMappings.ProjectToDtoFor(UserId? userId) : Expression<Func<TodoListEntity, TodoListDto>>,
identical in body to the two (now-deleted) inline lambdas — userId is still just a captured
closure variable, exactly as it was when each handler captured its own local variable inline;
only the lambda's declaration site moved from inline-in-the-handler to a shared static method's
return statement. EF Core's expression-tree translation doesn't care where a captured variable's
closure originates, only the final Expression<Func<TSource,TResult>> shape once
.Select(...) receives it — structurally unchanged from before.

What this satisfies vs. the story's stated AC

"De-duplicated into a single definition" — yes, one method, two call sites.
"Fails the build (preferred) if a new field is added without updating it" — no; a hand-written
expression doesn't get Mapperly's RequiredMappingStrategy.Source compile-time enforcement.
Falls back to the AC's explicitly-allowed second option instead: "at minimum only one place to
fix by hand" — satisfied, since a new TodoListDto field now only needs ProjectToDtoFor
updated once, not two near-identical .Select(...) bodies kept in sync by hand.
"No behavior change to either query's existing output" — yes, pure extraction, no logic change.
"A unit test exists asserting the projection's field-for-field shape" — yes, 3 new tests in
TodoListMappingsTests.cs, following the exact .Compile()-and-invoke-against-an-in-memory-
entity pattern that file's existing tests (from #62) already established for testing a mapping
without Docker/Testcontainers. All 3 run and pass locally (pure C#, no DB needed).

Pre-existing quirk, unchanged by this refactor (noted, not fixed)

TodoListUserRole.Owner is ordinal 0, so .FirstOrDefault() on a no-membership-row case
returns default(TodoListUserRole) == Owner, not a genuinely neutral "no role" value — a
non-member could, in principle, see CurrentUserRole: Owner on a list they don't belong to.
Verified this is identical to the original hand-rolled code (not introduced by this refactor) and
that it's unreachable in practice: both call sites are gated by authorization
(AuthorizeTodoListAccessForCurrentUserQuery/AuthorizeIsCurrentUserAuthenticatedQuery +
.Where(x => x.Users.Any(u => u.UserId == userId))) that already excludes non-members before this
projection ever runs. Not worth a follow-up ticket given it's already unreachable under existing
authorization, but noted here in case a future caller of this projection method skips that gate.

Verification boundary (honest limitation)

Reasoned rather than empirically confirmed that EF Core translates the shared expression
identically to the original inline lambdas — no Docker/Postgres available in this sandbox to run
the actual query against real Postgres and inspect the generated SQL. The transformation is
low-risk (pure syntactic extraction of an already-correct, already-shipped expression, not new
logic), unlike #63's decorator-wiring change earlier this session which did require live
verification to catch two real bugs. The full backend test suite (307 pre-existing
Docker-dependent failures, unchanged; 56 passing, up from 53 with the 3 new tests) shows no
regression in anything that can run locally.

**design** (`65_todolist_dto_projection_hand_rolled_design.md`) # `#65` — Design Note ## Decision: hand-written shared expression, not a Mapperly `ProjectToDto` Evaluated the Mapperly-based approach the story's own "why this wasn't folded into `#62`" section raised as an open question. `TodoMappings.ProjectToDto`/`UserMappings.ProjectToDto`'s existing precedent is parameterless (`this IQueryable<TEntity> entity`) because none of their target DTOs need external, per-request context. `TodoListDto.CurrentUserRole`/`CurrentUserEmailNotificationMode` both need the requesting `userId`, which `TodoListEntity` alone doesn't carry — Mapperly can support additional mapping-method parameters, but confirming it generates a *correct, EF-translatable* expression for a member that isn't a direct property copy (it needs a `.Where(u => u.UserId == userId).Select(...).FirstOrDefault()` sub-query referencing the extra parameter) isn't something this sandbox can verify empirically — no Docker/Postgres available to confirm the generated SQL actually works, and Mapperly's exact codegen behavior for this shape wasn't something to guess at for a live per-request query path. Chose the story's own explicitly-offered fallback instead: a single hand-written `TodoListMappings.ProjectToDtoFor(UserId? userId) : Expression<Func<TodoListEntity, TodoListDto>>`, identical in body to the two (now-deleted) inline lambdas — `userId` is still just a captured closure variable, exactly as it was when each handler captured its own local variable inline; only the *lambda's declaration site* moved from inline-in-the-handler to a shared static method's return statement. EF Core's expression-tree translation doesn't care where a captured variable's closure originates, only the final `Expression<Func<TSource,TResult>>` shape once `.Select(...)` receives it — structurally unchanged from before. ## What this satisfies vs. the story's stated AC - "De-duplicated into a single definition" — yes, one method, two call sites. - "Fails the build (preferred) if a new field is added without updating it" — no; a hand-written expression doesn't get Mapperly's `RequiredMappingStrategy.Source` compile-time enforcement. Falls back to the AC's explicitly-allowed second option instead: "at minimum only one place to fix by hand" — satisfied, since a new `TodoListDto` field now only needs `ProjectToDtoFor` updated once, not two near-identical `.Select(...)` bodies kept in sync by hand. - "No behavior change to either query's existing output" — yes, pure extraction, no logic change. - "A unit test exists asserting the projection's field-for-field shape" — yes, 3 new tests in `TodoListMappingsTests.cs`, following the exact `.Compile()`-and-invoke-against-an-in-memory- entity pattern that file's existing tests (from `#62`) already established for testing a mapping without Docker/Testcontainers. All 3 run and pass locally (pure C#, no DB needed). ## Pre-existing quirk, unchanged by this refactor (noted, not fixed) `TodoListUserRole.Owner` is ordinal `0`, so `.FirstOrDefault()` on a no-membership-row case returns `default(TodoListUserRole)` == `Owner`, not a genuinely neutral "no role" value — a non-member could, in principle, see `CurrentUserRole: Owner` on a list they don't belong to. Verified this is identical to the original hand-rolled code (not introduced by this refactor) and that it's unreachable in practice: both call sites are gated by authorization (`AuthorizeTodoListAccessForCurrentUserQuery`/`AuthorizeIsCurrentUserAuthenticatedQuery` + `.Where(x => x.Users.Any(u => u.UserId == userId))`) that already excludes non-members before this projection ever runs. Not worth a follow-up ticket given it's already unreachable under existing authorization, but noted here in case a future caller of this projection method skips that gate. ## Verification boundary (honest limitation) Reasoned rather than empirically confirmed that EF Core translates the shared expression identically to the original inline lambdas — no Docker/Postgres available in this sandbox to run the actual query against real Postgres and inspect the generated SQL. The transformation is low-risk (pure syntactic extraction of an already-correct, already-shipped expression, not new logic), unlike `#63`'s decorator-wiring change earlier this session which did require live verification to catch two real bugs. The full backend test suite (307 pre-existing Docker-dependent failures, unchanged; 56 passing, up from 53 with the 3 new tests) shows no regression in anything that *can* run locally.