[REFACTOR] MIDI 이벤트 저장 방식 수정#23
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMIDI 이벤트를 ChangesMIDI JSON 저장 및 완료 처리
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MidiEventData
participant Playing
participant JSONB
Client->>MidiEventData: sequence 포함 MIDI 이벤트 생성
MidiEventData-->>Client: 검증된 이벤트 반환
Client->>Playing: completeWithMidiData(events)
Playing->>Playing: 검증, 시간 필터링, 정렬
Playing->>JSONB: midiData 저장
Playing-->>Client: 완료 상태 및 MIDI 데이터 반환
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/mr/domain/playing/entity/Playing.java (1)
164-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win대용량 MIDI 이벤트 데이터에 대한 제한(Limit) 검증을 고려해 보세요.
단일 연주(
Playing) 전체를 한 번에 저장하는 구조라면 MIDI 이벤트가 매우 방대해질 수 있습니다. 이럴 경우 JSONB 컬럼을 메모리에서 직렬화/역직렬화하는 과정에서 애플리케이션의 메모리를 과도하게 점유하거나 OOM(Out of Memory)이 발생할 수 있습니다.시스템 안정성을 위해 한 번에 저장할 수 있는 최대 이벤트 개수(예: 10,000개 등)를 제한하는 검증 로직을 추가해 보는 것은 어떨까요? 데이터가 폭주해도 끄떡없는 튼튼한 서버를 위해 고려해 볼 만한 훌륭한 방어 로직입니다! 💪
🛡️ 리미트 검증 추가 예시
private static void validateMidiData(List<MidiEventData> midiData) { if (midiData == null || midiData.isEmpty()) { throw new GeneralException(PlayingErrorStatus.EMPTY_MIDI_EVENTS); } + + // TODO: MVP 이후 최대 허용 개수 초과 시 예외 처리 로직 추가 고려 + // if (midiData.size() > MAX_MIDI_EVENTS) { + // throw new GeneralException(PlayingErrorStatus.TOO_MANY_MIDI_EVENTS); + // } if (midiData.stream().anyMatch(Objects::isNull)) {🤖 Prompt for 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. In `@src/main/java/com/mr/domain/playing/entity/Playing.java` around lines 164 - 172, Update validateMidiData in Playing to reject MIDI event lists exceeding the system’s defined maximum event count, using the existing validation exception/status pattern. Preserve the current empty-list and null-event checks, and reference an established limit constant or add a single named constant for the maximum rather than embedding an unexplained literal.
🤖 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/entity/MidiEventData.java`:
- Around line 36-43: Update MidiEventData.create to be the Jackson
deserialization creator by adding `@JsonCreator` and explicit `@JsonProperty`
annotations for type, pitch, velocity, and timestampMs, ensuring JSON mapping
uses the existing validation path instead of the protected no-arg constructor.
---
Nitpick comments:
In `@src/main/java/com/mr/domain/playing/entity/Playing.java`:
- Around line 164-172: Update validateMidiData in Playing to reject MIDI event
lists exceeding the system’s defined maximum event count, using the existing
validation exception/status pattern. Preserve the current empty-list and
null-event checks, and reference an established limit constant or add a single
named constant for the maximum rather than embedding an unexplained literal.
🪄 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: 8b1de69a-37de-4172-ac52-dcd7f33cfd7f
📒 Files selected for processing (3)
src/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/PlayingErrorStatus.java
ownue
left a comment
ownue
left a comment
There was a problem hiding this comment.
코멘트 확인 부탁드립니다! 제가 잘못 이해했거나 잘못 말한 부분이 있을 수도 있어요....
| validatePitch(pitch); | ||
| validateVelocity(velocity); |
There was a problem hiding this comment.
MidiType을 지금 NOTE_ON, NOTE_OFF만 갖고 있는데 저희 MIDI 데이터를 요 정도만 받아도 되는 건지 궁금해요! (이건 PM님과 논의할 사안인 것 같지만....) 현재는 모든 타입에 pitch와 velocity를 필수로 검증하고 있어, 추후 페달이나 Control Change 이벤트까지 저장한다면 타입별 검증 또는 이벤트 모델 분리가 필요할 것 같아요!!
| public void saveMidiData(List<MidiEventData> midiData) { | ||
| validateMidiData(midiData); | ||
|
|
||
| this.midiData = new ArrayList<>(midiData); |
There was a problem hiding this comment.
midi 데이터를 저장할 때, 컬렉션 래퍼 참조가 끊기는 것을 방지하기 위해 this.midiData.clear(); 후 addAll() 방식으로 기존 컬렉션 객체를 유지하면서 갱신하는 방식은 어떤가요? 저도 비슷한 걸 찾아보다가 의견 어쭙습니다!
There was a problem hiding this comment.
현재 midiData는 JSON 컬럼에 매핑된 컬렉션이라 Hibernate 영속 컬렉션 래퍼를 유지해야 하는 상황은 아니라고 판단해서 기존 코드대로 ArrayList를 생성해 교체하는 방식을 유지하려고 합니다!
혹시 다른 의견 있으시면 말씀해 주세요
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/mr/domain/playing/entity/Playing.java (2)
46-54: 🗄️ Data Integrity & Integration | 🔵 Trivial신규 NOT NULL 컬럼(
version,midi_data) 백필 전략 확인 필요.기존
playing테이블에 데이터가 있는 상태에서nullable = false인version,midi_data컬럼을 추가하는 마이그레이션이라면, 기존 로우에 기본값(version=0,midi_data='[]')을 백필하는 스크립트가 함께 있는지 확인이 필요합니다. 마이그레이션 파일이 이번 리뷰 범위에 없어서 코드만으로는 확인이 어려운데요, 누락 시 배포 시점에 제약 조건 위반으로 실패할 수 있습니다.Also applies to: 91-93
🤖 Prompt for 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. In `@src/main/java/com/mr/domain/playing/entity/Playing.java` around lines 46 - 54, Update the migration that adds the Playing entity’s non-null version and midi_data columns to backfill existing playing rows with version=0 and midi_data='[]' before enforcing NOT NULL constraints. Ensure the migration is safe for already-populated tables and applies to both columns.
174-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
validateMidiData이중 호출 — 후반부 호출은 사실상 "필터 후 빈 리스트" 체크만 유효합니다.
sortedMidiData는 이미 null-free임이 보장된 원본에서 필터링만 한 결과라서, 재호출된validateMidiData(sortedMidiData)의 null-check/size-limit 검증은 항상 통과하는 중복 연산입니다(최대 10만 건 순회가 한 번 더 발생). 의도(필터링으로 전부 제거된 경우 방어)는 좋지만, 전용 체크로 좁히면 가독성과 성능 모두 이득이에요.♻️ 제안
- List<MidiEventData> sortedMidiData = midiData.stream() - .filter(event -> event.getTimestampMs() <= allowedTimestampMs) - .sorted(Comparator.comparingLong(MidiEventData::getTimestampMs)) - .toList(); - - validateMidiData(sortedMidiData); + List<MidiEventData> sortedMidiData = midiData.stream() + .filter(event -> event.getTimestampMs() <= allowedTimestampMs) + .sorted(Comparator.comparingLong(MidiEventData::getTimestampMs)) + .toList(); + + if (sortedMidiData.isEmpty()) { + throw new GeneralException(PlayingErrorStatus.EMPTY_MIDI_EVENTS); + }Also applies to: 213-225
🤖 Prompt for 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. In `@src/main/java/com/mr/domain/playing/entity/Playing.java` around lines 174 - 201, In completeWithMidiData, replace the second validateMidiData(sortedMidiData) call with a focused check for the filtered result being empty, preserving the intended guard when all events exceed allowedTimestampMs. Keep the initial validateMidiData(midiData) validation and the subsequent sorting and completion logic unchanged.
🤖 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/entity/Playing.java`:
- Around line 184-191: Update the MIDI event filtering flow in Playing so events
with timestamps above allowedTimestampMs are counted and the number of dropped
events is logged at WARN or INFO level. Preserve the existing timestamp boundary
and sorting behavior for retained events.
---
Nitpick comments:
In `@src/main/java/com/mr/domain/playing/entity/Playing.java`:
- Around line 46-54: Update the migration that adds the Playing entity’s
non-null version and midi_data columns to backfill existing playing rows with
version=0 and midi_data='[]' before enforcing NOT NULL constraints. Ensure the
migration is safe for already-populated tables and applies to both columns.
- Around line 174-201: In completeWithMidiData, replace the second
validateMidiData(sortedMidiData) call with a focused check for the filtered
result being empty, preserving the intended guard when all events exceed
allowedTimestampMs. Keep the initial validateMidiData(midiData) validation and
the subsequent sorting and completion logic unchanged.
🪄 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: 872403ff-2719-4a40-8b54-ff7543cad22e
📒 Files selected for processing (2)
src/main/java/com/mr/domain/playing/entity/Playing.javasrc/main/java/com/mr/domain/playing/exception/PlayingErrorStatus.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/mr/domain/playing/exception/PlayingErrorStatus.java
| // 실제 연주 허용 시간 = min(실제 연주 시간, 10분) + 오차 범위(500ms) | ||
| long allowedTimestampMs = Math.min(savedDurationMs + MIDI_TIMESTAMP_TOLERANCE_MS, | ||
| MAX_DURATION_MS); | ||
|
|
||
| List<MidiEventData> sortedMidiData = midiData.stream() | ||
| .filter(event -> event.getTimestampMs() <= allowedTimestampMs) | ||
| .sorted(Comparator.comparingLong(MidiEventData::getTimestampMs)) | ||
| .toList(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
허용 시간 범위를 벗어난 MIDI 이벤트가 조용히 삭제됩니다.
allowedTimestampMs를 초과하는 이벤트를 아무 로그 없이 필터링해서 버리고 있어요. 악의적/오차 있는 타임스탬프를 방어하는 목적 자체는 타당하지만, 클라이언트 타이머 버그로 다수 이벤트가 매번 조용히 잘려나가는 상황을 운영 중에 알아채기 어렵습니다. 최소한 드롭된 이벤트 수를 로그(WARN/INFO)로 남기면 추후 분석 결과 품질 이슈를 추적하기 훨씬 쉬워질 거예요.
🤖 Prompt for 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.
In `@src/main/java/com/mr/domain/playing/entity/Playing.java` around lines 184 -
191, Update the MIDI event filtering flow in Playing so events with timestamps
above allowedTimestampMs are counted and the number of dropped events is logged
at WARN or INFO level. Preserve the existing timestamp boundary and sorting
behavior for retained events.
There was a problem hiding this comment.
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/com/mr/domain/playing/entity/Playing.java (1)
98-100: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift대용량
midiData조회 경로를 분리해 주세요.
Playing엔티티에 JSONBmidiData필드가 기본 속성으로 매핑되어 있어, 목록/상세 조회마다 필요하지 않은 대용량 MIDI 이벤트를 함께 읽고 역직렬화할 수 있습니다. MIDI 분석 전용 조회·프로젝션 또는 별도 저장 구조로 분리하고; Hibernate bytecode enhancement를 통해 기본 필드도 lazy fetch로 처리하려면 Spring Boot bytecode enhancement 설정이 실제로 적용되는지 추가 검증이 필요합니다.🤖 Prompt for 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. In `@src/main/java/com/mr/domain/playing/entity/Playing.java` around lines 98 - 100, Playing 엔티티의 기본 midiData 매핑으로 목록·상세 조회 시 대용량 JSONB가 함께 로드되지 않도록 조회 경로를 분리하세요. MIDI 분석에 필요한 전용 조회·프로젝션 또는 별도 저장 구조를 도입하고, midiData를 lazy fetch에 의존한다면 Hibernate bytecode enhancement 설정이 실제 적용되는지도 검증하세요.
🤖 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.
Outside diff comments:
In `@src/main/java/com/mr/domain/playing/entity/Playing.java`:
- Around line 98-100: Playing 엔티티의 기본 midiData 매핑으로 목록·상세 조회 시 대용량 JSONB가 함께
로드되지 않도록 조회 경로를 분리하세요. MIDI 분석에 필요한 전용 조회·프로젝션 또는 별도 저장 구조를 도입하고, midiData를 lazy
fetch에 의존한다면 Hibernate bytecode enhancement 설정이 실제 적용되는지도 검증하세요.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0449f6a9-80ea-463c-9a3f-f77112cea6f8
📒 Files selected for processing (1)
src/main/java/com/mr/domain/playing/entity/Playing.java
4 participants
📍 개요
⛓️💥 관련 이슈
🛠️ 작업 내용
🔥 리뷰 요청 사항
✅ 체크리스트
📎 참고 사항
Summary by CodeRabbit