[DDING-99] 동아리 폼지 통계 주관식 상세조회 API 구현 #254
Conversation
KoSeonJe
requested review from
wonjunYou,
5uhwann and
Seooooo24
as code owners
|
Warning Rate limit exceeded@KoSeonJe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Walkthrough이번 PR은 텍스트 필드 통계를 조회하기 위한 기능을 추가합니다. API 인터페이스와 컨트롤러에 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as 클라이언트
participant Ctrl as CentralFormController
participant Facade as FacadeCentralFormServiceImpl
participant StatSvc as FormStatisticServiceImpl
participant Repo as FormAnswerRepository
Client->>Ctrl: GET /my/forms/statistics/text?fieldId={id}
Ctrl->>Facade: getTextFieldStatistics(fieldId)
Facade->>Repo: FormField 조회 및 isTextType() 검증
Facade->>StatSvc: createTextStatistics(formField) 호출
StatSvc->>Repo: findAllValueByFormFieldId / getTextAnswerInfosByFormFieldId 호출
Repo-->>StatSvc: 텍스트 통계 정보 반환
StatSvc-->>Facade: 통계 목록 반환
Facade-->>Ctrl: TextFieldStatisticsQuery 객체 반환
Ctrl-->>Client: TextFieldStatisticsResponse 변환 후 응답 전송
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
src/test/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepositoryTest.java (1)
21-49: 테스트 케이스 보완을 제안드립니다.현재 테스트는 기본적인 시나리오만 다루고 있습니다. 다음과 같은 엣지 케이스도 고려해보시면 좋을 것 같습니다:
- 빈 value 리스트를 가진 FormAnswer
- 존재하지 않는 FormField ID에 대한 조회
- 대용량 데이터 시나리오 (많은 수의 FormAnswer)
테스트 케이스를 추가하시겠습니까? 필요하시다면 제가 테스트 코드를 작성해드릴 수 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/dto/TextAnswerInfo.java (1)
3-8: 인터페이스 문서화 추가 필요각 메서드의 목적과 반환값에 대한 Javadoc 문서화를 추가하면 인터페이스의 사용 목적과 각 메서드의 역할을 더 명확하게 이해할 수 있습니다.
public interface TextAnswerInfo { + /** + * 답변의 고유 식별자를 반환합니다. + * @return 답변 ID + */ Long getId(); + + /** + * 답변자의 이름을 반환합니다. + * @return 답변자 이름 + */ String getName(); + + /** + * 답변 내용을 반환합니다. + * @return 답변 텍스트 + */ String getValue(); }src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/TextFieldStatisticsQuery.java (1)
5-16: 필드 유효성 검증 어노테이션 추가 필요데이터의 무결성을 보장하기 위해 각 필드에 대한 유효성 검증 어노테이션을 추가하는 것이 좋습니다.
+import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.NotBlank; + public record TextFieldStatisticsQuery( + @NotBlank(message = "type은 필수 입력값입니다") String type, + @NotNull(message = "answers는 null일 수 없습니다") List<TextStatisticsQuery> answers ) { public record TextStatisticsQuery( + @NotNull(message = "applicationId는 필수 입력값입니다") Long applicationId, + @NotBlank(message = "name은 필수 입력값입니다") String name, + @NotBlank(message = "answer는 필수 입력값입니다") String answer ) { } }src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java (1)
31-31: 메서드 문서화 추가 필요새로 추가된
getTextFieldStatistics메서드에 대한 Javadoc 문서화를 추가하여 메서드의 목적, 파라미터, 반환값, 예외 상황 등을 명시하면 좋겠습니다.+ /** + * 주어진 필드 ID에 해당하는 텍스트 필드의 통계 정보를 조회합니다. + * + * @param fieldId 조회할 필드의 ID + * @return 텍스트 필드 통계 정보 + * @throws IllegalArgumentException 유효하지 않은 필드 ID인 경우 + * @throws IllegalStateException 텍스트 필드가 아닌 경우 + */ TextFieldStatisticsQuery getTextFieldStatistics(Long fieldId);src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepository.java (1)
26-37: 쿼리 성능 최적화를 고려해보세요.네이티브 쿼리에서 JOIN 연산이 사용되고 있습니다. 대량의 데이터를 처리할 때 성능 이슈가 발생할 수 있으므로, 다음 사항들을 고려해보시기 바랍니다:
- field_id에 대한 인덱스 추가
- 페이지네이션 도입 검토
src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java (1)
85-89: 예외 처리를 보완하면 좋을 것 같습니다.구현이 기존 패턴을 잘 따르고 있습니다. 다만 다음 사항들을 고려해보시면 좋을 것 같습니다:
- 잘못된 fieldId가 입력된 경우의 예외 처리
- 텍스트 타입이 아닌 필드에 대한 요청 처리
- 응답 데이터가 너무 큰 경우의 페이지네이션 처리
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticServiceImpl.java (2)
117-128: createTextStatistics 메서드의 null 안전성 개선이 필요합니다.
textAnswerInfos가 null일 경우에 대한 처리가 없습니다. 빈 리스트를 반환하기 전에 null 체크를 추가하는 것이 좋겠습니다.@Override public List<TextStatisticsQuery> createTextStatistics(FormField formField) { + if (formField == null) { + return List.of(); + } List<TextAnswerInfo> textAnswerInfos = formAnswerRepository.getTextAnswerInfosByFormFieldId(formField.getId()); + if (textAnswerInfos == null) { + return List.of(); + } return textAnswerInfos.stream() .map(textAnswerInfo -> { Long id = textAnswerInfo.getId(); String name = textAnswerInfo.getName(); String answer = getAnswer(textAnswerInfo.getValue()); return new TextStatisticsQuery(id, name, answer); }) .toList(); }
130-136: getAnswer 메서드의 null 체크가 필요합니다.
value파라미터가 null일 경우에 대한 처리가 필요합니다.private String getAnswer(String value) { + if (value == null) { + return null; + } List<String> answer = stringListConverter.convertToEntityAttribute(value); if(answer.isEmpty()) { return null; } return answer.get(0); }src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (2)
144-153: 필드 타입 검증 로직 중복을 제거하는 것이 좋겠습니다.
getMultipleFieldStatistics와getTextFieldStatistics메서드에서 필드 타입 검증 로직이 중복됩니다. 공통 메서드로 추출하는 것을 고려해보세요.+ private void validateFieldType(FormField formField, boolean expectedType, String typeName) { + if (expectedType) { + throw new InvalidFieldTypeException(); + } + } @Override public TextFieldStatisticsQuery getTextFieldStatistics(Long fieldId) { FormField formField = formFieldService.getById(fieldId); - if (!formField.isTextType()) { - throw new InvalidFieldTypeException(); - } + validateFieldType(formField, !formField.isTextType(), "text"); String type = formField.getFieldType().name(); List<TextStatisticsQuery> textStatisticsQueries = formStatisticService.createTextStatistics(formField); return new TextFieldStatisticsQuery(type, textStatisticsQueries); }
145-145: 트랜잭션 어노테이션 추가를 고려해보세요.다른 유사한 메서드들처럼
@Transactional(readOnly = true)어노테이션을 추가하는 것이 일관성 있어 보입니다.+ @Transactional(readOnly = true) @Override public TextFieldStatisticsQuery getTextFieldStatistics(Long fieldId) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/response/TextFieldStatisticsResponse.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticServiceImpl.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/TextFieldStatisticsQuery.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepository.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/dto/TextAnswerInfo.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepositoryTest.java(2 hunks)
🔇 Additional comments (6)
src/test/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepositoryTest.java (1)
23-23: 메소드 이름 변경이 적절합니다!메소드 이름을
findAllValueByFormFieldId로 변경한 것이 매개변수의 의도를 더 명확하게 표현합니다.src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticService.java (1)
24-25: 인터페이스 확장이 잘 구현되었습니다!텍스트 필드 통계를 위한 메소드가 기존 패턴을 잘 따르고 있으며, 관심사의 분리가 명확합니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepository.java (1)
24-24: 메소드 이름이 명확하게 개선되었습니다.
findAllValueByFormField에서findAllValueByFormFieldId로의 변경은 메소드의 파라미터를 더 정확하게 반영합니다.src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java (1)
75-78: FILE 타입의 포함 여부를 검토해주세요.
isTextType()메소드에 FILE 타입이 포함되어 있습니다. FILE 타입이 텍스트 통계에 포함되어야 하는지 비즈니스 요구사항을 다시 한번 확인해주시기 바랍니다.현재 구현:
return this.fieldType == FieldType.TEXT || this.fieldType == FieldType.LONG_TEXT || this.fieldType == FieldType.FILE;src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java (1)
104-110: API 엔드포인트가 잘 구현되었습니다!OpenAPI 문서화가 잘 되어있고, 기존 객관식 통계 API와 일관된 패턴을 따르고 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticServiceImpl.java (1)
105-105: 메서드 이름 변경이 적절합니다.
findAllValueByFormField에서findAllValueByFormFieldId로의 변경은 메서드의 목적을 더 명확하게 표현합니다.
| public static TextFieldStatisticsResponse from(TextFieldStatisticsQuery query) { | ||
| List<TextStatisticsResponse> answers = query.answers().stream() | ||
| .map(TextStatisticsResponse::from) | ||
| .toList(); | ||
| return new TextFieldStatisticsResponse(query.type(), answers); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
null 체크 및 방어적 프로그래밍 추가 필요
from 메서드에서 null 값에 대한 처리가 없어 NPE가 발생할 수 있습니다. 방어적 프로그래밍을 통해 안정성을 높이는 것이 좋습니다.
public static TextFieldStatisticsResponse from(TextFieldStatisticsQuery query) {
+ if (query == null) {
+ throw new IllegalArgumentException("query는 null일 수 없습니다");
+ }
+ if (query.answers() == null) {
+ throw new IllegalArgumentException("answers는 null일 수 없습니다");
+ }
List<TextStatisticsResponse> answers = query.answers().stream()
.map(TextStatisticsResponse::from)
.toList();
return new TextFieldStatisticsResponse(query.type(), answers);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static TextFieldStatisticsResponse from(TextFieldStatisticsQuery query) { | |
| List<TextStatisticsResponse> answers = query.answers().stream() | |
| .map(TextStatisticsResponse::from) | |
| .toList(); | |
| return new TextFieldStatisticsResponse(query.type(), answers); | |
| } | |
| public static TextFieldStatisticsResponse from(TextFieldStatisticsQuery query) { | |
| if (query == null) { | |
| throw new IllegalArgumentException("query는 null일 수 없습니다"); | |
| } | |
| if (query.answers() == null) { | |
| throw new IllegalArgumentException("answers는 null일 수 없습니다"); | |
| } | |
| List<TextStatisticsResponse> answers = query.answers().stream() | |
| .map(TextStatisticsResponse::from) | |
| .toList(); | |
| return new TextFieldStatisticsResponse(query.type(), answers); | |
| } |
|
|
||
| public boolean isTextType() { | ||
| return this.fieldType == FieldType.TEXT || this.fieldType == FieldType.LONG_TEXT | ||
| || this.fieldType == FieldType.FILE; |
There was a problem hiding this comment.
FieldType이 File인 객체도 TextType이라고 판단해야 하는 이유가 무엇인지 여쭤봐도 될까요?
There was a problem hiding this comment.
사용하는 API도 같기도 하고 반환하는 값이 파일이름인 텍스트로 반환하기 때문입니다!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticServiceImpl.java (1)
136-148: 메서드 안전성과 문서화가 필요합니다.
- NullPointerException 발생 가능성이 있습니다.
- 메서드의 목적과 동작을 설명하는 JavaDoc이 누락되어 있습니다.
다음과 같은 개선을 제안드립니다:
+/** + * 답변 값을 처리하여 문자열로 반환합니다. + * 파일 타입인 경우 파일 이름을 반환하고, 그 외의 경우 첫 번째 답변을 반환합니다. + * + * @param value 처리할 답변 값 + * @param fieldType 필드 타입 + * @return 처리된 답변 문자열, 답변이 없는 경우 null + */ private String getAnswer(String value, FieldType fieldType) { + if (value == null || fieldType == null) { + return null; + } List<String> answer = stringListConverter.convertToEntityAttribute(value); if(answer.isEmpty()) { return null; } if(fieldType == FieldType.FILE) { String fileKey = answer.get(0); + if (fileKey == null) { + return null; + } String fileMetDataId = s3FileService.getUploadedFileUrl(fileKey).id(); FileMetaData fileMetaData = fileMetaDataService.getById(fileMetDataId); return fileMetaData.getFileName(); } return answer.get(0); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticServiceImpl.java(5 hunks)
🔇 Additional comments (2)
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticServiceImpl.java (2)
7-9: 의존성 주입이 올바르게 구현되었습니다!파일 관련 서비스들이 적절하게 주입되었으며, final 키워드를 사용하여 불변성을 보장하고 있습니다.
Also applies to: 45-46
111-111: 메서드 이름이 더 명확해졌습니다!
findAllValueByFormField에서findAllValueByFormFieldId로의 변경은 메서드가 ID를 사용한다는 것을 더 명확하게 표현합니다.
| @Override | ||
| public List<TextStatisticsQuery> createTextStatistics(FormField formField) { | ||
| List<TextAnswerInfo> textAnswerInfos = formAnswerRepository.getTextAnswerInfosByFormFieldId(formField.getId()); | ||
| return textAnswerInfos.stream() | ||
| .map(textAnswerInfo -> { | ||
| Long id = textAnswerInfo.getId(); | ||
| String name = textAnswerInfo.getName(); | ||
| String answer = getAnswer(textAnswerInfo.getValue(), formField.getFieldType()); | ||
| return new TextStatisticsQuery(id, name, answer); | ||
| }) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
입력값 검증이 필요합니다.
formField가 null이거나 텍스트 타입이 아닌 경우에 대한 검증이 누락되어 있습니다.
다음과 같이 검증 로직을 추가하는 것을 제안드립니다:
@Override
public List<TextStatisticsQuery> createTextStatistics(FormField formField) {
+ if (formField == null) {
+ throw new IllegalArgumentException("폼 필드는 null일 수 없습니다.");
+ }
+ if (!formField.getFieldType().isTextType()) {
+ throw new IllegalArgumentException("텍스트 타입의 필드만 처리할 수 있습니다.");
+ }
List<TextAnswerInfo> textAnswerInfos = formAnswerRepository.getTextAnswerInfosByFormFieldId(formField.getId());
return textAnswerInfos.stream()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public List<TextStatisticsQuery> createTextStatistics(FormField formField) { | |
| List<TextAnswerInfo> textAnswerInfos = formAnswerRepository.getTextAnswerInfosByFormFieldId(formField.getId()); | |
| return textAnswerInfos.stream() | |
| .map(textAnswerInfo -> { | |
| Long id = textAnswerInfo.getId(); | |
| String name = textAnswerInfo.getName(); | |
| String answer = getAnswer(textAnswerInfo.getValue(), formField.getFieldType()); | |
| return new TextStatisticsQuery(id, name, answer); | |
| }) | |
| .toList(); | |
| } | |
| @Override | |
| public List<TextStatisticsQuery> createTextStatistics(FormField formField) { | |
| if (formField == null) { | |
| throw new IllegalArgumentException("폼 필드는 null일 수 없습니다."); | |
| } | |
| if (!formField.getFieldType().isTextType()) { | |
| throw new IllegalArgumentException("텍스트 타입의 필드만 처리할 수 있습니다."); | |
| } | |
| List<TextAnswerInfo> textAnswerInfos = formAnswerRepository.getTextAnswerInfosByFormFieldId(formField.getId()); | |
| return textAnswerInfos.stream() | |
| .map(textAnswerInfo -> { | |
| Long id = textAnswerInfo.getId(); | |
| String name = textAnswerInfo.getName(); | |
| String answer = getAnswer(textAnswerInfo.getValue(), formField.getFieldType()); | |
| return new TextStatisticsQuery(id, name, answer); | |
| }) | |
| .toList(); | |
| } |
|
🚀 작업 내용
🤔 고민했던 내용
현재 지원하기 API에서 파일을 저장할 때, FormAnswer 엔터티의 value값에 어떤 값을 저장할 지 정해지지 않아, 제외하고 구현했습니다.
💬 리뷰 중점사항
query및 statisticService중점으로 봐주시면 감사하겠습니다
Summary by CodeRabbit