-
Notifications
You must be signed in to change notification settings - Fork 16
[신은수] Sprint12 #170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 신은수
Are you sure you want to change the base?
[신은수] Sprint12 #170
Conversation
| public void registerStompEndpoints(StompEndpointRegistry registry) { | ||
| // 클라이언트가 연결할 WebSocket 엔드포인트 정의 | ||
| registry.addEndpoint("/ws") | ||
| .withSockJS(); // SockJS fallback 지원 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.setAllowedOriginPatterns("*") 도 추가해주세요.
| @RequiredArgsConstructor | ||
| public class MessageWebSocketController { | ||
|
|
||
| private final SimpMessagingTemplate messagingTemplate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SimpMessagingTemplate 에 대한 의존은 서비스 단이나 리스너 단에서 처리되도록 구현해주세요!
| private final SimpMessagingTemplate messagingTemplate; | ||
|
|
||
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) | ||
| public void handleMessage(MessageCreatedEvent event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 비동기로 처리되도록 처리하시면 좋을것 같아요!
| private final SseService sseService; | ||
|
|
||
| @GetMapping(produces = MediaType.TEXT_EVENT_STREAM_VALUE) | ||
| public SseEmitter connect(@AuthenticationPrincipal DiscodeitUserDetails userDetails) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RequestParam(value = "LastEventId", required = false) UUID lastEventId 를 받아오도록 처리하시고 connect에 null이 아닌 lastEventId 를 받아서 처리하도록 구현하시면 될것 같습니다.
|
Sse 를 이벤트 핸들러를 통해 발행되도록 개선해보세요! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements WebSocket messaging, Server-Sent Events (SSE) for real-time notifications, and Docker Compose deployment architecture. The implementation includes comprehensive test coverage with unit, integration, and repository tests, along with configuration files for different deployment environments.
Reviewed Changes
Copilot reviewed 201 out of 208 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/resources/application-test.yaml | Test environment configuration with H2 database and JWT settings |
| src/test/java/**/storage/s3/* | S3 storage integration and API tests |
| src/test/java/**/service/basic/* | Unit tests for core services (User, Notification, Message, Channel, BinaryContent, Auth) |
| src/test/java/**/security/jwt/* | JWT token provider, authentication filter, and login handler tests |
| src/test/java/**/security/* | Login and CSRF security tests |
| src/test/java/**/repository/* | Repository layer tests for User, ReadStatus, Message, and Channel |
| src/test/java/**/integration/* | API integration tests for all major endpoints |
| src/test/java/**/event/listener/* | Event listener tests for notifications and binary content |
| src/test/java/**/controller/* | Controller layer unit tests |
| src/main/resources/static/* | Frontend static assets (HTML, CSS, JS) |
| src/main/resources/*.yaml | Application configuration for different environments (dev, prod) |
| src/main/resources/schema.sql | Database schema definition |
| src/main/resources/logback-spring.xml | Logging configuration with MDC support |
| src/main/java/**/storage/* | Storage abstraction with local and S3 implementations |
| src/main/java/**/service/basic/* | Core service implementations |
| settings.gradle | Gradle project configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| public UUID put(UUID binaryContentId, byte[] bytes) { | ||
| delay(4); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded delay of 4 seconds in the put method appears to be for testing/simulation purposes. This should either be removed for production use, made configurable via application properties, or at minimum documented with a comment explaining why this delay exists.
| User user = userRepository.findById(userId) | ||
| .orElseThrow(() -> UserNotFoundException.withId(userId)); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user retrieval before deletion is unnecessary since userRepository.deleteById() will throw an exception if the user doesn't exist. Consider checking existence with existsById() instead if validation is required, or remove this check entirely to reduce database calls.
| .orElseThrow(() -> { | ||
| UserNotFoundException exception = UserNotFoundException.withId(userId); | ||
| return exception; | ||
| }); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception creation can be simplified. Instead of creating a local variable and returning it, directly throw the exception inline: .orElseThrow(() -> UserNotFoundException.withId(userId))
| .orElseThrow(() -> { | |
| UserNotFoundException exception = UserNotFoundException.withId(userId); | |
| return exception; | |
| }); | |
| .orElseThrow(() -> UserNotFoundException.withId(userId)); |
| secret: ${JWT_ACCESS_SECRET:your-access-token-secret-key-here-make-it-long-and-random} | ||
| expiration-ms: ${JWT_ACCESS_EXPIRATION_MS:1800000} # 30 minutes | ||
| refresh-token: | ||
| secret: ${JWT_REFRESH_SECRET:your-refresh-token-secret-key-here-make-it-different-and-long} |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default JWT secret values are insecure placeholder strings. While using environment variables is correct, the fallback default values should either be removed entirely or fail fast if not provided, as these defaults could accidentally be used in production.
| secret: ${JWT_ACCESS_SECRET:your-access-token-secret-key-here-make-it-long-and-random} | |
| expiration-ms: ${JWT_ACCESS_EXPIRATION_MS:1800000} # 30 minutes | |
| refresh-token: | |
| secret: ${JWT_REFRESH_SECRET:your-refresh-token-secret-key-here-make-it-different-and-long} | |
| secret: ${JWT_ACCESS_SECRET} | |
| expiration-ms: ${JWT_ACCESS_EXPIRATION_MS:1800000} # 30 minutes | |
| refresh-token: | |
| secret: ${JWT_REFRESH_SECRET} |
| size bigint NOT NULL, | ||
| content_type varchar(100) NOT NULL, | ||
| status varchar(20) NOT NULL | ||
| -- ,bytes bytea NOT NULL |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out bytes column should either be removed if it's no longer needed, or properly documented explaining why it's commented out and when/if it should be enabled.
| -- ,bytes bytea NOT NULL |
| private S3Client getS3Client() { | ||
| return S3Client.builder() | ||
| .region(Region.of(region)) | ||
| .credentialsProvider( | ||
| StaticCredentialsProvider.create( | ||
| AwsBasicCredentials.create(accessKey, secretKey) | ||
| ) | ||
| ) | ||
| .build(); | ||
| } |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a new S3Client instance on every method call is inefficient. The S3Client should be created once during initialization and reused. Consider making this a final field initialized in the constructor or a PostConstruct method.
| } | ||
|
|
||
| private String generatePresignedUrl(String key, String contentType) { | ||
| try (S3Presigner presigner = getS3Presigner()) { |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the S3Client, creating a new S3Presigner instance for each presigned URL generation is inefficient. The S3Presigner should be created once and reused as a class field.
요구사항
기본
웹소켓 구현하기
1. 웹소켓 환경 구성
@Configuration @EnableWebSocketMessageBroker public class WebSocketConfig implements WebSocketMessageBrokerConfigurer {...}@Override public void configureMessageBroker(MessageBrokerRegistry config) {...}메시지 송신
@Controller public class MessageWebSocketController { ... @MessageMapping(...) }메시지 수신
2. SSE 구현하기
SSE 환경을 구성하세요.
기존에 클라이언트에서 폴링 방식으로 주기적으로 요청하던 데이터를 SSE를 이용해 서버에서 실시간으로 전달하는 방식으로 리팩토링하세요.
새로운 알림 이벤트 전송
파일 업로드 상태 변경 이벤트 전송
채널 갱신 이벤트 전송
사용자 갱신 이벤트 전송
3. 배포 아키텍처 구성하기
심화
주요 변경사항
스크린샷
멘토에게