Conversation
[FIX] Friend 목록 불러오기 api 수정
[REFACTOR] Log 형식 변경
There was a problem hiding this comment.
Pull request overview
This PR updates request/exception/logging behavior as part of a “main merge & deploy” effort, including propagating request IDs, refactoring the logging aspect, adjusting a friends list default sort, and updating related tests.
Changes:
- Refactor
MdcLoggingFiltertoOncePerRequestFilter, read/writeX-Request-ID, and clear MDC after request completion. - Simplify
LogAspectlogging format and execution timing logic. - Update exception advice logging format and adjust controller default pageable sort.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/loopon/global/log/MdcLoggingFilterTest.java | Updates tests for new X-Request-ID + MDC cleanup behavior. |
| src/test/java/com/loopon/global/log/LogAspectTest.java | Simplifies aspect test but significantly reduces verification scope. |
| src/main/java/com/loopon/user/presentation/FriendController.java | Changes default sort to updatedAt DESC. |
| src/main/java/com/loopon/global/log/MdcLoggingFilter.java | Implements request-id propagation via header + MDC in a once-per-request filter. |
| src/main/java/com/loopon/global/log/LogAspect.java | Refactors pointcuts and logging output format/timing. |
| src/main/java/com/loopon/global/exception/GlobalExceptionAdvice.java | Adds request context to logs; removes a previously present exception handler. |
Comments suppressed due to low confidence (1)
src/main/java/com/loopon/global/exception/GlobalExceptionAdvice.java:109
- Must fix:
AuthorizationExceptionhandling was removed fromGlobalExceptionAdvice. This exception is thrown in auth/JWT flows with a specificErrorCodeand status; without a dedicated handler it will fall into the genericExceptionhandler and return 500 instead of the intended error status. Restore an@ExceptionHandler(AuthorizationException.class)that returnsex.getErrorCode().getStatus()andCommonResponse.onFailure(ex.getErrorCode())(and include request info in the log if desired).
@ExceptionHandler(Exception.class)
public ResponseEntity<CommonResponse<Void>> handleException(Exception ex, HttpServletRequest request) {
log.error("[ERR] {} {} | Exception: ", request.getMethod(), request.getRequestURI(), ex);
return ResponseEntity
.status(ErrorCode.INTERNAL_SERVER_ERROR.getStatus())
.body(CommonResponse.onFailure(ErrorCode.INTERNAL_SERVER_ERROR));
}
|
|
||
| @Pointcut("execution(* com.loopon..*Controller.*(..))") | ||
| public void controllerMethods() { | ||
| @Pointcut("execution(* com.loopon.*Controller.*(..))") |
There was a problem hiding this comment.
Must fix: controller() pointcut is now execution(* com.loopon.*Controller.*(..)), which will not match controllers under subpackages like com.loopon.user.presentation.FriendController (and other ...presentation.*Controller). This effectively disables controller logging. Use a recursive package pattern (e.g., com.loopon..*Controller.*(..)), or otherwise align the pointcut to the actual controller package structure.
| @Pointcut("execution(* com.loopon.*Controller.*(..))") | |
| @Pointcut("execution(* com.loopon..*Controller.*(..))") |
| private String formatArgs(Object[] args) { | ||
| if (args == null || args.length == 0) return "[]"; | ||
| return Arrays.toString(args); | ||
| } |
There was a problem hiding this comment.
Must fix: The aspect now logs arguments via Arrays.toString(args) without any masking. This can leak credentials/tokens/passwords in controller/service logs (the previous masking logic was removed). Reintroduce sensitive-data masking (by parameter name and/or by DTO field annotations) or avoid logging full arguments for authentication-related endpoints.
| public Object logExecution(ProceedingJoinPoint joinPoint) throws Throwable { | ||
| StopWatch stopWatch = new StopWatch(); | ||
| MethodInfo methodInfo = null; | ||
| long startTime = System.currentTimeMillis(); | ||
| Object result = null; | ||
|
|
||
| try { | ||
| methodInfo = extractMethodInfo(joinPoint); | ||
|
|
||
| stopWatch.start(); | ||
| Object result = joinPoint.proceed(); | ||
| stopWatch.stop(); | ||
|
|
||
| logSuccess(methodInfo, result, stopWatch); | ||
| result = joinPoint.proceed(); | ||
| return result; | ||
|
|
||
| } catch (Throwable e) { | ||
| if (stopWatch.isRunning()) { | ||
| stopWatch.stop(); | ||
| } | ||
|
|
||
| logFailure(methodInfo, e, stopWatch); | ||
| throw e; | ||
| } finally { | ||
| long totalTime = System.currentTimeMillis() - startTime; | ||
| MethodSignature signature = (MethodSignature) joinPoint.getSignature(); | ||
|
|
||
| log.info("[{}] {}.{} | Args: {} | Ret: {} | {}ms", | ||
| getLayer(signature), // 1. 레이어 (API / SVC) | ||
| className(signature), // 2. 클래스명 | ||
| methodName(signature), // 3. 메서드명 | ||
| formatArgs(joinPoint.getArgs()), // 4. 파라미터 (단순화) | ||
| formatResult(result), // 5. 결과값 (길이 제한) | ||
| totalTime // 6. 소요시간 | ||
| ); |
There was a problem hiding this comment.
Must fix: Exception cases are no longer logged as errors. Because logging happens only in finally with log.info(...), a thrown exception will produce an INFO log with Ret: void and no exception details/stacktrace, which is a regression in observability. Capture the thrown exception (e.g., catch (Throwable t)), log at WARN/ERROR with details, then rethrow; keep the finally for timing/MDC cleanup if needed.
| @DisplayName("결과값이 100자를 넘으면 잘라야 한다") | ||
| void truncateResult() throws Throwable { | ||
| // given | ||
| String longString = "A".repeat(200); | ||
| given(joinPoint.proceed()).willReturn(longString); | ||
|
|
||
| // When | ||
| logAspect.logExecution(joinPoint); | ||
|
|
||
| // Then | ||
| assertThat(output.getOut()) | ||
| .contains(longString.substring(0, 100) + "...") | ||
| .doesNotContain(longString); | ||
| } | ||
|
|
||
| private void setupJoinPoint(Class<?> targetClass, String methodName, String[] paramNames, Object[] args) { | ||
| doReturn(targetClass).when(methodSignature).getDeclaringType(); | ||
| given(joinPoint.getSignature()).willReturn(signature); | ||
| given(signature.getDeclaringType()).willReturn(TestController.class); | ||
| given(signature.getName()).willReturn("testMethod"); | ||
| given(joinPoint.getArgs()).willReturn(new Object[]{}); | ||
|
|
||
| given(methodSignature.getName()).willReturn(methodName); | ||
| given(methodSignature.getParameterNames()).willReturn(paramNames); | ||
| // when | ||
| Object result = logAspect.logExecution(joinPoint); | ||
|
|
||
| given(joinPoint.getSignature()).willReturn(methodSignature); | ||
| given(joinPoint.getArgs()).willReturn(args); | ||
| // then | ||
| assertEquals(longString, result); |
There was a problem hiding this comment.
Must fix (tests): truncateResult() no longer verifies truncation behavior. The aspect truncates only in the log message, but this test asserts the returned value equals the full 200-character string and does not capture/assert log output, so it won't fail if truncation is broken (and the DisplayName is misleading). Add log output assertions (e.g., via OutputCaptureExtension) or adjust the test/DisplayName to match what is actually being tested.
| @DisplayName("헤더에 ID가 없으면 새로 생성하고 MDC에 넣어야 한다") | ||
| void generateRequestId() throws Exception { | ||
| // given | ||
| MockHttpServletRequest req = new MockHttpServletRequest(); | ||
| MockHttpServletResponse res = new MockHttpServletResponse(); | ||
| FilterChain chain = mock(FilterChain.class); | ||
|
|
||
| // when | ||
| filter.doFilter(req, res, chain); | ||
|
|
||
| // then | ||
| String requestId = res.getHeader("X-Request-ID"); | ||
| assertNotNull(requestId); | ||
| verify(chain, times(1)).doFilter(req, res); | ||
| assertNull(MDC.get("request_id")); |
There was a problem hiding this comment.
Discussion needed (tests): MdcLoggingFilterTest no longer verifies that the MDC value is present during filterChain.doFilter(...), nor that MDC cleanup still happens when the downstream chain throws. The current assertions only check the response header and that MDC is empty after completion, so regressions in MDC scoping/exception cleanup could slip through. Consider stubbing the chain to assert MDC inside the invocation and adding an exception-path test.
No description provided.