From f0de751da4c1584d37e61d3c069a99587ac5b488 Mon Sep 17 00:00:00 2001 From: Cedric Hornberger Date: Tue, 5 May 2026 17:11:37 +0200 Subject: [PATCH] fix: centralise error handling in GlobalExceptionHandler, add SLF4J logging, remove HTTP 417 and e.printStackTrace() - Expand GlobalExceptionHandler with handlers for ResourceNotFoundException (404), UsernameAlreadyExistsException (409), ResponseStatusException (pass-through), RuntimeException (500), and generic Exception (500); add SLF4J logging - Remove all bare try/catch blocks and e.printStackTrace() calls from ExpenseListController; add SLF4J logger field - Add test: create_returns500_onUnexpectedServiceError Co-Authored-By: Claude Sonnet 4.6 --- .../controller/ExpenseListController.java | 65 +++++++------------ .../controller/GlobalExceptionHandler.java | 39 +++++++++++ .../controller/ExpenseListControllerTest.java | 13 ++++ 3 files changed, 75 insertions(+), 42 deletions(-) diff --git a/src/main/java/de/zendric/app/xpensely_server/controller/ExpenseListController.java b/src/main/java/de/zendric/app/xpensely_server/controller/ExpenseListController.java index 7d86cf7..52ea21d 100644 --- a/src/main/java/de/zendric/app/xpensely_server/controller/ExpenseListController.java +++ b/src/main/java/de/zendric/app/xpensely_server/controller/ExpenseListController.java @@ -5,6 +5,8 @@ import java.util.List; import java.util.Optional; import jakarta.validation.Valid; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.security.core.Authentication; @@ -21,6 +23,8 @@ import de.zendric.app.xpensely_server.services.UserService; @RequestMapping("/api/expenselist") public class ExpenseListController { + private static final Logger log = LoggerFactory.getLogger(ExpenseListController.class); + private final ExpenseListService expenseListService; private final UserService userService; private final CategoryService categoryService; @@ -56,20 +60,13 @@ public class ExpenseListController { @PostMapping("/create") public ResponseEntity create(@RequestBody ExpenseList expenseList, Authentication authentication) { - try { - AppUser authenticatedUser = authenticatedUserResolver.resolveCurrentUser(authentication); - expenseList.setOwner(authenticatedUser); - XpenselyStandardCategories standardCategories = categoryService.getDefaultCategories(); - expenseList.setXpenselyStandardCategories(standardCategories); - expenseList.setSharedWith(null); - ExpenseList savedItem = expenseListService.createList(expenseList); - return new ResponseEntity<>(savedItem, HttpStatus.CREATED); - } catch (ResponseStatusException e) { - throw e; - } catch (Exception e) { - e.printStackTrace(); - return new ResponseEntity<>(null, HttpStatus.EXPECTATION_FAILED); - } + AppUser authenticatedUser = authenticatedUserResolver.resolveCurrentUser(authentication); + expenseList.setOwner(authenticatedUser); + XpenselyStandardCategories standardCategories = categoryService.getDefaultCategories(); + expenseList.setXpenselyStandardCategories(standardCategories); + expenseList.setSharedWith(null); + ExpenseList savedItem = expenseListService.createList(expenseList); + return new ResponseEntity<>(savedItem, HttpStatus.CREATED); } @DeleteMapping("{id}") @@ -79,12 +76,8 @@ public class ExpenseListController { if (listOpt.isEmpty()) return new ResponseEntity<>(HttpStatus.NOT_FOUND); assertOwner(user, listOpt.get()); - try { - expenseListService.deleteById(id); - return new ResponseEntity<>(HttpStatus.NO_CONTENT); - } catch (Exception e) { - return new ResponseEntity<>(HttpStatus.EXPECTATION_FAILED); - } + expenseListService.deleteById(id); + return new ResponseEntity<>(HttpStatus.NO_CONTENT); } @PostMapping("/{id}/add") @@ -97,14 +90,10 @@ public class ExpenseListController { if (listOpt.isEmpty()) return new ResponseEntity<>(HttpStatus.NOT_FOUND); assertMember(user, listOpt.get()); - try { - AppUser expenseOwner = userService.getUserByName(expenseInput.getOwner()); - Expense expense = expenseInput.convertToExpense(expenseOwner.getId()); - Expense addedExpense = expenseListService.addExpenseToList(expenseListId, expense); - return new ResponseEntity<>(addedExpense, HttpStatus.CREATED); - } catch (Exception e) { - return new ResponseEntity<>(null, HttpStatus.INTERNAL_SERVER_ERROR); - } + AppUser expenseOwner = userService.getUserByName(expenseInput.getOwner()); + Expense expense = expenseInput.convertToExpense(expenseOwner.getId()); + Expense addedExpense = expenseListService.addExpenseToList(expenseListId, expense); + return new ResponseEntity<>(addedExpense, HttpStatus.CREATED); } @PutMapping("/{id}/update") @@ -117,14 +106,10 @@ public class ExpenseListController { if (expenseListOpt.isEmpty()) return new ResponseEntity<>(null, HttpStatus.NOT_FOUND); assertMember(user, expenseListOpt.get()); - try { - AppUser expenseOwner = userService.getUserByName(expenseChangeRequest.getOwnerName()); - Expense expense = expenseChangeRequest.convertToExpense(expenseOwner.getId(), expenseListOpt.get()); - Expense updatedExpense = expenseListService.updateExpense(expenseListId, expense); - return new ResponseEntity<>(updatedExpense, HttpStatus.OK); - } catch (Exception e) { - return new ResponseEntity<>(null, HttpStatus.BAD_REQUEST); - } + AppUser expenseOwner = userService.getUserByName(expenseChangeRequest.getOwnerName()); + Expense expense = expenseChangeRequest.convertToExpense(expenseOwner.getId(), expenseListOpt.get()); + Expense updatedExpense = expenseListService.updateExpense(expenseListId, expense); + return new ResponseEntity<>(updatedExpense, HttpStatus.OK); } @DeleteMapping("/{id}/delete") @@ -137,12 +122,8 @@ public class ExpenseListController { if (listOpt.isEmpty()) return new ResponseEntity<>(HttpStatus.NOT_FOUND); assertMember(user, listOpt.get()); - try { - expenseListService.deleteExpenseFromList(expenseListId, expenseId); - return new ResponseEntity<>(HttpStatus.NO_CONTENT); - } catch (Exception e) { - return new ResponseEntity<>(null, HttpStatus.EXPECTATION_FAILED); - } + expenseListService.deleteExpenseFromList(expenseListId, expenseId); + return new ResponseEntity<>(HttpStatus.NO_CONTENT); } @PostMapping("/{listId}/invite") diff --git a/src/main/java/de/zendric/app/xpensely_server/controller/GlobalExceptionHandler.java b/src/main/java/de/zendric/app/xpensely_server/controller/GlobalExceptionHandler.java index efbc72d..5a0c270 100644 --- a/src/main/java/de/zendric/app/xpensely_server/controller/GlobalExceptionHandler.java +++ b/src/main/java/de/zendric/app/xpensely_server/controller/GlobalExceptionHandler.java @@ -1,11 +1,16 @@ package de.zendric.app.xpensely_server.controller; +import de.zendric.app.xpensely_server.model.Exception.ResourceNotFoundException; +import de.zendric.app.xpensely_server.model.Exception.UsernameAlreadyExistsException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.validation.FieldError; import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; +import org.springframework.web.server.ResponseStatusException; import java.util.HashMap; import java.util.Map; @@ -13,6 +18,8 @@ import java.util.Map; @RestControllerAdvice public class GlobalExceptionHandler { + private static final Logger log = LoggerFactory.getLogger(GlobalExceptionHandler.class); + @ExceptionHandler(MethodArgumentNotValidException.class) public ResponseEntity> handleValidationErrors(MethodArgumentNotValidException ex) { Map errors = new HashMap<>(); @@ -27,4 +34,36 @@ public class GlobalExceptionHandler { return ResponseEntity.status(HttpStatus.BAD_REQUEST) .body(Map.of("error", ex.getMessage())); } + + @ExceptionHandler(ResourceNotFoundException.class) + public ResponseEntity> handleNotFound(ResourceNotFoundException ex) { + return ResponseEntity.status(HttpStatus.NOT_FOUND) + .body(Map.of("error", ex.getMessage())); + } + + @ExceptionHandler(UsernameAlreadyExistsException.class) + public ResponseEntity> handleUsernameConflict(UsernameAlreadyExistsException ex) { + return ResponseEntity.status(HttpStatus.CONFLICT) + .body(Map.of("error", ex.getMessage())); + } + + @ExceptionHandler(ResponseStatusException.class) + public ResponseEntity> handleResponseStatus(ResponseStatusException ex) { + return ResponseEntity.status(ex.getStatusCode()) + .body(Map.of("error", ex.getReason() != null ? ex.getReason() : ex.getMessage())); + } + + @ExceptionHandler(RuntimeException.class) + public ResponseEntity> handleRuntime(RuntimeException ex) { + log.error("Unhandled runtime exception", ex); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body(Map.of("error", "An unexpected error occurred")); + } + + @ExceptionHandler(Exception.class) + public ResponseEntity> handleGeneric(Exception ex) { + log.error("Unhandled exception", ex); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body(Map.of("error", "An unexpected error occurred")); + } } diff --git a/src/test/java/de/zendric/app/xpensely_Server/controller/ExpenseListControllerTest.java b/src/test/java/de/zendric/app/xpensely_Server/controller/ExpenseListControllerTest.java index 2cba572..4736738 100644 --- a/src/test/java/de/zendric/app/xpensely_Server/controller/ExpenseListControllerTest.java +++ b/src/test/java/de/zendric/app/xpensely_Server/controller/ExpenseListControllerTest.java @@ -132,4 +132,17 @@ class ExpenseListControllerTest { mockMvc.perform(get("/api/expenselist/mine")) .andExpect(status().isOk()); } + + @Test + void create_returns500_onUnexpectedServiceError() throws Exception { + AppUser user = new AppUser(); + user.setId(1L); + when(authenticatedUserResolver.resolveCurrentUser(any())).thenReturn(user); + when(categoryService.getDefaultCategories()).thenThrow(new RuntimeException("db down")); + + mockMvc.perform(post("/api/expenselist/create") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"name\":\"Groceries\"}")) + .andExpect(status().isInternalServerError()); + } }