feat: STOMP Config 추가 #400
Conversation
- build.gradle 의존성 추가 - ErrorCode 추가 - config 작성
|
""" Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20분
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
build.gradle (1)
67-68: WebSocket 및 STOMP 의존성 추가가 적절합니다!
spring-boot-starter-websocket: Spring Boot의 WebSocket 지원을 위한 핵심 의존성stomp-websocket: STOMP 프로토콜 클라이언트 기능 제공다만
stomp-websocket의존성에 버전을 명시하는 것을 권장합니다:- implementation 'org.webjars:stomp-websocket' + implementation 'org.webjars:stomp-websocket:2.3.4'src/main/java/com/example/solidconnection/chat/config/StompEventListener.java (1)
13-34: STOMP 세션 이벤트 리스너 구현이 깔끔합니다!
ConcurrentHashMap.newKeySet()사용으로 스레드 안전성 확보- 연결/해제 이벤트 로깅으로 세션 상태 추적 가능
- Spring 이벤트 리스너 패턴을 올바르게 활용
세션 관리 기능을 확장할 경우를 대비해 다음과 같은 개선사항을 고려해볼 수 있습니다:
// 세션 정보를 더 상세히 관리하고 싶다면 private final Map<String, SessionInfo> sessionDetails = new ConcurrentHashMap<>(); // 또는 세션 정리 스케줄러 추가 @Scheduled(fixedRate = 300000) // 5분마다 public void cleanupStaleSession() { // 비활성 세션 정리 로직 }src/main/java/com/example/solidconnection/chat/config/StompWebSocketConfig.java (1)
31-45: 스레드 풀 설정에 대한 검토가 필요합니다현재 스레드 풀 설정:
- Inbound: 코어 6, 최대 12, 큐 용량 1000
- Outbound: 코어 6, 최대 12, 큐 용량 미지정
다음 사항을 고려하세요:
- 예상 동시 접속자 수에 따른 적절한 크기 산정
- Outbound 채널의 큐 용량 설정 추가
- PR에서 언급된 10MB 이미지 제한과의 연관성
부하 테스트를 통해 적절한 값을 산정하고, Outbound 채널에도 큐 용량을 설정하세요:
registration.taskExecutor() .corePoolSize(6) - .maxPoolSize(12); + .maxPoolSize(12) + .queueCapacity(1000);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build.gradle(1 hunks)src/main/java/com/example/solidconnection/chat/config/StompEventListener.java(1 hunks)src/main/java/com/example/solidconnection/chat/config/StompHandler.java(1 hunks)src/main/java/com/example/solidconnection/chat/config/StompWebSocketConfig.java(1 hunks)src/main/java/com/example/solidconnection/chat/config/WebSocketConfig.java(1 hunks)src/main/java/com/example/solidconnection/chat/config/WebSocketHandler.java(1 hunks)src/main/java/com/example/solidconnection/common/exception/ErrorCode.java(1 hunks)
🔇 Additional comments (5)
src/main/java/com/example/solidconnection/common/exception/ErrorCode.java (1)
114-116: WebSocket 구독 권한 에러 코드 추가가 적절합니다!
- HTTP 403 상태 코드 사용이 구독 권한 거부에 적합함
- 한글 메시지가 명확하고 일관성 있음
- 새로운 "socket" 섹션으로 체계적으로 분류됨
STOMP 핸들러에서의 JWT 토큰 검증 실패 시 활용될 것으로 보입니다.
src/main/java/com/example/solidconnection/chat/config/WebSocketHandler.java (2)
17-23: 세션 관리 구현이 적절합니다!
- 스레드 안전한
ConcurrentHashMap.newKeySet()사용- 연결 시 세션 추가 및 로깅 처리가 올바름
- 세션 ID 로깅으로 디버깅 지원
36-41: 연결 해제 처리가 깔끔합니다!세션 정리와 상태 로깅이 적절하게 구현되어 있습니다. CloseStatus 정보까지 로깅하여 연결 해제 원인 추적이 가능합니다.
src/main/java/com/example/solidconnection/chat/config/WebSocketConfig.java (2)
11-16: WebSocket 설정 클래스 구조가 적절합니다!
@Configuration과@EnableWebSocket어노테이션 올바른 사용WebSocketConfigurer인터페이스 구현으로 표준 패턴 준수- 의존성 주입을 통한 핸들러 연결이 깔끔함
24-31: 실제 이미지 전송 요구사항 및 메모리 영향 검토 요청.
현재 WebSocket 버퍼 크기가 10MB로 하드코딩되어 있습니다.
이 크기가 이미지 전송을 위한 실제 요구사항을 충족하는지 확인해 주세요.
동시에 여러 연결이 발생했을 때 전체 메모리 사용량이 어떻게 변하는지 검토 부탁드립니다.
큰 메시지 전송 시 네트워크 타임아웃이나 연결 불안정 현상이 발생하지 않는지도 함께 살펴봐 주세요.
운영 환경별로 유연하게 조정할 수 있도록 설정 값을 외부화하는 것을 추천합니다.변경 가이드라인을 아래에 정리합니다.
- 실제 요구사항 확인
- 이미지 1회 전송 시 최대 크기를 측정합니다.- 메모리 사용량 분석
- 동시 연결 수 × 버퍼 크기를 기반으로 예상 메모리 사용량을 계산합니다.- 네트워크 안정성 테스트
- 최대 크기 메시지 전송 시 타임아웃 또는 연결 끊김 여부를 점검합니다.- 설정 외부화
java @Value("${websocket.max-text-buffer-size:1048576}") // 1MB 기본값 private int maxTextBufferSize; @Value("${websocket.max-binary-buffer-size:1048576}") private int maxBinaryBufferSize; @Value("${websocket.session-timeout:300000}") private long sessionTimeout;
| if(StompCommand.CONNECT == accessor.getCommand()){ | ||
| log.info("connect요청시 토큰 유효성 검증"); | ||
|
|
||
| String bearerToken = accessor.getFirstNativeHeader("Authorization"); | ||
|
|
||
| if(bearerToken == null || !bearerToken.startsWith("Bearer ")) { | ||
| throw new IllegalArgumentException("Authorization header missing or invalid format"); | ||
| } | ||
|
|
||
| String token = bearerToken.substring(7); | ||
|
|
||
| Claims claims = jwtTokenProvider.parseClaims(token); | ||
|
|
||
| log.info("토큰 검증 성공, 유저 인증 완료 - 사용자: {}", claims.getSubject()); | ||
| } |
There was a problem hiding this comment.
인증 정보를 SecurityContext에 저장해야 합니다
CONNECT 시 JWT 토큰 검증은 성공적으로 수행되고 있지만, 인증된 사용자 정보를 Spring Security의 SecurityContext나 STOMP 세션에 저장하지 않고 있습니다. 이로 인해 후속 메시지 처리 시 사용자 컨텍스트를 활용할 수 없습니다.
다음과 같이 인증 정보를 저장하는 것을 권장합니다:
Claims claims = jwtTokenProvider.parseClaims(token);
log.info("토큰 검증 성공, 유저 인증 완료 - 사용자: {}", claims.getSubject());
+
+// 사용자 정보를 STOMP 세션 속성에 저장
+accessor.getSessionAttributes().put("userEmail", claims.getSubject());
+accessor.setUser(() -> claims.getSubject());📝 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.
| if(StompCommand.CONNECT == accessor.getCommand()){ | |
| log.info("connect요청시 토큰 유효성 검증"); | |
| String bearerToken = accessor.getFirstNativeHeader("Authorization"); | |
| if(bearerToken == null || !bearerToken.startsWith("Bearer ")) { | |
| throw new IllegalArgumentException("Authorization header missing or invalid format"); | |
| } | |
| String token = bearerToken.substring(7); | |
| Claims claims = jwtTokenProvider.parseClaims(token); | |
| log.info("토큰 검증 성공, 유저 인증 완료 - 사용자: {}", claims.getSubject()); | |
| } | |
| if (StompCommand.CONNECT == accessor.getCommand()) { | |
| log.info("connect요청시 토큰 유효성 검증"); | |
| String bearerToken = accessor.getFirstNativeHeader("Authorization"); | |
| if (bearerToken == null || !bearerToken.startsWith("Bearer ")) { | |
| throw new IllegalArgumentException("Authorization header missing or invalid format"); | |
| } | |
| String token = bearerToken.substring(7); | |
| Claims claims = jwtTokenProvider.parseClaims(token); | |
| log.info("토큰 검증 성공, 유저 인증 완료 - 사용자: {}", claims.getSubject()); | |
| // 사용자 정보를 STOMP 세션 속성에 저장 | |
| accessor.getSessionAttributes().put("userEmail", claims.getSubject()); | |
| accessor.setUser(() -> claims.getSubject()); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/solidconnection/chat/config/StompHandler.java
around lines 30 to 44, after successfully validating the JWT token, you need to
store the authenticated user information in the Spring Security SecurityContext.
To fix this, create an Authentication object using the user details extracted
from the token claims, then set this Authentication into the
SecurityContextHolder. This ensures that the user context is available for
subsequent message handling.
src/main/java/com/example/solidconnection/chat/config/StompHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/chat/config/StompHandler.java
Outdated
Show resolved
Hide resolved
| String roomId = destination.split("/")[2]; | ||
| log.info("사용자 {} 가 룸 {} 구독 시도", email, roomId); | ||
|
|
||
| // todo: room 검증로직 구현 |
There was a problem hiding this comment.
TODO: 채팅방 접근 권한 검증 구현 필수
채팅방 구독 시 사용자가 해당 채팅방에 접근 권한이 있는지 검증하는 로직이 누락되어 있습니다. 이는 보안상 중요한 이슈입니다.
채팅방 접근 권한 검증 로직을 구현하시겠습니까? 아니면 이를 추적하기 위한 새 이슈를 생성할까요?
🤖 Prompt for AI Agents
In src/main/java/com/example/solidconnection/chat/config/StompHandler.java at
line 67, the code lacks the implementation for verifying if a user has access
rights to a chat room upon subscription. To fix this, implement a validation
method that checks the user's permissions against the chat room ID before
allowing the subscription to proceed. This method should be called at the
indicated line to enforce access control and prevent unauthorized access.
src/main/java/com/example/solidconnection/chat/config/StompWebSocketConfig.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public void configureMessageBroker(MessageBrokerRegistry registry) { | ||
| registry.setApplicationDestinationPrefixes("/publish"); | ||
| registry.enableSimpleBroker("/topic") | ||
| .setHeartbeatValue(new long[]{15000, 15000}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
메시지 크기 제한 설정이 필요합니다
PR에서 언급된 10MB 이미지 크기 제한이 여기에 구성되어 있지 않습니다. STOMP 메시지 크기 제한을 설정해야 합니다.
다음과 같이 메시지 크기 제한을 추가하세요:
@Override
public void configureMessageBroker(MessageBrokerRegistry registry) {
registry.setApplicationDestinationPrefixes("/publish");
registry.enableSimpleBroker("/topic")
- .setHeartbeatValue(new long[]{15000, 15000});
+ .setHeartbeatValue(new long[]{15000, 15000})
+ .setTaskScheduler(taskScheduler()); // 필요시 스케줄러 추가
+
+ // 메시지 크기 제한 설정 (10MB)
+ registry.setCacheLimit(1024 * 1024 * 10);
}또한 WebSocketTransportRegistration을 통해 더 세밀한 제어가 가능합니다:
@Override
public void configureWebSocketTransport(WebSocketTransportRegistration registration) {
registration.setMessageSizeLimit(10 * 1024 * 1024); // 10MB
registration.setSendBufferSizeLimit(10 * 1024 * 1024);
registration.setSendTimeLimit(20 * 1000); // 20초
}🤖 Prompt for AI Agents
In
src/main/java/com/example/solidconnection/chat/config/StompWebSocketConfig.java
around lines 24 to 29, the STOMP message size limit is not configured, which is
necessary to enforce the 10MB image size restriction mentioned in the PR. Add a
configureWebSocketTransport method to the class that sets the message size limit
and send buffer size limit to 10MB (10 * 1024 * 1024 bytes) and the send time
limit to 20 seconds (20 * 1000 milliseconds) using the
WebSocketTransportRegistration parameter.
| registry.addHandler(webSocketHandler, "/connect") | ||
| .setAllowedOriginPatterns("*"); // todo: 사용하는 도메인으로 한정 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Origin 패턴 보안 설정을 우선적으로 개선해야 합니다.
현재 모든 Origin을 허용하는 설정("*")은 보안상 위험합니다:
- CSRF 공격 위험성
- 의도하지 않은 도메인에서의 WebSocket 연결 허용
- 프로덕션 환경에서 심각한 보안 취약점
다음과 같이 개선을 권장합니다:
// 개발 환경과 프로덕션 환경을 분리
@Value("${websocket.allowed-origins:http://localhost:3000,https://yourdomain.com}")
private String[] allowedOrigins;
@Override
public void registerWebSocketHandlers(WebSocketHandlerRegistry registry) {
registry.addHandler(webSocketHandler, "/connect")
.setAllowedOrigins(allowedOrigins);
}🤖 Prompt for AI Agents
In src/main/java/com/example/solidconnection/chat/config/WebSocketConfig.java
around lines 20 to 22, the WebSocket handler currently allows all origins with
setAllowedOriginPatterns("*"), which poses security risks. Replace this with a
configurable allowedOrigins array injected via @Value from application
properties, and use setAllowedOrigins(allowedOrigins) instead. This change
restricts allowed origins to trusted domains, improving security by preventing
CSRF and unauthorized WebSocket connections.
| protected void handleTextMessage(WebSocketSession session, TextMessage message) throws Exception { | ||
| String payload = message.getPayload(); | ||
| log.info("Received message : {}", payload); | ||
| for(WebSocketSession s : sessions) { | ||
| if(s.isOpen()) { | ||
| s.sendMessage(new TextMessage(payload)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
메시지 브로드캐스팅 로직에 대한 검토가 필요합니다.
현재 구현은 모든 연결된 세션에 메시지를 브로드캐스팅하는데, 이는 다음과 같은 우려사항이 있습니다:
- 보안 및 프라이버시: 모든 사용자가 다른 사용자의 메시지를 받을 수 있음
- 성능: 대량의 세션이 있을 때 성능 저하 가능성
- 비즈니스 로직: 실제 채팅 애플리케이션에서는 특정 방이나 사용자에게만 메시지 전송이 필요
다음과 같은 개선을 권장합니다:
// 예시: 룸 기반 메시징
private final Map<String, Set<WebSocketSession>> roomSessions = new ConcurrentHashMap<>();
protected void handleTextMessage(WebSocketSession session, TextMessage message) throws Exception {
try {
String payload = message.getPayload();
log.info("Received message : {}", payload);
// JSON 파싱하여 대상 룸이나 사용자 확인
// 권한 검증 로직 추가
// 특정 대상에게만 메시지 전송
} catch (Exception e) {
log.error("Error handling message", e);
// 에러 응답 전송
}
}🤖 Prompt for AI Agents
In src/main/java/com/example/solidconnection/chat/config/WebSocketHandler.java
around lines 26 to 34, the current handleTextMessage method broadcasts incoming
messages to all connected sessions, which raises security, privacy, and
performance concerns. Refactor this method to implement room-based messaging by
maintaining a concurrent map of room IDs to session sets. Parse the incoming
message payload to identify the target room or user, add authorization checks to
ensure message delivery only to permitted recipients, and send the message only
to sessions within the specified room. Also, add exception handling to log
errors and send error responses as needed.
There was a problem hiding this comment.
저도 이게 궁금했어요. 전체 session 에 대해서 for 문을 돌면
서버와 연결된 모든 세션에 브로드케스트될 것 같아요.
제가 잘못이해하는 부분이 있다면 말씀해주세요.!
There was a problem hiding this comment.
@nayonsoso 맞습니다! 이해하신 것이 정확해요.
현재 코드의 handleTextMessage 메서드에서:
for(WebSocketSession s : sessions) {
if(s.isOpen()) {
s.sendMessage(new TextMessage(payload));
}
}이 부분은 정말로 서버에 연결된 모든 세션에 브로드캐스트하고 있어요.
즉, 사용자 A가 메시지를 보내면 → 서버에 연결된 사용자 B, C, D... 모든 사용자가 그 메시지를 받게 되는 구조입니다.
1. 현재 구현의 문제점들:
- 보안: 모든 사용자가 다른 사용자의 메시지를 볼 수 있음
- 성능: 사용자가 많아질수록 하나의 메시지가 모든 연결에 전송됨
- 비즈니스 로직: 실제 채팅앱에서는 특정 방이나 상대방에게만 메시지를 보내야 함
2. 개선이 필요한 부분:
- 방(room) 기반 메시징 시스템
- 메시지 대상 식별 로직
- 권한 검증
잘못 이해하신 부분은 전혀 없어요. 오히려 중요한 문제점을 정확하게 파악하신 거예요! 🐰
There was a problem hiding this comment.
이 부분은 우선 삭제하고 config가 아니라 서비스로 옮기면서 로직 수정하도록 하겠습니다!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
아마 제 코드 참고해서 작성하다가 추가하신 거 같은데 stomp없이 순수 웹소켓으로 한 번 먼저 구현해보고싶어서 작성해놓았었습니다!
거기서 삭제하면 테스트해본 게 아까워서 @component만 주석처리해놓았었는데 설명을 조금 달아놓을 걸 그랬네요 ㅎㅎ..
참고
There was a problem hiding this comment.
안녕하세요 수연님!
궁금한 내용 위주로 코멘트를 달았습니다.
저도 소켓에 대해서는 잘 알지 못하는 부분들이 많은데, 이 PR에서 같이 이야기하며 답을 찾아갈 수 있으면 좋겠습니다~
이미지 크기 제한은 논의가 필요해 보입니당
저도 이번에 GPT와 Claude한테 물어봤습니다.
제 생각에는 10MB vs 5MB 사이에서 선택하면 될 것 같아요.
- 우리 서비스의 규모를 생각한다면 5MB
- 혹시 모르는 경우)까지 대비하려 한다면 10MB
제 포트폴리오가 4MB 정도더라고요. (pdf 약 10장)
교환학생과 관련된 정보를 pdf 10 장보다 큰 파일로 공유할 것 같진 않습니다.
이걸 기준으로 생각해봤을 때, 5MB 도 괜찮을 것 같습니다. 🧐
5MB를 넘어가는 상황에 대한 최선은 "업로드할 수 있는 파일의 크기를 초과했습니다"라고 예외를 응답하는게 최선이 아닐까 싶습니다.!
❗️그리고 추가된 파일들에 대해서 cmd + shift + option + L 로 포매팅해주세요❗️
| @Override | ||
| public void registerWebSocketHandlers(WebSocketHandlerRegistry registry) { | ||
| registry.addHandler(webSocketHandler, "/connect") | ||
| .setAllowedOriginPatterns("*"); // todo: 사용하는 도메인으로 한정 | ||
| } |
There was a problem hiding this comment.
| @Override | |
| public void registerWebSocketHandlers(WebSocketHandlerRegistry registry) { | |
| registry.addHandler(webSocketHandler, "/connect") | |
| .setAllowedOriginPatterns("*"); // todo: 사용하는 도메인으로 한정 | |
| } | |
| private final CorsProperties corsProperties; | |
| @Override | |
| public void registerWebSocketHandlers(WebSocketHandlerRegistry registry) { | |
| List<String> strings = corsProperties.allowedOrigins(); | |
| String[] allowedOrigins = strings.toArray(String[]::new); | |
| registry.addHandler(webSocketHandler, "/connect") | |
| .setAllowedOrigins(allowedOrigins); | |
| } |
이렇게 하면 어떨까요?
그럼 application-variable.yml 에 있는 cors.allowed-origins 에 있는 목록을 가져올 수 있습니다!
There was a problem hiding this comment.
오호 그 부분은 확인 못했네요 반영하겠습니다~!
| @Slf4j | ||
| public class WebSocketHandler extends TextWebSocketHandler { | ||
|
|
||
| private final Set<WebSocketSession> sessions = ConcurrentHashMap.newKeySet(); |
There was a problem hiding this comment.
ConcurrentHashMap.newKeySet(); 이라는 자료구조를 선택하신 이유가 있으신가요?
동시성 문제를 방지하기 위한 다른 Set 컬렉션들 중에서,
e.g. Collections.synchronizedSet(new HashSet<>());
특히나 ConcurrentHashMap.newKeySet() 을 선택하신 이유가 궁금해요!
There was a problem hiding this comment.
ConcurrentHashMap.newKeySet()이 segment lock을 활용하여 실시간성에서 이점이 크다고 생각했고요 제가 알기로 읽기에 최적화 되어있다고 알고 있습니다! 혹시 잘못 알고 있는 거라면 알려주시면 감사하겠습니당
There was a problem hiding this comment.
아마 근데 Set 내에 WebSocketSession는 thread-safe하지 않은 것으로 알고 있어서 이 부분에 대해서 테스트를 좀 진행해보면 좋을 거 같습니다!
There was a problem hiding this comment.
저도 Collections.synchronizedSet 이건 성능저하가 너무 심해서 레거시인 것으로 알고 있긴 한데 확인 한 번 해보겠습니다
| @Override | ||
| public void afterConnectionEstablished(WebSocketSession session) throws Exception { | ||
| sessions.add(session); | ||
| log.info("Connected : {}", session.getId()); | ||
| } |
There was a problem hiding this comment.
이번 구현에서 수연님이 여러군데에 로깅을 하셨더라고요.
로깅은 어떤 기준으로 추가하셨나요?
There was a problem hiding this comment.
규혁님 코드를 활용하면서 로깅을 그대로 채용했는데요 connect/disconnect과정의 모든 과정에서 로그를 확인할 수 있도록 하셨더라구요. 클로드에서 실시간 서비스는 문제가 생기지 않도록 계속 모니터링 하는 게 중요하다고 해서 괜찮다고 생각하여 그대로 유지하였습니다 혹시 문제가 될까요...?
There was a problem hiding this comment.
저는 테스트용이었어서 그냥 잘 작동하는지 확인하기 위해 많은 곳에 로그를 찍어놨었습니다 ㅎㅎ.. 정말 필요한 부분만 찍으셔도 좋을 거 같습니다!
만약 다 필요하다면 그거에 대한 기준만 정리해봐도 의미가 있을 거 같습니다!
There was a problem hiding this comment.
로그가 너무 많으면 어떤 단점이 있는지는 이미 알고계시리라 생각합니다.!
- 디스크 공간 낭비
- 비용 증가 (클라우드 저장 시)
- 중요 로그 묻힘 등등..
(물론 지금 서버가 최적의 로그만 남기고 있는가? 라고 물으신다면 그건 아니지만요.. 😅
리팩터링해야한다고 생각하고 있습니다!)
실시간 서비스는 문제가 생기지 않도록 계속 모니터링 하는 게 중요하다고 해서 괜찮다고 생각하여 그대로 유지하였습니다 혹시 문제가 될까요...?
이 말씀도 일리가 있지만, "연결이 수립되었음 / 해제되었음 / 메세지 받음"에 대해 모두 로그를 남기는 것은 많다고 생각해요.
예를 들어 연결 수립/해제 같은 "연결 히스토리"는 엔진엑스 access.log 에도 기록되고 있을것이고요.
메시지를 받았다는 것은 데이터베이스에도 기록되는 것이라 생각했어요!
우리가 집중적으로 모니터링할 부분이 생긴다면 그때 추가하거나,
문제가 발생했을 때만 로그를 찍는 방법은 좋을 것 같다는 생각입니다~
| protected void handleTextMessage(WebSocketSession session, TextMessage message) throws Exception { | ||
| String payload = message.getPayload(); | ||
| log.info("Received message : {}", payload); | ||
| for(WebSocketSession s : sessions) { | ||
| if(s.isOpen()) { | ||
| s.sendMessage(new TextMessage(payload)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
저도 이게 궁금했어요. 전체 session 에 대해서 for 문을 돌면
서버와 연결된 모든 세션에 브로드케스트될 것 같아요.
제가 잘못이해하는 부분이 있다면 말씀해주세요.!
| @Override | ||
| public void configureClientInboundChannel(ChannelRegistration registration) { | ||
| registration.interceptors(stompHandler) | ||
| .taskExecutor() | ||
| .corePoolSize(6) | ||
| .maxPoolSize(12) | ||
| .queueCapacity(1000); | ||
| } |
There was a problem hiding this comment.
이런 설정값들을 선택하신 이유가 궁금해요! 표준이 있는건가요?
그리고 너무 원칙주의적일수도 있는데요,
저는 매직넘버를 보면 "상수화 or 변수화"를 못참겠더라고요 ㅠ.ㅠ
참고 : https://dev-dmsgk.tistory.com/30
이 부분을 private static final 로 상수화, 혹은 application-variable.yml 에 환경변수로 관리할 수 있을 것 같아요.
제 생각에는 application-variable.yml 로 관리하는게 좋을 것 같은데 수연님 생각은 어떠신가요?
There was a problem hiding this comment.
100명 이하일 때 추천하는 설정을 채용했는데 저도 따로 관리하는 게 좋을 것 같습니다! 수정해놓을게용
There was a problem hiding this comment.
매직넘버를 지양하는 건 아주 좋다고 생각합니다 ㅎㅎ
| String token = bearerToken.substring(7); | ||
|
|
||
| Claims claims = jwtTokenProvider.parseClaims(token); | ||
|
|
||
| log.info("토큰 검증 성공, 유저 인증 완료 - 사용자: {}", claims.getSubject()); | ||
| } |
There was a problem hiding this comment.
음.. 사실 인증과 관련된 부분은 스프링 시큐리티에서 웹소켓에 대한 편의 기능들을 제공하는것으로 알아요.
그것들을 잘 사용한다면 이런 중복코드가 많이 줄어들 것이라 생각돼요.
하지만 문제는 ....
스프링 시큐리티 - 웹소켓에 대한 러닝커브인 것습니다. 🫠
제가 스프링 시큐리티에 대해서 관련 부분을 더 알아보고 수정할 수 있을 것 같아요! (시간이 필요하긴 합니다 🥲)
제가 성혁님 코드리뷰할 때도 말씀드렸는데,
다음주 월요일 개발 회의 시간에 제가 스프링 시큐리티 특강(?)을 해드릴게요.
이 PR에서는 인증과 관련된 부분은 잠시 뒤로하고, "웹소켓 연결 설정"에 대한 것들만 집중하여 리뷰 드리겠습니다!!!
우선 이게 머지되어야 다른 소켓 관련 기능들도 진행 가능할테니까요.
그래도 괜찮을까요?
There was a problem hiding this comment.
제가 알기로 Spring Security는 HTTP 요청 기반 필터 체인에서만 작동해서 웹소켓 관련해서 검증이 안되는 것으로 알고있습니다!
혹시 다른 방식을 찾아보신 게 있으신가요?
There was a problem hiding this comment.
제가 알기로 Spring Security는 HTTP 요청 기반 필터 체인에서만 작동해서 웹소켓 관련해서 검증이 안되는 것으로 알고있습니다!
혹시 다른 방식을 찾아보신 게 있으신가요?
저도 정확히는 모르지만 스프링 시큐리티에서 여차저차 처리를 하면,
"HTTP 세션에 저장된 SecurityContext를 WebSocket 세션으로 옮겨오는" 것이 가능하다고 합니다.
이게 된다면 위와 같은 코드들은 없어도 된다 생각했어요 💡
There was a problem hiding this comment.
오! 되면 너무 신기하고 편해지겠네요! 새로 또 배우네요 ㅎㅎ
src/main/java/com/example/solidconnection/chat/config/StompWebSocketConfig.java
Outdated
Show resolved
Hide resolved
whqtker
left a comment
whqtker
left a comment
There was a problem hiding this comment.
고생하셨습니다 ! 저도 WebSocket과 STOMP를 빠삭하게 알고 있는 게 아니라서 ... 알고 있는 지식 내로 의견 달았습니다 ..!!
src/main/java/com/example/solidconnection/chat/config/StompHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/chat/config/StompWebSocketConfig.java
Outdated
Show resolved
Hide resolved
build.gradle
Outdated
| implementation 'org.hibernate.validator:hibernate-validator' | ||
| implementation 'com.amazonaws:aws-java-sdk-s3:1.12.782' | ||
| implementation 'org.springframework.boot:spring-boot-starter-websocket' | ||
| implementation 'org.webjars:stomp-websocket' |
There was a problem hiding this comment.
- What went wrong:
Execution failed for task ':compileJava'.
Could not resolve all files for configuration ':compileClasspath'.
Could not find org.webjars:stomp-websocket:.
Required by:
project :
빌드 에러가 발생하는데, 아마 implementation 'org.webjars:stomp-websocket' 때문인 것 같습니다. 제 경험 상으론 해당 의존성은 없어도 STOMP 까지 잘 동작했던 거로 기억합니다 !
| public void configureMessageBroker(MessageBrokerRegistry registry) { | ||
| registry.setApplicationDestinationPrefixes("/publish"); | ||
| registry.enableSimpleBroker("/topic") | ||
| .setHeartbeatValue(new long[]{15000, 15000}); |
There was a problem hiding this comment.
애플리케이션 실행 시 아래 오류가 발생합니다.
Caused by: java.lang.IllegalArgumentException: Heartbeat values configured but no TaskScheduler provided
at org.springframework.util.Assert.isTrue(Assert.java:122)
at org.springframework.messaging.simp.broker.SimpleBrokerMessageHandler.startInternal(SimpleBrokerMessageHandler.java:273)
at org.springframework.messaging.simp.broker.AbstractBrokerMessageHandler.start(AbstractBrokerMessageHandler.java:211)
at org.springframework.context.support.DefaultLifecycleProcessor.doStart(DefaultLifecycleProcessor.java:179)
... 13 common frames omitted
만약 하트비트를 설정하신다면, 오류 메시지 내용처럼 TaskScheduler 빈을 생성하신 후, 등록해주시면 될 것 같습니다 !
There was a problem hiding this comment.
확인했습니다.... 이걸 확인안했네요 ㅜㅜ
|
|
||
| @Component | ||
| @Slf4j | ||
| public class WebSocketHandler extends TextWebSocketHandler { |
There was a problem hiding this comment.
저희는 STOMP를 통해 WebSocket을 구현하고 있고, STOMP는 내부적으로 SimpleBrokerMessageHandler 를 사용하기 때문에 WebSocketHandler는 없어도 잘 동작할 것 같습니다.
이 점은 저도 애매하기에, 잘못 알고 있는 점이 있다면 말씀해주세요 !
There was a problem hiding this comment.
넵 성혁님께서 말씀해주신 게 맞습니다! 수정할게용
| @Configuration | ||
| @EnableWebSocket | ||
| @RequiredArgsConstructor | ||
| public class WebSocketConfig implements WebSocketConfigurer { |
There was a problem hiding this comment.
(WebSockerHandler 리뷰의 연장선입니다.)
WebSocketConfig 도 마찬가지로 저희는 STOMP를 사용하기 때문에 없어도 잘 동작할 듯 합니다.
- 불필요한 의존성 삭제 - 불필요한 파일 삭제 - 매직넘버 application-variable에서 관리 - 에러코드 추가 - 중복 코드 함수화
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/example/solidconnection/chat/config/StompHandler.java (1)
29-33: 인증 정보를 세션에 저장하는 로직이 여전히 누락되어 있습니다토큰 검증은 성공적으로 수행되고 있지만, 인증된 사용자 정보를 STOMP 세션 속성에 저장하지 않고 있어요. 이로 인해 후속 메시지 처리 시 사용자 컨텍스트를 활용할 수 없습니다.
다음과 같이 개선해보세요:
if (StompCommand.CONNECT.equals(accessor.getCommand())) { log.info("CONNECT 요청 - 토큰 검증 시작"); Claims claims = validateAndExtractClaims(accessor, AUTHENTICATION_FAILED); log.info("토큰 검증 성공 - 사용자: {}", claims.getSubject()); + + // 사용자 정보를 STOMP 세션 속성에 저장 + accessor.getSessionAttributes().put("userEmail", claims.getSubject()); + accessor.setUser(() -> claims.getSubject()); }
🧹 Nitpick comments (1)
src/main/java/com/example/solidconnection/chat/config/StompHandler.java (1)
35-46: 구독 권한 검증 로직 구현이 시급합니다코드 중복 문제는 잘 해결되었지만, 채팅방 접근 권한 검증이 여전히 TODO 상태로 남아있어요. 이는 보안상 중요한 이슈입니다.
현재 누구나 임의의 채팅방을 구독할 수 있는 상태인데, 다음 중 하나를 선택해주세요:
- 지금 당장 권한 검증 로직을 구현하기
- 별도 이슈로 등록하여 추후 작업 계획 수립하기
채팅방 접근 권한 검증 로직을 구현하는 코드를 생성해드릴까요?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build.gradle(1 hunks)src/main/java/com/example/solidconnection/chat/config/StompHandler.java(1 hunks)src/main/java/com/example/solidconnection/chat/config/StompWebSocketConfig.java(1 hunks)src/main/java/com/example/solidconnection/common/exception/ErrorCode.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- build.gradle
- src/main/java/com/example/solidconnection/common/exception/ErrorCode.java
- src/main/java/com/example/solidconnection/chat/config/StompWebSocketConfig.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nayonsoso
PR: solid-connection/solid-connect-server#400
File: src/main/java/com/example/solidconnection/chat/config/StompWebSocketConfig.java:20-22
Timestamp: 2025-07-23T18:07:08.348Z
Learning: SockJS configuration should be added to STOMP endpoints using .withSockJS() for better browser compatibility and network environment support, even if frontend doesn't initially use SockJS client libraries.
src/main/java/com/example/solidconnection/chat/config/StompHandler.java (1)
Learnt from: nayonsoso
PR: #400
File: src/main/java/com/example/solidconnection/chat/config/StompWebSocketConfig.java:20-22
Timestamp: 2025-07-23T18:07:08.348Z
Learning: SockJS configuration should be added to STOMP endpoints using .withSockJS() for better browser compatibility and network environment support, even if frontend doesn't initially use SockJS client libraries.
🧬 Code Graph Analysis (1)
src/main/java/com/example/solidconnection/chat/config/StompHandler.java (2)
src/main/java/com/example/solidconnection/common/exception/CustomExceptionHandler.java (1)
Slf4j(22-88)src/main/java/com/example/solidconnection/chat/config/StompEventListener.java (1)
Component(13-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
src/main/java/com/example/solidconnection/chat/config/StompHandler.java (3)
1-24: 클래스 구조가 깔끔하게 정리되었네요!이전 버전 대비 개선된 점들을 확인할 수 있습니다:
- 적절한 어노테이션 사용으로 Spring 컴포넌트 등록
- Lombok을 활용한 생성자 주입 패턴 적용
- 로깅을 위한 @slf4j 어노테이션 추가
51-58: 코드 중복 문제가 훌륭하게 해결되었습니다!이전 버전에서 CONNECT와 SUBSCRIBE에서 중복되던 토큰 검증 로직을 깔끔하게 별도 메서드로 추출했네요.
특히 좋은 점들:
- ErrorCode를 파라미터로 받아 유연한 예외 처리 가능
- 메서드명이 역할을 명확히 표현
- 단일 책임 원칙을 잘 준수
60-69: 방어적 프로그래밍이 완벽하게 적용되었습니다!이전 버전에서 지적된 배열 인덱스 검증 문제가 모두 해결되었어요:
- null 체크로 NPE 방지
- 배열 길이 검증으로 IndexOutOfBoundsException 방지
- topic 경로 형식 검증으로 잘못된 destination 차단
- 적절한 CustomException 처리
안전하고 견고한 코드로 개선되었습니다!
…rver into feat/399-feat-stomp-config
Gyuhyeok99
left a comment
Gyuhyeok99
left a comment
There was a problem hiding this comment.
코멘트 다른분들이 남겨놓으신 거에 추가로 더 남겨놓았습니다!
추가로 잘 연결되는 거 같긴 한데 이걸 테스트코드로도 작성해야할지(작성할 수는 있는지...)는 잘 모르겠네요... 고생하셨습니다!
stomp는 테스트하기가 힘들더라구요
| @Value("${websocket.thread-pool.inbound.core-pool-size:6}") | ||
| private int inboundCorePoolSize; | ||
|
|
||
| @Value("${websocket.thread-pool.inbound.max-pool-size:12}") | ||
| private int inboundMaxPoolSize; | ||
|
|
||
| @Value("${websocket.thread-pool.inbound.queue-capacity:1000}") | ||
| private int inboundQueueCapacity; | ||
|
|
||
| @Value("${websocket.thread-pool.outbound.core-pool-size:6}") | ||
| private int outboundCorePoolSize; | ||
|
|
||
| @Value("${websocket.thread-pool.outbound.max-pool-size:12}") | ||
| private int outboundMaxPoolSize; | ||
|
|
||
| @Value("${websocket.heartbeat.server-interval:15000}") | ||
| private long heartbeatServerInterval; | ||
|
|
||
| @Value("${websocket.heartbeat.client-interval:15000}") |
There was a problem hiding this comment.
저희 코드 일부 화면인데요! 이런식으로 @ConfigurationProperties를 활용하면 @value를 사용하는 것보다 코드를 좀 더 깔끔하게 작성할 수 있습니다!
There was a problem hiding this comment.
저도 ConfigurationProperties 코멘트를 다려했었습니다 ㅎㅎ
설정과 관련된 클래스로, 관련 값들을 묶어서 정말로 중요한 코드에 집중하게 해주더라고요!
수연님께서 전에 사용한적이 없으셨다면, 이번에 알아두셔도 좋을 것 같습니다~
참고로 우테코에서 현직자분들께 코드리뷰를 받을 때,
대단한 기업에 다니셨던 멘토님들도 ConfigurationProperties 방법을 추천하셨어요!
- 로그 삭제 - @ConfigurationProperties 사용을 위한 파일 추가 - application.yml에 websocket 값 추가
| count: | ||
| scheduling: | ||
| delay: 3000 | ||
| websocket: |
There was a problem hiding this comment.
현재 WebSocket 관련 설정값이 test/resources/application.yml 에 작성되어 있는데, main/resources/application.yml 에 적어주어야 할 듯 합니다 ..!
There was a problem hiding this comment.
아니 분명 테스트랑 main 둘 다 올렸는데 merge 과정에서 날아간 것 같네요............. ㅜㅜㅜ 다시 올릴게용
| import org.springframework.web.socket.messaging.SessionDisconnectEvent; | ||
|
|
||
| @Component | ||
| @Slf4j |
There was a problem hiding this comment.
로그 없으니까 @Sl4fj 는 없애도 될 것 같습니다 !
There was a problem hiding this comment.
아직 반영이 안된 것 같습니다. push 하셨는지 확인해주세요~ 👀
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| @Slf4j |
There was a problem hiding this comment.
아직 반영이 안된 것 같습니다. push 하셨는지 확인해주세요~ 👀
…rver into feat/399-feat-stomp-config
| import org.springframework.web.socket.messaging.SessionDisconnectEvent; | ||
|
|
||
| @Component | ||
| @Slf4j |
There was a problem hiding this comment.
아직 반영이 안된 것 같습니다. push 하셨는지 확인해주세요~ 👀
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| @Slf4j |
There was a problem hiding this comment.
아직 반영이 안된 것 같습니다. push 하셨는지 확인해주세요~ 👀
There was a problem hiding this comment.
이 파일 전체에도 포매팅이 필요해보입니다 🏃♀️
만약 다른 개발자가 main.java 패키지 전체를 포매팅했을 때, 이 파일까지 함께 변경된다면
그 변경이 본인의 관심사와는 무관한 부분임에도 불구하고 PR에 포함될 것입니다.
이런 경우엔 불필요한 diff가 발생해서 리뷰나 히스토리 추적에 혼란을 줄 수 있다고 생각합니다~
그래서 미리 포매팅을 맞춰두는 게 좋을 것 같아요!
There was a problem hiding this comment.
분명 모든 파일에 포매팅을 했는데........ 빼먹은 것 같습니다 담부터 잘 확인 할게용 ㅜㅜ
| long clientInterval | ||
| ) {} | ||
| } | ||
|
|
There was a problem hiding this comment.
파일 끝 개행이 두줄 되어있는 것 같습니다 😲
| registration.interceptors(stompHandler) | ||
| .taskExecutor() | ||
| .corePoolSize(stompProperties.threadPool().inbound().corePoolSize()) | ||
| .maxPoolSize(stompProperties.threadPool().inbound().maxPoolSize()) | ||
| .queueCapacity(stompProperties.threadPool().inbound().queueCapacity()); |
There was a problem hiding this comment.
| registration.interceptors(stompHandler) | |
| .taskExecutor() | |
| .corePoolSize(stompProperties.threadPool().inbound().corePoolSize()) | |
| .maxPoolSize(stompProperties.threadPool().inbound().maxPoolSize()) | |
| .queueCapacity(stompProperties.threadPool().inbound().queueCapacity()); | |
| InboundProperties inboundProperties = stompProperties.threadPool().inbound(); | |
| registration.interceptors(stompHandler) | |
| .taskExecutor() | |
| .corePoolSize(inboundProperties.corePoolSize()) | |
| .maxPoolSize(inboundProperties.maxPoolSize()) | |
| .queueCapacity(inboundProperties.queueCapacity()); |
변수 분리하는건 어떨까요?
stompProperties.threadPool().inbound()가 조금 긴데, 세번이나 있는게 중복코드이기도 하고,
실수로 stompProperties.threadPool().outbound()가 포함되어있더라도 파악하기 어려울 수 있을 것 같아요.
변수로 분리하면 그런 실수를 방지할 수 있을 것 같습니다~
- stompProperties.threadPool().outbound()와
- stompProperties.heartbeat() 도 마찬가지입니다!
There was a problem hiding this comment.
좋은 아이디어 감사합니다! 반영하였습니다 : )
|
|
||
| // socket | ||
| UNAUTHORIZED_SUBSCRIBE(HttpStatus.FORBIDDEN.value(), "구독 권한이 없습니다."), | ||
| INVALID_ROOMID(HttpStatus.BAD_REQUEST.value(), "경로의 roomId가 잘못되었습니다."), |
There was a problem hiding this comment.
| INVALID_ROOMID(HttpStatus.BAD_REQUEST.value(), "경로의 roomId가 잘못되었습니다."), | |
| INVALID_ROOM_ID(HttpStatus.BAD_REQUEST.value(), "경로의 roomId가 잘못되었습니다."), |
- application-variable.yml에 설정값 추가 - code formatting, 오타 수정 - 불필요한 @Sl4fj제거 - StompWebSocketConfig 변수 분리
whqtker
left a comment
whqtker
left a comment
There was a problem hiding this comment.
어노테이션 제거 확인했습니다 ! 시크릿 파일만 변경사항 적용해주세요 !!
정말 고생하셨습니다~!! 👍
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
이 PR머지하시기 전에 secret 레포의 application-variable.yml 업데이트하시고
🔥submodule 해시 변경하는🔥 커밋 포함한 후 머지해주세요!!
4 participants
관련 이슈
작업 내용
특이 사항
리뷰 요구사항 (선택)
아직 STOMP에 대한 이해가 부족해서... 코드에 문제가 있으면 반드시 알려주세요!
이미지 크기 제한은 논의가 필요해 보입니당