Feat/도메인 jpa 분리#15
Hidden character warning
Conversation
…Repository 테스트 추가
…Repository 테스트 추가
…blic_id에 대한 고유 제약 조건 추가
There was a problem hiding this comment.
Pull request overview
이 PR은 JPA 엔티티를 POJO 도메인 엔티티와 분리하여 도메인 계층의 순수성을 유지하고, PK를 Long에서 UUIDv7로 마이그레이션하는 작업입니다.
Changes:
- JPA 엔티티(
PushSubscriptionEntity,UpcomingScheduleNotificationEntity)와 도메인 엔티티(PushSubscription,UpcomingScheduleNotification)를 분리하고 Repository Adapter 패턴 도입 - UUIDv7 생성기 구현 및 새로운
public_id컬럼 추가로 PK를 Long에서 UUID로 전환 - 기존 레코드의
public_idnull 값을 채우기 위한 배치 애플리케이션 작성
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
UuidV7Generator.java |
시간순 정렬이 가능한 UUIDv7 생성 유틸리티 구현 |
IdV7GeneratorAdapter.java |
도메인 계층의 IdGenerator 인터페이스 구현체 |
PublicIdBackfillRunner.java |
기존 레코드의 public_id 필드를 백필하는 배치 작업 |
PushSubscriptionEntity.java, UpcomingScheduleNotificationEntity.java |
JPA 엔티티로 분리, @PrePersist에서 public_id 자동 생성 |
PushSubscription.java, UpcomingScheduleNotification.java |
POJO 도메인 모델로 변경, legacyId와 id (UUID) 필드 추가 |
PushSubscriptionRepositoryAdapter.java, UpcomingScheduleNotificationRepositoryAdapter.java |
도메인 객체와 JPA 엔티티 간 변환을 담당하는 어댑터 |
ScheduleNotificationService.java, FcmService.java |
새로운 엔티티 생성 시 IdGenerator를 통해 UUID 할당 |
| Test files | 어댑터와 JPA 리포지토리에 대한 테스트, 기존 테스트의 도메인 모델 생성 로직 업데이트 |
backfill-public-id.sh |
배치 작업 실행을 위한 셸 스크립트 |
| int updated = jdbcTemplate.update( | ||
| "update " + table + " set public_id = ? where id = ? and public_id is null", |
There was a problem hiding this comment.
(1) 문제점: SQL 인젝션 취약점이 있습니다. 테이블 이름이 직접 문자열 연결로 UPDATE 쿼리에 삽입되고 있습니다.
(2) 영향: 악의적인 테이블 이름이 전달될 경우 임의의 SQL이 실행될 수 있습니다.
(3) 수정 제안: 테이블 이름을 화이트리스트로 검증하거나, 허용된 테이블 목록과 비교하여 검증해야 합니다.
| for (Long id : ids) { | ||
| int updated = jdbcTemplate.update( | ||
| "update " + table + " set public_id = ? where id = ? and public_id is null", | ||
| idGenerator.generate().toString(), | ||
| id | ||
| ); | ||
| total += updated; |
There was a problem hiding this comment.
(1) 문제점: 배치 업데이트가 개별 UPDATE 문으로 실행되어 성능이 매우 비효율적입니다. 각 레코드마다 별도의 데이터베이스 왕복이 발생합니다.
(2) 영향: 대량의 레코드를 백필해야 할 경우 실행 시간이 매우 오래 걸리고 데이터베이스에 과도한 부하가 발생할 수 있습니다.
(3) 수정 제안: jdbcTemplate.batchUpdate()를 사용하여 배치로 업데이트를 수행하거나, CASE 문을 사용한 단일 UPDATE 쿼리로 여러 레코드를 한번에 업데이트해야 합니다.
| for (Long id : ids) { | |
| int updated = jdbcTemplate.update( | |
| "update " + table + " set public_id = ? where id = ? and public_id is null", | |
| idGenerator.generate().toString(), | |
| id | |
| ); | |
| total += updated; | |
| int[] updatedCounts = jdbcTemplate.batchUpdate( | |
| "update " + table + " set public_id = ? where id = ? and public_id is null", | |
| ids, | |
| batchSize, | |
| (ps, id) -> { | |
| ps.setString(1, idGenerator.generate().toString()); | |
| ps.setLong(2, id); | |
| } | |
| ); | |
| for (int count : updatedCounts) { | |
| total += count; |
| List<UpcomingScheduleNotificationEntity> entities = notifications.stream() | ||
| .map(this::toEntity) | ||
| .toList(); | ||
| jpaRepository.deleteAllInBatch(entities); |
There was a problem hiding this comment.
(1) 문제점: deleteAllInBatch에서 toEntity 변환이 불완전합니다. legacyId가 null일 수 있는 도메인 객체를 엔티티로 변환하면 JPA가 새로운 레코드로 인식하여 삭제가 실패할 수 있습니다.
(2) 영향: deleteAllInBatch 호출 시 실제로 레코드가 삭제되지 않거나 예상치 못한 동작이 발생할 수 있습니다.
(3) 수정 제안: deleteAllInBatch는 도메인 객체를 받는 대신 ID 목록을 받도록 변경하거나, JPA의 delete 메서드 대신 직접 쿼리를 사용하여 scheduleId와 ownerId 조합으로 삭제해야 합니다.
| List<UpcomingScheduleNotificationEntity> entities = notifications.stream() | |
| .map(this::toEntity) | |
| .toList(); | |
| jpaRepository.deleteAllInBatch(entities); | |
| notifications.forEach(notification -> | |
| jpaRepository.deleteByScheduleIdAndOwnerId( | |
| notification.getScheduleId(), | |
| notification.getOwnerId() | |
| ) | |
| ); |
| @Test | ||
| void savesAndLoadsDomainWithPublicId() { | ||
| UUID publicId = UUID.randomUUID(); | ||
| UpcomingScheduleNotification created = new UpcomingScheduleNotification( | ||
| publicId, | ||
| 1L, | ||
| 2L, | ||
| "title", | ||
| "2025-01-01T00:00:00Z", | ||
| "key-1" | ||
| ); | ||
|
|
||
| UpcomingScheduleNotification saved = repository.save(created); | ||
|
|
||
| assertThat(saved.getLegacyId()).isNotNull(); | ||
| assertThat(saved.getId()).isEqualTo(publicId); | ||
|
|
||
| Optional<UpcomingScheduleNotification> loaded = | ||
| repository.findByScheduleIdAndOwnerId(2L, 1L); | ||
|
|
||
| assertThat(loaded).isPresent(); | ||
| assertThat(loaded.get().getId()).isEqualTo(publicId); | ||
| } |
There was a problem hiding this comment.
(1) 문제점: deleteAllInBatch 메서드에 대한 테스트 커버리지가 부족합니다. 새로 추가된 리포지토리 어댑터의 deleteAllInBatch 메서드가 실제로 올바르게 동작하는지 검증하는 테스트가 없습니다.
(2) 영향: deleteAllInBatch의 toEntity 변환 로직에 문제가 있을 수 있지만 이를 감지할 수 없습니다.
(3) 수정 제안: UpcomingScheduleNotificationRepositoryAdapterTest에 deleteAllInBatch 메서드를 테스트하는 케이스를 추가해야 합니다.
| @Test | ||
| void savesAndLoadsDomainWithPublicId() { | ||
| UUID publicId = UUID.randomUUID(); | ||
| PushSubscription created = new PushSubscription( | ||
| publicId, | ||
| 101L, | ||
| "device-1", | ||
| "token-1" | ||
| ); | ||
|
|
||
| PushSubscription saved = repository.save(created); | ||
|
|
||
| assertThat(saved.getLegacyId()).isNotNull(); | ||
| assertThat(saved.getId()).isEqualTo(publicId); | ||
|
|
||
| Optional<PushSubscription> loaded = | ||
| repository.findByMemberIdAndDeviceId(101L, "device-1"); | ||
|
|
||
| assertThat(loaded).isPresent(); | ||
| assertThat(loaded.get().getId()).isEqualTo(publicId); | ||
| } |
There was a problem hiding this comment.
(1) 문제점: PushSubscriptionRepositoryAdapterTest에 deleteByToken과 deleteByMemberIdAndDeviceId 메서드에 대한 테스트 커버리지가 없습니다.
(2) 영향: 어댑터 계층의 삭제 기능이 올바르게 동작하는지 검증할 수 없습니다.
(3) 수정 제안: 삭제 메서드들에 대한 테스트 케이스를 추가해야 합니다.
| public static UUID generate() { | ||
| byte[] bytes = new byte[16]; | ||
| long millis = System.currentTimeMillis(); | ||
|
|
||
| bytes[0] = (byte) (millis >>> 40); | ||
| bytes[1] = (byte) (millis >>> 32); | ||
| bytes[2] = (byte) (millis >>> 24); | ||
| bytes[3] = (byte) (millis >>> 16); | ||
| bytes[4] = (byte) (millis >>> 8); | ||
| bytes[5] = (byte) millis; | ||
|
|
||
| int randA = ThreadLocalRandom.current().nextInt(1 << 12); | ||
| bytes[6] = (byte) (0x70 | ((randA >>> 8) & 0x0F)); | ||
| bytes[7] = (byte) randA; | ||
|
|
||
| long randB = ThreadLocalRandom.current().nextLong(); | ||
| bytes[8] = (byte) (randB >>> 56); | ||
| bytes[9] = (byte) (randB >>> 48); | ||
| bytes[10] = (byte) (randB >>> 40); | ||
| bytes[11] = (byte) (randB >>> 32); | ||
| bytes[12] = (byte) (randB >>> 24); | ||
| bytes[13] = (byte) (randB >>> 16); | ||
| bytes[14] = (byte) (randB >>> 8); | ||
| bytes[15] = (byte) randB; | ||
|
|
||
| bytes[8] = (byte) ((bytes[8] & 0x3F) | 0x80); | ||
|
|
||
| long msb = 0; | ||
| long lsb = 0; | ||
| for (int i = 0; i < 8; i++) { | ||
| msb = (msb << 8) | (bytes[i] & 0xFF); | ||
| } | ||
| for (int i = 8; i < 16; i++) { | ||
| lsb = (lsb << 8) | (bytes[i] & 0xFF); | ||
| } | ||
| return new UUID(msb, lsb); | ||
| } | ||
| } |
There was a problem hiding this comment.
(1) 문제점: UuidV7Generator의 구현이 테스트되지 않았습니다. UUID v7 스펙을 정확히 구현했는지, 시간 기반 정렬이 보장되는지 등을 검증하는 테스트가 없습니다.
(2) 영향: UUID 생성 로직에 버그가 있어도 감지할 수 없으며, 향후 정렬 순서나 충돌 문제가 발생할 수 있습니다.
(3) 수정 제안: UuidV7GeneratorTest를 추가하여 생성된 UUID의 형식, 시간순 정렬, 고유성 등을 검증해야 합니다.
| public class PublicIdBackfillRunner implements CommandLineRunner { | ||
| private static final Logger log = LoggerFactory.getLogger(PublicIdBackfillRunner.class); | ||
|
|
||
| private final JdbcTemplate jdbcTemplate; | ||
| private final IdGenerator idGenerator; | ||
| private final int batchSize; | ||
|
|
||
| public PublicIdBackfillRunner(JdbcTemplate jdbcTemplate, | ||
| IdGenerator idGenerator, | ||
| @Value("${backfill.public-id.batch-size:500}") int batchSize) { | ||
| this.jdbcTemplate = jdbcTemplate; | ||
| this.idGenerator = idGenerator; | ||
| this.batchSize = batchSize; | ||
| } | ||
|
|
||
| @Override | ||
| public void run(String... args) { | ||
| backfillTable("upcoming_schedule_notification"); | ||
| backfillTable("push_subscription"); | ||
| } | ||
|
|
||
| private void backfillTable(String table) { | ||
| int total = 0; | ||
| while (true) { | ||
| List<Long> ids = jdbcTemplate.query( | ||
| "select id from " + table + " where public_id is null limit ?", | ||
| ps -> ps.setInt(1, batchSize), | ||
| (rs, rowNum) -> rs.getLong(1) | ||
| ); | ||
|
|
||
| if (ids.isEmpty()) { | ||
| break; | ||
| } | ||
|
|
||
| for (Long id : ids) { | ||
| int updated = jdbcTemplate.update( | ||
| "update " + table + " set public_id = ? where id = ? and public_id is null", | ||
| idGenerator.generate().toString(), | ||
| id | ||
| ); | ||
| total += updated; | ||
| } | ||
| } | ||
|
|
||
| log.info("Backfill completed for table={}, updatedRows={}", table, total); | ||
| } | ||
| } |
There was a problem hiding this comment.
(1) 문제점: PublicIdBackfillRunner에 대한 테스트가 없습니다. 배치 작업의 정확성, 멱등성, 에러 처리 등을 검증하는 테스트가 필요합니다.
(2) 영향: 백필 로직에 버그가 있어도 프로덕션 환경에서만 발견될 수 있으며, 데이터 정합성 문제를 초래할 수 있습니다.
(3) 수정 제안: PublicIdBackfillRunnerTest를 추가하여 배치 처리 로직, 빈 테이블 처리, 부분 업데이트 등의 시나리오를 테스트해야 합니다.
| List<Long> ids = jdbcTemplate.query( | ||
| "select id from " + table + " where public_id is null limit ?", |
There was a problem hiding this comment.
(1) 문제점: SQL 인젝션 취약점이 있습니다. 테이블 이름이 직접 문자열 연결로 SQL 쿼리에 삽입되고 있습니다.
(2) 영향: 악의적인 테이블 이름이 전달될 경우 임의의 SQL이 실행될 수 있습니다.
(3) 수정 제안: 테이블 이름을 화이트리스트로 검증하거나, PreparedStatement의 테이블 이름 파라미터화를 사용할 수 없으므로 허용된 테이블 목록과 비교하여 검증해야 합니다.
변경된 점