-
Notifications
You must be signed in to change notification settings - Fork 2
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
[BE] refactor: question 타입과 답변 내용 일치 검증 및 매핑 로직 개선 #958
Conversation
Test Results160 tests +6 157 ✅ +6 5s ⏱️ -1s Results for commit e0be296. ± Comparison against base commit 11a2b05. This pull request removes 20 and adds 26 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
백로그 쳐내기 좋네요 👍🏻 코멘트 확인해주세용!
backend/src/test/java/reviewme/review/service/mapper/AnswerMapperFactoryTest.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/review/service/mapper/AnswerMapper.java
Outdated
Show resolved
Hide resolved
} | ||
return new CheckboxAnswer(answerRequest.questionId(), answerRequest.selectedOptionIds()); | ||
protected boolean isAnswerMatchesQuestionType(ReviewAnswerRequest request) { | ||
return request.selectedOptionIds() != null && request.text() == null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
느슨하게 selectedOptionIds
만 검증하는 걸 제안합니다. 아래 두 가지 이유인데요,
- 서버 입장에서는 둘 다 오면 타입 따라서 검증하고 그 타입만 가져다 쓰면 된다고 생각해요. 설령 그게 잘못된 요청이더라도, 서버 입장에서는 잘못된? 악용된? 요청을 가릴 필요가 없다고 생각해요 (형식이 맞다면). 아래 두 로직 중 a가 조금 더 자연스럽다고 생각해요. 타입이 늘어나면 늘어나는 것에 대해서 모두 대응할 수 있지도 않고요.
a. (객관식 타입이네? -> 객관식 Id 가져다 써야지~)
b. (객관식 타입이네? -> 주관식 없나 볼까? -> 객관식 Id 가져다 써야지~)
(만약 타입이 더 많아지면: 객관식 확인 -> 객관식이 아닌 타입 전부 널 확인 -> 객관식 가져다쓰기)
request
에text
가 존재한다는 걸CheckboxAnswerMapper
가 몰랐으면 좋겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request에 text가 존재한다는 걸 CheckboxAnswerMapper가 몰랐으면 좋겠습니다.
이 말 때문에 마음이 이쪽으로 많이 기우는데요,
그런데 한가지 걸리는게 있어요🥲
사실 저도 이 부분에 대해서 '너무 빡빡하게 검증을 안하는게 좋지 않을까?' 라는 생각이 들기도 했었어요.
그럼에도 이렇게 구현한 이유는, 전에 말했던 '소프트웨어의 성질'을 따라야 한다고 생각했기 때문이에요.
대강 맞더라도, 서버와 약속한 형식과 100% 일치하는게 아니라면 예외를 응답해야 한다고 생각해요.
그래서 여기에 대해서는 다른 팀원들의 생각도 들어보고 싶어요!
@skylar1220 @Kimprodp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 100% 일치하지 않는 것에 대한 책임은 누가 지나요?
- 100% 일치하지 않았을 경우 피해는 누가 보나요? 정말 피해가 맞나요?
- 해당 경우를 고려하지 않았을 때의 사이드이펙트와 구현하는 데에 드는 자원을 생각했을 때 트레이드오프할 만큼 100% 일치하는 것이 중요한가요?
저는 1: 온전히 서버가 검증해야 함, 2: 서버가 봄, 단 그건 피해라고 보기 어려움, 3: 일치하는 것이 그렇게 중요하지 않음 (위 댓글의 a
에 해당함)
이라서 좀 강하게 의견 내고싶긴 하네요 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
약간 뭐랄까.. '이 선을 넘어도 될까?'의 느낌이라고 해야 할까요?
빡빡한 검증을 하지 않아도 되는건가? 이래도 되나? 싶은거죠 ㅎㅎㅠ
죄책감? 그런게 느껴진다고 해야할까요? 근거가 없는 생각일수도 있는데;; 무튼 그런 느낌이 들어요~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
검증을 한 것이죠..? 무시하는 것도 하나의 방법이지 않을까요? 😁😁😁😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
계속해서 고민을 해봤습니다🧘
제 안의 선을 넘기까지..! 시간이 걸렸습니다.
결론적으로,
-
이 검증이 해결할 수 있는 문제가 없다는 점
다른 검증들과 비교했을 때, 이 검증은 역할이 없습니다. -
이 검증이 기동성(≒빠르게 코드를 만들고 부수기)을 떨어트리고 있다는 점
크게 중요하지 않은 것 때문에 코드와 유지보수가 복잡해지고 있다는 생각이 들었습니다.
때문에 아루의 의견에 마음이 기울었습니다.
이 부분을 검증하지 않게 코드를 바꾼다면 많은것이 바뀔 것입니다.
아래의 템플릿 논의도 해결됩니다🤝
리뷰하기 쉽도록 커밋 잘 나눠서 이 PR에 붙여보겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
노파심에 하는 이야긴데, 산초가 방어적으로 생각하는 게 설계 관점에서 크게 도움이 된 적이 여럿 있어요. 데이터 정합성은 특히나 외래 키를 들지 않는 경우에는 반드시 필요한 부분이기도 하고요.
이 부분에서 제가 조금 강하게 의견을 표출했던 부분은 1. 정합성과 관련없었고 2. 우리가 알아서 하면 되는 일이라서였습니다. 산초가 이전에 이야기해줬던 의견들을 모두 부정하는 것이 아니여요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래와 같은 포인트들에 공감해 동의합니다~
- 빠르게 코드를 만들고 부수기를 떨어트리고 있다.
- (브라우저가 아닌 곳에서) 형식을 맞춰서 보낸 요청을 저장하나, 형식이 잘못된 요청을 저장하나 그저 '저장되는 데이터가 늘어난다' 이외에는 서버 측면에서 부담이 가는 사이드 이펙트가 없다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 지금 저 검증이 의미가 없는 건 맞습니다. 그래도 우린 API 형식을 정했고 그에 대한 검증은 해야 한다고 생각해요.
다만, 지금 이렇게 고민이 벌어지고 있는 이유. 즉, 이 검증을 하는 것에 타협안을 둔다는 것이 요청이 검증을 힘들게 하진 않나? 고민해볼 필요가 있다고 생각합니다. 위 내용처럼 질문 타입이 늘어날수록 검증도 추가되어야 해요. 그럼 검증이 꼭 필요한가? 보다는 요청이 확장성 있게 고려되지 않은 것이 아닐까? 라는 생각이 듭니다.
잘못된 요청에 피해가 없는 것처럼 보이더라도 원하지 않는 요청에 데이터가 저장이 될 수 있어요. 서버는 몰라도 API 요청자는 저장되지 않길 바라는 요청이 서버에 저장될 수 있을 수 있다고 생각합니다.
최근 다시 공유된 개발자 머피 법칙에도 비슷한 경우가 있어요. 우리의 API를 호출하는 사람은 클라이언트 뿐만이 아닙니다. 누군가가 하나만 보내도 되는건지, 둘 다 보내야 되는건지 헷갈려서 요청을 보낼 수도 있고 그 과정에서 여러 요청을 보낼 수 있습니다.
그래서 결론,
제 의견은 현재 단계에서는 검증을 추가하는 것이 맞고(원래 있던 검증이고 굳이 제외할 필요는 없음), 추후 질문 타입이 증가할수록 검증도 늘어나는 부분에 대한 고민은 요청에 대한 형식이나 방법을 다시 생각하면 좋겠어요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우린 API 형식을 정했고 그에 대한 검증은 해야 한다고 생각해요.
api 명세는 우리의 합의으로 만들어지는 것입니다. 그래서 “중요하지 않은 검증에 코드가 길어지는 것을 막기 위해 유연한 요청을 받겠다” 고 합의를 한다면 그게 또 다른 api 명세가 되는 것이라 생각해요. 그러니까 우리의 지금 상황은 api 명세 위반이 아니라 api 명세 변경에 가깝다 생각합니다.
그리고 다른 개발자들이 실수로 api 호출하는 것은 우리 팀 정도의 규모에서는 일어나지 않을 것이므로, 고려하지 않아도 될것이라 생각합니다👍 사실 이건 트레이드 오프 영역이긴 한데, 우리의 현재 상황에 더 이득이 되는 것을 생각했을 때 빡빡한 검증을 하지 않음으로써 얻는 간편함이 더 크다 생각해요.
backend/src/main/java/reviewme/review/service/mapper/CheckboxAnswerMapper.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/review/service/mapper/AnswerMapper.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/review/service/mapper/AnswerMapper.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/review/service/mapper/AnswerMapper.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/review/service/mapper/TextAnswerMapper.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/reviewme/review/service/mapper/TextAnswerMapperTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
산초 고생많았어요 추상화를 최대한 활용한게 너무 좋네요!!
전체적으로 이해를 하고(이해는 잘 됐어요!) 우선 코멘트로 얘기 진행중이어서 제 의견이 필요한 부분만 남겼습니다! 금요일 저녁에 꼼꼼히 보고 마저 남길게요😂
backend/src/main/java/reviewme/review/service/mapper/CheckboxAnswerMapper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제대로 확인하고 몇 가지 수정 제안해요~
backend/src/main/java/reviewme/review/service/mapper/AnswerMapper.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/review/service/dto/request/ReviewAnswerRequest.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/review/service/mapper/CheckboxAnswerMapper.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/review/service/mapper/TextAnswerMapper.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/reviewme/review/service/mapper/CheckboxAnswerMapperTest.java
Show resolved
Hide resolved
- 팀의 컨벤션을 위해 & 유지보수 편한 코드를 위해 접근 제한자를 public & private 으로 통일
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생했어요 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
산초의 이 리팩토링 흐름이 좋았어요.
타입 분기를 answerMapper 속으로 이동 > 템플릿 패턴 적용 > 고정된 패턴 없이 각 mapper들이 알아서 매핑을 구현하게 자유도를 높임 > 유연하게 확장 가능
직접 옮겨보고, 문제점을 느끼고 개선하는 자세를 저도 배워야겠어요..!
null 반환해서 상위에서 filter하는 부분이 이해가 안가서 그 부분 하나만 보려고 rc해요!
backend/src/main/java/reviewme/review/service/mapper/CheckboxAnswerMapper.java
Outdated
Show resolved
Hide resolved
} | ||
return new CheckboxAnswer(answerRequest.questionId(), answerRequest.selectedOptionIds()); | ||
protected boolean isAnswerMatchesQuestionType(ReviewAnswerRequest request) { | ||
return request.selectedOptionIds() != null && request.text() == null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래와 같은 포인트들에 공감해 동의합니다~
- 빠르게 코드를 만들고 부수기를 떨어트리고 있다.
- (브라우저가 아닌 곳에서) 형식을 맞춰서 보낸 요청을 저장하나, 형식이 잘못된 요청을 저장하나 그저 '저장되는 데이터가 늘어난다' 이외에는 서버 측면에서 부담이 가는 사이드 이펙트가 없다.
backend/src/test/java/reviewme/review/service/mapper/CheckboxAnswerMapperTest.java
Show resolved
Hide resolved
backend/src/main/java/reviewme/review/service/mapper/CheckboxAnswerMapper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null 반환에 대해서 답변 남긴거 확인했어요!
(얘 참 찝찝하네요😂 나중에 '왜 이렇게 null을 반환하게 했지?'하고 생각할 게 상상되는 부분..)
고생 많았어요 산쵸!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하나만 확인해주세용~
그리고 본문에서
이유1) 오버라이드를 할 이유가 없는 함수를 final 로 두려고
이유2) 외부에서 호출해서 사용할 필요가 없는 함수를 protected 로 두려고
이 내용은 어디에 해당된느건가요..? 이해를 못했습니다
@@ -16,12 +15,9 @@ public boolean supports(QuestionType questionType) { | |||
|
|||
@Override | |||
public TextAnswer mapToAnswer(ReviewAnswerRequest answerRequest) { | |||
if (!answerRequest.hasTextAnswer()) { | |||
if (answerRequest.hasNoText()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것만 검증하면 질문의 타입이 TEXT 이고, text와 selectedOptionIds 둘 다 있는 요청일때 TextAnswer 생성되지 않나요? (반대의 경우도 동일)
둘 다 들어온 경우에는 등록을 하면 안되지 않을까 합니다. (정상적이지 않은 요청)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 확인해주세용~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 본문에서
이유1) 오버라이드를 할 이유가 없는 함수를 final 로 두려고
이유2) 외부에서 호출해서 사용할 필요가 없는 함수를 protected 로 두려고
이 내용은 어디에 해당된느건가요..?
처음에는 AnswerMapper에서 '질문의 타입에 해당하는 답변이 오지 않았을 때의 검증'을 잘 하기 위해서 템플릿 메서드 패턴을 사용했었습니다. 그리고 이때 final 함수가 필요했습니다. 그런데 리뷰를 반영하며 이 검증을 하지 않게 되면서 삭제했습니다😊
관련 커밋 : 4b2bbd1
그리고 protected 키워드도 리뷰를 반영하며, 사용하지 않는 쪽으로 바뀌었습니다!
관련 코멘트 : #958 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#958 (comment)
관련해서 의견 남겼어요~ 다음 의견에 대한 결론이 제 결론과 다르더라도 어프룹 할게요. 확인만 해주세요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가로 산초에게 질문을 하나만 더할게요, (단순 궁금)
"AnswerMapper 를 인터페이스에서 추상 클래스로 바꿨는데요, 적절하다고 생각하시나요?" 이 부분에서
이유 3은 저도 공감합니다.
그리고 이유 1과 이유 2는 사라진 상태인데요. 그럼 이유 3만으로 추상 클래스가 적절하다고 생각되어 바뀌는 부분인가요? 타입만 변경되고 다른건 변경되는 게 없어보여요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 이 경우 인터페이스로 하던, 추상클래스로 하던 문제가 되진 않아요.
그런데 저는 의미상 추상클래스에 더 가깝다 생각했습니다.
인터페이스는 다중 구현이 가능하므로 '기능'이나 '명세'에 초점이 맞춰져있다면 (HAS-A)
추상클래스는 단일 상속만 가능하므로 '소속'을 의미한다고 생각합니다. (IS-A)
그런데 지금 다시 생각하니, AnswerMapper 가 중복 코드를 줄이는 구현을 제공하지 않는 이상
추상클래스로 할 이유도 없는 것 같긴 합니다😅
생각을 바꾼 계기는, 자바의 컬렉션을 생각해봤을 때 ArrayList 가
인터페이스 List 를 구현함으로써 기능에 대한 명세를 지키고,
추상 클래스 AbstractList 를 상속함으로써 중복되는 구체 메서드를 재사용했다는 것을 떠올렸어요.
그러므로 다시 인터페이스로 바꾸도록 하겠습니다😊
- abstract class -> interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생했습니다 초~ 꼼꼼하게 확인해주어 고맙습니다
- abstract class -> interface 변환 하면서 extends -> implements 변경되지 않은 부분 변경
🖼️ 배경
사실 처음에는 이전 PR을 적용할 생각이었습니다.
그런데 하다보니 다른 방법이 더 좋겠다 싶어서 방향을 완전 바꿨습니다.. ㅎㅎ
이 이슈가 생겨난 이유에 대해서 생각해봤습니다.
처음 의도는 ‘형식 검증을 비지니스 로직에서 분리하자’는 것이었습니다.
그런데 구현을 하고 보니 dto 에서 이를 검증하더라도,
‘질문의 유형에 해당하지 않은 응답 포함되었는지’에 대한 검증이 추가로 필요한 것을 깨달았습니다.
그런데 이 둘은 중복된 검증인 듯 해서, 후자의 검증만 남겨도 되겠다고 생각했습니다.
그래서 이제부터는 ‘어떻게 변경에 영향을 받지 않으면서 검증을 할 수 있을까?’가 고민되었습니다.
그래서 ✨아루가 만들어준 추상화를 활용하는 방법✨으로 구현해봤습니다.
🔥 어떻게 해결했나요 ?
'request에 의도하지 않은 값이 온 상황'을 알 방법이 없기 때문입니다.
(이후에는 request가 아니라 domain을 보고 검증하므로)
📝 어떤 부분에 집중해서 리뷰해야 할까요?
AnswerMapper 를 인터페이스에서 추상 클래스로 바꿨는데요, 적절하다고 생각하시나요?
이유1) 오버라이드를 할 이유가 없는 함수를 final 로 두려고
이유2) 외부에서 호출해서 사용할 필요가 없는 함수를 protected 로 두려고
이유3) AnswerMapper 와 TextAnswerMapper / CheckboxAnswerMapper 는 논리적으로
HAS
보다IS
관계라 생각해서템플릿 패턴을 적용하게 되었는데, (이게 목적은 아니었고 결과적으로 이게 되었어요) 읽기에 괜찮나요?
📚 참고 자료, 할 말
하하 어떤가요?