refactor: @Transactional(readOnly=true) 중첩사용

[boyekim]

1. saveAndFlush 고려를 해보았습니다.

• 그런데 필요성을 잘 모르겠어요. 현재 @Transactional이 Service 클래스 상위에 선언이 되어있고, 읽기전용으로 쓰는 것 에만 readOnly=true가 되어있는데요, 회원가입 시에 save후 바로 메서드가 끝나기때문에 DB에 무리없이 반영될 것 같습니다. 그리고 중복 이메일에 대한 건은 save전에 이메일 검증을 한번 더 해주기 때문에(이메일 중복 검증 버튼 이외에 한번 더 회원가입 버튼을 눌렀을 때 중복 검증이 되는 것) 과연 중복때문에 문제가 생길지 의문이었어요! 그래서 우선은 save로 남겨두었습니다

2. @Transactional(readOnly=true)를 PostService의 getCommentList 메서드에서 빼줬어요!! getCommentList 메서드는 getEachPost에서 호출되는것으로만 사용되는데용 getEachPost에 @Transactional(readOnly=true)가 되어있기 때문에 하위에도 적용이 될 것으로 생각했어요 참고

3. 하나 논의할 사항을 미리 말해 두자면 dto에 studyId 와 studyGroupId가 섞여서 쓰이는데요, 이걸 studyGroupId으로 통일하는게 좋을 것 같다는 의견이 나왔어요. 프론트에 피바람이 불 것 같아서 ... 멋대로 해두기보다 우선 함께 논의 후 진행하겠습니다..^^

4. url의 groups를 studygroups라고 하기에는 가독성이 너무 떨어지는 것 같아서 이거는 groups로 남겨두는게 어떤가요?

[gogo1414]

코드가 더 이뻐졌다👍👍

[gogo1414]

Suggested change

	Member nowCommentMember = comment.getMember();
	Member �commentMember = comment.getMember();

변수 네이밍을 이렇게 바꾸는건 어때?? nowCommentMember도 직관적인 표현이긴 한데, now가 없어도 이해가 되는 의미라고 생각해서!
편한대로 적용하면 좋을거 같어!

[gogo1414]

두 이미지를 보면 변수 네이밍에 대해 깃헙 코드로 해본건데 nowCommentMember 변수 네이밍이 우리 코드밖에 없네😂

[gogo1414]

의견

우선, 제가 설명했던 문제 발생 예시를 다시 가져와 볼게요!

사용자가 회원가입을 하고, 다른 사용자가 동일한 메일로 중복 검사를 실행했을 때, 아직 첫 번째 사용자의 정보가 DB에 반영되지 않아서 두 번째 사용자가 중복 검사를 통과하는 문제가 생길 수 있어요!

서비스의 흐름을 고려해보면, 사용자가 이메일 검증 버튼을 통해 이메일을 확인하고 그 후에 회원가입을 진행하게 됩니다. 수민이가 말한 것처럼 회원가입 시 save() 전에 이메일 중복 검사를 한 번 더 진행하면 이 문제는 예방이 가능해요!

그런데, 만약 중복 검사가 제대로 이루어지지 않는다면, 굳이 이메일 중복 검증 버튼이 필요한지에 대해 의문이에요! 버튼을 통한 검증이 충분히 신뢰할 수 없다면, 버튼을 없애고 가입 과정에서 검증만으로도 충분히 안전하다고 볼 수 있지 않을까요? 이메일 중복 검사는 서비스 흐름 상에서 이메일의 유일성을 보장해주는 기능인데, 회원가입 자체를 보장하지 못한다면 버튼은 없어도 될 것 같아요! (제가 생각했을 때 save() 전에 중복 검사를 한 번 더 하는 것은 안전장치라고 생각했어요!)

동시성 문제

제가 생각한 문제는 위의 설명 뿐만 아니라 동시성 문제도 존재했어요!

문제 발생 예시

두 사용자가 동시에 같은 이메일로 회원가입을 시도할 때, 첫 번째 사용자가 이메일 검증을 통과하고 save()를 호출하는 시점과, 두 번째 사용자가 검증을 통과하는 시점 사이에 시간이 생길 수 있어요. 그 결과, 두 사용자가 모두 중복 검사를 통과하고, 두 개의 동일한 이메일 계정이 등록될 위험이 있습니다.

이런 동시성 문제는 데이터베이스 트랜잭션의 처리 순서 때문에 발생할 수 있는 문제입니다. 실제로 서비스 사용자가 많지 않으면 문제가 될 가능성은 적지만, 안정성을 높이기 위해서는 고려해볼만 하다고 생각해요!

총정리

현재 서비스의 규모를 고려했을 때는 save()만으로 충분할 수 있지만, 서비스 흐름의 안정성을 보장하기 위해 이전 PR에 saveAndFlush() 도입에 대해 의견을 물어봤었어요!

제가 작성한 글을 읽고 코드를 작성한 수민이 뿐만 아니라 다른 사람들의 추가적인 의견도 궁금해요😁
(MVP 과정이 끝났으니 이런 신나는 주제에 대해 마음껏 얘기합시다!!!!)

[whxogus215]

의견 전달이 늦은 점 죄송합니다 😢 좋은 PR과 좋은 코멘트 덕분에 저도 생각치 못한 부분들을 고민해볼 수 있었습니다 👍

1. saveAndFlush 고려를 해보았습니다에 대한 의견

• 이메일 중복 검사와 동시성 측면에서 분석한 준수의 의견도 공감이 되었고, 간단한 기능에서도 이렇게 접근할 수 있구나라는 걸 느꼈습니다.
• 하지만 MemberRepository의 save()가 호출되고 바로 트랜잭션이 종료되기 때문에 영속성 컨텍스트에 머무는 시간과 DB에 반영되는 시간차가 크지 않다고 생각해서 저도 수민이의 의견에 동의하는 부분입니다.

2. @Transactional(readOnly=true)를 PostService의 getCommentList 메서드에서 빼줬어요에 대한 의견

• getCommentList가 특정 게시글을 조회하는 getEachPost에서만 호출되기 때문에 트랜잭션의 중첩을 제거한 부분은 납득이 됐습니다.
• 다만, getCommentList가 단독으로 사용될 경우에는 readOnly 옵션이 없이 트랜잭션이 시작될 것 같은데 코드를 수정할 때, 이 부분도 고려를 하셨는지 궁금합니다. getCommentList가 단독으로 사용될 경우는 존재하지 않는 것인지 만약, 사용될 경우가 존재한다면 다시 readOnly 옵션을 붙여야 하는건지에 대해서도 논의가 이루어졌다면 PR에 해당 내용이 같이 담겨져 있으면 더 좋을 것 같습니다.

3. 하나 논의할 사항을 미리 말해 두자면 dto에 studyId 와 studyGroupId가 섞여서 쓰이는데요에 대한 의견

• 현재 패키지가 studygroup이기 때문에 studyGroupId로 명칭을 통일하는게 좋을 것 같습니다.

4. url의 groups를 studygroups라고 하기에는 가독성이 너무 떨어지는 것 같아서에 대한 의견

• groups를 studygroups로 바꿨을 때, 가독성이 떨어진다고 느낀 부분이 이름이 길어져서인건가요? study-groups라고 하면 REST API 네이밍 규칙을 준수하면서 가독성이 떨어지는 느낌을 줄일 수 있지 않을까요.

[Choi-JJunho]

1. saveAndFlush에 대해

수민이랑 태현이 의견과 동일하게 회원가입의 Tx가 그렇게 길거같지 않아서 굳이? 싶어요~

2. getCommentList에서 Tx(readOnly=true) 제거

코드를 보니 getEachPost에서 위 메소드를 재사용하고있네요
별도로 사용되지 않는 메소드라면 private으로 선언하는건 어떨까요?
또한 getEachPost라는 메소드 명이 SRP를 잘 지키고있는지도 확인해보면 좋을 것 같아요
메소드 명에서는 Post를 가져온다 인것 같은데 내부적으로 List를 가져오는게 뭔가 애매한 느낌이 들어요

4. 하나 논의할 사항을 미리 말해 두자면 dto에 studyId 와 studyGroupId가 섞여서 쓰이는데요, 이걸 studyGroupId으로 통일하는게 좋을 것 같다는 의견이 나왔어요.

대찬성입니다 ㅋㅋㅋ
dto 통일해주시죠

5. url의 groups를 studygroups라고 하기에는 가독성이 너무 떨어지는 것 같아서 이거는 groups로 남겨두는게 어떤가요?

동감합니다.