Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Java lamp API implementation by adopting newer Java idioms (e.g., Stream#toList(), pattern matching for instanceof), and by making “not found” behavior explicit via LampNotFoundException + centralized @ControllerAdvice handling, while shifting service APIs away from nullable/boolean sentinel returns.
Changes:
- Updated
LampServiceto returnOptional<Lamp>fromfindById()and to throwLampNotFoundExceptionfromupdate()/delete()instead of returningnull/false. - Introduced
LampNotFoundExceptionand added aGlobalExceptionHandlermapping to HTTP 404 responses. - Replaced
Collectors.toList()withtoList()and updated unit/controller tests to reflect Optional/exception flows.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/java/src/test/java/org/openapitools/service/LampServiceTest.java | Updates service tests for Optional return type and exception-based update/delete behavior. |
| src/java/src/test/java/org/openapitools/exception/LampNotFoundExceptionTest.java | Adds unit coverage for the new exception message and ID retention. |
| src/java/src/test/java/org/openapitools/exception/GlobalExceptionHandlerUnitTest.java | Adds unit test coverage for the new 404 handler. |
| src/java/src/test/java/org/openapitools/controller/LampsControllerTest.java | Updates controller tests for Optional-based get flow and exception-based update/delete 404s. |
| src/java/src/main/java/org/openapitools/service/LampService.java | Shifts to Optional for findById, throws LampNotFoundException for update/delete, and uses toList(). |
| src/java/src/main/java/org/openapitools/repository/impl/InMemoryLampRepository.java | Replaces Collectors.toList() with toList() in in-memory queries. |
| src/java/src/main/java/org/openapitools/exception/LampNotFoundException.java | Adds a dedicated domain exception carrying the missing lamp ID. |
| src/java/src/main/java/org/openapitools/exception/GlobalExceptionHandler.java | Adds @ExceptionHandler mapping LampNotFoundException to 404 responses. |
| src/java/src/main/java/org/openapitools/entity/LampEntity.java | Uses pattern matching for instanceof in equals(). |
| src/java/src/main/java/org/openapitools/controller/LampsController.java | Removes local try/catch + unchecked casts; relies on centralized exception handling and Optional/exception-based flow. |
| public CompletableFuture<ResponseEntity<Void>> deleteLamp(final String lampId) { | ||
| return CompletableFuture.supplyAsync( | ||
| () -> { | ||
| try { | ||
| final UUID lampUuid = UUID.fromString(lampId); | ||
| final boolean deleted = lampService.delete(lampUuid); | ||
| if (deleted) { | ||
| return ResponseEntity.noContent().<Void>build(); | ||
| } else { | ||
| return ResponseEntity.notFound().<Void>build(); | ||
| } | ||
| } catch (IllegalArgumentException e) { | ||
| final Error error = new Error("INVALID_ARGUMENT"); | ||
| return (ResponseEntity<Void>) | ||
| (ResponseEntity<?>) ResponseEntity.badRequest().body(error); | ||
| } | ||
| final UUID lampUuid = UUID.fromString(lampId); | ||
| lampService.delete(lampUuid); | ||
| return ResponseEntity.noContent().<Void>build(); | ||
| }); |
There was a problem hiding this comment.
CompletableFuture.supplyAsync(...) is used without an explicit executor, which will run these request handlers on ForkJoinPool.commonPool(). That can create uncontrolled concurrency and lose request-scoped context (logging MDC, security context), and it diverges from the generated LampsApi defaults which use Runnable::run (synchronous). Prefer returning CompletableFuture.completedFuture(...) (since the logic is synchronous) or pass Runnable::run / a Spring-managed executor explicitly so request processing stays on the container thread pool.
- Replace .collect(Collectors.toList()) with .toList() (Java 16+) - Use pattern matching for instanceof in LampEntity.equals() (Java 16+) - Add LampNotFoundException for explicit 404 handling via @ControllerAdvice - Change LampService.findById() to return Optional<Lamp> - Change LampService.update()/delete() to throw LampNotFoundException - Remove @SuppressWarnings("unchecked") and unsafe double-cast patterns - Remove try/catch blocks from controller (GlobalExceptionHandler handles them) Closes #352 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
259b6dd to
90e4afd
Compare
Pass Runnable::run as the executor to CompletableFuture.supplyAsync() to keep request processing on the container thread pool, matching the generated LampsApi defaults. This preserves request-scoped context (MDC, security) and avoids uncontrolled concurrency via ForkJoinPool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add @serial serialVersionUID to satisfy PMD MissingSerialVersionUID rule, since the class extends RuntimeException (Serializable). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
.collect(Collectors.toList())with.toList()(Java 16+) across service and repository layersinstanceofinLampEntity.equals()(Java 16+)LampNotFoundExceptionwith@ControllerAdvicehandler for explicit 404 responsesLampService.findById()to returnOptional<Lamp>instead of nullableLampService.update()/delete()to throwLampNotFoundExceptioninstead of returning null/false@SuppressWarnings("unchecked"), unsafe double-cast patterns, and try/catch blocks from controller — exceptions now handled centrally byGlobalExceptionHandlerCloses #352
Test plan
mvn test)mvn spotless:check)LampNotFoundExceptionTestcovers exception classGlobalExceptionHandlerUnitTestcovers 404 handlerOptional/exception-based flow🤖 Generated with Claude Code