[DDING-95] 지원자 상세 조회 API 구현 #240
Conversation
Seooooo24
requested review from
wonjunYou,
5uhwann and
KoSeonJe
as code owners
|
""" Walkthrough이번 PR은 폼 어플리케이션의 세부 정보를 조회하는 기능을 추가합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as CentralFormApplicationController
participant Service as FacadeCentralFormApplicationServiceImpl
participant Repo as FormAnswerRepository
participant DTO as FormApplicationResponse
Client->>Controller: GET /forms/{formId}/applications/{applicationId}
Controller->>Service: getFormApplication(formId, applicationId, principal)
Service->>Repo: findAllByFormApplication(formApplication)
Repo-->>Service: List<FormAnswer>
Service-->>Controller: FormApplicationQuery
Controller->>DTO: FormApplicationResponse.from(query)
DTO-->>Client: FormApplicationResponse
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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: 3
🧹 Nitpick comments (5)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormAnswerService.java (1)
25-28: 트랜잭션 처리와 null 체크가 필요합니다.다음 사항들을 고려해 주세요:
- 메서드에
@Transactional(readOnly = true)어노테이션을 명시적으로 추가하는 것이 좋습니다.- formApplication이 null인 경우에 대한 처리가 필요합니다.
다음과 같이 수정하는 것을 제안합니다:
@Override + @Transactional(readOnly = true) public List<FormAnswer> getAllByApplication(FormApplication formApplication) { + if (formApplication == null) { + throw new IllegalArgumentException("FormApplication must not be null"); + } return formAnswerRepository.findAllByFormApplication(formApplication); }src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java (1)
11-12: 메서드 문서화 및 예외 처리 명세 필요새로 추가된
getById메서드에 대한 JavaDoc 문서화가 필요합니다. 특히 다음 사항들을 명시해주세요:
- 메서드의 목적
- 파라미터 설명
- 반환값 설명
- 발생 가능한 예외 상황
+ /** + * 지정된 ID로 폼 어플리케이션을 조회합니다. + * + * @param applicationId 조회할 폼 어플리케이션의 ID + * @return 조회된 폼 어플리케이션 + * @throws EntityNotFoundException 해당 ID의 폼 어플리케이션이 존재하지 않는 경우 + */ FormApplication getById(Long applicationId);src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/query/FormApplicationQuery.java (1)
33-39: 정적 팩토리 메서드의 의도 명시
from(FormAnswer formAnswer)는FormAnswer엔티티를 레코드로 매핑하는 로직을 담당합니다. 추가적으로 formAnswer가null이거나value가 비어있는 경우의 예외 처리를 고려하면 안정성을 높일 수 있습니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/CentralFormApplicationController.java (1)
27-32: 추가적인 예외 처리 제안
getFormApplication메서드가 반환하는FormApplicationResponse가 내부적으로null필드나 올바르지 않은 데이터에 의해 문제를 일으키지 않도록, 예외 케이스나 에러 응답에 대한 로직도 고려해 보시면 좋겠습니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java (1)
55-66: 트랜잭션 범위와 성능 최적화를 검토해주세요.
- 현재 메서드가 여러 서비스를 호출하고 있어 성능에 영향을 미칠 수 있습니다.
- readOnly 트랜잭션 내에서 여러 조회 작업이 수행되므로, N+1 문제가 발생할 수 있습니다.
다음과 같은 개선을 고려해보세요:
- 필요한 데이터를 한 번의 쿼리로 조회하도록 리포지토리 계층에서 최적화
- 페치 조인을 사용하여 N+1 문제 해결
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/FormQuery.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/CentralFormApplicationApi.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/CentralFormApplicationController.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/response/FormApplicationResponse.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormAnswerService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormAnswerService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/query/FormApplicationQuery.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (8)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormAnswerService.java (1)
12-12: 메서드 시그니처가 명확하고 적절합니다!FormApplication에 대한 모든 FormAnswer를 조회하는 메서드의 이름과 반환 타입이 명확하게 정의되어 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepository.java (1)
10-10: Spring Data JPA 명명 규칙을 잘 따르고 있습니다!findAllByFormApplication 메서드는 Spring Data JPA의 메서드 명명 규칙을 정확히 따르고 있으며, 서비스 계층의 요구사항과 잘 매칭됩니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/query/FormApplicationQuery.java (1)
19-27: 레코드 필드 구성의 명확성 확인
레코드 정의와 빌더 패턴을 함께 사용한 것은 가독성과 유지보수에 유리합니다. 다만FormApplicationQuery의 각 필드가 누락 없이 정확히 초기화되는지 확인하여, 예상치 못한 누락으로 인한 버그가 발생하지 않도록 주의해 주세요.src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationService.java (1)
11-11: 권한 검증 로직 주의
getFormApplication(Long formId, Long applicationId, User user)메서드는 사용자의 접근 권한 검증이 중요한 부분이므로, 실제 구현에서 사용자 정보와 신청서 간 연관 관계를 꼼꼼히 확인하고 예외 상황을 처리해야 합니다.src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/FormQuery.java (1)
23-23: LGTM! FormFieldListQuery에 식별자 필드가 잘 추가되었습니다.폼 필드를 고유하게 식별할 수 있는 id 필드가 적절하게 추가되었고, from 메서드에서도 올바르게 매핑되고 있습니다.
Also applies to: 33-33
src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/CentralFormApplicationApi.java (1)
35-45: API 명세가 잘 정의되었습니다.보안 요구사항, 응답 상태, 경로 변수 등이 적절하게 정의되어 있습니다. OpenAPI 어노테이션을 통한 문서화도 잘 되어있습니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java (1)
32-33: 의존성 주입이 적절히 이루어졌습니다.FormAnswerService와 FormFieldService가 final로 선언되어 불변성이 보장됩니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/response/FormApplicationResponse.java (1)
17-47: API 문서화가 잘 되어있습니다.각 필드에 대한 설명, 예시, 허용 값 등이 OpenAPI 어노테이션을 통해 상세하게 문서화되어 있습니다.
| @Override | ||
| public FormApplication getById(Long applicationId) { | ||
| return formApplicationRepository.findById(applicationId).orElse(null); | ||
| } |
There was a problem hiding this comment.
예외 처리 개선 필요
현재 구현에서 발견된 문제점들:
null을 반환하는 대신 명시적인 예외 처리가 필요합니다applicationId유효성 검증이 없습니다- 오류 로깅이 누락되었습니다
다음과 같이 개선을 제안합니다:
@Override
public FormApplication getById(Long applicationId) {
- return formApplicationRepository.findById(applicationId).orElse(null);
+ if (applicationId == null || applicationId <= 0) {
+ throw new IllegalArgumentException("유효하지 않은 applicationId입니다: " + applicationId);
+ }
+ return formApplicationRepository.findById(applicationId)
+ .orElseThrow(() -> new EntityNotFoundException("ID가 " + applicationId + "인 폼 어플리케이션을 찾을 수 없습니다"));
}추가로 로깅을 위해 다음 의존성을 추가하는 것을 추천드립니다:
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;그리고 클래스 상단에 로거를 선언:
private static final Logger log = LoggerFactory.getLogger(GeneralFormApplicationService.class);| public static FormApplicationQuery of(FormApplication formApplication, List<FormField> formFields, List<FormAnswer> formAnswers) { | ||
| List<FormFieldListQuery> formFieldListQueries = formFields.stream() | ||
| .map(FormFieldListQuery::from) | ||
| .toList(); | ||
| List<FormAnswerListQuery> formAnswerListQueries = formAnswers.stream() | ||
| .map(FormAnswerListQuery::from) | ||
| .toList(); | ||
| Map<Long, FormAnswerListQuery> answerMap = formAnswerListQueries.stream() | ||
| .collect(Collectors.toMap(FormAnswerListQuery::fieldId, Function.identity())); | ||
| List<FormFieldAnswerListQuery> formFieldAnswerListQueries = formFieldListQueries.stream() | ||
| .map(fieldQuery -> { | ||
| FormAnswerListQuery answerQuery = answerMap.get(fieldQuery.id()); | ||
| if (answerQuery == null) { | ||
| answerQuery = FormAnswerListQuery.builder() | ||
| .fieldId(fieldQuery.id()) | ||
| .value(null) | ||
| .build(); | ||
| } | ||
| return FormFieldAnswerListQuery.from(fieldQuery, answerQuery); | ||
| }) | ||
| .collect(Collectors.toList()); | ||
| return FormApplicationQuery.builder() | ||
| .createdAt(formApplication.getCreatedAt()) | ||
| .name(formApplication.getName()) | ||
| .studentNumber(formApplication.getStudentNumber()) | ||
| .department(formApplication.getDepartment()) | ||
| .status(formApplication.getStatus()) | ||
| .formFieldAnswers(formFieldAnswerListQueries) | ||
| .build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
결측 답변에 대한 처리 로직 보강 제안
of 메서드에서 필드에 대응하는 답변이 없을 때 value를 null로 설정하고 있습니다. 이 로직이 의도한 로직인지, 또는 빈 리스트(Collections.emptyList()) 등으로 처리하는 편이 더 안전한지 고려해 보시면 좋겠습니다. null 값이 뷰나 다른 계층에서 처리될 때 예외가 발생할 수 있으니, 처리 방식을 명확히 정의해 주세요.
| public static FormFieldAnswerListResponse from(FormFieldAnswerListQuery formFieldAnswerListQuery) { | ||
| return FormFieldAnswerListResponse.builder() | ||
| .question(formFieldAnswerListQuery.question()) | ||
| .type(formFieldAnswerListQuery.type()) | ||
| .options(formFieldAnswerListQuery.options()) | ||
| .required(formFieldAnswerListQuery.required()) | ||
| .order(formFieldAnswerListQuery.order()) | ||
| .section(formFieldAnswerListQuery.section()) | ||
| .value(formFieldAnswerListQuery.value()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
fieldId 매핑이 누락되었습니다.
FormFieldAnswerListResponse.from 메서드에서 fieldId 필드에 대한 매핑이 누락되었습니다.
다음과 같이 수정해주세요:
public static FormFieldAnswerListResponse from(FormFieldAnswerListQuery formFieldAnswerListQuery) {
return FormFieldAnswerListResponse.builder()
+ .fieldId(formFieldAnswerListQuery.id())
.question(formFieldAnswerListQuery.question())
.type(formFieldAnswerListQuery.type())
.options(formFieldAnswerListQuery.options())
.required(formFieldAnswerListQuery.required())
.order(formFieldAnswerListQuery.order())
.section(formFieldAnswerListQuery.section())
.value(formFieldAnswerListQuery.value())
.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 static FormFieldAnswerListResponse from(FormFieldAnswerListQuery formFieldAnswerListQuery) { | |
| return FormFieldAnswerListResponse.builder() | |
| .question(formFieldAnswerListQuery.question()) | |
| .type(formFieldAnswerListQuery.type()) | |
| .options(formFieldAnswerListQuery.options()) | |
| .required(formFieldAnswerListQuery.required()) | |
| .order(formFieldAnswerListQuery.order()) | |
| .section(formFieldAnswerListQuery.section()) | |
| .value(formFieldAnswerListQuery.value()) | |
| .build(); | |
| } | |
| public static FormFieldAnswerListResponse from(FormFieldAnswerListQuery formFieldAnswerListQuery) { | |
| return FormFieldAnswerListResponse.builder() | |
| .fieldId(formFieldAnswerListQuery.id()) | |
| .question(formFieldAnswerListQuery.question()) | |
| .type(formFieldAnswerListQuery.type()) | |
| .options(formFieldAnswerListQuery.options()) | |
| .required(formFieldAnswerListQuery.required()) | |
| .order(formFieldAnswerListQuery.order()) | |
| .section(formFieldAnswerListQuery.section()) | |
| .value(formFieldAnswerListQuery.value()) | |
| .build(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/response/FormApplicationResponse.java (3)
16-29: 필드 유효성 검증 어노테이션 추가 권장응답 DTO에 유효성 검증 어노테이션을 추가하면 API 문서화와 런타임 검증이 향상됩니다.
예를 들어:
name,studentNumber,department에@NotBlankstatus에@NotNullformFieldAnswers에@NotEmptypublic record FormApplicationResponse ( @Schema(description = "제출일시", example = "2025-01-01T00:00") + @NotNull LocalDateTime submittedAt, @Schema(description = "지원자 이름", example = "김띵동") + @NotBlank String name, @Schema(description = "지원자 학번", example = "60201111") + @NotBlank String studentNumber, @Schema(description = "지원자 학과", example = "융합소프트웨어학부") + @NotBlank String department, @Schema(description = "status", example = "SUBMITTED") + @NotNull FormApplicationStatus status, @ArraySchema(schema = @Schema(implementation = FormFieldAnswerListResponse.class)) + @NotEmpty List<FormFieldAnswerListResponse> formFieldAnswers )
31-48: 중첩 레코드에 유효성 검증 어노테이션 추가 권장
FormFieldAnswerListResponse레코드의 필드들도 유효성 검증이 필요합니다.record FormFieldAnswerListResponse ( @Schema(description = "폼지 질문 ID", example = "1") + @NotNull Long fieldId, @Schema(description = "폼지 질문", example = "성별이 무엇입니까??") + @NotBlank String question, @Schema(description = "폼지 질문 유형", example = "RADIO", allowableValues = {"CHECK_BOX", "RADIO", "TEXT", "LONG_TEXT", "FILE"}) + @NotNull FieldType type, @Schema(description = "폼지 지문", example = "[\"여성\", \"남성\"]") List<String> options, @Schema(description = "필수 여부", example = "true") + @NotNull Boolean required, @Schema(description = "질문 순서", example = "1") + @NotNull Integer order, @Schema(description = "섹션", example = "공통") + @NotBlank String section, @Schema(description = "질문 답변 값", example = "[\"지문1\"]") List<String> value )
62-75: 팩토리 메서드에 방어적 프로그래밍 적용 권장
from메서드에서null체크를 추가하여 더 안전한 객체 생성이 가능합니다.public static FormApplicationResponse from(FormApplicationQuery formApplicationQuery) { + if (formApplicationQuery == null) { + throw new IllegalArgumentException("FormApplicationQuery must not be null"); + } + + if (formApplicationQuery.formFieldAnswers() == null) { + throw new IllegalArgumentException("FormFieldAnswers must not be null"); + } + List<FormFieldAnswerListResponse> responses = formApplicationQuery.formFieldAnswers().stream() .map(FormFieldAnswerListResponse::from) .toList();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/response/FormApplicationResponse.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java (1)
53-82: 테스트 데이터 설정에서 불필요한 null 설정이 있습니다.Club 엔티티 생성 시 score와 clubMembers를 명시적으로 null로 설정하는 것은 불필요해 보입니다. FixtureMonkey가 자동으로 처리할 수 있습니다.
다음과 같이 수정하는 것을 제안합니다:
Club club = fixtureMonkey.giveMeBuilder(Club.class) .set("id", 1L) .set("user", savedUser) - .set("score", null) - .set("clubMembers", null) .set("deletedAt", null) .sample();src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormApplicationServiceImplTest.java (5)
23-24: Assertion 라이브러리를 혼용 사용 시 주의가 필요합니다.
AssertJ와JUnit기본 assertion을 동시에 사용하면 유지보수 시 혼란이 생길 수 있습니다. 특정 상황에서 필요한 경우가 아니라면, 가급적 한 가지 라이브러리로 통일하는 방안을 고려해 주세요.
30-30: 필드 주입 대신 생성자 주입 방식을 권장드립니다.
Spring에서는 생성자 주입이 테스트 안정성, 코드 가독성, 의존성 불변성을 높이는 데 유리합니다. 아래 예시처럼 수정할 수 있습니다.- @Autowired - private UserRepository userRepository; - @Autowired - private FormRepository formRepository; - @Autowired - private FormApplicationService formApplicationService; - @Autowired - private FacadeCentralFormApplicationService facadeCentralFormApplicationService; + private final UserRepository userRepository; + private final FormRepository formRepository; + private final FormApplicationService formApplicationService; + private final FacadeCentralFormApplicationService facadeCentralFormApplicationService; + @Autowired + public FacadeCentralFormApplicationServiceImplTest( + UserRepository userRepository, + FormRepository formRepository, + FormApplicationService formApplicationService, + FacadeCentralFormApplicationService facadeCentralFormApplicationService + ) { + this.userRepository = userRepository; + this.formRepository = formRepository; + this.formApplicationService = formApplicationService; + this.facadeCentralFormApplicationService = facadeCentralFormApplicationService; + }Also applies to: 36-36, 39-39, 41-42
48-48: 테스트 메서드명과 목적이 일치하지만, 검증 범위를 확장해 보세요.
현재name만 확인하고 있으므로, 응답 DTO에서 중요하게 다루는 다른 필드(status,department등)에 대한 검증이 추가된다면 테스트 신뢰도가 더 높아질 수 있습니다.
79-79: 하드코딩된 동아리 식별자 사용에 대한 고려가 필요합니다.
1L은 테스트 흐름상 가독성을 해치지는 않으나, 실제 도메인 로직에서 다른 식별자를 활용할 수 있도록 Fixture에서 동적으로 추출해 쓰면 유연성이 높아집니다.
81-81: 검증 범위 확장 제안
formApplicationQuery에 포함된studentNumber,department,status등도 함께 검증하면, 실제 도메인 요구사항에 대한 커버리지가 높아집니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormApplicationServiceImplTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (7)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java (1)
1-52: 테스트 클래스 설정이 적절합니다.의존성 주입과 테스트 데이터 생성을 위한 설정이 잘 되어있습니다.
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormApplicationServiceImplTest.java (6)
10-14: 새로운 폼 지원 도메인 관련 import 추가가 적절합니다.
테스트 클래스 내에서 폼 지원 엔티티 및 서비스 로직을 검증하기 위해 필요한 의존성을 명확히 드러내므로, 현재 import 구성이 논리적으로 타당합니다.
26-27: SpringBootTest와 TestContainerSupport 적용으로 통합 테스트 환경이 잘 구성되었습니다.
실제 환경과 유사하게 여러 계층을 포괄하는 테스트를 진행하는 방식은 안정적인 품질 보증에 도움을 줍니다.
44-44: FixtureMonkey를 이용한 객체 생성 상수 선언은 깔끔합니다.
테스트에서 일관된 Fixture를 사용하기에 적절한 접근입니다.
46-46: 테스트 시나리오 주석이 명확합니다.
테스트가 수행하는 기능—the "동아리가 지원자 응답을 상세조회 한다"—를 잘 설명하고 있어 가독성이 좋습니다.
50-50: FixtureMonkey를 이용한 Given 단계 구성이 명확합니다.
User,Club,Form엔티티를 성공적으로 연결해 저장하며 시나리오를 구축하고 있어, 데이터 흐름이 한눈에 들어옵니다.Also applies to: 55-55, 56-56, 63-63, 64-64, 67-67, 68-68, 69-69
70-77: 학번, 이름 등 민감 정보가 존재하므로 운영 로그 출력 시 주의가 필요합니다.
테스트 환경에서는 문제없으나, 운영 환경에서 로깅하거나 예외 메시지로 노출될 경우 잠재적 개인정보 침해 우려가 있을 수 있습니다.
| @DisplayName("동아리는 폼지를 상세조회 할 수 있다.") | ||
| @Test | ||
| void getForm() { | ||
| // given | ||
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | ||
| .set("id", 1L) | ||
| .set("title", "제목1") | ||
| .set("club", null) | ||
| .sample(); | ||
| Form form2 = fixtureMonkey.giveMeBuilder(Form.class) | ||
| .set("id", 2L) | ||
| .set("title", "제목2") | ||
| .set("club", null) | ||
| .sample(); | ||
| formService.create(form); | ||
| formService.create(form2); | ||
| // when | ||
| FormQuery formQuery = facadeCentralFormService.getForm(1L); | ||
| // then | ||
| assertThat(formQuery.title()).isEqualTo("제목1"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
getForm 테스트에 예외 케이스가 누락되었습니다.
존재하지 않는 ID로 조회하는 경우나 권한이 없는 사용자가 조회하는 경우 등의 예외 케이스에 대한 테스트가 필요합니다.
다음과 같은 테스트 케이스 추가를 제안드립니다:
@Test
@DisplayName("존재하지 않는 폼지 조회 시 예외가 발생한다")
void getForm_NotFound() {
assertThrows(FormNotFoundException.class, () -> {
facadeCentralFormService.getForm(999L);
});
}
@Test
@DisplayName("권한이 없는 사용자는 폼지를 조회할 수 없다")
void getForm_NoAuthority() {
// given
User unauthorizedUser = fixtureMonkey.giveMeBuilder(User.class)
.set("role", Role.STUDENT)
.sample();
// when & then
assertThrows(NonHaveAuthority.class, () -> {
facadeCentralFormService.getForm(1L, unauthorizedUser);
});
}| Form form = fixtureMonkey.giveMeBuilder(Form.class) | ||
| .set("id", 1L) | ||
| .set("title", "제목1") | ||
| .set("club", null) | ||
| .sample(); |
There was a problem hiding this comment.
Form 엔티티의 club이 null로 설정된 것이 적절하지 않습니다.
getForm 테스트에서 Form 엔티티의 club을 null로 설정하는 것은 실제 사용 사례와 맞지 않습니다. Form은 항상 Club과 연관관계를 가져야 합니다.
다음과 같이 수정하는 것을 제안합니다:
Form form = fixtureMonkey.giveMeBuilder(Form.class)
.set("id", 1L)
.set("title", "제목1")
- .set("club", null)
+ .set("club", club)
.sample();📝 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 = fixtureMonkey.giveMeBuilder(Form.class) | |
| .set("id", 1L) | |
| .set("title", "제목1") | |
| .set("club", null) | |
| .sample(); | |
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | |
| .set("id", 1L) | |
| .set("title", "제목1") | |
| .set("club", club) | |
| .sample(); |
| @DisplayName("폼지와 폼지 질문을 수정할 수 있다.") | ||
| @Test | ||
| void updateForm() { | ||
| // given | ||
| User user = fixtureMonkey.giveMeBuilder(User.class) | ||
| .set("id", 1L) | ||
| .set("Role", Role.CLUB) | ||
| .set("deletedAt", null) | ||
| .sample(); | ||
| User savedUser = userRepository.save(user); | ||
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | ||
| .set("id", 1L) | ||
| .set("user", savedUser) | ||
| .set("score", null) | ||
| .set("clubMembers", null) | ||
| .set("deletedAt", null) | ||
| .sample(); | ||
| clubRepository.save(club); | ||
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | ||
| .set("club", club) | ||
| .sample(); | ||
| Form savedForm = formService.create(form); | ||
| UpdateFormCommand updateFormCommand = fixtureMonkey.giveMeBuilder(UpdateFormCommand.class) | ||
| .set("title", "수정된 제목") | ||
| .set("description", "수정된 설명") | ||
| .set("formId", savedForm.getId()) | ||
| .set("formFieldCommands", List.of( | ||
| fixtureMonkey.giveMeBuilder(UpdateFormFieldCommand.class) | ||
| .set("question", "수정된 질문") | ||
| .sample()) | ||
| ) | ||
| .sample(); | ||
| // when | ||
| facadeCentralFormService.updateForm(updateFormCommand); | ||
| // then | ||
| Form found = formRepository.findById(savedForm.getId()).orElse(null); | ||
| List<FormField> formFields = formFieldRepository.findAllByForm(found); | ||
| assertThat(found).isNotNull(); | ||
| assertThat(found.getTitle()).isEqualTo("수정된 제목"); | ||
| assertThat(found.getDescription()).isEqualTo("수정된 설명"); | ||
| assertThat(formFields).isNotEmpty(); | ||
| assertThat(formFields.get(0).getQuestion()).isEqualTo("수정된 질문"); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
테스트 케이스에 검증이 부족합니다.
updateForm 테스트에서 폼 필드의 다른 속성들(예: 필수 여부, 필드 타입 등)에 대한 검증이 누락되어 있습니다.
추가적인 검증을 포함하는 것을 추천드립니다:
assertThat(formFields).isNotEmpty();
assertThat(formFields.get(0).getQuestion()).isEqualTo("수정된 질문");
+ assertThat(formFields.get(0).isRequired()).isNotNull();
+ assertThat(formFields.get(0).getFieldType()).isNotNull();📝 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.
| @DisplayName("폼지와 폼지 질문을 수정할 수 있다.") | |
| @Test | |
| void updateForm() { | |
| // given | |
| User user = fixtureMonkey.giveMeBuilder(User.class) | |
| .set("id", 1L) | |
| .set("Role", Role.CLUB) | |
| .set("deletedAt", null) | |
| .sample(); | |
| User savedUser = userRepository.save(user); | |
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | |
| .set("id", 1L) | |
| .set("user", savedUser) | |
| .set("score", null) | |
| .set("clubMembers", null) | |
| .set("deletedAt", null) | |
| .sample(); | |
| clubRepository.save(club); | |
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | |
| .set("club", club) | |
| .sample(); | |
| Form savedForm = formService.create(form); | |
| UpdateFormCommand updateFormCommand = fixtureMonkey.giveMeBuilder(UpdateFormCommand.class) | |
| .set("title", "수정된 제목") | |
| .set("description", "수정된 설명") | |
| .set("formId", savedForm.getId()) | |
| .set("formFieldCommands", List.of( | |
| fixtureMonkey.giveMeBuilder(UpdateFormFieldCommand.class) | |
| .set("question", "수정된 질문") | |
| .sample()) | |
| ) | |
| .sample(); | |
| // when | |
| facadeCentralFormService.updateForm(updateFormCommand); | |
| // then | |
| Form found = formRepository.findById(savedForm.getId()).orElse(null); | |
| List<FormField> formFields = formFieldRepository.findAllByForm(found); | |
| assertThat(found).isNotNull(); | |
| assertThat(found.getTitle()).isEqualTo("수정된 제목"); | |
| assertThat(found.getDescription()).isEqualTo("수정된 설명"); | |
| assertThat(formFields).isNotEmpty(); | |
| assertThat(formFields.get(0).getQuestion()).isEqualTo("수정된 질문"); | |
| } | |
| @DisplayName("폼지와 폼지 질문을 수정할 수 있다.") | |
| @Test | |
| void updateForm() { | |
| // given | |
| User user = fixtureMonkey.giveMeBuilder(User.class) | |
| .set("id", 1L) | |
| .set("Role", Role.CLUB) | |
| .set("deletedAt", null) | |
| .sample(); | |
| User savedUser = userRepository.save(user); | |
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | |
| .set("id", 1L) | |
| .set("user", savedUser) | |
| .set("score", null) | |
| .set("clubMembers", null) | |
| .set("deletedAt", null) | |
| .sample(); | |
| clubRepository.save(club); | |
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | |
| .set("club", club) | |
| .sample(); | |
| Form savedForm = formService.create(form); | |
| UpdateFormCommand updateFormCommand = fixtureMonkey.giveMeBuilder(UpdateFormCommand.class) | |
| .set("title", "수정된 제목") | |
| .set("description", "수정된 설명") | |
| .set("formId", savedForm.getId()) | |
| .set("formFieldCommands", List.of( | |
| fixtureMonkey.giveMeBuilder(UpdateFormFieldCommand.class) | |
| .set("question", "수정된 질문") | |
| .sample()) | |
| ) | |
| .sample(); | |
| // when | |
| facadeCentralFormService.updateForm(updateFormCommand); | |
| // then | |
| Form found = formRepository.findById(savedForm.getId()).orElse(null); | |
| List<FormField> formFields = formFieldRepository.findAllByForm(found); | |
| assertThat(found).isNotNull(); | |
| assertThat(found.getTitle()).isEqualTo("수정된 제목"); | |
| assertThat(found.getDescription()).isEqualTo("수정된 설명"); | |
| assertThat(formFields).isNotEmpty(); | |
| assertThat(formFields.get(0).getQuestion()).isEqualTo("수정된 질문"); | |
| assertThat(formFields.get(0).isRequired()).isNotNull(); | |
| assertThat(formFields.get(0).getFieldType()).isNotNull(); | |
| } |
There was a problem hiding this comment.
고생하셨습니다.
다른 api에서 사용한 dto를 사용하기전 봐야할 것은
가장 첫 번째로 해당 dto를 변경하지 않고 사용할 수 있는가이고,
이것도 위험하기때문에 두 번째로 정말 공통된 dto냐 생각해봐야합니다.
예를 들어 paging처리하는 dto는 정말 공통되기 때문에 feed쪽에서 제가 재사용한 적이 있습니다.
이처럼 다른 api에서 사용한 dto 재사용은 고민많이 하고 사용해야합니다.
하지만, 이번 재사용은 적절하지 않은 것 같아요! 변경하면서까지 사용해야 하기 때문입니다.
그리고 로직이 복잡하게 된 것은 FormField랑 FormAnswer랑 따로 생성하고 받았기 때문인데, FormAnswer내부 formField를 사용하면 로직이 많이 단순해 질것입니다.
| @@ -20,6 +20,7 @@ public record FormQuery( | |||
|
|
|||
| @Builder | |||
| public record FormFieldListQuery( | |||
| Long id, | |||
There was a problem hiding this comment.
fieldId를 FormFieldAnswerListQuery에서 사용해야 해서 추가했었습니다. 현재는 삭제하고 선배님 말씀 참고해서 삭제했습니다!
| String section, | ||
| List<String> value | ||
| ) { | ||
| public static FormFieldAnswerListQuery from(FormFieldListQuery formFieldListQuery, FormAnswerListQuery formAnswerListQuery) { |
There was a problem hiding this comment.
p3)
서로다른 api에서 dto를 재사용하는 것은 거의 드뭅니다! 거의 공통된 도메인을 목적으로 사용하는 것이 아니라면 따로 만드는 것이 좋아요
There was a problem hiding this comment.
넵 명심하겠습니다! 그런데 여쭤보고 싶은 점이 있습니다. feed에 있는 내용 거의 그대로인 PagingQuery와 PagingResponse 같은 경우 이후에 같은 용도로 사용될 일이 많을 것 같은데, 일반화해서 common 패키지에 빼놓는 것은 어떻게 생각하시나요? 당장 우선순위가 높은 일은 아니지만 궁금해서 선배님 의견을 여쭤보고 싶습니다!
There was a problem hiding this comment.
그래도 해당 도메인의 페이지 처리에 요구사항이 변경될 수 있다고 하는데, 그렇게 사용하는 건 위험이 너무 크다고 생각합니다.
하나 바꾸면 전체 바꿔야 하니까!.
사실 위 같은 이유때문에 common패키지란 것이 정말 사용을 많이 고민하고 사용해야 합니다
There was a problem hiding this comment.
그렇군요!! 설명해주셔서 감사합니다!
| } | ||
| FormApplication formApplication = formApplicationService.getById(applicationId); | ||
| List<FormField> formFields = formFieldService.findAllByForm(form); | ||
| List<FormAnswer> formAnswers = formAnswerService.getAllByApplication(formApplication); |
There was a problem hiding this comment.
p1) form으로 fields를 찾는 것보다 formAnswer내부의 field를 사용하면 dto내 복잡한 로직을 줄일 수 있을 것 같아요
There was a problem hiding this comment.
아하...!!! 제가 엔터티가 객체랑 매핑되어 있는 걸 잊었네요... 테이블에 있는 것처럼 id만 있다고 생각 했더니 이렇게 됐습니다. 알려주셔서 감사합니다! dto 재사용이 조심스러워야 한다는 점도 처음 알았습니다. 감사합니다. 수정 완료했습니다!
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java (1)
30-42:🧩 Analysis chain
접근 제어 로직이 누락되었습니다!
getMyFormApplicationPage메서드에서 사용자의 권한 검증 로직이 제거되었습니다. PR 목표에서 언급된 권한 관련 오류와 연관이 있을 수 있습니다.다음 사항을 확인해주세요:
- 폼에 대한 접근 권한 검증
- 사용자의 클럽 소속 여부 확인
권한 검증 로직이 다른 곳으로 이동되었는지 확인하기 위해 다음 스크립트를 실행해주세요:
🏁 Script executed:
#!/bin/bash # Description: Check if access control logic exists elsewhere # Search for access control related code rg -A 5 "AccessDeniedException|hasPermission|isOwner|getClub" --type javaLength of output: 23878
접근 제어 로직 누락 확인됨
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java의getMyFormApplicationPage메서드에서는 사용자 권한 검증 관련 로직이 보이지 않습니다.
실행한 검색 스크립트 결과, 다른 곳에서는 접근 제어와 관련된 코드(예:NonHaveAuthority체크)가 보이지만, 폼 애플리케이션 관련 로직에서는 해당 검증을 확인할 수 없습니다.검토 사항:
- 폼 애플리케이션 조회시 사용자에게 적절한 접근 권한 검증이 이루어지고 있는지 확인할 것
- 만약 검증 로직이 다른 레이어(예, 컨트롤러 또는 서비스 내부의 별도 메서드)로 이동되었다면 그 위치와 연계가 올바른지 재검토 필요
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/query/FormApplicationQuery.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/query/FormApplicationQuery.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java (1)
5-6: 의존성 추가가 적절합니다!새로운 기능 구현을 위한
FormAnswer관련 의존성 추가가 잘 되어있습니다.Also applies to: 27-27
| @Override | ||
| public FormApplicationQuery getFormApplication(Long formId, Long applicationId, User user) { | ||
| FormApplication formApplication = formApplicationService.getById(applicationId); | ||
| List<FormAnswer> formAnswers = formAnswerService.getAllByApplication(formApplication); | ||
| return FormApplicationQuery.of(formApplication, formAnswers); | ||
| } |
There was a problem hiding this comment.
필수 유효성 검사가 누락되었습니다!
getFormApplication 메서드에 다음과 같은 중요한 검증 로직이 누락되었습니다:
- 사용자의 접근 권한 검증
formApplicationnull 체크formId와application의 연관성 검증
다음과 같이 수정하는 것을 제안드립니다:
@Override
public FormApplicationQuery getFormApplication(Long formId, Long applicationId, User user) {
+ Form form = formService.getById(formId);
+ if (!form.getClub().equals(user.getClub())) {
+ throw new AccessDeniedException("해당 폼에 대한 접근 권한이 없습니다.");
+ }
FormApplication formApplication = formApplicationService.getById(applicationId);
+ if (formApplication == null) {
+ throw new EntityNotFoundException("지원서를 찾을 수 없습니다.");
+ }
+ if (!formApplication.getForm().getId().equals(formId)) {
+ throw new InvalidArgumentException("지원서가 해당 폼에 속하지 않습니다.");
+ }
List<FormAnswer> formAnswers = formAnswerService.getAllByApplication(formApplication);
return FormApplicationQuery.of(formApplication, formAnswers);
}📝 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 FormApplicationQuery getFormApplication(Long formId, Long applicationId, User user) { | |
| FormApplication formApplication = formApplicationService.getById(applicationId); | |
| List<FormAnswer> formAnswers = formAnswerService.getAllByApplication(formApplication); | |
| return FormApplicationQuery.of(formApplication, formAnswers); | |
| } | |
| @Override | |
| public FormApplicationQuery getFormApplication(Long formId, Long applicationId, User user) { | |
| Form form = formService.getById(formId); | |
| if (!form.getClub().equals(user.getClub())) { | |
| throw new AccessDeniedException("해당 폼에 대한 접근 권한이 없습니다."); | |
| } | |
| FormApplication formApplication = formApplicationService.getById(applicationId); | |
| if (formApplication == null) { | |
| throw new EntityNotFoundException("지원서를 찾을 수 없습니다."); | |
| } | |
| if (!formApplication.getForm().getId().equals(formId)) { | |
| throw new InvalidArgumentException("지원서가 해당 폼에 속하지 않습니다."); | |
| } | |
| List<FormAnswer> formAnswers = formAnswerService.getAllByApplication(formApplication); | |
| return FormApplicationQuery.of(formApplication, formAnswers); | |
| } |
|
|
||
| @Override | ||
| public FormApplicationQuery getFormApplication(Long formId, Long applicationId, User user) { |
There was a problem hiding this comment.
p3)
formId, user 파라미터는 해당 메서드에서 사용을 안하는데 따로 받는 이유가 있을까요?
없다면 제거해도 괜찮을거 같습니다.
There was a problem hiding this comment.
추후 해당 폼이 로그인한 사용자 소유의 폼인지 유효성 검사를 해야 할 것 같아서 받아왔는데 일단 삭제해두는 게 맞을까요?
| @@ -31,6 +31,11 @@ public Slice<FormApplication> getFormApplicationPageByFormId(Long formId, int si | |||
| return buildSlice(formApplicationPages, size); | |||
| } | |||
|
|
|||
| @Override | |||
| public FormApplication getById(Long applicationId) { | |||
| return formApplicationRepository.findById(applicationId).orElse(null); | |||
There was a problem hiding this comment.
p1)
getById는 현재 동작상 반드시 FormApplication 객체 반환을 보장해야 하는 거 같은데 조회된 값이 없을 시 null을 return 하는 거 보단 ResourceNotFoundException(커스텀 exception 정의되어 있음)을 던지는게 좋을거 같습니다!
There was a problem hiding this comment.
말씀해주신 부분 반영하였습니다! 감사합니다!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java (1)
35-39: 입력 유효성 검증과 로깅 추가 필요현재 구현은 이전 리뷰의 제안사항을 잘 반영했습니다만, 다음 개선사항을 고려해주세요:
- applicationId의 유효성 검증 추가
- 예외 발생 시 로깅 추가
다음과 같이 개선하는 것을 제안드립니다:
@Override public FormApplication getById(Long applicationId) { + if (applicationId == null || applicationId <= 0) { + log.error("유효하지 않은 applicationId: {}", applicationId); + throw new IllegalArgumentException("유효하지 않은 applicationId입니다: " + applicationId); + } + log.debug("FormApplication 조회 시도: id={}", applicationId); return formApplicationRepository.findById(applicationId) - .orElseThrow(() -> new ResourceNotFound("주어진 id로 해당 지원자를 찾을 수 없습니다.:"+applicationId)); + .orElseThrow(() -> { + log.error("FormApplication을 찾을 수 없음: id={}", applicationId); + return new ResourceNotFound("주어진 id로 해당 지원자를 찾을 수 없습니다.: " + applicationId); + }); }로깅을 위해 클래스 상단에 다음을 추가해주세요:
private static final Logger log = LoggerFactory.getLogger(GeneralFormApplicationService.class);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (2)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java (2)
3-3: 적절한 예외 클래스 import 추가!
ResourceNotFound예외를 사용하여 리소스를 찾을 수 없는 상황을 명확하게 처리하고 있습니다.
16-19: 트랜잭션 관리와 아키텍처 통합이 잘 되어있습니다!클래스 레벨의
@Transactional(readOnly = true)와 잘 통합되어 있으며, 기존 서비스 구조와 일관성있게 구현되었습니다.Also applies to: 35-39
🚀 작업 내용
지원자 상세 조회 API 구현하였습니다.
🤔 고민했던 내용
FormAnswerListQuery의 위치를 고민했습니다. FormFieldAnswerListQuery와 같은 뎁스에 있으니 가독성이 좋지 않은 것 같기도 합니다....
FormFieldAnswerListQuery의 위치 또한 고민됩니다. FormFieldListQuery는 FormQuery에 있고, FormAnswerListQuery는 FormApplicationListQuery에 있어서 이들을 어떻게 위치시켜야 가독성 측면에서 좋을지 궁금합니다. 그냥 FormFieldAnswerListQuery를 외부로 빼버리는 건 어떨까요?
💬 리뷰 중점사항
현재 로컬 swagger에서 폼 관련 요청 전부 500 에러가 납니다.
test 코드를 실행하면 FacadeCentralFormApplicationServiceImpl에서 예외처리한 사용자 소유 이외의 폼지를 조회했을 때 나는 에러(권한 없음 에러)가 나는데, swagger로도 테스트 해봐야 무엇 때문에 오류가 나는지 알 수 있을 것 같습니다.
Summary by CodeRabbit