[FIX] 댓글 작성자에게 알림이 가는 오류 해결 #127
Conversation
📝 WalkthroughWalkthroughReplaced getIncomingShow with getRecentlyHotShow (returns top-3 hot shows via new repository query); switched casting-image deletion to a batch lookup/delete; added ImageRepository batch finder; made converter null-safe; set AmateurCasting.amateurShow FK non-nullable; changed notifyNewComment recipient to board writer. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller as AmateurController
participant Service as AmateurServiceImpl
participant Repo as AmateurShowRepository
Client->>Controller: GET /incoming (memberId)
Controller->>Service: getRecentlyHotShow(memberId)
Service->>Repo: findHotShows(today, PageRequest.of(0,3))
Repo-->>Service: List<AmateurShow> (ordered by totalSoldTicket desc, start asc)
Service->>Service: map to AmateurShowList DTOs (limit 3)
Service-->>Controller: List<AmateurShowList>
Controller-->>Client: 200 OK (DTO list)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.javasrc/main/java/cc/backend/notice/service/NoticeServiceImpl.java
🔇 Additional comments (1)
src/main/java/cc/backend/notice/service/NoticeServiceImpl.java (1)
79-115: Fix correctly routes comment notifications to board author.The change to
.member(boardWriter)is correct—the board author receives notifications when comments are posted. The self-notification guard (lines 89-91) properly prevents duplicate notifications when authors comment on their own posts. Event integration is properly wired: CommentService publishes the event, CommentEventListener receives it via @eventlistener, and passes it to notifyNewComment().
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/cc/backend/amateurShow/converter/AmateurConverter.java (1)
158-166: Potential NPE in toSingleCasting.Line 163 calls
dto.getCastingImageRequestDTO().getImageUrl()without checking ifgetCastingImageRequestDTO()returns null. This is inconsistent with the null-safe approach used intoAmateurCastingEntity(lines 58-76).🔎 Proposed fix
public static AmateurCasting toSingleCasting(AmateurUpdateRequestDTO.UpdateCasting dto, AmateurShow show) { + String imageUrl = null; + if (dto.getCastingImageRequestDTO() != null) { + imageUrl = dto.getCastingImageRequestDTO().getImageUrl(); + } + return AmateurCasting.builder() .amateurShow(show) .actorName(dto.getActorName()) .castingName(dto.getCastingName()) - .castingImageUrl(dto.getCastingImageRequestDTO().getImageUrl() != null ? - dto.getCastingImageRequestDTO().getImageUrl() : null) + .castingImageUrl(imageUrl) .build(); }
♻️ Duplicate comments (1)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (1)
632-655: Extract hardcoded page size to a named constant.The
PageRequest.of(0, 3)hardcodes the limit of 3 hot shows. Extract this to a named constant (e.g.,RECENTLY_HOT_SHOW_LIMIT = 3) for better maintainability and clarity.🔎 Proposed refactor
Add a constant at the class level:
private static final int RECENTLY_HOT_SHOW_LIMIT = 3;Then use it:
-List<AmateurShow> shows = amateurShowRepository.findHotShows(today, PageRequest.of(0, 3)); +List<AmateurShow> shows = amateurShowRepository.findHotShows(today, PageRequest.of(0, RECENTLY_HOT_SHOW_LIMIT));Based on learnings, a previous review flagged a similar hardcoded limit issue.
🧹 Nitpick comments (1)
src/main/java/cc/backend/amateurShow/entity/AmateurCasting.java (1)
31-31: Standardize nullable constraints onamateur_show_idforeign keys for data consistency.
AmateurCastingenforcesnullable = falseon theamateur_show_idforeign key, while related entities (AmateurStaff,AmateurTicket,AmateurNotice,AmateurRounds,PhotoAlbum) do not explicitly specify this constraint, defaulting to nullable. Either applynullable = falseconsistently across all related entities if all amateur_show_id relationships should be mandatory, or add a comment explaining why certain entities allow nullable values.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/cc/backend/amateurShow/controller/AmateurController.javasrc/main/java/cc/backend/amateurShow/converter/AmateurConverter.javasrc/main/java/cc/backend/amateurShow/entity/AmateurCasting.javasrc/main/java/cc/backend/amateurShow/repository/AmateurShowRepository.javasrc/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurService.javasrc/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.javasrc/main/java/cc/backend/image/repository/ImageRepository.java
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/cc/backend/image/repository/ImageRepository.java (1)
src/main/java/cc/backend/image/entity/Image.java (1)
Entity(16-62)
src/main/java/cc/backend/amateurShow/converter/AmateurConverter.java (2)
src/main/java/cc/backend/amateurShow/entity/AmateurNotice.java (2)
Entity(11-41)update(33-40)src/main/java/cc/backend/amateurShow/dto/AmateurUpdateRequestDTO.java (2)
Getter(12-85)Getter(57-65)
src/main/java/cc/backend/amateurShow/entity/AmateurCasting.java (4)
src/main/java/cc/backend/amateurShow/entity/AmateurStaff.java (1)
Entity(10-32)src/main/java/cc/backend/amateurShow/entity/AmateurNotice.java (1)
Entity(11-41)src/main/java/cc/backend/amateurShow/entity/AmateurRounds.java (1)
Entity(12-50)src/main/java/cc/backend/amateurShow/entity/AmateurTicket.java (1)
Entity(15-41)
🔇 Additional comments (7)
src/main/java/cc/backend/image/repository/ImageRepository.java (1)
28-31: LGTM!The new batch query method is well-designed and leverages the existing composite index
idx_file_contenton(file_path, content_id)for efficient retrieval.src/main/java/cc/backend/amateurShow/converter/AmateurConverter.java (3)
58-76: LGTM: Improved null-safety.The refactored null-safe extraction of image data is a good improvement, preventing potential NPEs when
castingImageRequestDTOis null.
79-94: LGTM: Added null checks.The null checks prevent issues when notice or content is null, improving robustness.
98-113: LGTM: Consistent null-safety.The null-safety pattern is consistent with the first overload.
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (3)
426-438: LGTM: Batch deletion improves performance.The refactored batch deletion using
findByFilePathAndContentIdIneliminates the N+1 query problem and is more efficient than the previous approach.
1-41: No unintended Swing imports found. Verification confirms that the file contains nojavax.swing.*orjava.awt.*imports. The AI summary's mention of an "added unintended Swing import" does not reflect the actual code state.
632-655: The original review comment's premise is incorrect.The actual commit message is "fix: enrollShow casting, notice npe처리" (enrollShow casting and notice NPE handling), not "댓글 작성자에게 알림이 가는 오류 해결" as claimed. Both AmateurServiceImpl.java and CommentEventListener.java are new files added in the same commit (4a89847), so the AmateurServiceImpl changes are legitimately part of the same PR scope rather than inconsistent with the PR's objectives.
Likely an incorrect or invalid review comment.
| @GetMapping("/incoming") | ||
| @Operation(summary = "공연 임박인 공연 조회 API") | ||
| public ApiResponse<List<AmateurShowResponseDTO.AmateurShowList>> getShowIncoming(@AuthenticationPrincipal(expression = "member") Member member) { | ||
| return ApiResponse.onSuccess(amateurService.getIncomingShow(member.getId())); | ||
| return ApiResponse.onSuccess(amateurService.getRecentlyHotShow(member.getId())); | ||
| } |
There was a problem hiding this comment.
Semantic mismatch between endpoint and implementation.
The endpoint /incoming with operation summary "공연 임박인 공연 조회 API" (retrieve upcoming shows) now calls getRecentlyHotShow(), which returns shows sorted by sales (hot shows), not by temporal proximity (incoming shows). This creates a confusing API contract.
Consider either:
- Renaming the endpoint to
/hotor/popularand updating the operation summary to reflect sales-based ranking - Or reverting the implementation to match the "incoming" semantic intent
🤖 Prompt for AI Agents
In src/main/java/cc/backend/amateurShow/controller/AmateurController.java around
lines 92-96, the /incoming endpoint and its Operation summary indicate "upcoming
shows" but the method calls amateurService.getRecentlyHotShow()
(sales/popularity-based), causing a semantic mismatch; fix by either (A)
renaming the endpoint and summary to reflect popularity (e.g.,
@GetMapping("/hot") and update Operation summary to "인기 공연 조회 API" or similar)
and keep calling getRecentlyHotShow(), or (B) change the service call to the
correct temporal-based method (e.g.,
amateurService.getIncomingShows(member.getId())) and ensure the Operation
summary remains "공연 임박인 공연 조회 API"; apply consistent naming to method, mapping,
and javadoc/annotations accordingly.
| @Query(""" | ||
| SELECT a | ||
| FROM AmateurShow a | ||
| WHERE a.end >= :today | ||
| ORDER BY a.totalSoldTicket DESC, a.start ASC | ||
| """) | ||
| List<AmateurShow> findHotShows( | ||
| @Param("today") LocalDate today, | ||
| Pageable pageable | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if totalSoldTicket field allows null and get its declaration
rg -nP 'private\s+(Integer|Long)\s+totalSoldTicket' --type=java -A2 -B2
# Check for @Index annotations on AmateurShow entity
rg -n '@Index|@Table\(' src/main/java/cc/backend/amateurShow/entity/AmateurShow.java -A5
# Also check the full AmateurShow entity file for context
echo "=== Full AmateurShow entity file ==="
find . -path "*/amateurShow/entity/AmateurShow.java" -type f | head -1 | xargs catRepository: SeeATheater/CC_Backend
Length of output: 6042
Add COALESCE to ORDER BY clause and create a database index for the query.
The totalSoldTicket field is nullable (Integer type with no NOT NULL constraint), but the query orders by it without explicit null handling. SQL databases sort nulls first or last (not as 0), which differs from the entity's business logic that treats null as zero in increaseSoldTicket() and decreaseSoldTicket(). Update the query to ORDER BY COALESCE(a.totalSoldTicket, 0) DESC, a.start ASC to match the expected behavior.
Additionally, create a database index on totalSoldTicket (or a composite index on totalSoldTicket, start) to optimize the ordering and filtering performance.
🤖 Prompt for AI Agents
In src/main/java/cc/backend/amateurShow/repository/AmateurShowRepository.java
around lines 69 to 78, the ORDER BY uses a.totalSoldTicket which is nullable and
should be treated as 0; change the JPQL to ORDER BY COALESCE(a.totalSoldTicket,
0) DESC, a.start ASC so nulls are treated as zero; additionally add a database
migration to create an index to optimize this query (either a single-column
index on total_sold_ticket or a composite index on (total_sold_ticket, start)
depending on your DB naming conventions) and ensure the migration is included in
your migrations folder and tested.
#이슈번호,#이슈번호Summary by CodeRabbit
New Features
Bug Fixes
Database
✏️ Tip: You can customize this high-level summary in your review settings.