fix(comment): declare missing @PathVariable params on block comment endpoints #489

marcel · 2026-05-09T14:53:07+02:00

marcel commented

2026-05-09 14:53:07 +02:00

Closes #473

Problem

Two endpoints in CommentController had path variables declared in the URL template that were not bound to method parameters:

GET /api/documents/{documentId}/transcription-blocks/{blockId}/comments — documentId was in the URL but not declared as a @PathVariable, so Spring silently ignored it (any string was accepted)
POST /api/documents/{documentId}/transcription-blocks/{blockId}/comments/{commentId}/replies — blockId was missing; only documentId and commentId were bound

Spring MVC silently ignores undeclared path variables — it extracts them but discards the segment without type-checking. This means a non-UUID documentId or blockId would not produce a 400, and SpringDoc would emit an OpenAPI spec with a dangling path parameter.

Fix

Added the missing @PathVariable UUID documentId to getBlockComments and @PathVariable UUID blockId to replyToBlockComment. No logic changes — the parameters were already being passed correctly to the service layer; they just weren't declared for the endpoints that were missing them.

Tests

Added two behavioral RED→GREEN tests in CommentControllerTest:

getBlockComments_returns400_when_documentId_is_not_a_UUID — confirmed 200 before fix, 400 after
replyToBlockComment_returns400_when_blockId_is_not_a_UUID — confirmed 201 before fix, 400 after

Closes #473 ## Problem Two endpoints in `CommentController` had path variables declared in the URL template that were not bound to method parameters: - `GET /api/documents/{documentId}/transcription-blocks/{blockId}/comments` — `documentId` was in the URL but not declared as a `@PathVariable`, so Spring silently ignored it (any string was accepted) - `POST /api/documents/{documentId}/transcription-blocks/{blockId}/comments/{commentId}/replies` — `blockId` was missing; only `documentId` and `commentId` were bound Spring MVC silently ignores undeclared path variables — it extracts them but discards the segment without type-checking. This means a non-UUID `documentId` or `blockId` would not produce a 400, and SpringDoc would emit an OpenAPI spec with a dangling path parameter. ## Fix Added the missing `@PathVariable UUID documentId` to `getBlockComments` and `@PathVariable UUID blockId` to `replyToBlockComment`. No logic changes — the parameters were already being passed correctly to the service layer; they just weren't declared for the endpoints that were missing them. ## Tests Added two behavioral RED→GREEN tests in `CommentControllerTest`: - `getBlockComments_returns400_when_documentId_is_not_a_UUID` — confirmed 200 before fix, 400 after - `replyToBlockComment_returns400_when_blockId_is_not_a_UUID` — confirmed 201 before fix, 400 after

marcel added 1 commit 2026-05-09 14:53:08 +02:00

fix(comment): declare missing @PathVariable params on block comment endpoints

CI / Unit & Component Tests (push) Failing after 3m58s

Details

CI / OCR Service Tests (push) Successful in 49s

Details

CI / Backend Unit Tests (push) Failing after 3m19s

Details

CI / Unit & Component Tests (pull_request) Failing after 3m58s

Details

CI / OCR Service Tests (pull_request) Successful in 46s

Details

CI / Backend Unit Tests (pull_request) Failing after 3m25s

Details

fb8062a6c2

getBlockComments was missing documentId; replyToBlockComment was missing
blockId. Spring silently ignored undeclared path variables — the segments
were parsed but never bound. Now both parameters are explicitly declared so
Spring rejects non-UUID values with 400.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-05-09 14:54:35 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

TDD Evidence

Red/green discipline is solid. Both tests confirmed RED with the right failure messages (Status expected:<400> but was:<200> and Status expected:<400> but was:<201>) before the fix. That's exactly what you want — proof the test would have caught a real regression.

Code Quality

The fix is surgical and minimal — exactly what a bug fix should be. No logic changes, no refactoring, no feature creep.

One thing worth noting explicitly: documentId is now bound in getBlockComments but not forwarded to the service — the service only needs blockId to fetch block comments. This is intentional and correct. The parameter must be declared so Spring performs UUID type conversion (and rejects invalid values with 400), even if the value isn't used downstream. A future reader might question it — the PR description already explains this well, which is the right place for it.

Suggestions

None. This is a clean, focused fix with proper TDD evidence. Ship it.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** ### TDD Evidence Red/green discipline is solid. Both tests confirmed RED with the right failure messages (`Status expected:<400> but was:<200>` and `Status expected:<400> but was:<201>`) before the fix. That's exactly what you want — proof the test would have caught a real regression. ### Code Quality The fix is surgical and minimal — exactly what a bug fix should be. No logic changes, no refactoring, no feature creep. One thing worth noting explicitly: `documentId` is now bound in `getBlockComments` but not forwarded to the service — the service only needs `blockId` to fetch block comments. This is intentional and correct. The parameter must be declared so Spring performs UUID type conversion (and rejects invalid values with 400), even if the value isn't used downstream. A future reader might question it — the PR description already explains this well, which is the right place for it. ### Suggestions None. This is a clean, focused fix with proper TDD evidence. Ship it.

marcel commented

2026-05-09 14:54:46 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

What Was Fixed and Why It Matters

The root issue is CWE-20: Improper Input Validation. When a path variable appears in the URL template but isn't declared as a @PathVariable parameter, Spring MVC silently discards it — no type coercion, no validation. That means a caller could send any string in the documentId or blockId position and Spring would happily route the request to the handler.

In isolation this isn't a high-severity finding (no IDOR risk here since the service does its own lookup by blockId, and the endpoint is authenticated), but:

It breaks the OpenAPI spec — SpringDoc emits a dangling path parameter declaration
It weakens defence-in-depth — UUID validation is a cheap gate at the HTTP boundary
It's a correctness bug masquerading as a "just an annotation" issue

The fix correctly enforces UUID parsing at the Spring MVC dispatcher level. Non-UUID values now fail with 400 before the handler body executes.

Pre-existing Observation (not introduced by this PR)

getBlockComments has no @RequirePermission annotation — any authenticated user can read block comments for any block. postBlockComment requires ANNOTATE_ALL or WRITE_ALL. This asymmetry appears intentional (read = any authenticated user, write = privileged), but worth a quick confirmation that this is a deliberate product decision, not an overlooked annotation.

No New Concerns

Auth annotations on all write endpoints are correct. No unparameterised queries introduced. No logging of user-controlled input. LGTM.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** ### What Was Fixed and Why It Matters The root issue is **CWE-20: Improper Input Validation**. When a path variable appears in the URL template but isn't declared as a `@PathVariable` parameter, Spring MVC silently discards it — no type coercion, no validation. That means a caller could send any string in the `documentId` or `blockId` position and Spring would happily route the request to the handler. In isolation this isn't a high-severity finding (no IDOR risk here since the service does its own lookup by `blockId`, and the endpoint is authenticated), but: 1. It breaks the OpenAPI spec — SpringDoc emits a dangling path parameter declaration 2. It weakens defence-in-depth — UUID validation is a cheap gate at the HTTP boundary 3. It's a correctness bug masquerading as a "just an annotation" issue The fix correctly enforces UUID parsing at the Spring MVC dispatcher level. Non-UUID values now fail with 400 before the handler body executes. ### Pre-existing Observation (not introduced by this PR) `getBlockComments` has no `@RequirePermission` annotation — any authenticated user can read block comments for any block. `postBlockComment` requires `ANNOTATE_ALL` or `WRITE_ALL`. This asymmetry appears intentional (read = any authenticated user, write = privileged), but worth a quick confirmation that this is a deliberate product decision, not an overlooked annotation. ### No New Concerns Auth annotations on all write endpoints are correct. No unparameterised queries introduced. No logging of user-controlled input. LGTM.

marcel commented

2026-05-09 14:54:54 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

Architecture Check

Layer boundaries: Controller delegates to service, service is unchanged. No layer violations.
Domain boundary: The fix stays entirely within document.comment. No cross-domain reach.
Module ownership: CommentController owns its path variable declarations. The service's public API is untouched.
Database/schema: No migrations, no schema changes.

Documentation Check

Per my review table: this PR contains no new Flyway migrations, no new packages or domain modules, no new routes, no new Docker services, no new ErrorCode or Permission values, and no new external integrations. No doc updates are required for a bug fix of this scope.

Nothing to Flag

Clean, contained fix. The approach of declaring path variables explicitly (even when the controller method doesn't use the value downstream) is the correct Spring MVC idiom — it's how SpringDoc generates accurate parameter specs and how the dispatcher enforces type coercion before the handler executes. No architectural concerns.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** ### Architecture Check - **Layer boundaries**: Controller delegates to service, service is unchanged. No layer violations. - **Domain boundary**: The fix stays entirely within `document.comment`. No cross-domain reach. - **Module ownership**: `CommentController` owns its path variable declarations. The service's public API is untouched. - **Database/schema**: No migrations, no schema changes. ### Documentation Check Per my review table: this PR contains no new Flyway migrations, no new packages or domain modules, no new routes, no new Docker services, no new ErrorCode or Permission values, and no new external integrations. No doc updates are required for a bug fix of this scope. ### Nothing to Flag Clean, contained fix. The approach of declaring path variables explicitly (even when the controller method doesn't use the value downstream) is the correct Spring MVC idiom — it's how SpringDoc generates accurate parameter specs and how the dispatcher enforces type coercion before the handler executes. No architectural concerns.

marcel commented

2026-05-09 14:55:02 +02:00

🧪 Sara Holt — Senior QA Engineer

Verdict: ✅ Approved

Test Coverage Assessment

RED/GREEN evidence: Both tests confirmed RED before the fix:

getBlockComments_returns400_when_documentId_is_not_a_UUID: was 200, expected 400 ✓
replyToBlockComment_returns400_when_blockId_is_not_a_UUID: was 201, expected 400 ✓

Test naming: Descriptive and readable as sentences — passes my naming check.

Authority setup: @WithMockUser(authorities = "ANNOTATE_ALL") on the reply test is correct. Without it, the AOP permission check would return 403 before Spring even tries to bind the path variable, making the test meaningless for this specific behavior.

Test targeting: Each test targets precisely the missing parameter for its endpoint:

getBlockComments was missing documentId → test sends non-UUID documentId ✓
replyToBlockComment was missing blockId → test sends non-UUID blockId ✓

Test layer: @WebMvcTest slice is the right choice here — we're testing Spring MVC path variable binding behavior, which lives in the web layer. No database, no Testcontainers needed.

Minor Observation

The getBlockComments test uses a freshly generated blockId UUID that has no stub — commentService.getCommentsForBlock(blockId) would return null by default if Spring ever reached the handler. But since the test expects 400 (handler should never be reached), this is fine. The test would fail with a 200/NPE if the fix regressed, which is the desired behavior.

Full Suite

All 16 tests in CommentControllerTest pass. No regressions. LGTM.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ✅ Approved** ### Test Coverage Assessment **RED/GREEN evidence**: Both tests confirmed RED before the fix: - `getBlockComments_returns400_when_documentId_is_not_a_UUID`: was 200, expected 400 ✓ - `replyToBlockComment_returns400_when_blockId_is_not_a_UUID`: was 201, expected 400 ✓ **Test naming**: Descriptive and readable as sentences — passes my naming check. **Authority setup**: `@WithMockUser(authorities = "ANNOTATE_ALL")` on the reply test is correct. Without it, the AOP permission check would return 403 before Spring even tries to bind the path variable, making the test meaningless for this specific behavior. **Test targeting**: Each test targets precisely the missing parameter for its endpoint: - `getBlockComments` was missing `documentId` → test sends non-UUID `documentId` ✓ - `replyToBlockComment` was missing `blockId` → test sends non-UUID `blockId` ✓ **Test layer**: `@WebMvcTest` slice is the right choice here — we're testing Spring MVC path variable binding behavior, which lives in the web layer. No database, no Testcontainers needed. ### Minor Observation The `getBlockComments` test uses a freshly generated `blockId` UUID that has no stub — `commentService.getCommentsForBlock(blockId)` would return null by default if Spring ever reached the handler. But since the test expects 400 (handler should never be reached), this is fine. The test would fail with a 200/NPE if the fix regressed, which is the desired behavior. ### Full Suite All 16 tests in `CommentControllerTest` pass. No regressions. LGTM.

marcel commented

2026-05-09 14:55:06 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

No infrastructure, CI, Docker Compose, or deployment changes in this PR. Pure backend bug fix — two Java files changed.

The backend JAR builds and tests pass cleanly, which is all I need to confirm from a platform perspective. Nothing to flag.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** No infrastructure, CI, Docker Compose, or deployment changes in this PR. Pure backend bug fix — two Java files changed. The backend JAR builds and tests pass cleanly, which is all I need to confirm from a platform perspective. Nothing to flag.

marcel commented

2026-05-09 14:55:11 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Requirements Traceability

Issue #473 described a specific correctness gap: path variables declared in URL templates but not bound to handler parameters. The fix delivers exactly what was specified — no more, no less. There is no scope creep.

Acceptance criteria (implicit in the issue):

✅ GET /api/documents/{documentId}/transcription-blocks/{blockId}/comments rejects non-UUID documentId with 400
✅ POST /api/documents/{documentId}/transcription-blocks/{blockId}/comments/{commentId}/replies rejects non-UUID blockId with 400

Both are verified by the regression tests.

No NFR Concerns

This fix improves input validation (a security NFR) and OpenAPI spec correctness (a developer-experience NFR) with zero impact on performance, availability, or user-facing behavior for valid inputs. Existing callers passing valid UUIDs are unaffected.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** ### Requirements Traceability Issue #473 described a specific correctness gap: path variables declared in URL templates but not bound to handler parameters. The fix delivers exactly what was specified — no more, no less. There is no scope creep. **Acceptance criteria** (implicit in the issue): - ✅ `GET /api/documents/{documentId}/transcription-blocks/{blockId}/comments` rejects non-UUID `documentId` with 400 - ✅ `POST /api/documents/{documentId}/transcription-blocks/{blockId}/comments/{commentId}/replies` rejects non-UUID `blockId` with 400 Both are verified by the regression tests. ### No NFR Concerns This fix improves input validation (a security NFR) and OpenAPI spec correctness (a developer-experience NFR) with zero impact on performance, availability, or user-facing behavior for valid inputs. Existing callers passing valid UUIDs are unaffected.

marcel commented

2026-05-09 14:55:15 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

Backend-only fix — no Svelte components, no routes, no HTML, no CSS, no i18n strings. Nothing in this PR touches the UI layer.

The 400 error response for malformed UUIDs is handled by Spring's global exception handler, which maps to a structured error response. The frontend already handles API errors through getErrorMessage(). No UX impact to review.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** Backend-only fix — no Svelte components, no routes, no HTML, no CSS, no i18n strings. Nothing in this PR touches the UI layer. The 400 error response for malformed UUIDs is handled by Spring's global exception handler, which maps to a structured error response. The frontend already handles API errors through `getErrorMessage()`. No UX impact to review.

marcel merged commit 265b4f1484 into main

2026-05-09 15:46:09 +02:00

marcel deleted branch fix/issue-473-pathvariable-names

2026-05-09 15:46:11 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#489