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

[Team-26][BE : Shine] Issue-Tracker 2주차 1회차 PR #124

Merged
merged 18 commits into from
Jun 23, 2022
Merged

[Team-26][BE : Shine] Issue-Tracker 2주차 1회차 PR #124

merged 18 commits into from
Jun 23, 2022

Conversation

zbqmgldjfh
Copy link
Member

@zbqmgldjfh zbqmgldjfh commented Jun 22, 2022

안녕하세요! Shine 입니다!

저의 리뷰를 담당해주시게 되어 감사하다는 말씀 전하고 싶습니다.

질문 1가지

1. API에서의 Redirction은?

Client 측에서 POST로 자원 생성 요청이 올 경우,

  1. Server에서 사용자가 요청한 resource 를 생성 한 후, 302 Found 와 함께 Location 응답을 줄지?
  2. 그냥 Server는 resource를 생성하고, 정상적으로 생성됬다는 201 응답만 보내면 될지?
    궁금합니다!
    예전에 타임리프를 직접 사용하던 시기에는 1번이 맞는것 같은데... API에서의 방식은 다를지 궁금합니다!!

@zbqmgldjfh zbqmgldjfh added the review-BE Improvements or additions to documentation label Jun 22, 2022
@zbqmgldjfh zbqmgldjfh requested a review from wheejuni June 22, 2022 07:56
@zbqmgldjfh zbqmgldjfh self-assigned this Jun 22, 2022
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다. 💯 본문에 남겨주신 질문은 별도 댓글로 답 드립니다. 잠시만요..

@@ -39,6 +39,8 @@ dependencies {
implementation 'io.jsonwebtoken:jjwt:0.9.1'
implementation 'io.netty:netty-resolver-dns-native-macos:4.1.68.Final:osx-aarch_64'
implementation 'com.github.gavlyukovskiy:p6spy-spring-boot-starter:1.6.2'
implementation group: 'org.springframework.boot', name: 'spring-boot-starter-actuator', version: '2.7.0'

Choose a reason for hiding this comment

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

👍

CURRENT_PORT=$(cat /home/ec2-user/service_url.inc | grep -Po '[0-9]+' | tail -1)
TARGET_PORT=0

echo "> Current port of running WAS is ${CURRENT_PORT}."

Choose a reason for hiding this comment

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

워우 좋네요. 👍 어떤 회사에서 많이 사용하는 스타일의 스크립트입니다. @honux77 그쵸? ㅋㅋ

echo "> Now Nginx proxies to ${TARGET_PORT}."

# Reload nginx
sudo service nginx reload

Choose a reason for hiding this comment

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

nginx를 RHEL이나 RPM, apt로 까는 것과 직접 바이너리 다운로드/빌드 후 사용하는 것의 차이점도 한번 학습해 보시길 권합니다.
보통 기업들은 후자의 방법을 선호하기는 합니다. 어떤 차이점에서 그런 선호 포인트(?) 가 발생할지 한번 탐구해 보시죠

Copy link
Member Author

Choose a reason for hiding this comment

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

apt-get을 통해 설치하는 방식은 NGINX 공식 repository를 통해 다운받기 때문에 안전하다고 합니다.
또한 페키지메니저를 통해 설치할 경우, Nginx를 사용하는 port를 방화벽에서 따로 열어줄 필요 없이 사용하는 포트들을 자동 등록하며, 로그로테이트 설정이 자동으로 되기 때문에 편하다는 장점이 있습니다.

반대로 직접 컴파일 해서 사용할경우 기본패키지에 추가되어있는 모듈 외의 추가적인 모듈을 사용하고 싶다면 소스파일을 추가하여 컴파일 할수 있다는 장점이 있다고 합니다.
기업들 같은 경우 자기 회사만의 특이사항을 만족시킬수 있어야 할탠데, 이런 면에서 장점이 있다고 생각합니다!!
https://extrememanual.net/9910

Comment on lines +54 to +60
public static Comment create(String comment, Issue issue, User user) {
return Comment.builder()
.description(comment)
.issue(issue)
.user(user)
.build();
}

Choose a reason for hiding this comment

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

빌더가 이미 있는데 살짝 조금은 투머치 인거 같기도 합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 메서드를 제거하였습니다!!

Comment on lines +74 to +76
public boolean isSameCommentId(Long commentId) {
return this.id == commentId;
}

Choose a reason for hiding this comment

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

.equals() 오버라이드로 안되겠습니까?

Copy link
Member Author

Choose a reason for hiding this comment

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

생각해보니 Long 도 객체니 .equals()로 변경하도록 하겠습니다!!

User findUser = userRepository.findUserByEmail(userEmail)
.orElseThrow(() -> new NotFoundException(ErrorCode.USER_NOT_FOUND));

findIssue.addComment(Comment.create(comment, findIssue, findUser)); // 변경감지로 저장?

Choose a reason for hiding this comment

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

저장될지 안될지 테스트코드로 실험해 보시죠.

Copy link
Member Author

Choose a reason for hiding this comment

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

넵!! 테스트 코드를 통하여 확인해보겠습니다!!

@@ -27,7 +27,7 @@ public class Issue extends BaseTimeEntity {
private String title;
private boolean isOpen;

@OneToMany(mappedBy = "issue")
@OneToMany(mappedBy = "issue", cascade = CascadeType.ALL)

Choose a reason for hiding this comment

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

여기에 뭔가 하나의 옵션을 더 주면, Comment 에 대한 서비스 클래스의 별도의 접근 없이도 댓글 추가/삭제를 할 수 있을 것 같습니다.
어떤 옵션일까요? o. o.... or... orp.... h... 여기까집니다. 검색해보세요.

Copy link
Member Author

Choose a reason for hiding this comment

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

orphanRemoval = true 옵션을 추가하여 부모와의 life-cycle을 동일하게 가져가겠습니다!!


public void deleteComment(Comment comment) {
this.comments.remove(comment);
comment.deleteIssue();

Choose a reason for hiding this comment

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

양쪽에서 모두 참조 관계를 끊는 디테일 좋습니다. 💯

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다!! 양뱡향으로 매핑을 해놨기에 양쪽에서 끊어야 데이터의 정합성이 어플리케이션에서 부합할거라 생각했습니다!!

@wheejuni
Copy link

@zbqmgldjfh 사실 어떤 방법이든 FE와 협의하면 된다고 생각합니다만 회사에서의 제 경험은 거의 항상 "201, 200 응답 후 FE가 다음에 어디로 갈지 알아서 결정" 이었긴 합니다.

API를 설계한 목적이 리디렉션이 아닌 자원 혹은 정보의 생성 이었기 때문에 302는 좀 어색한 면도 있다고 생각합니다. 그래서 저는 200, 201 어떤 걸 사용해야 할지는 모르겠으나 200번대 응답에 한 표 드립니ㅏㄷ.

@zbqmgldjfh zbqmgldjfh merged commit e46b768 into codesquad-members-2022:team-26 Jun 23, 2022
@zbqmgldjfh
Copy link
Member Author

우선 merge후, 수정 사항들은 내일 PR에 함께 반영하서 올리도록 하겠습니다!! 갑사합니다!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants