[Team - 06][BE] 에어비앤비 프로젝트 3주차 1회차 PR #237

leejohy-0223 · 2022-06-08T06:45:09Z

안녕하세요 Dion! 매번 저희 팀의 리뷰를 담당해 주셔서 감사의 인사를 전합니다!!

1. 구현 사항

이번에는 API를 구분하여 각각 구현해보았고, 추가된 부분은 다음과 같습니다.

Wish API 구현
Reservation API 구현

2. 구현 예정

리팩토링
나머지 Validation 적용
테스트 코드 작성

3. 특이사항

진행하면서 ObjectMapper의 Snake Case를 yml을 통해 전역으로 설정해줬습니다. 그러다보니 API 호출시에도 Snake Case가 적용되어 정상적으로 request를 처리하지 못하는 문제가 발생하였습니다. 이 부분을 Bean 생성으로 적용해보아도 동일한 문제가 발생하여 해결중에 있습니다.

요약)

Snake Case 를 yml로 전역 설정 -> 다른곳 까지 적용되서 문제됨
Bean으로 만들어 필요한 곳에서 가져다 쓰도록 변경 -> 동일하게 다른 곳 까지 적용됨

4. 궁금한 점

지난번 답변주신 부분
지적해주신 로그가 (OAuth Server 이름에 대한 정보를 주긴 하지만) 로직이 정상적으로 동작하는 것에 큰 의미를 주지 않는다는 것은 알게 되었습니다. 디버깅을 위한 정보를 주라는 말씀이 예외 발생 시 catch를 통해 error log를 출력하는 것처럼 의미있는 로그를 남기라는 말씀이실까요?

- User와 House의 관계는 Wish 이지, WishList 관계가 아니다.

- 사용자의 id와 집의 id 를 받아 찜목록에 추가한다.

- 사용자의 email을 인터셉터로부터 받아 이를 통해 찜 목록을 반환한다.

- 사용자의 id 와 wish의 id를 받아서 해당 wish 를 삭제한다.

- 파싱을 Bearer 로 하고 있었는데, bearer 로 해야한다.

- MySQL truncated error로 인한 longitude, latitude 자리 변경

- 환경 변수 yml로 변경 - Component Scan 및 DI 적용

ksundong

안녕하세요 샤인 & 루시드, 리뷰어 dion 입니다.
변경량이 2천줄에 육박하네요. 읽는데 시간이 꽤 걸렸던 것 같습니다.
변경사항이 많은만큼 리뷰드린 내용도 많아졌습니다.

3번에서 중요한 점이 있죠. 우리가 스프링을 좀 더 공부를 해야하는 이유입니다.
만약에 너무 막히시면 DM이나 코멘트 주세요.

답변

좋은 로그는 어떤 것일지, 나쁜 로그는 어떤 것일지 한 번 생각해보시기 바랍니다.
catch할 때 로그를 남기는 것은 좋은 로그일까요?
실제로 로그를 봤을 때, 전달하고자 하는것이 쉽게 전달되는지 체크해보시기 바랍니다.

고생하셨습니다. 테스트 코드도 열심히 짜주셨네요 👍
마지막까지 지치지 않고 프로젝트 수행해주세요!

ksundong · 2022-06-08T15:53:01Z

BE/scripts/run_new_was.sh

 USER_NAME=$(env | grep username | cut -c 10-14)
 PASSWORD=$(env | grep password | cut -c 10-17)
 DATASOURCE=$(env | grep datasource | cut -c 12-97)
+GITHUB_CLIENT_SECRET=$(env | grep GITHUB_CLIENT_SECRET | cut -c 22-61)
+GITHUB_CLIENT_ID=$(env | grep GITHUB_CLIENT_ID | cut -c 18-37)
+KAKAO_CLIENT_ID=$(env | grep KAKAO_CLIENT_ID | cut -c 17-48)


이렇게 환경변수를 계속 추가하는 방식은 별로 좋지 못한 것 같네요. 계속 설정값이 추가될 때 마다 이렇게 해주실건가요?

환경변수를 한번에 불러올 수 있는 방법이 따로 있는것 이군요?

그 혹시 죄송한데 힌트를 주실수 있을까요?
쉘스크립트 자체도 아직 어색하여서.... 이부분은 뭐랄까 "고민" 자체가 힘드네요.

Java, Spring은 어느정도 익숙은 하니까 말씀해주시는 부분을 고민하고 수정하는데...
쉘 스크립트 자체는 아직 미숙한 부분이 많아서.... 뭘 추가해야할지 모르겠습니다 ....

힌트를 주신다면 감사하겠습니다!

쉘 스크립트가 아닌 스프링 설정 파일에서 관리해주는 방법이 있지 않을까요?

그 server-url이나, redirect-url 같은 부분들은 스프링 설정파일로 관리하는데,
password 같은 부분은 Linux 환경변수 자체에 등록하여 사용하고 있습니다.

password: ${password} 같이 설정파일에 지정후

리눅스 환경변수 로 설정

스크립트 상에서 실행할때 환경변수의 비밀번호를 찾아와서 커맨드로 추가
위와 같은 과정으로 진행되는것이 아닌가요?

설정 파일은 다음과 같이 사용중 입니다만...

spring: profiles: active: local datasource: url: ${datasource} driver-class-name: com.mysql.cj.jdbc.Driver username: ${username} password: ${password} jwt: token: validate-time: 1800 token-secret: "Shine-Lucid" kakao: token-server-uri: "https://kauth.kakao.com/oauth/token" oauth-server-uri: "https://kapi.kakao.com/v2/user/me" client-id: ${KAKAO_CLIENT_ID} redirect-uri: "http://3.38.93.166/api/login/oauth/kakao/callback" github: token-server-uri: "https://github.com/login/oauth/access_token" oauth-server-uri: "https://api.github.com/user" client-id: ${GITHUB_CLIENT_ID} client-secret: ${GITHUB_CLIENT_SECRET}

https://stackoverflow.com/questions/35531661/using-env-variable-in-spring-boots-application-properties
이렇게 시도해보시겠어요?

ksundong · 2022-06-08T15:59:17Z

BE/src/main/java/com/airbnb/api/houses/dto/GraphResponse.java

+import java.util.List;
+import java.util.Objects;
+
+public class GraphResponse {


무슨 Graph 인지 특정되는게 좋지않을까요?

HousePriceGraph와 같은 이름으로 특정화하겠습니다!

ksundong · 2022-06-08T16:02:15Z

BE/src/main/java/com/airbnb/api/houses/dto/ReservationInformationRequest.java

+    @JsonDeserialize(using = LocalDateTimeDeserializer.class)
+    @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyyMMddHHmm")
+    private LocalDateTime startDateTime;
+
+    @JsonDeserialize(using = LocalDateTimeDeserializer.class)
+    @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyyMMddHHmm")
+    private LocalDateTime endDateTime;


가급적이면 다음부터는 ISO8601 형식을 지키는 형태로 디자인해주세요.

좋은 내용 배워갑니다!! 이런 부분들 저희 혼자 공부할때는 못느끼는 부분들인데 배워갑니다!

네! 변경하도록 하겠습니다.

ksundong · 2022-06-08T16:06:14Z

BE/src/main/java/com/airbnb/api/houses/HouseController.java

+    @GetMapping("/reservation")
+    public ReservationsResponse ListReservation(@RequestAttribute("userEmail") String userEmail) {
+        return houseService.showReservations(userEmail);
+    }


해당 코드는 왜 이런 url을 선택하셨는지 궁금하네요.

유저의 예약리스트를 조회하는 API입니다.
예약 시 API는 POST : /api/houses/{houseId}/reservation으로 수행했기 때문에 조회도 자연스럽게 houses에 속하도록 작성하였습니다.
조회 시에는 사실 User 정보 기반으로 조회되기 때문에 /api/users/reservation으로 하는게 낫다는 생각도 듭니다.
이런 방식이 맞을까요? 아니면 api naming에 문제가 있을까요?

앗 좀 더 꼼꼼히 보니 메서드 네이밍 컨벤션도 문제가 있었군요.

제가 생각하는걸 좀 더 말씀드릴게요.
말씀하신대로, 후자의 의견을 저는 더 선호하는데요. 우리가 houses/reservations 라고 디자인하면 이건 일단 어색하지만, houses 의 예약 목록을 보여줄 것이라는 생각이 들 것 같아요.
그래서 자연스럽지 않다는 생각이 들었습니다.

ksundong · 2022-06-08T16:06:42Z

BE/src/main/java/com/airbnb/api/houses/HouseController.java

+    public AccommodationCostResponse calculateFee(@PathVariable Long id,
+                                                  @RequestParam @DateTimeFormat(pattern = "yyyyMMddHHmm") LocalDateTime startDateTime,
+                                                  @RequestParam @DateTimeFormat(pattern = "yyyyMMddHHmm") LocalDateTime endDateTime) {
+        log.info("[time] {}, {}", startDateTime, endDateTime);


info 로그를 찍을만큼의 가치가 있었나요?

데이터포맷 입력 테스트용으로 작성하였는데 불필요하니 삭제하도록 하겠습니다!

ksundong · 2022-06-08T16:37:22Z

BE/src/test/java/com/airbnb/service/HouseServiceTest.java

+        ReservationInformationRequest request = new ReservationInformationRequest(
+            LocalDateTime.now(), LocalDateTime.now(), 1, 10000);
+
+        Reservation reservation = new Reservation(request.getFee(), request.getNumberOfGuests(),
+            request.getStartDateTime(), request.getEndDateTime(), null, house);
+
+        given(houseRepository.findById(anyLong()))
+            .willReturn(Optional.of(house));
+
+        given(userRepository.findUserByEmail(anyString()))
+            .willReturn(Optional.of(user));
+
+        given(reservationRepository.save(any(Reservation.class)))
+            .willReturn(reservation);


왜 모든 로직을 다 개행으로 분리해주셨나요?

가독성을 위해 분리해주는 편입니다!

가급적이면 연관성 있는 라인을 묶어주는 습관을 들여주세요.
빈 개행도 의미의 단위여서 가독성을 떨어뜨리는 효과를 줄 수 있습니다.

네 알겠슴니다!

ksundong · 2022-06-08T16:37:55Z

BE/src/test/java/com/airbnb/service/HouseServiceTest.java

+        Assertions.assertThat(result.getHouseName()).isEqualTo("house1");
+        verify(houseRepository, times(1)).findById(anyLong());
+        verify(userRepository, times(1)).findUserByEmail(anyString());
+        verify(reservationRepository, times(1)).save(any(Reservation.class));


이렇게 밖에 검증이 되지 않나요?

@Transactional public ReservationResponse reserveHouse(Long id, ReservationInformationRequest request, String userEmail) { House house = houseRepository.findById(id) .orElseThrow(() -> new IllegalStateException("찾을 수 없는 숙소 정보입니다.")); User host = userRepository.findUserByEmail(userEmail) .orElseThrow(() -> new IllegalStateException("찾을 수 없는 유저입니다.")); Reservation reservation = new Reservation( request.getFee(), request.getNumberOfGuests(), request.getStartDateTime(), request.getEndDateTime(), host, house); Reservation save = reservationRepository.save(reservation); return new ReservationResponse(save); }

테스트 대상 메서드를 살펴보면 결국 return되는 결과(ReservationResponse)는 Mocking되는 ReservationRepository의 save 메서드 반환 결과에 영향을 받는다고 생각되었습니다.

통합 테스트를 통해 Repository에서 받아오는 결과는 의미가 있지만, Mocking 하는 객체에서 반환되는 결과는 제가 결과를 테스트 코드 상에서 만들었다고(?) 생각했기에 큰 의미가 없으므로 result의 모든 것을 검증할 필요는 없다고 생각되었습니다.

다만 내부 로직 상 외부 메서드를 호출하는 부분(findById, findUserByEmail, save)은 예외 발생 시 호출되지 않을 수 있기 때문에 호출 여부를 검증해야한다고 생각하였습니다.

any 로 검증해주시는게 정말 맞을까요?
호출만되면 OK인 코드였나요?

생각해보니 Repository save 중 발생하는 예외도 고려해야겠네요..!

ksundong · 2022-06-08T16:39:05Z

BE/src/test/java/com/airbnb/wish/integrate/WishControllerTest.java

+    @DisplayName("token이 없는 회원이 wish list에 숙소를 추가하면 에러 메시지를 반환한다.")
+    public void create_wish_list_error_test() throws Exception {


무슨 에러 메시지를 반환하길 기대하나요?

IllegalStateException 예외 를 던지면 이를 catch하도록 테스트를 수정하였습니다.
지금 보니 예외를 발생시킨후, 예외가 발생하는지 "검증"을 안하고 있었습니다!

ksundong · 2022-06-08T16:39:34Z

BE/src/test/java/com/airbnb/wish/integrate/WishControllerTest.java

+    @DisplayName("가입한 회원은 위시 리스트 요청시 리스트를 반환한다.")
+    public void get_wish_list_test() throws Exception {


테스트에 대한 의미가 전달이 잘 안됩니다.

음 혹시 조금더 알려주실 수 있을까요?
사실 저부분을 작성한 저 스스로는 의미가 전달이 된다고 생각해서 저렇게 작성한 것이라 ㅠ,ㅠ
3자의 입장에서 어떤 부분이 불명확한지 알려주신다면 감사하겠습니다!

가입한 회원이 왜 위시리스트 요청시에 리스트를 반환해야하나요?
뭔가 중간 과정은 건너뛰었다고 생각이 되지 않으실까요?

아.... "서버에서" 리스트를 반환해야 할것 같습니다.
어디서 반환을 해야하는지에 대한 부분이 빠져있었습니다!

음 제가 말씀드린 부분은 가입한 회원이 위시 리스트 요청시에 리스트를 반환한다는게 아니라
어떤 행동을 했을 경우에 리스트가 반환되지 않을까요? 직접 데이터를 밀어넣고 테스트하는 것 보단 행위 중심으로 테스트하시면 좋을 것 같아요.

ksundong · 2022-06-08T16:40:32Z

BE/src/test/java/com/airbnb/wish/integrate/WishControllerTest.java

+                .andExpect(jsonPath("valid").value("true"))
+                .andExpect(jsonPath("message").value("OK"))
+                .andExpect(header().string(HttpHeaders.CONTENT_TYPE, MediaTypes.HAL_JSON_VALUE))


검증하고자 하는것이 응답만인가요? 아니면 응답 코드인가요?

상태코드 같은 경우 바로 위의 status().isOk() 를 통해 검증하고,
해당 message 부분같은 경우 응답 메시지를 검증하고 싶었습니다! 상태코드는 따로 위에서 검증하게 됩니다.

넵넵 저라면 삭제하는 행위를 더 검증하고 싶었을 것 같네요. 😊

zbqmgldjfh and others added 30 commits June 8, 2022 12:41

[chore]: Validation 의존성 추가

b7b20d7

[refactor]: User와 House의 관계 이름 수정

26abdce

- User와 House의 관계는 Wish 이지, WishList 관계가 아니다.

[feat]: WishController.createWish 메서드 구현

36de0c3

- 사용자의 id와 집의 id 를 받아 찜목록에 추가한다.

[feat]: WishController.getWishList 메서드 구현

5fe83fd

- 사용자의 email을 인터셉터로부터 받아 이를 통해 찜 목록을 반환한다.

[feat]: WishController.deleteWish 메서드 구현

53d6a50

- 사용자의 id 와 wish의 id를 받아서 해당 wish 를 삭제한다.

[fix]: AuthenticationInterceptor.preHandle 에서 파싱할때 bearer 로 변경

9430239

- 파싱을 Bearer 로 하고 있었는데, bearer 로 해야한다.

[fix]: Entity의 순환참조 문제 해결

b2b5179

[test]: Wish Service Test에 존재하지 않는 Wish를 조회하는 경우에 대한 테스트 추가

c725617

[test]: Wish 통합 테스트 구현완료

8aaea3c

[fix]: data.sql Point 입력 변경

cd2a325

- MySQL truncated error로 인한 longitude, latitude 자리 변경

[refactor]: HouseController로 예약 API 이동

7ed81a3

[feat]: 예약 진행 API 구현

ea8a941

[feat]: 예약 리스트 조회 API 구현

87a33c3

[refactor]: Collection에 대해 Fetch join(outer) 적용

949d7d4

[feat]: 예약 상세 확인 API 구현

e89635f

[refactor]: 조회 시 자신의 예약만 볼 수 있도록 JPQL에 and 조건 추가

09ea1be

[feat]: soft delete 방식으로 예약 취소 API 구현

91435b6

[test]: HouserService Test 작성

2f89fee

[chore]: 설정파일 충돌 해결

f3ed426

[fix]: GeometryUtils.toPoint 위도, 경도 순서 수정

74631c2

[fix]: HouseController 수정

298337f

[refactor]: Wish DTO에 Equals HashCode 구현

45e7975

[feat]: Test용 data.sql에 wish 부분 추가

0a9a8e1

[docs]: 4차 PR Todo list 작성

10e291b

[refactor]: callback url 수정

2211a69

[refactor]: token parsing 불필요한 optional 제거

9473b1f

[refactor]: log 구체화

8d1ea83

[refactor]: DI로 변경

61d791a

- 환경 변수 yml로 변경 - Component Scan 및 DI 적용

[refactor]: GitHubOAuthServer.getUserProfile 에서 NoSuchElement로 예외로 변경

95b7a79

[refactor]: ObjectMapper의 strategy를 yml에서 설정하도록 변경

252eb17

zbqmgldjfh and others added 4 commits June 8, 2022 12:41

[refactor]: WishResponse self link 수정

9d10a70

[refactor]: WishController 반환 Dto 수정

df2e7ea

[feat]: run_new_was.sh 에 Oauth 정보 추가

038c8b4

[fix]: parsing을 위한 dateTimeformat 적용

2015bd7

leejohy-0223 requested a review from ksundong June 8, 2022 06:45

leejohy-0223 assigned zbqmgldjfh and leejohy-0223 Jun 8, 2022

leejohy-0223 added the review-BE Improvements or additions to documentation label Jun 8, 2022

ksundong approved these changes Jun 8, 2022

View reviewed changes

ksundong merged commit 562971d into codesquad-members-2022:team-06 Jun 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Team - 06][BE] 에어비앤비 프로젝트 3주차 1회차 PR #237

[Team - 06][BE] 에어비앤비 프로젝트 3주차 1회차 PR #237

leejohy-0223 commented Jun 8, 2022 •

edited by zbqmgldjfh

ksundong left a comment •

edited

ksundong Jun 8, 2022

zbqmgldjfh Jun 9, 2022 •

edited

ksundong Jun 10, 2022

zbqmgldjfh Jun 11, 2022

ksundong Jun 11, 2022

ksundong Jun 8, 2022

leejohy-0223 Jun 9, 2022

ksundong Jun 8, 2022

zbqmgldjfh Jun 9, 2022

leejohy-0223 Jun 9, 2022

ksundong Jun 8, 2022

leejohy-0223 Jun 9, 2022

ksundong Jun 10, 2022

ksundong Jun 8, 2022

leejohy-0223 Jun 9, 2022

ksundong Jun 8, 2022

leejohy-0223 Jun 9, 2022

ksundong Jun 10, 2022

leejohy-0223 Jun 11, 2022

ksundong Jun 8, 2022

leejohy-0223 Jun 9, 2022 •

edited

ksundong Jun 10, 2022

leejohy-0223 Jun 11, 2022

ksundong Jun 8, 2022

zbqmgldjfh Jun 9, 2022

ksundong Jun 8, 2022

zbqmgldjfh Jun 9, 2022

ksundong Jun 10, 2022

zbqmgldjfh Jun 11, 2022

ksundong Jun 11, 2022

ksundong Jun 8, 2022

zbqmgldjfh Jun 9, 2022

ksundong Jun 10, 2022

		@DisplayName("token이 없는 회원이 wish list에 숙소를 추가하면 에러 메시지를 반환한다.")
		public void create_wish_list_error_test() throws Exception {

		@DisplayName("가입한 회원은 위시 리스트 요청시 리스트를 반환한다.")
		public void get_wish_list_test() throws Exception {

[Team - 06][BE] 에어비앤비 프로젝트 3주차 1회차 PR #237

[Team - 06][BE] 에어비앤비 프로젝트 3주차 1회차 PR #237

Conversation

leejohy-0223 commented Jun 8, 2022 • edited by zbqmgldjfh

1. 구현 사항

2. 구현 예정

3. 특이사항

4. 궁금한 점

ksundong left a comment • edited

Choose a reason for hiding this comment

답변

Choose a reason for hiding this comment

zbqmgldjfh Jun 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejohy-0223 Jun 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejohy-0223 commented Jun 8, 2022 •

edited by zbqmgldjfh

ksundong left a comment •

edited

zbqmgldjfh Jun 9, 2022 •

edited

leejohy-0223 Jun 9, 2022 •

edited