fix: 시간표/위시리스트 중복 삽입 방지(유니크 제약 + 트랜잭션)#15
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는 시간표 및 위시리스트 기능에서 동일 과목이 중복으로 추가되는 문제를 해결하고 데이터 정합성을 강화하는 것을 목표로 합니다. 데이터베이스 레벨에서 유니크 제약 조건을 추가하고, 서비스 로직에서 트랜잭션 처리 및 예외 처리를 개선하여 동시성 상황에서의 안정성을 높였습니다. 또한, 삭제 및 수정 로직을 개선하여 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
Activity
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}
| @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); | ||
| Optional<WishlistItem> findByUserIdAndSubjectIdAndSemester(@Param("userId") Long userId, @Param("subjectId") Long subjectId, @Param("semester") String semester); |
There was a problem hiding this comment.
findByUserIdAndSubjectIdAndSemester 메소드의 JPQL 쿼리에 semester 조건이 누락되었습니다.
@Query 어노테이션에 AND w.semester = :semester 조건을 추가해야 의도대로 특정 학기의 항목만 조회할 수 있습니다. 현재 구현은 학기에 상관없이 첫 번째로 찾아진 과목을 반환하여, 다른 학기에 동일 과목이 있을 경우 잘못된 데이터를 다룰 수 있습니다.
| @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); | |
| Optional<WishlistItem> findByUserIdAndSubjectIdAndSemester(@Param("userId") Long userId, @Param("subjectId") Long subjectId, @Param("semester") String semester); | |
| @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); |
| Optional<UserTimetable> existing = userTimetableRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester); | ||
| if (existing.isPresent()) { | ||
| throw new RuntimeException("이미 시간표에 추가된 과목입니다."); | ||
| } |
There was a problem hiding this comment.
데이터베이스에 UNIQUE 제약 조건이 있고, DataIntegrityViolationException을 try-catch로 처리하여 중복 삽입을 방지하고 있으므로, save() 호출 전 existing.isPresent()를 통한 중복 확인 로직은 불필요해 보입니다.
이 사전 검사는 동시성 환경에서 완벽한 해결책이 되지 못하며(race condition 발생 가능), try-catch 블록이 이미 최종적인 데이터 정합성을 보장하고 있습니다.
사전 검사를 제거하면 코드가 더 간결해지고, 데이터 정합성 보장 책임을 데이터베이스 제약 조건으로 일원화할 수 있습니다.
| Optional<WishlistItem> existing = wishlistRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester); | ||
| if (existing.isPresent()) { | ||
| throw new RuntimeException("이미 위시리스트에 추가된 과목입니다."); | ||
| } |
There was a problem hiding this comment.
TimetableService와 마찬가지로, 여기서도 save() 호출 전의 중복 확인 로직은 DataIntegrityViolationException 처리 로직과 중복됩니다.
데이터베이스의 UNIQUE 제약 조건을 신뢰하고 예외 처리를 통해 중복 삽입을 막는 것이 더 견고하고 간결한 방법입니다. 이 사전 검사를 제거하는 것을 고려해보세요.
There was a problem hiding this comment.
Pull request overview
This PR enhances data integrity for the timetable and wishlist features by implementing database-level unique constraints and improving concurrent request handling. The changes prevent duplicate entries that could occur during rapid successive requests (double-clicks, concurrent tabs) by enforcing uniqueness at the database level using (user_id, subject_id, semester) composite keys.
Changes:
- Added unique constraints on
user_timetablesandwishlist_itemstables for(user_id, subject_id, semester)combination - Enhanced service layer with
@Transactionalannotations andDataIntegrityViolationExceptionhandling - Updated repository methods and controllers to consistently use semester parameter for CRUD operations
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/inu/timetable/entity/UserTimetable.java |
Added unique constraint annotation for user-subject-semester combination |
src/main/java/inu/timetable/entity/WishlistItem.java |
Added unique constraint annotation for user-subject-semester combination |
src/main/java/inu/timetable/repository/UserTimetableRepository.java |
Added semester-aware query methods and conditional delete operation |
src/main/java/inu/timetable/repository/WishlistRepository.java |
Added semester-aware query methods and conditional delete operation |
src/main/java/inu/timetable/service/TimetableService.java |
Added transactions, semester-based duplicate checks, and exception handling |
src/main/java/inu/timetable/service/WishlistService.java |
Added transactions, semester-based duplicate checks, and exception handling |
src/main/java/inu/timetable/controller/TimetableController.java |
Updated endpoints to accept optional semester parameter |
src/main/java/inu/timetable/controller/WishlistController.java |
Updated endpoints to accept optional semester parameter |
scripts/2026-02-13_add_unique_constraints.sql |
PostgreSQL migration script to cleanup duplicates and add constraints |
README.md |
Documented data integrity feature and race condition prevention |
.gitignore |
Added exception to allow SQL scripts in the scripts directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -23,9 +23,11 @@ public interface WishlistRepository extends JpaRepository<WishlistItem, Long> { | |||
| List<WishlistItem> findByUserIdAndSemesterWithSubjectAndSchedules(@Param("userId") Long userId, @Param("semester") String semester); | |||
|
|
|||
| @Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s WHERE w.user.id = :userId AND s.id = :subjectId") | |||
There was a problem hiding this comment.
The JPQL query does not include the semester parameter in the WHERE clause. The query should filter by semester as well to match the method signature. The current query "SELECT w FROM WishlistItem w JOIN FETCH w.subject s WHERE w.user.id = :userId AND s.id = :subjectId" is missing "AND w.semester = :semester".
| @Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s WHERE w.user.id = :userId AND s.id = :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") |
| public WishlistItem updatePriority(Long userId, Long subjectId, String semester, Integer priority) { | ||
| WishlistItem wishlistItem; | ||
| if (semester != null && !semester.isBlank()) { | ||
| wishlistItem = wishlistRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester) | ||
| .orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.")); | ||
| } else { | ||
| wishlistItem = wishlistRepository.findByUserIdAndSubjectId(userId, subjectId).stream() | ||
| .findFirst() | ||
| .orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.")); | ||
| } |
There was a problem hiding this comment.
The update methods have fallback logic that calls findByUserIdAndSubjectId when semester is null or blank, but this method returns a List, not an Optional. Using .stream().findFirst() on a list that could contain multiple items from different semesters introduces ambiguity. Since the unique constraint is now on (user_id, subject_id, semester), the same user-subject combination can exist across multiple semesters. When semester is not provided, it's unclear which one to update. Consider either making semester required for these update operations, or documenting that the fallback behavior updates an arbitrary entry when multiple exist.
| public WishlistItem updateRequired(Long userId, Long subjectId, String semester, Boolean isRequired) { | ||
| WishlistItem wishlistItem; | ||
| if (semester != null && !semester.isBlank()) { | ||
| wishlistItem = wishlistRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester) | ||
| .orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.")); | ||
| } else { | ||
| wishlistItem = wishlistRepository.findByUserIdAndSubjectId(userId, subjectId).stream() | ||
| .findFirst() | ||
| .orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.")); | ||
| } |
There was a problem hiding this comment.
The update methods have fallback logic that calls findByUserIdAndSubjectId when semester is null or blank, but this method returns a List, not an Optional. Using .stream().findFirst() on a list that could contain multiple items from different semesters introduces ambiguity. Since the unique constraint is now on (user_id, subject_id, semester), the same user-subject combination can exist across multiple semesters. When semester is not provided, it's unclear which one to update. Consider either making semester required for these update operations, or documenting that the fallback behavior updates an arbitrary entry when multiple exist.
| @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.
The delete query uses "(:semester IS NULL OR w.semester = :semester)" which allows deleting all entries for a user-subject pair when semester is null. However, the service method removeFromWishlist now accepts an optional semester parameter from the controller. This creates ambiguity: if the frontend doesn't provide semester, all matching entries across all semesters will be deleted. Given that the PR's goal is to enforce semester-based uniqueness, consider either making semester required for delete operations or clearly documenting this behavior, as it may not be the intended user experience (deleting from one semester but accidentally removing from all semesters).
| @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") |
| public UserTimetable updateMemo(Long userId, Long subjectId, String semester, String memo) { | ||
| UserTimetable userTimetable; | ||
| if (semester != null && !semester.isBlank()) { | ||
| userTimetable = userTimetableRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester) | ||
| .orElseThrow(() -> new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다.")); | ||
| } else { | ||
| userTimetable = userTimetableRepository.findByUserIdAndSubjectId(userId, subjectId).stream() | ||
| .findFirst() | ||
| .orElseThrow(() -> new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다.")); | ||
| } | ||
|
|
||
| userTimetable.setMemo(memo); | ||
| return userTimetableRepository.save(userTimetable); | ||
| } |
There was a problem hiding this comment.
The updateMemo method performs database writes but is not marked with @transactional. While Spring Data JPA handles simple saves within a transaction context, it's a best practice to explicitly mark write operations with @transactional for consistency with other write methods in this service (addSubjectToTimetable and removeSubjectFromTimetable are both marked @transactional). This ensures transaction boundaries are clearly defined and consistent.
| UserTimetable userTimetable; | ||
| if (semester != null && !semester.isBlank()) { | ||
| userTimetable = userTimetableRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester) | ||
| .orElseThrow(() -> new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다.")); | ||
| } else { | ||
| userTimetable = userTimetableRepository.findByUserIdAndSubjectId(userId, subjectId).stream() | ||
| .findFirst() | ||
| .orElseThrow(() -> new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다.")); | ||
| } | ||
|
|
There was a problem hiding this comment.
The update method has fallback logic that calls findByUserIdAndSubjectId when semester is null or blank, but this method returns a List, not an Optional. Using .stream().findFirst() on a list that could contain multiple items from different semesters introduces ambiguity. Since the unique constraint is now on (user_id, subject_id, semester), the same user-subject combination can exist across multiple semesters. When semester is not provided, it's unclear which one to update. Consider either making semester required for this update operation, or documenting that the fallback behavior updates an arbitrary entry when multiple exist.
| UserTimetable userTimetable; | |
| if (semester != null && !semester.isBlank()) { | |
| userTimetable = userTimetableRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester) | |
| .orElseThrow(() -> new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다.")); | |
| } else { | |
| userTimetable = userTimetableRepository.findByUserIdAndSubjectId(userId, subjectId).stream() | |
| .findFirst() | |
| .orElseThrow(() -> new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다.")); | |
| } | |
| if (semester == null || semester.isBlank()) { | |
| throw new IllegalArgumentException("semester는 메모 수정 시 필수 입력 값입니다."); | |
| } | |
| UserTimetable userTimetable = userTimetableRepository | |
| .findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester) | |
| .orElseThrow(() -> new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다.")); |
| 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.
The delete query uses "(:semester IS NULL OR ut.semester = :semester)" which allows deleting all entries for a user-subject pair when semester is null. However, the service method removeSubjectFromTimetable now accepts an optional semester parameter from the controller. This creates ambiguity: if the frontend doesn't provide semester, all matching entries across all semesters will be deleted. Given that the PR's goal is to enforce semester-based uniqueness, consider either making semester required for delete operations or clearly documenting this behavior, as it may not be the intended user experience (deleting from one semester but accidentally removing from all semesters).
| @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") |
| public WishlistItem updatePriority(Long userId, Long subjectId, String semester, Integer priority) { | ||
| WishlistItem wishlistItem; | ||
| if (semester != null && !semester.isBlank()) { | ||
| wishlistItem = wishlistRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester) | ||
| .orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.")); | ||
| } else { | ||
| wishlistItem = wishlistRepository.findByUserIdAndSubjectId(userId, subjectId).stream() | ||
| .findFirst() | ||
| .orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.")); | ||
| } | ||
|
|
||
| wishlistItem.setPriority(priority); | ||
| return wishlistRepository.save(wishlistItem); | ||
| } |
There was a problem hiding this comment.
The updatePriority and updateRequired methods perform database writes but are not marked with @transactional. While Spring Data JPA handles simple saves within a transaction context, it's a best practice to explicitly mark write operations with @transactional for consistency with other write methods in this service (addToWishlist and removeFromWishlist are both marked @transactional). This ensures transaction boundaries are clearly defined and consistent.
| public WishlistItem updateRequired(Long userId, Long subjectId, String semester, Boolean isRequired) { | ||
| WishlistItem wishlistItem; | ||
| if (semester != null && !semester.isBlank()) { | ||
| wishlistItem = wishlistRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, semester) | ||
| .orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.")); | ||
| } else { | ||
| wishlistItem = wishlistRepository.findByUserIdAndSubjectId(userId, subjectId).stream() | ||
| .findFirst() | ||
| .orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.")); | ||
| } | ||
|
|
||
| wishlistItem.setIsRequired(isRequired); | ||
| return wishlistRepository.save(wishlistItem); | ||
| } |
There was a problem hiding this comment.
The updatePriority and updateRequired methods perform database writes but are not marked with @transactional. While Spring Data JPA handles simple saves within a transaction context, it's a best practice to explicitly mark write operations with @transactional for consistency with other write methods in this service (addToWishlist and removeFromWishlist are both marked @transactional). This ensures transaction boundaries are clearly defined and consistent.
|
리뷰 코멘트 반영해서 추가 커밋 푸시했습니다. 반영 사항
추가 확인 부탁드립니다. |
|
추가 요청 반영하여 백엔드에도 회원탈퇴 기능을 이어서 반영했습니다. 추가 반영 사항
프론트 연동 PR은 별도로 생성했습니다:
|
배경
동일 과목을 매우 짧은 간격으로 연속 요청할 때(더블클릭/중복 탭 요청) 중복 row가 저장되고,
이후 삭제 시 단건 가정 로직 때문에 제거 실패/혼선이 발생할 수 있었습니다.
변경 내용
user_timetables:(user_id, subject_id, semester)wishlist_items:(user_id, subject_id, semester)@Transactional적용DataIntegrityViolationException을 사용자 메시지("이미 추가된 과목")로 변환userId + subjectId + semester기준으로 수행semester파라미터를 선택적으로 받아 해당 학기 우선 처리scripts/2026-02-13_add_unique_constraints.sql기대 효과
참고
ddl-auto: validate이므로, 실제 운영 반영 시 SQL 스크립트 실행이 필요합니다.