[육선우] sprint5#106
Hidden character warning
Conversation
…ser and Channel api
…dd UserAuthApi and ReadStatusApi
There was a problem hiding this comment.
p2. 이 부분 아마 여러차례 말씀 드렸던 것 같은데 github 에는 불필요한 데이터 파일들, 로컬 설정, 로컬 빌드 결과 등은 제외 필요합니다!
There was a problem hiding this comment.
~Api 네이밍 보다는 ~Controller 네이밍이 더 일반적이라 변경하는 편이 좋을 것 같습니다!
서버 단보단 보통 Client Side에서 호출할 때 이런 네이밍을 많이 써요
| public interface BinaryContentApi { | ||
|
|
||
| @Operation(summary = "BinaryContent 전체 조회") | ||
| @ApiResponses(value = { | ||
| @ApiResponse( | ||
| responseCode = "200", description = "전체 조회 성공", | ||
| content = @Content(array = @ArraySchema(schema = @Schema(implementation = BinaryContent.class))) | ||
| ) | ||
| }) | ||
| ResponseEntity<List<BinaryContent>> findAllByIds( | ||
| @Parameter(description = "조회할 파일 ID 목록") List<UUID> ids | ||
| ); | ||
|
|
||
| @Operation(summary = "BinaryContent 조회") | ||
| @ApiResponses(value = { | ||
| @ApiResponse( | ||
| responseCode = "200", description = "BinaryContent 조회 성공", | ||
| content = @Content(schema = @Schema(implementation = BinaryContent.class)) | ||
| ), | ||
| @ApiResponse( | ||
| responseCode = "404", description = "없는 ID입니다", | ||
| content = @Content(examples = @ExampleObject("{ids} not found")) | ||
| ) | ||
| }) | ||
| ResponseEntity<BinaryContent> find( | ||
| @Parameter(description = "조회할 Id") UUID id | ||
| ); |
There was a problem hiding this comment.
p3. Entity 바로 리턴하고 계신데, 그러면 지나치게 많은 응답 결과가 보내지게 됩니다. Entity 수정에 따라서 응답 결과도 달라지게 되구요. 꼭 ResponseDto 를 만들어줘야 합니다 ! 이 부분 기본적인 부분이라 챙겨가시면 좋을 것 같네요!
There was a problem hiding this comment.
~API랑 Controller 랑 두개를 두신 이유를 잘 이해 못했습니다 별도 이유가 있으실까요?
joonfluence
left a comment
joonfluence
left a comment
There was a problem hiding this comment.
전체적으로 Entity, Repository(JCF/File), Service, Controller, DTO 전 레이어가 잘 구성되어 있습니다. 몇 가지 버그와 설계 개선 포인트를 코멘트로 남깁니다.
- p1: 2건 (반드시 수정 필요)
- p2: 3건 (적극 고려 권장)
- p3: 3건 (웬만하면 반영)
- p4: 1건
- p5: 1건
p5. .DS_Store와 .discodeit/ 하위 직렬화 파일들이 커밋에 포함되어 있습니다. .gitignore 추가 전에 커밋된 것으로 보입니다. git rm --cached로 트래킹을 해제하세요.
| // | ||
| private final BinaryContentRepository binaryContentRepository; | ||
| private final UserStatusRepository userStatusRepository; | ||
| private BinaryContent binaryContent; |
There was a problem hiding this comment.
p1. Service는 싱글톤 빈인데 BinaryContent를 인스턴스 필드로 선언하면 동시성 문제가 발생합니다. 또한 이 필드는 초기화되지 않아 아래 create() 메서드의 binaryContentRepository.save(binaryContent)에서 항상 null이 전달됩니다.
profileRequest 데이터로 new BinaryContent(...)를 메서드 내 지역 변수로 생성해야 합니다. (update() 메서드에서는 올바르게 구현되어 있으니 참고하세요.)
|
|
||
| if (existingUser.isPresent()) { | ||
| System.out.println("이미 존재하는 이메일입니다: " + email); | ||
| return existingUser.get(); |
There was a problem hiding this comment.
p1. 이메일/username이 중복일 때 예외를 던지지 않고 기존 사용자를 반환하고 있습니다. 클라이언트는 새 사용자가 생성된 것으로 오해할 수 있습니다.
IllegalArgumentException 등 적절한 예외를 던지는 것이 맞습니다. System.out.println 대신 로거(@Slf4j) 사용도 권장합니다.
참고로 아래 update() 메서드에서는 중복 체크 시 올바르게 예외를 던지고 있어서, create()도 동일한 패턴으로 맞춰주시면 됩니다.
| User user = userRepository.findByUsername(username) | ||
| .orElseThrow(() -> new NoSuchElementException("User with username " + username + " not found")); | ||
|
|
||
| if (!user.getPassword().equals(password)) { |
There was a problem hiding this comment.
p2. 비밀번호를 평문으로 비교하고 있습니다. 스프린트 과제 범위에서 벗어날 수 있지만, 최소한 TODO 주석으로 향후 BCrypt 등 해싱 적용이 필요하다는 점을 남겨두시면 좋겠습니다.
| if (!user.getPassword().equals(password)) { | |
| // TODO: BCrypt 등 비밀번호 해싱 적용 필요 | |
| if (!user.getPassword().equals(password)) { |
| private final AuthService authService; | ||
| private final UserService userService; | ||
| private final UserStatusRepository userStatusRepository; | ||
|
|
There was a problem hiding this comment.
p2. Controller가 UserStatusRepository를 직접 의존하면 레이어 아키텍처를 위반합니다. 로그인 시 UserStatus 갱신 로직은 AuthService 또는 UserStatusService로 이동시키는 것이 좋습니다.
Controller는 Service만 의존하고, Service가 Repository를 의존하는 구조를 유지해주세요.
| return allChannels.stream() | ||
| .map(this::toDto) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
p2. 메서드명은 findAllByUserId인데 실제로는 전체 채널을 반환하고 있습니다. PUBLIC 채널 + 해당 userId가 참여한 PRIVATE 채널만 반환하도록 필터링 로직이 필요합니다.
| } | |
| return allChannels.stream() | |
| .filter(channel -> channel.getType().equals(ChannelType.PUBLIC) | |
| || readStatusRepository.findAllByChannelId(channel.getId()).stream() | |
| .anyMatch(rs -> rs.getUserId().equals(userId))) | |
| .map(this::toDto) | |
| .toList(); |
| public class GlobalExceptionHandler { | ||
|
|
||
| // 모든 예외를 낚아채서 우리가 만든 ErrorResponse로 변환합니다. | ||
| @ExceptionHandler(Exception.class) |
There was a problem hiding this comment.
p3. 모든 예외를 500 Internal Server Error로 처리하고 있습니다. 예외 유형별로 적절한 HTTP 상태 코드를 매핑하면 클라이언트가 에러를 구분하기 쉬워집니다.
예시:
NoSuchElementException→ 404 Not FoundIllegalArgumentException→ 400 Bad RequestException(fallback) → 500 Internal Server Error
| @Override | ||
| public ResponseEntity<List<ChannelDto>> findAllByUserId( | ||
| @RequestParam("userId") UUID userId) { | ||
| List<ChannelDto> channels = channelService.findAllByUserId(userId); |
There was a problem hiding this comment.
p3. path에 {userId}를 정의했으면 @PathVariable을 사용해야 합니다. @RequestParam은 쿼리 파라미터(?userId=xxx) 용도입니다. 현재 코드는 path variable과 request param이 충돌하여 런타임 에러가 발생할 수 있습니다.
| List<ChannelDto> channels = channelService.findAllByUserId(userId); | |
| @PathVariable("userId") UUID userId) { |
| @Override | ||
| public Message update(UUID messageId, MessageUpdateRequest request, | ||
| List<BinaryContentCreateRequest> attachments) { | ||
| String newContent = request.newContent(); |
There was a problem hiding this comment.
p3. update 메서드에서 attachments 파라미터를 받지만 실제로 사용하지 않습니다. 새 첨부파일이 전달되면 기존 것을 교체하는 로직이 필요하거나, 사용하지 않는다면 파라미터를 제거하는 것이 좋습니다.
| // this.DIRECTORY = Paths.get(System.getProperty("user.dir"), fileDirectory, | ||
| // Channel.class.getSimpleName()); | ||
| this.DIRECTORY = Paths.get(System.getProperty("user.dir"), "discodeit", fileDirectory, | ||
| Channel.class.getSimpleName()); |
There was a problem hiding this comment.
p4. FileChannelRepository와 FileReadStatusRepository는 경로에 "discodeit" 하위 디렉토리를 포함하지만, 나머지 File Repository들(FileBinaryContentRepository, FileMessageRepository 등)은 포함하지 않습니다. 일관된 경로 전략을 사용하는 것이 좋겠습니다.
2 participants
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게