feat: add pricing catalog API handling#5
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a pricing catalog and pricing plan management system, including controllers, services, repositories, entities, and DTOs. It also updates the Dockerfile base image to Java 25 and configures security rules to permit public access to pricing catalogs. Key feedback points include reverting the Dockerfile base image to Java 21 due to Java 25's lack of stability, ensuring consistent UTC timezone usage across entity lifecycle hooks, refining the active pricing plan query logic to handle date ranges correctly, validating the chronological order of effective dates, and handling potential null values safely when sorting catalogs by publication date.
| @@ -1,8 +1,8 @@ | |||
| FROM eclipse-temurin:21-jre | |||
| FROM eclipse-temurin:25-jre | |||
| LocalDateTime now = LocalDateTime.now(); | ||
|
|
||
| if (createdAt == null) { | ||
| createdAt = now; | ||
| } | ||
|
|
||
| if (updatedAt == null) { | ||
| updatedAt = now; | ||
| } |
There was a problem hiding this comment.
현재 LocalDateTime.now()를 사용하여 createdAt과 updatedAt을 설정하고 있습니다. LocalDateTime.now()는 시스템의 기본 시간대(System Default Timezone)를 사용하므로, 서버 환경에 따라 시간대가 달라질 수 있습니다. 반면, PricingPlanService 등에서는 UTC 기준으로 시간을 처리하고 있으며, PricingCatalogController에서도 ZoneOffset.UTC를 기준으로 HTTP 헤더를 생성하고 있습니다. 일관성을 유지하고 시간대 관련 버그를 방지하기 위해, 모든 LocalDateTime.now() 호출 시 ZoneOffset.UTC를 명시하여 UTC 기준으로 저장하도록 수정하는 것이 안전합니다.
| LocalDateTime now = LocalDateTime.now(); | |
| if (createdAt == null) { | |
| createdAt = now; | |
| } | |
| if (updatedAt == null) { | |
| updatedAt = now; | |
| } | |
| LocalDateTime now = java.time.LocalDateTime.now(java.time.ZoneOffset.UTC); | |
| if (createdAt == null) { | |
| createdAt = now; | |
| } | |
| if (updatedAt == null) { | |
| updatedAt = now; | |
| } |
|
|
||
| @PreUpdate | ||
| void preUpdate() { | ||
| updatedAt = LocalDateTime.now(); |
| if (createdAt == null) { | ||
| createdAt = LocalDateTime.now(); | ||
| } |
There was a problem hiding this comment.
| @Query(""" | ||
| select p | ||
| from PricingPlan p | ||
| where (:provider is null or p.provider = :provider) | ||
| and (:model is null or p.model = :model) | ||
| and (:activeOnly = false or p.effectiveTo is null) | ||
| order by p.provider asc, p.model asc, p.effectiveFrom desc | ||
| """) | ||
| List<PricingPlan> findPricingPlans( | ||
| String provider, | ||
| String model, | ||
| boolean activeOnly | ||
| ); |
There was a problem hiding this comment.
activeOnly가 true일 때 p.effectiveTo is null 조건만 검사하고 있습니다. 하지만 실제 서비스에서 '활성(Active)' 상태는 시작일(effectiveFrom)이 현재 시간 이전이고, 종료일(effectiveTo)이 없거나 현재 시간 이후여야 합니다. 현재 구현으로는 시작일이 미래인 예약된 플랜이 조회되거나, 종료일이 미래로 설정되어 현재 활성 상태인 플랜이 제외될 수 있습니다. 따라서 쿼리에 현재 시간(now)을 파라미터로 전달받아 정확한 활성 여부를 판단하도록 수정하는 것이 안전합니다.
@Query("""
select p
from PricingPlan p
where (:provider is null or p.provider = :provider)
and (:model is null or p.model = :model)
and (:activeOnly = false or (p.effectiveFrom <= :now and (p.effectiveTo is null or p.effectiveTo >= :now)))
order by p.provider asc, p.model asc, p.effectiveFrom desc
""")
List<PricingPlan> findPricingPlans(
String provider,
String model,
boolean activeOnly,
java.time.LocalDateTime now
);| public PricingPlanListResponse getPricingPlans(String provider, String model, Boolean activeOnly) { | ||
| List<PricingPlanItemResponse> items = | ||
| pricingPlanRepository.findPricingPlans( | ||
| blankToNull(provider), | ||
| blankToNull(model), | ||
| Boolean.TRUE.equals(activeOnly) | ||
| ) |
There was a problem hiding this comment.
PricingPlanRepository.findPricingPlans 메서드가 현재 시간(now)을 파라미터로 받도록 수정됨에 따라, 서비스 레이어에서도 UTC 기준의 현재 시간을 전달하도록 수정해야 합니다.
| public PricingPlanListResponse getPricingPlans(String provider, String model, Boolean activeOnly) { | |
| List<PricingPlanItemResponse> items = | |
| pricingPlanRepository.findPricingPlans( | |
| blankToNull(provider), | |
| blankToNull(model), | |
| Boolean.TRUE.equals(activeOnly) | |
| ) | |
| public PricingPlanListResponse getPricingPlans(String provider, String model, Boolean activeOnly) { | |
| List<PricingPlanItemResponse> items = | |
| pricingPlanRepository.findPricingPlans( | |
| blankToNull(provider), | |
| blankToNull(model), | |
| Boolean.TRUE.equals(activeOnly), | |
| java.time.LocalDateTime.now(java.time.ZoneOffset.UTC) | |
| ) |
| public PricingPlanCreateResponse create(PricingPlanCreateRequest request) { | ||
| PricingPlan plan = PricingPlan.builder() |
There was a problem hiding this comment.
가격표 등록 시 시작일(effectiveFrom)과 종료일(effectiveTo)의 선후 관계에 대한 검증 로직이 없습니다. 종료일이 시작일보다 앞서는 경우 잘못된 데이터가 저장될 수 있으므로, effectiveTo가 effectiveFrom보다 이후인지 검증하는 로직을 추가하는 것이 안전합니다.
| public PricingPlanCreateResponse create(PricingPlanCreateRequest request) { | |
| PricingPlan plan = PricingPlan.builder() | |
| @Transactional | |
| public PricingPlanCreateResponse create(PricingPlanCreateRequest request) { | |
| if (request.effectiveTo() != null && request.effectiveTo().isBefore(request.effectiveFrom())) { | |
| throw new IllegalArgumentException("종료일(effectiveTo)은 시작일(effectiveFrom)보다 이후여야 합니다."); | |
| } | |
| PricingPlan plan = PricingPlan.builder() |
| @Query(""" | ||
| select c | ||
| from PricingCatalog c | ||
| where c.catalogKey = :catalogKey | ||
| and c.isActive = true | ||
| order by c.publishedAt desc | ||
| """) | ||
| List<PricingCatalog> findLatestActiveCatalog( | ||
| String catalogKey, | ||
| Pageable pageable | ||
| ); |
There was a problem hiding this comment.
publishedAt 컬럼은 null을 허용합니다. 데이터베이스 종류(MySQL, PostgreSQL, H2 등)에 따라 ORDER BY ... DESC 정렬 시 null 값의 위치(처음 또는 마지막)가 다르게 처리될 수 있습니다. 이로 인해 로컬 테스트 환경(H2)과 운영 환경(PostgreSQL/MySQL)에서 '최신 활성 카탈로그'를 조회하는 결과가 달라지는 잠재적 버그가 발생할 수 있습니다. coalesce(c.publishedAt, c.createdAt)를 사용하여 null 안전하게 정렬하도록 수정하는 것을 권장합니다.
| @Query(""" | |
| select c | |
| from PricingCatalog c | |
| where c.catalogKey = :catalogKey | |
| and c.isActive = true | |
| order by c.publishedAt desc | |
| """) | |
| List<PricingCatalog> findLatestActiveCatalog( | |
| String catalogKey, | |
| Pageable pageable | |
| ); | |
| @Query(""" | |
| select c | |
| from PricingCatalog c | |
| where c.catalogKey = :catalogKey | |
| and c.isActive = true | |
| order by coalesce(c.publishedAt, c.createdAt) desc | |
| """) | |
| List<PricingCatalog> findLatestActiveCatalog( | |
| String catalogKey, | |
| Pageable pageable | |
| ); |
가격표 API
확인해주실 부분
DB에서는
is_active기본값이0입니다.그런데 엔티티
PricingCatalog의@PrePersist에서isActive를true로 넣고 있으면, JPA로 저장할 때 DB 기본값0은 사실상 안 쓰입니다.의도적으로 새 카탈로그를 기본 활성화하려는 거면 괜찮습니다. 반대로 “생성은 비활성, 배포할 때만 활성” 정책이면 이 부분은
false로 바꾸는 게 DB 설계와 맞습니다.또
published_at이 DB에서null허용이라, 최신 활성 카탈로그로 쓰는 데이터는 가능하면published_at을 꼭 넣어야 합니다. 컨트롤러에서는publishedAt == null일 때updatedAt이나createdAt으로 fallback 처리해두는 게 안전합니다.라고 AI가 하더라고요...생성이 아니라 등록만 있는 API인 것 같아서 일단 첫번째 방향으로 했습니다.