[feat] 야구 문화 API 구현 및 테스트코드를 작성한다. #54
Conversation
juuuunny
added
🚀 feat
There was a problem hiding this comment.
gitignore 설정이 잘 안되는 것 같아용
일단 올릴 때 포함 안하게끔 잘 보면 좋을 것 같네요!
| if (boundary.equals("내부")) { | ||
| existCourse = Course.of(course); | ||
| } |
There was a problem hiding this comment.
("내부").equals(boundary)처럼 값이 보장되어 있는 걸 앞에 두는 게 좋을 것 같아요!
그럴 일은 잘 없겠지만 boundary가 null인 경우에는 NullPointerException이 발생할 수 있어서요
There was a problem hiding this comment.
아하 equals에서는 null값이 있으면 안되는군요!
수정하겠습니다!
There was a problem hiding this comment.
오 계속 신경 안쓰고 동등성 확인햇었는데
이제 앞에 null 안오도록 확인해야겠네요!
감사합니다~
| @Lob | ||
| @Convert(converter = StringListConverter.class) | ||
| private List<String> explanation; | ||
|
|
||
| @Lob | ||
| @Convert(converter = StringListConverter.class) | ||
| private List<String> tip; |
There was a problem hiding this comment.
두 필드들은 리스트인데
explanation tips 처럼 s를 붙이는 것이 덜 헷갈리지 않을까요? 🤔
지금이 좀 더 의미가 적절하다면 그대로 해도 좋을 것 같습니다!
There was a problem hiding this comment.
생각 안했던 거라 이것두 수정하겠습니다!
| @Getter | ||
| @RequiredArgsConstructor | ||
| public enum Boundary { | ||
|
|
||
| INTERIOR("내부"), | ||
| EXTERIOR("외부"); | ||
|
|
||
| private final String name; | ||
|
|
||
| public static Boundary of(String name) { | ||
| for (Boundary boundary : Boundary.values()) { | ||
| if (boundary.getName().equals(name)) { | ||
| return boundary; | ||
| } | ||
| } | ||
| throw new CustomException(FoodErrorStatus._BAD_REQUEST_BOUNDARY); | ||
| } | ||
| } |
There was a problem hiding this comment.
Enum 클래스에서 에러를 던지는 건 개인적으로 조금 적절하지 않은 것 같아요 🤔
// Boundary에 해당 name이 있는지 확인하는 메서드
public static boolean isExists(String name) {
return Arrays.stream(Boundary.values())
.anyMatch(boundary -> boundary.getName().equals(name));
}이런식으로 내부에 값이 있는지 확인하는 메서드를 만드는 건 어떨까요??
@Transactional(readOnly = true)
public GetEntertainmentsResponseDto getSuitableEntertainments(String stadiumName, String boundary) {
Stadium stadium = stadiumRepository.findByName(stadiumName)
.orElseThrow(() -> new CustomException(StadiumErrorStatus._NOT_FOUND_STADIUM));
// Boundary에 해당 값이 존재하는지 먼저 확인
if (!Boundary.isExists(boundary)) {
throw new CustomException(FoodErrorStatus._BAD_REQUEST_BOUNDARY);
}
// 이미 존재 여부를 확인했으므로 안전하게 of 호출
Boundary existBoundary = Boundary.of(boundary);
List<GetEntertainmentsResponseDto.EntertainmentDto> entertainments = entertainmentRepository.findAllByStadiumAndBoundary(stadium, existBoundary)
.stream()
.map(GetEntertainmentsResponseDto.EntertainmentDto::from)
.toList();
return GetEntertainmentsResponseDto.of(entertainments);
}그리고 서비스 단에서 이렇게 메서드를 호출해서 확인 후 진행하는 걸로요!
대부분의 중요한 로직은 서비스에서 처리하는 게 좋다고 생각을 합니다..!
(주석은 참고용입니다)
There was a problem hiding this comment.
넵 예외 처리나 이런 것은 최대한 서비스단에서 하는 게 좋을 것 같아요!
@Getter
@RequiredArgsConstructor
public enum Boundary {
INTERIOR("내부"),
EXTERIOR("외부");
private final String name;
public static boolean isExists(String name) {
return Arrays.stream(Boundary.values())
.anyMatch(boundary -> boundary.getName().equals(name));
}
public static Boundary of(String name) {
for (Boundary boundary : Boundary.values()) {
if (boundary.getName().equals(name)) {
return boundary;
}
}
return null;
}
}이런식으로 바꿨는데 Boundary를 반환해야 하는데 그것이 안되는 경우에 예외처리를 넣어주면 해결이 되어서 했던거라
return null; 로 바꿔두었습니다.
There was a problem hiding this comment.
흠,, 클라이언트측에서 입력받은거 그대로 해서 바로 뺄수 있게 하려고 public static Boundary of(String name){}
이걸 쓰는 건데
차라리 switch case로 하는 게 좋아보일까요?
There was a problem hiding this comment.
아아 흐음 null을 반환하게 된다면 좀 애매한 것 같아요..!
이러면 isExist 메서드가 굳이 필요할까? 라는 생각이 들어요.
아니면 혹시 of 메서드 대신에
public static Optional<Boundary> findByName(String name) {
return Arrays.stream(Boundary.values())
.filter(boundary -> boundary.getName().equals(name))
.findFirst();
}위 메서드를 두는 건 어떨까요? 현재 of 메서드의 동작이 enum에서 찾아서 반환하는 건데 이는 findBy~에 더 맞는 것 같아서요!
그리고 서비스 단에서
@Transactional(readOnly = true)
public GetEntertainmentsResponseDto getSuitableEntertainments(String stadiumName, String boundary) {
Stadium stadium = stadiumRepository.findByName(stadiumName)
.orElseThrow(() -> new CustomException(StadiumErrorStatus._NOT_FOUND_STADIUM));
Boundary existBoundary = Boundary.findByName(boundary)
.orElseThrow(() -> new CustomException(FoodErrorStatus._BAD_REQUEST_BOUNDARY));
List<GetEntertainmentsResponseDto.EntertainmentDto> entertainments = entertainmentRepository.findAllByStadiumAndBoundary(stadium, existBoundary)
.stream()
.map(GetEntertainmentsResponseDto.EntertainmentDto::from)
.toList();
return GetEntertainmentsResponseDto.of(entertainments);
}이렇게 한다면 불필요한 부분이 제거되지 않을까요??!
There was a problem hiding this comment.
아 이렇게 Optional로 해서 없으면 커스텀에러 던지고 있으면 Boundary 반환되게 하는 거 좋은 것 같아요.
이렇게 수정할게요!
| public static Course of(String name) { | ||
| for (Course course : Course.values()) { | ||
| if (course.getName().equals(name)) { | ||
| return course; | ||
| } | ||
| } | ||
| throw new CustomException(FoodErrorStatus._BAD_REQUEST_COURSE); | ||
| } | ||
| } |
There was a problem hiding this comment.
이것도 위와 동일 의견입니다!
of라는 메서드는 단순히 파라미터를 받아서 새로운 객체를 반환하는 정적 팩터리 메서드인데 너무 많은 역할을 하는 것 같아요.
There was a problem hiding this comment.
넵 이것두 위와 동일하게 수정하겠습니다!
| @Override | ||
| public List<Food> findFoodsByConditions(Stadium stadium, Boundary boundary, Course course) { | ||
|
|
||
| BooleanBuilder builder = new BooleanBuilder(); | ||
| builder.and(food.stadium.eq(stadium)); | ||
| builder.and(food.boundary.eq(boundary)); | ||
| if (boundary == Boundary.INTERIOR) { | ||
| builder.and(food.course.eq(course)); | ||
| } | ||
|
|
||
| return jpaQueryFactory | ||
| .selectFrom(food) | ||
| .distinct() | ||
| .where(builder) | ||
| .fetch(); | ||
| } |
| public record GetFoodsResponseDto(List<FoodDto> foods) { | ||
|
|
||
| public record FoodDto(String imgUrl, Boundary boundary, Course course, String name, String location, List<String> menu, String price, String tip) { | ||
| public static FoodDto from(Food food) { | ||
| return new FoodDto(food. getImgUrl(), food.getBoundary(), food.getCourse(), food.getName(), food.getLocation(), food.getMenu(), food.getPrice(), food.getTip()); | ||
| } | ||
| } |
| @GetMapping("/culture/entertainments") | ||
| public ResponseEntity<ApiResponse<GetEntertainmentsResponseDto>> getSuitableEntertainments( | ||
| @RequestParam String stadiumName, | ||
| @RequestParam String boundary |
There was a problem hiding this comment.
이거 파라미터 유효성 검사 자동으로 되지 않나요?
@RequestParam(message = "~~~~") 이렇게 해줘야 하나요?
쿼리 붙이는 건 써보질 않았어가지고
There was a problem hiding this comment.
@RequestBody에서만 dto안에서 파라미터 검증으로 NotBlank, NotNull해주면 될 거 같고,
쿼리에서는 할 필요 없을 거 같아요
안해도 아래처럼 이렇게 잘 나와요!
There was a problem hiding this comment.
@RestController
@RequiredArgsConstructor
@RequestMapping("/api/v1/chatbot")
@Validated
public class ChatbotController {
private final ChatbotService chatbotService;
private final ClovaService clovaService;
/**
* 가이드 챗봇 답변 조회 API
*
* 이 API는 특정 경기장과 카테고리에 맞는 가이드 답변을 조회합니다.
* 예를 들어, 사용자가 "stadium", "baseball", "manner" 등의 카테고리에 대해
* 안내를 요청할 때, 해당 카테고리와 경기장에 맞는 정보를 제공합니다.
*
* @param stadiumName 유효하지 않은 값일 수 없는 경기장 이름
* @param categoryName 유효하지 않은 값일 수 없는 카테고리 이름 (예: stadium, baseball 등)
* @param orderNumber 1 이상의 유효한 순서 번호
* @return 요청한 경기장과 카테고리에 맞는 가이드 답변 및 관련 이미지 URL
*/
@GetMapping("/guide")
public ResponseEntity<ApiResponse<GetGuideChatbotAnswerResponse>> getGuideChatbotAnswer(
@RequestParam("stadiumName") @NotBlank String stadiumName,
@RequestParam("categoryName") @NotBlank String categoryName,
@RequestParam("orderNumber") @Min(1) int orderNumber){
GetGuideChatbotAnswerResponse response = chatbotService.getGuideChatbotAnswer(stadiumName, categoryName, orderNumber);
return ApiResponse.onSuccess(ChatbotSuccessStatus._GET_GUIDE_CHATBOT_ANSWER, response);
}
...이런식으로 파라미터 @NotBlank @Min(1) 등을 옆에 붙여서 할 수 있습니다!
그리고 클래스 단에 @Validated 어노테이션 붙여주셔야 하구요
ChatbotController 클래스 참고해주시면 될 것 같습니다!
There was a problem hiding this comment.
테스트를 해봤는데
1. @Validated 처리 안하는 경우
쿼리 파라미터 자체가 누락된 경우에는 잘 처리가 됩니다!
이는 GlobalExceptionHandler 클래스 내의
// MissingServletRequestParameterException 처리 (필수 쿼리 파라미터가 입력되지 않은 경우)
@Override
protected ResponseEntity<Object> handleMissingServletRequestParameter(MissingServletRequestParameterException ex,
HttpHeaders headers,
HttpStatusCode status,
WebRequest request) {
String errorMessage = "필수 파라미터 '" + ex.getParameterName() + "'가 없습니다.";
logError("MissingServletRequestParameterException", errorMessage);
return ApiResponse.onFailure(ErrorStatus._BAD_REQUEST, errorMessage);
}이 부분에서 처리해주어서 그래요
There was a problem hiding this comment.
다만 이렇게
쿼리 파라미터에 띄어쓰기나 공백이 있는 경우에는 당연하게도 처리가 안됩니다!
orderNumber도 1 이상이어야 하는데 유효성 검증이 안되고 있구요.
There was a problem hiding this comment.
넵 보니까 이렇게 Min(1L) 이런식으로 해주면 유효성 검증 되네요!!
다 추가하고 있습니다!
There was a problem hiding this comment.
2. @Validated 처리 하는 경우
하지만 처리를 하게 되면 이렇게 공백인 경우에도 유효성 검증이 됩니다!
(테스트 하면서 알게 된 건데, (공백)lg(공백) 이런 경우에는 자체적으로 공백 제거를 해주어서 에러가 안터지는 것 같네요)
그래서 결론적으로 조금 더 상세하게 검증하기 위해서는 쓰는 게 좋지 않을까? 하는 생각입니다!
| @RequestParam(required = false) String course | ||
| ) { | ||
| FoodSuccessStatus status; | ||
| if (boundary.equals("내부") && course.equals("식사")) { |
There was a problem hiding this comment.
("내부").equals(boundary) && ("식사").equals(course)| @RequestParam String boundary | ||
| ) { | ||
| EntertainmentSuccessStatus status; | ||
| if (boundary.equals("내부")) { |
| FoodSuccessStatus status; | ||
| if (boundary.equals("내부") && course.equals("식사")) { | ||
| status = FoodSuccessStatus._OK_GET_INTERIOR_MEALS; | ||
| } else if (boundary.equals("내부") && course.equals("후식")) { |
There was a problem hiding this comment.
("내부").equals(boundary) && ("후식").equals(course)
✅ PR 유형
어떤 변경 사항이 있었나요?
📝 작업 내용
이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
- 먹거리
✏️ 관련 이슈
🎸 기타 사항 or 추가 코멘트