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

세 번째 리뷰 요청 드립니다. #40

Open
wants to merge 55 commits into
base: review
Choose a base branch
from

Conversation

MuseopKim
Copy link
Contributor

@MuseopKim MuseopKim commented Feb 24, 2021

안녕하세요.
되도록 작은 단위로 진행 하기 위해 바로 이렇게 리뷰 요청을 드리게 됐습니다.
매번 도움이 되는 의견을 제시 해주셔서 감사합니다 🙇🏻‍♂️
커밋이 좀 많아 보이는데, 마지막에 자동 배포 스크립트에서 조금 헤맸던 탓에 많이 늘어나게 되었습니다.
개발 내용은 많지 않습니다... 😅 코드량에 부담 없으시면 좋겠네요.

구현 내용

  • Validation
  • 예외 처리
  • RestDocs 적용
  • 자동 배포 적용

구현하지 못한 것

  • 아직 두 번째 리뷰 해주신 내용이 반영 되어있지 않습니다. (두 번째 리뷰 요청 드리자마자 개발을 진행 했던 것들 입니다.)
  • 외부 API 테스트에 대한 피드백을 아직 반영하지 못했습니다. 테스트를 어떻게 수정하는게 좋을지 아직 고민 하고 있습니다.
  • RestDocs를 테스트에 적용은 했는데 아직 asciidoc 문서는 정리를 하지 않았습니다.
    협업이면 서두르겠지만, 1인 개발 중이라 계속 코드에만 적용 하다가 마지막에 정리하게 되지 않을까 싶습니다.

API 문서

요청, 응답 JSON 확인 하실 수 있도록 링크를 첨부 합니다. (지난번과 변동 내용은 없습니다.)

링크


부담이 되지 않으시는 선에서 천천히 봐주셔도 괜찮습니다.
감사합니다!

ErrorResponse
- `@Valid` 에러 응답 양식을 공통화 하기 위해 BindingResult를 포함하는 생성자 추가

ErrorCode
- 에러 코드 및 메시지 한 곳에 정의

ValueError
- `@Valid` 로 발생하는 에러 응답 중 필요한 두 필드 값만 추출하여 정의
EntityNotFoundException
- 도메인 404 NOT_FOUND 상위 예외 클래스 정의

AccountNotFoundException,
LetterNotFoundException,
SongNotFoundException
- EntityNotFoundException을 상속 받는 각 엔티티 예외 클래스 정의

BusinessException
- 비즈니스 요구사항과 맞지 않을 경우 발생하는 최상위 예외 클래스 정의
`@RestControllerAdvice` 전역 예외 처리 핸들러
NewLetterRequest,
SongRequest
- Validation 적용

build.gradle
- validator 의존성 추가
NewLetterRequest가 유효하지 않을 경우 에러 발생 테스트 추가
Error 응답 관련 객체들 response 패키지로 모두 이동

ErrorCode,
ErrorResponse,
ValueError
각 Validation 필드별로 메시지 추가
30자 제한으로 설정

GlobalExceptionHandler
- ConstraintViolationException 예외 핸들러 추가

SongControllerTest
- 관련 예외 테스트 추가
ParsingFailedException,
SearchResultParsingException
- CheckedException 을 RuntimeException으로 구체화 하기 위해 별도의 예외 정의

ErrorCode
- SEARCH_RESULT_ERROR

GlobalExceptionHandler
- handleParsingFailedException() 추가

SearchApiService,
LastFmApiService,
ManiaDbApiService,
SongService
- 예외를 내부에서 직접 처리 하게 됨에 따라 throws 삭제

JsonTranslatorTest,
XmlTranslatorTest
- 관련 예외 테스트 추가
- business, parsing 으로 구분
- 각각의 상위 클래스는 패키지 상단에 위치하도록 수정
처리 되지 않은 에러를 공통 에러 객체로 응답하기 위해 GlobalExceptionHandler에 추가

Method Not Allowed,
Internal Server Error
RestDocs 설정 추가 이후 기본 테스트 완료
ResponseFields,
LetterControllerTest
- fieldWithPath를 통해 작성 할 경우 가독성을 해치는 문제가 있고 다루기 어려워 항목을 별도로 관리 하도록 분리 하였다.
 ResponseFields
 - common()에서 data가 응답에 따라 다양한 타입을 지원하기 때문에 ARRAY -> VARIES 변경 (추후 common을 별도의 snippets로 관리 하는 것을 고려)
ResponseFields와 마찬가지로 상세 명세를 별도의 클래스로 분리
ResponseFields와 마찬가지로 본 테스트 코드의 가독성을 위해 RequestFields 클래스 별도로 추가
ResponseFields
- Error field descriptor 추가
- PathParameters,RequestParameters 를 포괄하도록 이름 변경
- letterIdParameters() -> letterId() : 클래스명과 중복되므로 변경
RestDocs 관련 설정 공통화
Controller 테스트 코드의 가독성을 위해 Fields, Parameters 별도로 분리
ResponseFields
- 별도의 BindingResult를 갖지 않는 에러 값도 함께 처리 하기 위해 errors[] 하위 필드에 optional 조건 추가
dto 라는 패키지가 별도로 필요하지 않다고 판단, dto 디렉토리를 삭제하고 하단에 있던 request/response 로 바로 접근 할 수 있도록 배치
Properties에 의존하여 발생하는 테스트 실패로 인하여 배포 환경에서 해당 테스트 제거
Chore: 배포 전 테스트에서 외부 API 테스트를 실행 하지 않도록 수정
AWS S3 버킷 업로드 관련 설정 추가
Chore: 자동 배포를 위한 Travis 설정 값 추가
내용 : Skipping a deployment with the s3 provider because this branch is not permitted: main
`on:
       all_branches: true` 옵션 추가
Chore: AWS S3 - Branch is not permitted error 해결
Chore: AWS CodeDeploy 설정 추가
Chore: Notification 설정 오류 수정
(+) application.yml - 배포 과정에서 command line arguments를 사용 하므로 default profiles 를 dev로 수정
Chore: 배포 스크립트 추가
Copy link
Collaborator

@kses1010 kses1010 left a comment

Choose a reason for hiding this comment

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

고생많으셨습니다. 코드를 읽으면서 정말 테스트코드를 정성스레 짰구나라는걸 느꼈네요. 이전 프로젝트에서도 테스트코드를 정성스레 짤걸 그랬는데 후회도 오는군요.

이번 리뷰에서 느낀건 예외처리 AOP, validation, CI/CD이군요. CI/CD 파이프라인 구축하는건 정말 거지같지만 한번 만들면 여러모로 도움이 되니 나중에 문서화하는것도 추천합니다.

Comment on lines +42 to +63
- provider: codedeploy
access_key_id: $AWS_ACCESS_KEY
secret_access_key: $AWS_SECRET_KEY

bucket: songrequest-backend-build
key: songrequest-backend.zip

bundle_type: zip
application: songrequest-backend

deployment_group: songrequest-backend-group

region: ap-northeast-2
wait-until-deployed: true

on:
all_branches: true

notifications:
email:
recipients:
- [email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

travis CI 이군요. 저도 써봤지만 Github Actions보다 느리고 디버깅하는데 불편해서 전 쓰다가 바꿨습니다.
현재는 Github Actions + AWS CodeBuild를 병합해서 사용하고 있는데 CI툴 중 travis ci를 사용하신 이유가 궁금합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

travis CI를 사용한 기술적인 이유는 없었습니다ㅎㅎ
CI / CD를 다양한 툴로 경험 해보고 싶어서 백엔드는 travis CI, 프론트는 Github Actions로 배포 환경을 구성 했습니다.

travis CI를 써보고 느낀 것은... 디버깅 과정에서 불편함은 없었고, 다만 말씀 하신 것처럼 조금 느린 것이 약간 불편 했습니다.
프론트와 빌드 과정이 다르기 때문에 절대적인 비교는 불가 하지만 저도 Github Actions가 약간 더 빠른 느낌이었네요.

files:
- source: /
destination: /home/ubuntu/app/songrequest/zip/
overwrite: yes
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 아셔야할건 만약 blue/green 배포를 하실 경우 overwrite가 먹질 않습니다. 그래서 beforeInstall 쉘스크립트를 작성하셔서 파일 삭제하는 스크립트를 짜셔야합니다. 이 내용은 언젠가 제 블로그에 적을 예정이니 궁금하시면 보세용

Copy link
Contributor Author

Choose a reason for hiding this comment

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

만약 개인 프로젝트로 blue / green 배포까지 하게 된다면 매우 큰 성과겠네요ㅎㅎ
써니 블로그에 잘 보고 있습니다. 😄 좋은 팁 감사합니다.

Comment on lines +14 to +18
hooks:
ApplicationStart:
- location: deploy.sh
timeout: 60
runas: ubuntu
Copy link
Collaborator

Choose a reason for hiding this comment

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

만약 본인이 blue/green 무중단 배포를 하실경우 여기서 BeforeInstall 쉘스크립트 넣으셔서 이전 빌드된파일 삭제하시는 쉘스크립트를 넣어야 정상적으로 작동되니 꼭 기억하세요

Comment on lines +49 to +52
implementation 'javax.validation:validation-api:2.0.1.Final'
implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-xml:2.11.1'
implementation 'org.hibernate.validator:hibernate-validator:6.1.7.Final'
implementation 'org.apache.commons:commons-lang3:3.11'
Copy link
Collaborator

Choose a reason for hiding this comment

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

이것도 좋지만 DTO <-> Entity역할을 하는 기존의 modelmapper도 좋지만 Mapstruct도 훨씬 성능도 좋습니다. 실무에서도 사용하는 중입니다.
https://huisam.tistory.com/entry/mapStruct

@@ -0,0 +1,38 @@
#!/bin/bash

REPOSITORY=/home/ubuntu/app/songrequest
Copy link
Collaborator

Choose a reason for hiding this comment

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

home에 그대로 배포하는 것보다 폴더를 생성해서 관리하는 게 좋습니다. 잘 적용하셨네요

Comment on lines +16 to +25
@NotEmpty(message = NOT_EMPTY_MESSAGE)
@Size(min = TITLE_MIN, max = TITLE_MAX, message = TITLE_MESSAGE)
private String title;

@NotEmpty(message = NOT_EMPTY_MESSAGE)
@Size(min = ARTIST_MIN, max = ARTIST_MAX, message = ARTIST_MESSAGE)
private String artist;

@Size(max = IMAGE_MAX, message = IMAGE_MESSAGE)
private String imageUrl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

validation 괜찮네요

Comment on lines +3 to +25
public interface ValidationCondition {

// Common
String NOT_EMPTY_MESSAGE = "필수 입력 정보 입니다.";

// Artist
String ARTIST_MESSAGE = "아티스트는 30자 미만 입니다.";
int ARTIST_MAX = 30;
int ARTIST_MIN = 1;

// Title
String TITLE_MESSAGE = "제목은 30자 미만 입니다.";
int TITLE_MAX = 30;
int TITLE_MIN = 1;

// Image URL
String IMAGE_MESSAGE = "유효한 이미지 정보가 아닙니다.";
int IMAGE_MAX = 100;

// Song Story
String SONG_STORY_MESSAGE = "사연은 500자 미만이어야 합니다.";
int SONG_STORY_MAX = 500;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

interface에 그대로 value값을 지정하면 좋지않습니다. 이펙티브 자바에서 본 거같은데 내용이 정확하게 기억은 안나지만 이렇게 하지말라고 강하게 경고했던 기억이 나네요.

그리고 내용상 interface보단 enum에 어울리는데 enum은 어떤가요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

p3: 저도 enum이나 다른 class으로 관리하시는 게 좋다고 생각되네요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

인터페이스가 static 상수와 추상 메서드만 접근 가능해서 이렇게 적용 해봤었는데, 다시 보니 Enum으로 관리 하는게 더 적합 할거 같습니다.
(+) 언급하신 이펙티브 자바의 내용은 (3rd edition 기준) Item 22 에 있네요!

Comment on lines +21 to +26
@ExceptionHandler(ConstraintViolationException.class)
protected ResponseEntity<ErrorResponse> handleConstraintViolationException(ConstraintViolationException exception) {
log.error("handleConstraintViolationException", exception);
ErrorResponse errorResponse = ErrorResponse.from(ErrorCode.INVALID_INPUT_VALUE);
return new ResponseEntity<>(errorResponse, HttpStatus.BAD_REQUEST);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 코드가 validation 예외처리로 알고 있습니다. 작동이 잘되나요? 한번 테스트코드에서 확인해보겠습니다.

@@ -28,7 +27,7 @@ public Song findSongByRequest(SongRequest songRequest) {
});
}

public SearchApiResponse searchSong(String artist, String title) throws JsonProcessingException {
public SearchApiResponse searchSong(String artist, String title) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

controllerAdvice에서 처리하니 본 로직에서 사라졌네요. 좋습니당

Comment on lines +160 to +168
resultActions.andDo(print())
.andExpect(status().isBadRequest())
.andExpect(jsonPath("statusCode").value(ErrorCode.INVALID_INPUT_VALUE.getStatusCode()))
.andExpect(jsonPath("message").value(ErrorCode.INVALID_INPUT_VALUE.getMessage()))
.andExpect(jsonPath("errors").isNotEmpty())
.andDo(document("error-create-letter",
responseFields(ResponseFields.error())
))
;
Copy link
Collaborator

Choose a reason for hiding this comment

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

와 validation 테스트코드가 있네요 아주 멋지십니다.

Copy link
Collaborator

@102092 102092 left a comment

Choose a reason for hiding this comment

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

이번 기능도 고생하셨습니다!

여러 모로 탄탄한 프로젝트를 구현하시기 위해 노력하시는 듯 하네요!
특히 validation, 자동배포는 인상 깊었습니다.
완성도를 높혀주고, 안정성을 만들어갈 수 있도록 도와주는 부분이라 생각합니다.

궁금증과 추천해드리는 기능이 각각 하나씩 있습니다!

우선 Restdocs를 사용하시기로 한 이유가 좀 궁금합니다.
이 프로젝트에서 작성한 api를 관리하시기 위한 목적이라면, 현재 하고 계신 노션을 통해도 충분하지 않을까 생각이 듭니다.
1인 프로젝트이기도 하고, 추후 이 부분을 강조하실 생각이 아니시라면..? 굳이 할 필요가 있을까 하는 생각이 드네요.

그리고 예외 처리에 로그를 어떻게 처리하실 것인지도 한번 생각해보셨으면 좋을 것 같습니다.
지금은 단순히 서버 로그창에만 남도록 하고 계신 것 같습니다.
좀 더 나아가자면, 이 로그를 로컬에 저장한다면 어떤 레벨의 로그를 저장할지, 어떻게 저장할지, 시간 단위는 어떻게 해야될지 등을 생각해보셨으면 좋겠네요
nginx를 사용하신다면... 여기도 로그가 있으니 이 부분도 한번 알아보셔도 좋을 것 같구요..

이 부분은 실제 배포 후에, 한번 생각할 todo 정도로 남겨놓으시면 좋을듯 합니다!

고생하셨어요! 감사합니다 👍


// Common
INVALID_INPUT_VALUE(400, "입력한 요청 값이 올바르지 않습니다. 다시 요청 해주세요."),
SEARCH_RESULT_ERROR(500, "검색 결과를 가져오지 못했습니다. 다시 요청 해주세요."),
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4: 검색 결과를 가져오지 못한 것이, 서버 에러에 해당할까요?
어떤 이유에서 5xx대 에러라고 생각하셨는지 여쭤봐도 될까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 에러는 JSON, XML을 파싱 하는 과정에서 발생하는 에러인데요.
외부 API 객체를 확인하지 못하고 그냥 진행 시킨 경우에 해당한다 생각하여 이는 서버 에러에 포함 된다고 생각을 했었습니다.
확신을 가지고 정의 하지는 못했었는데, 혹시 이 경우에 더 적합한 상태 코드가 있다면 의견을 주실 수 있을까요?

Comment on lines +1 to +38
#!/bin/bash

REPOSITORY=/home/ubuntu/app/songrequest
PROJECT_NAME=song-request

echo "> Build 파일 복사"

cp $REPOSITORY/zip/*.jar $REPOSITORY/

echo "> 현재 구동 중인 애플리케이션 pid 확인"

CURRENT_PID=$(pgrep -fl song-request | grep jar | awk '{print $1}')

echo "현재 구동 중인 애플리케이션 pid : $CURRENT_PID"

if [ -z "$CURRENT_PID" ]; then
echo "> 현재 구동 중인 애플리케이션이 없으므로 종료하지 않습니다."
else
echo "> kill -15 $CURRENT_PID"
kill -15 $CURRENT_PID
sleep 5
fi

echo "> 새 애플리케이션 배포"

JAR_NAME=$(ls -tr $REPOSITORY/*.jar | tail -n 1)

echo "> JAR Name : $JAR_NAME"

echo "> $JAR_NAME에 실행 권한 추가"

chmod +x $JAR_NAME

echo "> $JAR_NAME 실행"

nohup java -jar \
-Dspring.profiles.active=prod \
$JAR_NAME > $REPOSITORY/nohup.out 2>&1 &
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4 : 자동 배포 스크립트 군요 👍
간단한 팁이지만.. 여기서 사용하고 있는 tail, grep등등 에 대한 기본적인 이해 정도는 있으시면 좋을듯 합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

말씀 하신 것처럼 EC2와 Nginx를 설정하고 배포 환경을 구성하는 과정에서 리눅스 명령어가 많이 부족하다는 것을 느꼈습니다.
그래서 별도로 조금씩 공부를 하고 있어요ㅎㅎ 조언 해주셔서 감사합니다! 🙂

Comment on lines +3 to +25
public interface ValidationCondition {

// Common
String NOT_EMPTY_MESSAGE = "필수 입력 정보 입니다.";

// Artist
String ARTIST_MESSAGE = "아티스트는 30자 미만 입니다.";
int ARTIST_MAX = 30;
int ARTIST_MIN = 1;

// Title
String TITLE_MESSAGE = "제목은 30자 미만 입니다.";
int TITLE_MAX = 30;
int TITLE_MIN = 1;

// Image URL
String IMAGE_MESSAGE = "유효한 이미지 정보가 아닙니다.";
int IMAGE_MAX = 100;

// Song Story
String SONG_STORY_MESSAGE = "사연은 500자 미만이어야 합니다.";
int SONG_STORY_MAX = 500;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3: 저도 enum이나 다른 class으로 관리하시는 게 좋다고 생각되네요!

Comment on lines +11 to +12
public class ValueError {
private final String value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4 : 한칸 엔터 해주시면 좋을듯 합니다!

Comment on lines +11 to +13
url: ${DB_URL}
username: ${DB_USERNAME}
password: ${DB_PASSWORD}
Copy link
Collaborator

Choose a reason for hiding this comment

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

중요 정보를 이렇게 숨기고 있는 점은 💯 !!입니다!

@MuseopKim
Copy link
Contributor Author

@kses1010 블로그와 이전 프로젝트에서의 경험이 CI / CD 관련해서 많은 도움이 되었습니다. 감사합니다 😄

이전 프로젝트가 미완결로 정체되어 있는거 같아 저도 많이 아쉬운데요... 현재 회사에서 적응 하느라 바쁘실테고, 저도 실무를 시작하면 다음 개선할 때 훨씬 더 좋은 코드가 나오지 않을까 싶어요.

중간중간 실무를 하시며 느꼈던 점들을 공유 해주셔서 또 새로운 관점을 생각 해볼 수 있었습니다.
이번 리뷰도 정말 감사 드립니다 🙇🏻‍♂️

@MuseopKim
Copy link
Contributor Author

MuseopKim commented Mar 8, 2021

@102092 질문 주신 내용에 대해 답변을 드리자면... RestDocs 를 적용한 이유는, 사용 목적보다는 학습과 경험이 목적 입니다.
말씀 해주신 것처럼 이번 프로젝트의 지향점이 "실제 사용해도 흠결이 없을만큼 견고하고 탄탄한 개발을 해보자" 였는데요...ㅎㅎ
(실제 그렇게 되고 있는지는 모르겠지만...🤔)

그래서 기존에 사용하지 않았던 (그러나 해보고 싶었던) 기술들을 조금씩 추가 해보는 과정에서 RestDocs를 적용하게 되었습니다.
RestDocs 뿐만 아니라 앞으로 추가하게 될 수도 있는 QueryDSL도 사실 이 프로젝트에 꼭 필요한 기술들은 아닐 수도 있는데요.
테스트 코드에 많은 비중을 두고 이런 것들 모두가 학습과 경험이 일차 목적이 될거 같네요.

개인적으로 이전에 했던 프로젝트들 모두 기능 구현에 집중 하느라 그 밖에 신경쓰지 못했던 것들이 너무 많았다는 아쉬움에... 이번에는 시간은 조금 더 걸려도 견고하게 만들어 보고 싶었습니다.
사실 해보고 싶은 프로젝트가 두가지 정도 더 있었는데 기능이 복잡하지 않은 주제를 가장 먼저 선택한 이유도 여기에 있었던거 같아요ㅎㅎ

나중에 실무를 하면서도 개인 프로젝트를 할 여유가 있을지 모르겠지만, 새로운 시도를 해볼 수 있다는게 사이드 프로젝트의 장점이 아닐까 싶어요.

로그는 안그래도 걸리는 점이 많았는데 이번 기회에 학습을 해보고 방법에 대해 생각 해보겠습니다.
마침 Nginx를 사용하고 있는데 이것도 좋은 공부 주제가 될거 같네요. 좋은 의견 주셔서 감사합니다!

이번 요청에도 리뷰를 해주셔서 정말 감사합니다 🙂
조만간 뵙겠습니다... 🙇🏻‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants