Skip to content

feat: add pricing catalog API handling#5

Open
Leejaewang03 wants to merge 2 commits into
mainfrom
feature/pricing-catalog-api
Open

feat: add pricing catalog API handling#5
Leejaewang03 wants to merge 2 commits into
mainfrom
feature/pricing-catalog-api

Conversation

@Leejaewang03
Copy link
Copy Markdown
Contributor

가격표 API

확인해주실 부분

DB에서는 is_active 기본값이 0입니다.

is_active tinyint(1) not null default 0

그런데 엔티티 PricingCatalog@PrePersist에서 isActivetrue로 넣고 있으면, JPA로 저장할 때 DB 기본값 0은 사실상 안 쓰입니다.

if (isActive == null) {
	isActive = true;
}

의도적으로 새 카탈로그를 기본 활성화하려는 거면 괜찮습니다. 반대로 “생성은 비활성, 배포할 때만 활성” 정책이면 이 부분은 false로 바꾸는 게 DB 설계와 맞습니다.

if (isActive == null) {
	isActive = false;
}

published_at이 DB에서 null 허용이라, 최신 활성 카탈로그로 쓰는 데이터는 가능하면 published_at을 꼭 넣어야 합니다. 컨트롤러에서는 publishedAt == null일 때 updatedAt이나 createdAt으로 fallback 처리해두는 게 안전합니다.

라고 AI가 하더라고요...생성이 아니라 등록만 있는 API인 것 같아서 일단 첫번째 방향으로 했습니다.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Dockerfile
@@ -1,8 +1,8 @@
FROM eclipse-temurin:21-jre
FROM eclipse-temurin:25-jre
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

현재 Base 이미지로 eclipse-temurin:25-jre를 사용하도록 변경되었습니다. Java 25는 아직 정식 출시(LTS)되지 않은 버전이므로, Docker Hub에 해당 태그의 이미지가 존재하지 않거나 불안정할 수 있어 빌드 및 배포가 실패할 것입니다. 기존에 사용하던 안정적인 LTS 버전인 eclipse-temurin:21-jre를 계속 사용하는 것을 권장합니다.

FROM eclipse-temurin:21-jre

Comment on lines +64 to +72
LocalDateTime now = LocalDateTime.now();

if (createdAt == null) {
createdAt = now;
}

if (updatedAt == null) {
updatedAt = now;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

현재 LocalDateTime.now()를 사용하여 createdAtupdatedAt을 설정하고 있습니다. LocalDateTime.now()는 시스템의 기본 시간대(System Default Timezone)를 사용하므로, 서버 환경에 따라 시간대가 달라질 수 있습니다. 반면, PricingPlanService 등에서는 UTC 기준으로 시간을 처리하고 있으며, PricingCatalogController에서도 ZoneOffset.UTC를 기준으로 HTTP 헤더를 생성하고 있습니다. 일관성을 유지하고 시간대 관련 버그를 방지하기 위해, 모든 LocalDateTime.now() 호출 시 ZoneOffset.UTC를 명시하여 UTC 기준으로 저장하도록 수정하는 것이 안전합니다.

Suggested change
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();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

preUpdate 메서드에서도 마찬가지로 시스템 기본 시간대 대신 UTC 기준의 현재 시간을 사용하도록 수정해야 합니다.

Suggested change
updatedAt = LocalDateTime.now();
updatedAt = java.time.LocalDateTime.now(java.time.ZoneOffset.UTC);

Comment on lines +64 to +66
if (createdAt == null) {
createdAt = LocalDateTime.now();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

PricingPlan 엔티티의 createdAt 필드도 시스템 기본 시간대 대신 UTC 기준의 현재 시간을 사용하도록 수정하여 시간대 일관성을 유지해야 합니다.

Suggested change
if (createdAt == null) {
createdAt = LocalDateTime.now();
}
if (createdAt == null) {
createdAt = java.time.LocalDateTime.now(java.time.ZoneOffset.UTC);
}

Comment on lines +12 to +24
@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
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

activeOnlytrue일 때 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
	);

Comment on lines +48 to +54
public PricingPlanListResponse getPricingPlans(String provider, String model, Boolean activeOnly) {
List<PricingPlanItemResponse> items =
pricingPlanRepository.findPricingPlans(
blankToNull(provider),
blankToNull(model),
Boolean.TRUE.equals(activeOnly)
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

PricingPlanRepository.findPricingPlans 메서드가 현재 시간(now)을 파라미터로 받도록 수정됨에 따라, 서비스 레이어에서도 UTC 기준의 현재 시간을 전달하도록 수정해야 합니다.

Suggested change
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)
)

Comment on lines +28 to +29
public PricingPlanCreateResponse create(PricingPlanCreateRequest request) {
PricingPlan plan = PricingPlan.builder()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

가격표 등록 시 시작일(effectiveFrom)과 종료일(effectiveTo)의 선후 관계에 대한 검증 로직이 없습니다. 종료일이 시작일보다 앞서는 경우 잘못된 데이터가 저장될 수 있으므로, effectiveToeffectiveFrom보다 이후인지 검증하는 로직을 추가하는 것이 안전합니다.

Suggested change
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()

Comment on lines +14 to +24
@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
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

publishedAt 컬럼은 null을 허용합니다. 데이터베이스 종류(MySQL, PostgreSQL, H2 등)에 따라 ORDER BY ... DESC 정렬 시 null 값의 위치(처음 또는 마지막)가 다르게 처리될 수 있습니다. 이로 인해 로컬 테스트 환경(H2)과 운영 환경(PostgreSQL/MySQL)에서 '최신 활성 카탈로그'를 조회하는 결과가 달라지는 잠재적 버그가 발생할 수 있습니다. coalesce(c.publishedAt, c.createdAt)를 사용하여 null 안전하게 정렬하도록 수정하는 것을 권장합니다.

Suggested change
@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
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant