Conversation
Summary of ChangesHello @wlgusqkr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 시스템의 유지보수성과 테스트 안정성을 향상시키는 데 중점을 둡니다. 주요 변경 사항으로는 오래된 요약 데이터를 자동으로 정리하는 스케줄러를 도입하여 데이터베이스 부하를 줄이고, 테스트 실행 간 데이터 독립성을 보장하는 데이터베이스 격리 메커니즘을 구현하여 테스트 신뢰도를 높였습니다. 또한, 엔티티 객체 생성 방식을 표준화하고 불필요한 코드를 제거하여 코드의 가독성과 유지보수성을 개선했습니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Test Results13 tests 13 ✅ 1s ⏱️ Results for commit 799d176. ♻️ This comment has been updated with latest results. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 PR은 오래된 요약 데이터를 정리하는 스케줄러를 추가하는 내용으로, 데이터 관리에 있어 좋은 기능 추가입니다. 스케줄러 구현과 함께 리포지토리 변경, 테스트 코드까지 포함되어 있습니다. 특히 데이터베이스 격리를 위한 테스트 설정은 매우 훌륭한 추가 사항입니다.
다만, 검토 과정에서 컴파일 또는 런타임 오류를 유발할 수 있는 몇 가지 치명적인 문제들을 발견했습니다. 주로 중복된 Lombok 애노테이션과 엔티티-DB 스키마 불일치에 관한 것입니다. 또한 테스트 용이성과 코드 명확성을 개선하기 위한 몇 가지 제안 사항도 있습니다. 아래의 리뷰 코멘트를 확인해 주세요.
| @Builder | ||
| private Subscription(Url url, boolean isAlarmEnabled, String alias, User user, String lastSeenPostId) { | ||
| this.url = url; | ||
| this.isAlarmEnabled = isAlarmEnabled; | ||
| this.alias = alias; | ||
| this.user = user; | ||
| this.lastSeenPostId = lastSeenPostId; | ||
| } |
There was a problem hiding this comment.
@Builder 애노테이션이 클래스 레벨(30라인)과 생성자 레벨(104라인)에 중복으로 사용되었습니다. Lombok은 한 클래스에 여러 개의 @Builder를 허용하지 않으므로 컴파일 오류가 발생합니다.
새로 추가된 create 정적 팩토리 메서드와 생성자 빌더 패턴을 사용하시려면, 클래스 레벨의 @Builder와 @AllArgsConstructor를 제거해야 합니다.
예시:
@Entity
@Getter
//@Builder // <- 제거
@Table(name = "subscriptions")
@NoArgsConstructor(access = AccessLevel.PROTECTED)
//@AllArgsConstructor(access = AccessLevel.PRIVATE) // <- 제거
public class Subscription extends BaseEntity {
//...
}이렇게 수정하면 새로 추가한 빌더만 사용되어 문제가 해결됩니다.
| @Builder | ||
| private User(String userId, String hashedSecret, String secretFingerprint, UserType userType, boolean isActive, | ||
| String plainSecret) { | ||
| this.userId = userId; | ||
| this.hashedSecret = hashedSecret; | ||
| this.secretFingerprint = secretFingerprint; | ||
| this.userType = userType; | ||
| this.isActive = isActive; | ||
| this.plainSecret = plainSecret; | ||
| } |
There was a problem hiding this comment.
Subscription 엔티티와 동일한 문제가 있습니다. @Builder 애노테이션이 클래스 레벨(26라인)과 생성자 레벨(165라인)에 중복으로 선언되어 컴파일 오류를 유발합니다.
의도하신 대로 create 정적 팩토리 메서드를 사용하려면 클래스 레벨의 @Builder와 @AllArgsConstructor를 제거해 주세요.
예시:
@Entity
@Getter
@Setter
//@Builder // <- 제거
@Table(name = "users")
@NoArgsConstructor(access = AccessLevel.PROTECTED)
//@AllArgsConstructor(access = AccessLevel.PRIVATE) // <- 제거
public class User extends BaseEntity {
//...
}| created_at datetime(6) not null, | ||
| hash_tag varchar(255) not null, | ||
| is_read bit d not null, | ||
| is_read bit not null, |
There was a problem hiding this comment.
summaries 테이블의 컬럼명이 is_read로 정의되어 있지만, Summary 엔티티에서는 is_keyword_matched로 매핑되어 있습니다. (@Column(name = "is_keyword_matched"))
이 불일치로 인해 애플리케이션 실행 시 Summary 엔티티를 조회하거나 저장할 때 오류가 발생합니다. 엔티티의 필드명 또는 DB 컬럼명을 통일해야 합니다. 예를 들어, Summary 엔티티를 다음과 같이 수정할 수 있습니다.
Summary.java:
// ...
@Column(name = "is_read", nullable = false)
private boolean isRead;
// ...또는 DB 스키마의 컬럼명을 is_keyword_matched로 변경해야 합니다.
| @Scheduled(cron = "0 0 3 * * *") // 매일 새벽 3시에 실행 | ||
| public void deleteOldSummaries() { | ||
|
|
||
| LocalDateTime threshold = LocalDateTime.now().minusDays(7); |
There was a problem hiding this comment.
LocalDateTime.now()를 직접 사용하면 서버의 기본 시간대에 의존하게 되어 예기치 않은 동작을 유발할 수 있고, 시간과 관련된 로직을 테스트하기 어렵습니다.
java.time.Clock을 빈으로 등록하고 주입받아 사용하면 시간대 문제를 해결하고 테스트 용이성을 높일 수 있습니다.
제안:
Clock빈을 설정 파일에 등록합니다.
@Configuration
public class AppConfig {
@Bean
public Clock clock() {
return Clock.systemDefaultZone(); // 또는 Clock.systemUTC()
}
}- 스케줄러에서
Clock을 주입받아 사용합니다.
@Component
@RequiredArgsConstructor
public class SummaryCleanupScheduler {
private final SummaryRepository summaryRepository;
private final Clock clock; // Clock 주입
@Transactional
@Scheduled(cron = "0 0 3 * * *")
public void deleteOldSummaries() {
LocalDateTime threshold = LocalDateTime.now(clock).minusDays(7); // 주입받은 clock 사용
summaryRepository.deleteByCreatedAtBefore(threshold);
}
}테스트에서는 Clock.fixed(...)를 사용하여 시간을 고정할 수 있어 보다 안정적인 테스트가 가능해집니다.
| private SummaryCleanupScheduler summaryCleanupScheduler; | ||
|
|
||
| @Test | ||
| void _7일_지난로직은_삭제한다() { |
There was a problem hiding this comment.
테스트 메서드명 _7일_지난로직은_삭제한다는 어떤 동작을 테스트하는지 명확하게 설명하지 못하는 것 같습니다. "로직"을 삭제하는 것이 아니라 "데이터"를 삭제하는 것이므로, 테스트의 의도를 더 잘 나타내는 이름으로 변경하는 것이 좋겠습니다.
예를 들어, 다음과 같은 이름을 제안합니다.
오래된_요약_데이터를_삭제한다7일_이상된_요약_데이터는_삭제된다
이렇게 하면 테스트의 목적을 한눈에 파악하기 쉬워집니다.
| void _7일_지난로직은_삭제한다() { | |
| void _7일_이상된_요약_데이터는_삭제된다() { |
| return entityManager.createNativeQuery("SHOW TABLES") | ||
| .getResultList() | ||
| .stream() | ||
| .filter(name -> !name.equals("flyway_schema_history")) | ||
| .toList(); |
There was a problem hiding this comment.
createNativeQuery(...).getResultList()는 제네릭을 사용하지 않아 List<Object>를 반환합니다. 현재 코드에서는 @SuppressWarnings("unchecked")를 사용하여 경고를 무시하고 있지만, 이는 타입 안정성을 해칠 수 있습니다.
stream()을 사용하기 전에 각 원소를 String으로 안전하게 변환하는 것이 좋습니다. map(String::valueOf)를 추가하여 이를 해결할 수 있습니다.
return entityManager.createNativeQuery("SHOW TABLES")
.getResultList()
.stream()
.map(String::valueOf)
.filter(name -> !name.equals("flyway_schema_history"))
.toList();
|
/gemini review |
| LocalDateTime threshold = LocalDateTime.now().minusDays(7); | ||
| summaryRepository.deleteByCreatedAtBefore(threshold); |
There was a problem hiding this comment.
deleteByCreatedAtBefore 메서드는 삭제할 데이터가 많을 경우, 단일 DELETE 쿼리로 인해 데이터베이스에 상당한 부하를 줄 수 있습니다. 이는 장시간의 테이블 락을 유발하여 다른 작업에 영향을 미칠 수 있습니다.
성능 및 안정성을 위해 대량 삭제는 배치(batch) 방식으로 처리하는 것을 고려해 보세요. 예를 들어, 삭제할 데이터를 일정 개수씩 끊어서 여러 번에 걸쳐 삭제하는 방식입니다.
이를 위해 Repository에 LIMIT을 사용하는 메서드를 추가할 수 있습니다.
// SummaryRepository.java
@Modifying
@Query("DELETE FROM Summary s WHERE s.createdAt < :threshold")
int deleteBatchByCreatedAtBefore(@Param("threshold") LocalDateTime threshold, Pageable pageable);
// SummaryCleanupScheduler.java
public void deleteOldSummaries() {
LocalDateTime threshold = LocalDateTime.now().minusDays(7);
int deletedCount;
do {
// 예를 들어 1000개씩 삭제
deletedCount = summaryRepository.deleteBatchByCreatedAtBefore(threshold, PageRequest.of(0, 1000));
} while (deletedCount > 0);
}위 예시는 JPA의 Pageable을 활용한 배치 삭제 방법입니다. 실제 구현은 프로젝트 상황에 맞게 조정이 필요할 수 있습니다.
| ReflectionTestUtils.setField(oldSummary, "createdAt", LocalDateTime.now().minusDays(8)); | ||
| ReflectionTestUtils.setField(newSummary, "createdAt", LocalDateTime.now().minusDays(6)); |
There was a problem hiding this comment.
ReflectionTestUtils를 사용하여 private 필드인 createdAt의 값을 직접 수정하는 방식은 캡슐화를 위반하며, 향후 엔티티의 내부 구조가 변경될 때 테스트 코드가 깨지기 쉽게 만듭니다.
더 견고한 테스트를 위해 다음과 같은 방법을 고려해볼 수 있습니다.
-
테스트용 팩토리 메서드 추가:
@VisibleForTesting(Guava) 어노테이션 등을 사용하여 테스트에서만 사용됨을 명시하고,createdAt을 파라미터로 받는 팩토리 메서드를Summary엔티티에 추가합니다.// Summary.java @VisibleForTesting public static Summary createForTest(..., LocalDateTime createdAt) { Summary summary = Summary.create(...); summary.createdAt = createdAt; return summary; }
-
Clock추상화:java.time.Clock을 사용하여 시간을 생성하고, 테스트 시에는Clock의 구현을 mock으로 대체하여 시간을 제어하는 방법입니다. 이 방식은 시스템 전반의 시간 관련 테스트를 용이하게 합니다.
이러한 방법을 통해 리플렉션 없이도 엔티티의 상태를 제어하며 더 안정적인 테스트를 작성할 수 있습니다.
| // @Test | ||
| // void 최근_알람을_조회한다() { | ||
| // // given | ||
| // Url url = Url.create("http://example.com", "Example Site"); | ||
| // urlRepository.save(url); | ||
| // | ||
| // User user = User.create("userId1", "hashedSecret", "fingerPrint", UserType.USER, true, "plainSecret"); | ||
| // userRepository.save(user); | ||
| // | ||
| // Subscription subscription = Subscription.create(url, true, "alias", user, "lastSeenPostId"); | ||
| // subscriptionRepository.save(subscription); | ||
| // | ||
| // Summary summary1 = Summary.create("hash1", "title1", "content1", "postUrl1", "postDate1", false, subscription); | ||
| // Summary summary2 = Summary.create("hash2", "title2", "content2", "postUrl2", "postDate2", false, subscription); | ||
| // Summary summary3 = Summary.create("hash3", "title3", "content3", "postUrl3", "postDate3", false, subscription); | ||
| // Summary summary4 = Summary.create("hash4", "title4", "content4", "postUrl4", "postDate4", false, subscription); | ||
| // ReflectionTestUtils.setField(summary4, "updatedAt", LocalDateTime.now().minusDays(1)); | ||
| // ReflectionTestUtils.setField(summary3, "updatedAt", LocalDateTime.now().minusDays(2)); | ||
| // ReflectionTestUtils.setField(summary2, "updatedAt", LocalDateTime.now().minusDays(3)); | ||
| // ReflectionTestUtils.setField(summary1, "updatedAt", LocalDateTime.now().minusDays(4)); | ||
| // summaryRepository.saveAll(List.of(summary1, summary2, summary3, summary4)); | ||
| // | ||
| // // when | ||
| // List<RecentAlarmResponse> result = alarmQueryService.getRecentAlarms(new PageRequest(0, 5), user.getUserId(), user.getHashedSecret()); | ||
| // | ||
| // // then | ||
| // assertThat(result.get(0).alias()).isEqualTo("title4"); | ||
| // assertThat(result.get(1).alias()).isEqualTo("title3"); | ||
| // assertThat(result.get(2).alias()).isEqualTo("title2"); | ||
| // assertThat(result.get(3).alias()).isEqualTo("title1"); | ||
| // } |
There was a problem hiding this comment.
최근_알람을_조회한다 테스트 메서드 전체가 주석 처리되어 있습니다.
- 개발 중인 기능인 경우: 테스트를 완성하고 주석을 제거하여 기능의 정확성을 검증하는 것이 좋습니다.
- 더 이상 필요 없는 코드인 경우: 코드베이스를 깔끔하게 유지하고 다른 개발자들의 혼동을 막기 위해 해당 코드를 삭제하는 것을 권장합니다.
주석 처리된 코드는 레거시로 남을 경우 코드의 의도를 파악하기 어렵게 만들 수 있습니다.
1 participant
Related issue 🛠
어떤 변경사항이 있었나요?
Work Description ✏️
작업 내용을 작성해주세요.
Uncompleted Tasks 😅
To Reviewers 📢
리뷰어가 알면 좋은 내용을 작성해주세요.