Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Note
|
| 코호트 / 파일 | 요약 |
|---|---|
Redis 인프라 build.gradle, src/main/java/OneQ/OnSurvey/global/common/config/RedisConfig.java, src/main/java/OneQ/OnSurvey/global/common/util/RedisUtils.java |
Redisson Spring Boot 스타터 의존성을 추가하고, 새로운 RedisConfig 빈과 RedisUtils 유틸리티 클래스를 도입합니다. RedisUtils는 분산 잠금, 값 연산, 정렬된 세트 연산을 포함한 정적 메서드를 제공합니다. |
서비스 계층 Redis 통합 src/main/java/OneQ/OnSurvey/domain/participation/service/response/ResponseCommandService.java, src/main/java/OneQ/OnSurvey/domain/survey/service/SurveyGlobalStatsService.java, src/main/java/OneQ/OnSurvey/domain/survey/service/command/SurveyCommandService.java |
StringRedisTemplate을 RedisUtils로 대체합니다. 직접적인 Redis 호출을 단순화하고, 일부 private 헬퍼 메서드를 제거합니다. |
조회 서비스 및 인증 계층 src/main/java/OneQ/OnSurvey/domain/survey/service/query/SurveyQueryService.java, src/main/java/OneQ/OnSurvey/global/auth/application/TossAuthFacade.java, src/main/java/OneQ/OnSurvey/global/auth/token/TokenStore.java |
StringRedisTemplate을 RedisUtils로 대체합니다. SurveyQueryService에 Redis 기반 분산 잠금 메커니즘을 추가합니다. TokenStore에서 공개 메서드 3개를 제거합니다. |
예상 코드 리뷰 노력
🎯 4 (복잡함) | ⏱️ ~60분
관련 가능성 있는 PR
- ✨ OMF-32 현재 접속 중인 고객 수 카운트 #87: 이 PR은 PR
#87에서도입된 Redis 기반 일일 사용자 추적 및 관련 Redis 연산을 직접 StringRedisTemplate 사용에서 새로운 RedisUtils/Redisson 기반 구현으로 마이그레이션합니다. - ✨ OMF-60 설문 응답자가 목표인원을 넘지 않도록 카운트 기능 추가 #82: 두 PR 모두 동일한 참여 및 설문 조사 Redis 로직(예: ResponseCommandService, SurveyQueryService, SurveyCommandService, TokenStore/TossAuthFacade)에 접근하며, 검색된 PR은 Redis 기반 참여자 계산/하트비트 API를 추가하고, 메인 PR은 이러한 직접적인 StringRedisTemplate 호출을 새로운 RedisUtils/Redisson 기반 구현으로 마이그레이션합니다.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 46.34% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | 제목은 PR의 주요 변경 사항(Redis 분산락 적용 및 RedisUtils 유틸리티)을 명확하게 요약하고 있습니다. |
| Description check | ✅ Passed | PR 설명이 주어진 템플릿 구조를 따르고 있으며, 관련 이슈, 완료된 작업 항목, 그리고 검토 요청사항이 포함되어 있습니다. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat/OMF-132
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 help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/OneQ/OnSurvey/domain/participation/service/response/ResponseCommandService.java (2)
53-74:⚠️ Potential issue | 🟠 Major
@Transactional범위 내 Redis 연산의 롤백 불일치
responseRepository.save(),surveyInfo.increaseCompletedCount(),survey.updateSurveyStatus()등 DB 연산은@Transactional로 묶여 있지만,RedisUtils.incrementValue,RedisUtils.removeFromZSet,RedisUtils.deleteKeys는 JPA 트랜잭션에 참여하지 않습니다.즉, DB 트랜잭션이 롤백되더라도 이미 실행된 Redis 연산은 되돌릴 수 없습니다. 대표적인 시나리오:
- Line 62에서
updateCounter가 실행되어 Redis 카운터가 증가하고 ZSet에서 제거된 뒤, 이후 코드에서 예외가 발생하면 DB는 롤백되지만 Redis 카운터는 이미 증가된 상태로 남습니다.PR에서 "현재 로직에서는 보상 트랜잭션이 불필요하다"고 판단하셨으나, 이 불일치가 실제로 문제가 없는 이유(예: 멱등성 보장, 재시도 시 올바른 결과 보장)를 팀 내에서 명확히 정의해두는 것을 권장합니다.
44-77:⚠️ Potential issue | 🔴 Critical분산락이 적용되지 않음 - 동시성 제어 필요
createResponse메서드에 Redisson 분산락이 적용되지 않았습니다. 현재 코드는@Transactional주석만 있으며, 분산락 획득/해제 코드가 없습니다. ParticipationController에서도 직접 호출할 뿐 별도의 락 래핑이 없습니다.다음 임계 구역 전체가 보호되지 않아 TOCTOU(Time-of-check Time-of-use) 취약점이 발생합니다:
- 참여 여부 확인 (
getIsResponded())- 참여 표시 (
markResponded())- Redis 카운터 증가 (
updateCounter())- 설문 상태 변경 (dueCount 도달 시)
RedisUtils.executeWithLock()을 사용하여 전체 메서드를 분산락으로 보호하세요:`@Override` public Boolean createResponse(Long surveyId, Long memberId, Long userKey) { return RedisUtils.executeWithLock( "response:lock:" + surveyId + ":" + memberId, 10, 30, () -> { // 기존 로직 전체 ... } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/OneQ/OnSurvey/domain/participation/service/response/ResponseCommandService.java` around lines 44 - 77, The createResponse method lacks a Redisson distributed lock and is vulnerable to TOCTOU; wrap the entire method body (everything from reading response via responseRepository.findBySurveyIdAndMemberId, the getIsResponded check, response.markResponded()/save, surveyGlobalStatsService.addCompletedCount, surveyInfoRepository.findBySurveyId/increaseCompletedCount, updateCounter call, surveyRepository.getSurveyById/updateSurveyStatus and RedisUtils.deleteKeys) inside RedisUtils.executeWithLock using a per-survey+member key (e.g., "response:lock:" + surveyId + ":" + memberId) with appropriate wait/lease time (like 10,30), return the lambda result, and ensure exceptions propagate as before so existing CustomException handling remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/OneQ/OnSurvey/domain/participation/service/response/ResponseCommandService.java`:
- Line 23: ResponseCommandService 클래스에 `@Slf4j` 어노테이션이 붙어있지만 log 호출이 전혀 없으므로
어노테이션을 제거하거나 실제 로깅을 추가하세요; 구체적으로 ResponseCommandService 내 분산락 처리, Redis 연동, 설문
상태 변경(예: CLOSED 전환) 등 예외/오류 발생 지점에서 log.error/debug/info를 적절히 사용해 실패 원인과
컨텍스트(파라미터, 예외 메시지)를 기록하거나, 당장 로깅이 필요 없다면 클래스 상단의 `@Slf4j` 어노테이션을 삭제해 불필요한 의존성을
제거하세요.
- Around line 62-63: In ResponseCommandService, replace the equality check that
uses Objects.equals(currCompleted, surveyInfo.getDueCount()) with a primitive
comparison using currCompleted >= surveyInfo.getDueCount() (or equivalent) so
you avoid unnecessary autoboxing and handle cases where the counter may exceed
dueCount; locate the updateCounter call and the subsequent if block that
references currCompleted and surveyInfo.getDueCount() and update the condition
to use the >= operator.
- Around line 79-86: The updateCounter method is silently treating a null return
from RedisUtils.incrementValue(this.completedKey + surveyId) as if the counter
became 1; instead detect currCompleted == null as an error and throw a runtime
exception (e.g., IllegalStateException) that includes context (surveyId and
userKey) so calling code can handle the failure, and update the comment to
reflect that null indicates a Redis pipeline/transaction/connection error rather
than “key missing”; keep the call to RedisUtils.removeFromZSet(this.potentialKey
+ surveyId, String.valueOf(userKey)) but do not mask a null increment result.
In
`@src/main/java/OneQ/OnSurvey/domain/survey/service/command/SurveyCommandService.java`:
- Around line 323-329: In applySurveyRuntimeCache, guard against a negative TTL
by computing Duration ttl = Duration.between(LocalDateTime.now(), deadline) and
if ttl.isNegative() set ttl = Duration.ZERO (or a small positive Duration like
Duration.ofSeconds(1)) before calling RedisUtils.setValue; ensure the same
non-negative ttl is used for setting dueCountKey and creatorKey, and keep the
RedisUtils.addToZSet call unchanged (still using System.currentTimeMillis()) so
no Redis set call receives a negative Duration.
In
`@src/main/java/OneQ/OnSurvey/domain/survey/service/query/SurveyQueryService.java`:
- Around line 414-419: Move the expired-potential cleanup into the distributed
lock action to make it atomic with the activePotentialCount calculation: inside
SurveyQueryService's RedisUtils.executeWithLock block (the lambda passed when
acquiring lockKey + surveyId), call RedisUtils.rangeRemoveFromZSet(potentialKey,
0, System.currentTimeMillis() - potentialDuration.toMillis()) before reading
existingScore/getZSetScore and computing activePotentialCount, and remove the
finally-block try/catch that currently calls rangeRemoveFromZSet after lock
release so cleanup happens under the lock and the separate cleanup call is no
longer needed.
- Around line 471-474: The INFO-level log prints a raw boolean duplicate of the
return value from checkAgeSegmentation(...) && checkGenderSegmentation(...),
causing noisy, context-less logs; update the logging in SurveyQueryService
(around the evaluation using checkAgeSegmentation, checkGenderSegmentation,
surveySegmentation and memberSegmentation.convertBirthDayIntoAgeRange) by either
removing the log.info line entirely or demoting it to log.debug with a clear
message (e.g., "segmentation match result for surveyId=X: {}" and include
identifying context) so you don't emit a bare true/false at INFO level while
preserving useful debug info when needed.
- Around line 447-458: The setDueCount method can compute a negative Duration
(Duration.between(LocalDateTime.now(), survey.getDeadline())) which causes
RedisUtils.setValue to be called with a negative TTL and throws
IllegalArgumentException; update setDueCount to compute the TTL safely by
checking survey.getDeadline() (via surveyRepository.getSurveyById) against
LocalDateTime.now() and if deadline is before or equal to now use Duration.ZERO
or a minimal positive TTL (or skip calling RedisUtils.setValue) before calling
RedisUtils.setValue(this.dueCountKey + surveyId,
String.valueOf(surveyInfo.getDueCount()), duration); ensure you reference
setDueCount, surveyInfoRepository, surveyRepository, dueCountKey and
RedisUtils.setValue when making the change.
In `@src/main/java/OneQ/OnSurvey/global/common/config/RedisConfig.java`:
- Around line 19-25: RedisConfig의 password
필드(`@Value`("${spring.data.redis.password}"))가 필수로 되어 있어 프로퍼티 미설정 시 예외를 발생시키고, 빈
문자열을 setPassword("")로 전달하면 Redisson이 AUTH ""를 보내 인증 오류를 유발합니다; 이를 고치려면 `@Value를`
기본값을 허용하도록 변경(예: "${spring.data.redis.password:}" 또는 Optional/nullable 타입)하고
redisson() 메서드 내에서 setPassword(password)를 무조건 호출하지 말고 password가 null 또는 빈 문자열이
아닌 경우에만 config.useSingleServer().setAddress(...).setPassword(password)를 호출하도록
조건문으로 감싸서 빈 비밀번호일 때는 setPassword를 생략하게 하세요 (참조 심볼: class RedisConfig, field
password, method redisson(), call setPassword).
In `@src/main/java/OneQ/OnSurvey/global/common/util/RedisUtils.java`:
- Around line 16-30: RedisUtils currently initializes staticRedisson and
staticRedisTemplate in `@PostConstruct` (init), which can cause NPEs if static
methods are called before init; change the approach by removing reliance on
static fields: make RedisUtils a normal Spring bean and convert any static
helper methods to instance methods (use dependency injection of
StringRedisTemplate and RedissonClient into RedisUtils), or if static access is
absolutely required, replace the static fields with lazy lookups from
ApplicationContext (e.g., fetch RedissonClient/StringRedisTemplate via the
context at call-time) and remove the `@PostConstruct` initialization; update all
call sites that reference RedisUtils.staticRedisson/staticRedisTemplate to use
the instance methods or the lazy lookup API.
- Around line 46-61: The current executeWithLock uses lock.tryLock(waitTIme,
leaseTime, TimeUnit.SECONDS) which can expire the lock before action.get()
completes, causing lock.unlock() to throw IllegalMonitorStateException; update
the finally block to only call unlock when the current thread still holds the
lock (use RLock.isHeldByCurrentThread() or equivalent) or switch to the tryLock
overload without leaseTime to use Redisson's watchdog; specifically modify
executeWithLock (and the finally that calls lock.unlock()) to guard unlock with
lock.isHeldByCurrentThread() to avoid unlocking after leaseTime expiration.
- Line 47: 메서드 시그니처의 파라미터명에 오타가 있습니다: `waitTIme` → `waitTime`으로 수정하세요;
RedisUtils 클래스에서 해당 메서드(찾기 쉬운 식별자: 파라미터명 `waitTIme`) 선언을 변경하고 해당 파라미터를 참조하는 모든
내부 사용처 및 Javadoc/주석을 `waitTime`으로 통일한 뒤 컴파일과 테스트를 실행해 변경이 잘 반영되는지 확인하세요; 퍼블릭 API
영향이 우려되면 기존 `waitTIme` 시그니처에 `@Deprecated된` 오버로드를 추가하고 새 이름을 가진 메서드로 위임하는 방식을
고려하세요.
- Around line 77-90: getIntValue 및 getLongValue에서 Redis에 숫자가 아닌 값이 들어와
Integer.parseInt/Long.parseLong가 던지는 NumberFormatException이 호출부로 전파되므로, 각 메서드에서
parsing을 try-catch로 감싸고 NumberFormatException 발생 시 적절한 CustomException(서비스의 기존
예외 타입과 에러코드 사용)으로 변환하여 던지거나(또는 정책상 안전한 기본값 0/0L로 폴백하고 경고 로깅) 처리하도록 변경하세요; 참조 대상
심볼: getIntValue, getLongValue, staticRedisTemplate, CustomException.
---
Outside diff comments:
In
`@src/main/java/OneQ/OnSurvey/domain/participation/service/response/ResponseCommandService.java`:
- Around line 44-77: The createResponse method lacks a Redisson distributed lock
and is vulnerable to TOCTOU; wrap the entire method body (everything from
reading response via responseRepository.findBySurveyIdAndMemberId, the
getIsResponded check, response.markResponded()/save,
surveyGlobalStatsService.addCompletedCount,
surveyInfoRepository.findBySurveyId/increaseCompletedCount, updateCounter call,
surveyRepository.getSurveyById/updateSurveyStatus and RedisUtils.deleteKeys)
inside RedisUtils.executeWithLock using a per-survey+member key (e.g.,
"response:lock:" + surveyId + ":" + memberId) with appropriate wait/lease time
(like 10,30), return the lambda result, and ensure exceptions propagate as
before so existing CustomException handling remains intact.
✨ Related Issue
📌 Task Details
RedisUtils유틸리티 메서드 구현💬 Review Requirements (Optional)
현재 로직에서는 DB, Redis 간 보상 트랜잭션이 필요하진 않은 것 같아서 추가하진 않았는데, 추후에 크리티컬한 로직이 추가되면 보상 트랜잭션에 대해서도 논의하면 좋을 것 같아요
Summary by CodeRabbit
릴리스 노트
New Features
Refactor
Breaking Changes