Repository navigation
20260201 #229 출첵qr - #212
Merged
Hidden character warning
The head ref may contain hidden characters: "20260201_#229_\ucd9c\uccb5QR"
Merged
Conversation
WalkthroughQuartz 기반 스케줄러와 윈도우 단위 회전형 QR 토큰 유틸, SSE 토큰 스트리밍 서비스 및 컨트롤러 엔드포인트, 그리고 관련 권한·유지보수 로직이 추가/수정되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as AttendanceRoundController
participant StreamService as QrTokenStreamService
participant RoundService as AttendanceRoundService
participant Repo as AttendanceRoundRepository
participant Scheduler as Quartz Scheduler
participant Job as AttendanceRoundStateJob
rect rgba(100,200,150,0.5)
Note over Client,Controller: SSE QR 토큰 구독 플로우
Client->>Controller: GET /api/attendance/rounds/{roundId}/qr-stream
Controller->>StreamService: subscribe(roundId, userId)
StreamService->>RoundService: validateRound(roundId) / check admin
RoundService->>Repo: fetch round
StreamService-->>Controller: SseEmitter 반환
Controller-->>Client: SSE 연결 설정
StreamService->>StreamService: 정기 토큰 발행 및 핑 전송
StreamService-->>Client: event "qrToken" (토큰 페이로드)
StreamService-->>Client: event "ping" ("ok")
end
rect rgba(150,150,200,0.5)
Note over Scheduler,Job: Quartz 기반 라운드 상태 유지보수
Scheduler->>Job: trigger at ROUND_STATE_CRON
Job->>RoundService: runRoundStatusMaintenance()
RoundService->>Repo: 조회 및 상태 전환 (UPCOMING→ACTIVE, ACTIVE→CLOSED)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/org/sejongisc/backend/common/auth/controller/AuthCookieHelper.java (1)
17-24:⚠️ Potential issue | 🟠 Major삭제 쿠키의
secure/sameSite가 생성 쿠키와 불일치합니다.로컬 HTTP 환경에서
deleteCookie()가secure=true로 내려가면 브라우저가 응답을 무시할 수 있어 로그아웃이 실패할 위험이 있습니다. 생성/삭제 쿠키 속성을 동일한 설정으로 맞추세요.✅ 제안 수정안(속성 일관화)
public ResponseCookie deleteCookie(String name) { return ResponseCookie.from(name, "") .httpOnly(true) - .secure(true) - .sameSite("None") + .secure(cookieSecure) + .sameSite(cookieSameSite) .path("/") .maxAge(0) .build(); }Also applies to: 27-37
🤖 Fix all issues with AI agents
In
`@backend/src/main/java/org/sejongisc/backend/attendance/config/AttendanceQuartzConfig.java`:
- Around line 48-58: The override of
AutowiringSpringBeanJobFactory.createJobInstance currently calls
beanFactory.createBean(jobClass) and bypasses SpringBeanJobFactory's JobDataMap
injection; change it to call Object job = super.createJobInstance(bundle) to
preserve the parent class's JobDataMap wiring, then perform dependency injection
via the AutowireCapableBeanFactory (e.g., beanFactory.autowireBean(job) or
beanFactory.initializeBean(job, jobClass.getName())), and finally return that
job instance so both JobDataMap and Spring DI are applied (references:
AutowiringSpringBeanJobFactory, createJobInstance, SpringBeanJobFactory,
beanFactory).
In
`@backend/src/main/java/org/sejongisc/backend/attendance/service/QrTokenStreamService.java`:
- Line 46: The ScheduledExecutorService field scheduler in QrTokenStreamService
is never shut down; add a lifecycle cleanup to call scheduler.shutdown() (and
optionally scheduler.shutdownNow() after a timeout and awaitTermination) to
avoid thread leaks. Implement this by adding a `@PreDestroy-annotated` method
(e.g., cleanup() in QrTokenStreamService) or by implementing
DisposableBean.destroy(), call scheduler.shutdown(), wait for termination with a
bounded timeout, and log/handle InterruptedException and non-termination cases
so the executor is reliably stopped on application shutdown.
In
`@backend/src/main/java/org/sejongisc/backend/common/auth/controller/AuthCookieHelper.java`:
- Around line 27-37: The createCookie method currently hardcodes ResponseCookie
builder flags (.secure(false), .sameSite("Lax")), which will send auth cookies
in plaintext in production; update AuthCookieHelper.createCookie to read a
configuration flag (e.g., injected boolean like cookie.secure or active profile)
and branch the builder so that in production it sets .secure(true) and
.sameSite("None") (or configurable SameSite) while keeping
.secure(false)/SameSite=Lax for local/dev; add the config property in
application.properties and inject it into AuthCookieHelper (via `@Value` or
Environment) so the cookie settings are driven by configuration rather than
hardcoded values.
🧹 Nitpick comments (3)
backend/src/main/java/org/sejongisc/backend/attendance/service/AttendanceRoundStateJob.java (1)
23-30: 예외 처리 방식 검토 권장현재 모든 예외를 로그만 남기고 무시하고 있습니다. Quartz에서 작업 실패를 추적하려면
JobExecutionException을 던지는 것이 좋습니다.💡 선택적 개선안
`@Override` public void execute(JobExecutionContext context) throws JobExecutionException { try { attendanceRoundService.runRoundStatusMaintenance(); } catch (Exception e) { log.error("AttendanceRoundStateJob failed", e); + throw new JobExecutionException("Round state maintenance failed", e, false); } }유지보수 작업의 특성상 현재 방식도 합리적이지만, Quartz 모니터링을 활용하려면 예외를 전파하는 것을 고려해 주세요.
backend/src/main/java/org/sejongisc/backend/attendance/util/RollingQrTokenUtil.java (1)
105-113:MessageDigest.isEqual()사용 고려JDK 표준 라이브러리의
MessageDigest.isEqual()을 사용하면 timing-safe 비교를 더 간결하게 구현할 수 있습니다. 같은 코드베이스의QrTokenUtil에서도 이 방식을 사용하고 있습니다.💡 선택적 개선안
+import java.security.MessageDigest; + private static boolean constantTimeEquals(String a, String b) { - if (a == null || b == null) return false; - if (a.length() != b.length()) return false; - int result = 0; - for (int i = 0; i < a.length(); i++) { - result |= a.charAt(i) ^ b.charAt(i); - } - return result == 0; + if (a == null || b == null) return false; + return MessageDigest.isEqual( + a.getBytes(StandardCharsets.UTF_8), + b.getBytes(StandardCharsets.UTF_8) + ); }backend/src/main/java/org/sejongisc/backend/attendance/config/AttendanceQuartzConfig.java (1)
19-35: 스케줄 타임존 명시 여부를 확인해주세요.Quartz 크론은 기본적으로 서버 타임존을 사용하므로, 서버가 UTC 등 다른 TZ이면 실제 실행 시간이 밀릴 수 있습니다. 업무 기준 TZ를 명시하거나 설정값으로 주입하는 편이 안전합니다 (Line 34).
🕒 타임존 명시 예시
+import java.util.TimeZone; ... - .withSchedule(CronScheduleBuilder.cronSchedule(ROUND_STATE_CRON)) + .withSchedule( + CronScheduleBuilder.cronSchedule(ROUND_STATE_CRON) + .inTimeZone(TimeZone.getTimeZone("Asia/Seoul")) + )
discipline24
previously approved these changes
Feb 1, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@backend/src/main/java/org/sejongisc/backend/attendance/service/AttendanceRoundStateJob.java`:
- Around line 25-29: The catch in AttendanceRoundStateJob around
attendanceRoundService.runRoundStatusMaintenance() currently only logs and
swallows exceptions; change it to rethrow a JobExecutionException so Quartz
recognizes failures: in the catch(Exception e) block wrap the caught exception
into new JobExecutionException("AttendanceRoundStateJob failed", e) and throw it
(preserving the original exception as the cause and message), while keeping the
existing log if desired.
In
`@backend/src/main/java/org/sejongisc/backend/attendance/service/QrTokenStreamService.java`:
- Around line 97-123: In startRoundTasksIfAbsent wrap both scheduled runnables
(the one passed to tokenTaskByRound via scheduler.scheduleAtFixedRate that calls
broadcastNewToken(rid) and the ping runnable stored in pingTaskByRound) in a
top-level try-catch(Exception) that logs the exception (using the class logger)
and prevents it from escaping; ensure broadcastNewToken/emit loops are invoked
inside that try block so any RuntimeException from findRoundById or
RollingQrTokenUtil.issue() (or emitter.send) is caught and logged and the
scheduled task continues running.
- Around line 172-181: completeAll has the same race as removeEmitter: between
emittersByRound.get(roundId) and emittersByRound.remove(roundId) a new
subscriber can be added; fix by atomically removing the list before iterating —
e.g., call emittersByRound.remove(roundId) (or use emittersByRound.compute to
atomically extract-and-remove) to obtain the List<SseEmitter> and then iterate
and complete each emitter; reference the completeAll method and emittersByRound,
and follow the same atomic approach used in removeEmitter/subscribe
(compute/computeIfAbsent) to avoid lost/duplicated emitters.
- Around line 161-170: removeEmitter has a check-then-act race: concurrently a
subscribe() using computeIfAbsent can add an emitter between emitters.isEmpty()
and emittersByRound.remove(roundId), causing newly added emitters to be lost.
Fix removeEmitter(UUID roundId, SseEmitter emitter) to update the
ConcurrentHashMap atomically using emittersByRound.compute (or
computeIfPresent): inside the remapping function remove the emitter from the
list and return null if the list becomes empty (to delete the key) or return the
list otherwise; then call stopRoundTasksIfNoEmitters only when the compute
result indicates the key was removed (i.e., compute returned null or previous
mapping removed).
🧹 Nitpick comments (1)
backend/src/main/java/org/sejongisc/backend/attendance/service/QrTokenStreamService.java (1)
47-47: 고정 스레드 풀 크기 4가 동시 활성 라운드 수에 비해 부족할 수 있습니다.각 활성 라운드마다 토큰 브로드캐스트 + 핑 = 2개의 주기적 태스크가 필요합니다.
scheduleAtFixedRate는 스레드가 부족하면 실행이 지연되어 토큰 갱신 타이밍이 어긋날 수 있습니다. 동시에 활성화되는 라운드 수가 2개를 초과할 가능성이 있다면 풀 크기를 늘리거나, 구성 가능하게(@Value) 만드는 것을 고려하세요.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@backend/src/main/java/org/sejongisc/backend/attendance/service/QrTokenStreamService.java`:
- Around line 70-92: The subscribe() flow adds the emitter to emittersByRound
but only removes it on IOException; if RollingQrTokenUtil.issue(...) or any
non-IOException throws, the emitter remains orphaned and
startRoundTasksIfAbsent(...) is never called. Fix by wrapping the immediate
token-send and its emitter registration in a try/catch/finally that on any
Exception calls removeEmitter(roundId, emitter) (or removes on failure before
rethrowing) and only calls startRoundTasksIfAbsent(roundId) after a successful
send; reference emittersByRound, removeEmitter(...),
RollingQrTokenUtil.issue(...), emitter, and startRoundTasksIfAbsent(...) when
making the change.
discipline24
approved these changes
Feb 7, 2026
This was referenced Feb 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
새 기능
개선 사항
보안