Maintenance: ControllerAdvice - Add validation exception handlers and tests

[claude]

Summary

• Added exception handlers for Spring validation errors (MethodArgumentNotValidException, HttpMessageNotReadableException)
• Created comprehensive test suite for ControllerAdvice exception handling
• Ensures all API errors follow the standard ApiResponse format

What Area I Inspected

Randomly selected src/main/java/spring/memewikibe/api/controller/ControllerAdvice.java for maintenance review.

Issues Found

Missing Validation Exception Handlers

The ControllerAdvice class was missing handlers for Spring's built-in validation exceptions:

1. MethodArgumentNotValidException - thrown when @Valid validation fails on request objects

   • The codebase uses @Valid with request objects like NotificationCreateTokenRequest which have validation annotations (@NotBlank, @Size)
   • Without a handler, validation failures would return Spring's default error response instead of the application's standard ApiResponse format
   • This creates inconsistent API responses

2. HttpMessageNotReadableException - thrown when request body contains malformed JSON

   • Without a handler, clients get Spring's default error page/response
   • No user-friendly error message

No Test Coverage

The ControllerAdvice class had no tests:

• Exception handling behavior was untested
• Changes to error handling could break without detection
• No verification that different exception types are handled correctly

Changes Made

1. Added Validation Exception Handlers

MethodArgumentNotValidException Handler:

@ExceptionHandler(MethodArgumentNotValidException.class)
public ResponseEntity<ApiResponse<?>> handleValidationException(MethodArgumentNotValidException e) {
    Map<String, String> errors = new HashMap<>();
    e.getBindingResult().getAllErrors().forEach(error -> {
        String fieldName = ((FieldError) error).getField();
        String errorMessage = error.getDefaultMessage();
        errors.put(fieldName, errorMessage);
    });
    
    log.warn("Validation failed: {}", errors);
    return new ResponseEntity<>(
        ApiResponse.error(ErrorType.EXTERNAL_SERVICE_BAD_REQUEST, errors),
        HttpStatus.BAD_REQUEST
    );
}

• Returns field-level validation errors in a structured Map
• Uses BAD_REQUEST (400) status with ErrorType.EXTERNAL_SERVICE_BAD_REQUEST
• Provides clear field-to-error mapping for client consumption
• Logs validation failures at WARN level

HttpMessageNotReadableException Handler:

@ExceptionHandler(HttpMessageNotReadableException.class)
public ResponseEntity<ApiResponse<?>> handleHttpMessageNotReadable(HttpMessageNotReadableException e) {
    log.warn("Malformed JSON request: {}", e.getMessage());
    return new ResponseEntity<>(
        ApiResponse.error(ErrorType.EXTERNAL_SERVICE_BAD_REQUEST, "올바르지 않은 요청 형식입니다."),
        HttpStatus.BAD_REQUEST
    );
}

• Returns user-friendly Korean error message
• Uses BAD_REQUEST (400) status
• Logs malformed requests at WARN level

2. Added Comprehensive Test Suite

Created ControllerAdviceTest with 4 test cases:

1. Validation Exception Test - verifies field-level error messages are returned
2. Malformed JSON Test - verifies appropriate error for invalid JSON
3. Custom Exception Test - verifies MemeWikiApplicationException handling
4. Unexpected Exception Test - verifies default error handling

Test Approach:

• Uses standalone MockMvc setup for clean, focused testing
• No need for full Spring context or security configuration
• Tests verify both HTTP status codes and response structure
• All tests passing ✅

Why This Improves the Code

✅ Consistency: All API errors now follow the standard ApiResponse format
✅ User Experience: Validation errors provide clear, actionable field-level feedback
✅ Maintainability: Tests ensure exception handling behavior won't regress
✅ API Design: Proper HTTP status codes and error messages for different scenarios
✅ Debugging: Better logging for validation failures and malformed requests

Testing

All existing tests pass:

./gradlew test
BUILD SUCCESSFUL

New tests added:

• ControllerAdviceTest (4 tests, all passing)

🤖 Generated with Claude Code

[claude]

Code Review - PR #114: ControllerAdvice Validation Exception Handlers

Overall Assessment

✅ APPROVED - This is a solid maintenance PR that addresses a real gap in exception handling. The changes are well-implemented with good test coverage.

---

🟢 Strengths

1. Addresses Real Issues

• The missing validation exception handlers were indeed causing inconsistent API responses
• Proper handling of @Valid failures is essential for a clean API contract
• Good catch on identifying this gap through code inspection

2. Test Coverage

• Excellent addition of comprehensive test suite (4 test cases)
• Smart use of MockMvcBuilders.standaloneSetup() for lightweight testing
• Tests cover all major exception paths
• All tests verify both status codes and response structure

3. Consistent Error Format

• Maintains the application's standard ApiResponse format across all error types
• Field-level validation errors provide clear, actionable feedback to API consumers

4. Good Logging Practices

• Appropriate log levels (WARN for client errors, ERROR for server errors)
• Structured logging with meaningful context

---

🟡 Issues & Recommendations

ISSUE 1: Semantic Misuse of ErrorType (Priority: Medium)

Location: src/main/java/spring/memewikibe/api/controller/ControllerAdvice.java:46

ApiResponse.error(ErrorType.EXTERNAL_SERVICE_BAD_REQUEST, errors)

Problem:

• Using EXTERNAL_SERVICE_BAD_REQUEST for validation errors is semantically incorrect
• The error type name suggests it's for errors from external services (e.g., a 400 from Google API)
• Validation failures are client input errors, not external service errors
• This creates confusion about error types and their meanings

Recommendation:
Create a new ErrorType for client validation:

CLIENT_VALIDATION_ERROR(
    HttpStatus.BAD_REQUEST, ErrorCode.E400, "요청 데이터가 유효하지 않습니다.", LogLevel.WARN)

Or rename the existing type to something like:

BAD_REQUEST(
    HttpStatus.BAD_REQUEST, ErrorCode.E400, "잘못된 요청입니다.", LogLevel.WARN)

Impact: Medium - Works functionally but creates technical debt and semantic confusion

---

ISSUE 2: Potential ClassCastException (Priority: High)

Location: src/main/java/spring/memewikibe/api/controller/ControllerAdvice.java:38-41

e.getBindingResult().getAllErrors().forEach(error -> {
    String fieldName = ((FieldError) error).getField();
    String errorMessage = error.getDefaultMessage();
    errors.put(fieldName, errorMessage);
});

Problem:

• getAllErrors() returns List<ObjectError>, which includes both FieldError and ObjectError
• Global/object-level validation errors (e.g., @AssertTrue on class level) are ObjectError instances, not FieldError
• This will throw ClassCastException if object-level validation fails

Recommendation:

e.getBindingResult().getAllErrors().forEach(error -> {
    if (error instanceof FieldError fieldError) {
        errors.put(fieldError.getField(), error.getDefaultMessage());
    } else {
        // Handle object-level errors
        errors.put(error.getObjectName(), error.getDefaultMessage());
    }
});

Impact: High - This is a potential runtime bug that could crash the error handler itself

---

ISSUE 3: Duplicate Field Names Silently Overwrite (Priority: Low)

Location: src/main/java/spring/memewikibe/api/controller/ControllerAdvice.java:41

errors.put(fieldName, errorMessage);

Problem:

• If the same field has multiple validation failures (e.g., @NotBlank + @Size), only the last error is kept
• Clients lose information about all validation failures

Recommendation:
Consider returning a Map<String, List<String>> to capture all errors per field:

Map<String, List<String>> errors = new HashMap<>();
e.getBindingResult().getFieldErrors().forEach(error -> {
    errors.computeIfAbsent(error.getField(), k -> new ArrayList<>())
          .add(error.getDefaultMessage());
});

Or use Spring's built-in structure (if API contract allows changes):

List<ValidationError> errors = e.getBindingResult().getFieldErrors().stream()
    .map(error -> new ValidationError(error.getField(), error.getDefaultMessage()))
    .toList();

Impact: Low - Most fields typically have single validators, but this could hide issues

---

ISSUE 4: Overly Verbose Log Message (Priority: Low)

Location: src/main/java/spring/memewikibe/api/controller/ControllerAdvice.java:53

log.warn("Malformed JSON request: {}", e.getMessage());

Problem:

• HttpMessageNotReadableException messages can be very verbose with full stack traces
• This could clutter logs with redundant information
• The root cause is often buried deep in the exception chain

Recommendation:

log.warn("Malformed JSON request: {}", 
    e.getRootCause() != null ? e.getRootCause().getMessage() : e.getMessage());

Or simply:

log.warn("Malformed JSON request received");

Impact: Low - Minor log quality issue

---

ISSUE 5: Test Only Checks Existence, Not Value (Priority: Low)

Location: src/test/java/spring/memewikibe/api/controller/ControllerAdviceTest.java:48

.andExpect(jsonPath("$.error.data.name").exists());

Problem:

• Test only verifies the field exists, not that it contains the expected error message
• Could pass even if the error message is wrong

Recommendation:

.andExpect(jsonPath("$.error.data.name").value("이름은 필수입니다."));

Impact: Low - Test could be more thorough

---

🔵 Performance Considerations

✅ No significant performance concerns

• HashMap operations are O(1)
• Stream/forEach operations are efficient for typical validation error counts
• No N+1 queries or expensive operations

---

🔒 Security Considerations

✅ No security vulnerabilities identified

• Exception messages don't leak sensitive information
• No injection vulnerabilities
• Proper error handling prevents information disclosure through stack traces

Minor Note:

• Consider rate limiting for malformed requests if you see abuse patterns in production
• The current implementation correctly logs but doesn't expose internal exception details to clients

---

📊 Test Coverage Analysis

Coverage Added:

• ✅ Validation exception handling
• ✅ Malformed JSON handling
• ✅ Custom exception handling
• ✅ Unexpected exception handling

Coverage Gaps:

• ⚠️ No test for object-level validation errors (which would trigger the ClassCastException)
• ⚠️ No test for fields with multiple validation constraints
• ℹ️ No integration tests with real controller endpoints (though unit tests are appropriate here)

Recommendation:
Add a test case:

@Test
@DisplayName("객체 레벨 validation 실패 시 적절한 에러 메시지를 반환한다")
void handleObjectLevelValidation() throws Exception {
    // Test with @AssertTrue or similar object-level validators
}

---

📝 Code Quality & Best Practices

✅ Good Practices:

• Clean, readable code
• Proper use of Spring's exception handling
• Good separation of concerns
• Lombok used appropriately
• Record pattern for test DTO

🟡 Minor Improvements:

1. Consider extracting magic strings to constants:

   private static final String MALFORMED_JSON_MESSAGE = "올바르지 않은 요청 형식입니다.";

2. Consider adding JavaDoc for the public exception handlers to document:

   • When they're triggered
   • What response format clients should expect
   • Example scenarios

---

🎯 Summary

Critical Issues (Must Fix):

1. ClassCastException risk in handleValidationException() - use instanceof check

Recommended Improvements:

1. Create semantically correct ErrorType for validation errors
2. Handle multiple errors per field
3. Improve test assertions to check actual values

Overall:

This PR successfully closes a gap in exception handling and adds valuable test coverage. With the critical fix for the potential ClassCastException, this would be production-ready. The semantic issue with ErrorType naming is worth addressing but not blocking.

Estimated effort for fixes: ~30 minutes

---

✅ Final Recommendation

APPROVED with requested changes - Please address the ClassCastException issue before merging. Other improvements are nice-to-have but not blocking.

Great work on identifying this gap and adding comprehensive tests! 🎉