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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Nov 22, 2024
Merged

[BE] refactor: question 타입과 답변 내용 일치 검증 및 매핑 로직 개선 #958

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
526784e
refactor: 타입에 대한 분기를 answerMapper 안에서 처리할 수 있도록
nayonsoso Nov 11, 2024
60fece1
refactor: 사용하지 않는 코드 제거
nayonsoso Nov 11, 2024
8b2fe15
refactor: 하나의 테스트에 한가지를 테스트하도록
nayonsoso Nov 11, 2024
473a10c
refactor: 추상화를 더욱 활용하도록
nayonsoso Nov 11, 2024
867508b
style: 개행 통일
nayonsoso Nov 11, 2024
3947423
refactor: getQuestionType 없애고 바로 supports를 사용하도록
nayonsoso Nov 11, 2024
4b2bbd1
refactor: 다른 유형의 답변이 동시에 입력되는지에 대한 검증 삭제
nayonsoso Nov 14, 2024
1507446
refactor: 함수 이름 변경
nayonsoso Nov 15, 2024
ff0850d
style: 띄어쓰기 추가
nayonsoso Nov 15, 2024
34c26cd
refactor: 접근제한자 변경
nayonsoso Nov 15, 2024
5b909c9
refactor: 접근제한자 변경
nayonsoso Nov 15, 2024
8deefc3
refactor: AnswerMapper 변경
nayonsoso Nov 22, 2024
e0be296
fix: 컴파일 에러 해결
nayonsoso Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions backend/src/main/java/reviewme/review/service/mapper/AnswerMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,9 @@ public final Answer mapToAnswer(ReviewAnswerRequest answerRequest) {

protected abstract Answer doMap(ReviewAnswerRequest answerRequest);
nayonsoso marked this conversation as resolved.
Show resolved Hide resolved

protected abstract QuestionType getQuestionType();
protected abstract boolean isAnswerMatchesQuestionType(ReviewAnswerRequest request);
nayonsoso marked this conversation as resolved.
Show resolved Hide resolved

private boolean isAnswerMatchesQuestionType(ReviewAnswerRequest request) {
if (getQuestionType() == QuestionType.CHECKBOX) {
return request.selectedOptionIds() != null && request.text() == null;
}
return request.text() != null && request.selectedOptionIds() == null;
}
protected abstract boolean isAnswerEmpty(ReviewAnswerRequest request);

private boolean isAnswerEmpty(ReviewAnswerRequest request) {
if (getQuestionType() == QuestionType.CHECKBOX) {
return request.selectedOptionIds() != null && request.selectedOptionIds().isEmpty();
}
return request.text() != null && request.text().isEmpty();
}
protected abstract QuestionType getQuestionType();
}
10 changes: 10 additions & 0 deletions backend/src/main/java/reviewme/review/service/mapper/CheckboxAnswerMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ protected CheckboxAnswer doMap(ReviewAnswerRequest answerRequest) {
return new CheckboxAnswer(answerRequest.questionId(), answerRequest.selectedOptionIds());
}

@Override
protected boolean isAnswerMatchesQuestionType(ReviewAnswerRequest request) {
return request.selectedOptionIds() != null && request.text() == null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

느슨하게 selectedOptionIds만 검증하는 걸 제안합니다. 아래 두 가지 이유인데요,

  1. 서버 입장에서는 둘 다 오면 타입 따라서 검증하고 그 타입만 가져다 쓰면 된다고 생각해요. 설령 그게 잘못된 요청이더라도, 서버 입장에서는 잘못된? 악용된? 요청을 가릴 필요가 없다고 생각해요 (형식이 맞다면). 아래 두 로직 중 a가 조금 더 자연스럽다고 생각해요. 타입이 늘어나면 늘어나는 것에 대해서 모두 대응할 수 있지도 않고요.

a. (객관식 타입이네? -> 객관식 Id 가져다 써야지~)
b. (객관식 타입이네? -> 주관식 없나 볼까? -> 객관식 Id 가져다 써야지~)
(만약 타입이 더 많아지면: 객관식 확인 -> 객관식이 아닌 타입 전부 널 확인 -> 객관식 가져다쓰기)

  1. request에 text가 존재한다는 걸 CheckboxAnswerMapper가 몰랐으면 좋겠습니다.

Copy link
Contributor Author

@nayonsoso nayonsoso Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request에 text가 존재한다는 걸 CheckboxAnswerMapper가 몰랐으면 좋겠습니다.

이 말 때문에 마음이 이쪽으로 많이 기우는데요,
그런데 한가지 걸리는게 있어요🥲

사실 저도 이 부분에 대해서 '너무 빡빡하게 검증을 안하는게 좋지 않을까?' 라는 생각이 들기도 했었어요.
그럼에도 이렇게 구현한 이유는, 전에 말했던 '소프트웨어의 성질'을 따라야 한다고 생각했기 때문이에요.
대강 맞더라도, 서버와 약속한 형식과 100% 일치하는게 아니라면 예외를 응답해야 한다고 생각해요.

image
왜냐하면 그것이.. api 명세이기 때문입니다..

그래서 여기에 대해서는 다른 팀원들의 생각도 들어보고 싶어요!
@skylar1220 @Kimprodp

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 100% 일치하지 않는 것에 대한 책임은 누가 지나요?
  2. 100% 일치하지 않았을 경우 피해는 누가 보나요? 정말 피해가 맞나요?
  3. 해당 경우를 고려하지 않았을 때의 사이드이펙트와 구현하는 데에 드는 자원을 생각했을 때 트레이드오프할 만큼 100% 일치하는 것이 중요한가요?

저는 1: 온전히 서버가 검증해야 함, 2: 서버가 봄, 단 그건 피해라고 보기 어려움, 3: 일치하는 것이 그렇게 중요하지 않음 (위 댓글의 a에 해당함)

이라서 좀 강하게 의견 내고싶긴 하네요 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

약간 뭐랄까.. '이 선을 넘어도 될까?'의 느낌이라고 해야 할까요?
빡빡한 검증을 하지 않아도 되는건가? 이래도 되나? 싶은거죠 ㅎㅎㅠ
죄책감? 그런게 느껴진다고 해야할까요? 근거가 없는 생각일수도 있는데;; 무튼 그런 느낌이 들어요~~

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

검증을 한 것이죠..? 무시하는 것도 하나의 방법이지 않을까요? 😁😁😁😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

계속해서 고민을 해봤습니다🧘
제 안의 선을 넘기까지..! 시간이 걸렸습니다.

결론적으로,

  • 이 검증이 해결할 수 있는 문제가 없다는 점
    다른 검증들과 비교했을 때, 이 검증은 역할이 없습니다.

  • 이 검증이 기동성(≒빠르게 코드를 만들고 부수기)을 떨어트리고 있다는 점
    크게 중요하지 않은 것 때문에 코드와 유지보수가 복잡해지고 있다는 생각이 들었습니다.

때문에 아루의 의견에 마음이 기울었습니다.

이 부분을 검증하지 않게 코드를 바꾼다면 많은것이 바뀔 것입니다.
아래의 템플릿 논의도 해결됩니다🤝
리뷰하기 쉽도록 커밋 잘 나눠서 이 PR에 붙여보겠습니다.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

노파심에 하는 이야긴데, 산초가 방어적으로 생각하는 게 설계 관점에서 크게 도움이 된 적이 여럿 있어요. 데이터 정합성은 특히나 외래 키를 들지 않는 경우에는 반드시 필요한 부분이기도 하고요.

이 부분에서 제가 조금 강하게 의견을 표출했던 부분은 1. 정합성과 관련없었고 2. 우리가 알아서 하면 되는 일이라서였습니다. 산초가 이전에 이야기해줬던 의견들을 모두 부정하는 것이 아니여요~

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래와 같은 포인트들에 공감해 동의합니다~

  1. 빠르게 코드를 만들고 부수기를 떨어트리고 있다.
  2. (브라우저가 아닌 곳에서) 형식을 맞춰서 보낸 요청을 저장하나, 형식이 잘못된 요청을 저장하나 그저 '저장되는 데이터가 늘어난다' 이외에는 서버 측면에서 부담이 가는 사이드 이펙트가 없다.

Copy link
Contributor

@Kimprodp Kimprodp Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 지금 저 검증이 의미가 없는 건 맞습니다. 그래도 우린 API 형식을 정했고 그에 대한 검증은 해야 한다고 생각해요.
다만, 지금 이렇게 고민이 벌어지고 있는 이유. 즉, 이 검증을 하는 것에 타협안을 둔다는 것이 요청이 검증을 힘들게 하진 않나? 고민해볼 필요가 있다고 생각합니다. 위 내용처럼 질문 타입이 늘어날수록 검증도 추가되어야 해요. 그럼 검증이 꼭 필요한가? 보다는 요청이 확장성 있게 고려되지 않은 것이 아닐까? 라는 생각이 듭니다.

잘못된 요청에 피해가 없는 것처럼 보이더라도 원하지 않는 요청에 데이터가 저장이 될 수 있어요. 서버는 몰라도 API 요청자는 저장되지 않길 바라는 요청이 서버에 저장될 수 있을 수 있다고 생각합니다.

최근 다시 공유된 개발자 머피 법칙에도 비슷한 경우가 있어요. 우리의 API를 호출하는 사람은 클라이언트 뿐만이 아닙니다. 누군가가 하나만 보내도 되는건지, 둘 다 보내야 되는건지 헷갈려서 요청을 보낼 수도 있고 그 과정에서 여러 요청을 보낼 수 있습니다.
image

그래서 결론,
제 의견은 현재 단계에서는 검증을 추가하는 것이 맞고(원래 있던 검증이고 굳이 제외할 필요는 없음), 추후 질문 타입이 증가할수록 검증도 늘어나는 부분에 대한 고민은 요청에 대한 형식이나 방법을 다시 생각하면 좋겠어요.

Copy link
Contributor Author

@nayonsoso nayonsoso Nov 22, 2024 •
edited
Loading

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 호출하는 것은 우리 팀 정도의 규모에서는 일어나지 않을 것이므로, 고려하지 않아도 될것이라 생각합니다👍 사실 이건 트레이드 오프 영역이긴 한데, 우리의 현재 상황에 더 이득이 되는 것을 생각했을 때 빡빡한 검증을 하지 않음으로써 얻는 간편함이 더 크다 생각해요.

}

@Override
protected boolean isAnswerEmpty(ReviewAnswerRequest request) {
return request.selectedOptionIds() != null && request.selectedOptionIds().isEmpty();
nayonsoso marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
protected QuestionType getQuestionType() {
return QuestionType.CHECKBOX;
Expand Down
10 changes: 10 additions & 0 deletions backend/src/main/java/reviewme/review/service/mapper/TextAnswerMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ protected TextAnswer doMap(ReviewAnswerRequest answerRequest) {
return new TextAnswer(answerRequest.questionId(), answerRequest.text());
}

@Override
protected boolean isAnswerMatchesQuestionType(ReviewAnswerRequest request) {
return request.text() != null && request.selectedOptionIds() == null;
}

@Override
protected boolean isAnswerEmpty(ReviewAnswerRequest request) {
return request.text() != null && request.text().isEmpty();
}
donghoony marked this conversation as resolved.
Show resolved Hide resolved

@Override
protected QuestionType getQuestionType() {
return QuestionType.TEXT;
Expand Down
9 changes: 8 additions & 1 deletion backend/src/test/java/reviewme/review/service/mapper/AnswerMapperFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,18 @@ class AnswerMapperFactoryTest {
protected Answer doMap(ReviewAnswerRequest answerRequest) {
return null;
}

@Override
protected QuestionType getQuestionType() {
return QuestionType.CHECKBOX;
}
@Override
protected boolean isAnswerMatchesQuestionType(ReviewAnswerRequest request) {
return false;
}
@Override
protected boolean isAnswerEmpty(ReviewAnswerRequest request) {
nayonsoso marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
};

@Test
Expand Down