Feat/time table #385
Feat/time table #385
Conversation
Summary of ChangesHello @chominju02, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 시험 시간표를 PDF 형식으로 생성하고 제공하는 새로운 기능을 도입합니다. 이를 위해 PDF 생성 및 처리를 담당하는 서비스와 프로세서가 추가되었으며, 회원 및 파트너를 위한 전용 API 엔드포인트가 구현되었습니다. 또한, 시간표 데이터 조회를 위한 새로운 데이터베이스 쿼리가 정의되었고, 기존 시험표 정보 조회 로직에서 과목명 표시 방식이 개선되었습니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughAdds a new TimeTable feature: repository projections and queries, service, processor to generate PDF, DTOs, and controller endpoints with whitelist entry. Adjusts subject-name mapping in exam-ticket processors. Updates a cron job schedule time. No public method signature changes except new APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant C as TimeTableController
participant S as TimeTableService
participant R as ExamApplicationJpaRepository
participant RS as ExamSubjectJpaRepository
participant P as GenerateTimeTableProcessor
Client->>C: GET /time-table/{member|partner}?examDate=YYYY-MM-DD
alt member
C->>S: getMemberTimeTables(examDate)
S->>R: findMemberTimeTable(examDate)
else partner
C->>S: getPartnerTimeTables(examDate)
S->>R: findPartnerTimeTable(examDate)
end
R-->>S: List<TimeTableInfoProjection>
loop each application
S->>RS: findByExamApplicationId(id)
RS-->>S: Stream<ExamSubjectJpaEntity>
S->>S: map subjects (SOCIETY_AND_CULTURE → "사회·문화"), sort by ordinal
end
S->>P: process(List<TimeTableInfoResponse>)
P-->>S: TimeTableFileResponse (PDF bytes, filename, contentType)
S-->>C: TimeTableFileResponse
C-->>Client: 200 OK (Content-Type: application/pdf)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
시간표 생성 기능 추가를 위한 PR 잘 보았습니다. 전반적으로 PDF 생성 로직과 API 구현이 잘 이루어졌습니다. 다만, 여러 파일에 걸쳐 코드 중복이 발견되어 유지보수성 측면에서 개선할 부분이 보입니다. 특히 과목명을 변환하는 로직과 서비스 레이어의 메서드 구조가 중복되고 있습니다. 또한, 예외 처리 방식을 더 명확하게 하고, 불필요한 코드를 제거하여 가독성을 높일 수 있는 부분들에 대해 몇 가지 의견을 제안했습니다. 리뷰 코멘트를 참고하여 코드를 개선하면 더 견고하고 유지보수하기 좋은 코드가 될 것입니다.
| .map(subject -> { | ||
| if (subject == Subject.SOCIETY_AND_CULTURE) { | ||
| return "사회·문화"; | ||
| } | ||
| return subject.getSubjectName(); | ||
| }) |
There was a problem hiding this comment.
이 과목명 변환 로직은 GetPartnerExamTicketInfoProcessor, TimeTableService 등 여러 곳에서 중복되고 있습니다. 코드 중복은 향후 유지보수를 어렵게 만듭니다. 이 로직을 Subject enum 내부에 getDisplayName()과 같은 공통 메서드로 추출하여 중앙에서 관리하는 것을 강력히 권장합니다.
| .map(subject -> { | ||
| if (subject == Subject.SOCIETY_AND_CULTURE) { | ||
| return "사회·문화"; | ||
| } | ||
| return subject.getSubjectName(); | ||
| }) |
There was a problem hiding this comment.
이 과목명 변환 로직은 GetMemberExamTicketInfoProcessor, TimeTableService 등 여러 곳에서 중복되고 있습니다. 코드 중복은 향후 유지보수를 어렵게 만듭니다. 이 로직을 Subject enum 내부에 getDisplayName()과 같은 공통 메서드로 추출하여 중앙에서 관리하는 것을 강력히 권장합니다.
| public TimeTableFileResponse getMemberTimeTables(LocalDate examDate) { | ||
| List<TimeTableInfoResponse> entries = examApplicationJpaRepository.findMemberTimeTable(examDate) | ||
| .stream() | ||
| .map(info -> { | ||
| Long examApplicationId = info.examApplicationId(); | ||
| List<String> subjects = getSubjects(examApplicationId); | ||
|
|
||
| return TimeTableInfoResponse.of( | ||
| info.examNumber(), | ||
| info.userName(), | ||
| subjects, | ||
| info.schoolName() | ||
| ); | ||
| }) | ||
| .toList(); | ||
|
|
||
| return generateTimeTableProcessor.process(entries); | ||
| } | ||
|
|
||
| @Transactional(readOnly = true, propagation = Propagation.SUPPORTS) | ||
| public TimeTableFileResponse getPartnerTimeTables(LocalDate examDate) { | ||
| List<TimeTableInfoResponse> entries = examApplicationJpaRepository.findPartnerTimeTable(examDate) | ||
| .stream() | ||
| .map(info -> { | ||
| Long examApplicationId = info.examApplicationId(); | ||
| List<String> subjects = getSubjects(examApplicationId); | ||
|
|
||
| return TimeTableInfoResponse.of( | ||
| info.examNumber(), | ||
| info.userName(), | ||
| subjects, | ||
| info.schoolName() | ||
| ); | ||
| }) | ||
| .toList(); | ||
|
|
||
| return generateTimeTableProcessor.process(entries); | ||
| } |
There was a problem hiding this comment.
getMemberTimeTables와 getPartnerTimeTables 메서드는 데이터 조회 부분을 제외하고는 로직이 거의 동일합니다. 코드 중복을 줄이고 재사용성을 높이기 위해 공통 로직을 별도의 private 메서드로 추출하는 것이 좋습니다. 또한, TimeTableInfoResponse의 불필요한 of 메서드 대신 생성자를 직접 사용하도록 함께 수정하는 것을 제안합니다.
@Transactional(readOnly = true, propagation = Propagation.SUPPORTS)
public TimeTableFileResponse getMemberTimeTables(LocalDate examDate) {
List<life.mosu.mosuserver.domain.examapplication.projection.TimeTableInfoProjection> infos = examApplicationJpaRepository.findMemberTimeTable(examDate);
return generateTimeTablesFor(infos);
}
@Transactional(readOnly = true, propagation = Propagation.SUPPORTS)
public TimeTableFileResponse getPartnerTimeTables(LocalDate examDate) {
List<life.mosu.mosuserver.domain.examapplication.projection.TimeTableInfoProjection> infos = examApplicationJpaRepository.findPartnerTimeTable(examDate);
return generateTimeTablesFor(infos);
}
private TimeTableFileResponse generateTimeTablesFor(List<life.mosu.mosuserver.domain.examapplication.projection.TimeTableInfoProjection> timeTableInfos) {
List<TimeTableInfoResponse> entries = timeTableInfos.stream()
.map(info -> {
Long examApplicationId = info.examApplicationId();
List<String> subjects = getSubjects(examApplicationId);
return new TimeTableInfoResponse(
info.examNumber(),
info.userName(),
subjects,
info.schoolName()
);
})
.toList();
return generateTimeTableProcessor.process(entries);
}| .map(subject -> { | ||
| if (subject == Subject.SOCIETY_AND_CULTURE) { | ||
| return "사회·문화"; | ||
| } | ||
| return subject.getSubjectName(); | ||
| }) |
There was a problem hiding this comment.
이 과목명 변환 로직은 GetMemberExamTicketInfoProcessor, GetPartnerExamTicketInfoProcessor 등 여러 곳에서 중복되고 있습니다. 코드 중복은 향후 유지보수를 어렵게 만듭니다. 이 로직을 Subject enum 내부에 getDisplayName()과 같은 공통 메서드로 추출하여 중앙에서 관리하는 것을 강력히 권장합니다.
| String sub1 = (e.subjects() != null && e.subjects().size() > 0) ? nz(e.subjects().get(0)) : ""; | ||
| String sub2 = (e.subjects() != null && e.subjects().size() > 1) ? nz(e.subjects().get(1)) : ""; |
There was a problem hiding this comment.
과목 문자열을 가져오는 로직이 다소 장황하고, 불필요한 null 체크를 포함하고 있습니다. TimeTableService의 getSubjects 메서드는 항상 null이 아닌 List를 반환하므로 e.subjects() != null 체크는 필요하지 않습니다. 또한 nz 메서드 호출도 중복됩니다. 코드를 더 간결하게 개선할 수 있습니다.
| String sub1 = (e.subjects() != null && e.subjects().size() > 0) ? nz(e.subjects().get(0)) : ""; | |
| String sub2 = (e.subjects() != null && e.subjects().size() > 1) ? nz(e.subjects().get(1)) : ""; | |
| String sub1 = e.subjects().isEmpty() ? "" : e.subjects().get(0); | |
| String sub2 = e.subjects().size() > 1 ? e.subjects().get(1) : ""; |
| } catch (Exception e) { | ||
| throw new RuntimeException("Generate time-table PDF failed", e); | ||
| } |
There was a problem hiding this comment.
catch (Exception e)는 너무 광범위한 예외 처리 방식입니다. IOException과 같이 예상되는 checked exception만 명시적으로 처리하고, RuntimeException은 그대로 전파하거나 더 구체적인 커스텀 예외로 감싸는 것이 좋습니다. 이렇게 하면 디버깅이 용이해지고 코드의 의도가 명확해집니다.
| } catch (Exception e) { | |
| throw new RuntimeException("Generate time-table PDF failed", e); | |
| } | |
| } catch (java.io.IOException e) { | |
| throw new RuntimeException("Generate time-table PDF failed", e); | |
| } |
| } catch (Exception e) { | ||
| throw new RuntimeException("Resource not found: " + classpath, e); | ||
| } |
There was a problem hiding this comment.
catch (Exception e)는 너무 광범위한 예외 처리 방식입니다. IOException과 같이 리소스 로딩 시 발생할 수 있는 특정 예외를 처리하는 것이 좋습니다. 모든 Exception을 잡으면 예상치 못한 RuntimeException까지 숨겨버릴 수 있어 디버깅을 어렵게 만듭니다.
| } catch (Exception e) { | |
| throw new RuntimeException("Resource not found: " + classpath, e); | |
| } | |
| } catch (java.io.IOException e) { | |
| throw new RuntimeException("Resource not found: " + classpath, e); | |
| } |
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to draw text", e); | ||
| } |
There was a problem hiding this comment.
drawText 메서드 내에서 Exception을 포괄적으로 잡고 있습니다. PDPageContentStream의 메서드들은 IOException을 던질 수 있으므로, IOException을 명시적으로 잡는 것이 더 좋습니다. 이렇게 하면 예외 처리의 의도가 명확해집니다.
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to draw text", e); | |
| } | |
| } catch (java.io.IOException e) { | |
| throw new RuntimeException("Failed to draw text", e); | |
| } |
|
|
||
| public static TimeTableInfoResponse of( | ||
| String examNumber, | ||
| String userName, | ||
| List<String> subjects, | ||
| String schoolName | ||
| ) { | ||
| return new TimeTableInfoResponse( | ||
| examNumber, | ||
| userName, | ||
| subjects, | ||
| schoolName | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Java record는 모든 필드를 인자로 받는 public 생성자를 자동으로 생성합니다. 현재 of 정적 팩토리 메서드는 단순히 이 생성자를 호출하는 역할만 하므로 불필요합니다. 코드를 간결하게 유지하기 위해 이 메서드를 제거하고 생성자를 직접 사용하는 것을 권장합니다. (TimeTableService의 관련 호출부도 함께 수정되어야 합니다.)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
src/main/java/life/mosu/mosuserver/infra/cron/job/ExamNumberGenerationJobRound1.java (1)
14-14: Document the schedule change rationaleAdd a brief comment above the
@CronJobannotation in ExamNumberGenerationJobRound1.java explaining why the execution time was moved to 19:30 on 2025-10-13.src/main/java/life/mosu/mosuserver/presentation/timetable/dto/TimeTableInfoResponse.java (2)
12-24: Consider removing the redundant factory method.The
offactory method is a simple pass-through to the canonical constructor without any additional logic. Records provide a canonical constructor by default, so callers can usenew TimeTableInfoResponse(...)directly.If you prefer the factory method for consistency or future extensibility, you can keep it as-is.
Apply this diff to simplify:
- public static TimeTableInfoResponse of( - String examNumber, - String userName, - List<String> subjects, - String schoolName - ) { - return new TimeTableInfoResponse( - examNumber, - userName, - subjects, - schoolName - ); - } - }
12-24: Consider removing the redundant factory method.The
of(...)factory method simply delegates to the canonical constructor without adding any logic, validation, or transformation. The canonical constructor already provides the same functionality.Apply this diff to simplify the code:
- public static TimeTableInfoResponse of( - String examNumber, - String userName, - List<String> subjects, - String schoolName - ) { - return new TimeTableInfoResponse( - examNumber, - userName, - subjects, - schoolName - ); - } -Then update call sites to use the constructor directly:
// Instead of: TimeTableInfoResponse.of(examNumber, userName, subjects, schoolName) // Use: new TimeTableInfoResponse(examNumber, userName, subjects, schoolName)src/main/java/life/mosu/mosuserver/domain/examapplication/repository/ExamApplicationJpaRepository.java (1)
304-320: Consider using INNER JOINs where filtering on joined table fields.The query uses
LEFT JOINforPaymentJpaEntity,ExamJpaEntity,UserJpaEntity, andProfileJpaEntity, but filters onp.paymentStatus = 'DONE'ande.examDate = :examDate. Since the WHERE clause requires non-null values from these tables, the LEFT JOINs effectively behave as INNER JOINs.Using INNER JOINs explicitly would clarify intent and potentially improve query optimization:
- LEFT JOIN PaymentJpaEntity p on p.examApplicationId = ea.id - LEFT JOIN ExamJpaEntity e on ea.examId = e.id - LEFT JOIN UserJpaEntity u on ea.userId = u.id - LEFT JOIN ProfileJpaEntity pr on pr.userId = u.id + JOIN PaymentJpaEntity p on p.examApplicationId = ea.id + JOIN ExamJpaEntity e on ea.examId = e.id + JOIN UserJpaEntity u on ea.userId = u.id + JOIN ProfileJpaEntity pr on pr.userId = u.id WHERE p.paymentStatus = 'DONE'Alternatively, if you intend to include exam applications without payments/exams/profiles, adjust the WHERE clause to handle nulls (e.g.,
AND (p.paymentStatus = 'DONE' OR p.id IS NULL)). However, for timetable generation, requiring complete data likely makes sense.src/main/java/life/mosu/mosuserver/application/timetable/TimeTableService.java (2)
28-65: Extract common logic to reduce duplication.
getMemberTimeTablesandgetPartnerTimeTablesshare nearly identical logic. Consider extracting the common pattern into a private helper method.Example refactoring (after addressing N+1):
@Transactional(readOnly = true, propagation = Propagation.SUPPORTS) public TimeTableFileResponse getMemberTimeTables(LocalDate examDate) { return generateTimeTable(examApplicationJpaRepository.findMemberTimeTable(examDate)); } @Transactional(readOnly = true, propagation = Propagation.SUPPORTS) public TimeTableFileResponse getPartnerTimeTables(LocalDate examDate) { return generateTimeTable(examApplicationJpaRepository.findPartnerTimeTable(examDate)); } private TimeTableFileResponse generateTimeTable(List<TimeTableInfoProjection> projections) { // Common mapping and processing logic here List<TimeTableInfoResponse> entries = projections.stream() .map(info -> { Long examApplicationId = info.examApplicationId(); List<String> subjects = getSubjects(examApplicationId); return TimeTableInfoResponse.of( info.examNumber(), info.userName(), subjects, info.schoolName() ); }) .toList(); return generateTimeTableProcessor.process(entries); }
27-27: Consider usingPropagation.REQUIREDinstead ofSUPPORTS.
Propagation.SUPPORTSruns non-transactionally if no transaction exists, which could lead to inconsistent reads when fetching timetable data and subjects across multiple repository calls. Since these are read-only operations that query multiple entities, usingPropagation.REQUIREDwould ensure consistent snapshot isolation.Apply this diff:
- @Transactional(readOnly = true, propagation = Propagation.SUPPORTS) + @Transactional(readOnly = true) public TimeTableFileResponse getMemberTimeTables(LocalDate examDate) { ... - @Transactional(readOnly = true, propagation = Propagation.SUPPORTS) + @Transactional(readOnly = true) public TimeTableFileResponse getPartnerTimeTables(LocalDate examDate) {Also applies to: 47-47
src/main/java/life/mosu/mosuserver/presentation/timetable/TimeTableController.java (2)
45-50: Set typed content-type and Content-LengthUse builder APIs and include Content-Length for better client behavior.
private ResponseEntity<byte[]> buildPdfResponse(TimeTableFileResponse file) { - return ResponseEntity.ok() - .header(HttpHeaders.CONTENT_TYPE, file.contentType()) - .header(HttpHeaders.CONTENT_DISPOSITION, contentDispositionInlineUtf8(file.filename())) - .body(file.bytes()); + byte[] bytes = file.bytes(); + return ResponseEntity.ok() + .contentType(MediaType.parseMediaType(file.contentType())) + .header(HttpHeaders.CONTENT_DISPOSITION, contentDispositionInlineUtf8(file.filename())) + .contentLength(bytes.length) + .body(bytes); }
52-58: Harden filename fallback for edge casesIf normalization strips everything, fallback can become empty. Provide a safe default preserving extension.
private String contentDispositionInlineUtf8(String filename) { - String asciiFallback = Normalizer.normalize(filename, Normalizer.Form.NFKD) + String asciiFallback = Normalizer.normalize(filename, Normalizer.Form.NFKD) .replaceAll("\\p{M}+", "") .replaceAll("[^A-Za-z0-9._-]", "_"); + if (asciiFallback.isBlank()) { + String ext = ""; + int dot = filename.lastIndexOf('.'); + if (dot > -1 && dot < filename.length() - 1) { + ext = filename.substring(dot); + } + asciiFallback = "download" + ext; + } String encoded = URLEncoder.encode(filename, StandardCharsets.UTF_8).replace("+", "%20"); return "inline; filename=\"" + asciiFallback + "\"; filename*=UTF-8''" + encoded; }src/main/java/life/mosu/mosuserver/application/timetable/processor/GenerateTimeTableProcessor.java (1)
143-154: Sanitize newlines before showTextPDFBox showText can misbehave with raw CR/LF in strings. Replace line breaks with spaces.
private void drawText(PDPageContentStream cs, PDType0Font font, int size, float x, float y, String text) { try { cs.beginText(); cs.setFont(font, size); cs.newLineAtOffset(x, y); - cs.showText(text == null ? "" : text); + String safe = text == null ? "" : text.replaceAll("[\\r\\n]+", " "); + cs.showText(safe); cs.endText(); } catch (Exception e) { throw new RuntimeException("Failed to draw text", e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/static/time-table.pdfis excluded by!**/*.pdf
📒 Files selected for processing (11)
src/main/java/life/mosu/mosuserver/application/examticket/processor/GetMemberExamTicketInfoProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/application/examticket/processor/GetPartnerExamTicketInfoProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/application/timetable/TimeTableService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/timetable/processor/GenerateTimeTableProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/projection/TimeTableInfoProjection.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/repository/ExamApplicationJpaRepository.java(2 hunks)src/main/java/life/mosu/mosuserver/global/filter/Whitelist.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/cron/job/ExamNumberGenerationJobRound1.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/timetable/TimeTableController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/timetable/dto/TimeTableFileResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/timetable/dto/TimeTableInfoResponse.java(1 hunks)
🔇 Additional comments (17)
src/main/java/life/mosu/mosuserver/presentation/timetable/dto/TimeTableFileResponse.java (2)
3-8: LGTM!The record structure is appropriate for transferring PDF file data. The use of
byte[]for the file content and accompanying metadata fields (filename,contentType) provides a clean contract for file responses.
3-8: LGTM!The record definition is clean and appropriate for encapsulating file response data. The structure correctly supports the timetable PDF generation workflow.
src/main/java/life/mosu/mosuserver/domain/examapplication/projection/TimeTableInfoProjection.java (2)
3-9: LGTM!The projection record appropriately captures timetable data from repository queries with well-typed fields.
3-9: LGTM!The projection record is well-structured for repository query results and correctly encapsulates the required timetable information fields.
src/main/java/life/mosu/mosuserver/global/filter/Whitelist.java (1)
57-58: LGTM!The whitelist entry correctly exempts the timetable GET endpoints from authentication, consistent with other read-only public endpoints.
src/main/java/life/mosu/mosuserver/application/examticket/processor/GetPartnerExamTicketInfoProcessor.java (2)
47-57: LGTM!The refactoring to use direct repository streaming eliminates the intermediate list and streamlines the subject mapping. The special handling for
SOCIETY_AND_CULTURE→ "사회·문화" is correctly implemented inline while preserving ordinal-based ordering.
47-57: LGTM!The refactor to direct streaming and inline subject mapping improves efficiency and readability. The SOCIETY_AND_CULTURE localization is correctly applied.
src/main/java/life/mosu/mosuserver/application/examticket/processor/GetMemberExamTicketInfoProcessor.java (2)
31-41: LGTM!The refactoring mirrors the partner processor changes, using direct repository streaming and inline subject mapping. The implementation correctly handles
SOCIETY_AND_CULTURElocalization while maintaining ordinal-based sort order.
31-41: LGTM!The refactor maintains consistency with the partner processor and correctly handles the SOCIETY_AND_CULTURE localization.
src/main/java/life/mosu/mosuserver/application/timetable/TimeTableService.java (3)
27-27: Verify the transaction propagation choice.Using
Propagation.SUPPORTSmeans "execute within a transaction if one exists, otherwise run non-transactionally." For read-only service methods,Propagation.REQUIRED(default) or omitting the propagation attribute is more common, ensuring consistent transactional behavior.If this choice is intentional (e.g., to support both transactional and non-transactional callers), document the rationale. Otherwise, consider using the default propagation:
-@Transactional(readOnly = true, propagation = Propagation.SUPPORTS) +@Transactional(readOnly = true)
47-47: Same propagation concern asgetMemberTimeTables.Verify whether
Propagation.SUPPORTSis intentional. See the comment on Line 27.
28-45: LGTM with refactoring suggestions!The service methods correctly orchestrate timetable data retrieval and PDF generation. The transformation logic is clear and the delegation to the processor is appropriate.
Also applies to: 48-65
src/main/java/life/mosu/mosuserver/domain/examapplication/repository/ExamApplicationJpaRepository.java (3)
323-339: INNER JOINs are correct and intentional
VirtualAccountLog entries are created for every application in the registration flow, so requiring them via INNER JOIN aligns with the domain model.
12-12: LGTM!The import addition is correct and necessary for the new query methods.
304-320: Verify Profile existence or use INNER JOIN. ProfileJpaEntity.userId is non-nullable, but users without a Profile row will yield null pr.userName—confirm that every User always has a Profile created (or handle nulls), or change to an INNER JOIN if Profile is mandatory.src/main/java/life/mosu/mosuserver/presentation/timetable/TimeTableController.java (1)
27-43: Endpoints LGTM; confirm unauth access is intendedLooks clean. Since Whitelist was updated, please confirm these GETs are intentionally unauthenticated.
src/main/java/life/mosu/mosuserver/presentation/timetable/dto/TimeTableInfoResponse.java (1)
5-10: LGTM!The record structure appropriately captures timetable information for PDF generation.
| @Override | ||
| public TimeTableFileResponse process(List<TimeTableInfoResponse> list) { | ||
| try (PDDocument templateDoc = Loader.loadPDF(templatePdf); | ||
| PDDocument outDoc = new PDDocument(); | ||
| ByteArrayOutputStream out = new ByteArrayOutputStream()) { | ||
|
|
||
| PDType0Font font = PDType0Font.load(outDoc, new ByteArrayInputStream(fontBytes)); | ||
|
|
||
| int total = list.size(); | ||
| int pageCount = (total + PER_PAGE - 1) / PER_PAGE; | ||
|
|
||
| for (int p = 0; p < pageCount; p++) { | ||
| PDPage page = outDoc.importPage(templateDoc.getPage(0)); | ||
|
|
||
| try (PDPageContentStream cs = new PDPageContentStream( | ||
| outDoc, page, PDPageContentStream.AppendMode.APPEND, true)) { | ||
|
|
||
| for (int slot = 0; slot < PER_PAGE; slot++) { | ||
| int idx = p * PER_PAGE + slot; | ||
| if (idx >= total) break; | ||
|
|
||
| TimeTableInfoResponse e = list.get(idx); | ||
|
|
||
| int row = slot / COLS; // 0,1,2 | ||
| int col = slot % COLS; // 0,1 | ||
|
|
||
| float dx = col * CELL_DX; | ||
| float dy = row * CELL_DY; | ||
|
|
||
| // 수험번호 | ||
| drawText(cs, font, FONT_SIZE, | ||
| FIRST_EXAM_NUMBER_X + dx, | ||
| FIRST_EXAM_NUMBER_Y - dy, | ||
| e.examNumber()); | ||
|
|
||
| // 성명 | ||
| drawText(cs, font, FONT_SIZE, | ||
| FIRST_NAME_X + dx, | ||
| FIRST_NAME_Y - dy, | ||
| e.userName()); | ||
|
|
||
| // 탐구 과목 | ||
| String sub1 = (e.subjects() != null && e.subjects().size() > 0) ? nz(e.subjects().get(0)) : ""; | ||
| String sub2 = (e.subjects() != null && e.subjects().size() > 1) ? nz(e.subjects().get(1)) : ""; | ||
|
|
||
| drawText(cs, font, FONT_SIZE, | ||
| FIRST_SUBJECT1_X + dx, | ||
| FIRST_SUBJECT1_Y - dy, | ||
| sub1); | ||
|
|
||
| drawText(cs, font, FONT_SIZE, | ||
| FIRST_SUBJECT2_X + dx, | ||
| FIRST_SUBJECT2_Y - dy, | ||
| sub2); | ||
|
|
||
| // 학교명 | ||
| drawText(cs, font, FONT_SIZE, | ||
| FIRST_SCHOOL_X + dx, | ||
| FIRST_SCHOOL_Y - dy, | ||
| e.schoolName()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| outDoc.save(out); | ||
| return new TimeTableFileResponse(out.toByteArray(), "time-tables.pdf", "application/pdf"); | ||
|
|
||
| } catch (Exception e) { | ||
| throw new RuntimeException("Generate time-table PDF failed", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty input produces an invalid PDF (no pages)
If list is empty, outDoc has 0 pages; saving likely fails. Guard and return a valid PDF (e.g., the template) or raise a domain exception.
@Override
public TimeTableFileResponse process(List<TimeTableInfoResponse> list) {
+ if (list == null || list.isEmpty()) {
+ // Option A: return the template as a 1-page PDF
+ return new TimeTableFileResponse(templatePdf, "time-tables.pdf", "application/pdf");
+ // Option B (alternative): throw a domain exception to map to 204/404
+ // throw new NoTimeTableDataException();
+ }
try (PDDocument templateDoc = Loader.loadPDF(templatePdf);
PDDocument outDoc = new PDDocument();
ByteArrayOutputStream out = new ByteArrayOutputStream()) {🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/timetable/processor/GenerateTimeTableProcessor.java
around lines 63-133, the method builds an outDoc from entries but when the input
list is empty outDoc has zero pages and saving fails; add an early guard for
empty list: if list.isEmpty() then either load the templatePdf into a byte[] and
return a TimeTableFileResponse built from that template (so a valid PDF with
pages is returned) or throw a specific domain exception (e.g.,
NoTimeTablesException) before creating outDoc; implement the chosen behavior
consistently (load template via Loader.loadPDF(templatePdf) and write it to a
ByteArrayOutputStream to produce the response, or throw the exception) and
ensure resources are closed properly.
| try (PDPageContentStream cs = new PDPageContentStream( | ||
| outDoc, page, PDPageContentStream.AppendMode.APPEND, true)) { | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify PDPageContentStream constructor vs. PDFBox version
Using Loader suggests PDFBox 3.x; in 3.x the PDPageContentStream constructors changed (may require a resetContext parameter). Ensure this signature matches your pdfbox version.
🏁 Script executed:
#!/bin/bash
# Check declared PDFBox version(s)
rg -nP --type-add 'gradle:*.gradle' --type-add 'maven:*.xml' --type gradle --type maven -C2 '(?i)pdfbox'Length of output: 220
Use correct PDPageContentStream constructor for PDFBox 3.x
The project uses PDFBox 3.0.5, where the 4-arg constructor (PDDocument, PDPage, AppendMode, boolean) no longer exists. Update this to the 5-arg signature by adding the resetContext flag, for example:
try (PDPageContentStream cs = new PDPageContentStream(
outDoc, page, PDPageContentStream.AppendMode.APPEND, true, /*resetContext*/ true)) {
…
}🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/timetable/processor/GenerateTimeTableProcessor.java
around lines 77 to 79, the PDPageContentStream constructor call uses the removed
4-arg signature; update it to the 5-arg PDFBox 3.x signature by adding the
resetContext boolean (e.g., pass true or false as appropriate) so the call
becomes PDPageContentStream(outDoc, page, PDPageContentStream.AppendMode.APPEND,
true, true) and compile against PDFBox 3.0.5.
| List<TimeTableInfoResponse> entries = examApplicationJpaRepository.findMemberTimeTable(examDate) | ||
| .stream() | ||
| .map(info -> { | ||
| Long examApplicationId = info.examApplicationId(); | ||
| List<String> subjects = getSubjects(examApplicationId); | ||
|
|
||
| return TimeTableInfoResponse.of( | ||
| info.examNumber(), | ||
| info.userName(), | ||
| subjects, | ||
| info.schoolName() | ||
| ); | ||
| }) | ||
| .toList(); | ||
|
|
||
| return generateTimeTableProcessor.process(entries); |
There was a problem hiding this comment.
N+1 query problem: consider batch-fetching subjects.
For each TimeTableInfoProjection, getSubjects(examApplicationId) executes a separate database query. If there are N exam applications for a given date, this results in 1 query for the projections + N queries for subjects (classic N+1 problem).
Recommended solution:
Fetch all subjects for the exam applications in a single query using an IN clause:
@Transactional(readOnly = true, propagation = Propagation.SUPPORTS)
public TimeTableFileResponse getMemberTimeTables(LocalDate examDate) {
List<TimeTableInfoProjection> projections = examApplicationJpaRepository.findMemberTimeTable(examDate);
// Batch fetch all subjects
List<Long> examApplicationIds = projections.stream()
.map(TimeTableInfoProjection::examApplicationId)
.toList();
Map<Long, List<String>> subjectsByApplicationId = examSubjectJpaRepository
.findByExamApplicationIdIn(examApplicationIds)
.stream()
.collect(Collectors.groupingBy(
ExamSubjectJpaEntity::getExamApplicationId,
Collectors.mapping(
entity -> {
Subject subject = entity.getSubject();
if (subject == Subject.SOCIETY_AND_CULTURE) {
return "사회·문화";
}
return subject.getSubjectName();
},
Collectors.toList()
)
));
// Sort subjects for each application
subjectsByApplicationId.replaceAll((id, subjects) ->
subjects.stream()
.sorted(/* by original Subject ordinal - you'll need to preserve that info */)
.toList()
);
List<TimeTableInfoResponse> entries = projections.stream()
.map(info -> TimeTableInfoResponse.of(
info.examNumber(),
info.userName(),
subjectsByApplicationId.getOrDefault(info.examApplicationId(), List.of()),
info.schoolName()
))
.toList();
return generateTimeTableProcessor.process(entries);
}Note: You'll need to add a findByExamApplicationIdIn(List<Long>) method to ExamSubjectJpaRepository. For the sorting to work correctly with batch fetching, you may need to preserve ordinal information or fetch the subjects in a way that allows reconstruction of the original order.
| List<TimeTableInfoResponse> entries = examApplicationJpaRepository.findPartnerTimeTable(examDate) | ||
| .stream() | ||
| .map(info -> { | ||
| Long examApplicationId = info.examApplicationId(); | ||
| List<String> subjects = getSubjects(examApplicationId); | ||
|
|
||
| return TimeTableInfoResponse.of( | ||
| info.examNumber(), | ||
| info.userName(), | ||
| subjects, | ||
| info.schoolName() | ||
| ); | ||
| }) | ||
| .toList(); | ||
|
|
||
| return generateTimeTableProcessor.process(entries); |
There was a problem hiding this comment.
Apply the same N+1 query fix here.
This method has the same N+1 query problem as getMemberTimeTables. Apply the batch-fetching pattern described in the previous comment.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/timetable/TimeTableService.java
around lines 49 to 64, this method causes an N+1 query by calling
getSubjects(examApplicationId) inside the stream map; change it to batch-fetch
subjects for all examApplicationIds at once: collect all examApplicationId
values from the repository result into a list/set, call the repository method
that returns all subjects for those ids in one query, build a Map<Long,
List<String>> keyed by examApplicationId, then replace the per-item getSubjects
call with a lookup into that map when constructing each TimeTableInfoResponse so
the stream mapping uses the pre-fetched subject lists.
| private List<String> getSubjects(Long examApplicationId) { | ||
| return examSubjectJpaRepository.findByExamApplicationId(examApplicationId) | ||
| .stream() | ||
| .map(ExamSubjectJpaEntity::getSubject) | ||
| .sorted(Comparator.comparingInt(Subject::ordinal)) | ||
| .map(subject -> { | ||
| if (subject == Subject.SOCIETY_AND_CULTURE) { | ||
| return "사회·문화"; | ||
| } | ||
| return subject.getSubjectName(); | ||
| }) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated subject mapping logic to eliminate code duplication.
The subject mapping logic (fetch → sort by ordinal → map SOCIETY_AND_CULTURE → convert to names) is duplicated in three places:
GetMemberExamTicketInfoProcessor(lines 35-40)GetPartnerExamTicketInfoProcessor(lines 51-56)- This method (lines 67-79)
This violates DRY and makes maintenance harder if the mapping rules change.
Consider extracting to a shared utility or helper:
@Component
public class SubjectMappingHelper {
private final ExamSubjectJpaRepository examSubjectJpaRepository;
public List<String> getLocalizedSubjectNames(Long examApplicationId) {
return examSubjectJpaRepository.findByExamApplicationId(examApplicationId)
.stream()
.map(ExamSubjectJpaEntity::getSubject)
.sorted(Comparator.comparingInt(Subject::ordinal))
.map(subject -> {
if (subject == Subject.SOCIETY_AND_CULTURE) {
return "사회·문화";
}
return subject.getSubjectName();
})
.toList();
}
}Then inject and use this helper in all three locations:
List<String> subjects = subjectMappingHelper.getLocalizedSubjectNames(examApplicationId);| AND e.examDate = :examDate | ||
| ORDER BY ea.examNumber | ||
| """) | ||
| List<TimeTableInfoProjection> findMemberTimeTable(@Param("examDate")LocalDate examDate); |
There was a problem hiding this comment.
Add spacing after annotation.
Missing space after @Param("examDate") before the parameter type.
Apply this diff:
- List<TimeTableInfoProjection> findMemberTimeTable(@Param("examDate")LocalDate examDate);
+ List<TimeTableInfoProjection> findMemberTimeTable(@Param("examDate") LocalDate examDate);📝 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<TimeTableInfoProjection> findMemberTimeTable(@Param("examDate")LocalDate examDate); | |
| List<TimeTableInfoProjection> findMemberTimeTable(@Param("examDate") LocalDate examDate); |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/domain/examapplication/repository/ExamApplicationJpaRepository.java
around line 320, the method signature lacks a space between the @Param
annotation and the parameter type; change it so there is a single space after
the annotation (i.e., replace '@Param("examDate")LocalDate examDate' with
'@Param("examDate") LocalDate examDate') to conform to formatting conventions.
| @Query(""" | ||
| SELECT new life.mosu.mosuserver.domain.examapplication.projection.TimeTableInfoProjection( | ||
| ea.id, | ||
| ea.examNumber, | ||
| u.name, | ||
| e.schoolName | ||
| ) | ||
| FROM ExamApplicationJpaEntity ea | ||
| JOIN ApplicationJpaEntity a on a.id = ea.applicationId | ||
| JOIN UserJpaEntity u on a.userId = u.id | ||
| JOIN VirtualAccountLogJpaEntity v on v.applicationId = a.id | ||
| JOIN ExamJpaEntity e on ea.examId = e.id | ||
| WHERE v.depositStatus = 'DONE' | ||
| AND e.examDate = :examDate | ||
| ORDER BY ea.examNumber | ||
| """) | ||
| List<TimeTableInfoProjection> findPartnerTimeTable(@Param("examDate")LocalDate examDate); |
There was a problem hiding this comment.
Inconsistent JOIN type between member and partner queries.
The findPartnerTimeTable query uses INNER JOINs while findMemberTimeTable (lines 304-320) uses LEFT JOINs. This inconsistency means:
- Partner query: Excludes rows if Application, User, VirtualAccountLog, or Exam is missing
- Member query: Includes rows even if Payment, Exam, User, or Profile is missing (subject to WHERE filters)
Verify that this difference is intentional. If both queries should handle missing data the same way, standardize the JOIN types.
Apply this diff if INNER JOINs are appropriate for both:
@Query("""
SELECT new life.mosu.mosuserver.domain.examapplication.projection.TimeTableInfoProjection(
ea.id,
ea.examNumber,
pr.userName,
e.schoolName
)
FROM ExamApplicationJpaEntity ea
- LEFT JOIN PaymentJpaEntity p on p.examApplicationId = ea.id
- LEFT JOIN ExamJpaEntity e on ea.examId = e.id
- LEFT JOIN UserJpaEntity u on ea.userId = u.id
- LEFT JOIN ProfileJpaEntity pr on pr.userId = u.id
+ JOIN PaymentJpaEntity p on p.examApplicationId = ea.id
+ JOIN ExamJpaEntity e on ea.examId = e.id
+ JOIN UserJpaEntity u on ea.userId = u.id
+ JOIN ProfileJpaEntity pr on pr.userId = u.id
WHERE p.paymentStatus = 'DONE'
AND e.examDate = :examDate
ORDER BY ea.examNumber
""")
List<TimeTableInfoProjection> findMemberTimeTable(@Param("examDate")LocalDate examDate);Or if LEFT JOINs are correct for both, update the partner query accordingly.
| EXAM_TICKET_GUEST("/api/v1/exam-ticket", WhitelistMethod.GET), | ||
| ADMIN_IMPORT_CSV("/api/v1/admin/applications/import", WhitelistMethod.ALL); | ||
| ADMIN_IMPORT_CSV("/api/v1/admin/applications/import", WhitelistMethod.ALL), | ||
| TIME_TABLE("/api/v1/time-table", WhitelistMethod.GET); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the security implications of public timetable access.
The TIME_TABLE whitelist entry allows unauthenticated GET access to /api/v1/time-table/* endpoints. Since timetables contain PII (user names, exam numbers, school names), confirm that public access is intentional and complies with your privacy requirements.
Run the following script to verify what endpoints will be publicly accessible:
Additionally, consider if these endpoints should require authentication or at minimum some form of access control (e.g., date-based restrictions, API keys for partners).
🏁 Script executed:
#!/bin/bash
# Description: Identify all timetable controller endpoints that will be publicly accessible
# Find TimeTableController and list its endpoints
ast-grep --pattern $'@RestController
$$$
class TimeTableController {
$$$
}'
# Also search for @GetMapping, @PostMapping annotations in TimeTableController
rg -A 5 '@(Get|Post|Put|Delete)Mapping' --type java -g '**/TimeTableController.java'Length of output: 1701
Restrict unauthenticated access to timetable endpoints.
The TIME_TABLE whitelist entry (Whitelist.java:58) currently allows unauthenticated GET access to /api/v1/time-table/member and /api/v1/time-table/partner, exposing PII in PDFs. Confirm if this is intentional; otherwise enforce authentication or partner-specific access control.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/Whitelist.java around line
58, the TIME_TABLE whitelist entry currently permits unauthenticated GET access
to /api/v1/time-table (which also covers /member and /partner) and may expose
PII in generated PDFs; remove or narrow this whitelist entry and require
authentication (or role/partner-scoped authorization) for those endpoints
instead — either remove TIME_TABLE from the public whitelist and ensure the
security filter enforces authentication for /api/v1/time-table/**, or replace
the single permissive entry with explicit, authenticated-only entries and add
partner-specific access checks in the controller/service layer so only
authorized users (and partner accounts when applicable) can retrieve timetable
PDFs.
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit