Conversation
Seooooo24
requested review from
5uhwann,
KoSeonJe and
wonjunYou
as code owners
Walkthrough페어게임 신청자 등록(파일 업로드 포함), 신청자 수 조회, 메타데이터 조회 관련 REST API·엔티티·레포지토리·서비스·페사드·예외·마이그레이션과 S3 multipart 업로드 및 파일메타데이터 조회 확장 추가. (50단어 이내) Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Controller as UserPairGameController
participant Facade as FacadeUserPairGameService
participant S3 as S3FileService
participant PairService as PairGameService
participant Repo as PairGameRepository
participant FileMeta as FileMetaDataService
participant DB as Database
Client->>Controller: POST /pair-game/appliers (request + file)
Controller->>Facade: createPairGameApplier(command)
Facade->>S3: uploadMultipartFile(file, dateTime, "pair-game")
S3->>S3: generateKey(contentType, dateTime, directory, uuid)
S3-->>Facade: uploadedKey
Facade->>PairService: validateStudentNumberUnique(studentNumber)
PairService-->>Facade: OK / throws
Facade->>PairService: create(entity with imageUrl)
PairService->>Repo: save(entity)
Repo->>DB: INSERT
DB-->>Repo: OK
Repo-->>PairService: savedEntity
PairService-->>Facade: created entity
Facade-->>Controller: void
Controller-->>Client: 201 Created
Client->>Controller: GET /pair-game/metadata
Controller->>Facade: getPairGameMetaData()
Facade->>FileMeta: getCoupledAllByDomainType(CLUB_PROFILE)
FileMeta->>DB: SELECT WHERE file_status = COUPLED
DB-->>FileMeta: metadataList
FileMeta-->>Facade: metadataList
Facade->>S3: getUploadedFileUrl(each key)
S3-->>Facade: urls
Facade-->>Controller: PairGameMetaDataQuery(urls)
Controller-->>Client: 200 OK (images)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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
🤖 Fix all issues with AI agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/entity/PairGameApplier.java`:
- Around line 14-18: The PairGameApplier entity has an incorrect `@SQLDelete`
target table ("applier") causing soft-delete failures; update the
PairGameApplier class by adding `@Table`(name = "pair_game_applier") and change
the `@SQLDelete` SQL to reference the correct table name (e.g., "update
pair_game_applier set deleted_at = CURRENT_TIMESTAMP where id=?"); keep the
existing `@SQLRestriction/`@SQLDelete annotations in place but ensure their table
name matches the actual table created by
V51__create_pair_game_applier_table.sql.
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/repository/PairGameRepository.java`:
- Around line 7-8: The getPairGameApplierAmount() method currently calls
repository.findAll().size(), which loads all PairGameApplier entities into
memory; change that call to repository.count() to let JPA/DB return the row
count efficiently (respecting the `@SQLRestriction` on PairGameApplier). Locate
the method that references repository.findAll().size() (the repository variable
is a PairGameRepository that extends JpaRepository<PairGameApplier, Long>) and
replace the size-based approach with repository.count(), keeping the return type
and semantics the same.
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java`:
- Around line 29-34: createApplier 메서드에서 studentFeeImageFile에 대한 null 체크 및 파일
크기/타입 검증이 빠져 있어 업로드 실패 시 불명확한 에러가 발생할 수 있습니다; createApplier에서 먼저
studentFeeImageFile이 null인지 또는 isEmpty()인지 검사하고 허용할 MIME 타입(예: image/png,
image/jpeg)과 최대 크기(예: MAX_IMAGE_SIZE 상수)를 확인한 뒤 유효하지 않으면 명확한
예외(IllegalArgumentException 또는 커스텀 ValidateException)를 던지도록 변경하세요; 검증을 통과하면 기존
s3FileService.uploadMultipartFile(...) 및 s3FileService.getUploadedFileUrl(...)
호출을 진행하고 생성된 URL을 CreatePairGameApplierCommand.toEntity(...)에 전달해
pairGameService.create(...)를 호출하도록 유지하세요.
- Around line 40-48: In getPairGameMetaData, avoid mutating the original list
returned by
fileMetaDataService.getCoupledAllByDomainType(DomainType.CLUB_PROFILE) by
creating a defensive copy (e.g., new ArrayList<>(allClubProfileMetaData)) before
calling Collections.shuffle; then operate on the copied list when building
pairGameMetaData. Also handle empty or small lists by checking if the copied
list is empty and returning PairGameMetaDataQuery.of(Collections.emptyList())
(or an appropriate empty response) and ensure the .limit(18) logic still works
when size < 18 so you don’t throw or return null.
In `@src/main/java/ddingdong/ddingdongBE/file/service/S3FileService.java`:
- Around line 116-137: uploadMultipartFile currently calls
MultipartFile.getOriginalFilename() without null-checking before passing it to
extractFileExtension, which can cause NPE; add a null-safe handling: obtain
filename via file.getOriginalFilename(), if null or blank set a safe default or
throw a meaningful custom exception, then call extractFileExtension. Also
replace the raw RuntimeException thrown on IOException with the existing custom
AWS-related exception pattern (e.g., throw a new AwsClient or a new custom
FileReadException consistent with AwsService/AwsClient) so error handling is
consistent; update references in uploadMultipartFile and related exception
handling accordingly.
🧹 Nitpick comments (5)
src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/dto/query/PairGameMetaDataQuery.java (1)
7-15: 단일 필드 record에 대해@Builder사용이 과도할 수 있습니다.
images단일 필드만 가진 record에서@Builder와 정적 팩토리 메서드of()가 모두 존재합니다. 단순화를 위해 builder 없이 직접 생성자 또는of()메서드만 사용하는 것을 고려해 주세요.♻️ 간소화 제안
package ddingdong.ddingdongBE.domain.pairgame.service.dto.query; -import lombok.Builder; - import java.util.List; -@Builder public record PairGameMetaDataQuery( List<String> images ) { public static PairGameMetaDataQuery of(List<String> images) { - return PairGameMetaDataQuery.builder() - .images(images) - .build(); + return new PairGameMetaDataQuery(images); } }src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/dto/query/PairGameApplierAmountQuery.java (1)
5-12: 단일 primitive 필드에 대해@Builder사용이 과도합니다.
int amount단일 필드만 가진 record에서 builder 패턴은 불필요한 복잡성을 추가합니다. 직접 생성자 호출로 간소화할 수 있습니다.♻️ 간소화 제안
package ddingdong.ddingdongBE.domain.pairgame.service.dto.query; -import lombok.Builder; - -@Builder public record PairGameApplierAmountQuery( int amount ) { public static PairGameApplierAmountQuery of(int amount) { - return PairGameApplierAmountQuery.builder() - .amount(amount).build(); + return new PairGameApplierAmountQuery(amount); } }src/main/java/ddingdong/ddingdongBE/domain/pairgame/controller/dto/request/CreatePairGameApplierRequest.java (1)
17-23: 전화번호 및 학번 형식 검증 추가 고려
@NotNull만으로는 빈 문자열이나 잘못된 형식의 입력을 방지할 수 없습니다.@Pattern또는@NotBlank를 사용하여 형식 검증을 추가하는 것을 권장합니다.♻️ 개선 제안
- `@NotNull`(message = "응모자 학번은 필수 입력 사항입니다.") + `@NotBlank`(message = "응모자 학번은 필수 입력 사항입니다.") + `@Pattern`(regexp = "^[0-9]{8}$", message = "학번은 8자리 숫자여야 합니다.") `@Schema`(description = "응모자 학번", example = "60000000") String studentNumber, - `@NotNull`(message = "응모자 전화번호는 필수 입력 사항입니다.") + `@NotBlank`(message = "응모자 전화번호는 필수 입력 사항입니다.") + `@Pattern`(regexp = "^010-[0-9]{4}-[0-9]{4}$", message = "전화번호 형식이 올바르지 않습니다.") `@Schema`(description = "응모자 전화번호", example = "010-0000-0000") String phoneNumbersrc/main/resources/db/migration/V51__create_pair_game_applier_table.sql (1)
1-12: 중복 응모 방지 및 URL 길이 제약 조건 확인
student_fee_image_url필드의VARCHAR(255)크기 제약에 주의하세요. AWS S3 객체 키는 최대 1,024바이트이며, 일반적인 CDN URL 패턴(프로토콜 + 도메인 + S3 경로)을 고려하면 총 길이가 255자를 쉽게 초과할 수 있습니다.VARCHAR(512)이상으로 증가시키는 것을 권장합니다.또한
student_number에 유니크 제약 조건이 없어 동일 학생의 중복 응모가 가능합니다. 비즈니스 요구사항에 따라 제약 조건 추가를 고려해 주세요.♻️ 개선 제안
CREATE TABLE pair_game_applier ( id BIGINT NOT NULL AUTO_INCREMENT PRIMARY KEY, name VARCHAR(50) NOT NULL, department VARCHAR(50) NOT NULL, - student_number VARCHAR(50) NOT NULL, + student_number VARCHAR(50) NOT NULL UNIQUE, phone_number VARCHAR(50) NOT NULL, - student_fee_image_url VARCHAR(255) NOT NULL, + student_fee_image_url VARCHAR(512) NOT NULL, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NULL, updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP NULL, deleted_at TIMESTAMP NULL DEFAULT NULL ); + +CREATE INDEX idx_pair_game_applier_student_number ON pair_game_applier(student_number);src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/PairGameService.java (1)
22-24: 응모자 수 계산은count()로 대체 권장
findAll().size()는 전체 로우를 로딩하므로 불필요한 메모리/DB 비용이 큽니다.count()로 바로 집계하는 것이 안전합니다.♻️ 수정 제안
public PairGameApplierAmountQuery getPairGameApplierAmount() { - int amount = pairGameRepository.findAll().size(); + int amount = (int) pairGameRepository.count(); return PairGameApplierAmountQuery.of(amount); }
src/main/java/ddingdong/ddingdongBE/domain/pairgame/entity/PairGameApplier.java
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/pairgame/repository/PairGameRepository.java
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java
Show resolved
Hide resolved
Comment on lines
40
to
48
| public PairGameMetaDataQuery getPairGameMetaData() { | ||
| List<FileMetaData> allClubProfileMetaData = fileMetaDataService.getCoupledAllByDomainType(DomainType.CLUB_PROFILE); | ||
| Collections.shuffle(allClubProfileMetaData); | ||
| List<String> pairGameMetaData = allClubProfileMetaData.stream() | ||
| .limit(18) | ||
| .map(file -> s3FileService.getUploadedFileUrl(file.getFileKey()).cdnUrl()) | ||
| .collect(Collectors.toList()); | ||
| return PairGameMetaDataQuery.of(pairGameMetaData); | ||
| } |
There was a problem hiding this comment.
원본 리스트 수정 문제 및 빈 리스트 처리 누락
Collections.shuffle(allClubProfileMetaData)는 원본 리스트를 직접 수정합니다. FileMetaDataService가 캐시된 리스트를 반환하는 경우 예기치 않은 동작이 발생할 수 있습니다.
또한 allClubProfileMetaData가 비어있거나 18개 미만일 경우에 대한 처리가 없습니다.
🛡️ 수정 제안
public PairGameMetaDataQuery getPairGameMetaData() {
List<FileMetaData> allClubProfileMetaData = fileMetaDataService.getCoupledAllByDomainType(DomainType.CLUB_PROFILE);
- Collections.shuffle(allClubProfileMetaData);
- List<String> pairGameMetaData = allClubProfileMetaData.stream()
+ List<FileMetaData> shuffled = new ArrayList<>(allClubProfileMetaData);
+ Collections.shuffle(shuffled);
+ List<String> pairGameMetaData = shuffled.stream()
.limit(18)
.map(file -> s3FileService.getUploadedFileUrl(file.getFileKey()).cdnUrl())
.collect(Collectors.toList());
return PairGameMetaDataQuery.of(pairGameMetaData);
}🤖 Prompt for AI Agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java`
around lines 40 - 48, In getPairGameMetaData, avoid mutating the original list
returned by
fileMetaDataService.getCoupledAllByDomainType(DomainType.CLUB_PROFILE) by
creating a defensive copy (e.g., new ArrayList<>(allClubProfileMetaData)) before
calling Collections.shuffle; then operate on the copied list when building
pairGameMetaData. Also handle empty or small lists by checking if the copied
list is empty and returning PairGameMetaDataQuery.of(Collections.emptyList())
(or an appropriate empty response) and ensure the .limit(18) logic still works
when size < 18 so you don’t throw or return null.
KoSeonJe
reviewed
Jan 30, 2026
Collaborator
KoSeonJe
left a comment
KoSeonJe
left a comment
There was a problem hiding this comment.
코드 리뷰 🔍
전반적으로 깔끔한 구조와 계층 분리가 잘 되어있습니다. 몇 가지 개선 제안 사항을 남깁니다.
🔴 중요 - 보안 및 데이터 무결성
-
학번 중복 체크 필요
- 현재 동일 학번으로 중복 신청이 가능합니다.
PairGameRepository에existsByStudentNumber()메서드 추가 후 검증 로직 권장
-
Rate Limiting 고려
POST /pair-game/appliers가 permitAll로 열려있어 악의적인 대량 요청에 취약할 수 있습니다.- Rate limiting 또는 CAPTCHA 추가를 고려해주세요.
🟡 개선 제안
-
전화번호 형식 검증
CreatePairGameApplierRequest에@Pattern추가 권장
-
성능 최적화
getPairGameMetaData()에서 전체 파일 로드 후 셔플하는 방식은 데이터가 많아지면 성능 이슈 가능- DB 레벨 랜덤 샘플링 고려
✅ 좋은 점
- Command/Query 패턴으로 관심사 분리 잘 됨
- Swagger 문서화 충실
- Soft Delete 구현
위 사항들 검토 부탁드립니다! 🙏
KoSeonJe
requested changes
Jan 30, 2026
Collaborator
KoSeonJe
left a comment
KoSeonJe
left a comment
There was a problem hiding this comment.
고생하셨습니다 코멘트 확인해주세요.
서비스 통합테스트 작성 부탁드립니다.
s3 업로드같은 경우는 수동테스트 한번 부탁드려요
| @ResponseStatus(HttpStatus.CREATED) | ||
| @PostMapping("/pair-game/appliers") | ||
| void createPairGameApplier( | ||
| @Valid @RequestPart("request") CreatePairGameApplierRequest request, |
Collaborator
There was a problem hiding this comment.
p1) RequestPart 필요한가요? 내부에 MultipartFile이 존재하다면, 사용한다는데 분리되어있는 것 같아서요!
Collaborator
Author
There was a problem hiding this comment.
JSON 데이터와 MultipartFile를 함께 받으려면 RequestPart로 각각 받아야 한다고 알고 있어서 해당 방법으로 구현했습니다! 찾아보니 @ModelAttribute로 한 번에 받는 방법도 있는데, 혹시 해당 방법으로 바꿀 필요가 있을까요?
Collaborator
There was a problem hiding this comment.
requestdto 내부에 Multipartfile이 존재하지 않고, 따로 파라미터로 받고 있어서 질문하였습니다.
dto내부에는 json데이터만 담고 있는 것 같아서요!
Collaborator
Author
There was a problem hiding this comment.
@RequestPart로 MultipartFile을 받으려면 dto 내부에 위치하면 안 되는 것으로 알고 있어 해당 방식으로 구현하였습니다..!! 혹시 dto 내부에 위치시키는 다른 방식이 있을까요? 해당 방식은 프론트에서 form-data 형식으로 각각 key 지정해서 보내주면 되는 것 같습니다!
| public void createApplier(CreatePairGameApplierCommand createPairGameApplierCommand, MultipartFile studentFeeImageFile) { | ||
| if (studentFeeImageFile == null || studentFeeImageFile.isEmpty()) { | ||
| throw new UploadedFileNotFoundException(); | ||
| } |
Collaborator
There was a problem hiding this comment.
p2)
입력받을 때 request dto에 유효성 검증으로 하면 어떤가요?
src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/PairGameService.java`:
- Around line 23-25: The code in PairGameService.getPairGameApplierAmount casts
pairGameRepository.count() (a long) to int which can overflow; change the
conversion to use Math.toIntExact(pairGameRepository.count()) or, preferably,
update PairGameApplierAmountQuery to accept a long and pass the raw long value
from pairGameRepository.count(); update the call site
PairGameApplierAmountQuery.of(...) accordingly to preserve correctness for large
counts.
Comment on lines
+23
to
+25
| public PairGameApplierAmountQuery getPairGameApplierAmount() { | ||
| int amount = (int) pairGameRepository.count(); | ||
| return PairGameApplierAmountQuery.of(amount); |
There was a problem hiding this comment.
카운트 결과의 int 캐스팅 오버플로우 가능성
count()는 long을 반환하므로 큰 데이터에서는 오버플로우가 날 수 있습니다. Math.toIntExact로 안전하게 변환하거나 DTO를 long으로 바꾸는 쪽을 권장합니다.
🔧 제안 수정
- int amount = (int) pairGameRepository.count();
+ int amount = Math.toIntExact(pairGameRepository.count());📝 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.
Suggested change
| public PairGameApplierAmountQuery getPairGameApplierAmount() { | |
| int amount = (int) pairGameRepository.count(); | |
| return PairGameApplierAmountQuery.of(amount); | |
| public PairGameApplierAmountQuery getPairGameApplierAmount() { | |
| int amount = Math.toIntExact(pairGameRepository.count()); | |
| return PairGameApplierAmountQuery.of(amount); |
🤖 Prompt for AI Agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/PairGameService.java`
around lines 23 - 25, The code in PairGameService.getPairGameApplierAmount casts
pairGameRepository.count() (a long) to int which can overflow; change the
conversion to use Math.toIntExact(pairGameRepository.count()) or, preferably,
update PairGameApplierAmountQuery to accept a long and pass the raw long value
from pairGameRepository.count(); update the call site
PairGameApplierAmountQuery.of(...) accordingly to preserve correctness for large
counts.
Gopistol
approved these changes
Jan 31, 2026
Testcontainer API 버전 명시
팀 컨벤션에 맞추어 find -> get 수정
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/ddingdong/ddingdongBE/domain/club/service/FacadeAdminClubServiceImplTest.java (1)
69-80:⚠️ Potential issue | 🟡 Minor테스트 메서드명과 실제 테스트 대상 메서드 간 불일치
테스트 메서드명이
getAll()로 변경되었지만, 실제로 테스트하는 파사드 메서드는 여전히findAll()입니다 (Line 80). 테스트 메서드명은 테스트 대상 메서드와 일치해야 코드 가독성과 유지보수성이 향상됩니다.제안: 테스트 메서드명을 원래대로 유지하거나, 파사드 메서드명도 함께 변경
`@DisplayName`("어드민: 동아리 목록 조회") `@Test` -void getAll() { +void findAll() { // given List<Club> clubs = new ArrayList<>();또는 파사드 레이어의
findAll()메서드도getAll()로 변경하는 것을 검토해주세요.
🤖 Fix all issues with AI agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/club/service/GeneralClubService.java`:
- Around line 59-62: getAllByIds currently returns whatever
clubRepository.findAllById(ids) finds and can miss IDs (so later
clubNameMap.get(file.getEntityId()) yields null); modify getAllByIds to validate
that all requested IDs were returned (compare returned list size or build a Set
of returned IDs against input IDs) and either throw a clear exception (e.g.,
EntityNotFoundException) when any ID is missing or return a map of id->Club and
let callers handle missing entries; update callers such as
FacadeUserPairGameService to handle missing Clubs (avoid placing null values
into responses) by filtering out or substituting a safe value and ensure any
usage of clubNameMap.get(file.getEntityId()) checks for null before adding to
response.
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java`:
- Around line 35-43: The current createPairGameApplier method uploads to S3 via
s3FileService.uploadMultipartFile before calling pairGameService.create, which
can leave orphaned objects if the DB transaction rolls back; change the flow so
the S3 interaction is performed only after a successful commit or compensated on
failure: either (A) defer the upload until after commit by moving the upload
into a TransactionSynchronizationManager.registerSynchronization/afterCommit
callback (keep CreatePairGameApplierCommand and UploadedFileNotFoundException
checks in place, then call s3FileService.uploadMultipartFile inside afterCommit
and pass the returned key into pairGameService.create via a two-step approach or
by creating the entity without the key then update it post-commit), or (B) keep
pre-commit upload but register an afterCompletion callback to delete the
uploaded key if status == STATUS_ROLLED_BACK (call s3FileService.delete(key) on
rollback) so orphan cleanup is guaranteed; update createPairGameApplier to use
one of these patterns and reference s3FileService.uploadMultipartFile,
pairGameService.create, and CreatePairGameApplierCommand accordingly.
Comment on lines
+59
to
+62
| @Override | ||
| public List<Club> getAllByIds(List<Long> clubIds) { | ||
| return clubRepository.findAllById(clubIds); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A 5 'getAllByIds' --type javaRepository: COW-dev/ddingdong-be
Length of output: 4933
🏁 Script executed:
cat -n src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java | sed -n '50,70p'Repository: COW-dev/ddingdong-be
Length of output: 1255
🏁 Script executed:
# Check if FileMetaData has foreign key constraint to Club
rg -A 10 'class FileMetaData' --type javaRepository: COW-dev/ddingdong-be
Length of output: 6617
🏁 Script executed:
cat -n src/main/java/ddingdong/ddingdongBE/domain/filemetadata/entity/FileMetaData.javaRepository: COW-dev/ddingdong-be
Length of output: 3216
🏁 Script executed:
# Check the second call site more thoroughly
cat -n src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java | sed -n '100,120p'Repository: COW-dev/ddingdong-be
Length of output: 762
🏁 Script executed:
# Check if there's any null checking or validation for clubName after it's retrieved
cat -n src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java | sed -n '59,67p'Repository: COW-dev/ddingdong-be
Length of output: 591
🏁 Script executed:
# Check if there are any tests that show the expected behavior
rg -A 10 'getAllByIds' --type java -B 5 src/testRepository: COW-dev/ddingdong-be
Length of output: 46
🏁 Script executed:
# Check if Club entity has any cascade delete or soft delete
rg -A 20 'class Club' --type java src/main/java/ddingdong/ddingdongBE/domain/clubRepository: COW-dev/ddingdong-be
Length of output: 1953
getAllByIds 메서드의 반환 데이터 검증 필요
findAllById는 존재하지 않는 ID에 대해 예외를 발생시키지 않고 찾은 엔티티만 반환합니다. FileMetaData는 Club과의 외래 키 제약이 없어 entityId가 삭제된 Club을 참조할 수 있으며, 이 경우 clubNameMap.get(file.getEntityId())가 null을 반환합니다.
특히 FacadeUserPairGameService (line 60)에서 null 값이 response에 포함되므로, 반환된 데이터 크기 검증 또는 null 체크 로직이 필요합니다.
🤖 Prompt for AI Agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/club/service/GeneralClubService.java`
around lines 59 - 62, getAllByIds currently returns whatever
clubRepository.findAllById(ids) finds and can miss IDs (so later
clubNameMap.get(file.getEntityId()) yields null); modify getAllByIds to validate
that all requested IDs were returned (compare returned list size or build a Set
of returned IDs against input IDs) and either throw a clear exception (e.g.,
EntityNotFoundException) when any ID is missing or return a map of id->Club and
let callers handle missing entries; update callers such as
FacadeUserPairGameService to handle missing Clubs (avoid placing null values
into responses) by filtering out or substituting a safe value and ensure any
usage of clubNameMap.get(file.getEntityId()) checks for null before adding to
response.
Comment on lines
+35
to
+43
| @Transactional | ||
| public void createPairGameApplier(CreatePairGameApplierCommand createPairGameApplierCommand) { | ||
| MultipartFile studentFeeImageFile = createPairGameApplierCommand.studentFeeImageFile(); | ||
| if (studentFeeImageFile == null || studentFeeImageFile.isEmpty()) { | ||
| throw new UploadedFileNotFoundException(); | ||
| } | ||
| pairGameService.validateStudentNumberUnique(createPairGameApplierCommand.studentNumber()); | ||
| String key = s3FileService.uploadMultipartFile(studentFeeImageFile, LocalDateTime.now(), "pair-game"); | ||
| pairGameService.create(createPairGameApplierCommand.toEntity(key)); |
There was a problem hiding this comment.
트랜잭션 내 S3 업로드로 인한 고아 파일 가능성
Line 42에서 S3 업로드가 선행되고, 이후 DB 저장(Line 43) 실패/롤백 시 S3 객체가 남을 수 있습니다. 업로드 실패 보상(삭제) 또는 커밋 이후 업로드(트랜잭션 동기화/이벤트)로 분리하는 흐름을 고려해주세요.
🤖 Prompt for AI Agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java`
around lines 35 - 43, The current createPairGameApplier method uploads to S3 via
s3FileService.uploadMultipartFile before calling pairGameService.create, which
can leave orphaned objects if the DB transaction rolls back; change the flow so
the S3 interaction is performed only after a successful commit or compensated on
failure: either (A) defer the upload until after commit by moving the upload
into a TransactionSynchronizationManager.registerSynchronization/afterCommit
callback (keep CreatePairGameApplierCommand and UploadedFileNotFoundException
checks in place, then call s3FileService.uploadMultipartFile inside afterCommit
and pass the returned key into pairGameService.create via a two-step approach or
by creating the entity without the key then update it post-commit), or (B) keep
pre-commit upload but register an afterCompletion callback to delete the
uploaded key if status == STATUS_ROLLED_BACK (call s3FileService.delete(key) on
rollback) so orphan cleanup is guaranteed; update createPairGameApplier to use
one of these patterns and reference s3FileService.uploadMultipartFile,
pairGameService.create, and CreatePairGameApplierCommand accordingly.
게임 메타데이터 조회 API 응답에 category 추가
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/controller/dto/response/PairGameMetaDataResponse.java`:
- Around line 25-36: PairGameMetaDataResponse.from(PairGameMetaDataQuery)
currently assumes query and query.metaData() are non-null and can NPE; add
defensive checks: validate query at method start (e.g.,
Objects.requireNonNull(query, "query must not be null") or throw
IllegalArgumentException) and replace direct query.metaData() use with a
null-safe alternative (e.g., List.of() when metaData is null or
Optional.ofNullable(query.metaData()).orElseGet(List::of)) before streaming;
also add a null check inside
PairGameClubAndImageResponse.from(PairGameClubAndImageQuery) to validate its
query parameter similarly so mapping .map(PairGameClubAndImageResponse::from) is
safe.
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java`:
- Around line 60-66: The mapping in FacadeUserPairGameService that builds
pairGameMetaData can NPE when clubMap.get(file.getEntityId()) returns null;
update the stream mapping over selectedMetaData to check the retrieved Club
(from clubMap) and if null throw a clear exception (e.g., IllegalStateException
or your domain NotFoundException) including the missing entityId, or
alternatively filter out missing entries before mapping; ensure the error
message references the file.getEntityId() so callers can diagnose the missing
Club, and keep usage of PairGameClubAndImageQuery and
s3FileService.getUploadedFileUrl(file.getFileKey()) unchanged after the
null-check.
Comment on lines
+25
to
+36
| public static PairGameClubAndImageResponse from(PairGameClubAndImageQuery query) { | ||
| return PairGameClubAndImageResponse.builder() | ||
| .clubName(query.clubName()) | ||
| .category(query.category()) | ||
| .imageUrl(query.imageUrl()) | ||
| .build(); | ||
| } | ||
| } | ||
| public static PairGameMetaDataResponse from(PairGameMetaDataQuery query) { | ||
| List<PairGameClubAndImageResponse> responses = query.metaData().stream() | ||
| .map(PairGameClubAndImageResponse::from) | ||
| .toList(); |
There was a problem hiding this comment.
from(...) 매핑 시 null 방어(또는 계약 명시)가 필요합니다.
query 또는 query.metaData()가 null이면 NPE가 발생합니다. 서비스 계층에서 항상 non-null을 보장하지 않는다면 방어 로직(또는 명시적 검증) 추가를 고려해주세요.
🛠️ 수정 제안
import lombok.Builder;
import java.util.List;
+import java.util.Objects;
) {
public static PairGameClubAndImageResponse from(PairGameClubAndImageQuery query) {
+ Objects.requireNonNull(query, "query must not be null");
return PairGameClubAndImageResponse.builder()
.clubName(query.clubName())
.category(query.category())
.imageUrl(query.imageUrl())
.build();
}
}
public static PairGameMetaDataResponse from(PairGameMetaDataQuery query) {
- List<PairGameClubAndImageResponse> responses = query.metaData().stream()
- .map(PairGameClubAndImageResponse::from)
- .toList();
+ Objects.requireNonNull(query, "query must not be null");
+ List<PairGameClubAndImageResponse> responses =
+ query.metaData() == null ? List.of()
+ : query.metaData().stream()
+ .map(PairGameClubAndImageResponse::from)
+ .toList();
return PairGameMetaDataResponse.builder()
.metaData(responses)
.build();
}📝 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.
Suggested change
| public static PairGameClubAndImageResponse from(PairGameClubAndImageQuery query) { | |
| return PairGameClubAndImageResponse.builder() | |
| .clubName(query.clubName()) | |
| .category(query.category()) | |
| .imageUrl(query.imageUrl()) | |
| .build(); | |
| } | |
| } | |
| public static PairGameMetaDataResponse from(PairGameMetaDataQuery query) { | |
| List<PairGameClubAndImageResponse> responses = query.metaData().stream() | |
| .map(PairGameClubAndImageResponse::from) | |
| .toList(); | |
| import lombok.Builder; | |
| import java.util.List; | |
| import java.util.Objects; | |
| public static PairGameClubAndImageResponse from(PairGameClubAndImageQuery query) { | |
| Objects.requireNonNull(query, "query must not be null"); | |
| return PairGameClubAndImageResponse.builder() | |
| .clubName(query.clubName()) | |
| .category(query.category()) | |
| .imageUrl(query.imageUrl()) | |
| .build(); | |
| } | |
| } | |
| public static PairGameMetaDataResponse from(PairGameMetaDataQuery query) { | |
| Objects.requireNonNull(query, "query must not be null"); | |
| List<PairGameClubAndImageResponse> responses = | |
| query.metaData() == null ? List.of() | |
| : query.metaData().stream() | |
| .map(PairGameClubAndImageResponse::from) | |
| .toList(); | |
| return PairGameMetaDataResponse.builder() | |
| .metaData(responses) | |
| .build(); | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/controller/dto/response/PairGameMetaDataResponse.java`
around lines 25 - 36, PairGameMetaDataResponse.from(PairGameMetaDataQuery)
currently assumes query and query.metaData() are non-null and can NPE; add
defensive checks: validate query at method start (e.g.,
Objects.requireNonNull(query, "query must not be null") or throw
IllegalArgumentException) and replace direct query.metaData() use with a
null-safe alternative (e.g., List.of() when metaData is null or
Optional.ofNullable(query.metaData()).orElseGet(List::of)) before streaming;
also add a null check inside
PairGameClubAndImageResponse.from(PairGameClubAndImageQuery) to validate its
query parameter similarly so mapping .map(PairGameClubAndImageResponse::from) is
safe.
Comment on lines
60
to
66
| List<PairGameClubAndImageQuery> pairGameMetaData = selectedMetaData.stream().map(file -> { | ||
| Club club = clubMap.get(file.getEntityId()); | ||
| return PairGameClubAndImageQuery.of( | ||
| club.getName(), | ||
| club.getCategory(), | ||
| s3FileService.getUploadedFileUrl(file.getFileKey()).cdnUrl() | ||
| ); |
There was a problem hiding this comment.
클럽 미존재 시 NPE 가능성
clubMap.get(...)가 null이면 club.getName()에서 NPE가 납니다. 누락된 클럽을 명확히 처리해주세요.
🔧 수정 제안
- List<PairGameClubAndImageQuery> pairGameMetaData = selectedMetaData.stream().map(file -> {
- Club club = clubMap.get(file.getEntityId());
- return PairGameClubAndImageQuery.of(
- club.getName(),
- club.getCategory(),
- s3FileService.getUploadedFileUrl(file.getFileKey()).cdnUrl()
- );
- }).toList();
+ List<PairGameClubAndImageQuery> pairGameMetaData = selectedMetaData.stream()
+ .map(file -> {
+ Club club = clubMap.get(file.getEntityId());
+ if (club == null) {
+ throw new IllegalStateException("클럽 정보를 찾을 수 없습니다. clubId=" + file.getEntityId());
+ }
+ return PairGameClubAndImageQuery.of(
+ club.getName(),
+ club.getCategory(),
+ s3FileService.getUploadedFileUrl(file.getFileKey()).cdnUrl()
+ );
+ })
+ .toList();🤖 Prompt for AI Agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java`
around lines 60 - 66, The mapping in FacadeUserPairGameService that builds
pairGameMetaData can NPE when clubMap.get(file.getEntityId()) returns null;
update the stream mapping over selectedMetaData to check the retrieved Club
(from clubMap) and if null throw a clear exception (e.g., IllegalStateException
or your domain NotFoundException) including the missing entityId, or
alternatively filter out missing entries before mapping; ensure the error
message references the file.getEntityId() so callers can diagnose the missing
Club, and keep usage of PairGameClubAndImageQuery and
s3FileService.getUploadedFileUrl(file.getFileKey()) unchanged after the
null-check.
KoSeonJe
approved these changes
Feb 3, 2026
Collaborator
KoSeonJe
left a comment
KoSeonJe
left a comment
There was a problem hiding this comment.
고생하셨습니다! 간단한 코멘트만 남겼습니다
| @ResponseStatus(HttpStatus.CREATED) | ||
| @PostMapping("/pair-game/appliers") | ||
| void createPairGameApplier( | ||
| @Valid @RequestPart("request") CreatePairGameApplierRequest request, |
Collaborator
There was a problem hiding this comment.
requestdto 내부에 Multipartfile이 존재하지 않고, 따로 파라미터로 받고 있어서 질문하였습니다.
dto내부에는 json데이터만 담고 있는 것 같아서요!
| } | ||
|
|
||
| public PairGameMetaDataQuery getPairGameMetaData() { | ||
| List<FileMetaData> allClubProfileMetaData = fileMetaDataService.getCoupledAllByDomainType(DomainType.CLUB_PROFILE); |
Collaborator
There was a problem hiding this comment.
p3)
현재 파일메타데이터 내 club_profile 타입이 정상적인 값들로만 구성되어있는지 확인해보면 좋을 것 같아요
Comment on lines
61
to
66
| Club club = clubMap.get(file.getEntityId()); | ||
| return PairGameClubAndImageQuery.of( | ||
| club.getName(), | ||
| club.getCategory(), | ||
| s3FileService.getUploadedFileUrl(file.getFileKey()).cdnUrl() | ||
| ); |
Collaborator
There was a problem hiding this comment.
p3)
들여쓰기가 되어있는데, 확인해주시면 감사하겠습니다.
Collaborator
Author
There was a problem hiding this comment.
인덴트 레벨 줄였습니다! 감사합니다!
| department VARCHAR(50) NOT NULL, | ||
| student_number VARCHAR(50) NOT NULL UNIQUE, | ||
| phone_number VARCHAR(50) NOT NULL, | ||
| student_fee_image_url VARCHAR(255) NOT NULL, |
Collaborator
Author
There was a problem hiding this comment.
저번에 말씀해주신대로 key를 저장하기로 했어서 넘진 않을 것 같습니다! 컬럼명 수정해두겠습니다!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java`:
- Line 58: The current clubMap creation using Collectors.toMap(Club::getId, club
-> club) can throw IllegalStateException when duplicate Club IDs exist; update
the code that builds clubMap (the expression creating Map<Long, Club> clubMap
from clubs) to provide a merge function or use a grouping approach so duplicate
keys are resolved (for example, keep the existing entry or decide how to merge
Club entries) to prevent the exception at runtime.
| List<Long> clubIds = selectedMetaData.stream().map(FileMetaData::getEntityId).toList(); | ||
| List<Club> clubs = clubService.getAllByIds(clubIds); | ||
|
|
||
| Map<Long, Club> clubMap = clubs.stream().collect(Collectors.toMap(Club::getId, club -> club)); |
There was a problem hiding this comment.
중복 클럽 ID 시 예외 발생 가능
Collectors.toMap(Club::getId, club -> club)는 동일한 클럽 ID가 존재할 경우 IllegalStateException을 발생시킵니다. selectedMetaData에 같은 클럽의 여러 파일이 포함될 수 있다면 merge function 추가가 필요합니다.
🛡️ 수정 제안
-Map<Long, Club> clubMap = clubs.stream().collect(Collectors.toMap(Club::getId, club -> club));
+Map<Long, Club> clubMap = clubs.stream().collect(Collectors.toMap(Club::getId, club -> club, (existing, replacement) -> existing));🤖 Prompt for AI Agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/pairgame/service/FacadeUserPairGameService.java`
at line 58, The current clubMap creation using Collectors.toMap(Club::getId,
club -> club) can throw IllegalStateException when duplicate Club IDs exist;
update the code that builds clubMap (the expression creating Map<Long, Club>
clubMap from clubs) to provide a merge function or use a grouping approach so
duplicate keys are resolved (for example, keep the existing entry or decide how
to merge Club entries) to prevent the exception at runtime.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚀 작업 내용
응모자 생성 및 현황 조회 기능 구현하였습니다.
게임 메타데이터 조회(이미지 리스트 조회) 기능 구현하였습니다.
게임 메타데이터 조회 API 관련하여 원래 설계했던 대로 모든 이미지를 전송하면 이미지 업로드 소요 시간이 다소 길어질 것 같다고 프론트 측에서 말씀해주셨습니다. 따라서 필요한 18개의 이미지만 랜덤으로 뽑아 전송하도록 하였습니다.
++
디자인 요구사항 충족을 위해 게임 메타데이터 조회 API 응답 형식 변경 후 동아리 이름 추가하였습니다.
서비스 통합 테스트 작성하였습니다.
🤔 고민했던 내용
💬 리뷰 중점사항
pr 사이즈가 너무 커져버렸네요... 죄송합니다... 다음부터 주의하겠습니다.
Summary by CodeRabbit