refactor: semester 기반 시간표/위시리스트 조작 정합성 강화#17
Conversation
Summary of ChangesHello @coldmans, 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! 이 Pull Request는 시간표 및 위시리스트 관리 기능의 데이터 정합성을 강화하기 위해 학기(semester) 정보를 모든 관련 작업에 통합합니다. 이를 통해 다른 학기의 데이터가 의도치 않게 오염되거나 삭제되는 것을 방지하고, 중복 및 삭제 불일치 케이스를 줄여 사용자 경험의 안정성을 향상시키는 것을 목표로 합니다. 기존 인증(auth) 리팩토링 PR과의 충돌을 최소화하기 위해 시간표/위시리스트 도메인 파일만 수정되었습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
![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은 semester 파라미터를 추가하여 시간표 및 위시리스트 기능의 데이터 정합성을 강화하는 중요한 리팩토링을 수행합니다. 이를 통해 다른 학기의 데이터가 실수로 변경되거나 삭제되는 것을 방지할 수 있습니다. 전반적으로 좋은 방향의 개선이지만, 몇 가지 잠재적인 위험과 개선점을 발견했습니다.
특히, semester가 제공되지 않았을 때 모든 학기의 데이터를 삭제할 수 있는 삭제 쿼리는 의도치 않은 데이터 손실을 유발할 수 있어 수정이 필요해 보입니다. 또한, 컨트롤러에서 Map 대신 DTO를 사용하고 사용되지 않는 레포지토리 메서드를 제거하여 코드 품질을 더욱 향상시킬 수 있습니다.
| @Query("DELETE FROM UserTimetable ut WHERE ut.user.id = :userId AND ut.subject.id = :subjectId AND (:semester IS NULL OR ut.semester = :semester)") | ||
| int deleteByUserIdAndSubjectIdAndSemester(@Param("userId") Long userId, @Param("subjectId") Long subjectId, @Param("semester") String semester); |
There was a problem hiding this comment.
semester 파라미터가 null일 경우, (:semester IS NULL OR ut.semester = :semester) 조건이 항상 참이 되어 userId와 subjectId가 일치하는 모든 시간표 항목을 삭제하게 됩니다. 이는 PR의 목표인 "다른 학기 데이터 오삭제 방지"와 상충되며, 의도치 않은 데이터 손실을 유발할 수 있는 위험한 동작입니다.
semester가 null일 때의 동작을 명확히 정의하고, 모든 데이터를 삭제하는 대신 예외를 발생시키거나, 특정 학기만 삭제하도록 로직을 수정하는 것을 권장합니다. 예를 들어, 서비스 레이어에서 semester가 null인 경우를 처리하고, 레포지토리에서는 특정 학기만 삭제하도록 할 수 있습니다.
| @Query("DELETE FROM WishlistItem w WHERE w.user.id = :userId AND w.subject.id = :subjectId AND (:semester IS NULL OR w.semester = :semester)") | ||
| int deleteByUserIdAndSubjectIdAndSemester(@Param("userId") Long userId, @Param("subjectId") Long subjectId, @Param("semester") String semester); |
There was a problem hiding this comment.
semester 파라미터가 null일 경우, (:semester IS NULL OR w.semester = :semester) 조건이 항상 참이 되어 userId와 subjectId가 일치하는 모든 위시리스트 항목을 삭제하게 됩니다. 이는 의도치 않은 데이터 손실로 이어질 수 있는 위험한 동작이며, PR의 목표인 데이터 정합성 강화와 상충됩니다.
UserTimetableRepository의 경우와 마찬가지로, semester가 null일 때의 동작을 보다 안전하게 수정하는 것을 권장합니다. 예를 들어, semester가 명시적으로 제공될 때만 삭제가 수행되도록 제한할 수 있습니다.
| Optional<UserTimetable> findByUserIdAndSubjectId(Long userId, Long subjectId); | ||
| Optional<UserTimetable> findByUserIdAndSubjectIdAndSemester(Long userId, Long subjectId, String semester); | ||
|
|
||
| java.util.List<UserTimetable> findByUserIdAndSubjectId(Long userId, Long subjectId); |
There was a problem hiding this comment.
새로 추가된 findByUserIdAndSubjectId 메소드가 현재 코드베이스에서 사용되지 않는 것으로 보입니다. 사용되지 않는 코드는 혼란을 줄 수 있고 유지보수 비용을 증가시키므로, 특별한 의도가 없다면 제거하는 것이 좋습니다. 만약 향후 사용될 예정이라면, 해당 목적을 설명하는 주석을 추가하는 것을 고려해 주세요.
| @Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s WHERE w.user.id = :userId AND s.id = :subjectId AND w.semester = :semester") | ||
| Optional<WishlistItem> findByUserIdAndSubjectIdAndSemester(@Param("userId") Long userId, @Param("subjectId") Long subjectId, @Param("semester") String semester); | ||
|
|
||
| List<WishlistItem> findByUserIdAndSubjectId(Long userId, Long subjectId); |
There was a problem hiding this comment.
새로 추가된 findByUserIdAndSubjectId 메소드가 현재 코드베이스에서 사용되지 않는 것으로 보입니다. 코드의 간결성과 유지보수성을 위해, 사용되지 않는 코드는 제거하는 것이 좋습니다.
There was a problem hiding this comment.
Pull request overview
시간표(timetable)와 위시리스트(wishlist) 조작을 userId + subjectId + semester 기준으로 수행하도록 리팩토링해, 학기 간 데이터 오염/오삭제를 줄이려는 PR입니다.
Changes:
- wishlist/timetable 조회·수정·삭제 경로에
semester기준을 반영하도록 서비스/리포지토리 메서드 조정 - remove API에
semester(optional) 파라미터 추가 및 delete 쿼리를 “삭제된 row 수 반환” 방식으로 변경 - memo/priority/required 업데이트도 semester 기반 조회로 변경
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/inu/timetable/service/WishlistService.java |
wishlist 조회/삭제/업데이트를 semester 기반으로 변경 |
src/main/java/inu/timetable/service/TimetableService.java |
timetable 조회/삭제/메모 업데이트를 semester 기반으로 변경 및 delete 최적화 |
src/main/java/inu/timetable/repository/WishlistRepository.java |
semester 조건 포함 조회/삭제 쿼리 및 삭제 row count 반환 도입 |
src/main/java/inu/timetable/repository/UserTimetableRepository.java |
semester 조건 포함 조회/삭제 메서드 추가 및 delete row count 반환 도입 |
src/main/java/inu/timetable/controller/WishlistController.java |
remove/priority/required API에서 semester 전달 추가 |
src/main/java/inu/timetable/controller/TimetableController.java |
remove/memo API에서 semester 전달 추가 |
Comments suppressed due to low confidence (2)
src/main/java/inu/timetable/service/TimetableService.java:82
updateMemonow depends onsemester, but the controller pulls it from the body without validation and may pass null. In that case the semester-specific lookup may fail unexpectedly (or target only null-semester rows). If semester is required for correctness, validate it and return a clear 400 error when missing.
public UserTimetable updateMemo(Long userId, Long subjectId, String semester, String memo) {
UserTimetable userTimetable = userTimetableRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester)
.orElseThrow(() -> new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다."));
userTimetable.setMemo(memo);
return userTimetableRepository.save(userTimetable);
src/main/java/inu/timetable/service/WishlistService.java:85
updateRequiredhas the same issue asupdatePriority:semestermay be null from the request body, but the lookup is semester-equality based. If semester is meant to be required for correctness, validate it and return a clear 400 error when missing (instead of a generic "찾을 수 없습니다" due to a null lookup).
public WishlistItem updateRequired(Long userId, Long subjectId, String semester, Boolean isRequired) {
WishlistItem wishlistItem = wishlistRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester)
.orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다."));
wishlistItem.setIsRequired(isRequired);
return wishlistRepository.save(wishlistItem);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| List<WishlistItem> findByUserIdAndSubjectId(Long userId, Long subjectId); | ||
|
|
There was a problem hiding this comment.
These repositories introduce findByUserIdAndSubjectId(...) methods that are not referenced anywhere in the codebase (verified via search). If they’re only for transitional/debug purposes, consider removing them or adding a comment explaining the intended use; otherwise they add dead API surface and confusion (especially since they return cross-semester results).
| List<WishlistItem> findByUserIdAndSubjectId(Long userId, Long subjectId); | |
|
|
||
| java.util.List<UserTimetable> findByUserIdAndSubjectId(Long userId, Long subjectId); |
There was a problem hiding this comment.
findByUserIdAndSubjectId(...) is declared but not used anywhere (verified via search). If it’s not needed for the refactor, consider removing it to keep the repository API focused; if it is needed for handling null/legacy semester cases, add the corresponding service logic so it doesn’t remain unused.
| java.util.List<UserTimetable> findByUserIdAndSubjectId(Long userId, Long subjectId); |
|
|
||
| @Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s WHERE w.user.id = :userId AND s.id = :subjectId") | ||
| Optional<WishlistItem> findByUserIdAndSubjectId(@Param("userId") Long userId, @Param("subjectId") Long subjectId); | ||
| @Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s WHERE w.user.id = :userId AND s.id = :subjectId AND w.semester = :semester") |
There was a problem hiding this comment.
findByUserIdAndSubjectIdAndSemester uses w.semester = :semester. If the caller passes semester=null (which can happen from the controllers), Hibernate will generate semester = NULL and the row will not be found even when w.semester is null. Either enforce non-null/blank semester at the API boundary, or change the query to handle null explicitly (e.g., match both-null via IS NULL).
| @Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s WHERE w.user.id = :userId AND s.id = :subjectId AND w.semester = :semester") | |
| @Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s " + | |
| "WHERE w.user.id = :userId AND s.id = :subjectId " + | |
| "AND ((:semester IS NULL AND w.semester IS NULL) OR w.semester = :semester)") |
| public void removeFromWishlist(Long userId, Long subjectId, String semester) { | ||
| int deleted = wishlistRepository.deleteByUserIdAndSubjectIdAndSemester(userId, subjectId, semester); | ||
| if (deleted == 0) { | ||
| throw new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
removeFromWishlist can be invoked with semester=null (controller marks it optional), and the repository delete query will then delete across all semesters. Also, the logic only checks deleted == 0, so accidental multi-row deletes won't be detected. Recommend requiring semester here, or handling the null case safely (e.g., resolve to exactly one target row before deleting, and error if ambiguous).
| public void removeFromWishlist(Long userId, Long subjectId, String semester) { | |
| int deleted = wishlistRepository.deleteByUserIdAndSubjectIdAndSemester(userId, subjectId, semester); | |
| if (deleted == 0) { | |
| throw new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다."); | |
| } | |
| public void removeFromWishlist(Long userId, Long subjectId, String semester) { | |
| if (semester == null || semester.trim().isEmpty()) { | |
| throw new IllegalArgumentException("학기를 지정해야 위시리스트에서 과목을 삭제할 수 있습니다."); | |
| } | |
| int deleted = wishlistRepository.deleteByUserIdAndSubjectIdAndSemester(userId, subjectId, semester); | |
| if (deleted == 0) { | |
| throw new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다."); | |
| } | |
| if (deleted > 1) { | |
| throw new RuntimeException("위시리스트에서 여러 개의 항목이 삭제되었습니다. 관리자에게 문의하세요."); | |
| } |
| public WishlistItem updatePriority(Long userId, Long subjectId, String semester, Integer priority) { | ||
| WishlistItem wishlistItem = wishlistRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester) | ||
| .orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.")); | ||
|
|
||
| wishlistItem.setPriority(priority); | ||
| return wishlistRepository.save(wishlistItem); |
There was a problem hiding this comment.
updatePriority now depends on a semester argument, but the controller can pass null (body field not required). With the current repository query, null semester will likely fail to find existing rows (or target only rows with null semester), breaking updates for existing data. Consider validating semester is present/non-blank for this endpoint (or providing a deterministic default) before calling the repository.
| java.util.List<UserTimetable> findByUserIdAndSubjectId(Long userId, Long subjectId); | ||
|
|
||
| @Modifying | ||
| @Query("DELETE FROM UserTimetable ut WHERE ut.user.id = :userId AND ut.subject.id = :subjectId AND (:semester IS NULL OR ut.semester = :semester)") |
There was a problem hiding this comment.
deleteByUserIdAndSubjectIdAndSemester will delete across all semesters when semester=null due to (:semester IS NULL OR ...). Given the refactor goal (학기 정합성), it’s safer to require semester for this operation or explicitly fail when semester is missing and multiple semester rows exist.
| @Query("DELETE FROM UserTimetable ut WHERE ut.user.id = :userId AND ut.subject.id = :subjectId AND (:semester IS NULL OR ut.semester = :semester)") | |
| @Query("DELETE FROM UserTimetable ut WHERE ut.user.id = :userId AND ut.subject.id = :subjectId AND ut.semester = :semester") |
| Optional<UserTimetable> findByUserIdAndSubjectId(Long userId, Long subjectId); | ||
| Optional<UserTimetable> findByUserIdAndSubjectIdAndSemester(Long userId, Long subjectId, String semester); | ||
|
|
||
| java.util.List<UserTimetable> findByUserIdAndSubjectId(Long userId, Long subjectId); |
There was a problem hiding this comment.
Minor inconsistency: this repository already imports java.util.List but the new method uses a fully-qualified java.util.List<> type. Prefer using the imported List for consistency/readability.
| java.util.List<UserTimetable> findByUserIdAndSubjectId(Long userId, Long subjectId); | |
| List<UserTimetable> findByUserIdAndSubjectId(Long userId, Long subjectId); |
| try { | ||
| Long userId = Long.valueOf(request.get("userId").toString()); | ||
| Long subjectId = Long.valueOf(request.get("subjectId").toString()); | ||
| String semester = (String) request.get("semester"); | ||
| String memo = (String) request.get("memo"); | ||
|
|
||
| UserTimetable userTimetable = timetableService.updateMemo(userId, subjectId, memo); | ||
| UserTimetable userTimetable = timetableService.updateMemo(userId, subjectId, semester, memo); | ||
| return ResponseEntity.ok(userTimetable); |
There was a problem hiding this comment.
semester is optional in this request body, but the service update is semester-specific. Without validating presence/non-blank semester, clients that omit it will get inconsistent behavior or failures. Recommend rejecting missing/blank semester with 400 (or implementing an explicit fallback semester selection).
| @Modifying | ||
| @Query("DELETE FROM WishlistItem w WHERE w.user.id = :userId AND w.subject.id = :subjectId") | ||
| void deleteByUserIdAndSubjectId(@Param("userId") Long userId, @Param("subjectId") Long subjectId); | ||
| @Query("DELETE FROM WishlistItem w WHERE w.user.id = :userId AND w.subject.id = :subjectId AND (:semester IS NULL OR w.semester = :semester)") |
There was a problem hiding this comment.
deleteByUserIdAndSubjectIdAndSemester treats semester as optional (:semester IS NULL OR ...), so calling it with semester=null will delete the subject across all semesters for that user. That undermines the PR goal of preventing cross-semester 오삭제. Consider requiring semester for this delete path (controller/service validation), or if backward-compat is needed, first detect multiple matches and fail when semester is missing rather than deleting broadly.
| @Query("DELETE FROM WishlistItem w WHERE w.user.id = :userId AND w.subject.id = :subjectId AND (:semester IS NULL OR w.semester = :semester)") | |
| @Query("DELETE FROM WishlistItem w WHERE w.user.id = :userId AND w.subject.id = :subjectId AND w.semester = :semester") |
| @Transactional | ||
| public void removeSubjectFromTimetable(Long userId, Long subjectId, String semester) { | ||
| int deleted = userTimetableRepository.deleteByUserIdAndSubjectIdAndSemester(userId, subjectId, semester); | ||
| if (deleted == 0) { | ||
| throw new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
removeSubjectFromTimetable can be called with semester=null (controller marks it optional). With the current delete query, that will remove the subject from all semesters for the user, and deleted == 0 won’t catch multi-row deletes. Consider requiring semester here, or safely handling the null case by resolving to a single target row before deleting.
|
리뷰 코멘트 반영 완료했습니다.
재리뷰 부탁드립니다. |
변경 사항
semester(optional) 파라미터 추가userId + subjectId + semester기준으로 조회/수정/삭제findByUserIdAndSubjectIdAndSemesterdeleteByUserIdAndSubjectIdAndSemester분리 목적
기대 효과