[FEAT] TOOL Refresh API 추가 #37
Conversation
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit릴리스 노트
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughPOST /v1/mcp/tools/refresh 엔드포인트가 ChatController에 추가되어 McpToolService.refreshTools()를 호출하고 204 No Content를 반환합니다. 신규 McpToolService는 SyncMcpToolCallbackProvider.invalidateCache()를 호출해 MCP 도구 캐시를 무효화합니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ChatController
participant McpToolService
participant SyncMcpToolCallbackProvider
Note over Client,ChatController: POST /v1/mcp/tools/refresh
Client->>ChatController: POST /v1/mcp/tools/refresh
ChatController->>McpToolService: refreshTools()
McpToolService->>SyncMcpToolCallbackProvider: invalidateCache()
SyncMcpToolCallbackProvider-->>McpToolService: cache invalidated
McpToolService-->>ChatController: return
ChatController-->>Client: 204 No Content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
chat/src/main/java/core/chat/controller/ChatController.java(3 hunks)mcp-client/src/main/java/core/mcpclient/service/McpToolService.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-20T07:10:22.381Z
Learnt from: kseysh
Repo: kseysh/factory-chatbot PR: 24
File: chat/src/main/java/core/chat/service/ChatFacade.java:96-100
Timestamp: 2025-11-20T07:10:22.381Z
Learning: In chat/src/main/java/core/chat/service/ChatFacade.java, the `checkUserCanDeleteRoom` method returns `orElse(true)` when a chat room doesn't exist. This is intentional to ensure idempotency of the DELETE operation - allowing deletion of non-existent rooms to succeed silently, treating DELETE as a declaration of desired end state rather than an action.
Applied to files:
chat/src/main/java/core/chat/controller/ChatController.java
🔇 Additional comments (5)
chat/src/main/java/core/chat/controller/ChatController.java (2)
16-16: LGTM!필요한 의존성 import가 올바르게 추가되었습니다.
30-30: LGTM!@requiredargsconstructor를 통한 생성자 주입이 올바르게 적용되었습니다.
mcp-client/src/main/java/core/mcpclient/service/McpToolService.java (3)
1-5: LGTM!패키지 선언과 import 문이 적절합니다. 필요한 의존성만 포함되어 있습니다.
7-9: LGTM!Spring @service 어노테이션과 Lombok @requiredargsconstructor를 적절히 사용하여 생성자 기반 의존성 주입을 구현했습니다.
11-11: No action needed - invalidateCache() is the correct Spring AI MCP APIThe
SyncMcpToolCallbackProvider.invalidateCache()method is the documented and intended API for invalidating cached tool callbacks and forcing re-discovery on the next request. This is the correct approach for refreshing the tool list.
| @PostMapping("/v1/mcp/tools/refresh") | ||
| public ResponseEntity<Void> refreshTools() { | ||
| mcpToolService.refreshTools(); | ||
| return ResponseEntity.ok().build(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
응답 코드 일관성 개선 고려
본문이 없는 작업에 대해 deleteChatRoom 엔드포인트(Line 124)는 ResponseEntity.noContent().build() (204)를 사용하는 반면, 이 엔드포인트는 ResponseEntity.ok().build() (200)를 사용합니다. 일관성을 위해 동일한 접근 방식을 사용하는 것을 고려하세요.
일관성을 위한 diff:
@PostMapping("/v1/mcp/tools/refresh")
public ResponseEntity<Void> refreshTools(@UserId String userId) {
mcpToolService.refreshTools();
- return ResponseEntity.ok().build();
+ return ResponseEntity.noContent().build();
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In chat/src/main/java/core/chat/controller/ChatController.java around lines 127
to 131, the POST /v1/mcp/tools/refresh endpoint currently returns
ResponseEntity.ok().build() (200) for a bodyless operation while similar
bodyless endpoints use ResponseEntity.noContent().build() (204); change the
return to ResponseEntity.noContent().build() so the response code is consistent
with other bodyless operations.
🧩 Analysis chain
🏁 Script executed:
cat -n chat/src/main/java/core/chat/controller/ChatController.java | head -160Repository: kseysh/factory-chatbot
Length of output: 6854
🏁 Script executed:
rg -n --type=java '@PreAuthorize|@Secured|@RolesAllowed|hasRole|ROLE_' -A 2 | head -80Repository: kseysh/factory-chatbot
Length of output: 48
🏁 Script executed:
rg -n --type=java '@UserId' -A 2 -B 2 | head -30Repository: kseysh/factory-chatbot
Length of output: 2792
🏁 Script executed:
find . -type f -name "*Security*" -o -name "*Auth*" -o -name "*Filter*" | grep -E "\.(java|xml|properties|yml)$" | head -20Repository: kseysh/factory-chatbot
Length of output: 48
🏁 Script executed:
find . -type f -name "*.java" -path "*global/auth*" | head -20Repository: kseysh/factory-chatbot
Length of output: 181
🏁 Script executed:
rg -n "class UserId|interface UserId|@interface UserId" --type=javaRepository: kseysh/factory-chatbot
Length of output: 289
🏁 Script executed:
cat -n ./chat/src/main/java/core/global/auth/UserId.javaRepository: kseysh/factory-chatbot
Length of output: 427
🏁 Script executed:
cat -n ./chat/src/main/java/core/global/auth/UserIdArgumentResolver.javaRepository: kseysh/factory-chatbot
Length of output: 1483
🏁 Script executed:
rg -n "class McpToolService|def refreshTools|void refreshTools" --type=java -A 5 | head -50Repository: kseysh/factory-chatbot
Length of output: 835
🏁 Script executed:
find . -type f -name "McpToolService.java"Repository: kseysh/factory-chatbot
Length of output: 135
Missing authentication on admin endpoint
The refreshTools() endpoint lacks the @UserId annotation required by all other business logic endpoints in this controller. Since mcpToolService.refreshTools() invalidates the tool cache for all users, this is a system-level admin operation that must require authentication.
Without @UserId, this endpoint accepts unauthenticated requests, allowing anyone to:
- Invalidate the MCP tool cache
- Force unnecessary re-syncs with MCP servers
- Potentially cause DoS via repeated calls
Add the @UserId annotation to restrict access to authenticated users:
@PostMapping("/v1/mcp/tools/refresh")
-public ResponseEntity<Void> refreshTools() {
+public ResponseEntity<Void> refreshTools(@UserId String userId) {
mcpToolService.refreshTools();
return ResponseEntity.ok().build();
}Additionally, consider whether this operation should be further restricted to admin users via role-based access control if the codebase adopts such mechanisms in the future.
📝 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.
| @PostMapping("/v1/mcp/tools/refresh") | |
| public ResponseEntity<Void> refreshTools() { | |
| mcpToolService.refreshTools(); | |
| return ResponseEntity.ok().build(); | |
| } | |
| @PostMapping("/v1/mcp/tools/refresh") | |
| public ResponseEntity<Void> refreshTools(@UserId String userId) { | |
| mcpToolService.refreshTools(); | |
| return ResponseEntity.ok().build(); | |
| } |
🤖 Prompt for AI Agents
In chat/src/main/java/core/chat/controller/ChatController.java around lines
127-131, the refreshTools() admin endpoint is missing the @UserId annotation so
it accepts unauthenticated calls; add a @UserId-annotated parameter (e.g. add a
method param like @UserId String userId) to the controller method signature so
the framework enforces authentication before calling
mcpToolService.refreshTools(), and if the service API requires it, forward the
userId to the service; optionally annotate or guard the method with any existing
role-based check (admin role) if such RBAC is available in the codebase.
| @RequiredArgsConstructor | ||
| @Service | ||
| public class McpToolService { | ||
|
|
||
| private final SyncMcpToolCallbackProvider syncMcpToolCallbackProvider; | ||
|
|
||
| public void refreshTools() { | ||
| syncMcpToolCallbackProvider.invalidateCache(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
JavaDoc 추가 고려
클래스와 메서드에 대한 문서화를 추가하면 코드 가독성과 유지보수성이 향상됩니다.
+/**
+ * MCP 도구 관련 서비스
+ * MCP 서버로부터 도구 목록을 갱신하는 기능을 제공합니다.
+ */
@RequiredArgsConstructor
@Service
public class McpToolService {
private final SyncMcpToolCallbackProvider syncMcpToolCallbackProvider;
+ /**
+ * MCP 도구 캐시를 무효화하여 다음 요청 시 서버로부터 도구 목록을 재조회하도록 합니다.
+ */
public void refreshTools() {
syncMcpToolCallbackProvider.invalidateCache();
}
}🤖 Prompt for AI Agents
In mcp-client/src/main/java/core/mcpclient/service/McpToolService.java around
lines 7 to 15, the class and its public method lack JavaDoc; add concise JavaDoc
above the class describing its responsibility (a Spring @Service that refreshes
MCP tools by invalidating the SyncMcpToolCallbackProvider cache) and add JavaDoc
above refreshTools() describing the action it performs and side-effects
(invalidates provider cache), include @author or @since tags if your project
uses them, and document that the method has no parameters and returns void.
| public void refreshTools() { | ||
| syncMcpToolCallbackProvider.invalidateCache(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
에러 처리와 로깅 추가 권장
refreshTools() 메서드에 다음 개선사항을 적용하는 것을 권장합니다:
- 에러 처리:
invalidateCache()호출 시 발생할 수 있는 예외를 처리하지 않아, 예외 발생 시 호출자에게 그대로 전파됩니다. - 로깅: 도구 목록 갱신 시점을 추적할 수 있는 로그가 없어 모니터링 및 디버깅이 어렵습니다.
- 피드백: void 반환 타입으로 인해 작업 성공 여부를 호출자가 알 수 없습니다.
다음과 같이 개선할 수 있습니다:
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
@RequiredArgsConstructor
@Service
public class McpToolService {
private final SyncMcpToolCallbackProvider syncMcpToolCallbackProvider;
public void refreshTools() {
+ try {
+ log.info("MCP 도구 캐시 무효화 시작");
syncMcpToolCallbackProvider.invalidateCache();
+ log.info("MCP 도구 캐시 무효화 완료");
+ } catch (Exception e) {
+ log.error("MCP 도구 캐시 무효화 실패", e);
+ throw e;
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mcp-client/src/main/java/core/mcpclient/service/McpToolService.java around
lines 13 to 15, refreshTools() currently calls
syncMcpToolCallbackProvider.invalidateCache() with no try/catch, no logging, and
returns void; change it to catch and handle exceptions thrown by
invalidateCache(), log start/success/failure with contextual messages (including
exception details on failure), and change the method to return a boolean (or an
appropriate status) indicating success/failure so callers can react; ensure the
catch logs the exception and returns false, successful path logs and returns
true.
🧩 Analysis chain
🏁 Script executed:
# Find SyncMcpToolCallbackProvider and understand its invalidateCache implementation
fd -t f "SyncMcpToolCallbackProvider" --type f
# Also search for the class definition
rg -l "class SyncMcpToolCallbackProvider" -t javaRepository: kseysh/factory-chatbot
Length of output: 48
🏁 Script executed:
# List the structure of mcp-client
find mcp-client -type f -name "*.java" | head -20
# Search for invalidateCache method
rg -n "invalidateCache" -t java
# Search for refreshTools usage
rg -n "refreshTools" -t javaRepository: kseysh/factory-chatbot
Length of output: 1447
🏁 Script executed:
# Read McpToolService.java
cat -n mcp-client/src/main/java/core/mcpclient/service/McpToolService.java
# Read ChatController.java to see the REST endpoint
cat -n chat/src/main/java/core/chat/controller/ChatController.java | head -150Repository: kseysh/factory-chatbot
Length of output: 7003
🏁 Script executed:
# Find the build configuration to see Spring AI version
find . -name "pom.xml" -o -name "build.gradle" -o -name "build.gradle.kts" | head -5
# Check if there's a dependency file that shows Spring AI version
rg -n "spring-ai|SyncMcpToolCallbackProvider" --type xml --type gradleRepository: kseysh/factory-chatbot
Length of output: 714
🏁 Script executed:
# Check root build.gradle for Spring AI version
cat -n build.gradle | grep -A 5 -B 5 "springAiVersion"
# Also check for gradle.properties
cat -n gradle.properties 2>/dev/null || echo "No gradle.properties"Repository: kseysh/factory-chatbot
Length of output: 634
🌐 Web query:
Spring AI 1.1.0 SyncMcpToolCallbackProvider invalidateCache thread-safe
💡 Result:
Short answer: invalidateCache() on SyncMcpToolCallbackProvider is not documented as thread‑safe. The Javadoc merely exposes invalidateCache() (no threading guarantees) — so treat it as not thread‑safe and synchronize externally (or call from a single thread / the ApplicationEvent thread). [1][2]
References
- SyncMcpToolCallbackProvider API (1.1.x). [1]
- SyncMcpToolCallbackProvider API (1.1.0‑M2 / 1.1.0 docs). [2]
Add synchronization to prevent concurrent cache invalidation issues
invalidateCache() in Spring AI 1.1.0's SyncMcpToolCallbackProvider is not documented as thread-safe. Since refreshTools() is exposed as a public REST endpoint at /v1/mcp/tools/refresh, concurrent HTTP requests can call this method simultaneously, potentially causing cache corruption or race conditions. Synchronize access to invalidateCache() using either a synchronized block, ReentrantLock, or a Spring-managed mechanism like @Synchronized.
🤖 Prompt for AI Agents
In mcp-client/src/main/java/core/mcpclient/service/McpToolService.java around
lines 13 to 15, refreshTools() directly calls
syncMcpToolCallbackProvider.invalidateCache() without any synchronization which
can lead to race conditions under concurrent HTTP requests; modify the class to
serialize access to invalidateCache() — for example add a private final
ReentrantLock cacheLock and wrap the call in cacheLock.lock()/try {
syncMcpToolCallbackProvider.invalidateCache(); } finally { cacheLock.unlock(); }
(alternatively mark the method synchronized or use a Spring @Synchronized on the
method) to ensure only one thread invalidates the cache at a time and always
release the lock in a finally block.
Related issue 🛠
Work Description ✏️