[DDING-91] 지원하기 API 구현 #233
Conversation
develop 브랜치 풀 받아야 해서 미완성인 상태로 커밋했습니다.
Seooooo24
requested review from
wonjunYou,
5uhwann and
KoSeonJe
as code owners
|
Caution Review failedThe pull request is closed. Walkthrough이번 PR은 폼 관련 서비스와 엔티티에 신규 메서드 및 기능을 추가합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant U as UserFormController
participant F as FacadeUserFormServiceImpl
participant A as GeneralFormApplicationService
participant B as GeneralFormAnswerService
participant DB as Database
C->>U: POST /forms/{formId}/applications (요청)
U->>F: createFormResponse(formId, request)
F->>A: create(formApplication)
A->>DB: FormApplication 저장
F->>B: createAll(formAnswers) 호출
B->>DB: FormAnswer 저장
DB-->>B: 저장 결과 반환
DB-->>A: 저장 결과 반환
F-->>U: 처리 완료 응답 전달
U-->>C: 201 Created 응답
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
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: 9
🧹 Nitpick comments (10)
src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormFieldService.java (1)
25-29: 트랜잭션 설정이 적절합니다!클래스 레벨의 readOnly=true 설정과 메서드 레벨의 @transactional 어노테이션이 잘 구성되어 있습니다. 리포지토리로의 위임도 깔끔하게 구현되었습니다.
성능 최적화를 위해 다음과 같이 readOnly 속성을 추가하는 것을 고려해보세요:
- @Transactional + @Transactional(readOnly = true) @Override public Optional<FormField> findById(Long id) { return formFieldRepository.findById(id); }이는 읽기 전용 작업임을 명시적으로 표현하여 데이터베이스 최적화에 도움이 될 수 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeUserFormService.java (1)
5-8: Facade 패턴이 적절하게 적용되었습니다!Facade 패턴의 첫 구현으로서 다음과 같은 장점들이 잘 반영되었습니다:
- 폼 응답 생성의 복잡한 프로세스를 단순화된 인터페이스로 제공
- Command 객체를 통한 깔끔한 데이터 전달
- 하위 시스템들(FormResponse, FormAnswer)의 세부사항을 숨김
추가 개선사항으로 고려해볼 만한 사항들:
- 응답 생성 실패 시의 예외 처리 방식 정의
- 비동기 처리 가능성 검토
src/main/java/ddingdong/ddingdongBE/domain/form/api/UserFormApi.java (1)
15-15: API 경로 개선 필요현재 "/server" 경로는 너무 일반적이며 폼 관련 API의 의도를 명확히 전달하지 못합니다. 다음과 같이 개선하는 것을 추천드립니다:
- "/api/v1/forms" 또는 "/api/v1/form-responses"와 같이 더 구체적이고 버전이 포함된 경로 사용
-@RequestMapping("/server") +@RequestMapping("/api/v1/forms")src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormAnswer.java (1)
19-20: value 필드에 대한 유효성 검증 추가 필요value 필드에 대한 기본적인 유효성 검증이 누락되어 있습니다.
+import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.Size; -@Column(nullable = false) +@Column(nullable = false) +@NotBlank(message = "답변 값은 필수입니다") +@Size(max = 1000, message = "답변은 1000자를 초과할 수 없습니다") private String value;src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeUserFormServiceImpl.java (3)
14-23: Facade 패턴이 올바르게 구현되었습니다.서비스 구성이 잘 되어 있으며, 각 책임이 적절히 분리되어 있습니다. 다만, 서비스 인터페이스들의 의존성 주입 순서를 일관성 있게 정렬하면 좋을 것 같습니다.
- private final FormResponseService formResponseService; - private final FormAnswerService formAnswerService; - private final FormService formService; - private final FormFieldService formFieldService; + private final FormService formService; + private final FormFieldService formFieldService; + private final FormResponseService formResponseService; + private final FormAnswerService formAnswerService;
24-33: 트랜잭션 관리가 적절합니다.클래스 레벨의
@Transactional(readOnly = true)와 메소드 레벨의@Transactional이 잘 구성되어 있습니다. 하지만 예외 처리를 좀 더 구체적으로 할 필요가 있습니다.@Transactional @Override public void createFormResponse(CreateFormResponseCommand createFormResponseCommand) { + try { Form form = formService.getById(createFormResponseCommand.form().getId()); FormResponse formResponse = createFormResponseCommand.toEntity(form); FormResponse savedFormResponse = formResponseService.create(formResponse); List<FormAnswer> formAnswers = toFormAnswers(savedFormResponse, createFormResponseCommand.formAnswerCommands()); formAnswerService.createAll(formAnswers); + } catch (Exception e) { + throw new FormResponseCreationException("폼 응답 생성 중 오류가 발생했습니다.", e); + } }
35-43: 예외 메시지가 명확하고 구체적입니다.필드 ID를 찾을 수 없을 때의 예외 메시지가 잘 작성되어 있습니다. 하지만 성능 최적화를 위해 스트림 처리를 병렬로 수행할 수 있습니다.
private List<FormAnswer> toFormAnswers(FormResponse savedFormResponse, List<CreateFormResponseCommand.CreateFormAnswerCommand> createFormAnswerCommands) { return createFormAnswerCommands.stream() + .parallel() .map(formAnswerCommand -> { FormField formField = formFieldService.findById(formAnswerCommand.fieldId()) .orElseThrow(() -> new IllegalArgumentException("해당 field를 id로 찾을 수 없습니다: " + formAnswerCommand.fieldId())); return formAnswerCommand.toEntity(savedFormResponse, formField); }) .toList(); }src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/request/CreateFormResponseRequest.java (2)
13-27: 필수 필드에 대한 유효성 검증이 잘 구현되어 있습니다.모든 필수 필드에
@NotNull어노테이션이 적절히 적용되어 있으며, 스키마 문서화도 잘 되어 있습니다. 하지만 학번과 이름에 대한 추가적인 유효성 검증이 필요합니다.@NotNull(message = "지원자 이름은 필수 입력 사항입니다.") + @Size(min = 2, max = 50, message = "이름은 2자 이상 50자 이하여야 합니다.") + @Pattern(regexp = "^[가-힣]+$", message = "이름은 한글만 입력 가능합니다.") @Schema(description = "지원자 이름", example = "김띵동") String name, @NotNull(message = "지원자 학번은 필수 입력 사항입니다.") + @Pattern(regexp = "^[0-9]{8}$", message = "학번은 8자리 숫자여야 합니다.") @Schema(description = "학번", example = "60200000") String studentNumber,
32-52: 중첩된 레코드의 구조가 명확합니다.
CreateFormAnswerRequest레코드의 구조와 유효성 검증이 잘 구현되어 있습니다. 하지만valueType에 대한 열거형 사용을 고려해보세요.+ public enum ValueType { + TEXT, RADIO, CHECKBOX, SELECT + } record CreateFormAnswerRequest( @NotNull(message = "질문 id는 null이 될 수 없습니다.") @Schema(description = "질문 id", example = "1") Long fieldId, @Schema(description = "답변 값") String value, @NotNull(message = "질문 타입은 null이 될 수 없습니다.") @Schema(description = "질문 타입", example = "RADIO") - String valueType + ValueType valueTypesrc/main/resources/db/migration/V35_create_form_response_and_answer_table.sql (1)
1-12: form_response 테이블 구조가 적절합니다.외래 키 제약 조건과 타임스탬프 필드가 잘 구성되어 있습니다. 하지만 조회 성능 향상을 위한 인덱스 추가가 필요합니다.
CONSTRAINT fk_response_form FOREIGN KEY (form_id) REFERENCES form (id) ON DELETE CASCADE ); +CREATE INDEX idx_form_response_student ON form_response(student_number); +CREATE INDEX idx_form_response_status ON form_response(status);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/main/java/ddingdong/ddingdongBE/domain/form/api/UserFormApi.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/UserFormController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/request/CreateFormResponseRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormAnswer.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormResponse.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormResponseStatus.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/repository/FormAnswerRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/repository/FormResponseRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeUserFormService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeUserFormServiceImpl.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FormAnswerService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FormFieldService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FormResponseService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FormService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormAnswerService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormFieldService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormResponseService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/command/CreateFormResponseCommand.java(1 hunks)src/main/resources/db/migration/V35_create_form_response_and_answer_table.sql(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormResponseStatus.java
- src/main/java/ddingdong/ddingdongBE/domain/form/repository/FormAnswerRepository.java
- src/main/java/ddingdong/ddingdongBE/domain/form/repository/FormResponseRepository.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (5)
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormFieldService.java (1)
5-5: 구현이 깔끔하고 적절합니다!Optional을 사용하여 null 처리를 안전하게 하고 있으며, 메서드 시그니처가 명확합니다.
Also applies to: 11-11
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormService.java (1)
8-9: LGTM - Form 조회 메서드가 잘 추가되었습니다!Form 엔티티를 ID로 조회하는 메서드의 시그니처가 명확하고 적절합니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormResponseService.java (1)
5-8: LGTM - FormResponse 생성을 위한 인터페이스가 잘 설계되었습니다!단일 책임 원칙을 잘 준수하고 있으며, 메서드 시그니처가 명확합니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormAnswerService.java (1)
6-9: 일괄 처리 메서드가 잘 설계되었습니다만, 예외 처리 고려가 필요합니다.일괄 생성 메서드의 시그니처는 적절하나, 구현 시 다음 사항들을 고려해 주시기 바랍니다:
- 빈 리스트 처리
- 트랜잭션 실패 시 롤백 처리
- 대량 데이터 처리 시 성능 최적화
src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormResponseService.java (1)
16-19: 🛠️ Refactor suggestion폼 응답 생성 시 유효성 검증이 필요합니다.
현재 구현은 기본적인 저장 기능만 제공합니다. 다음 사항들을 고려해주세요:
- 중복 제출 검증
- 폼 응답 데이터 유효성 검증
- 폼 제출 기간 확인
다음과 같은 검증 로직 추가를 제안드립니다:
@Override public FormResponse create(FormResponse formResponse) { + validateFormResponse(formResponse); + validateSubmissionPeriod(formResponse.getForm()); + validateDuplicateSubmission(formResponse); return formResponseRepository.save(formResponse); } + + private void validateFormResponse(FormResponse response) { + // 필수 필드 검증 + // 데이터 형식 검증 + }
| @Override | ||
| public void createFormResponse( | ||
| CreateFormResponseRequest createFormResponseRequest | ||
| ) { | ||
|
|
||
| } |
There was a problem hiding this comment.
서비스 구현 및 의존성 주입이 필요합니다.
현재 createFormResponse 메서드가 비어있습니다. Facade 패턴을 올바르게 구현하기 위해 다음 사항들이 필요합니다:
FacadeUserFormService의존성 주입- 요청 처리 및 예외 처리 로직 구현
- 적절한 응답 반환
다음과 같이 구현을 제안드립니다:
public class UserFormController implements UserFormApi {
+ private final FacadeUserFormService facadeUserFormService;
@Override
public void createFormResponse(
CreateFormResponseRequest createFormResponseRequest
) {
+ facadeUserFormService.createFormResponse(
+ createFormResponseRequest.toCommand()
+ );
}
}📝 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 void createFormResponse( | |
| CreateFormResponseRequest createFormResponseRequest | |
| ) { | |
| } | |
| public class UserFormController implements UserFormApi { | |
| private final FacadeUserFormService facadeUserFormService; | |
| @Override | |
| public void createFormResponse(CreateFormResponseRequest createFormResponseRequest) { | |
| facadeUserFormService.createFormResponse( | |
| createFormResponseRequest.toCommand() | |
| ); | |
| } | |
| } |
| @Transactional | ||
| @Override | ||
| public Form getById(Long id) { | ||
| return formRepository.getById(id); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
JPA getById 메서드 대신 getReferenceById를 사용해주세요.
getById는 JPA에서 deprecated 되었습니다. 대신 getReferenceById를 사용하는 것이 권장됩니다.
다음과 같이 수정을 제안드립니다:
@Transactional
@Override
public Form getById(Long id) {
- return formRepository.getById(id);
+ return formRepository.getReferenceById(id);
}📝 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.
| @Transactional | |
| @Override | |
| public Form getById(Long id) { | |
| return formRepository.getById(id); | |
| } | |
| @Transactional | |
| @Override | |
| public Form getById(Long id) { | |
| return formRepository.getReferenceById(id); | |
| } |
| @Transactional | ||
| @Override | ||
| public void createAll(List<FormAnswer> formAnswers) { | ||
| formAnswerRepository.saveAll(formAnswers); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
폼 답변 일괄 저장 시 검증 로직이 필요합니다.
현재 구현은 단순 일괄 저장만 수행합니다. 다음 사항들을 고려해주세요:
- 답변 데이터 유효성 검증
- 필수 답변 항목 확인
- 대용량 데이터 처리를 위한 배치 처리 고려
다음과 같은 개선을 제안드립니다:
@Transactional
@Override
public void createAll(List<FormAnswer> formAnswers) {
+ validateFormAnswers(formAnswers);
+
+ // 대용량 데이터 처리를 위한 배치 처리
+ List<List<FormAnswer>> batches = Lists.partition(formAnswers, 1000);
+ for (List<FormAnswer> batch : batches) {
+ formAnswerRepository.saveAll(batch);
+ }
- formAnswerRepository.saveAll(formAnswers);
}
+
+ private void validateFormAnswers(List<FormAnswer> 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.
| @Transactional | |
| @Override | |
| public void createAll(List<FormAnswer> formAnswers) { | |
| formAnswerRepository.saveAll(formAnswers); | |
| } | |
| @Transactional | |
| @Override | |
| public void createAll(List<FormAnswer> formAnswers) { | |
| validateFormAnswers(formAnswers); | |
| // 대용량 데이터 처리를 위한 배치 처리 | |
| List<List<FormAnswer>> batches = Lists.partition(formAnswers, 1000); | |
| for (List<FormAnswer> batch : batches) { | |
| formAnswerRepository.saveAll(batch); | |
| } | |
| } | |
| private void validateFormAnswers(List<FormAnswer> answers) { | |
| // 필수 답변 검증 | |
| // 답변 형식 검증 | |
| } |
| @Column(nullable = false) | ||
| private String valueType; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
valueType 필드의 타입 안정성 개선 필요
String 타입으로 저장되는 valueType은 타입 안정성 문제를 야기할 수 있습니다. Enum을 사용하여 허용된 값 타입을 명시적으로 정의하는 것을 추천드립니다.
+@Getter
+public enum FormAnswerValueType {
+ TEXT,
+ NUMBER,
+ DATE,
+ BOOLEAN
+}
-@Column(nullable = false)
-private String valueType;
+@Enumerated(EnumType.STRING)
+@Column(nullable = false)
+private FormAnswerValueType valueType;📝 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.
| @Column(nullable = false) | |
| private String valueType; | |
| // Existing imports and package declarations remain unchanged | |
| // You may need to add these imports if not already present | |
| import javax.persistence.Column; | |
| import javax.persistence.Enumerated; | |
| import javax.persistence.EnumType; | |
| import lombok.Getter; | |
| @Getter | |
| public enum FormAnswerValueType { | |
| TEXT, | |
| NUMBER, | |
| DATE, | |
| BOOLEAN | |
| } | |
| public class FormAnswer { | |
| // Other fields and methods ... | |
| @Enumerated(EnumType.STRING) | |
| @Column(nullable = false) | |
| private FormAnswerValueType valueType; | |
| // Remaining class implementation ... | |
| } |
| @Builder | ||
| private FormResponse(LocalDateTime submittedAt, String name, String studentNumber, String department, FormResponseStatus status, Form form) { | ||
| this.submittedAt = submittedAt; | ||
| this.name = name; | ||
| this.studentNumber = studentNumber; | ||
| this.department = department; | ||
| this.status = status; | ||
| this.form = form; | ||
| } |
There was a problem hiding this comment.
FormResponse 생성 시 초기 상태 설정 필요
FormResponse 생성 시 status 필드가 초기화되지 않고 있습니다. 기본값으로 PENDING 등의 초기 상태를 설정해야 합니다.
@Builder
private FormResponse(LocalDateTime submittedAt, String name, String studentNumber, String department, FormResponseStatus status, Form form) {
this.submittedAt = submittedAt;
this.name = name;
this.studentNumber = studentNumber;
this.department = department;
- this.status = status;
+ this.status = FormResponseStatus.PENDING;
this.form = form;
}📝 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.
| @Builder | |
| private FormResponse(LocalDateTime submittedAt, String name, String studentNumber, String department, FormResponseStatus status, Form form) { | |
| this.submittedAt = submittedAt; | |
| this.name = name; | |
| this.studentNumber = studentNumber; | |
| this.department = department; | |
| this.status = status; | |
| this.form = form; | |
| } | |
| @Builder | |
| private FormResponse(LocalDateTime submittedAt, String name, String studentNumber, String department, FormResponseStatus status, Form form) { | |
| this.submittedAt = submittedAt; | |
| this.name = name; | |
| this.studentNumber = studentNumber; | |
| this.department = department; | |
| this.status = FormResponseStatus.PENDING; | |
| this.form = form; | |
| } |
| @Column(nullable = false) | ||
| private String name; | ||
|
|
||
| @Column(nullable = false) | ||
| private String studentNumber; | ||
|
|
||
| @Column(nullable = false) | ||
| private String department; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
학생 정보 필드에 대한 유효성 검증 추가 필요
학생 이름, 학번, 학과 필드에 대한 유효성 검증이 누락되어 있습니다.
+import jakarta.validation.constraints.NotBlank;
+import jakarta.validation.constraints.Pattern;
-@Column(nullable = false)
+@Column(nullable = false)
+@NotBlank(message = "이름은 필수입니다")
+@Pattern(regexp = "^[가-힣]{2,10}$", message = "이름은 2-10자의 한글만 허용됩니다")
private String name;
-@Column(nullable = false)
+@Column(nullable = false)
+@NotBlank(message = "학번은 필수입니다")
+@Pattern(regexp = "^\\d{8}$", message = "학번은 8자리 숫자여야 합니다")
private String studentNumber;
-@Column(nullable = false)
+@Column(nullable = false)
+@NotBlank(message = "학과는 필수입니다")
private String department;📝 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.
| @Column(nullable = false) | |
| private String name; | |
| @Column(nullable = false) | |
| private String studentNumber; | |
| @Column(nullable = false) | |
| private String department; | |
| import jakarta.validation.constraints.NotBlank; | |
| import jakarta.validation.constraints.Pattern; | |
| @Column(nullable = false) | |
| @NotBlank(message = "이름은 필수입니다") | |
| @Pattern(regexp = "^[가-힣]{2,10}$", message = "이름은 2-10자의 한글만 허용됩니다") | |
| private String name; | |
| @Column(nullable = false) | |
| @NotBlank(message = "학번은 필수입니다") | |
| @Pattern(regexp = "^\\d{8}$", message = "학번은 8자리 숫자여야 합니다") | |
| private String studentNumber; | |
| @Column(nullable = false) | |
| @NotBlank(message = "학과는 필수입니다") | |
| private String department; |
| public FormResponse toEntity(Form form) { | ||
| return FormResponse.builder() | ||
| .submittedAt(submittedAt) | ||
| .name(name) | ||
| .studentNumber(studentNumber) | ||
| .department(department) | ||
| .form(form) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
FormResponse 엔티티 변환 시 status 필드 누락
toEntity 메서드에서 status 필드가 설정되지 않아 FormResponse 생성 시 오류가 발생할 수 있습니다.
public FormResponse toEntity(Form form) {
return FormResponse.builder()
.submittedAt(submittedAt)
.name(name)
.studentNumber(studentNumber)
.department(department)
.form(form)
+ .status(FormResponseStatus.PENDING)
.build();
}📝 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 FormResponse toEntity(Form form) { | |
| return FormResponse.builder() | |
| .submittedAt(submittedAt) | |
| .name(name) | |
| .studentNumber(studentNumber) | |
| .department(department) | |
| .form(form) | |
| .build(); | |
| } | |
| public FormResponse toEntity(Form form) { | |
| return FormResponse.builder() | |
| .submittedAt(submittedAt) | |
| .name(name) | |
| .studentNumber(studentNumber) | |
| .department(department) | |
| .form(form) | |
| .status(FormResponseStatus.PENDING) | |
| .build(); | |
| } |
| @Builder | ||
| public record CreateFormResponseCommand( | ||
| Form form, | ||
| LocalDateTime submittedAt, | ||
| String name, | ||
| String studentNumber, | ||
| String department, | ||
| List<CreateFormAnswerCommand> formAnswerCommands | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Command 객체에 유효성 검증 추가 필요
Command 객체에 유효성 검증 어노테이션이 누락되어 있어 잘못된 데이터가 서비스 계층까지 전달될 수 있습니다.
+import jakarta.validation.constraints.NotBlank;
+import jakarta.validation.constraints.NotNull;
+import jakarta.validation.constraints.Pattern;
@Builder
public record CreateFormResponseCommand(
- Form form,
+ @NotNull(message = "폼은 필수입니다")
+ Form form,
- LocalDateTime submittedAt,
+ @NotNull(message = "제출 시간은 필수입니다")
+ LocalDateTime submittedAt,
- String name,
+ @NotBlank(message = "이름은 필수입니다")
+ @Pattern(regexp = "^[가-힣]{2,10}$", message = "이름은 2-10자의 한글만 허용됩니다")
+ String name,
- String studentNumber,
+ @NotBlank(message = "학번은 필수입니다")
+ @Pattern(regexp = "^\\d{8}$", message = "학번은 8자리 숫자여야 합니다")
+ String studentNumber,
- String department,
+ @NotBlank(message = "학과는 필수입니다")
+ String department,
- List<CreateFormAnswerCommand> formAnswerCommands
+ @NotNull(message = "답변은 필수입니다")
+ List<CreateFormAnswerCommand> formAnswerCommands
) {📝 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.
| @Builder | |
| public record CreateFormResponseCommand( | |
| Form form, | |
| LocalDateTime submittedAt, | |
| String name, | |
| String studentNumber, | |
| String department, | |
| List<CreateFormAnswerCommand> formAnswerCommands | |
| ) { | |
| import jakarta.validation.constraints.NotBlank; | |
| import jakarta.validation.constraints.NotNull; | |
| import jakarta.validation.constraints.Pattern; | |
| @Builder | |
| public record CreateFormResponseCommand( | |
| @NotNull(message = "폼은 필수입니다") | |
| Form form, | |
| @NotNull(message = "제출 시간은 필수입니다") | |
| LocalDateTime submittedAt, | |
| @NotBlank(message = "이름은 필수입니다") | |
| @Pattern(regexp = "^[가-힣]{2,10}$", message = "이름은 2-10자의 한글만 허용됩니다") | |
| String name, | |
| @NotBlank(message = "학번은 필수입니다") | |
| @Pattern(regexp = "^\\d{8}$", message = "학번은 8자리 숫자여야 합니다") | |
| String studentNumber, | |
| @NotBlank(message = "학과는 필수입니다") | |
| String department, | |
| @NotNull(message = "답변은 필수입니다") | |
| List<CreateFormAnswerCommand> formAnswerCommands | |
| ) { |
| CREATE TABLE form_answer | ||
| ( | ||
| id BIGINT NOT NULL AUTO_INCREMENT PRIMARY KEY, | ||
| value VARCHAR(255) NULL, | ||
| value_type VARCHAR(50) NOT NULL, | ||
| response_id BIGINT NOT NULL, | ||
| field_id BIGINT NOT NULL, | ||
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NULL, | ||
| updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP NULL, | ||
| CONSTRAINT fk_answer_response FOREIGN KEY (response_id) REFERENCES form_response (id) ON DELETE CASCADE, | ||
| CONSTRAINT fk_answer_field FOREIGN KEY (field_id) REFERENCES form_field (id) ON DELETE CASCADE | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
form_answer 테이블 개선 사항 확인 결과
value컬럼은 여전히VARCHAR(255)로 정의되어 있어, 긴 답변 저장을 위한TEXT타입 변경이 필요합니다.response_id및field_id컬럼에 대한 인덱스 생성 구문이 존재하지 않습니다.
해당 사항들을 반영하여 변경해주시기 바랍니다.
🔗 Analysis chain
form_answer 테이블의 외래 키 구성이 적절합니다.
CASCADE 삭제 옵션이 잘 적용되어 있습니다. 하지만 value 컬럼의 길이 제한을 검토해 볼 필요가 있습니다.
긴 답변을 저장할 수 있도록 TEXT 타입 사용을 고려해보세요:
- value VARCHAR(255) NULL,
+ value TEXT NULL,또한 조회 성능 향상을 위한 인덱스 추가가 필요합니다:
CREATE INDEX idx_form_answer_response ON form_answer(response_id);
CREATE INDEX idx_form_answer_field ON form_answer(field_id);검증 결과: form_answer 테이블 개선 사항 검토 필요
- form_answer 테이블의 외래키 구성과 CASCADE 삭제 옵션은 올바르게 적용되어 있습니다.
- 현재
value컬럼은 VARCHAR(255)로 정의되어 있어, 긴 답변을 저장하기에는 제약이 있으므로 TEXT 타입 사용을 고려해볼 필요가 있습니다. - 또한 조회 성능 향상을 위해 response_id 및 field_id 컬럼에 대해 인덱스 추가가 필요합니다.
해당 사항들을 검토 후 개선하시기 바랍니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
echo "---- form_answer 테이블의 'value' 컬럼(VARCHAR(255)) 정의 확인 ----"
grep -R "value[[:space:]]\+VARCHAR(255)" src/main/resources/db/migration/V35_create_form_response_and_answer_table.sql || echo "'value' 컬럼에 VARCHAR(255) 정의를 찾을 수 없습니다."
echo "---- form_answer 테이블의 'value' 컬럼(TEXT) 정의 확인 ----"
grep -R "value[[:space:]]\+TEXT" src/main/resources/db/migration/V35_create_form_response_and_answer_table.sql || echo "'value' 컬럼에 TEXT 정의를 찾을 수 없습니다."
echo "---- 인덱스 생성 구문 확인 (response_id 및 field_id) ----"
grep -R "CREATE INDEX" src/main/resources/db/migration/V35_create_form_response_and_answer_table.sql || echo "인덱스 생성 구문을 찾을 수 없습니다."
Length of output: 979
There was a problem hiding this comment.
코멘트가 좀 많은데 잘 확인해주세요~
그리고 조금 귀찮을 수 있는데, 하나의 패키지 안에서 같이 작업하는 건 충돌 위험이 너무 높아서, 먼저 작업 모두 완료하고 추후에 합치는 게 좋을 것 같습니다.
그래서 현재 아영님이 생성하신 클래스를 formapplication으로 패키지 하나 만들어서 옮겨주실 수 있을까요?
기존의 것인 FormFieldService, FormService, GeneralFormFieldService, GeneralFormService 이 4가지를 제외하고 모두 그냥 그대로 옮기면 될 것 같아요
| @Operation(summary = "지원하기 API") | ||
| @ApiResponse(responseCode = "201", description = "지원하기 성공") | ||
| @ResponseStatus(HttpStatus.CREATED) | ||
| @PostMapping |
There was a problem hiding this comment.
p1) REST API의 url을 적절하게 작성해주지 않은 것 같습니다
| @PostMapping | |
| @PostMapping("/forms/{formId}/applications") |
There was a problem hiding this comment.
확인하지 못했던 부분인데 짚어주셔서 감사합니다. 반영했습니다.
| public class UserFormController implements UserFormApi { | ||
|
|
||
| @Override | ||
| public void createFormResponse( | ||
| CreateFormResponseRequest createFormResponseRequest | ||
| ) { | ||
|
|
||
| } |
There was a problem hiding this comment.
p1)
facadeService를 의존받는 부분과 사용하는 메소드를 호출하는 부분이 없습니다...
There was a problem hiding this comment.
제가 덜 작성하고 pr을 올렸네요............ 감사합니다! 수정 완료했습니다.
| @ApiResponse(responseCode = "201", description = "지원하기 성공") | ||
| @ResponseStatus(HttpStatus.CREATED) | ||
| @PostMapping | ||
| void createFormResponse(@Valid @RequestBody CreateFormResponseRequest request); |
There was a problem hiding this comment.
| void createFormResponse(@Valid @RequestBody CreateFormResponseRequest request); | |
| void createFormResponse(@Valid @RequestBody CreateFormApplicationRequest request); |
p2)
현재 저희가 사용하고 있는 DTO 컨벤션인 ~Response 와 formResponse라는 엔터티 이름이 겹칩니다.
그래서 엔터티 이름을 formApplication으로 수정하는 것은 어떨까요?
There was a problem hiding this comment.
고민해봤던 부분인데 너무 좋습니다! 반영 완료했습니다!
| @Schema(description = "답변 값") | ||
| String value, |
There was a problem hiding this comment.
p2)
말씀해주신 것 처럼, 체크 박스는 여러 개 값을 가지기 때문에 List로 받아와야 할 것 같습니다
There was a problem hiding this comment.
StringListConverter 사용해서 수정했습니다!
| @ArraySchema(schema = @Schema(implementation = CreateFormAnswerRequest.class)) | ||
| List<CreateFormAnswerRequest> formAnswers | ||
| ) { | ||
| record CreateFormAnswerRequest( |
| @@ -19,4 +21,10 @@ public class GeneralFormFieldService implements FormFieldService { | |||
| public void createAll(List<FormField> formFields) { | |||
| formFieldRepository.saveAll(formFields); | |||
| } | |||
|
|
|||
| @Transactional | |||
There was a problem hiding this comment.
p1) 조회메소드인데 Transactional 어노테이션을 선언할 필요는 없습니다!
readOnly = true 설정이 어떤 역할 하시는지 알아보시면 좋을 것 같아요!!
There was a problem hiding this comment.
수정했습니다! 이해를 잘 하지 못하고 사용한 것 같습니다... 공부하도록 하겠습니다!
| @Override | ||
| public FormResponse create(FormResponse formResponse) { | ||
| return formResponseRepository.save(formResponse); | ||
| } |
There was a problem hiding this comment.
p1) 데이터 추가, 삭제, 수정에는 @Transactional을 선언해줘야합니다
| @Transactional | ||
| @Override | ||
| public Form getById(Long id) { | ||
| return formRepository.getById(id); | ||
| } |
There was a problem hiding this comment.
| @Transactional | |
| @Override | |
| public Form getById(Long id) { | |
| return formRepository.getById(id); | |
| } | |
| @Override | |
| public Form getById(Long id) { | |
| return formRepository.getById(id); | |
| } |
p1) 제거해주세요!
| CREATE TABLE form_answer | ||
| ( | ||
| id BIGINT NOT NULL AUTO_INCREMENT PRIMARY KEY, | ||
| value VARCHAR(255) NULL, |
There was a problem hiding this comment.
p3) Varchar(255)의 길이를 늘려야 할 것 같습니다.
LongText인 경우도 있기 때문입니다. 한 1000자? 아니면 1500자로?!
There was a problem hiding this comment.
p1)
해당 ddl sql문은 entity에서 설정한 것과 똑같아야 합니다.
아니면 에러 발생하여 개발서버가 다운됩니다.
예를 들면, form_id not Null 설정을 안했는데 ddl 스크립트에는 작성되어있어요.
다시 한번 확인해주세요
There was a problem hiding this comment.
명심했습니다. 감사합니다! 수정했습니다.
null값 방지를 위해서 초기화
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormFieldService.java (1)
14-14: @transactional 어노테이션 사용에 대한 제안클래스 레벨의
@Transactional(readOnly = true)는 읽기 전용 메서드가 많은 서비스에서 좋은 기본값으로 작용합니다.createAll메서드에서 개별적으로@Transactional을 지정한 것도 적절합니다.하지만 몇 가지 고려사항이 있습니다:
- readOnly = true의 장점:
- 성능 최적화
- 실수로 데이터가 변경되는 것을 방지
- 주의사항:
- 읽기 전용 트랜잭션이 정말 필요한지 검토
- 트랜잭션 범위가 너무 넓어지지 않도록 주의
Also applies to: 19-19
src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/GeneralFormApplicationService.java (1)
1-22: 퍼사드 패턴 구현에 대한 제안현재 서비스는 기본적인 CRUD 작업에 중점을 두고 있습니다. 퍼사드 패턴을 더 효과적으로 활용하기 위해 다음과 같은 개선을 고려해보세요:
- 복잡한 비즈니스 로직을 캡슐화
- 다른 서비스들과의 통합
- 트랜잭션 경계 명확화
예시 구조:
@Service public class FormApplicationFacade { private final FormApplicationService formApplicationService; private final FormFieldService formFieldService; private final FormService formService; @Transactional public FormApplication submitApplication(ApplicationCommand command) { Form form = formService.getById(command.getFormId()); List<FormField> fields = formFieldService.getAllByFormId(form.getId()); // 비즈니스 규칙 검증 validateApplication(command, fields); // FormApplication 생성 및 저장 FormApplication application = createApplication(command, form); return formApplicationService.create(application); } }src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FormApplicationService.java (1)
7-7: 인터페이스 메서드 시그니처 개선 제안서비스 인터페이스에서 직접 엔티티를 사용하면 구현 세부사항이 노출될 수 있습니다. DTO를 사용하여 도메인 계층을 보호하는 것이 좋습니다.
다음과 같이 DTO를 사용하는 것을 고려해보세요:
-FormApplication create(FormApplication formApplication); +FormApplicationDto create(CreateFormApplicationDto formApplicationDto);src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/GeneralFormAnswerService.java (1)
20-22: 벌크 작업 성능 최적화 및 모니터링 개선 필요대량의 데이터를 저장할 때 성능과 모니터링을 고려해야 합니다.
다음과 같은 개선사항을 제안합니다:
@Transactional @Override public void createAll(List<FormAnswer> formAnswers) { + log.info("Saving {} form answers", formAnswers.size()); + + // 배치 크기 설정으로 메모리 사용량 최적화 + int batchSize = 100; + for (int i = 0; i < formAnswers.size(); i += batchSize) { + int end = Math.min(formAnswers.size(), i + batchSize); + List<FormAnswer> batch = formAnswers.subList(i, end); formAnswerRepository.saveAll(batch); + log.debug("Saved batch {}/{}", end, formAnswers.size()); + } + + log.info("Successfully saved {} form answers", formAnswers.size()); }src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/entity/FormAnswer.java (1)
28-32: 연관관계 설정을 개선해야 합니다.FormApplication과 FormField와의 관계에서 cascade 설정이 누락되어 있습니다.
-@ManyToOne(fetch = FetchType.LAZY) +@ManyToOne(fetch = FetchType.LAZY, cascade = CascadeType.PERSIST) private FormApplication formApplication; -@ManyToOne(fetch = FetchType.LAZY) +@ManyToOne(fetch = FetchType.LAZY, optional = false) private FormField formField;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormFieldService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/api/UserFormApi.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/controller/UserFormController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/controller/dto/request/CreateFormApplicationRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/entity/FormAnswer.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/entity/FormApplication.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/entity/FormApplicationStatus.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/repository/FormAnswerRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/repository/FormApplicationRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FacadeUserFormService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FacadeUserFormServiceImpl.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FormAnswerService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FormApplicationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/GeneralFormAnswerService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/GeneralFormApplicationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/dto/CreateFormApplicationCommand.java(1 hunks)src/main/resources/db/migration/V35_create_form_response_and_answer_table.sql(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/entity/FormApplicationStatus.java
- src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/repository/FormApplicationRepository.java
- src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/repository/FormAnswerRepository.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/db/migration/V35_create_form_response_and_answer_table.sql
🔇 Additional comments (7)
src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormService.java (1)
22-25: 구현이 깔끔해 보입니다!클래스 레벨의
@Transactional(readOnly = true)어노테이션을 활용한 점이 좋습니다.src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormFieldService.java (1)
25-28: 구현이 깔끔하고 적절합니다!Optional을 사용하여 null 처리를 안전하게 하고 있으며, 리포지토리 메서드를 잘 활용하고 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/GeneralFormApplicationService.java (2)
9-12: 클래스 레벨 트랜잭션 설정이 적절합니다.읽기 전용 트랜잭션을 기본값으로 설정하고 필요한 메서드에만 쓰기 트랜잭션을 적용한 것이 좋은 설계입니다.
14-14: 의존성 주입이 적절히 구현되었습니다.
@RequiredArgsConstructor와 함께final필드를 사용하여 생성자 주입을 구현한 것이 좋습니다.src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FormAnswerService.java (1)
8-8: 🛠️ Refactor suggestion벌크 작업에 대한 유효성 검사 및 오류 처리 추가 필요
대량의 FormAnswer를 처리할 때 발생할 수 있는 문제를 고려해야 합니다:
- 빈 리스트 처리
- 부분 실패 시 롤백 전략
- 최대 허용 크기 제한
다음과 같은 개선을 제안합니다:
-void createAll(List<FormAnswer> formAnswers); +CreateFormAnswersResult createAll(List<FormAnswer> formAnswers); +@Value +public class CreateFormAnswersResult { + int successCount; + List<FormAnswerError> errors; +}src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/GeneralFormAnswerService.java (1)
13-13: readOnly 트랜잭션 설정 검토 필요클래스 레벨에서
@Transactional(readOnly = true)를 설정했지만, 현재는 쓰기 작업만 있습니다. 향후 읽기 작업이 추가될 예정이 아니라면 클래스 레벨의 readOnly 설정은 불필요할 수 있습니다.src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FacadeUserFormServiceImpl.java (1)
16-19: Facade 패턴이 잘 구현되었습니다.여러 서비스 간의 조율이 잘 이루어져 있으며, 트랜잭션 관리도 적절합니다.
| @Transactional | ||
| @Override | ||
| public FormApplication create(FormApplication formApplication) { | ||
| return formApplicationRepository.save(formApplication); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
서비스 메서드 구현에 대한 개선 제안
현재 구현은 기본적인 CRUD 작업만을 수행하고 있습니다. 다음 사항들을 고려해보시기 바랍니다:
- FormAnswer가 리스트 형태일 수 있다는 요구사항에 대한 검증 로직 추가
- 비즈니스 규칙 검증 (예: 필수 필드 확인)
- 예외 처리 강화
예시 구현:
@Transactional
@Override
public FormApplication create(FormApplication formApplication) {
+ validateFormApplication(formApplication);
return formApplicationRepository.save(formApplication);
}
+
+private void validateFormApplication(FormApplication formApplication) {
+ // 필수 필드 검증
+ if (formApplication.getAnswers() == null || formApplication.getAnswers().isEmpty()) {
+ throw new IllegalArgumentException("답변이 필요합니다");
+ }
+
+ // FormAnswer 검증
+ formApplication.getAnswers().forEach(this::validateFormAnswer);
+}
+
+private void validateFormAnswer(FormAnswer answer) {
+ if (answer.getValue() == null) {
+ throw new IllegalArgumentException("답변 값이 필요합니다");
+ }
+ // 리스트 형태의 답변에 대한 추가 검증 로직
+}|
|
||
| public interface FacadeUserFormService { | ||
|
|
||
| void createFormApplication(CreateFormApplicationCommand createFormApplicationCommand); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
예외 처리 명세 추가 필요
Facade 패턴 구현이 잘 되어있으나, 발생 가능한 예외 상황에 대한 명세가 필요합니다.
다음과 같이 발생 가능한 예외를 명시하는 것을 추천드립니다:
-void createFormApplication(CreateFormApplicationCommand createFormApplicationCommand);
+void createFormApplication(CreateFormApplicationCommand createFormApplicationCommand)
+ throws FormNotFoundException, InvalidFormAnswerException, DuplicateApplicationException;또한 각 예외 상황에 대한 문서화도 추가하면 좋을 것 같습니다:
/**
* 폼 신청을 생성합니다.
* @throws FormNotFoundException 폼이 존재하지 않는 경우
* @throws InvalidFormAnswerException 폼 답변이 유효하지 않은 경우
* @throws DuplicateApplicationException 이미 신청한 경우
*/| @Override | ||
| public void createFormResponse(CreateFormApplicationRequest createFormApplicationRequest) { | ||
|
|
||
| facadeUserFormService.createFormApplication(createFormApplicationRequest.toCommand()); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
예외 처리 및 응답 타입 개선이 필요합니다.
컨트롤러에서 다음 사항들을 고려해 주시기 바랍니다:
- 예외 상황에 대한 처리가 없습니다
- 성공/실패 응답 타입이 정의되어 있지 않습니다
다음과 같이 개선하는 것을 추천드립니다:
@Override
-public void createFormResponse(CreateFormApplicationRequest createFormApplicationRequest) {
+public ResponseEntity<ApiResponse> createFormResponse(CreateFormApplicationRequest createFormApplicationRequest) {
facadeUserFormService.createFormApplication(createFormApplicationRequest.toCommand());
+ return ResponseEntity.status(HttpStatus.CREATED)
+ .body(ApiResponse.success("지원서가 성공적으로 제출되었습니다."));
}📝 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 void createFormResponse(CreateFormApplicationRequest createFormApplicationRequest) { | |
| facadeUserFormService.createFormApplication(createFormApplicationRequest.toCommand()); | |
| } | |
| @Override | |
| public ResponseEntity<ApiResponse> createFormResponse(CreateFormApplicationRequest createFormApplicationRequest) { | |
| facadeUserFormService.createFormApplication(createFormApplicationRequest.toCommand()); | |
| return ResponseEntity.status(HttpStatus.CREATED) | |
| .body(ApiResponse.success("지원서가 성공적으로 제출되었습니다.")); | |
| } |
| @Operation(summary = "지원하기 API") | ||
| @ApiResponse(responseCode = "201", description = "지원하기 성공") | ||
| @ResponseStatus(HttpStatus.CREATED) | ||
| @PostMapping("/forms/{formId}/applications") | ||
| void createFormResponse(@Valid @RequestBody CreateFormApplicationRequest request); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
API 문서화 및 경로 변수 처리 개선이 필요합니다.
다음 사항들을 보완해 주시기 바랍니다:
- formId에 @PathVariable 어노테이션이 누락되었습니다
- API 오류 응답에 대한 문서화가 필요합니다
다음과 같이 개선하는 것을 추천드립니다:
@Operation(summary = "지원하기 API")
@ApiResponse(responseCode = "201", description = "지원하기 성공")
+@ApiResponse(responseCode = "400", description = "잘못된 요청")
+@ApiResponse(responseCode = "404", description = "폼을 찾을 수 없음")
@ResponseStatus(HttpStatus.CREATED)
@PostMapping("/forms/{formId}/applications")
-void createFormResponse(@Valid @RequestBody CreateFormApplicationRequest request);
+void createFormResponse(
+ @PathVariable Long formId,
+ @Valid @RequestBody CreateFormApplicationRequest request
+);📝 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.
| @Operation(summary = "지원하기 API") | |
| @ApiResponse(responseCode = "201", description = "지원하기 성공") | |
| @ResponseStatus(HttpStatus.CREATED) | |
| @PostMapping("/forms/{formId}/applications") | |
| void createFormResponse(@Valid @RequestBody CreateFormApplicationRequest request); | |
| @Operation(summary = "지원하기 API") | |
| @ApiResponse(responseCode = "201", description = "지원하기 성공") | |
| @ApiResponse(responseCode = "400", description = "잘못된 요청") | |
| @ApiResponse(responseCode = "404", description = "폼을 찾을 수 없음") | |
| @ResponseStatus(HttpStatus.CREATED) | |
| @PostMapping("/forms/{formId}/applications") | |
| void createFormResponse( | |
| @PathVariable Long formId, | |
| @Valid @RequestBody CreateFormApplicationRequest request | |
| ); |
| @Column(nullable = false) | ||
| private String value; | ||
|
|
||
| @Enumerated(EnumType.STRING) | ||
| @Column(nullable = false) | ||
| private FieldType valueType; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
필드 값 검증 로직이 필요합니다.
value 필드와 valueType 필드에 대한 유효성 검증이 누락되어 있습니다. 특히 valueType에 따른 value 값의 형식 검증이 필요합니다.
다음과 같이 검증 로직을 추가하는 것을 추천드립니다:
@Column(nullable = false)
private String value;
@Enumerated(EnumType.STRING)
@Column(nullable = false)
private FieldType valueType;
+
+private void validateValue() {
+ if (value == null || value.isBlank()) {
+ throw new IllegalArgumentException("답변 값은 필수입니다.");
+ }
+
+ switch (valueType) {
+ case NUMBER:
+ if (!value.matches("\\d+")) {
+ throw new IllegalArgumentException("숫자 형식이 아닙니다.");
+ }
+ break;
+ case EMAIL:
+ if (!value.matches("^[A-Za-z0-9+_.-]+@(.+)$")) {
+ throw new IllegalArgumentException("이메일 형식이 아닙니다.");
+ }
+ break;
+ }
+}📝 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.
| @Column(nullable = false) | |
| private String value; | |
| @Enumerated(EnumType.STRING) | |
| @Column(nullable = false) | |
| private FieldType valueType; | |
| @Column(nullable = false) | |
| private String value; | |
| @Enumerated(EnumType.STRING) | |
| @Column(nullable = false) | |
| private FieldType valueType; | |
| private void validateValue() { | |
| if (value == null || value.isBlank()) { | |
| throw new IllegalArgumentException("답변 값은 필수입니다."); | |
| } | |
| switch (valueType) { | |
| case NUMBER: | |
| if (!value.matches("\\d+")) { | |
| throw new IllegalArgumentException("숫자 형식이 아닙니다."); | |
| } | |
| break; | |
| case EMAIL: | |
| if (!value.matches("^[A-Za-z0-9+_.-]+@(.+)$")) { | |
| throw new IllegalArgumentException("이메일 형식이 아닙니다."); | |
| } | |
| break; | |
| } | |
| } |
|
|
||
| @Builder | ||
| public record CreateFormApplicationCommand( | ||
| Form form, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Form 객체를 직접 노출하는 것은 캡슐화를 위반할 수 있습니다.
Command 객체에서 Form 엔티티를 직접 참조하는 대신 formId를 사용하는 것이 좋습니다.
- Form form,
+ Long formId,📝 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.
| Form form, | |
| Long formId, |
| public FormApplication toEntity(Form form) { | ||
| return FormApplication.builder() | ||
| .name(name) | ||
| .studentNumber(studentNumber) | ||
| .department(department) | ||
| .status(status) | ||
| .form(form) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
엔티티 생성 시 유효성 검증이 필요합니다.
toEntity 메서드에서 필수 필드에 대한 유효성 검증을 추가하는 것이 좋습니다.
public FormApplication toEntity(Form form) {
+ if (name == null || name.isBlank()) {
+ throw new IllegalArgumentException("이름은 필수 입력 항목입니다.");
+ }
+ if (studentNumber == null || studentNumber.isBlank()) {
+ throw new IllegalArgumentException("학번은 필수 입력 항목입니다.");
+ }
+ if (department == null || department.isBlank()) {
+ throw new IllegalArgumentException("학과는 필수 입력 항목입니다.");
+ }
return FormApplication.builder()
.name(name)
.studentNumber(studentNumber)
.department(department)
.status(status)
.form(form)
.build();
}📝 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 FormApplication toEntity(Form form) { | |
| return FormApplication.builder() | |
| .name(name) | |
| .studentNumber(studentNumber) | |
| .department(department) | |
| .status(status) | |
| .form(form) | |
| .build(); | |
| } | |
| public FormApplication toEntity(Form form) { | |
| if (name == null || name.isBlank()) { | |
| throw new IllegalArgumentException("이름은 필수 입력 항목입니다."); | |
| } | |
| if (studentNumber == null || studentNumber.isBlank()) { | |
| throw new IllegalArgumentException("학번은 필수 입력 항목입니다."); | |
| } | |
| if (department == null || department.isBlank()) { | |
| throw new IllegalArgumentException("학과는 필수 입력 항목입니다."); | |
| } | |
| return FormApplication.builder() | |
| .name(name) | |
| .studentNumber(studentNumber) | |
| .department(department) | |
| .status(status) | |
| .form(form) | |
| .build(); | |
| } |
| @Transactional | ||
| @Override | ||
| public void createFormApplication(CreateFormApplicationCommand createFormApplicationCommand) { | ||
| Form form = formService.getById(createFormApplicationCommand.form().getId()); | ||
| FormApplication formApplication = createFormApplicationCommand.toEntity(form); | ||
| FormApplication savedFormApplication = formApplicationService.create(formApplication); | ||
|
|
||
| List<FormAnswer> formAnswers = toFormAnswers(savedFormApplication, createFormApplicationCommand.formAnswerCommands()); | ||
| formAnswerService.createAll(formAnswers); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
예외 처리를 개선할 필요가 있습니다.
서비스 계층에서 발생할 수 있는 예외들을 적절히 처리하고 의미 있는 비즈니스 예외로 변환해야 합니다.
@Transactional
@Override
public void createFormApplication(CreateFormApplicationCommand createFormApplicationCommand) {
+ try {
Form form = formService.getById(createFormApplicationCommand.form().getId());
FormApplication formApplication = createFormApplicationCommand.toEntity(form);
FormApplication savedFormApplication = formApplicationService.create(formApplication);
List<FormAnswer> formAnswers = toFormAnswers(savedFormApplication, createFormApplicationCommand.formAnswerCommands());
formAnswerService.createAll(formAnswers);
+ } catch (IllegalArgumentException e) {
+ throw new FormApplicationException("폼 신청 생성 중 오류가 발생했습니다: " + e.getMessage());
+ } catch (Exception e) {
+ throw new FormApplicationException("예상치 못한 오류가 발생했습니다.");
+ }
}📝 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.
| @Transactional | |
| @Override | |
| public void createFormApplication(CreateFormApplicationCommand createFormApplicationCommand) { | |
| Form form = formService.getById(createFormApplicationCommand.form().getId()); | |
| FormApplication formApplication = createFormApplicationCommand.toEntity(form); | |
| FormApplication savedFormApplication = formApplicationService.create(formApplication); | |
| List<FormAnswer> formAnswers = toFormAnswers(savedFormApplication, createFormApplicationCommand.formAnswerCommands()); | |
| formAnswerService.createAll(formAnswers); | |
| } | |
| @Transactional | |
| @Override | |
| public void createFormApplication(CreateFormApplicationCommand createFormApplicationCommand) { | |
| try { | |
| Form form = formService.getById(createFormApplicationCommand.form().getId()); | |
| FormApplication formApplication = createFormApplicationCommand.toEntity(form); | |
| FormApplication savedFormApplication = formApplicationService.create(formApplication); | |
| List<FormAnswer> formAnswers = toFormAnswers(savedFormApplication, createFormApplicationCommand.formAnswerCommands()); | |
| formAnswerService.createAll(formAnswers); | |
| } catch (IllegalArgumentException e) { | |
| throw new FormApplicationException("폼 신청 생성 중 오류가 발생했습니다: " + e.getMessage()); | |
| } catch (Exception e) { | |
| throw new FormApplicationException("예상치 못한 오류가 발생했습니다."); | |
| } | |
| } |
| public record CreateFormApplicationRequest( | ||
|
|
||
| @NotNull(message = "지원자 이름은 필수 입력 사항입니다.") | ||
| @Schema(description = "지원자 이름", example = "김띵동") | ||
| String name, | ||
|
|
||
| @NotNull(message = "지원자 학번은 필수 입력 사항입니다.") | ||
| @Schema(description = "학번", example = "60200000") | ||
| String studentNumber, | ||
|
|
||
| @NotNull(message = "지원자 학과는 필수 입력 사항입니다.") | ||
| @Schema(description = "학과", example = "융합소프트웨어학부 응용소프트웨어전공") | ||
| String department, | ||
|
|
||
| @ArraySchema(schema = @Schema(implementation = CreateFormAnswerRequest.class)) | ||
| List<CreateFormAnswerRequest> formAnswers | ||
| ) { |
There was a problem hiding this comment.
폼 ID가 누락되었습니다.
폼 신청을 생성하기 위해서는 어떤 폼에 대한 신청인지 식별할 수 있는 formId가 필요합니다.
public record CreateFormApplicationRequest(
+ @NotNull(message = "폼 ID는 필수 입력 사항입니다.")
+ @Schema(description = "폼 ID", example = "1")
+ Long formId,
@NotNull(message = "지원자 이름은 필수 입력 사항입니다.")
@Schema(description = "지원자 이름", example = "김띵동")
String name,📝 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 record CreateFormApplicationRequest( | |
| @NotNull(message = "지원자 이름은 필수 입력 사항입니다.") | |
| @Schema(description = "지원자 이름", example = "김띵동") | |
| String name, | |
| @NotNull(message = "지원자 학번은 필수 입력 사항입니다.") | |
| @Schema(description = "학번", example = "60200000") | |
| String studentNumber, | |
| @NotNull(message = "지원자 학과는 필수 입력 사항입니다.") | |
| @Schema(description = "학과", example = "융합소프트웨어학부 응용소프트웨어전공") | |
| String department, | |
| @ArraySchema(schema = @Schema(implementation = CreateFormAnswerRequest.class)) | |
| List<CreateFormAnswerRequest> formAnswers | |
| ) { | |
| public record CreateFormApplicationRequest( | |
| @NotNull(message = "폼 ID는 필수 입력 사항입니다.") | |
| @Schema(description = "폼 ID", example = "1") | |
| Long formId, | |
| @NotNull(message = "지원자 이름은 필수 입력 사항입니다.") | |
| @Schema(description = "지원자 이름", example = "김띵동") | |
| String name, | |
| @NotNull(message = "지원자 학번은 필수 입력 사항입니다.") | |
| @Schema(description = "학번", example = "60200000") | |
| String studentNumber, | |
| @NotNull(message = "지원자 학과는 필수 입력 사항입니다.") | |
| @Schema(description = "학과", example = "융합소프트웨어학부 응용소프트웨어전공") | |
| String department, | |
| @ArraySchema(schema = @Schema(implementation = CreateFormAnswerRequest.class)) | |
| List<CreateFormAnswerRequest> formAnswers | |
| ) { |
| public CreateFormApplicationCommand toCommand() { | ||
| List<CreateFormAnswerCommand> createFormAnswerCommands = formAnswers.stream() | ||
| .map(CreateFormAnswerRequest::toCommand) | ||
| .toList(); | ||
| return CreateFormApplicationCommand.builder() | ||
| .name(name) | ||
| .studentNumber(studentNumber) | ||
| .department(department) | ||
| .status(FormApplicationStatus.SUBMITTED) | ||
| .formAnswerCommands(createFormAnswerCommands) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
toCommand 메서드에서 Form 객체 생성이 누락되었습니다.
Command 객체 생성 시 Form 정보가 포함되어야 합니다.
public CreateFormApplicationCommand toCommand() {
List<CreateFormAnswerCommand> createFormAnswerCommands = formAnswers.stream()
.map(CreateFormAnswerRequest::toCommand)
.toList();
return CreateFormApplicationCommand.builder()
+ .formId(formId)
.name(name)
.studentNumber(studentNumber)
.department(department)
.status(FormApplicationStatus.SUBMITTED)
.formAnswerCommands(createFormAnswerCommands)
.build();
}📝 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 CreateFormApplicationCommand toCommand() { | |
| List<CreateFormAnswerCommand> createFormAnswerCommands = formAnswers.stream() | |
| .map(CreateFormAnswerRequest::toCommand) | |
| .toList(); | |
| return CreateFormApplicationCommand.builder() | |
| .name(name) | |
| .studentNumber(studentNumber) | |
| .department(department) | |
| .status(FormApplicationStatus.SUBMITTED) | |
| .formAnswerCommands(createFormAnswerCommands) | |
| .build(); | |
| } | |
| public CreateFormApplicationCommand toCommand() { | |
| List<CreateFormAnswerCommand> createFormAnswerCommands = formAnswers.stream() | |
| .map(CreateFormAnswerRequest::toCommand) | |
| .toList(); | |
| return CreateFormApplicationCommand.builder() | |
| .formId(formId) | |
| .name(name) | |
| .studentNumber(studentNumber) | |
| .department(department) | |
| .status(FormApplicationStatus.SUBMITTED) | |
| .formAnswerCommands(createFormAnswerCommands) | |
| .build(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormFieldService.java (1)
30-34: 예외 메시지를 더 구체적으로 개선해보세요.예외 메시지에 field의 구체적인 정보를 포함하면 디버깅에 도움이 될 것 같습니다.
다음과 같이 수정하는 것을 제안합니다:
- .orElseThrow(() -> new IllegalArgumentException("해당 field를 id로 찾을 수 없습니다: " + id)); + .orElseThrow(() -> new IllegalArgumentException(String.format("FormField를 찾을 수 없습니다. (id: %d)", id)));src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/entity/FormAnswer.java (2)
33-38: 양방향 연관관계 설정이 필요할 수 있습니다.FormApplication에서 FormAnswer 목록에 접근해야 하는 경우가 있을 수 있습니다. 양방향 연관관계 설정을 고려해보시기 바랍니다.
예시:
@OneToMany(mappedBy = "formApplication", cascade = CascadeType.ALL, orphanRemoval = true) private List<FormAnswer> answers = new ArrayList<>(); // 연관관계 편의 메서드 public void addAnswer(FormAnswer answer) { this.answers.add(answer); answer.setFormApplication(this); }
1-41: 도메인 이벤트 발행 기능 추가를 고려해보세요.폼 답변이 생성될 때 이를 구독하는 다른 도메인이 있을 수 있습니다. 도메인 이벤트를 발행하는 기능을 추가하는 것이 좋습니다.
예시:
@Getter public class FormAnswerCreatedEvent { private final Long formAnswerId; private final Long formApplicationId; private final Long formFieldId; public FormAnswerCreatedEvent(FormAnswer formAnswer) { this.formAnswerId = formAnswer.getId(); this.formApplicationId = formAnswer.getFormApplication().getId(); this.formFieldId = formAnswer.getFormField().getId(); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormFieldService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormFieldService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/controller/dto/request/CreateFormApplicationRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/entity/FormAnswer.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FacadeUserFormServiceImpl.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/dto/CreateFormApplicationCommand.java(1 hunks)src/main/resources/db/migration/V35_create_form_response_and_answer_table.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FacadeUserFormServiceImpl.java
- src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/dto/CreateFormApplicationCommand.java
- src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/controller/dto/request/CreateFormApplicationRequest.java
- src/main/resources/db/migration/V35_create_form_response_and_answer_table.sql
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (3)
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormFieldService.java (1)
11-13: 인터페이스 설계가 잘 되었습니다!Optional을 활용한
findById와 필수 값을 반환하는getById메서드를 분리한 것이 좋은 설계 방식입니다. 이는 Java의 표준 패턴을 잘 따르고 있으며, 클라이언트 코드에서 존재 여부에 따른 처리를 명확하게 할 수 있게 해줍니다.src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormFieldService.java (1)
25-28: 구현이 깔끔합니다!
findById메서드의 구현이 간단하고 명확합니다. 또한 이전 리뷰 피드백을 반영하여 조회 메서드에 불필요한 @transactional 어노테이션을 추가하지 않은 점이 좋습니다.src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/entity/FormAnswer.java (1)
23-25: value 필드의 유효성 검증이 필요합니다.이전 리뷰에서 언급된 value 필드의 유효성 검증이 아직 구현되지 않았습니다.
| @ManyToOne(fetch = FetchType.LAZY) | ||
| private FormApplication formApplication; | ||
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| private FormField formField; |
There was a problem hiding this comment.
연관 관계의 무결성 보장이 필요합니다.
FormApplication과 FormField에 대한 null 체크가 누락되어 있습니다. 엔티티 생성 시점에 이를 검증하는 것이 좋습니다.
다음과 같이 생성자에 검증 로직을 추가하는 것을 추천드립니다:
@Builder
private FormAnswer(List<String> value, FormApplication formApplication, FormField formField) {
+ validateRelations(formApplication, formField);
this.value = value;
this.formApplication = formApplication;
this.formField = formField;
}
+private void validateRelations(FormApplication formApplication, FormField formField) {
+ if (formApplication == null) {
+ throw new IllegalArgumentException("FormApplication은 null일 수 없습니다.");
+ }
+ if (formField == null) {
+ throw new IllegalArgumentException("FormField는 null일 수 없습니다.");
+ }
+}📝 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.
| @ManyToOne(fetch = FetchType.LAZY) | |
| private FormApplication formApplication; | |
| @ManyToOne(fetch = FetchType.LAZY) | |
| private FormField formField; | |
| @Entity | |
| @Getter | |
| public class FormAnswer { | |
| @ManyToOne(fetch = FetchType.LAZY) | |
| private FormApplication formApplication; | |
| @ManyToOne(fetch = FetchType.LAZY) | |
| private FormField formField; | |
| private List<String> value; | |
| @Builder | |
| private FormAnswer(List<String> value, FormApplication formApplication, FormField formField) { | |
| validateRelations(formApplication, formField); | |
| this.value = value; | |
| this.formApplication = formApplication; | |
| this.formField = formField; | |
| } | |
| private void validateRelations(FormApplication formApplication, FormField formField) { | |
| if (formApplication == null) { | |
| throw new IllegalArgumentException("FormApplication은 null일 수 없습니다."); | |
| } | |
| if (formField == null) { | |
| throw new IllegalArgumentException("FormField는 null일 수 없습니다."); | |
| } | |
| } | |
| // ... other methods and fields ... | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/api/UserFormApi.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/controller/UserFormController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FacadeUserFormService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FacadeUserFormServiceImpl.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplicaion/service/FacadeUserFormService.java (1)
5-7: 🛠️ Refactor suggestion메서드 문서화 및 예외 명세가 필요합니다.
Facade 인터페이스의 메서드에 대한 문서화와 발생 가능한 예외를 명시해 주세요.
다음과 같이 개선하는 것을 추천드립니다:
+/** + * 폼 신청을 생성합니다. + * @param formId 폼 ID + * @param createFormApplicationCommand 폼 신청 생성 커맨드 + * @throws FormNotFoundException 폼을 찾을 수 없는 경우 + * @throws FormFieldNotFoundException 폼 필드를 찾을 수 없는 경우 + * @throws InvalidFormAnswerException 폼 답변이 유효하지 않은 경우 + */ -void createFormApplication(Long formId, CreateFormApplicationCommand createFormApplicationCommand); +void createFormApplication(Long formId, CreateFormApplicationCommand createFormApplicationCommand) + throws FormNotFoundException, FormFieldNotFoundException, InvalidFormAnswerException;Likely invalid or redundant comment.
| @Override | ||
| public void createFormResponse(Long formId, CreateFormApplicationRequest createFormApplicationRequest) { | ||
| facadeUserFormService.createFormApplication(formId, createFormApplicationRequest.toCommand()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
응답 타입과 검증 로직이 필요합니다.
컨트롤러에서 다음 사항들을 개선해 주시기 바랍니다:
- 성공/실패에 대한 응답 타입이 정의되어 있지 않습니다
- formId에 대한 검증이 누락되었습니다
다음과 같이 개선하는 것을 추천드립니다:
-public void createFormResponse(Long formId, CreateFormApplicationRequest createFormApplicationRequest) {
+public ResponseEntity<ApiResponse> createFormResponse(
+ @PathVariable @Positive Long formId,
+ @Valid @RequestBody CreateFormApplicationRequest createFormApplicationRequest
+) {
facadeUserFormService.createFormApplication(formId, createFormApplicationRequest.toCommand());
+ return ResponseEntity.status(HttpStatus.CREATED)
+ .body(ApiResponse.success("폼 신청이 성공적으로 생성되었습니다."));
}📝 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 void createFormResponse(Long formId, CreateFormApplicationRequest createFormApplicationRequest) { | |
| facadeUserFormService.createFormApplication(formId, createFormApplicationRequest.toCommand()); | |
| } | |
| @Override | |
| public ResponseEntity<ApiResponse> createFormResponse( | |
| @PathVariable @Positive Long formId, | |
| @Valid @RequestBody CreateFormApplicationRequest createFormApplicationRequest | |
| ) { | |
| facadeUserFormService.createFormApplication(formId, createFormApplicationRequest.toCommand()); | |
| return ResponseEntity.status(HttpStatus.CREATED) | |
| .body(ApiResponse.success("폼 신청이 성공적으로 생성되었습니다.")); | |
| } |
| @Tag(name = "Form - User", description = "User Form API") | ||
| @RequestMapping("/server") | ||
| public interface UserFormApi { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
API 문서화 개선이 필요합니다.
API 문서화와 경로에 대해 다음 사항들을 개선해 주시기 바랍니다:
- 오류 응답에 대한 문서화가 누락되었습니다
- "/server" 기본 경로가 RESTful 관행에 맞지 않습니다
다음과 같이 개선하는 것을 추천드립니다:
@Tag(name = "Form - User", description = "User Form API")
-@RequestMapping("/server")
+@RequestMapping("/api/v1")
public interface UserFormApi {그리고 createFormResponse 메서드에 다음 문서화를 추가해 주세요:
@Operation(summary = "지원하기 API")
@ApiResponse(responseCode = "201", description = "지원하기 성공")
+@ApiResponse(responseCode = "400", description = "잘못된 요청")
+@ApiResponse(responseCode = "404", description = "폼을 찾을 수 없음")
+@ApiResponse(responseCode = "422", description = "폼 답변이 유효하지 않음")Committable suggestion skipped: line range outside the PR's diff.
| private List<FormAnswer> toFormAnswers(FormApplication savedFormApplication, List<CreateFormAnswerCommand> createFormAnswerCommands) { | ||
| return createFormAnswerCommands.stream() | ||
| .map(formAnswerCommand -> { | ||
| FormField formField = formFieldService.getById(formAnswerCommand.fieldId()); | ||
| return formAnswerCommand.toEntity(savedFormApplication, formField); | ||
| }) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
폼 필드 유효성 검증이 필요합니다.
toFormAnswers 메서드에서 폼 필드와 답변 값의 유효성을 검증해야 합니다.
다음과 같이 개선하는 것을 추천드립니다:
private List<FormAnswer> toFormAnswers(FormApplication savedFormApplication, List<CreateFormAnswerCommand> createFormAnswerCommands) {
return createFormAnswerCommands.stream()
.map(formAnswerCommand -> {
FormField formField = formFieldService.getById(formAnswerCommand.fieldId());
+ validateAnswerValue(formField, formAnswerCommand.value());
return formAnswerCommand.toEntity(savedFormApplication, formField);
})
.toList();
}
+private void validateAnswerValue(FormField formField, String value) {
+ if (formField.isRequired() && (value == null || value.trim().isEmpty())) {
+ throw new InvalidFormAnswerException("필수 항목에 답변이 누락되었습니다.");
+ }
+
+ if (!formField.isValidValue(value)) {
+ throw new InvalidFormAnswerException("답변 형식이 올바르지 않습니다.");
+ }
+}📝 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.
| private List<FormAnswer> toFormAnswers(FormApplication savedFormApplication, List<CreateFormAnswerCommand> createFormAnswerCommands) { | |
| return createFormAnswerCommands.stream() | |
| .map(formAnswerCommand -> { | |
| FormField formField = formFieldService.getById(formAnswerCommand.fieldId()); | |
| return formAnswerCommand.toEntity(savedFormApplication, formField); | |
| }) | |
| .toList(); | |
| } | |
| private List<FormAnswer> toFormAnswers(FormApplication savedFormApplication, List<CreateFormAnswerCommand> createFormAnswerCommands) { | |
| return createFormAnswerCommands.stream() | |
| .map(formAnswerCommand -> { | |
| FormField formField = formFieldService.getById(formAnswerCommand.fieldId()); | |
| validateAnswerValue(formField, formAnswerCommand.value()); | |
| return formAnswerCommand.toEntity(savedFormApplication, formField); | |
| }) | |
| .toList(); | |
| } | |
| private void validateAnswerValue(FormField formField, String value) { | |
| if (formField.isRequired() && (value == null || value.trim().isEmpty())) { | |
| throw new InvalidFormAnswerException("필수 항목에 답변이 누락되었습니다."); | |
| } | |
| if (!formField.isValidValue(value)) { | |
| throw new InvalidFormAnswerException("답변 형식이 올바르지 않습니다."); | |
| } | |
| } |
| public void createFormApplication(Long formId, CreateFormApplicationCommand createFormApplicationCommand) { | ||
| Form form = formService.getById(formId); | ||
| FormApplication formApplication = createFormApplicationCommand.toEntity(form); | ||
| FormApplication savedFormApplication = formApplicationService.create(formApplication); | ||
|
|
||
| List<FormAnswer> formAnswers = toFormAnswers(savedFormApplication, createFormApplicationCommand.formAnswerCommands()); | ||
| formAnswerService.createAll(formAnswers); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
비즈니스 검증 로직과 예외 처리가 필요합니다.
Facade 서비스에서 다음 사항들을 개선해 주시기 바랍니다:
- 폼 답변의 유효성 검증이 누락되었습니다
- 예외 처리가 누락되었습니다
다음과 같이 개선하는 것을 추천드립니다:
@Transactional
@Override
public void createFormApplication(Long formId, CreateFormApplicationCommand createFormApplicationCommand) {
+ try {
Form form = formService.getById(formId);
+ validateFormStatus(form);
+ validateFormAnswers(form, createFormApplicationCommand.formAnswerCommands());
FormApplication formApplication = createFormApplicationCommand.toEntity(form);
FormApplication savedFormApplication = formApplicationService.create(formApplication);
List<FormAnswer> formAnswers = toFormAnswers(savedFormApplication, createFormApplicationCommand.formAnswerCommands());
formAnswerService.createAll(formAnswers);
+ } catch (FormNotFoundException | FormFieldNotFoundException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new FormApplicationException("폼 신청 생성 중 오류가 발생했습니다: " + e.getMessage());
+ }
}
+private void validateFormStatus(Form form) {
+ if (!form.isAcceptingApplications()) {
+ throw new InvalidFormStatusException("현재 신청이 불가능한 폼입니다.");
+ }
+}
+private void validateFormAnswers(Form form, List<CreateFormAnswerCommand> answers) {
+ // 필수 답변 검증
+ Set<Long> requiredFieldIds = form.getRequiredFieldIds();
+ Set<Long> answeredFieldIds = answers.stream()
+ .map(CreateFormAnswerCommand::fieldId)
+ .collect(Collectors.toSet());
+
+ if (!answeredFieldIds.containsAll(requiredFieldIds)) {
+ throw new InvalidFormAnswerException("필수 항목이 모두 답변되지 않았습니다.");
+ }
+}📝 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 void createFormApplication(Long formId, CreateFormApplicationCommand createFormApplicationCommand) { | |
| Form form = formService.getById(formId); | |
| FormApplication formApplication = createFormApplicationCommand.toEntity(form); | |
| FormApplication savedFormApplication = formApplicationService.create(formApplication); | |
| List<FormAnswer> formAnswers = toFormAnswers(savedFormApplication, createFormApplicationCommand.formAnswerCommands()); | |
| formAnswerService.createAll(formAnswers); | |
| } | |
| @Transactional | |
| @Override | |
| public void createFormApplication(Long formId, CreateFormApplicationCommand createFormApplicationCommand) { | |
| try { | |
| Form form = formService.getById(formId); | |
| validateFormStatus(form); | |
| validateFormAnswers(form, createFormApplicationCommand.formAnswerCommands()); | |
| FormApplication formApplication = createFormApplicationCommand.toEntity(form); | |
| FormApplication savedFormApplication = formApplicationService.create(formApplication); | |
| List<FormAnswer> formAnswers = toFormAnswers(savedFormApplication, createFormApplicationCommand.formAnswerCommands()); | |
| formAnswerService.createAll(formAnswers); | |
| } catch (FormNotFoundException | FormFieldNotFoundException e) { | |
| throw e; | |
| } catch (Exception e) { | |
| throw new FormApplicationException("폼 신청 생성 중 오류가 발생했습니다: " + e.getMessage()); | |
| } | |
| } | |
| private void validateFormStatus(Form form) { | |
| if (!form.isAcceptingApplications()) { | |
| throw new InvalidFormStatusException("현재 신청이 불가능한 폼입니다."); | |
| } | |
| } | |
| private void validateFormAnswers(Form form, List<CreateFormAnswerCommand> answers) { | |
| // 필수 답변 검증 | |
| Set<Long> requiredFieldIds = form.getRequiredFieldIds(); | |
| Set<Long> answeredFieldIds = answers.stream() | |
| .map(CreateFormAnswerCommand::fieldId) | |
| .collect(Collectors.toSet()); | |
| if (!answeredFieldIds.containsAll(requiredFieldIds)) { | |
| throw new InvalidFormAnswerException("필수 항목이 모두 답변되지 않았습니다."); | |
| } | |
| } |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeUserFormServiceImplTest.java (3)
28-50: 테스트 설정 최적화 제안현재 테스트가
@SpringBootTest를 사용하여 전체 애플리케이션 컨텍스트를 로드하고 있습니다. 서비스 계층 테스트의 경우, 다음과 같은 최적화를 고려해보세요:
@SpringBootTest대신@DataJpaTest나@WebMvcTest와 같은 더 가벼운 테스트 어노테이션 사용- 필요한 빈만 선택적으로 로드하여 테스트 실행 시간 단축
69-70: 중복된 저장 작업 제거69번 라인에서 club이 중복 저장되고 있습니다:
-clubRepository.save(club); -Club savedClub = clubRepository.save(club); +Club savedClub = clubRepository.save(club);
54-81: 테스트 데이터 설정 리팩토링 제안테스트 데이터 설정 코드가 너무 길어 가독성이 떨어집니다. 다음과 같은 개선을 제안합니다:
- 테스트 데이터 생성을 별도의 헬퍼 메소드로 분리
- 테스트 픽스처 클래스 도입
- 빌더 패턴을 활용한 더 명확한 테스트 데이터 설정
예시 코드:
@TestInstance(TestInstance.Lifecycle.PER_CLASS) class TestFixtures { private final FixtureMonkey fixtureMonkey = FixtureMonkeyFactory.getNotNullBuilderIntrospectorMonkey(); public User createTestUser() { return fixtureMonkey.giveMeBuilder(User.class) .set("id", 1L) .set("Role", Role.CLUB) .set("deletedAt", null) .sample(); } public Club createTestClub(User user) { return fixtureMonkey.giveMeBuilder(Club.class) .set("id", 1L) .set("user", user) .set("score", null) .set("clubMembers", null) .set("deletedAt", null) .sample(); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeUserFormServiceImplTest.java(1 hunks)
| List<FormApplication> formApplications = formApplicationRepository.findAll(); | ||
| List<FormAnswer> formAnswers = formAnswerRepository.findAll(); | ||
|
|
||
| assertThat(formApplications).isNotEmpty(); | ||
| assertThat(formAnswers).isNotEmpty(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
구체적인 검증 로직 추가 필요
현재 테스트는 단순히 리스트가 비어있지 않은지만 확인하고 있습니다. 다음과 같은 구체적인 검증을 추가하는 것이 좋습니다:
- 생성된 FormApplication의 정확한 개수 확인
- FormAnswer의 데이터 정확성 검증
- Form과 FormApplication 간의 연관관계 검증
예시 코드:
-assertThat(formApplications).isNotEmpty();
-assertThat(formAnswers).isNotEmpty();
+assertThat(formApplications).hasSize(1);
+FormApplication createdApplication = formApplications.get(0);
+assertThat(createdApplication.getForm().getId()).isEqualTo(savedForm.getId());
+assertThat(formAnswers).allSatisfy(answer ->
+ assertThat(answer.getFormApplication()).isEqualTo(createdApplication)
+);📝 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.
| List<FormApplication> formApplications = formApplicationRepository.findAll(); | |
| List<FormAnswer> formAnswers = formAnswerRepository.findAll(); | |
| assertThat(formApplications).isNotEmpty(); | |
| assertThat(formAnswers).isNotEmpty(); | |
| List<FormApplication> formApplications = formApplicationRepository.findAll(); | |
| List<FormAnswer> formAnswers = formAnswerRepository.findAll(); | |
| - assertThat(formApplications).isNotEmpty(); | |
| - assertThat(formAnswers).isNotEmpty(); | |
| + assertThat(formApplications).hasSize(1); | |
| + FormApplication createdApplication = formApplications.get(0); | |
| + assertThat(createdApplication.getForm().getId()).isEqualTo(savedForm.getId()); | |
| + assertThat(formAnswers).allSatisfy(answer -> | |
| + assertThat(answer.getFormApplication()).isEqualTo(createdApplication) | |
| + ); |
🚀 작업 내용
지원하기 API(FormResponse 생성 API) 구현하였습니다.
🤔 고민했던 내용
FormField의 id를 받아오는 부분을 고민했습니다... 객체를 받아오려 했다가 마지막에 id를 받아오도록 수정하였습니다.
FormAnswer의 value가 리스트가 될 수도 있을텐데 이 부분을 아직 수정하지 못했습니다. 빠르게 수정하겠습니다.
테스트 코드 또한 빠르게 작성하도록 하겠습니다.
💬 리뷰 중점사항
파사드 패턴을 처음 적용해봐서... 서비스를 제대로 구현했는지에 대해서 자신이 없습니다.
부족한 부분이 많을 것 같습니다...
Summary by CodeRabbit