refactor(ticketItemOption) : 옵션 적용하기 구조 리펙토링 #215

kim-wonjin · 2023-01-28T20:25:19Z

개요

close 🔨[refactor] 옵션 적용하기 구조 변경 #211

작업사항

ticketItem 내에서 itemOptionGroup을 관리할수 있도록 구조변경
검증로직은 도메인 서비스 레이어로 옮겼고, 세부 검증 로직은 도메인 내부 메소드로 구현하였습니다.
재고가 이미 감소된 티켓상품의 경우 옵션 변경이 불가능하도록 에러처리 추가하였습니다.

질문

재고 감소 로직과 함께 락을 걸어야 할거 같다는 생각이 들었는데, 티켓 발행시 재고 감소시키는 부분에서 락을 걸어줬으니 여기서는 걸어줄 필요가 없는건가요? 아니면 양쪽에서 걸어야 하는건지 잘 모르겠어용

# Conflicts: # DuDoong-Domain/src/main/java/band/gosrock/domain/domains/ticket_item/exception/TicketItemErrorCode.java

ImNM · 2023-01-29T04:26:58Z

...ng-Api/src/main/java/band/gosrock/api/ticketItem/dto/response/ApplyTicketOptionResponse.java

-    private final Long itemOptionGroupId;

    @Schema(description = "티켓상품 id")
    private final Long ticketItemId;

-    @Schema(description = "옵션그룹 id")
-    private final Long optionGroupId;
+    @Schema(description = "옵션그룹 id 리스트")
+    private final List<Long> optionGroupIds;


내려주는 정보에서
id 정보보다는 티켓 정보랑
옵션 정보 뿌려주는게 프론트에서 편할것같긴해유

ImNM · 2023-01-29T04:28:08Z

DuDoong-Api/src/main/java/band/gosrock/api/ticketItem/service/ApplyTicketOptionUseCase.java

-                        ticketOptionMapper.toItemOptionGroup(ticketItem, optionGroup));
-        return ApplyTicketOptionResponse.from(itemOptionGroup);
+        TicketItem ticketItem =
+                itemOptionGroupService.createItemOptionGroup(ticketItemId, optionGroupId, eventId);


addItemOptionGroup 이 더 적절한 네이밍일듯!

eventId 는
티켓 아이템이랑 옵션그룹 엔티티에서 둘다 조회가능한 정보이니
인자로 받지말고 도메인 서비스 내부에서
티켓 아이템의 이벤트랑 옵션그룹의 이벤트가 같은지 검증하면 될것같긴하네유!
근데뭐.. 이건 자율! 의견입니다!

세개가 모두 같은지 확인하는게 좋을거 같아서 요렇게 했숨니당

ImNM · 2023-01-29T04:30:56Z

...in/src/main/java/band/gosrock/domain/domains/ticket_item/service/ItemOptionGroupService.java

+    // @RedissonLock(LockName = "티켓 옵션적용", identifier = "ticketItemId")
+    @Transactional


여기에도 락을 걸긴해야해요
재고 감소

https://github.com/Gosrock/DuDoong-Backend/blob/refactor/112-order-validation-and-docs/DuDoong-Domain/src/main/java/band/gosrock/domain/domains/issuedTicket/service/IssuedTicketDomainService.java

@RedissonLock(LockName = "티켓재고관리", identifier = "itemId") @Transactional public void withDrawIssuedTicket(Long itemId, List<IssuedTicket> issuedTickets) {

위처럼요! 현재는 ticketItemId 가 identifier 이구
분산락 안에 트랜잭션 존재하니 트랜잭션 어노테이션은 빼도됩니당!

ImNM · 2023-01-29T04:31:27Z

...in/src/main/java/band/gosrock/domain/domains/ticket_item/service/ItemOptionGroupService.java

+        if (ticketItem.isQuantityReduced()) {
+            throw ForbiddenOptionChangeException.EXCEPTION;
        }


티켓 아이템 도메인 내부로 이동해도 될것같습니다~!

ImNM · 2023-01-29T04:31:39Z

...in/src/main/java/band/gosrock/domain/domains/ticket_item/service/ItemOptionGroupService.java

+        if (ticketItem.hasItemOptionGroup(optionGroup.getId())) {
            throw DuplicatedItemOptionGroupException.EXCEPTION;
        }


여기두요!

위에꺼랑 마찬가지로 주문내역이 있는지, 적용된 옵션인지 확인할일이 또 있을까봐 (무조건 쳐내지않고 확인) exception은 밖으로 뺀건데 그럴일 없을라나요?

hasItemOptionGroup 메서드 그대로 두시고
위 세줄을 또 메서드로 파심됩니다!

오옹 exception은 도메인 내부 메소드에서 던지는게 더 좋나요?

그쵸
지금 우리가 코딩하니깐 그런거지
ItemOptionGroupService 가
지금소스보면 ticketItem.hasItemOptionGroup 메서드로
체크를 하잖아유

그말인즉슨 ItemOptionGroupService 가 ticketItem의 내부 정보까지 알아야 코딩이 가능한건데
ticketItem.validHasItemOptionGroup 메서드나

ticketItem.addOptionGroup() 메서드를 실행시키면
자동으로 재고 감소 되어있는지를 검증해준다거나

둘중하나의 방식으로 캡슐화를 잘 지켜야 된다고 봅니다
저는 후자의방식 추천드릴게유 그게 더 안전하잖아요?
addOptionGroup 전에 항상 validHasItemOptionGroup 를 호출해야한다라는걸
누구나 빼먹을수 있기때문에

ImNM · 2023-01-29T04:32:21Z

...ain/src/main/java/band/gosrock/domain/domains/ticket_item/exception/TicketItemErrorCode.java

-            BAD_REQUEST, "Option_400_1", "옵션에 대한 답변이 올바르지 않습니다. T/F형일 경우 예 아니요 로 보내주세요.");
-
+            BAD_REQUEST, "Option_400_1", "옵션에 대한 답변이 올바르지 않습니다. T/F형일 경우 예 아니요 로 보내주세요."),
+    FORBIDDEN_OPTION_CHANGE(FORBIDDEN, "Item_Option_Group_403_1", "옵션 변경이 불가능한 상태입니다.");


FORBIDDEN 말고 400번대로 고쳐주세요
403 으로 응답주면 클라이언트에서 자동으로 로그아웃 됩니다!

ImNM · 2023-01-29T04:43:04Z

질문

재고 감소 로직과 함께 락을 걸어야 할거 같다는 생각이 들었는데, 티켓 발행시 재고 감소시키는 부분에서 락을 걸어줬으니 여기서는 걸어줄 필요가 없는건가요? 아니면 양쪽에서 걸어야 하는건지 잘 모르겠어용

결론부터 말씀드리면 양쪽에서 걸어야합니다!

만일 옵션 변경을 할때에 같은 락을 안건다면

옵션 변경할때 재고가 100으로 풀로 채워져있는걸 확인하고
누가 그사이에 티켓을 발급해버림 ( 재고 감소 ) <- 이것때문에 안됩니당!!
100으로 풀로 채워진줄알고 옵션 변경

옵션 변경할때에도 재고 감소 체크하는 락을건다면?

옵션 변경할때 재고가 100으로 풀로 채워져있는걸 확인
누가 그 사이에 티켓을 발급 하고싶지만 락이 걸려져있어서 재고 감소에 접근하지못하고 기다림
옵션 변경작업 끝남
이후에 티켓 발급 가능해짐

이순입니다!

@kim-wonjin 그리고 분산락 안에는 트랜잭션 전파속성이 Required_New 라서
항상 새로운 트랜잭션이 시작된다는점 염두해주세요!

왜냐면 트랜잭션 읽기가 Read commited 이므로
먼저 시작된 트랜잭션 안에 Required_New 트랜잭션이 새로 시작되고
옵션이 변경되었다면 먼저 시작된( 응답값을 뿌려주는 트랜잭션은 ) 해당 업데이트된 정보가 안보입니다!

응답값 뿌려줄때 김영한 선생님이 말씀하신것처럼 커맨드 칠때는 id 값 정도만 리턴해주고
맵퍼 활용하셔서 id 값으로 새로 레포지토리에서 찾아와야
업데이트 된 정보가 보일 수 있습니다!

재고감소 쪽 락이 주문 승인 된 다음 <- 예도 실패
티켓 발급 할때 락 <- 예가 실패

ImNM · 2023-01-29T04:44:39Z

현재 ApplyTicketOptionUseCase 구조론
크게 이상없을것 같은에 만일 막 업데이트 된 정보가 안보인다하면
위 상황 한번 생각해주세요
Required_New 로 분산락 안에 트랜잭션이 새로 시작되는 문제!

sanbonai06

리팩토링 하시느라 수고하셨습니당 LGTM ⭐

sonarqubecloud · 2023-01-29T08:28:47Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
223 Code Smells

15.9% Coverage
0.0% Duplication

kim-wonjin added 5 commits January 29, 2023 01:23

refactor(itemOptionGroup) : 티켓상품 내 옵션그룹 관리 도메인 메소드 생성

a4974a5

refactor(itemOptionGroup) : 요청DTO 내 id검증 어노테이션 사용

65d9953

refactor(itemOptionGroup) : 옵션적용 로직 구조 리펙토링 및 검증 추가

2ffd0af

Merge branch 'dev' into refactor/211-apply-option

d41f876

# Conflicts: # DuDoong-Domain/src/main/java/band/gosrock/domain/domains/ticket_item/exception/TicketItemErrorCode.java

refactor(itemOptionGroup) : 에러코드 컨플릭트 해결

f9b3903

kim-wonjin self-assigned this Jan 28, 2023

kim-wonjin requested review from ImNM, sanbonai06, cofls6581 and gengminy as code owners January 28, 2023 20:25

kim-wonjin changed the title ~~Refactor/211 apply option~~ Refactor/211 옵션 적용하기 구조 리펙토링 Jan 28, 2023

ImNM approved these changes Jan 29, 2023

View reviewed changes

sanbonai06 approved these changes Jan 29, 2023

View reviewed changes

refactor(itemOptionGroup) : 분산락, 도메인 내부로 메소드 이, 에러코드 수정

110ecfa

kim-wonjin merged commit c06439e into dev Jan 29, 2023

kim-wonjin deleted the refactor/211-apply-option branch January 29, 2023 08:53

kim-wonjin changed the title ~~Refactor/211 옵션 적용하기 구조 리펙토링~~ refactor(ticketItemOption) : 옵션 적용하기 구조 리펙토링 Jan 30, 2023

kim-wonjin added For: API [이슈 대상] 외부 API Type: Refactor [이슈 목적] 프로덕션 코드 리팩토링 labels Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ticketItemOption) : 옵션 적용하기 구조 리펙토링 #215

refactor(ticketItemOption) : 옵션 적용하기 구조 리펙토링 #215

kim-wonjin commented Jan 28, 2023

ImNM Jan 29, 2023

ImNM Jan 29, 2023

ImNM Jan 29, 2023

kim-wonjin Jan 29, 2023

ImNM Jan 29, 2023

ImNM Jan 29, 2023

ImNM Jan 29, 2023

kim-wonjin Jan 29, 2023

ImNM Jan 29, 2023

kim-wonjin Jan 29, 2023

ImNM Jan 29, 2023

ImNM Jan 29, 2023

ImNM commented Jan 29, 2023 •

edited

Loading

질문

ImNM commented Jan 29, 2023

sanbonai06 left a comment

sonarqubecloud bot commented Jan 29, 2023

		// @RedissonLock(LockName = "티켓 옵션적용", identifier = "ticketItemId")
		@Transactional

refactor(ticketItemOption) : 옵션 적용하기 구조 리펙토링 #215

refactor(ticketItemOption) : 옵션 적용하기 구조 리펙토링 #215

Conversation

kim-wonjin commented Jan 28, 2023

개요

작업사항

질문

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ImNM commented Jan 29, 2023 • edited Loading

질문

ImNM commented Jan 29, 2023

sanbonai06 left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 29, 2023

ImNM commented Jan 29, 2023 •

edited

Loading