Conversation
📝 WalkthroughWalkthroughThis PR updates the MeetingController to use MidPointAsyncUseCase, enhances SubwayRouteService with retry logic and station name normalization to reduce unnecessary API calls, and improves SeoulMetroClient with better error handling and manual JSON parsing for robustness. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SubwayRouteService
participant SeoulMetroClient
participant SeoulMetroAPI as Seoul Metro API
Client->>SubwayRouteService: getRoute(startStation, endStation)
alt Identical stations detected
SubwayRouteService->>SubwayRouteService: normalizeStationName()
SubwayRouteService->>Client: return zero-time, zero-distance route
else Different stations
loop Retry up to 10 times
SubwayRouteService->>SeoulMetroClient: call API
SeoulMetroClient->>SeoulMetroAPI: HTTP request
alt API returns 5xx
SeoulMetroAPI-->>SeoulMetroClient: 5xx error
SeoulMetroClient->>SeoulMetroClient: log error, return Mono error
SeoulMetroClient-->>SubwayRouteService: error signal
SubwayRouteService->>SubwayRouteService: retry logic
else API returns success
SeoulMetroAPI-->>SeoulMetroClient: response body
SeoulMetroClient->>SeoulMetroClient: parse JSON manually
SeoulMetroClient-->>SubwayRouteService: SeoulMetroRouteResponse
SubwayRouteService->>SubwayRouteService: validate & transform
SubwayRouteService->>Client: return route
break Successful retrieval
end
end
end
alt Retries exhausted
SubwayRouteService->>SubwayRouteService: log error
SubwayRouteService->>Client: throw exception
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/swyp/mingling/external/SeoulMetroClient.java (1)
39-39:⚠️ Potential issue | 🔴 CriticalHardcoded date string left in production code.
Line 39 has a hardcoded date
"2026-06-06 14:00:00"with the realLocalDateTime.now()commented out on line 37. This means every API call uses the same fixed timestamp regardless of when it's actually made. This looks like debug/test code that was accidentally left in.Proposed fix
-// String now = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss")); - // 지하철 운행시간 종료 후 - String now = "2026-06-06 14:00:00"; + String now = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss"));
🤖 Fix all issues with AI agents
In `@src/main/java/swyp/mingling/domain/subway/service/SubwayRouteService.java`:
- Around line 59-63: The log block in SubwayRouteService uses
response.getPathInfoList().size() without ensuring getPathInfoList() is
non-null, risking NPE; update the logging to null-check
response.getPathInfoList() (or use Optional) before calling size and log a safe
value (e.g., 0 or "null") when the list is null, keeping the existing checks for
response and response.getErrorCode(); ensure you update the same logging code
that references response.getPathInfoList() and response.getBody().
- Around line 30-47: The comparison of normalizedStart and normalizedEnd in
SubwayRouteService can NPE when normalizeStationName returns null (e.g.,
startStation null); fix by guarding nulls before calling equals — either
null-check the raw inputs at the start of the method or use a null-safe
comparison (e.g., Objects.equals(normalizeStationName(startStation),
normalizeStationName(endStation))). Update the early-return block that builds
SubwayRouteInfo when stations match to rely on the null-safe check and ensure
normalizeStationName is only invoked in a safe way (or validate
startStation/endStation and throw/handle appropriately) so
normalizedStart.equals(...) cannot be called on a null.
In `@src/main/java/swyp/mingling/external/SeoulMetroClient.java`:
- Around line 52-58: The onStatus handler in SeoulMetroClient currently returns
Mono.error(new RuntimeException(...)) which bypasses the retry in
SubwayRouteService.getRoute(); change the error produced in the 5xx branch to a
BusinessException with the appropriate error code (e.g.,
BusinessException(ErrorCode.SUBWAY_API_SERVER_ERROR)) so that
SubwayRouteService.getRoute() can catch and retry, preserving the existing log
lines in the handler and ensuring the emitted Mono.error carries the
BusinessException instead of RuntimeException.
- Around line 64-71: Replace per-call ObjectMapper construction with a shared
instance and stop hiding parse failures: introduce a single ObjectMapper for
SeoulMetroClient (e.g., a private static final ObjectMapper or injected field)
and in the block that currently does new ObjectMapper().readValue(...,
SeoulMetroRouteResponse.class) change the catch path to propagate the parsing
failure via Mono.error(...) (after logging the error with the exception) instead
of returning Mono.just(new SeoulMetroRouteResponse()); update references to
ObjectMapper, SeoulMetroRouteResponse and the parsing block so the client uses
the shared mapper and parsing errors surface to callers.
In
`@src/test/java/swyp/mingling/domain/subway/service/SubwayRouteServiceTest.java`:
- Around line 22-24: SubwayRouteServiceTest currently hardcodes a Seoul Metro
API key in the `@TestPropertySource`; rotate the compromised key immediately, then
remove the literal and change the test to reference an environment variable or
secret (e.g., replace the value with a placeholder like ${SEOUL_METRO_API_KEY})
so the key is injected at runtime, and scope the test to an integration profile
(e.g., annotate or configure the test class SubwayRouteServiceTest to run only
under an "integration" profile) so it won't run in normal CI; ensure any local
CI/test configs document how to supply the SEOUL_METRO_API_KEY from environment
or secrets manager.
🧹 Nitpick comments (3)
src/main/java/swyp/mingling/domain/meeting/controller/MeetingController.java (1)
55-64: JavaDoc says "동기 버전" but the use case isMidPointAsyncUseCase— contradictory naming.The comment labels this endpoint as the synchronous variant, yet the injected dependency is
MidPointAsyncUseCase. This will confuse future readers about the actual execution model. Either rename the use case or update the comment to reflect the true behavior.Also, consider tightening the return type from
ApiResponse<Object>toApiResponse<List<GetMidPointResponse>>for compile-time type safety.Proposed fix
/** - * 중간지점 조회 API (동기 버전) + * 중간지점 조회 API (비동기 버전) * * `@param` meetingId 모임 식별자 (UUID) * `@return` 중간지점 번화가 및 추천 장소 목록 */ `@MeetingApiDocumentation.GetMidpointDoc` `@GetMapping`("/{meetingId}/midpoint") - public ApiResponse<Object> getMidpoint(`@PathVariable`("meetingId") UUID meetingId) { + public ApiResponse<List<GetMidPointResponse>> getMidpoint(`@PathVariable`("meetingId") UUID meetingId) {src/main/java/swyp/mingling/external/SeoulMetroClient.java (1)
23-32: Station name normalization is duplicated betweenSeoulMetroClientandSubwayRouteService.The same "역" suffix stripping logic (with the "서울역" exception) exists in both
SeoulMetroClient.searchRoute()(lines 26–32) andSubwayRouteService.normalizeStationName()(lines 116–125), as well asFindStationCoordinateUseCase.excute(). Consider centralizing this into a single utility method to avoid inconsistencies.src/main/java/swyp/mingling/domain/subway/service/SubwayRouteService.java (1)
49-103: 10 retries with fixed 1-second sleep is excessive and lacks backoff.A maximum of 10 retries with a fixed 1-second delay means this method can block the calling thread for up to ~10 seconds on a failing API. This is particularly concerning since the PR description mentions moving toward async patterns.
Consider:
- Reducing
maxRetriesto 3 (a more standard default).- Using exponential backoff (e.g.,
Thread.sleep(1000 * retryCount)) to avoid hammering a struggling server.- Extracting retry parameters to configuration properties so they can be tuned without code changes.
Suggested approach with exponential backoff and reduced retries
- int maxRetries = 10; + int maxRetries = 3; int retryCount = 0; BusinessException lastException = null; while (retryCount < maxRetries) { try { ... } catch (BusinessException e) { ... if (e.getErrorCode() == ErrorCode.SUBWAY_API_SERVER_ERROR) { if (retryCount < maxRetries) { log.warn("API 호출 실패 (시도 {}/{}): {} -> {}. 재시도합니다...", retryCount, maxRetries, startStation, endStation); try { - Thread.sleep(1000); // 1초 대기 + Thread.sleep(1000L * retryCount); // exponential-ish backoff } catch (InterruptedException ie) {
| // 0. 출발역과 도착역이 같은지 체크 (역 이름 정규화 후 비교) | ||
| String normalizedStart = normalizeStationName(startStation); | ||
| String normalizedEnd = normalizeStationName(endStation); | ||
|
|
||
| if (normalizedStart.equals(normalizedEnd)) { | ||
| log.info("출발역과 도착역이 같습니다: {} -> {}, 소요시간: 0분", startStation, endStation); | ||
| return SubwayRouteInfo.builder() | ||
| .startStation(startStation) | ||
| .startStationLine("") | ||
| .endStation(endStation) | ||
| .endStationLine("") | ||
| .totalTravelTime(0) | ||
| .totalDistance(0.0) | ||
| .transferCount(0) | ||
| .transferPath(List.of()) | ||
| .stations(List.of()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
NullPointerException if startStation is null.
normalizeStationName returns null when the input is null (line 117–118). Calling normalizedStart.equals(normalizedEnd) on line 34 will throw NullPointerException if startStation is null.
Proposed fix
- private String normalizeStationName(String stationName) {
- if (stationName == null) {
- return null;
- }
+ private String normalizeStationName(String stationName) {
+ if (stationName == null) {
+ return "";
+ }Or guard the comparison:
- if (normalizedStart.equals(normalizedEnd)) {
+ if (normalizedStart != null && normalizedStart.equals(normalizedEnd)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 0. 출발역과 도착역이 같은지 체크 (역 이름 정규화 후 비교) | |
| String normalizedStart = normalizeStationName(startStation); | |
| String normalizedEnd = normalizeStationName(endStation); | |
| if (normalizedStart.equals(normalizedEnd)) { | |
| log.info("출발역과 도착역이 같습니다: {} -> {}, 소요시간: 0분", startStation, endStation); | |
| return SubwayRouteInfo.builder() | |
| .startStation(startStation) | |
| .startStationLine("") | |
| .endStation(endStation) | |
| .endStationLine("") | |
| .totalTravelTime(0) | |
| .totalDistance(0.0) | |
| .transferCount(0) | |
| .transferPath(List.of()) | |
| .stations(List.of()) | |
| .build(); | |
| } | |
| // 0. 출발역과 도착역이 같은지 체크 (역 이름 정규화 후 비교) | |
| String normalizedStart = normalizeStationName(startStation); | |
| String normalizedEnd = normalizeStationName(endStation); | |
| if (normalizedStart != null && normalizedStart.equals(normalizedEnd)) { | |
| log.info("출발역과 도착역이 같습니다: {} -> {}, 소요시간: 0분", startStation, endStation); | |
| return SubwayRouteInfo.builder() | |
| .startStation(startStation) | |
| .startStationLine("") | |
| .endStation(endStation) | |
| .endStationLine("") | |
| .totalTravelTime(0) | |
| .totalDistance(0.0) | |
| .transferCount(0) | |
| .transferPath(List.of()) | |
| .stations(List.of()) | |
| .build(); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/swyp/mingling/domain/subway/service/SubwayRouteService.java`
around lines 30 - 47, The comparison of normalizedStart and normalizedEnd in
SubwayRouteService can NPE when normalizeStationName returns null (e.g.,
startStation null); fix by guarding nulls before calling equals — either
null-check the raw inputs at the start of the method or use a null-safe
comparison (e.g., Objects.equals(normalizeStationName(startStation),
normalizeStationName(endStation))). Update the early-return block that builds
SubwayRouteInfo when stations match to rely on the null-safe check and ensure
normalizeStationName is only invoked in a safe way (or validate
startStation/endStation and throw/handle appropriately) so
normalizedStart.equals(...) cannot be called on a null.
| if (response != null && response.getErrorCode() != null) { | ||
| log.info("API 응답 코드: {}, 메시지: {}", response.getErrorCode(), response.getErrorMessage()); | ||
| log.info("경로 데이터 개수: {}", response.getPathInfoList().size()); | ||
| log.info("Body 존재 여부: {}", response.getBody() != null); | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException on response.getPathInfoList().size().
Line 61 calls response.getPathInfoList().size() inside a block guarded only by response.getErrorCode() != null. If getPathInfoList() returns null (e.g., on an error response with an error code but no path data), this will throw NullPointerException.
Proposed fix
if (response != null && response.getErrorCode() != null) {
log.info("API 응답 코드: {}, 메시지: {}", response.getErrorCode(), response.getErrorMessage());
- log.info("경로 데이터 개수: {}", response.getPathInfoList().size());
+ log.info("경로 데이터 개수: {}", response.getPathInfoList() != null ? response.getPathInfoList().size() : 0);
log.info("Body 존재 여부: {}", response.getBody() != null);
}🤖 Prompt for AI Agents
In `@src/main/java/swyp/mingling/domain/subway/service/SubwayRouteService.java`
around lines 59 - 63, The log block in SubwayRouteService uses
response.getPathInfoList().size() without ensuring getPathInfoList() is
non-null, risking NPE; update the logging to null-check
response.getPathInfoList() (or use Optional) before calling size and log a safe
value (e.g., 0 or "null") when the list is null, keeping the existing checks for
response and response.getErrorCode(); ensure you update the same logging code
that references response.getPathInfoList() and response.getBody().
| .onStatus( | ||
| status -> status.is5xxServerError(), | ||
| clientResponse -> { | ||
| log.error("공공 데이터 API 서버 오류 발생 - HTTP 상태: {}", clientResponse.statusCode()); | ||
| return clientResponse.bodyToMono(String.class) | ||
| .doOnNext(body -> log.error("API 에러 응답 body: {}", body)) | ||
| .then(Mono.error(new RuntimeException("공공 데이터 포털 API 서버 오류"))); |
There was a problem hiding this comment.
RuntimeException from 5xx handler bypasses the retry loop in SubwayRouteService.
The onStatus handler throws RuntimeException on 5xx errors (line 58), but the retry loop in SubwayRouteService.getRoute() only catches BusinessException. This means HTTP 5xx errors will propagate as uncaught RuntimeException and will not be retried, which directly contradicts the purpose of this PR.
Either throw BusinessException(ErrorCode.SUBWAY_API_SERVER_ERROR) here, or widen the catch in SubwayRouteService to also handle RuntimeException from the client.
Proposed fix — throw BusinessException
clientResponse -> {
log.error("공공 데이터 API 서버 오류 발생 - HTTP 상태: {}", clientResponse.statusCode());
return clientResponse.bodyToMono(String.class)
.doOnNext(body -> log.error("API 에러 응답 body: {}", body))
- .then(Mono.error(new RuntimeException("공공 데이터 포털 API 서버 오류")));
+ .then(Mono.error(new BusinessException(ErrorCode.SUBWAY_API_SERVER_ERROR)));
}🤖 Prompt for AI Agents
In `@src/main/java/swyp/mingling/external/SeoulMetroClient.java` around lines 52 -
58, The onStatus handler in SeoulMetroClient currently returns Mono.error(new
RuntimeException(...)) which bypasses the retry in
SubwayRouteService.getRoute(); change the error produced in the 5xx branch to a
BusinessException with the appropriate error code (e.g.,
BusinessException(ErrorCode.SUBWAY_API_SERVER_ERROR)) so that
SubwayRouteService.getRoute() can catch and retry, preserving the existing log
lines in the handler and ensuring the emitted Mono.error carries the
BusinessException instead of RuntimeException.
| try { | ||
| // JSON 파싱 시도 | ||
| return Mono.just(new com.fasterxml.jackson.databind.ObjectMapper() | ||
| .readValue(rawBody, SeoulMetroRouteResponse.class)); | ||
| } catch (Exception e) { | ||
| log.error("JSON 파싱 실패: {}", e.getMessage()); | ||
| return Mono.just(new SeoulMetroRouteResponse()); | ||
| } |
There was a problem hiding this comment.
New ObjectMapper allocated on every call; parse failure silently returns empty response.
Two issues here:
-
A new
ObjectMapper()is instantiated on every API call.ObjectMapperis thread-safe and expensive to construct — inject it as a field or use astatic finalinstance. -
On parse failure (line 70), returning an empty
SeoulMetroRouteResponsesilently hides the real error. Downstream validation may eventually throwSUBWAY_API_SERVER_ERROR(if header/body are null), but this is indirect and misleading — the root cause (malformed response) is lost. Consider propagating the error viaMono.error(...)instead.
Proposed fix
Add ObjectMapper as a field:
public class SeoulMetroClient {
private final WebClient webClient;
+ private final ObjectMapper objectMapper;
- public SeoulMetroClient(`@Qualifier`("seoulMetroWebClient") WebClient webClient) {
+ public SeoulMetroClient(`@Qualifier`("seoulMetroWebClient") WebClient webClient, ObjectMapper objectMapper) {
this.webClient = webClient;
+ this.objectMapper = objectMapper;
}In the flatMap, propagate parse errors:
.flatMap(rawBody -> {
try {
- return Mono.just(new com.fasterxml.jackson.databind.ObjectMapper()
- .readValue(rawBody, SeoulMetroRouteResponse.class));
+ return Mono.just(objectMapper.readValue(rawBody, SeoulMetroRouteResponse.class));
} catch (Exception e) {
log.error("JSON 파싱 실패: {}", e.getMessage());
- return Mono.just(new SeoulMetroRouteResponse());
+ return Mono.error(new BusinessException(ErrorCode.SUBWAY_API_SERVER_ERROR));
}
})🤖 Prompt for AI Agents
In `@src/main/java/swyp/mingling/external/SeoulMetroClient.java` around lines 64 -
71, Replace per-call ObjectMapper construction with a shared instance and stop
hiding parse failures: introduce a single ObjectMapper for SeoulMetroClient
(e.g., a private static final ObjectMapper or injected field) and in the block
that currently does new ObjectMapper().readValue(...,
SeoulMetroRouteResponse.class) change the catch path to propagate the parsing
failure via Mono.error(...) (after logging the error with the exception) instead
of returning Mono.just(new SeoulMetroRouteResponse()); update references to
ObjectMapper, SeoulMetroRouteResponse and the parsing block so the client uses
the shared mapper and parsing errors surface to callers.
| @TestPropertySource(properties = { | ||
| "seoul.metro.api-key=지하철 API 키 입력" | ||
| "seoul.metro.api-key=4a6342524f7373793633547a456576" | ||
| }) |
There was a problem hiding this comment.
Hardcoded API key committed to source control — credential leak.
Line 23 contains what appears to be a real Seoul Metro API key in plain text. This key is now part of the repository history. Even if removed in a future commit, it remains recoverable via git log.
Actions needed:
- Rotate this API key immediately — it is compromised once pushed.
- Use environment variables or a secrets manager (e.g.,
${SEOUL_METRO_API_KEY}) and reference them in test properties. - Consider adding this test class to a dedicated profile (e.g.,
integration) so it doesn't run in normal CI builds.
Proposed fix — use environment variable
`@TestPropertySource`(properties = {
- "seoul.metro.api-key=4a6342524f7373793633547a456576"
+ "seoul.metro.api-key=${SEOUL_METRO_API_KEY:test-placeholder}"
})🤖 Prompt for AI Agents
In
`@src/test/java/swyp/mingling/domain/subway/service/SubwayRouteServiceTest.java`
around lines 22 - 24, SubwayRouteServiceTest currently hardcodes a Seoul Metro
API key in the `@TestPropertySource`; rotate the compromised key immediately, then
remove the literal and change the test to reference an environment variable or
secret (e.g., replace the value with a placeholder like ${SEOUL_METRO_API_KEY})
so the key is injected at runtime, and scope the test to an integration profile
(e.g., annotate or configure the test class SubwayRouteServiceTest to run only
under an "integration" profile) so it won't run in normal CI; ensure any local
CI/test configs document how to supply the SEOUL_METRO_API_KEY from environment
or secrets manager.
2 participants
PR 제목
작업 유형
작업 내용
체크리스트
참고 사항 (선택)
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores