Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ public ResponseEntity<Page<CustomerListResponse>> getAllCustomers(
public ResponseEntity<CustomerSearchResponse> searchByPhone(
@RequestBody PhoneSearchRequest request) {
Customer customer = customerService.loadByPhone(request.phoneRaw());

Long customerId = customer.getCustomerId();

List<SubscriptionResponse> subscriptions =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public Slice<SubscriptionListResponse> getAllSubscriptions(Pageable pageable) {
sub,
totalUsedBytes,
subPlan.getAllotmentAmount(),
subPlan.getPlanName(),
subPlan.getPlan().getPlanName(),
Comment thread
arlen02-01 marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

subPlan.getPlan().getPlanName() 호출은 plan 엔티티가 지연 로딩(FetchType.LAZY)으로 설정되어 있어 N+1 쿼리 문제를 유발할 수 있습니다. getAllSubscriptions 메소드는 페이지네이션된 여러 구독을 처리하므로, 각 구독에 대해 Plan을 가져오기 위한 추가 쿼리가 발생하여 성능 저하를 일으킬 수 있습니다.

SubscriptionPlan 엔티티에는 planName 필드가 이미 존재하며, 이는 구독 계획 생성 시점의 요금제 이름을 스냅샷으로 저장하는 역할을 합니다. subPlan.getPlanName()을 사용하면 추가 쿼리 없이 이 문제를 해결하고, 원래의 스냅샷 데이터를 사용하는 의도에도 부합합니다.

아마 테스트 코드에서 subPlan.getPlanName()에 대한 모의(mock) 설정이 누락되어 이 부분을 수정하신 것으로 보입니다. 프로덕션 코드를 변경하는 대신, 테스트에서 given(subPlan.getPlanName()).willReturn(...)을 추가하여 해결하는 것이 더 나은 접근 방식입니다.

Suggested change
subPlan.getPlan().getPlanName(),
subPlan.getPlanName(),

decryptedEmail,
decryptedPhone);
})
Expand Down Expand Up @@ -115,7 +115,7 @@ public SubscriptionDetailResponse getSubscriptionDetailByPhone(String phoneRaw)
sub,
totalUsedBytes,
subPlan.getAllotmentAmount(),
subPlan.getPlanName(),
subPlan.getPlan().getPlanName(),
Comment thread
arlen02-01 marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

subPlan.getPlan().getPlanName() 호출은 plan 엔티티가 지연 로딩(FetchType.LAZY)으로 설정되어 있어 불필요한 쿼리를 유발할 수 있습니다. getSubscriptionDetailByPhone 메소드에서는 단일 항목을 조회하므로 getAllSubscriptions 만큼 심각한 성능 문제는 아니지만, 일관성을 유지하고 설계 의도에 맞게 SubscriptionPlan에 스냅샷으로 저장된 planName 필드를 사용하는 것이 좋습니다.

이렇게 하면 불필요한 데이터베이스 조회를 피하고, SubscriptionPlanplanName 필드가 가진 스냅샷의 의미를 명확히 할 수 있습니다.

Suggested change
subPlan.getPlan().getPlanName(),
subPlan.getPlanName(),

decryptedEmail,
decryptedPhone);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,4 @@ Page<TemplateGroup> search(
@Param("includeDeleted") boolean includeDeleted,
@Param("keyword") String keyword,
Pageable pageable);

@Query(
"""
select g from TemplateGroup g
where (:includeDeleted = true or g.isDeleted = false)
and (:isActive is null or g.isActive = :isActive)
""")
Page<TemplateGroup> searchWithoutKeyword(
@Param("isActive") Boolean isActive,
@Param("includeDeleted") boolean includeDeleted,
Pageable pageable);
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,9 @@ public TemplateGroupResponse create(TemplateGroupCreateRequest request) {
// 그룹 목록 조회
public Page<TemplateGroupResponse> getGroups(
Boolean isActive, boolean includeDeleted, String keyword, Pageable pageable) {
if (keyword == null || keyword.isBlank()) {
return groupRepository
.searchWithoutKeyword(isActive, includeDeleted, pageable)
.map(TemplateGroupResponse::from);
}
String normalizedKeyword = (keyword == null || keyword.isBlank()) ? null : keyword;
return groupRepository
.search(isActive, includeDeleted, keyword, pageable)
.search(isActive, includeDeleted, normalizedKeyword, pageable)
.map(TemplateGroupResponse::from);
Comment thread
arlen02-01 marked this conversation as resolved.
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/application-secret.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ ureca:
hash-key: ${URECA_HASH_KEY}
billing:
grafana:
iframe-url: http://34.22.76.102/d-solo/cfb0vm63z241sc/test?orgId=1&from=1769139882304&to=1769161482304&timezone=browser&panelId=1&__feature.dashboardSceneSolo
iframe-url: https://grafana.4e-um.cloud/public-dashboards/1656cac08ded4a22820d1ec97d3edad6?orgId=1
Comment thread
arlen02-01 marked this conversation as resolved.
refresh-interval: 5s
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,7 @@ void getAllSubscriptionsSuccess() {
Slice<Subscription> slice = new SliceImpl<>(List.of(sub));

// SubscriptionPlan Mock 설정 (Mockito mock 사용)
SubscriptionPlan subPlan = org.mockito.Mockito.mock(SubscriptionPlan.class);
given(subPlan.getSubscription()).willReturn(sub);
given(subPlan.getAllotmentPeriod()).willReturn(AllotmentPeriod.MONTH);
given(subPlan.getAllotmentAmount()).willReturn(10240L);
SubscriptionPlan subPlan = setupMockSubscriptionPlan(sub);

given(subscriptionRepository.findAllSlice(pageable)).willReturn(slice);
given(subscriptionPlanRepository.findActivePlanBySubId(1L))
Expand Down Expand Up @@ -110,10 +107,7 @@ void getSubscriptionDetailByPhoneSuccess() {
Subscription sub = newInstanceSubscription(1L, customer);

// SubscriptionPlan Mock 설정 (Mockito mock 사용)
SubscriptionPlan subPlan = org.mockito.Mockito.mock(SubscriptionPlan.class);
given(subPlan.getSubscription()).willReturn(sub);
given(subPlan.getAllotmentPeriod()).willReturn(AllotmentPeriod.MONTH);
given(subPlan.getAllotmentAmount()).willReturn(10240L);
SubscriptionPlan subPlan = setupMockSubscriptionPlan(sub);

given(contactHashUtil.hmacSha256Base64(phoneRaw)).willReturn(hash);
given(subscriptionRepository.findByPhoneHash(hash)).willReturn(Optional.of(sub));
Expand Down Expand Up @@ -154,6 +148,18 @@ private static Subscription newInstanceSubscription(Long subId, Customer custome
return subscription;
}

private static SubscriptionPlan setupMockSubscriptionPlan(Subscription sub) {
SubscriptionPlan subPlan = org.mockito.Mockito.mock(SubscriptionPlan.class);
com.project.core.infra.entity.plan.Plan plan =
org.mockito.Mockito.mock(com.project.core.infra.entity.plan.Plan.class);
Comment thread
arlen02-01 marked this conversation as resolved.
Comment on lines +153 to +154
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

com.project.core.infra.entity.plan.Plan 클래스의 전체 경로를 사용하고 있어 코드가 길어지고 가독성이 떨어집니다. 파일 상단에 import com.project.core.infra.entity.plan.Plan;를 추가하고, 여기서는 짧은 클래스 이름 Plan을 사용하는 것이 좋습니다.

Suggested change
com.project.core.infra.entity.plan.Plan plan =
org.mockito.Mockito.mock(com.project.core.infra.entity.plan.Plan.class);
Plan plan = org.mockito.Mockito.mock(Plan.class);

given(plan.getPlanName()).willReturn("BASIC");
given(subPlan.getPlan()).willReturn(plan);
given(subPlan.getSubscription()).willReturn(sub);
given(subPlan.getAllotmentPeriod()).willReturn(AllotmentPeriod.MONTH);
given(subPlan.getAllotmentAmount()).willReturn(10240L);
return subPlan;
}

@Test
@DisplayName("[조회] 성공 - 고객의 회선 목록 조회(응답 DTO 변환)")
void findSubscriptionResponsesSuccess() {
Expand Down