Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[유저 제보] @Transactional 어노테이션에 대한 고찰 및 제보 #1477

Closed
brixxt27 opened this issue Dec 12, 2023 · 2 comments
Closed
Assignees
Labels
BE Backend tasks BUG Issuing bugs

Comments

@brixxt27
Copy link

버그 설명

CabinetFacadeServiceImpl 클래스에서 getBuildingFloorsResponse() 메서드는 cabinetOptionalFetcher를 사용하여 모든 건물의 층 정보를 단순히 조회하여 가져옵니다. 이때 이 친구는 @transactional(readOnly = true)를 사용합니다. 그런데 같은 조회이지만 비교적 더 복잡한 기능을 수행하는 getCabinetsPerSection()는 트랜잭션을 사용하지 않습니다. 이는 건물의 층별 각 구역들에 있는 사물함들의 정보를 가져오는 메서드이죠. cabinetOptionalFetcher를 사용하는 곳만 일단 두 곳인데 그 중간에 데이터가 변경 되거나 중간에 하나가 실패한다고 하면 개발자가 의도한대로 흘러가지 못할 것 같습나다.
image

image

image

재현 방법

@transactional(readOnly = true)를 해당 메서드에 추가한다.

기대했던 정상 동작

에러 상황 시 개발자가 예외를 컨트롤 가능하다.

시스템 환경 (선택 사항)

No response

추가 건의사항

@Ssuamje 저번에 피드백 주신 Transaction 을 도입하려고 합니다. 이 때 해당 질문은 Spring에서 AOP로 구현 된 @transactional 어노테이션을 어느 단위에, 또 어디에 적어야 할까라는 물음에서 시작했습니다. 다음은 Cabi 코드를 직접 리뷰하고 난 다음에 작성한 질문입니다.

  1. Transactional 어노테이션은 컨트롤러의 단위가 아닌, 직접적인 SQL 등의 세부로직이 작성 되는 서비스 레벨에서 사용하는 것 같은데 맞나요? 맞다면 제가 언급한 기준이 맞을까요?
    image

  2. 어노테이션을 서비스 클래스 단위에 적을 때도 있고, 메서드에 직접 적을 때도 있습니다. 이때 이 차이가 클래스 내에 private 메서드 존재의 차이라 생각했습니다. 그러나 사진에 첨부한 LentExtensionOptionalFetcher 클래스는 public 메서드만 존재하고, 모든 메서드가 읽기 전용 트랜잭션을 사용하는데 클래스 단위에서 @transactional(readOnly = true)를 사용하지 않은 이유가 있을 까요? 이는 이후 메서드 추가 등의 유지 보수를 고려한 것일까요?

  3. CabinetFacadeServiceImpl 클래스의 질문입니다. getBuildingFloorsResponse() 메서드는 cabinetOptionalFetcher를 사용하여 모든 건물의 층 정보를 단순히 조회하여 가져옵니다. 이때 이 친구는 @transactional(readOnly = true)를 사용합니다. 그런데 같은 조회이지만 비교적 더 복잡한 기능을 수행하는 getCabinetsPerSection()는 트랜잭션을 사용하지 않습니다. 이는 건물의 층별 각 구역들에 있는 사물함들의 정보를 가져오는 메서드이죠. 어노테이션 사용에서 둘의 차이가 있는 이유가 있을까요?

@brixxt27 brixxt27 changed the title [유저 버그 제보] @Transactional 어노테이션에 대한 고찰 및 제보 [유저 제보] @Transactional 어노테이션에 대한 고찰 및 제보 Dec 12, 2023
@Ssuamje
Copy link
Collaborator

Ssuamje commented Dec 12, 2023

854472e
해당 제보주신 부분에 대해서 위 커밋으로 핫픽스하였습니다!

해당 질문에 대한 답변은 이전에 DM으로 문의주셨을 때에 답변으로 갈음합니다!

  1. @transactional은 서비스부터 사용하는게 맞다고 생각합니다(물론 컨트롤러에서 사용할 수 없는 건 아니지만 권장하지 않습니다). 트랜잭션 자체가 ‘여러 조작을 하나의 단위로 묶음(원자성)’을 목표로 하는 부분이라고 생각해서요(관련해서는 ACID 원칙을 찾아보시면 좋을 것 같아요). 물론 컨트롤러 자체에서 하나의 라우트가 여러가지 서비스를 호출하고, 그것에 대한 원자성을 보장하려면 그때는 컨트롤러에서도 @transactional이 달릴 수 있겠지만 그건 컨트롤러의 책임 느낌은 아니라서, Facade를 따로 두었습니다.

  2. 클래스에 달아두면 전체적용, 메서드에 달아두면 개별적용이라고 생각하시면 될 것 같아요. 관련해서는 @transactional의 전파(propagation)을 찾아보시면 좋을 것 같습니다! 엄밀하게 걸지 않아도 알아서 해주는 부분이 있어서, 그 클래스가 그냥 전부 Transactional이면 클래스에 걸고 나뉘면 메서드에 거는 것 같아요. 물론 이것도 코드 작성하는 사람 마음 나름인 것 같습니다. 유지보수라기에는 사실 단순히 어노테이션 일일히 붙여주는 것 정도일 것 같아요.

  3. 흠 지금 살펴보니 사실 걸려있어야하는게 맞는 것 같네요. 그리고 Transactional(readOnly=true)로 해야할 것 같구요(조회만 수행하는 쿼리니까). 이 경우에는 OptionalFetcher를 두번 호출하는데, 해당 클래스는 어차피 트랜잭션이 각 메서드별로 걸려있어서 각 요청에 대해 조회하는 것은 문제가 없겠지만 그 두 번의 호출 사이에 내용이 바뀌게 되면 데이터 정합이 보장되지는 않을 수 있겠네요! jayoon님이 오히려 리포팅을 해주셔서 놓친 문제점을 찾은 것 같습니다(물론 실제 사용에서는 일어날 확률이 매우 드물어서 지장은 없었던 것 같아요).

@Ssuamje Ssuamje self-assigned this Dec 12, 2023
@Ssuamje Ssuamje added BUG Issuing bugs BE Backend tasks labels Dec 12, 2023
@Ssuamje
Copy link
Collaborator

Ssuamje commented Dec 12, 2023

제보 감사합니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE Backend tasks BUG Issuing bugs
Projects
None yet
Development

No branches or pull requests

2 participants