[DDING-000] form, form_field soft delete 적용#325
Conversation
KoSeonJe
requested review from
5uhwann,
Seooooo24 and
wonjunYou
as code owners
WalkthroughForm 및 FormField 엔티티에 소프트 삭제(deleted_at) 도입 및 DB 마이그레이션 추가. ClubMember 관련 서비스 및 리포지토리에 배치 업데이트/조회 메서드와 삭제 호출 흐름(GeneralClubMemberService: deleteAll 사용)·Club.removeAll 추가, FacadeCentralClubMemberServiceImpl의 동작 흐름 수정 및 관련 테스트 갱신이 포함됨. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Service as GeneralClubMemberService
participant Repo as ClubMemberRepository
participant DB
Note over Service,Repo: 배치 업데이트/삭제 흐름 (신규)
Client->>Service: updateAll(updateClubMemberInfos)
Service->>Repo: findByIdIn(ids from updateClubMemberInfos)
Repo->>DB: SELECT ... WHERE id IN (...)
DB-->>Repo: existingMembers
Service->>Service: for each existing -> updateInformation(from map)
Service->>Repo: saveAll? / flush (persist 변경)
Note over Service,Repo: 삭제되는 멤버 계산
Service->>Service: club.removeAll(deletedMembers)
Service->>Repo: deleteAll(deletedMembers)
Repo->>DB: DELETE / update (soft delete handled at 엔티티 레벨 for form/form_field)
DB-->>Repo: deleted
sequenceDiagram
autonumber
participant Client
participant Service as FormService (JPA/Hibernate)
participant JPA
participant DB
Note over JPA,DB: soft-delete via @SQLDelete/@SQLRestriction
Client->>Service: delete(formId)
Service->>JPA: EntityManager.remove(Form)
JPA->>DB: UPDATE form SET deleted_at = CURRENT_TIMESTAMP WHERE id = ?
DB-->>JPA: updated
JPA-->>Service: 완료
Note over JPA,DB: 이후 조회 시 WHERE deleted_at IS NULL 자동 적용
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ 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. 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: 1
🧹 Nitpick comments (6)
src/main/resources/db/migration/V45__add_form_deleted_at.sql (1)
1-5: 소프트 삭제 기본 필터 성능 강화를 위한 인덱스 추가 제안deleted_at으로 항상 필터링되므로 인덱스가 없으면 스캔 비용이 큽니다. 단일 인덱스(form)와 조합 인덱스(form_field: form_id, deleted_at)를 권장합니다.
ALTER TABLE form ADD deleted_at timestamp NULL; ALTER TABLE form_field ADD deleted_at timestamp NULL; + +-- Soft delete 조회 성능 개선용 인덱스 +CREATE INDEX IF NOT EXISTS idx_form_deleted_at ON form (deleted_at); +CREATE INDEX IF NOT EXISTS idx_form_field_form_id_deleted_at ON form_field (form_id, deleted_at);src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/GeneralClubMemberService.java (1)
31-34: 대량 삭제 시 N건 개별 UPDATE로 인한 성능 부담 완화 방안대량 목록에서 deleteAll은 엔티티 단위로 UPDATE가 N회 발생합니다. JPQL 벌크 소프트 삭제 메서드 추가를 고려해 주세요.
Service 쪽 변경 예:
- clubMemberRepository.deleteAll(clubMembers); + var ids = clubMembers.stream().map(ClubMember::getId).toList(); + clubMemberRepository.softDeleteAllById(ids);Repository 예시(별도 파일):
public interface ClubMemberRepository extends JpaRepository<ClubMember, Long> { @Modifying @Query("update ClubMember c set c.deletedAt = CURRENT_TIMESTAMP where c.id in :ids") int softDeleteAllById(@Param("ids") List<Long> ids); }추가로 Hibernate 배치 설정(hibernate.jdbc.batch_size)도 함께 확인해 주세요.
src/main/java/ddingdong/ddingdongBE/domain/form/entity/Form.java (3)
62-64: deleted_at 매핑 명시화 권장다른 엔티티(Club, ClubMember 등)와 동일하게 컬럼명을 명시하면 스키마와의 정합성이 더 확실해집니다.
- @Column(columnDefinition = "TIMESTAMP") + @Column(name = "deleted_at", columnDefinition = "TIMESTAMP") private LocalDateTime deletedAt;
107-115: 폼 필드 삭제 경로의 성능 주의removeAll + orphanRemoval은 소프트 삭제 UPDATE가 필드 수만큼 발생합니다. 필드가 많은 폼에서는 비용이 커질 수 있으니, 필요 시 FormField에 대해서도 벌크 소프트 삭제(IDs 기반 JPQL UPDATE) 도입을 검토해 주세요.
89-92: 메서드 네이밍 가독성 개선(Nitpick)addFormFields는 단수 파라미터를 받아 실제로는 하나를 추가합니다. addFormField로의 이름 변경을 고려해 주세요.
src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java (1)
27-28: 소프트 삭제 설정 사용 적절Hibernate @SQLDelete + @SQLRestriction 조합으로 기본 조회에서 삭제 데이터 배제 동작이 명확합니다. 다만 native/bulk 쿼리는 제한을 우회하니 서비스/리포지토리 계층에서 사용 시 주의해주세요. 빈번한 조회가 있다면 deleted_at에 대한 인덱스(예: (form_id, deleted_at))도 고려해 주세요.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/GeneralClubMemberService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/entity/Form.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java(2 hunks)src/main/resources/db/migration/V45__add_form_deleted_at.sql(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java (5)
src/main/java/ddingdong/ddingdongBE/domain/form/entity/Form.java (1)
Entity(30-155)src/main/java/ddingdong/ddingdongBE/domain/club/entity/Club.java (1)
Entity(29-131)src/main/java/ddingdong/ddingdongBE/domain/clubmember/entity/ClubMember.java (1)
Entity(29-97)src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormAnswer.java (1)
Entity(25-67)src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplication.java (1)
Entity(14-81)
src/main/java/ddingdong/ddingdongBE/domain/form/entity/Form.java (5)
src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java (1)
Entity(24-106)src/main/java/ddingdong/ddingdongBE/domain/club/entity/Club.java (1)
Entity(29-131)src/main/java/ddingdong/ddingdongBE/domain/clubmember/entity/ClubMember.java (1)
Entity(29-97)src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormAnswer.java (1)
Entity(25-67)src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplication.java (1)
Entity(14-81)
⏰ 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_and_push
🔇 Additional comments (5)
src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/GeneralClubMemberService.java (2)
33-34: deleteAllInBatch → deleteAll 전환 잘 하셨습니다하드 삭제 우회 문제를 피하고 @SQLDelete를 타게 됩니다. 변경 방향 적절합니다.
33-34: 완료: 레거시 하드 삭제 경로 없음 확인
저장소 전역에서 deleteAllInBatch, JPQL/Native bulk delete 호출이 검색되지 않아 잔존 경로가 없습니다.src/main/java/ddingdong/ddingdongBE/domain/form/entity/Form.java (1)
33-35: 소프트 삭제 설정(@SQLDelete/@SQLRestriction) 추가 적절엔티티 기본 조회에서 삭제행을 배제하는 의도가 정확히 반영되었습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java (2)
21-22: 필요한 Hibernate 어노테이션 import 추가 적절변경 의도와 일치하며 불필요한 import 없음으로 보입니다.
15-15: LocalDateTime import OK신규 필드와 일치하며 불필요한 의존성 없습니다.
| @Column(columnDefinition = "TIMESTAMP") | ||
| private LocalDateTime deletedAt; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
deleted_at 컬럼 매핑을 명시적으로 고정 권장
SQLDelete에서 "deleted_at"를 사용하므로 컬럼 매핑도 명시해 두는 편이 안전합니다(네이밍 전략 변경 시 깨짐 방지, 타 엔티티와의 일관성 개선).
적용 diff:
- @Column(columnDefinition = "TIMESTAMP")
+ @Column(name = "deleted_at", columnDefinition = "TIMESTAMP")
private LocalDateTime deletedAt;다음 스크립트로 네이밍 전략 사용 여부를 확인해 주세요(스네이크 케이스 자동 변환 의존 여부 점검).
🏁 Script executed:
#!/bin/bash
# Hibernate/Spring 네이밍 전략 설정 탐색
rg -n -C2 -e 'hibernate.*naming' -e 'PhysicalNamingStrategy' -e 'ImplicitNamingStrategy' -e 'spring\.jpa\.hibernate\.naming' -- \
application*.yml application*.properties src/main/resources || true
# deleted_at 명시 매핑 사용 현황
rg -n '@Column\s*\(\s*name\s*=\s*"deleted_at"' -SLength of output: 2092
deletedAt 컬럼에 name 속성 명시
현재 application 설정에 NamingStrategy가 별도 지정되어 있지 않아 기본 전략(camelCase→snake_case)에 의존 중입니다. 다른 엔티티들도 @Column(name = "deleted_at", …)을 사용하므로, FormField에도 일관성 유지와 네이밍 전략 변경 대비를 위해 다음과 같이 수정하세요.
- @Column(columnDefinition = "TIMESTAMP")
+ @Column(name = "deleted_at", columnDefinition = "TIMESTAMP")[src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java:57-58]
📝 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.
| @Column(columnDefinition = "TIMESTAMP") | |
| private LocalDateTime deletedAt; | |
| @Column(name = "deleted_at", columnDefinition = "TIMESTAMP") | |
| private LocalDateTime deletedAt; |
🤖 Prompt for AI Agents
In src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java around
lines 57-58, the @Column on deletedAt lacks an explicit name which relies on the
default naming strategy; update the annotation to specify the column name as
deleted_at (e.g., @Column(name = "deleted_at", columnDefinition = "TIMESTAMP"))
so it matches other entities and remains stable if NamingStrategy changes.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
src/main/java/ddingdong/ddingdongBE/domain/clubmember/repository/ClubMemberRepository.java (2)
10-10: 불필요한 파생 쿼리 제거:findAllById로 대체 권장Spring Data JPA가 기본 제공하는
findAllById(Iterable<ID>)로 동일 목적을 달성할 수 있습니다. 레포지토리 표면적을 줄여 유지보수성을 높이죠.적용 diff:
- List<ClubMember> findByIdIn(Collection<Long> ids);참고: 서비스 쪽에서는
clubMemberRepository.findAllById(updatedMemberMap.keySet())로 대체하면 됩니다.
12-12: 조회 결과 순서 보장 필요 시 정렬 메서드/파라미터 추가 고려현재
findByClubId(Long clubId)는 결과 순서를 보장하지 않습니다. 동기화/비교 로직에서 결정성이 필요하다면 정렬을 반영하세요.옵션:
- 메서드 시그니처 변경
- List<ClubMember> findByClubId(Long clubId); + List<ClubMember> findByClubIdOrderByIdAsc(Long clubId);
- 또는 오버로드로
Sort파라미터 허용List<ClubMember> findByClubId(Long clubId, Sort sort);운영 측면: DB에
club_member.club_id인덱스 존재 여부도 확인해 주세요. 없으면 마이그레이션으로 추가 권장합니다.src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/ClubMemberService.java (1)
18-21: 메서드 계약 명확화 및 파라미터 네이밍 개선 제안
updateAll(List<ClubMember> updateClubMemberInfos)가 "엔티티 자체를 업데이트 명세로" 사용하고 있어 의도가 모호합니다. 변경 허용 필드의 통제를 위해 UpdateCommand/DTO 도입 또는 네이밍 단순화(예:updates)를 권장합니다.간단 네이밍 개선:
- void updateAll(List<ClubMember> updateClubMemberInfos); + void updateAll(List<ClubMember> updates);동시에 javadoc/코멘트로 "ID 필수, 생성용 엔티티 금지" 등을 명시하면 사용 오류를 줄일 수 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/GeneralClubMemberService.java (1)
62-65: 정렬 결정성 필요 시 반영 권장
getByClubId는 현재 정렬이 없어 호출 시점/DB 플랜에 따라 순서가 달라질 수 있습니다. 비교/동기화 로직에서 순서를 기대한다면 정렬을 명시하세요(예: ID ASC).src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceImpl.java (2)
59-62: 삭제 흐름 점검: soft delete 보장 및 영속성 컨텍스트 상태 확인
- 순서(분리 → 삭제)는 타당합니다. 다만
deleteAll이 소프트 삭제를 확실히 보장하는지(하드 삭제 금지), 그리고 이후club.getClubMembers()등 컬렉션 캐시가 갱신되는지(= 일관성) 확인 부탁드립니다.필요 시 서비스에 원자적 메서드(예:
deleteAllAndDetach(club, members))로 캡슐화하는 것도 고려해주세요.
111-111: 콘솔 출력 삭제 및 로거로 대체
System.out.println은 남기지 마세요. 디버그 로그로 교체 권장.- System.out.println(deletedMemberIds); + if (log.isDebugEnabled()) { + log.debug("deletedMemberIds={}", deletedMemberIds); + }src/test/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceTest.java (2)
93-93: 엑셀 ID를 숫자 셀로 기록하면 정밀도 손실 위험Apache POI의
setCellValue(long)오버로드가 없어 double로 기록됩니다. ID가 커지면 정밀도 손실 가능성이 있습니다. 문자열로 기록하거나 extractor가 문자열 ID도 처리하도록 보장해 주세요.- row1.createCell(0).setCellValue(existingMembers.get(0).getId()); + row1.createCell(0).setCellValue(String.valueOf(existingMembers.get(0).getId())); ... - row2.createCell(0).setCellValue(existingMembers.get(1).getId()); + row2.createCell(0).setCellValue(String.valueOf(existingMembers.get(1).getId()));또는, 현 방식을 유지한다면 extractor가 숫자 → Long 변환 시 범위를 점검하는지 확인 부탁드립니다.
Also applies to: 102-102
129-134: 검증 보강 제안: soft delete 대상 ID 부재 및 다른 필드 업데이트 확인이름만이 아니라 삭제 대상 ID의 부재와 포지션/학과 업데이트도 함께 검증하면 회귀 방지에 도움이 됩니다.
List<ClubMember> remainingMembers = clubMemberRepository.findAll(); assertThat(remainingMembers).hasSize(2); // member3는 soft delete됨 assertThat(remainingMembers) .extracting(ClubMember::getName) .containsExactlyInAnyOrder("수정된멤버1", "수정된멤버2"); + // 삭제 대상 ID가 존재하지 않는지 확인 + Long deletedId = existingMembers.get(2).getId(); + assertThat(remainingMembers).noneMatch(m -> Objects.equals(m.getId(), deletedId)); + // 다른 필드 업데이트도 확인 + assertThat(remainingMembers).allSatisfy(m -> { + assertThat(m.getPosition().name()).isEqualTo("MEMBER"); + assertThat(m.getDepartment()).isEqualTo("컴퓨터공학과"); + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/ddingdong/ddingdongBE/domain/club/entity/Club.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/clubmember/repository/ClubMemberRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/ClubMemberService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceImpl.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/GeneralClubMemberService.java(3 hunks)src/test/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceTest.java (2)
src/test/java/ddingdong/ddingdongBE/common/fixture/UserFixture.java (1)
UserFixture(6-14)src/test/java/ddingdong/ddingdongBE/common/fixture/ClubFixture.java (1)
ClubFixture(11-30)
⏰ 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_and_push
🔇 Additional comments (4)
src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/GeneralClubMemberService.java (1)
33-37: ClubMember 엔티티에 소프트 삭제가 적용되어 deleteAll 호출은 하드 삭제가 아닙니다
ClubMember 클래스에 @SQLDelete/@SQLRestriction 애너테이션이 적용되어 있어 deleteAll·delete 메서드는 소프트 삭제로 동작하므로, 동기화 플로우에서 호출을 제거할 필요가 없습니다.src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceImpl.java (1)
48-48: getByClubId가 소프트 삭제 레코드를 제외하는지 확인 필요soft delete 적용 이후 조회 기본 정책이 중요합니다.
clubMemberService.getByClubId(club.getId())가 deleted(예: deletedAt != null) 레코드를 자동 제외하는지 확인해주세요. 포함된다면 동기화 로직이 오작동할 수 있습니다.src/test/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceTest.java (2)
65-78: 테스트 시나리오 구성 명확하고 재현성 좋습니다실제 영속 엔티티로 3명을 만들고 엑셀 입력으로 2명만 유지하는 플로우가 명확합니다. 컬렉션 양방향 연관까지 설정한 점도 좋습니다.
138-138: 메서드명 갱신 일관성 OK테스트 메서드명(
updateAll)을 실제 검증 의도에 맞게 정리한 점 좋습니다.
| public void removeAll(final List<ClubMember> deletedMembers) { | ||
| this.clubMembers.removeAll(deletedMembers); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
양방향 연관관계 일관성 보장: owning side(ClubMember.club)도 끊어주세요
removeAll이 컬렉션에서만 제거하고 있어 영속성 컨텍스트 내 객체 그래프가 불일치합니다. 즉시 삭제하지 않고 별도 삭제 로직을 태우는 현재 플로우에서도 in-memory 상태 불일치/예상치 못한 flush 사이드이펙트가 생길 수 있습니다.
적용 diff:
- public void removeAll(final List<ClubMember> deletedMembers) {
- this.clubMembers.removeAll(deletedMembers);
- }
+ public void removeAll(final List<ClubMember> deletedMembers) {
+ if (deletedMembers == null || deletedMembers.isEmpty()) {
+ return;
+ }
+ // owning side 정리(ClubMember.club) 후 컬렉션에서 제거
+ deletedMembers.forEach(member -> member.setClubForConvenience(null));
+ this.clubMembers.removeAll(deletedMembers);
+ }setClubForConvenience(null)가 허용되지 않는다면, null 허용 또는 unlinkFromClub() 같은 전용 메서드 제공을 권장합니다.
📝 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.
| public void removeAll(final List<ClubMember> deletedMembers) { | |
| this.clubMembers.removeAll(deletedMembers); | |
| } | |
| public void removeAll(final List<ClubMember> deletedMembers) { | |
| if (deletedMembers == null || deletedMembers.isEmpty()) { | |
| return; | |
| } | |
| // owning side 정리(ClubMember.club) 후 컬렉션에서 제거 | |
| deletedMembers.forEach(member -> member.setClubForConvenience(null)); | |
| this.clubMembers.removeAll(deletedMembers); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/ddingdong/ddingdongBE/domain/club/entity/Club.java around lines
132 to 134, removeAll currently only removes members from the clubMembers
collection causing the bidirectional association to become inconsistent; iterate
deletedMembers and for each member unset the owning side (e.g., call
member.setClubForConvenience(null) or member.unlinkFromClub()), then remove them
from the collection (or do removal as part of unlink method); if
setClubForConvenience cannot accept null, add a dedicated unlinkFromClub()
method on ClubMember that clears the club reference and any related fields, and
call that for each deleted member before/while removing from the collection to
keep in-memory graph and persistence state consistent.
| Set<Long> updatedMemberInfoIds = updateClubMemberInfos.stream() | ||
| .map(ClubMember::getId) | ||
| .collect(Collectors.toSet()); | ||
| Set<Long> currentMemberIds = clubMembers.stream() | ||
| .map(ClubMember::getId) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| clubMemberService.saveAll(filterCreatedMembers(updatedClubMembers, updatedMemberIds, currentMemberIds)); | ||
| clubMemberService.deleteAll(filterDeletedMembers(clubMembers, updatedMemberIds, currentMemberIds)); | ||
| clubMemberService.saveAll(filterCreatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds)); | ||
| clubMemberService.updateAll(filterUpdatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds)); | ||
|
|
There was a problem hiding this comment.
엑셀 ID 기반 분기 로직에 데이터 오염 위험: 타 동아리/유효하지 않은 ID가 '생성'으로 처리될 수 있음
현재 “생성/수정” 분기가 “업데이트 목록 ID 집합 - 현재 회원 ID 집합”으로만 판단됩니다. 엑셀에 타 동아리의 기존 ID(또는 존재하지 않는 ID)가 들어오면, saveAll 시 JPA가 INSERT가 아니라 UPDATE(merge)로 처리해 타 동아리 데이터를 덮어쓸 수 있습니다. “생성”은 반드시 ID == null 인 경우로 한정하고, ID가 있으나 현재 동아리에 속하지 않는 경우는 예외로 막아주세요.
다음과 같이 방어 로직과 분기 기준을 보강해주세요:
@@ public void updateMemberList(UpdateClubMemberListCommand command) {
- Set<Long> updatedMemberInfoIds = updateClubMemberInfos.stream()
+ Set<Long> updatedMemberInfoIds = updateClubMemberInfos.stream()
.map(ClubMember::getId)
.collect(Collectors.toSet());
Set<Long> currentMemberIds = clubMembers.stream()
.map(ClubMember::getId)
.collect(Collectors.toSet());
+ // 1) 유효하지 않은(현재 동아리에 속하지 않는) ID 방어
+ Set<Long> unknownIds = updatedMemberInfoIds.stream()
+ .filter(id -> id != null && !currentMemberIds.contains(id))
+ .collect(Collectors.toSet());
+ if (!unknownIds.isEmpty()) {
+ throw new IllegalArgumentException("엑셀에 현재 동아리 소속이 아닌 ID가 포함되어 있습니다: " + unknownIds);
+ }
+
- clubMemberService.saveAll(filterCreatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds));
- clubMemberService.updateAll(filterUpdatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds));
+ // 2) 생성: ID == null 인 레코드만
+ clubMemberService.saveAll(filterCreatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds));
+ // 3) 수정: ID != null 이면서 현재 동아리에 존재하는 레코드만
+ clubMemberService.updateAll(filterUpdatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds));추가로, 아래 헬퍼들도 같이 보강해주세요(시그니처 변경 없이 내부 로직만 교체):
@@ private List<ClubMember> filterCreatedMembers(List<ClubMember> updatedClubMembers, Set<Long> updatedMemberIds,
- Set<Long> currentMemberIds) {
- Set<Long> createdMemberIds = new HashSet<>(updatedMemberIds);
- createdMemberIds.removeAll(currentMemberIds);
- return updatedClubMembers.stream()
- .filter(member -> createdMemberIds.contains(member.getId()))
- .toList();
+ Set<Long> currentMemberIds) {
+ // 생성은 반드시 ID == null 인 경우로 한정
+ return updatedClubMembers.stream()
+ .filter(member -> member.getId() == null)
+ .toList();
}📝 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.
| Set<Long> updatedMemberInfoIds = updateClubMemberInfos.stream() | |
| .map(ClubMember::getId) | |
| .collect(Collectors.toSet()); | |
| Set<Long> currentMemberIds = clubMembers.stream() | |
| .map(ClubMember::getId) | |
| .collect(Collectors.toSet()); | |
| clubMemberService.saveAll(filterCreatedMembers(updatedClubMembers, updatedMemberIds, currentMemberIds)); | |
| clubMemberService.deleteAll(filterDeletedMembers(clubMembers, updatedMemberIds, currentMemberIds)); | |
| clubMemberService.saveAll(filterCreatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds)); | |
| clubMemberService.updateAll(filterUpdatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds)); | |
| // --- in FacadeCentralClubMemberServiceImpl.java, inside updateMemberList(...) around lines 49–58 | |
| Set<Long> updatedMemberInfoIds = updateClubMemberInfos.stream() | |
| .map(ClubMember::getId) | |
| .collect(Collectors.toSet()); | |
| Set<Long> currentMemberIds = clubMembers.stream() | |
| .map(ClubMember::getId) | |
| .collect(Collectors.toSet()); | |
| // 1) 유효하지 않은(현재 동아리에 속하지 않는) ID 방어 | |
| Set<Long> unknownIds = updatedMemberInfoIds.stream() | |
| .filter(id -> id != null && !currentMemberIds.contains(id)) | |
| .collect(Collectors.toSet()); | |
| if (!unknownIds.isEmpty()) { | |
| throw new IllegalArgumentException( | |
| "엑셀에 현재 동아리 소속이 아닌 ID가 포함되어 있습니다: " + unknownIds | |
| ); | |
| } | |
| // 2) 생성: ID == null 인 레코드만 | |
| clubMemberService.saveAll( | |
| filterCreatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds) | |
| ); | |
| // 3) 수정: ID != null 이면서 현재 동아리에 존재하는 레코드만 | |
| clubMemberService.updateAll( | |
| filterUpdatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds) | |
| ); |
| Set<Long> updatedMemberInfoIds = updateClubMemberInfos.stream() | |
| .map(ClubMember::getId) | |
| .collect(Collectors.toSet()); | |
| Set<Long> currentMemberIds = clubMembers.stream() | |
| .map(ClubMember::getId) | |
| .collect(Collectors.toSet()); | |
| clubMemberService.saveAll(filterCreatedMembers(updatedClubMembers, updatedMemberIds, currentMemberIds)); | |
| clubMemberService.deleteAll(filterDeletedMembers(clubMembers, updatedMemberIds, currentMemberIds)); | |
| clubMemberService.saveAll(filterCreatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds)); | |
| clubMemberService.updateAll(filterUpdatedMembers(updateClubMemberInfos, updatedMemberInfoIds, currentMemberIds)); | |
| // --- in FacadeCentralClubMemberServiceImpl.java, replacing filterCreatedMembers(...) | |
| private List<ClubMember> filterCreatedMembers( | |
| List<ClubMember> updatedClubMembers, | |
| Set<Long> updatedMemberIds, | |
| Set<Long> currentMemberIds | |
| ) { | |
| // 생성은 반드시 ID == null 인 경우로 한정 | |
| return updatedClubMembers.stream() | |
| .filter(member -> member.getId() == null) | |
| .toList(); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceImpl.java
around lines 49-58, the current create/update branch uses set-difference of
incoming IDs vs current IDs which can cause data corruption if the Excel
contains IDs belonging to other clubs or non-existent IDs; change logic so that
"create" strictly requires member.getId() == null, and any incoming DTO/entity
with a non-null ID that is not contained in currentMemberIds must cause a
validation exception (or be rejected) rather than being treated as create;
likewise, "update" must only include entities whose IDs are present in
currentMemberIds; adapt the two helper methods (filterCreatedMembers,
filterUpdatedMembers) by replacing their internals (no signature change) to
implement these rules: created = entries with id == null, updated = entries with
id != null && currentMemberIds.contains(id); and throw or report an error if any
id != null and !currentMemberIds.contains(id).
| private List<ClubMember> filterUpdatedMembers(List<ClubMember> updatedClubMembers, Set<Long> updatedMemberIds, | ||
| Set<Long> currentMemberIds) { | ||
| Set<Long> willUpdateMemberIds = new HashSet<>(currentMemberIds); | ||
| willUpdateMemberIds.retainAll(updatedMemberIds); | ||
| return updatedClubMembers.stream() | ||
| .filter(member -> willUpdateMemberIds.contains(member.getId())) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
filterUpdatedMembers: null ID 제외 조건을 추가하고, 현재 동아리 소속 검증만으로 한정
수정 대상은 “ID != null” 이고 “현재 동아리에 존재”하는 교집합만 허용하도록 명시적으로 제한하세요.
- private List<ClubMember> filterUpdatedMembers(List<ClubMember> updatedClubMembers, Set<Long> updatedMemberIds,
- Set<Long> currentMemberIds) {
- Set<Long> willUpdateMemberIds = new HashSet<>(currentMemberIds);
- willUpdateMemberIds.retainAll(updatedMemberIds);
- return updatedClubMembers.stream()
- .filter(member -> willUpdateMemberIds.contains(member.getId()))
- .toList();
- }
+ private List<ClubMember> filterUpdatedMembers(List<ClubMember> updatedClubMembers, Set<Long> updatedMemberIds,
+ Set<Long> currentMemberIds) {
+ Set<Long> willUpdateMemberIds = new HashSet<>(currentMemberIds);
+ willUpdateMemberIds.retainAll(updatedMemberIds);
+ return updatedClubMembers.stream()
+ .filter(member -> member.getId() != null && willUpdateMemberIds.contains(member.getId()))
+ .toList();
+ }📝 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.
| private List<ClubMember> filterUpdatedMembers(List<ClubMember> updatedClubMembers, Set<Long> updatedMemberIds, | |
| Set<Long> currentMemberIds) { | |
| Set<Long> willUpdateMemberIds = new HashSet<>(currentMemberIds); | |
| willUpdateMemberIds.retainAll(updatedMemberIds); | |
| return updatedClubMembers.stream() | |
| .filter(member -> willUpdateMemberIds.contains(member.getId())) | |
| .toList(); | |
| } | |
| private List<ClubMember> filterUpdatedMembers(List<ClubMember> updatedClubMembers, Set<Long> updatedMemberIds, | |
| Set<Long> currentMemberIds) { | |
| Set<Long> willUpdateMemberIds = new HashSet<>(currentMemberIds); | |
| willUpdateMemberIds.retainAll(updatedMemberIds); | |
| return updatedClubMembers.stream() | |
| .filter(member -> member.getId() != null && willUpdateMemberIds.contains(member.getId())) | |
| .toList(); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceImpl.java
around lines 98-105, restrict filtered members to those with non-null IDs and
that belong to the current club: ensure willUpdateMemberIds is computed from
non-null IDs (or at least exclude nulls) and update the stream filter to first
check member.getId() != null and then
willUpdateMemberIds.contains(member.getId()) so only members with a non-null ID
and present in the currentMemberIds set are returned.
| @Override | ||
| @Transactional | ||
| public void updateAll(final List<ClubMember> updateClubMemberInfos) { | ||
| Map<Long, ClubMember> updatedMemberMap = updateClubMemberInfos.stream() | ||
| .collect(Collectors.toMap(ClubMember::getId, Function.identity())); | ||
|
|
||
| clubMemberRepository.findByIdIn(updatedMemberMap.keySet()) | ||
| .forEach(member -> | ||
| member.updateInformation(updatedMemberMap.get(member.getId()))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
updateAll의 입력 검증/경계 조건 보강 및 중복/NULL ID 처리
현재 구현은 다음 리스크가 있습니다:
- 빈 목록/NULL 입력 시 불필요한 쿼리 호출
nullID 포함 시toMapNPE- 중복 ID 포함 시
IllegalStateException - 커스텀
findByIdIn의존
적용 diff:
- public void updateAll(final List<ClubMember> updateClubMemberInfos) {
- Map<Long, ClubMember> updatedMemberMap = updateClubMemberInfos.stream()
- .collect(Collectors.toMap(ClubMember::getId, Function.identity()));
-
- clubMemberRepository.findByIdIn(updatedMemberMap.keySet())
- .forEach(member ->
- member.updateInformation(updatedMemberMap.get(member.getId())));
- }
+ public void updateAll(final List<ClubMember> updateClubMemberInfos) {
+ if (updateClubMemberInfos == null || updateClubMemberInfos.isEmpty()) {
+ return;
+ }
+ // null ID 제거 + 중복 ID는 마지막 값 우선
+ Map<Long, ClubMember> updatedMemberMap = updateClubMemberInfos.stream()
+ .filter(cm -> cm.getId() != null)
+ .collect(Collectors.toMap(
+ ClubMember::getId,
+ Function.identity(),
+ (prev, curr) -> curr
+ ));
+ if (updatedMemberMap.isEmpty()) {
+ return;
+ }
+ clubMemberRepository.findAllById(updatedMemberMap.keySet())
+ .forEach(member -> member.updateInformation(updatedMemberMap.get(member.getId())));
+ }또한 존재하지 않는 ID가 포함된 경우(레포지토리 결과 수 < 요청 수) 로깅/예외 중 하나를 선택해 일관된 계약을 유지하는 것도 고려해 주세요.
(cherry picked from commit 627f8d4)
1 participant
🚀 작업 내용
Summary by CodeRabbit