[FEAT] MIDI 이벤트 저장 API 구현 - #72
Conversation
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough인증된 사용자가 연주 세션에 MIDI 이벤트를 일괄 저장하는 POST API가 추가되었습니다. 요청 검증, MIDI 정규화·저장, 소유자·상태·빈도 검증, 오류 코드 및 관련 테스트가 함께 변경되었습니다. ChangesMIDI 이벤트 저장 기능
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PlayingController
participant PlayingService
participant PlayingRepository
participant Playing
Client->>PlayingController: POST /api/playings/{playingId}/midi-events
PlayingController->>PlayingService: saveMidiEvents(userId, playingId, request)
PlayingService->>PlayingRepository: 조회 및 최근 완료 여부 확인
PlayingRepository-->>PlayingService: Playing 또는 저장 이력 여부
PlayingService->>Playing: 소유자 검증 및 MIDI 완료 처리
Playing-->>PlayingService: saved MIDI data
PlayingService-->>PlayingController: MidiEventSaveResponse
PlayingController-->>Client: ApiResponse
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/mr/domain/playing/dto/req/MidiEventSaveRequest.java`:
- Around line 13-14: Update the events field in MidiEventSaveRequest to enforce
the 100,000-item maximum during request validation, preferably by reusing the
shared Playing limit constant instead of duplicating it. Ensure validation
occurs before PlayingService converts entries to MidiEventData; configure a
request-body size limit as well if the existing application configuration
provides the appropriate setting.
In `@src/main/java/com/mr/domain/playing/exception/PlayingErrorStatus.java`:
- Around line 17-19: Update PlayingErrorStatus so the existing
MISSING_PLAYING_MODE, MISSING_PLAYING_STATUS, and UNSUPPORTED_PLAYING_MODE
entries retain their prior PLAYING_400_11 through PLAYING_400_13 codes. Add
MIDI-specific errors in a separate code range without renumbering these
established non-MIDI codes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: afc90716-482a-4af6-bc9e-1297126b5216
📒 Files selected for processing (10)
src/main/java/com/mr/domain/playing/controller/PlayingController.javasrc/main/java/com/mr/domain/playing/dto/req/MidiEventSaveRequest.javasrc/main/java/com/mr/domain/playing/dto/res/MidiEventSaveResponse.javasrc/main/java/com/mr/domain/playing/entity/MidiEventData.javasrc/main/java/com/mr/domain/playing/entity/Playing.javasrc/main/java/com/mr/domain/playing/exception/MidiEventErrorStatus.javasrc/main/java/com/mr/domain/playing/exception/PlayingErrorStatus.javasrc/main/java/com/mr/domain/playing/service/PlayingService.javasrc/test/java/com/mr/domain/playing/controller/PlayingControllerTest.javasrc/test/java/com/mr/domain/playing/service/PlayingServiceTest.java
| Integer velocity, | ||
|
|
||
| @NotNull(message = "MIDI 이벤트 발생 시간은 필수입니다.") | ||
| @Min(value = 0, message = "MIDI 이벤트 발생 시간은 0 이상이어야 합니다.") |
There was a problem hiding this comment.
PR 참고 사항에는 10분을 초과하는 MIDI 이벤트는 저장 대상에서 제외한다고 되어 있는데, 현재 DTO와 서비스 로직에서는 timestampMs >= 0만 검증하고 전체 이벤트를 그대로 저장하는 것으로 보입니다! timestampMs > 600_000인 이벤트를 필터링하려는 의도인지, 요청 자체를 예외 처리하려는 의도인지 명확히 해야 할 것 같습니다...🥹🥹
There was a problem hiding this comment.
최대 timestampMs는 요청 시점에 알 수 있는 고정값이 아니라 실제 연주 종료 시각을 기준으로 동적으로 결정되므로, DTO에서는 상한 검증을 두지 않았어요! (음수 값만 검증)
→ Playing.completeWithMidiData()에서 실제 연주 시간 + 500ms의 허용 오차를 적용하고, 최대 10분(600_000ms)까지만 저장하도록 필터링하는 비즈니스 로직으로 처리
다만 연주 시간이 정확히 10분에 도달한 경우에 최대 10분 제한으로 인해 추가 500ms의 허용 오차가 적용되지 않는데, 해당 동작이 적절한지에 대해서는 팀원분들 의견도 궁금합니다ㅎㅎ
There was a problem hiding this comment.
도메인에 대한 이해가 많이 부족하지만.... 저는 지금은 괜찮다구 생각해요!!
추후 QA를 거쳐 디벨롭하면 될 것 같습니동
There was a problem hiding this comment.
지금 요 테스트는 completeWithMidiData()가 호출됐는지만 확인하고 있어, 실제 MIDI 저장/정렬과 IN_PROGRESS → COMPLETED 상태 전이는 테스트되지 않는 것 같은데 의도하신 건지 궁금합니다!
의도가 아니라면, 이번 기능의 핵심이 Playing.completeWithMidiData()에 있으므로 실제 Playing 객체를 생성한 도메인 테스트에서 상태 변경, 중복 sequence, 빈 목록, 이벤트 개수 제한 등을 검증하는 테스트도 추가로 진행하면 좋을 것 같습니다!
| public record MidiEventSaveRequest ( | ||
| @NotEmpty(message = "MIDI 이벤트 목록은 필수입니다") | ||
| @Size(max = 100_000, message = "MIDI 이벤트 개수가 허용 범위를 초과했습니다.") | ||
| List<@Valid MidiEventRequest> events |
There was a problem hiding this comment.
관련 문서 확인해 보니, Bean Validation 2.0부터 List<@Valid MidiEventRequest>처럼 컨테이너의 타입 인자에 @Valid를 적용하는 방식 권장 + @Valid List 방식도 기존 방식으로 계속 지원되고 있다고 나와서, 우선은 현재 작성한 List<@Valid MidiEventRequest> 형태를 유지해도 괜찮을 것 같다는 개인적인 의견입니다..!
다만 말씀해 주신 것처럼 실제 프로젝트 환경에서 리스트 내부 DTO의 검증이 정상적으로 동작하는지는 테스트 코드 추가해서 확인해 볼게요!
참고 문서: Jakarta Bean Validation
p1001q
left a comment
p1001q
left a comment
There was a problem hiding this comment.
제가 남긴 리뷰들은 리팩토링 때 고려하셔도 될 거 같습니다! 수고하셨어요!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/mr/domain/playing/exception/PlayingErrorStatus.java`:
- Around line 21-25: Update the PLAYING_ACCESS_DENIED entry in
PlayingErrorStatus so its code string uses the 403 HTTP-status prefix while
preserving its FORBIDDEN status and access-denied message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cba43fdd-4c19-43d1-be4f-9ea3a2500a45
📒 Files selected for processing (11)
src/main/java/com/mr/domain/playing/constant/MidiEventConstants.javasrc/main/java/com/mr/domain/playing/dto/req/MidiEventSaveRequest.javasrc/main/java/com/mr/domain/playing/entity/Playing.javasrc/main/java/com/mr/domain/playing/exception/MidiEventErrorStatus.javasrc/main/java/com/mr/domain/playing/exception/PlayingErrorStatus.javasrc/main/java/com/mr/domain/playing/repository/PlayingRepository.javasrc/main/java/com/mr/domain/playing/service/PlayingService.javasrc/test/java/com/mr/domain/playing/controller/PlayingControllerTest.javasrc/test/java/com/mr/domain/playing/dto/req/MidiEventSaveRequestTest.javasrc/test/java/com/mr/domain/playing/entity/PlayingTest.javasrc/test/java/com/mr/domain/playing/service/PlayingServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/mr/domain/playing/service/PlayingServiceTest.java
4 participants
📍 개요
⛓️💥 관련 이슈
🛠️ 작업 내용
PlayingErrorStatus에서 공동으로 관리하던 코드를MidiEventErrorStatus로 분리🔥 리뷰 요청 사항
✅ 체크리스트
📎 참고 사항
Summary by CodeRabbit
/api/playings/{playingId}/midi-events)를 추가했습니다.