112 lines
6.9 KiB
Markdown
112 lines
6.9 KiB
Markdown
# Report: Code-Review-Fixes — Kritische und hohe Befunde
|
||
|
||
## Status: DONE
|
||
|
||
Alle 3 kritischen und 5 hohen Befunde wurden implementiert, getestet und gemerged. Build und Tests gruen (313/313). Post-Merge-Review ergab 2 wichtige und 1 minor Empfehlung (nicht blockierend).
|
||
|
||
---
|
||
|
||
## What Was Built
|
||
|
||
8 Code-Review-Befunde im EEG Portal Backend behoben — 3 Sicherheitsfixes (KRITISCH) und 5 Code-Qualitäts-Verbesserungen (HOCH):
|
||
|
||
| # | Schwere | Befund | Dateien |
|
||
|---|---------|--------|---------|
|
||
| 1 | KRITISCH | `EnergyCommunityAdminController` — `@PreAuthorize("hasRole('ADMIN')")` auf Klassenebene | `EnergyCommunityAdminController.java` |
|
||
| 2 | KRITISCH | `NotificationService.markAsRead` — IDOR-Fix: `@CurrentUserId` + Ownership-Check | `NotificationController.java`, `NotificationService.java` |
|
||
| 3 | KRITISCH | `GlobalExceptionHandler` — `MethodArgumentNotValidException`-Handler (400 mit komma-separierten Feldfehlern) | `GlobalExceptionHandler.java` |
|
||
| 4 | HOCH | Exception-Unification — `ResponseStatusException` durch `IllegalArgumentException`/`IllegalStateException` ersetzt | `AdminIamService.java`, `EnergyCommunityService.java` |
|
||
| 5 | HOCH | `TariffService` — `validateUserTariffRequest()` private Methode extrahiert (~30 Zeilen Deduplizierung) | `TariffService.java` |
|
||
| 6 | HOCH | `AtNumberAlreadyExistsException`-Handler — `ex.getMessage()` statt hardcoded String | `GlobalExceptionHandler.java` |
|
||
| 7 | HOCH | `MeteringPoint.memberships` — `orphanRemoval = true` hinzugefuegt | `MeteringPoint.java` |
|
||
| 8 | HOCH | `ddl-auto` Prod-Default — von `update` auf `validate` geaendert | `application-prod.yml` |
|
||
|
||
**Tests neu/hinzugefuegt:**
|
||
- `GlobalExceptionHandlerTest` — 8 Tests fuer alle Handler-Pfade
|
||
- `NotificationServiceTest` — 3 neue Tests fuer Ownership-Check und Not-Found
|
||
- `AdminIamServiceTest` — rejectUser-Tests aktualisiert (Exception-Typen)
|
||
- `EnergyCommunityServiceTest` — Exception-Typen angepasst
|
||
|
||
---
|
||
|
||
## Architecture
|
||
|
||
Die Aenderungen betreffen ausschliesslich das Backend (Java/Spring Boot). Die Architektur-Aenderungen:
|
||
|
||
1. **Exception-Strategie vereinheitlicht:** Services werfen `IllegalArgumentException` (→ 400) oder `IllegalStateException` (→ 409). `GlobalExceptionHandler` mapped diese konsistent. `ResponseStatusException` wird nur noch in Controllern verwendet (nicht in Services).
|
||
|
||
2. **Authorization auf Klassenebene:** `EnergyCommunityAdminController` erhaelt `@PreAuthorize("hasRole('ADMIN')")` — konsistent mit `AdminIamController`.
|
||
|
||
3. **IDOR-Verhinderung:** `NotificationController.markAsRead` erhaelt `@CurrentUserId` Parameter. Service prueft Ownership vor Statusaenderung.
|
||
|
||
4. **Validierung konsolidiert:** `TariffService.validateUserTariffRequest()` bündelt 6 Validierungspruefungen (source/target ACTIVE, Community-Tarif vorhanden, Preis <= Max, beide User aktive Members).
|
||
|
||
---
|
||
|
||
## Design Decisions
|
||
|
||
1. **`@PreAuthorize` auf Klassenebene (nicht pro Methode):** Konsistent mit `AdminIamController`. Vorteil: neue Endpunkte sind automatisch gesichert. Nachteil: `@PreAuthorize("permitAll()")` noetig fuer einzelne Methoden, falls gewuenscht (hier nicht der Fall).
|
||
|
||
2. **`validateUserTariffRequest()` return: `void` statt `Pair`:** Die geladenen MeteringPoints werden fuer den Ownership-Check in `updateUserTariff` nicht gebraucht (Ownership basiert auf `tariff.getSourceMeteringPointId()`). `void` bleibt simpel; die redundanten DB-Queries (Review-Finding) sind ein bekanntes, toleriertes Trade-off.
|
||
|
||
3. **`orphanRemoval = true` defensive:** Kein aktueller Delete-Endpoint, aber das Flag verhindert konsistente DB-Zustaende bei zukuenftigen Loesch-Workflows.
|
||
|
||
4. **`ddl-auto: validate` statt `none`:** `validate` prueft Schema-Konsistenz beim Start, ohne es zu aendern. Kompromiss zwischen `update` (Schema-aendernd) und `none` (keine Pruefung).
|
||
|
||
5. **Post-Merge-Review-Findings (nicht blockierend):**
|
||
- `TariffService` laedt MeteringPoints doppelt (in `validateUserTariffRequest` + danach fuer Ownership/Invite-Check). Redundant aber korrekt — Refactoring auf `Pair`-Rueckgabe empfohlen, aber nicht kritisch.
|
||
- `TariffService` verwendet fully-qualified `org.springframework.security.access.AccessDeniedException` statt Import. Konsistenz-Problem, nicht funktional.
|
||
|
||
---
|
||
|
||
## Usage
|
||
|
||
Kein Nutzungs-Interface-Aenderung. Die Aenderungen sind intern:
|
||
|
||
- `@Valid`-Fehler geben jetzt konsistentes `ErrorResponse`-JSON zurueck (statt Spring-Default)
|
||
- Admin-Endpoints verlangen explizit `ADMIN`-Rolle (vorher: nur URL-basiert gesichert)
|
||
- `Notification.markAsRead` verlangt Besitz der Notification
|
||
- Exception-Messages aus `GlobalExceptionHandler` sind informativer (enthalten AT-Nummer etc.)
|
||
|
||
---
|
||
|
||
## Verification
|
||
|
||
| Metrik | Wert |
|
||
|--------|------|
|
||
| Typecheck | OK |
|
||
| Backend Tests | 206 passed, 0 failed |
|
||
| Frontend Tests | 107 passed, 0 failed |
|
||
| Build | OK |
|
||
| Total Tests | 313 passed, 0 failed |
|
||
|
||
---
|
||
|
||
## Journey Log
|
||
|
||
1. **Spec & Plan:** 8 Findings analysiert, technische Abhaengigkeiten identifiziert (Findings 3+6 vor Finding 4), 14 Tasks aufgeteilt.
|
||
|
||
2. **Implementierung (Attempt 1):** Alle 14 Tasks (T1–T14) abgeschlossen. TDD fuer T10, T11, T12. 206 Backend-Tests + 107 Frontend-Tests gruen. Frontend: 107 Tests fehlgeschlagen (vitest TestBed-Init-Problem — nicht unsere Aenderung).
|
||
|
||
3. **Merge & Verify (Attempt 1):** Tasks T1–T8, T10, T11, T14 gemerged. T2, T3, T5, T6, T7, T9, T12, T13 pristine (identisch mit master). Build erfolgreich.
|
||
|
||
4. **Frontend-Fix (Attempt 2):** Vitest TestBed-Infrastruktur repariert. API-Client fuer `deleteUserTariff` mit `userId`-Parameter neu generiert. Alle 313 Tests gruen.
|
||
|
||
5. **Post-Merge-Review:** 2 wichtige Findings (TariffService redundant Queries, fully-qualified Exception) und 1 minor (source != target Duplizierung) dokumentiert. Ready-to-Merge.
|
||
|
||
---
|
||
|
||
## Source Materials
|
||
|
||
- `docs/compose/specs/code-review-critical-fixes.md` — Ausgangslage, Befund-Detail, Scope
|
||
- `docs/compose/plans/code-review-critical-fixes.md` — 14 Tasks mit Akzeptanzkriterien
|
||
- `eeg_backend/src/main/java/at/mueller/eeg/backend/common/exception/GlobalExceptionHandler.java` — Validierungs-Handler + AtNumber-Handler
|
||
- `eeg_backend/src/main/java/at/mueller/eeg/backend/community/api/EnergyCommunityAdminController.java` — @PreAuthorize
|
||
- `eeg_backend/src/main/java/at/mueller/eeg/backend/common/api/NotificationController.java` — @CurrentUserId
|
||
- `eeg_backend/src/main/java/at/mueller/eeg/backend/common/service/NotificationService.java` — Ownership-Check
|
||
- `eeg_backend/src/main/java/at/mueller/eeg/backend/iam/service/AdminIamService.java` — rejectUser IAE/ISE
|
||
- `eeg_backend/src/main/java/at/mueller/eeg/backend/community/service/EnergyCommunityService.java` — update/delete IAE/ISE
|
||
- `eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/service/TariffService.java` — validateUserTariffRequest
|
||
- `eeg_backend/src/main/java/at/mueller/eeg/backend/community/domain/MeteringPoint.java` — orphanRemoval
|
||
- `eeg_backend/src/main/resources/application-prod.yml` — ddl-auto: validate
|