[UPLUS-57] 할인 추가삭제 기능 추가/developer브랜치와 병합 및 오류 수정 - #10
arlen02-01 wants to merge 2 commits into
Conversation
Summary of ChangesHello @arlen02-01, 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! 이 PR은 고객 및 구독 관리 기능을 대폭 확장하고 새로운 할인 정책 관리 시스템을 도입합니다. 고객 정보를 조회하고 수정하는 API를 추가하며, 구독에 대한 할인 정책을 유연하게 관리할 수 있는 엔티티와 서비스 로직을 구현합니다. 또한, 민감한 구독 정보를 안전하게 처리하기 위해 전화번호 복호화 및 마스킹 기능을 포함합니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
arlen02-01
changed the title
![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은 할인 추가/삭제 기능과 관련된 새로운 엔티티, 리포지토리, 서비스를 추가하고 기존 고객 및 구독 관련 코드를 수정합니다. 전반적으로 기능 추가의 방향은 좋지만, 몇 가지 중요한 문제점들이 보입니다. 특히, SubscriptionService에서 엔티티를 직접 수정하여 마스킹된 데이터를 저장하는 부분은 데이터 손실을 유발할 수 있는 심각한 버그입니다. 또한, 금액을 다루는 필드에 Double 타입을 사용하는 것은 정밀도 문제를 일으킬 수 있으므로 BigDecimal로 변경해야 합니다. 컨트롤러에서 엔티티를 직접 노출하는 대신 DTO를 사용하는 것이 좋으며, 예외 처리와 명명 규칙을 일관성 있게 적용할 필요가 있습니다. 아래의 상세 리뷰를 확인하고 수정해 주시기 바랍니다.
| public void setPhoneNumber(String maskedNum) { | ||
| this.phoneNumber = maskedNum; | ||
| } |
There was a problem hiding this comment.
phoneNumber 필드는 암호화된 전화번호를 저장해야 하는 중요한 데이터입니다. 이 setPhoneNumber 메서드는 SubscriptionService에서 마스킹된 전화번호를 저장하는 데 사용되고 있어, 영구적으로 데이터를 손상시킬 위험이 있습니다. 엔티티의 상태는 비즈니스 로직에 의해서만 변경되어야 하며, 화면 표시용 데이터로 덮어쓰여서는 안 됩니다. 이 메서드를 제거하고, SubscriptionService에서는 DTO를 사용하여 마스킹된 데이터를 전달하도록 리팩토링하는 것을 강력히 권장합니다.
| public List<Subscription> findSubscription(Long customerId) throws Exception { | ||
| List<Subscription> subscriptions = | ||
| subscriptionRepository.findByCustomer_CustomerId(customerId); | ||
|
|
||
| if (subscriptions.isEmpty()) { | ||
| throw new IllegalStateException("보유중인 회선이 없습니다."); | ||
| }else { | ||
| //복호화 -> 마스킹 | ||
| for (int i = 0; i < subscriptions.size(); i++) { | ||
| String num = aesUtil.decrypt(subscriptions.get(i).getPhoneNumber()); | ||
| String maskedNum = num.substring(0,4)+"**"+num.substring(6,9)+"**"+num.substring(11,12); //"010-**34-**78" | ||
| subscriptions.get(i).setPhoneNumber(maskedNum); | ||
| } | ||
| } | ||
|
|
||
| return subscriptions; | ||
| } |
There was a problem hiding this comment.
@Transactional 메서드 내에서 조회한 Subscription 엔티티의 전화번호를 마스킹 처리한 값으로 변경하고 있습니다. 이로 인해 트랜잭션이 커밋될 때 암호화된 원래 전화번호가 마스킹된 값으로 데이터베이스에 덮어씌워져 데이터가 유실되는 심각한 문제가 발생합니다.
프레젠테이션 계층에 필요한 데이터 가공(마스킹)은 엔티티를 직접 수정하는 대신 별도의 DTO(Data Transfer Object)를 만들어 처리해야 합니다. Subscription 엔티티의 상태를 변경하지 않도록 하고, 마스킹된 값을 DTO에 담아 반환하도록 전체적인 구조를 변경해주세요.
| @Column(name = "value", nullable = false) | ||
| private Double value; |
There was a problem hiding this comment.
금액이나 비율과 같은 값을 Double 타입으로 저장하면 부동 소수점 연산 시 정밀도 문제가 발생할 수 있습니다. 금융 관련 계산에서는 BigDecimal 타입을 사용하는 것이 표준적인 방법입니다. value 필드의 타입을 BigDecimal로 변경해주세요.
| @Column(name = "value", nullable = false) | |
| private Double value; | |
| @Column(name = "value", nullable = false) | |
| private java.math.BigDecimal value; |
| private DiscountType discountType; | ||
|
|
||
| @Column(name = "value", nullable = false) | ||
| private Double value; |
There was a problem hiding this comment.
금액이나 비율과 같은 값을 Double 타입으로 저장하면 부동 소수점 연산 시 정밀도 문제가 발생할 수 있습니다. 금융 관련 계산에서는 BigDecimal 타입을 사용하는 것이 표준적인 방법입니다. value 필드의 타입을 BigDecimal로 변경해주세요. 이 변경에 맞춰 빌더(SubscriptionDiscount.builder())의 파라미터와 내부 로직도 함께 수정해야 합니다.
| private Double value; | |
| private java.math.BigDecimal value; |
| public ResponseEntity<List<Customer>> loadByContactEnc( | ||
| @RequestParam String contactEnc | ||
| ) { | ||
| List<Customer> customers = customerService.loadByContactEnc(contactEnc); | ||
| return ResponseEntity.ok(customers); | ||
| } |
There was a problem hiding this comment.
컨트롤러에서 JPA 엔티티(Customer)를 직접 반환하고 있습니다. 이는 다음과 같은 문제를 야기할 수 있습니다.
- 엔티티의 모든 필드가 외부에 노출되어 API 스펙이 내부 구현에 강하게 결합됩니다.
- 지연 로딩(Lazy Loading) 관련 예외가 발생할 수 있습니다.
- 엔티티 필드 변경 시 API 응답 구조가 예기치 않게 변경될 수 있습니다.
Customer 엔티티의 정보를 담는 별도의 DTO(e.g., CustomerResponseDto)를 생성하고, 엔티티를 DTO로 변환하여 반환하도록 수정하는 것이 좋습니다.
| 복지 | ||
| ,프로모션 | ||
| ,결합 |
There was a problem hiding this comment.
Enum 상수를 한글로 정의하면 일부 시스템이나 라이브러리에서 인코딩 문제를 일으킬 수 있으며, 다국어 지원 시 관리가 어려워집니다. Enum 상수는 일반적으로 영문 대문자와 스네이크 케이스(UPPER_SNAKE_CASE)로 작성하는 것이 표준입니다. 한글명은 별도의 필드로 관리하는 것을 권장합니다.
| 복지 | |
| ,프로모션 | |
| ,결합 | |
| WELFARE, | |
| PROMOTION, | |
| COMBINATION |
| Rate | ||
| ,Fixed |
There was a problem hiding this comment.
Java Enum 상수 명명 규칙에 따라 UPPER_SNAKE_CASE를 사용하는 것이 좋습니다. (e.g., Rate -> RATE).
| Rate | |
| ,Fixed | |
| RATE, | |
| FIXED |
| private final CustomerRepository customerRepository; | ||
|
|
||
| @Transactional | ||
| public List<Customer> loadByContactEnc(String contactEnc) {//유저 조회 |
There was a problem hiding this comment.
코드 내에 한글 주석(//유저 조회)이 있습니다. 팀의 컨벤션에 따라 다르겠지만, 일반적으로 주석은 영문으로 작성하거나, 메서드에 대한 설명은 Javadoc을 사용하는 것이 유지보수와 가독성 측면에서 더 좋습니다.
| public List<Customer> loadByContactEnc(String contactEnc) {//유저 조회 | |
| public List<Customer> loadByContactEnc(String contactEnc) { |
| if (discounts.isEmpty()) { | ||
| throw new IllegalStateException("해당 회선에 적용된 할인이 없습니다"); | ||
| } |
There was a problem hiding this comment.
엔티티를 찾지 못했을 때 IllegalStateException을 던지고 있습니다. 프로젝트의 다른 부분(CustomerService)에서는 EntityNotFoundException을 사용하고 있으므로, 일관성을 위해 EntityNotFoundException을 사용하는 것이 좋습니다. 이를 위해 적절한 ErrorCode를 정의하고 사용해주세요.
| //복호화 -> 마스킹 | ||
| for (int i = 0; i < subscriptions.size(); i++) { | ||
| String num = aesUtil.decrypt(subscriptions.get(i).getPhoneNumber()); | ||
| String maskedNum = num.substring(0,4)+"**"+num.substring(6,9)+"**"+num.substring(11,12); //"010-**34-**78" |
There was a problem hiding this comment.
전화번호 마스킹 로직이 substring과 인덱스를 사용하여 구현되어 있어 특정 형식(010-xxxx-xxxx)에 강하게 의존적입니다. 전화번호 형식이 달라지면 StringIndexOutOfBoundsException이 발생할 수 있습니다. 또한, 코드 옆의 주석(//"010-**34-**78")과 실제 마스킹 결과가 일치하지 않아 혼란을 줍니다. 정규식을 사용하거나 유틸리티 클래스를 만들어 더 안정적이고 명확한 방법으로 마스킹을 처리하는 것을 고려해보세요.
1 participant
🍀 이슈 번호
UPLUS-57
✅ 작업 사항
할인 추가삭제 기능 추가/developer브랜치와 병합 및 오류 수정
📋 체크리스트
⌨ 기타