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 <noreply@anthropic.com>
This commit is contained in:
@@ -5,6 +5,8 @@ import java.util.List;
|
|||||||
import java.util.Optional;
|
import java.util.Optional;
|
||||||
|
|
||||||
import jakarta.validation.Valid;
|
import jakarta.validation.Valid;
|
||||||
|
import org.slf4j.Logger;
|
||||||
|
import org.slf4j.LoggerFactory;
|
||||||
import org.springframework.http.HttpStatus;
|
import org.springframework.http.HttpStatus;
|
||||||
import org.springframework.http.ResponseEntity;
|
import org.springframework.http.ResponseEntity;
|
||||||
import org.springframework.security.core.Authentication;
|
import org.springframework.security.core.Authentication;
|
||||||
@@ -21,6 +23,8 @@ import de.zendric.app.xpensely_server.services.UserService;
|
|||||||
@RequestMapping("/api/expenselist")
|
@RequestMapping("/api/expenselist")
|
||||||
public class ExpenseListController {
|
public class ExpenseListController {
|
||||||
|
|
||||||
|
private static final Logger log = LoggerFactory.getLogger(ExpenseListController.class);
|
||||||
|
|
||||||
private final ExpenseListService expenseListService;
|
private final ExpenseListService expenseListService;
|
||||||
private final UserService userService;
|
private final UserService userService;
|
||||||
private final CategoryService categoryService;
|
private final CategoryService categoryService;
|
||||||
@@ -56,7 +60,6 @@ public class ExpenseListController {
|
|||||||
@PostMapping("/create")
|
@PostMapping("/create")
|
||||||
public ResponseEntity<ExpenseList> create(@RequestBody ExpenseList expenseList,
|
public ResponseEntity<ExpenseList> create(@RequestBody ExpenseList expenseList,
|
||||||
Authentication authentication) {
|
Authentication authentication) {
|
||||||
try {
|
|
||||||
AppUser authenticatedUser = authenticatedUserResolver.resolveCurrentUser(authentication);
|
AppUser authenticatedUser = authenticatedUserResolver.resolveCurrentUser(authentication);
|
||||||
expenseList.setOwner(authenticatedUser);
|
expenseList.setOwner(authenticatedUser);
|
||||||
XpenselyStandardCategories standardCategories = categoryService.getDefaultCategories();
|
XpenselyStandardCategories standardCategories = categoryService.getDefaultCategories();
|
||||||
@@ -64,12 +67,6 @@ public class ExpenseListController {
|
|||||||
expenseList.setSharedWith(null);
|
expenseList.setSharedWith(null);
|
||||||
ExpenseList savedItem = expenseListService.createList(expenseList);
|
ExpenseList savedItem = expenseListService.createList(expenseList);
|
||||||
return new ResponseEntity<>(savedItem, HttpStatus.CREATED);
|
return new ResponseEntity<>(savedItem, HttpStatus.CREATED);
|
||||||
} catch (ResponseStatusException e) {
|
|
||||||
throw e;
|
|
||||||
} catch (Exception e) {
|
|
||||||
e.printStackTrace();
|
|
||||||
return new ResponseEntity<>(null, HttpStatus.EXPECTATION_FAILED);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@DeleteMapping("{id}")
|
@DeleteMapping("{id}")
|
||||||
@@ -79,12 +76,8 @@ public class ExpenseListController {
|
|||||||
if (listOpt.isEmpty())
|
if (listOpt.isEmpty())
|
||||||
return new ResponseEntity<>(HttpStatus.NOT_FOUND);
|
return new ResponseEntity<>(HttpStatus.NOT_FOUND);
|
||||||
assertOwner(user, listOpt.get());
|
assertOwner(user, listOpt.get());
|
||||||
try {
|
|
||||||
expenseListService.deleteById(id);
|
expenseListService.deleteById(id);
|
||||||
return new ResponseEntity<>(HttpStatus.NO_CONTENT);
|
return new ResponseEntity<>(HttpStatus.NO_CONTENT);
|
||||||
} catch (Exception e) {
|
|
||||||
return new ResponseEntity<>(HttpStatus.EXPECTATION_FAILED);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@PostMapping("/{id}/add")
|
@PostMapping("/{id}/add")
|
||||||
@@ -97,14 +90,10 @@ public class ExpenseListController {
|
|||||||
if (listOpt.isEmpty())
|
if (listOpt.isEmpty())
|
||||||
return new ResponseEntity<>(HttpStatus.NOT_FOUND);
|
return new ResponseEntity<>(HttpStatus.NOT_FOUND);
|
||||||
assertMember(user, listOpt.get());
|
assertMember(user, listOpt.get());
|
||||||
try {
|
|
||||||
AppUser expenseOwner = userService.getUserByName(expenseInput.getOwner());
|
AppUser expenseOwner = userService.getUserByName(expenseInput.getOwner());
|
||||||
Expense expense = expenseInput.convertToExpense(expenseOwner.getId());
|
Expense expense = expenseInput.convertToExpense(expenseOwner.getId());
|
||||||
Expense addedExpense = expenseListService.addExpenseToList(expenseListId, expense);
|
Expense addedExpense = expenseListService.addExpenseToList(expenseListId, expense);
|
||||||
return new ResponseEntity<>(addedExpense, HttpStatus.CREATED);
|
return new ResponseEntity<>(addedExpense, HttpStatus.CREATED);
|
||||||
} catch (Exception e) {
|
|
||||||
return new ResponseEntity<>(null, HttpStatus.INTERNAL_SERVER_ERROR);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@PutMapping("/{id}/update")
|
@PutMapping("/{id}/update")
|
||||||
@@ -117,14 +106,10 @@ public class ExpenseListController {
|
|||||||
if (expenseListOpt.isEmpty())
|
if (expenseListOpt.isEmpty())
|
||||||
return new ResponseEntity<>(null, HttpStatus.NOT_FOUND);
|
return new ResponseEntity<>(null, HttpStatus.NOT_FOUND);
|
||||||
assertMember(user, expenseListOpt.get());
|
assertMember(user, expenseListOpt.get());
|
||||||
try {
|
|
||||||
AppUser expenseOwner = userService.getUserByName(expenseChangeRequest.getOwnerName());
|
AppUser expenseOwner = userService.getUserByName(expenseChangeRequest.getOwnerName());
|
||||||
Expense expense = expenseChangeRequest.convertToExpense(expenseOwner.getId(), expenseListOpt.get());
|
Expense expense = expenseChangeRequest.convertToExpense(expenseOwner.getId(), expenseListOpt.get());
|
||||||
Expense updatedExpense = expenseListService.updateExpense(expenseListId, expense);
|
Expense updatedExpense = expenseListService.updateExpense(expenseListId, expense);
|
||||||
return new ResponseEntity<>(updatedExpense, HttpStatus.OK);
|
return new ResponseEntity<>(updatedExpense, HttpStatus.OK);
|
||||||
} catch (Exception e) {
|
|
||||||
return new ResponseEntity<>(null, HttpStatus.BAD_REQUEST);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@DeleteMapping("/{id}/delete")
|
@DeleteMapping("/{id}/delete")
|
||||||
@@ -137,12 +122,8 @@ public class ExpenseListController {
|
|||||||
if (listOpt.isEmpty())
|
if (listOpt.isEmpty())
|
||||||
return new ResponseEntity<>(HttpStatus.NOT_FOUND);
|
return new ResponseEntity<>(HttpStatus.NOT_FOUND);
|
||||||
assertMember(user, listOpt.get());
|
assertMember(user, listOpt.get());
|
||||||
try {
|
|
||||||
expenseListService.deleteExpenseFromList(expenseListId, expenseId);
|
expenseListService.deleteExpenseFromList(expenseListId, expenseId);
|
||||||
return new ResponseEntity<>(HttpStatus.NO_CONTENT);
|
return new ResponseEntity<>(HttpStatus.NO_CONTENT);
|
||||||
} catch (Exception e) {
|
|
||||||
return new ResponseEntity<>(null, HttpStatus.EXPECTATION_FAILED);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@PostMapping("/{listId}/invite")
|
@PostMapping("/{listId}/invite")
|
||||||
|
|||||||
@@ -1,11 +1,16 @@
|
|||||||
package de.zendric.app.xpensely_server.controller;
|
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.HttpStatus;
|
||||||
import org.springframework.http.ResponseEntity;
|
import org.springframework.http.ResponseEntity;
|
||||||
import org.springframework.validation.FieldError;
|
import org.springframework.validation.FieldError;
|
||||||
import org.springframework.web.bind.MethodArgumentNotValidException;
|
import org.springframework.web.bind.MethodArgumentNotValidException;
|
||||||
import org.springframework.web.bind.annotation.ExceptionHandler;
|
import org.springframework.web.bind.annotation.ExceptionHandler;
|
||||||
import org.springframework.web.bind.annotation.RestControllerAdvice;
|
import org.springframework.web.bind.annotation.RestControllerAdvice;
|
||||||
|
import org.springframework.web.server.ResponseStatusException;
|
||||||
|
|
||||||
import java.util.HashMap;
|
import java.util.HashMap;
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
@@ -13,6 +18,8 @@ import java.util.Map;
|
|||||||
@RestControllerAdvice
|
@RestControllerAdvice
|
||||||
public class GlobalExceptionHandler {
|
public class GlobalExceptionHandler {
|
||||||
|
|
||||||
|
private static final Logger log = LoggerFactory.getLogger(GlobalExceptionHandler.class);
|
||||||
|
|
||||||
@ExceptionHandler(MethodArgumentNotValidException.class)
|
@ExceptionHandler(MethodArgumentNotValidException.class)
|
||||||
public ResponseEntity<Map<String, String>> handleValidationErrors(MethodArgumentNotValidException ex) {
|
public ResponseEntity<Map<String, String>> handleValidationErrors(MethodArgumentNotValidException ex) {
|
||||||
Map<String, String> errors = new HashMap<>();
|
Map<String, String> errors = new HashMap<>();
|
||||||
@@ -27,4 +34,36 @@ public class GlobalExceptionHandler {
|
|||||||
return ResponseEntity.status(HttpStatus.BAD_REQUEST)
|
return ResponseEntity.status(HttpStatus.BAD_REQUEST)
|
||||||
.body(Map.of("error", ex.getMessage()));
|
.body(Map.of("error", ex.getMessage()));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ExceptionHandler(ResourceNotFoundException.class)
|
||||||
|
public ResponseEntity<Map<String, String>> handleNotFound(ResourceNotFoundException ex) {
|
||||||
|
return ResponseEntity.status(HttpStatus.NOT_FOUND)
|
||||||
|
.body(Map.of("error", ex.getMessage()));
|
||||||
|
}
|
||||||
|
|
||||||
|
@ExceptionHandler(UsernameAlreadyExistsException.class)
|
||||||
|
public ResponseEntity<Map<String, String>> handleUsernameConflict(UsernameAlreadyExistsException ex) {
|
||||||
|
return ResponseEntity.status(HttpStatus.CONFLICT)
|
||||||
|
.body(Map.of("error", ex.getMessage()));
|
||||||
|
}
|
||||||
|
|
||||||
|
@ExceptionHandler(ResponseStatusException.class)
|
||||||
|
public ResponseEntity<Map<String, String>> handleResponseStatus(ResponseStatusException ex) {
|
||||||
|
return ResponseEntity.status(ex.getStatusCode())
|
||||||
|
.body(Map.of("error", ex.getReason() != null ? ex.getReason() : ex.getMessage()));
|
||||||
|
}
|
||||||
|
|
||||||
|
@ExceptionHandler(RuntimeException.class)
|
||||||
|
public ResponseEntity<Map<String, String>> 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<Map<String, String>> handleGeneric(Exception ex) {
|
||||||
|
log.error("Unhandled exception", ex);
|
||||||
|
return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR)
|
||||||
|
.body(Map.of("error", "An unexpected error occurred"));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
+13
@@ -132,4 +132,17 @@ class ExpenseListControllerTest {
|
|||||||
mockMvc.perform(get("/api/expenselist/mine"))
|
mockMvc.perform(get("/api/expenselist/mine"))
|
||||||
.andExpect(status().isOk());
|
.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());
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user