Conversation
| @@ -12,8 +12,6 @@ | |||
| import org.springframework.security.core.annotation.AuthenticationPrincipal; | |||
| import org.springframework.web.bind.annotation.*; | |||
|
|
|||
There was a problem hiding this comment.
PetController의 getAllPets 메서드가 삭제되었습니다. 이 메서드는 모든 펫을 조회하는 기능을 담당했는데, 삭제 이유와 대체 로직이 있는지 확인해야 합니다. 삭제 이유를 commit message에 명확히 명시해주세요.
| import cmf.commitField.domain.pet.service.PetService; | ||
| import cmf.commitField.domain.pet.service.UserPetService; | ||
| import cmf.commitField.domain.user.entity.User; | ||
| import cmf.commitField.domain.user.entity.CustomOAuth2User; |
There was a problem hiding this comment.
User 엔티티 대신 CustomOAuth2User를 import 합니다. Spring Security의 AuthenticationPrincipal 어노테이션과 함께 사용하는 것으로 보이며, 사용자 인증 방식과 일관성을 유지하는지 확인해야 합니다.
| import cmf.commitField.domain.pet.service.UserPetService; | ||
| import cmf.commitField.domain.user.entity.User; | ||
| import cmf.commitField.domain.user.entity.CustomOAuth2User; | ||
| import cmf.commitField.domain.user.service.CustomOAuth2UserService; |
There was a problem hiding this comment.
GlobalResponse와 관련된 import 문이 삭제되었습니다. GlobalResponse 클래스가 어떤 역할을 했는지 확인하고, 삭제된 이유와 대체 로직이 있는지 확인해야 합니다. ResponseEntity로 변경된 부분과의 연관성을 확인하세요.
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.security.core.annotation.AuthenticationPrincipal; | ||
| import org.springframework.web.bind.annotation.GetMapping; |
There was a problem hiding this comment.
@PathVariable을 사용한 userId 파라미터가 삭제되고, @AuthenticationPrincipal을 사용하여 CustomOAuth2User를 주입받도록 변경되었습니다. 사용자 인증 방식이 변경된 것으로 보이며, 보안 및 기능적인 측면에서 검토가 필요합니다. 이 변경으로 인해 발생할 수 있는 문제점을 고려해야 합니다.
| import org.springframework.web.bind.annotation.PathVariable; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
| import org.springframework.web.bind.annotation.RestController; | ||
|
|
There was a problem hiding this comment.
getUserPetCollection 메서드의 반환 타입이 GlobalResponse<List>에서 ResponseEntity로 변경되었습니다. GlobalResponse를 사용하지 않는 이유를 확인하고, ResponseEntity를 사용하는 것이 적절한지 확인해야 합니다.
| @@ -24,11 +22,11 @@ public class UserPetController { | |||
| private final CustomOAuth2UserService customOAuth2UserService; | |||
| private final PetService petService; | |||
|
|
|||
There was a problem hiding this comment.
주석 // TODO: 기능 확장시 추가 예정 이 삭제되었습니다. TODO 주석은 개발 과정에서 중요한 정보이므로, 삭제 이유를 확인해야 합니다. 만약 기능 확장 계획이 변경되었다면, commit message에 명확히 기록해야 합니다.
| public GlobalResponse<List<Pet>> getUserPetCollection(@PathVariable Long userId) { | ||
| User user = customOAuth2UserService.getUserById(userId).orElse(null); | ||
| return GlobalResponse.success(userPetService.getUserPetCollection(user)); | ||
| @GetMapping("/collection") |
There was a problem hiding this comment.
기존의 getUserPetCollection 메소드가 수정되었습니다. PathVariable userId를 사용하지 않고, AuthenticationPrincipal을 이용하여 사용자 정보를 가져오도록 변경되었습니다. 보안 및 성능 측면을 고려하여 이 변경이 적절한지 검토해야 합니다.
| @@ -0,0 +1,17 @@ | |||
| package cmf.commitField.domain.pet.dto; | |||
There was a problem hiding this comment.
새로운 DTO 클래스 UserPetListDto가 추가되었습니다. 이 DTO가 어떤 목적으로 사용되는지 확인하고, 필드의 타입과 이름이 적절한지 검토해야 합니다. 특히, petList Map의 Integer 키는 펫 타입을 나타내는 것으로 보이는데, 100개의 타입을 지원하는 것이 적절한지 확인하고, 더 효율적인 방법이 있는지 고려해봐야 합니다. FIXME: 차후 갯수 수정 필요 부분을 수정해야 합니다. 어떤 방식으로 펫 타입의 갯수를 동적으로 관리할 수 있을지 고려해야 합니다. 예를 들어, enum을 사용하는 것이 좋을 수 있습니다.
| // 모든 펫 조회 | ||
| public List<Pet> getAllPets() { | ||
| return petRepository.findAll(); | ||
| // 유저가 소유한 펫 조회 |
There was a problem hiding this comment.
getAllPets 메서드가 수정되었습니다. 이제 사용자 이름을 받아서 해당 사용자가 소유한 펫 목록을 UserPetListDto 형태로 반환합니다. petsNum 배열의 크기가 100으로 고정되어 있는데, 이는 매우 비효율적이며, 향후 펫 타입이 100개를 넘어가면 문제가 발생합니다. FIXME: 차후 갯수 수정 필요 라는 주석이 있는데, 더 나은 데이터 구조 (예: Map)를 사용하여 펫 타입과 개수를 관리해야 합니다. 또한, max 변수를 사용하여 최대 펫 타입을 찾는 부분도 비효율적입니다. Map을 사용하면 이 부분이 필요 없습니다. 전반적으로 코드의 효율성을 개선해야 합니다.
| private final CustomOAuth2UserService userService; | ||
| private final PetService petService; | ||
|
|
||
| // 현재 펫 경험치 상승 및 상승 시 레벨업 처리 |
There was a problem hiding this comment.
새로운 API 엔드포인트 추가입니다. 좋은데, @GetMapping("/getall") 경로는 getAllPets 보다 구체적인 이름으로 변경하는 것이 좋겠습니다. 예를 들어 /myPets 와 같이요. 또한, CustomOAuth2User 에서 username을 가져오는 방식은 보안상의 문제가 발생할 수 있습니다. username을 가져오는 부분에 대한 보안 검토가 필요합니다. 만약 CustomOAuth2User 가 user ID를 제공한다면 ID를 이용하여 petService.getAllPets 를 호출하도록 변경해야합니다.
| @@ -54,11 +63,6 @@ public ResponseEntity<Pet> createPet(@AuthenticationPrincipal CustomOAuth2User o | |||
| return ResponseEntity.ok(pet); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
기존의 getAllPets 메소드가 삭제되었습니다. 삭제 이유와 @GetMapping("/getall") 메서드와의 관계를 주석으로 명확히 설명해주세요. 두 메서드가 중복되는 기능을 가지고 있었다면, 왜 새 메서드로 대체되었는지에 대한 설명이 필요합니다.
| import cmf.commitField.domain.pet.service.PetService; | ||
| import cmf.commitField.domain.pet.service.UserPetService; | ||
| import cmf.commitField.domain.user.entity.User; | ||
| import cmf.commitField.domain.user.entity.CustomOAuth2User; |
There was a problem hiding this comment.
User import문이 CustomOAuth2User 로 변경되었습니다. 변경 사유에 대한 주석이 필요합니다. CustomOAuth2User 사용으로 인해 권한 관리에 영향이 있는지 확인해야 합니다.
| import cmf.commitField.domain.pet.service.UserPetService; | ||
| import cmf.commitField.domain.user.entity.User; | ||
| import cmf.commitField.domain.user.entity.CustomOAuth2User; | ||
| import cmf.commitField.domain.user.service.CustomOAuth2UserService; |
There was a problem hiding this comment.
불필요한 import문(GlobalResponse, CustomOAuth2UserService)이 삭제되었습니다. 코드 정리에 도움이 됩니다. CustomOAuth2UserService 의 사용이 사라진 이유를 확인해야 합니다. 해당 서비스에서 제공하던 기능이 다른 곳으로 이동했는지 확인해야 합니다.
| import cmf.commitField.domain.user.service.CustomOAuth2UserService; | ||
| import cmf.commitField.global.globalDto.GlobalResponse; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.http.ResponseEntity; |
There was a problem hiding this comment.
필요한 import문(ResponseEntity, AuthenticationPrincipal)이 추가되었습니다. @AuthenticationPrincipal 어노테이션을 사용하는 것은 좋습니다.
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.security.core.annotation.AuthenticationPrincipal; | ||
| import org.springframework.web.bind.annotation.GetMapping; |
There was a problem hiding this comment.
불필요한 import문(PathVariable)이 삭제되었습니다. 코드 정리에 도움이 됩니다.
| private final UserPetService userPetService; | ||
| private final CustomOAuth2UserService customOAuth2UserService; | ||
| private final PetService petService; | ||
|
|
There was a problem hiding this comment.
기존의 getUserPetCollection 메서드가 @AuthenticationPrincipal 을 사용하는 새로운 메서드로 변경되었습니다. 이는 보안 및 사용자 인증 측면에서 개선된 부분입니다. 하지만, userId를 통한 접근 방식은 삭제되었는데, 이 부분의 필요성 여부를 재고해야 합니다. 특정 상황에서 userId 를 통한 접근이 필요할 수 있습니다.
| public GlobalResponse<List<Pet>> getUserPetCollection(@PathVariable Long userId) { | ||
| User user = customOAuth2UserService.getUserById(userId).orElse(null); | ||
| return GlobalResponse.success(userPetService.getUserPetCollection(user)); | ||
| @GetMapping("/collection") |
There was a problem hiding this comment.
ResponseEntity<UserPetListDto> 반환 타입은 좋은 선택입니다. 에러 핸들링을 위해 예외처리(예: try-catch 블록)를 추가하는 것을 고려해 보세요.
| // 모든 펫 조회 | ||
| public List<Pet> getAllPets() { | ||
| return petRepository.findAll(); | ||
| public List<UserPetDto> getAllPets(String username){ |
There was a problem hiding this comment.
새로운 메서드 getAllPets(String username)가 추가되었습니다. username을 사용하는 것은 좋지만, SQL Injection 취약성을 방지하기 위해 PreparedStatement를 사용하는 것이 중요합니다. 또한, 에러 핸들링 로직이 필요합니다. 예외처리(try-catch)를 추가하는 것을 고려하세요. 그리고, UserPetDto 생성시 불필요한 객체 생성을 줄이는 방법을 고려해 보세요. (ex. builder pattern 최적화)
| // 유저가 소유한 펫 도감 조회 | ||
| public UserPetListDto getUserCollection(String username) { | ||
| List<Pet> pets = petRepository.findByUserUsername(username); | ||
| int[] petsNum = new int[100]; //FIXME: 차후 갯수 수정 필요 |
There was a problem hiding this comment.
새로운 메서드 getUserCollection(String username)가 추가되었습니다. petsNum 배열의 크기(100)가 고정되어 있는데, 이는 향후 펫 종류가 늘어날 경우 문제가 될 수 있습니다. 동적 크기의 자료구조(예: ArrayList)를 사용하는 것이 좋습니다. 또한, SQL Injection 취약성 방지를 위해 PreparedStatement 사용이 중요하며, 에러 핸들링 로직 추가가 필요합니다. 그리고, 코드 가독성을 위해 변수명을 더 명확하게 하는 것이 좋습니다. (예: petsNum -> petCounts)
| private final CustomOAuth2UserService userService; | ||
| private final PetService petService; | ||
|
|
||
| // 현재 펫 경험치 상승 및 상승 시 레벨업 처리 |
There was a problem hiding this comment.
새로운 /getall 엔드포인트를 추가했습니다. @AuthenticationPrincipal을 사용하여 사용자 인증을 처리하는 것은 좋습니다. 하지만, CustomOAuth2User에서 getName()으로 사용자 이름을 가져오는 부분은 CustomOAuth2User의 구현에 따라 다를 수 있으므로, 좀 더 명확하고 안전한 방법을 고려해야 합니다. 예를 들어, oAuth2User.getAttribute("username") 와 같이 attribute 이름을 명시적으로 사용하는 것이 좋습니다. 또한, 에러 핸들링이 전혀 없으므로, 예외 발생 시 적절한 응답을 반환하도록 수정해야 합니다. (예: ExceptionHandler 사용)
| @@ -31,13 +40,6 @@ public ResponseEntity<UserPetDto> getPetExp(@AuthenticationPrincipal CustomOAuth | |||
| return ResponseEntity.ok(userPetDto); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
기존의 /getall 엔드포인트가 삭제되었습니다. 삭제 이유와 새로운 /getall 엔드포인트와의 차이점을 명확히 해주세요. 기존 기능이 완전히 대체되는 것인지, 아니면 다른 기능을 담당하게 될 것인지 설명이 필요합니다.
| import cmf.commitField.domain.pet.service.PetService; | ||
| import cmf.commitField.domain.pet.service.UserPetService; | ||
| import cmf.commitField.domain.user.entity.User; | ||
| import cmf.commitField.domain.user.entity.CustomOAuth2User; |
There was a problem hiding this comment.
User 엔티티 대신 CustomOAuth2User를 import 합니다. 사용자 정보를 얻는 방법이 변경되었음을 나타냅니다. 일관성을 유지하기 위해 다른 부분도 CustomOAuth2User를 사용하는지 확인해야 합니다.
| import cmf.commitField.domain.pet.service.UserPetService; | ||
| import cmf.commitField.domain.user.entity.User; | ||
| import cmf.commitField.domain.user.entity.CustomOAuth2User; | ||
| import cmf.commitField.domain.user.service.CustomOAuth2UserService; |
There was a problem hiding this comment.
불필요한 import가 삭제되었습니다. CustomOAuth2UserService가 더 이상 필요없는 이유를 명시해주세요.
| import cmf.commitField.domain.user.service.CustomOAuth2UserService; | ||
| import cmf.commitField.global.globalDto.GlobalResponse; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.http.ResponseEntity; |
There was a problem hiding this comment.
필요한 import 추가: ResponseEntity와 @AuthenticationPrincipal
| public GlobalResponse<List<Pet>> getUserPetCollection(@PathVariable Long userId) { | ||
| User user = customOAuth2UserService.getUserById(userId).orElse(null); | ||
| return GlobalResponse.success(userPetService.getUserPetCollection(user)); | ||
| @GetMapping("/collection") |
There was a problem hiding this comment.
/collection/{userId} 엔드포인트를 /collection으로 변경하고, @AuthenticationPrincipal을 사용하여 사용자 인증을 처리합니다. 이 변경으로 인해 CustomOAuth2UserService와 PathVariable이 필요 없어졌습니다. 이 부분에 대한 설명이 필요합니다. 또한, GlobalResponse에서 ResponseEntity로 변경된 이유를 설명해주세요. ResponseEntity를 사용하는 것은 좋지만, 일관성을 위해 모든 API에서 ResponseEntity를 사용하는지 확인해야 합니다.
| @@ -0,0 +1,17 @@ | |||
| package cmf.commitField.domain.pet.dto; | |||
There was a problem hiding this comment.
새로운 DTO 클래스 UserPetListDto 추가. 이 DTO의 용도와 각 필드의 의미를 명확하게 설명해야 합니다. 특히 petList의 Map<Integer, Integer> 타입은 Integer 키(펫 타입)와 Integer 값(소유 개수)으로 구성됩니다. 키 값의 범위와 에러 처리에 대한 고려가 필요합니다. petList의 키값(펫 타입)이 100개를 넘어가면 어떻게 될까요? 예외처리가 필요할 것 같습니다.
| // 모든 펫 조회 | ||
| public List<Pet> getAllPets() { | ||
| return petRepository.findAll(); | ||
| // 유저가 소유한 펫 도감 조회 |
There was a problem hiding this comment.
새로운 메서드 getUserCollection 추가. 유저가 소유한 펫 목록을 UserPetListDto로 반환합니다. petsNum 배열은 펫 타입별 개수를 저장하는데, 크기가 100으로 고정되어 있습니다. 이 크기는 펫 타입의 최대 개수를 고려하여 설정해야 하며, 향후 펫 타입이 추가될 경우 크기를 동적으로 조정하거나, 더 효율적인 데이터 구조 (예: HashMap)를 사용하는 것을 고려해야 합니다. FIXME 주석이 있는 부분을 수정해야 합니다. 또한, 에러 핸들링이 없습니다. 예외 발생 시 적절한 예외 처리를 추가해야 합니다.
#️⃣ Issue Number
#150
📝 요약(Summary)
펫 조회 api 추가
🛠️ PR 유형
어떤 변경 사항이 있나요?
📸스크린샷 (선택)
💬 공유사항 to 리뷰어
✅ PR Checklist
PR이 다음 요구 사항을 충족하는지 확인하세요.