Skip to content
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package com.wayble.server.user.controller;

import com.wayble.server.common.config.security.jwt.JwtTokenProvider;
import com.wayble.server.common.exception.ApplicationException;
import com.wayble.server.common.response.CommonResponse;
import com.wayble.server.user.dto.UserPlaceListResponseDto;
import com.wayble.server.user.dto.UserPlaceRequestDto;
import com.wayble.server.user.exception.UserErrorCase;
import com.wayble.server.user.service.UserPlaceService;
Expand All @@ -12,12 +14,16 @@
import lombok.RequiredArgsConstructor;
import org.springframework.web.bind.annotation.*;

import java.util.List;

@RestController
@RequestMapping("/api/v1/users/{userId}/places")
@RequiredArgsConstructor
public class UserPlaceController {

private final UserPlaceService userPlaceService;
private final JwtTokenProvider jwtProvider;
Copy link
Member

Choose a reason for hiding this comment

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

밑에 말씀드린 부분을 적용하면 jwtProvider 주입도 자연스럽게 빠질 것 같습니다..!



@PostMapping
@Operation(summary = "유저 장소 저장", description = "유저가 웨이블존을 장소로 저장합니다.")
Expand All @@ -29,16 +35,47 @@ public class UserPlaceController {
public CommonResponse<String> saveUserPlace(
@PathVariable Long userId,
@RequestBody @Valid UserPlaceRequestDto request,

// TODO: 로그인 구현 후 Authorization 헤더 필수로 변경 필요
@RequestHeader(value = "Authorization", required = false) String authorizationHeader
@RequestHeader(value = "Authorization") String authorizationHeader
) {
// Path variable과 request body의 userId 일치 여부 확인
if (!userId.equals(request.userId())) {
throw new ApplicationException(UserErrorCase.INVALID_USER_ID);
String token = authorizationHeader.replace("Bearer ", "");
if (!jwtProvider.validateToken(token)) {
Copy link
Member

Choose a reason for hiding this comment

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

어짜피 요청 과정에서 jwt filter 과정을 통해 validateToken() 메서드가 자동으로 실행되는 것으로 알고 있습니다! 이 부분은 중복 검증인 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

JWT가 올바른지 직접 검증하는 핵심 로직이 컨트롤러와 서비스까지 있는게 해당 부분의 로직을 보게된다면 추적성 측면에서 좋다고 생각하였습니다. 그리고 인증 필터 이외에 컨트롤러에 검증 로직을 추가하면, 테스트, API에서도 예외를 잡을 수 있다고 생각하였고 토큰 검증은 워낙 중요하다고 생각해서 계속 검증 로직을 더 신경썻던거 같아요!

throw new ApplicationException(UserErrorCase.FORBIDDEN);
}
Long tokenUserId = jwtProvider.getUserId(token);
Copy link
Member

Choose a reason for hiding this comment

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

Long userId = (Long) SecurityContextHolder.getContext().getAuthentication().getPrincipal(); 와 같은 형식(문법이 이게 맞는지는 모르겠네요)으로 userId를 추출할 수 있는걸로 알고 있습니다!

토큰을 전달받아 userId를 추출하는 것보다 코드도 짧고 훨씬 간결할 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

이 부분은 SecurityContextHolder 에서 userId를 꺼내는 것이 간결하고, 유지보수에도 좋을것 같아요 감사합니다! 반영해서 리팩토링 해보겠습니다.


// Path variable과 request body의 userId 일치 여부 확인
if (!userId.equals(request.userId()) || !userId.equals(tokenUserId)) {
Copy link
Member

Choose a reason for hiding this comment

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

JWT에서 userId를 포함하고, pathVariable에도 userId가 포함되어 있기 때문에 request body에 userId가 꼭 필요할지 의문이에요!

Copy link
Member Author

Choose a reason for hiding this comment

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

이것도 말씀하신것처럼 PathVariable과 JWT 토큰에서 이미 userId를 체크하고 있는데 중복이라는 단점이 존재하는것 같아요. 리뷰 주신 대로 request body에선 userId를 빼고, path variable/JWT 기반으로만 검증하도록 수정하겠습니다!

throw new ApplicationException(UserErrorCase.FORBIDDEN);
}
userPlaceService.saveUserPlace(request);
return CommonResponse.success("장소가 저장되었습니다.");
}

@GetMapping
@Operation(
summary = "내가 저장한 장소 목록 조회",
description = "유저가 저장한 모든 장소 및 해당 웨이블존 정보를 조회합니다."
)
@ApiResponses({
@ApiResponse(responseCode = "200", description = "장소 목록 조회 성공"),
@ApiResponse(responseCode = "404", description = "해당 유저를 찾을 수 없음"),
@ApiResponse(responseCode = "403", description = "권한이 없습니다.")
})
public CommonResponse<List<UserPlaceListResponseDto>> getUserPlaces(
@PathVariable Long userId,
@RequestHeader("Authorization") String authorizationHeader
) {
String token = authorizationHeader.replace("Bearer ", "");
if (!jwtProvider.validateToken(token)) {
throw new ApplicationException(UserErrorCase.FORBIDDEN);
}
Long tokenUserId = jwtProvider.getUserId(token);

if (!userId.equals(tokenUserId)) {
throw new ApplicationException(UserErrorCase.FORBIDDEN);
}

List<UserPlaceListResponseDto> places = userPlaceService.getUserPlaces(userId);
return CommonResponse.success(places);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package com.wayble.server.user.dto;

import lombok.Builder;



@Builder
public record UserPlaceListResponseDto(
Long place_id,
String title,
WaybleZoneDto wayble_zone
) {
@Builder
public record WaybleZoneDto(
Long wayble_zone_id,
String name,
String category,
double rating,
String address,
String image_url
) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ public enum UserErrorCase implements ErrorCase {
PLACE_ALREADY_SAVED(400, 1003, "이미 저장한 장소입니다."),
INVALID_USER_ID(400, 1004, "요청 경로의 유저 ID와 바디의 유저 ID가 일치하지 않습니다."),
USER_ALREADY_EXISTS(400, 1005, "이미 존재하는 회원입니다."),
INVALID_CREDENTIALS(400, 1006, "아이디 혹은 비밀번호가 잘못되었습니다.");
INVALID_CREDENTIALS(400, 1006, "아이디 혹은 비밀번호가 잘못되었습니다."),
FORBIDDEN(403, 1007, "권한이 없습니다.");

private final Integer httpStatusCode;
private final Integer errorCode;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
import com.wayble.server.user.entity.UserPlaceWaybleZoneMapping;
import org.springframework.data.jpa.repository.JpaRepository;

import java.util.List;

public interface UserPlaceWaybleZoneMappingRepository extends JpaRepository<UserPlaceWaybleZoneMapping, Long> {
boolean existsByUserPlace_User_IdAndWaybleZone_Id(Long userId, Long zoneId);
List<UserPlaceWaybleZoneMapping> findAllByUserPlace_User_Id(Long userId);
}
35 changes: 35 additions & 0 deletions src/main/java/com/wayble/server/user/service/UserPlaceService.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@


import com.wayble.server.common.exception.ApplicationException;
import com.wayble.server.user.dto.UserPlaceListResponseDto;
import com.wayble.server.user.dto.UserPlaceRequestDto;
import com.wayble.server.user.entity.User;
import com.wayble.server.user.entity.UserPlace;
Expand All @@ -16,6 +17,8 @@
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;

import java.util.List;

@Service
@RequiredArgsConstructor
public class UserPlaceService {
Expand Down Expand Up @@ -56,4 +59,36 @@ public void saveUserPlace(UserPlaceRequestDto request) {
.build()
);
}

public List<UserPlaceListResponseDto> getUserPlaces(Long userId) {
// 유저 존재 여부 확인
User user = userRepository.findById(userId)
.orElseThrow(() -> new ApplicationException(UserErrorCase.USER_NOT_FOUND));

List<UserPlaceWaybleZoneMapping> mappings = mappingRepository.findAllByUserPlace_User_Id(userId);

return mappings.stream().map(mapping -> {
UserPlace userPlace = mapping.getUserPlace();
WaybleZone waybleZone = mapping.getWaybleZone();

// 웨이블존 대표 이미지 가져오기
String imageUrl = waybleZone.getWaybleZoneImageList().stream()
Copy link
Member

Choose a reason for hiding this comment

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

waybleZone 엔티티에서 대표 이미지 url 필드가 따로 있으면 좋을 것 같아요!
매번 추출하는 것도 귀찮고, 웨이블존 상세 페이지 이외에 모든 화면들에서는 대표 이미지만 쓰이니까요...!
그리고 현재 방식처럼 쓰면 N + 1 문제가 발생할 것 같습니다!

elastic search에 웨이블존 저장할 때도 대표 이미지 url 필드만 따로 지정해놓아서 개발했는데, 값 동기화할 때도 이게 편할 것 같구요!

Copy link
Member Author

Choose a reason for hiding this comment

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

다시 고민해보니깐 이건 지적해주신게 완전하게 맞는 말인거같아요 감사합니다! 현재는 대표 이미지를 사용할 때마다 getWaybleZoneImageList().stream()에 find로 매번 찾아야하는데 이건 웨이블존 상세가 아닌 곳에서 매번 stream 연산 필요하기때문에 N+1 문제가 발생 가능할것 같아요. 쿼리 최적화를 위해 칼럼 추가해서 리팩토링 하겠습니다. 좋은 리뷰 감사합니다!!

.findFirst().map(img -> img.getImageUrl()).orElse(null);

return UserPlaceListResponseDto.builder()
.place_id(userPlace.getId())
.title(userPlace.getTitle())
.wayble_zone(
UserPlaceListResponseDto.WaybleZoneDto.builder()
.wayble_zone_id(waybleZone.getId())
.name(waybleZone.getZoneName())
.category(waybleZone.getZoneType().toString())
.rating(waybleZone.getRating())
.address(waybleZone.getAddress().toFullAddress())
.image_url(imageUrl)
.build()
)
.build();
}).toList();
}
}
Loading