[FEAT] 백킹트랙 기반 연주 세션 시작 API 구현 - #85
Conversation
|
Warning Review limit reached
Next review available in: 53 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 (2)
📝 WalkthroughWalkthrough백킹트랙 기반 연주 세션 시작 및 MIDI 이벤트 저장 API를 추가했습니다. 요청·응답 DTO, 검증 오류, MIDI 정규화·완료 처리, 서비스·저장소·컨트롤러 연결과 테스트를 구현했습니다. Changes연주 세션 및 MIDI 처리
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PlayingController
participant PlayingService
participant PlayingRepository
participant Playing
Client->>PlayingController: 세션 시작 또는 MIDI 저장 요청
PlayingController->>PlayingService: 인증 사용자 ID와 요청 전달
PlayingService->>PlayingRepository: 사용자·백킹트랙·Playing 조회
PlayingService->>Playing: 세션 시작 또는 MIDI 완료 처리
PlayingService->>PlayingRepository: Playing 저장
PlayingService-->>PlayingController: 응답 DTO 반환
PlayingController-->>Client: ApiResponse 반환
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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: 5
🧹 Nitpick comments (6)
src/test/java/com/mr/domain/playing/entity/PlayingTest.java (1)
29-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
startPlaying과startPlaying_success(Line 568-595)가 사실상 같은 시나리오입니다.후자가 시작 시각의 구간(
isBetween)까지 단정하는 상위 집합이므로, 앞의 테스트는 삭제해도 커버리지가 줄지 않습니다. 이름이 비슷한 테스트가 둘 있으면 나중에 어느 쪽을 고쳐야 할지 헷갈립니다.🤖 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/test/java/com/mr/domain/playing/entity/PlayingTest.java` around lines 29 - 44, Remove the redundant startPlaying test method near the top of PlayingTest, keeping the more comprehensive startPlaying_success test that also validates the startedAt time range. Do not alter the remaining test coverage or behavior.src/test/java/com/mr/domain/playing/service/PlayingServiceTest.java (1)
607-648: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mockUser()와mockBackingTrack()은 아무도 호출하지 않습니다.
startPlaying_success는setUp()에서 만든 목에 직접 스터빙(Line 440-456)하고 있어 이 두 헬퍼는 순수 죽은 코드입니다. 게다가 안에given(...)이 여러 개 있어 나중에 그대로 호출하면MockitoExtension의 STRICT_STUBS 정책 때문에UnnecessaryStubbingException으로 이어질 수 있습니다. 삭제하거나, 헬퍼로 통일하되 필요한 스터빙만 남기는 쪽을 권합니다.참고: Mockito 공식 문서의 Strictness /
UnnecessaryStubbingException절이 이 동작을 잘 설명합니다.🤖 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/test/java/com/mr/domain/playing/service/PlayingServiceTest.java` around lines 607 - 648, Remove the unused mockUser() and mockBackingTrack() helper methods from PlayingServiceTest, or replace the setUp() stubbing used by startPlaying_success with these helpers while retaining only the stubbings actually required by the test. Ensure no unused Mockito stubs remain under MockitoExtension strict stubbing.src/main/java/com/mr/domain/playing/entity/Playing.java (2)
238-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
hasInvalidEventOrder검사는 사실상 도달할 수 없습니다.
MidiEventData의 private 생성자가validateSequence/validateTimestampMs로 null·음수를 이미 거르므로(MidiEventData.javaLine 55-83), 여기까지 온MidiEventData인스턴스의sequence/timestampMs는 항상 non-null·비음수입니다. 즉 Line 238-239 분기는 커버할 수 없는 코드이며, 실제로PlayingTest에도 이 분기를 겨냥한 테스트가 없습니다.값 객체의 불변식을 신뢰해 제거하는 편이 의도를 더 잘 드러냅니다. 반대로 "역직렬화 등 우회 경로가 있으니 방어적으로 남긴다"가 의도라면, 주석 한 줄로 근거를 남겨 두면 다음 사람이 지우지 않습니다.
♻️ 제안 diff (제거안)
if (midiData.stream().anyMatch(Objects::isNull)) { throw new GeneralException(MidiEventErrorStatus.INVALID_MIDI_EVENT); } - - if (midiData.stream().anyMatch(Playing::hasInvalidEventOrder)) { - throw new GeneralException(MidiEventErrorStatus.INVALID_MIDI_EVENT); - }- private static boolean hasInvalidEventOrder( - MidiEventData event - ) { - return event.getTimestampMs() == null - || event.getTimestampMs() < 0 - || event.getSequence() == null - || event.getSequence() < 0; - }Also applies to: 338-345
🤖 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 238 - 239, Remove the unreachable hasInvalidEventOrder validation branches in the relevant Playing methods, including the logic around the shown stream check and the additionally referenced section. Rely on MidiEventData’s validated value-object invariants for sequence and timestampMs; if defensive checks must remain for bypassed construction paths, retain them only with a concise comment documenting that rationale.
230-232: 🚀 Performance & Scalability | 🔵 Trivial단일 jsonb 컬럼에 최대 10만 이벤트 — 운영 관점 사전 점검을 권합니다.
이벤트 하나가 JSON으로 대략 50
70바이트라면 상한치에서7MB에 달합니다. 아래 항목을 미리 확인해 두면 프로덕션에서 놀라는 일이 줄어듭니다.midi_data한 행이 5
- 요청 본문 크기 제한(
spring.servlet.multipart/server.max-http-request-header-size가 아닌 WAS 바디 한계, 리버스 프록시client_max_body_size)- PostgreSQL TOAST 압축·해제 비용과
Playing조회 시 이 컬럼이 함께 로딩되는 경로(목록 조회에서midi_data를 제외할 수 있는지)@Version낙관적 락과 결합된 대형 행 UPDATE의 WAL 증가량장기적으로는 MIDI 이벤트를 별도 테이블 또는 오브젝트 스토리지로 분리하는 편이 조회 성능과 백업 비용 모두에 유리합니다.
🤖 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 230 - 232, Review the 100,000-event limit enforced in Playing by MAX_MIDI_EVENT_COUNT and validate the end-to-end operational limits for large midiData payloads, including request-body and proxy size limits, PostgreSQL TOAST behavior, eager loading in list queries, and `@Version` update/WAL impact. Preserve the current validation while documenting or adjusting configuration and query/storage design as needed to prevent oversized JSONB rows in production.src/main/java/com/mr/domain/playing/dto/req/MidiEventSaveRequest.java (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMIDI 제약 값이 DTO·엔티티·검증 메시지 세 곳에 흩어져 있습니다.
MAX_MIDI_EVENT_COUNT는MidiEventConstants로 잘 뽑아냈지만, 개수 상한이 메시지 문자열에 다시 하드코딩되고 피치/강도 범위(0, 127)는 두 계층에 각각 리터럴로 남아 있어 한쪽만 바뀌면 계층 간 계약이 조용히 어긋납니다. 상수 클래스 한 곳으로 모으는 것이 근본 해법입니다.
src/main/java/com/mr/domain/playing/dto/req/MidiEventSaveRequest.java#L16-L18:@Size메시지의100,000을{max}보간으로 바꾸고,@Min/@Max의0·127을MidiEventConstants의static final int상수로 교체하세요(MidiEventSaveRequestTest가 메시지를 문자 단위로 단정하므로 함께 갱신 필요).src/main/java/com/mr/domain/playing/entity/MidiEventData.java#L57-L82:validatePitch/validateVelocity의0·127리터럴을 같은 상수로 교체하세요.🤖 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/dto/req/MidiEventSaveRequest.java` around lines 16 - 18, Centralize all MIDI constraint values in MidiEventConstants: in src/main/java/com/mr/domain/playing/dto/req/MidiEventSaveRequest.java lines 16-18, use {max} in the `@Size` message and replace `@Min/`@Max literals 0 and 127 with the corresponding constants, then update MidiEventSaveRequestTest’s exact message assertions; in src/main/java/com/mr/domain/playing/entity/MidiEventData.java lines 57-82, replace the 0 and 127 literals in validatePitch and validateVelocity with those same constants.src/test/java/com/mr/domain/playing/controller/PlayingControllerTest.java (1)
270-310: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value10만 건 페이로드를 매번 직렬화하는 컨트롤러 테스트는 CI 시간을 잡아먹습니다.
MAX_MIDI_EVENT_COUNT + 1개의 DTO 생성 + Jackson 직렬화 + MockMvc 왕복이라 수백 MB급 문자열이 오갈 수 있습니다.@Size어노테이션이 걸려 있다는 사실은 이미MidiEventSaveRequestTest처럼Validator를 직접 호출하는 단위 테스트에서 훨씬 저렴하게 검증할 수 있고, HTTP 계층 테스트에서는 대표 케이스 하나만 남기는 편이 실용적입니다.경계값을 HTTP 계층에서 꼭 확인하려면 JSON 문자열을 스트림으로 만들어 넘기는 방식이 그나마 메모리에 유리합니다. 판단은 팀 CI 예산에 맡기겠습니다.
🤖 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/test/java/com/mr/domain/playing/controller/PlayingControllerTest.java` around lines 270 - 310, Replace the large-payload HTTP test in saveMidiEvents_eventCountExceeded with a lightweight representative controller validation case, removing the MAX_MIDI_EVENT_COUNT + 1 DTO construction and Jackson serialization; keep the exact `@Size` boundary assertion in MidiEventSaveRequestTest via direct Validator testing, or use streamed JSON only if the HTTP boundary must remain covered.
🤖 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 197-206: Update Playing.completeWithMidiData() and
normalizeMidiData() to track discarded MIDI events and detect when
timestamp-based filtering exceeds the configured MIDI_TIMESTAMP_TOLERANCE_MS
threshold relative to the request. Reject or warn and require retransmission
before setting PlayingStatus.COMPLETED; retain EMPTY_MIDI_EVENTS for the
all-discarded case, and ensure the discard count is explicitly exposed if
MidiEventSaveResponse is used to report save results.
In `@src/main/java/com/mr/domain/playing/service/PlayingService.java`:
- Around line 73-85: 원자성이 보장되지 않는 validateMidiSaveRequestInterval(userId)의 조회 후
completeWithMidiData(midiEvents) 완료 흐름을 수정하세요. 동일 userId의 동시 요청이 하나만 1분 제한을
통과하도록 최근 완료 이력 조회에 PESSIMISTIC_WRITE 잠금을 적용하거나 DB 기본값/애너테이션 기반 원자적 rate-limit
삽입을 사용하고, 기존 MIDI 이벤트 변환 및 완료 동작은 유지하세요.
In `@src/test/java/com/mr/domain/playing/controller/PlayingControllerTest.java`:
- Around line 72-76: Update the CustomUserDetails construction in
PlayingControllerTest to use the existing USER_ID constant instead of the
hardcoded 1L, keeping the authenticated principal ID consistent with the
verification at line 141.
In `@src/test/java/com/mr/domain/playing/dto/req/MidiEventSaveRequestTest.java`:
- Around line 60-69: Update the request DTO’s event list declaration, likely the
field or accessor in MidiEventSaveRequest, to apply a non-null container-element
constraint alongside `@Valid` (for example, annotate the generic MidiEventRequest
type argument). Preserve cascade validation for non-null elements and ensure a
list containing null produces a constraint violation as asserted by
MidiEventSaveRequestTest.
In `@src/test/java/com/mr/domain/playing/service/PlayingServiceTest.java`:
- Around line 591-605: Update startPlaying_nullBackingTrackId to construct and
pass a PlayingStartRequest with a null backing-track ID, so it exercises the
DTO’s `@NotNull` validation path; keep the existing exception and
repository-never-save assertions, and cover a null request separately if that
path is required.
---
Nitpick comments:
In `@src/main/java/com/mr/domain/playing/dto/req/MidiEventSaveRequest.java`:
- Around line 16-18: Centralize all MIDI constraint values in
MidiEventConstants: in
src/main/java/com/mr/domain/playing/dto/req/MidiEventSaveRequest.java lines
16-18, use {max} in the `@Size` message and replace `@Min/`@Max literals 0 and 127
with the corresponding constants, then update MidiEventSaveRequestTest’s exact
message assertions; in
src/main/java/com/mr/domain/playing/entity/MidiEventData.java lines 57-82,
replace the 0 and 127 literals in validatePitch and validateVelocity with those
same constants.
In `@src/main/java/com/mr/domain/playing/entity/Playing.java`:
- Around line 238-239: Remove the unreachable hasInvalidEventOrder validation
branches in the relevant Playing methods, including the logic around the shown
stream check and the additionally referenced section. Rely on MidiEventData’s
validated value-object invariants for sequence and timestampMs; if defensive
checks must remain for bypassed construction paths, retain them only with a
concise comment documenting that rationale.
- Around line 230-232: Review the 100,000-event limit enforced in Playing by
MAX_MIDI_EVENT_COUNT and validate the end-to-end operational limits for large
midiData payloads, including request-body and proxy size limits, PostgreSQL
TOAST behavior, eager loading in list queries, and `@Version` update/WAL impact.
Preserve the current validation while documenting or adjusting configuration and
query/storage design as needed to prevent oversized JSONB rows in production.
In `@src/test/java/com/mr/domain/playing/controller/PlayingControllerTest.java`:
- Around line 270-310: Replace the large-payload HTTP test in
saveMidiEvents_eventCountExceeded with a lightweight representative controller
validation case, removing the MAX_MIDI_EVENT_COUNT + 1 DTO construction and
Jackson serialization; keep the exact `@Size` boundary assertion in
MidiEventSaveRequestTest via direct Validator testing, or use streamed JSON only
if the HTTP boundary must remain covered.
In `@src/test/java/com/mr/domain/playing/entity/PlayingTest.java`:
- Around line 29-44: Remove the redundant startPlaying test method near the top
of PlayingTest, keeping the more comprehensive startPlaying_success test that
also validates the startedAt time range. Do not alter the remaining test
coverage or behavior.
In `@src/test/java/com/mr/domain/playing/service/PlayingServiceTest.java`:
- Around line 607-648: Remove the unused mockUser() and mockBackingTrack()
helper methods from PlayingServiceTest, or replace the setUp() stubbing used by
startPlaying_success with these helpers while retaining only the stubbings
actually required by the test. Ensure no unused Mockito stubs remain under
MockitoExtension strict stubbing.
🪄 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: 3e79f33b-6760-4f83-ba29-68a306833b71
📒 Files selected for processing (16)
src/main/java/com/mr/domain/playing/constant/MidiEventConstants.javasrc/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/req/PlayingStartRequest.javasrc/main/java/com/mr/domain/playing/dto/res/MidiEventSaveResponse.javasrc/main/java/com/mr/domain/playing/dto/res/PlayingStartResponse.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/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
| validateMidiSaveRequestInterval(userId); | ||
|
|
||
| List<MidiEventData> midiEvents = request.events() | ||
| .stream() | ||
| .map(event -> MidiEventData.of( | ||
| event.sequence(), | ||
| event.type(), | ||
| event.pitch(), | ||
| event.velocity(), | ||
| event.timestampMs() | ||
| )).toList(); | ||
|
|
||
| playing.completeWithMidiData(midiEvents); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 \
'validateMidiSaveRequestInterval|completeWithMidiData|existsByUser_UserIdAndStatusAndEndedAtAfterAndDeletedAtIsNull' \
src/main/java/com/mr/domain/playing/service/PlayingService.java
rg -n -C 4 '`@Version`|completeWithMidiData|endedAt|status' \
src/main/java/com/mr/domain/playing/entity/Playing.java
rg -n -C 4 '`@Lock`|PESSIMISTIC_WRITE|LockModeType' \
src/main/java/com/mr/domainRepository: Musereview/BE
Length of output: 7798
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== PlayingService outline =="
ast-grep outline src/main/java/com/mr/domain/playing/service/PlayingService.java || true
echo "== PlayingService relevant sections =="
sed -n '1,180p' src/main/java/com/mr/domain/playing/service/PlayingService.java
echo "== PlayingRepository methods =="
rg -n -C 6 'interface .*Repository|existsByUser_UserIdAndStatusAndEndedAtAfterAndDeletedAtIsNull|save|Lock|PESSIMISTIC|PLAYING_NOT_FOUND|deleteByIdAndDeletedAtIsNull|findByIdAndDeletedAtIsNull' src/main/java/com/mr/domain/playing
echo "== Playing entity relevant sections =="
sed -n '40,230p' src/main/java/com/mr/domain/playing/entity/Playing.java
echo "== Controller save method if present =="
rg -n -C 8 'save|MidiEventSave|playing_id|userId' src/main/java/com/mr/domain/playing | sed -n '1,220p'Repository: Musereview/BE
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Java/gradle/disk availability =="
command -v java || true
command -v javac || true
command -v gradle || true
command -v mvn || true
javac -version 2>&1 || true
echo "== Minimal Java version-concurrency probe =="
java_home="$(dirname "$(dirname "$(command -v java 2>/dev/null || true)")")"
if [ -n "$java_home" ] && command -v javac >/dev/null 2>&1; then
tmpdir="$(mktemp -d)"
cat > "$tmpdir/Probe.java" <<'EOF'
import java.util.concurrent.*;
import java.util.concurrent.atomic.*;
public class Probe {
public static void main(String[] args) throws Exception {
CyclicBarrier c = new CyclicBarrier(2);
AtomicInteger firstReads = new AtomicInteger();
AtomicInteger firstCommitted = new AtomicInteger();
class State {
boolean recentlyCompleted = false;
int playedCount = 0;
}
var state = new State();
Runnable request = () -> {
try {
boolean beforeCommit1 = state.recentlyCompleted;
c.await();
boolean beforeCommit2 = state.recentlyCompleted;
c.await();
state.recentlyCompleted = true;
state.playedCount++;
c.await();
if (beforeCommit1 && beforeCommit2) firstCommitted.incrementAndGet();
else firstReads.incrementAndGet();
} catch (Exception ignored) {}
};
Thread[] ts = new Thread[2];
for (int i = 0; i < ts.length; i++) ts[i] = new Thread(request);
for (Thread t : ts) t.start();
for (Thread t : ts) t.join();
System.out.println("java.runtime.version=" + System.getProperty("java.runtime.version"));
System.out.println("current.count=" + state.playedCount);
System.out.println("firstReads.false=true=" + firstReads.get());
System.out.println("firstReads.true=true=" + firstCommitted.get());
}
}
EOF
javac "$tmpdir/Probe.java"
java -cp "$tmpdir" Probe
else
echo "javac/java unavailable or javac command missing"
fiRepository: Musereview/BE
Length of output: 333
🌐 Web query:
Java ConcurrentModificationException ArrayList parallelStream forEach modification during iteration
💡 Result:
Modifying an ArrayList while iterating over it using parallelStream.forEach is an unsafe, non-compliant operation that violates the core principle of stream non-interference [1][2]. It is strongly discouraged and can lead to several unpredictable outcomes [2][3][4]. The primary risks and behaviors include: 1. ConcurrentModificationException: While the stream framework and the underlying ArrayList spliterator attempt to detect structural modifications (adding, removing, or resizing) in a fail-fast manner, this is done on a "best-effort" basis [5][6][7]. Depending on the timing and the specific implementation of the split, you might encounter a ConcurrentModificationException, but you cannot rely on it to catch all violations [5][6]. 2. Data Loss and Corruption: ArrayList is not thread-safe [6][4]. When a parallel stream modifies the list concurrently, multiple threads may attempt to perform structural modifications simultaneously [8][4]. This can cause race conditions where data is lost (e.g., two threads overwriting the same index) or the internal structure of the ArrayList becomes corrupted, potentially leading to other errors such as ArrayIndexOutOfBoundsException [3][8][4]. 3. Undefined Behavior: Even if an exception is not thrown, the results of the operation are non-deterministic [2][4]. You may experience incorrect data, inconsistent output, or silent failures [2][3][9]. To avoid these issues, follow these best practices: - Do not use stateful lambdas that modify the source collection [3][9][4]. - Use proper Stream collectors (e.g.,.collect(Collectors.toList)) to generate new collections rather than mutating existing ones [3][4]. - If you must accumulate results into a collection, use thread-safe alternatives or, preferably, the built-in reduction operations provided by the Stream API [3][9][8]. As noted in the official documentation, stream pipelines should be non-interfering, meaning the data source should not be modified during the execution of the stream pipeline [1][10]. Relying on fail-fast exceptions like ConcurrentModificationException for program correctness is considered an anti-pattern and a bug [5][6].
Citations:
- 1: https://docs.oracle.com/javase/tutorial/collections/streams/parallelism.html
- 2: https://stackoverflow.com/questions/30590378/java-8-stream-api-exceptions-when-modifying-lists
- 3: https://stackoverflow.com/questions/63914806/java-8-parallelstream-foreach-result-data-loss
- 4: https://stackoverflow.com/questions/44381516/why-using-parallelstream-access-and-modify-collection-get-different-result
- 5: https://stackoverflow.com/questions/50987911/java-stream-foreach-concurrentmodificationexception-unexpected-behaviour
- 6: https://docs.oracle.com/javase/8/docs/api/java/util/ArrayList.html
- 7: https://cr.openjdk.org/~jjg/8159855/api.01/api/java/util/ArrayList.html
- 8: https://stackoverflow.com/questions/64353250/getting-arrayindexoutofboundsexception-when-using-parallel-stream
- 9: https://stackoverflow.com/questions/77444519/parallelstream-foreach-giving-inconsistent-output
- 10: https://stackoverflow.com/questions/53923668/java-util-concurrentmodificationexception-streams
MIDI 저장 제한을 “조회 + 완료” 단위로 원자화하세요.
validateMidiSaveRequestInterval(userId)가 exists...로 조회한 뒤 completeWithMidiData()로 상태를 바꿀 때까지, 동일 사용자의 동시 요청은 둘 다 최근 완료 이력이 없다고 읽을 수 있습니다. 같은 userId에 대해 PESSIMISTIC_WRITE 조회하거나 DB 기본값/애너테이션 단위의 원자적 rate-limit 삽입을 사용해 1분 제한을 보장하면 좋습니다. 관련 개념: https://docs.spring.io/spring-framework/reference/data-access/transactional/declarative-locking.html
🤖 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/service/PlayingService.java` around lines
73 - 85, 원자성이 보장되지 않는 validateMidiSaveRequestInterval(userId)의 조회 후
completeWithMidiData(midiEvents) 완료 흐름을 수정하세요. 동일 userId의 동시 요청이 하나만 1분 제한을
통과하도록 최근 완료 이력 조회에 PESSIMISTIC_WRITE 잠금을 적용하거나 DB 기본값/애너테이션 기반 원자적 rate-limit
삽입을 사용하고, 기존 MIDI 이벤트 변환 및 완료 동작은 유지하세요.
There was a problem hiding this comment.
현재 요구사항이 단순히 연속 클릭이나 중복 요청을 줄이기 위한 보조 검증을 위한 기능인데, 동일 playingId 중복 요청은 @Version 낙관적 락으로 방어할 수 있지 않나요?
p1001q
left a comment
p1001q
left a comment
There was a problem hiding this comment.
리뷰 요청하신 READY→IN_PROGRESS 흐름:
지금 구현(startPlaying())은 createBackingTrack()(READY 생성) 직후 바로 start()(IN_PROGRESS 전환)를 같은 트랜잭션에서 호출해서, 클라이언트 입장에선 READY 상태가 실제로 존재하는 순간이 없어요(응답도 항상 IN_PROGRESS로 옴).
"대시보드 설정 후 바로 시작 안 할 수도 있다"는 가정을 살리려면 생성(READY)과 시작(IN_PROGRESS 전환)을 별도 API로 나눠야 할 텐데, 지금 PR 스코프엔 그 별도 엔드포인트가 없어서 이 설계가 실제로 활용되고 있진 않습니다.
지금 당장 문제는 아니고(READY 검증 로직 자체는 있으니 나중에 별도 "연주 재개/시작" API가 생기면 바로 쓸 수 있음), 지금 스코프에서는 그냥 createBackingTrack() 결과를 바로 IN_PROGRESS로 만들어도 결과는 동일하다는 정도로 이해하시면 될 것 같아요
굳이 지금 구조를 바꿀 필요는 없어 보입니다!
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/service/PlayingService.java (1)
45-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win테스트 stub을 서비스 호출과 일치시키세요.
Line [45]에서 서비스는
findByIdAndDeletedAtIsNull(...)을 호출합니다. 하지만src/test/java/com/mr/domain/playing/service/PlayingServiceTest.java의startPlaying_success()는findById(...)만 stub합니다. 따라서 성공 테스트가BACKING_TRACK_NOT_FOUND예외로 종료됩니다.테스트 stub을 다음처럼 변경하세요.
수정 예시
- given(backingTrackRepository.findById(backingTrackId)) + given(backingTrackRepository.findByIdAndDeletedAtIsNull(backingTrackId))🤖 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/service/PlayingService.java` around lines 45 - 46, Update the success-path stubbing in PlayingServiceTest.startPlaying_success() to mock backingTrackRepository.findByIdAndDeletedAtIsNull(...) with the requested backing-track ID, matching the call used by PlayingService; remove the mismatched findById stub so the test reaches the normal success flow.
🤖 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/service/PlayingService.java`:
- Around line 45-46: Update the success-path stubbing in
PlayingServiceTest.startPlaying_success() to mock
backingTrackRepository.findByIdAndDeletedAtIsNull(...) with the requested
backing-track ID, matching the call used by PlayingService; remove the
mismatched findById stub so the test reaches the normal success flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c987670f-d50c-46f6-bccf-b6fdc205cc87
📒 Files selected for processing (4)
src/main/java/com/mr/domain/playing/exception/MidiEventErrorStatus.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/service/PlayingServiceTest.java
💤 Files with no reviewable changes (3)
- src/main/java/com/mr/domain/playing/repository/PlayingRepository.java
- src/main/java/com/mr/domain/playing/exception/MidiEventErrorStatus.java
- src/test/java/com/mr/domain/playing/service/PlayingServiceTest.java
3 participants
📍 개요
⛓️💥 관련 이슈
🛠️ 작업 내용
🔥 리뷰 요청 사항
✅ 체크리스트
📎 참고 사항
Summary by CodeRabbit
새 기능
버그 수정
테스트