[Refactor] StatusFilterAspect 동작 수정#352
Open
seongjunnoh wants to merge 7 commits intodevelopfrom
Open
Conversation
- statusFilter는 'status = active' 쿼리로 고정 - 굳이 in 절 필요없다고 판단
- active_only, unfiltered(필터 적용 X == status 조건 제거) 2가지 mode - active_only mode가 default
- 트랜잭션 경계에서 status filter 자동으로 동작하도록 설정 - @unfiltered 동작 시 ThreadLocal mode 변경 & 필터 동작 X - 사용하지 않는 @IncludeInactive 제거
- status filter는 트랜잭션이 열려있어야 동작 가능 - 조회 시 사용하는 영속성 adapter 클래스에 read only 트랜잭션 명시 - QueryPersistenceAdapter 하위의 QueryDSL, spring data jpa repository 인터페이스를 활용하는 DB access 코드 모두 status filter 동작 가능 - 트랜잭션 명시로 인해 DB에 추가적인 command가 날라가지만, 이는 감수할만한 트레이드 오프라고 판단
…sFilterAspect 동작하도록 순서 강제 (#351) - TransactionInterceptor 동작으로 Hibernate Session이 열려야, StatusFilterAspect 에서 Session에 Hibernate Filter 활성화 가능
3 tasks
Walkthrough트랜잭션 경계와 무관하게 상태 필터를 일관되게 적용하도록 시스템을 재구성했습니다. 여러 QueryPersistenceAdapter에 읽기 전용 트랜잭션 어노테이션을 추가하고, ThreadLocal 기반 FilterContextHolder를 도입하여 StatusFilterAspect를 간소화했으며 BaseJpaEntity의 Hibernate 필터 정의를 단순화했습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Service / Controller
participant Aspect as StatusFilterAspect
participant FilterCtx as FilterContextHolder
participant Tx as TransactionManager
participant EM as EntityManager / Session
participant DB as Database
Service->>Aspect: 호출 (메서드, `@Unfiltered` 여부)
Aspect->>FilterCtx: 기존 모드 저장 및 필요시 set(UNFILTERED)
Aspect->>Service: proceed()
Service->>Tx: 트랜잭션 시작 (optional)
Tx->>EM: em.unwrap(Session) / 세션 획득
EM->>DB: 쿼리 실행 (Hibernate filter state 적용 여부에 따라 필터 적용)
DB-->>EM: 결과 반환
Tx-->>Service: 트랜잭션 종료 (optional)
Service-->>Aspect: 반환
Aspect->>FilterCtx: 이전 모드 복원
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Test Results487 tests 487 ✅ 45s ⏱️ Results for commit a2e0d6b. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the StatusFilterAspect and related infrastructure to fix several correctness and robustness issues with the Hibernate statusFilter mechanism in the hexagonal (ports & adapters) architecture.
Changes:
- Simplifies the Hibernate filter from a parameterized
status IN (:statuses)to a hardcodedstatus = 'ACTIVE'condition, removing the@IncludeInactiveannotation in favor of a single@Unfilteredmode. - Introduces
FilterContextHolder(ThreadLocal) to decouple@Unfilteredintent (set at service level) from actual Session manipulation (performed at@Transactionaladapter boundary), allowing@Unfilteredto work without requiring@Transactionalon the service method itself. - Adds
@Transactional(readOnly = true)to all*QueryPersistenceAdapterclasses and configures explicit AOP ordering (@EnableTransactionManagement(order = LOWEST_PRECEDENCE - 1)) to guarantee the transaction is open beforeStatusFilterAspectfires.
Reviewed changes
Copilot reviewed 9 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
FilterContextHolder.java |
New ThreadLocal holder for FilterMode (ACTIVE_ONLY / UNFILTERED) to propagate filter intent across AOP boundaries |
StatusFilterAspect.java |
Refactored to use ThreadLocal; unfiltered() records intent, enableActiveByDefault() reads it; simplified filter enable/disable logic |
AppConfig.java |
Adds @EnableTransactionManagement(order = LOWEST_PRECEDENCE - 1) to ensure transaction opens before StatusFilterAspect runs |
BaseJpaEntity.java |
Simplifies @FilterDef / @Filter to parameterless status = 'ACTIVE' |
IncludeInactive.java |
Deleted — no longer supported |
ErrorCode.java |
Removes PERSISTENCE_TRANSACTION_REQUIRED (no longer needed) |
All *QueryPersistenceAdapter files (14) |
Adds @Transactional(readOnly = true) at class level to guarantee filter activation |
StatusFilterTestConfig.java, StatusFilterTest.java, TestUserJpaRepository.java |
Rewrites tests to validate adapter-boundary filter behavior and @Unfiltered with/without @Transactional |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+36
to
44
| private static final String PCUT_UNFILTERED = "@annotation(" + ANN_UNFILTERED + ")"; | ||
|
|
||
| /** | ||
| * 기본: @Transactional 경계에서 ThreadLocal을 읽어 filter를 활성화. | ||
| * @Unfiltered가 붙은 메서드는 제외 (PCUT_UNFILTERED에서 처리). | ||
| */ | ||
| private static final String PCUT_TX_DEFAULT = | ||
| "(" + "@annotation(" + ANN_TX + ") || @within(" + ANN_TX + ")" + ")" + | ||
| " && !" + "@annotation(" + ANN_INCLUDE_INACTIVE + ")" + | ||
| " && !" + "@annotation(" + ANN_UNFILTERED + ")"; |
There was a problem hiding this comment.
The @Unfiltered annotation (declared in Unfiltered.java) uses @Target({ElementType.METHOD, ElementType.TYPE}), which allows class-level usage. However, neither PCUT_UNFILTERED nor the exclusion in PCUT_TX_DEFAULT handles class-level @Unfiltered:
PCUT_UNFILTERED = "@annotation(ANN_UNFILTERED)"— only matches method-level annotations.PCUT_TX_DEFAULTexclusion&& !@annotation(ANN_UNFILTERED)— also only checks method-level annotations.
If @Unfiltered is placed on a class, its methods would still be intercepted by PCUT_TX_DEFAULT (which uses @within for @Transactional), causing the filter to be incorrectly activated. The AOP implementation should either remove ElementType.TYPE from @Unfiltered's @Target to prevent unsupported class-level usage, or extend both pointcuts with @within handling (e.g., || @within(ANN_UNFILTERED)) to properly support class-level annotation.
Comment on lines
+21
to
+24
| static void clear() { | ||
| context.remove(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The clear() method is defined with package-private visibility, making it inaccessible from outside the konkuk.thip.common.aop package. It is currently not called from anywhere in the codebase (neither production code nor tests). If it's intended for use as a cleanup mechanism (e.g., after each HTTP request via a filter), it needs to be made public. If it's intended only for unit tests of the StatusFilterAspect, it should be called from within the test. Otherwise, this is dead code and should be removed.
Suggested change
| static void clear() { | |
| context.remove(); | |
| } |
![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 (3)
src/main/java/konkuk/thip/room/adapter/out/persistence/RoomParticipantQueryPersistenceAdapter.java (1)
6-10: 조회 어댑터 전용 메타 어노테이션으로 묶어두는 편이 안전합니다.이 PR부터 상태 필터의 정상 동작이
@Transactional(readOnly = true)누락 여부에 달리게 됐는데, 지금처럼 각 QueryPersistenceAdapter에 수동으로 붙이면 신규 어댑터에서 빠뜨리기 쉽습니다.@Repository와@Transactional(readOnly = true)를 합성한 공통 어노테이션으로 의도를 고정해 두는 쪽을 권합니다.Based on learnings, THIP 팀은
Transactional어노테이션을 서비스 메서드에 붙이는 것으로 통일했다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/konkuk/thip/room/adapter/out/persistence/RoomParticipantQueryPersistenceAdapter.java` around lines 6 - 10, Create a composed annotation (e.g., `@ReadOnlyRepository`) that meta-annotates `@Repository` and `@Transactional`(readOnly = true), apply it to RoomParticipantQueryPersistenceAdapter (replacing the existing `@Repository` and `@Transactional` annotations), and update other QueryPersistenceAdapter classes to use this composed annotation so new adapters won’t miss the readOnly transactional intent; remove the manual `@Repository/`@Transactional usage from those classes.src/main/java/konkuk/thip/common/aop/StatusFilterAspect.java (2)
62-72: 필터가 활성화되지 않은 경우에도disableFilter()호출됨
FilterContextHolder.get() == UNFILTERED인 경우 필터가 활성화되지 않지만,wasEnabled가 false이면finally블록에서disableFilter(s)가 호출됩니다. Hibernate의disableFilter는 이미 비활성화된 필터에 대해 호출해도 안전하지만, 불필요한 호출을 피하는 것이 좋습니다.♻️ 제안된 수정
`@Around`(PCUT_TX_DEFAULT) public Object enableActiveByDefault(ProceedingJoinPoint pjp) throws Throwable { Session s = em.unwrap(Session.class); boolean wasEnabled = isFilterEnabled(s); + boolean enabledByThisAdvice = false; if (FilterContextHolder.get() == ACTIVE_ONLY && !wasEnabled) { enableFilter(s); + enabledByThisAdvice = true; } try { return pjp.proceed(); } finally { - if (!wasEnabled) { + if (enabledByThisAdvice) { disableFilter(s); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/konkuk/thip/common/aop/StatusFilterAspect.java` around lines 62 - 72, The finally block is disabling the filter even when it was never enabled (e.g., when FilterContextHolder.get() == UNFILTERED); fix by tracking whether this method actually enabled the filter and only disabling in finally when we enabled it here. Concretely, inside StatusFilterAspect (around the code that checks FilterContextHolder.get() == ACTIVE_ONLY and calls enableFilter(s)), set a local boolean (e.g., enabledHere) to true when enableFilter(s) is invoked, and in the finally block replace the current condition with a check that disables only if enabledHere is true (or if wasEnabled was true when your logic requires it). This uses the existing symbols enableFilter(s), disableFilter(s), FilterContextHolder, ACTIVE_ONLY, wasEnabled and pjp.proceed() to locate the change.
36-44: 클래스 레벨@Unfiltered지원 부족
PCUT_UNFILTERED포인트컷이@annotation()만 사용하고@within()을 포함하지 않아, 클래스 레벨에 선언된@Unfiltered는 인터셉트되지 않습니다.@Unfiltered어노테이션이@Target({ElementType.METHOD, ElementType.TYPE})으로 정의되어 있는 것과 불일치합니다.
PCUT_TX_DEFAULT의 제외 조건(44줄)도@annotation(ANN_UNFILTERED)만 체크하므로, 클래스 레벨@Unfiltered적용 시 해당 클래스의 메서드들이 올바르게 제외되지 않습니다.현재 코드베이스에는 클래스 레벨 사용이 없어 즉각적인 문제는 없으나, 일관성 있는 설계를 위해 포인트컷에
@within()을 추가하는 것이 권장됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/konkuk/thip/common/aop/StatusFilterAspect.java` around lines 36 - 44, PCUT_UNFILTERED currently checks only method-level `@annotation`(ANN_UNFILTERED); update the pointcut to also include class-level `@within`(ANN_UNFILTERED) so class-scoped `@Unfiltered` is matched. Similarly, modify PCUT_TX_DEFAULT's exclusion (the !"@annotation(" + ANN_UNFILTERED + ")") to exclude methods when either `@annotation`(ANN_UNFILTERED) OR `@within`(ANN_UNFILTERED) is present. Apply these changes to the constants PCUT_UNFILTERED and PCUT_TX_DEFAULT in StatusFilterAspect so they reference ANN_UNFILTERED in both `@annotation`(...) and `@within`(...).
🤖 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/konkuk/thip/common/aop/FilterContextHolder.java`:
- Around line 10-22: The ThreadLocal in FilterContextHolder currently only
supports set(FilterMode) and clear(), which loses the outer caller's mode when
nested `@Unfiltered` calls occur; modify FilterContextHolder to save and restore
previous values by adding a method like setAndGetPrevious(FilterMode) or a
restore(FilterMode) (or make set return the previous FilterMode) so callers can
capture the prior mode, and change StatusFilterAspect's around advice to call
get() before setting the new mode and then in finally restore(previous) (or call
clear() only when previous was null) so nested invocations correctly revert to
the outer FilterMode instead of defaulting back to ACTIVE_ONLY.
---
Nitpick comments:
In `@src/main/java/konkuk/thip/common/aop/StatusFilterAspect.java`:
- Around line 62-72: The finally block is disabling the filter even when it was
never enabled (e.g., when FilterContextHolder.get() == UNFILTERED); fix by
tracking whether this method actually enabled the filter and only disabling in
finally when we enabled it here. Concretely, inside StatusFilterAspect (around
the code that checks FilterContextHolder.get() == ACTIVE_ONLY and calls
enableFilter(s)), set a local boolean (e.g., enabledHere) to true when
enableFilter(s) is invoked, and in the finally block replace the current
condition with a check that disables only if enabledHere is true (or if
wasEnabled was true when your logic requires it). This uses the existing symbols
enableFilter(s), disableFilter(s), FilterContextHolder, ACTIVE_ONLY, wasEnabled
and pjp.proceed() to locate the change.
- Around line 36-44: PCUT_UNFILTERED currently checks only method-level
`@annotation`(ANN_UNFILTERED); update the pointcut to also include class-level
`@within`(ANN_UNFILTERED) so class-scoped `@Unfiltered` is matched. Similarly,
modify PCUT_TX_DEFAULT's exclusion (the !"@annotation(" + ANN_UNFILTERED + ")")
to exclude methods when either `@annotation`(ANN_UNFILTERED) OR
`@within`(ANN_UNFILTERED) is present. Apply these changes to the constants
PCUT_UNFILTERED and PCUT_TX_DEFAULT in StatusFilterAspect so they reference
ANN_UNFILTERED in both `@annotation`(...) and `@within`(...).
In
`@src/main/java/konkuk/thip/room/adapter/out/persistence/RoomParticipantQueryPersistenceAdapter.java`:
- Around line 6-10: Create a composed annotation (e.g., `@ReadOnlyRepository`)
that meta-annotates `@Repository` and `@Transactional`(readOnly = true), apply it to
RoomParticipantQueryPersistenceAdapter (replacing the existing `@Repository` and
`@Transactional` annotations), and update other QueryPersistenceAdapter classes to
use this composed annotation so new adapters won’t miss the readOnly
transactional intent; remove the manual `@Repository/`@Transactional usage from
those classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4286f2ee-3757-47d6-a364-4cd4d3c89c2d
📒 Files selected for processing (24)
src/main/java/konkuk/thip/book/adapter/out/persistence/BookQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/comment/adapter/out/persistence/CommentLikeQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/comment/adapter/out/persistence/CommentQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/common/annotation/persistence/IncludeInactive.javasrc/main/java/konkuk/thip/common/aop/FilterContextHolder.javasrc/main/java/konkuk/thip/common/aop/StatusFilterAspect.javasrc/main/java/konkuk/thip/common/entity/BaseJpaEntity.javasrc/main/java/konkuk/thip/common/exception/code/ErrorCode.javasrc/main/java/konkuk/thip/config/AppConfig.javasrc/main/java/konkuk/thip/feed/adapter/out/persistence/FeedQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/notification/adapter/out/persistence/NotificationQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/post/adapter/out/persistence/PostLikeQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/post/adapter/out/persistence/PostQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/recentSearch/adapter/out/persistence/RecentSearchQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/room/adapter/out/persistence/RoomParticipantQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/room/adapter/out/persistence/RoomQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/roompost/adapter/out/persistence/AttendanceCheckQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/roompost/adapter/out/persistence/RecordQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/roompost/adapter/out/persistence/VoteQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/user/adapter/out/persistence/FollowingQueryPersistenceAdapter.javasrc/main/java/konkuk/thip/user/adapter/out/persistence/UserQueryPersistenceAdapter.javasrc/test/java/konkuk/thip/common/persistence/StatusFilterTest.javasrc/test/java/konkuk/thip/config/StatusFilterTestConfig.javasrc/test/java/konkuk/thip/config/TestUserJpaRepository.java
💤 Files with no reviewable changes (2)
- src/main/java/konkuk/thip/common/exception/code/ErrorCode.java
- src/main/java/konkuk/thip/common/annotation/persistence/IncludeInactive.java
Comment on lines
+10
to
+22
| private static final ThreadLocal<FilterMode> context = | ||
| ThreadLocal.withInitial(() -> FilterMode.ACTIVE_ONLY); | ||
|
|
||
| public static FilterMode get() { | ||
| return context.get(); | ||
| } | ||
|
|
||
| public static void set(FilterMode mode) { | ||
| context.set(mode); | ||
| } | ||
|
|
||
| static void clear() { | ||
| context.remove(); |
There was a problem hiding this comment.
중첩 @Unfiltered 호출에서 상태가 유실됩니다.
지금 API는 set() 후 clear()만 가능해서, @Unfiltered 메서드 A 안에서 또 다른 @Unfiltered 메서드 B를 호출하면 B의 finally에서 clear()한 뒤 A의 남은 구간이 다시 ACTIVE_ONLY로 돌아갑니다. 이전 모드를 저장하고 finally에서 복원하는 방식으로 바꿔야 합니다.
🔧 예시 방향
- public static void set(FilterMode mode) {
- context.set(mode);
- }
-
- static void clear() {
- context.remove();
- }
+ static FilterMode push(FilterMode mode) {
+ FilterMode previous = context.get();
+ context.set(mode);
+ return previous;
+ }
+
+ static void restore(FilterMode previous) {
+ if (previous == null) {
+ context.remove();
+ return;
+ }
+ context.set(previous);
+ }StatusFilterAspect 쪽에서는 around advice에서 이전 값을 받아 finally에서 restore(previous) 하도록 맞추면 됩니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/konkuk/thip/common/aop/FilterContextHolder.java` around lines
10 - 22, The ThreadLocal in FilterContextHolder currently only supports
set(FilterMode) and clear(), which loses the outer caller's mode when nested
`@Unfiltered` calls occur; modify FilterContextHolder to save and restore previous
values by adding a method like setAndGetPrevious(FilterMode) or a
restore(FilterMode) (or make set return the previous FilterMode) so callers can
capture the prior mode, and change StatusFilterAspect's around advice to call
get() before setting the new mode and then in finally restore(previous) (or call
clear() only when previous was null) so nested invocations correctly revert to
the outer FilterMode instead of defaulting back to ACTIVE_ONLY.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/test/java/konkuk/thip/common/persistence/FlushOrderTest.java (1)
20-40: PR 범위와 무관한 테스트로 보입니다 — 별도 PR 분리 권장.이 PR(
#352)의 목표는StatusFilterAspect동작 수정,FilterContextHolder도입,open-in-view: false적용, Query Persistence Adapter에@Transactional(readOnly = true)부여입니다. 반면 본 파일은 HibernateActionQueue의 INSERT/UPDATE flush 순서를 검증하는 학습/문서성 테스트로, AI 요약에서도 "이 테스트는 PR의 필터/트랜잭션 경계 변경과 독립적"이라고 명시하고 있습니다. 리뷰 범위와 변경 의도를 분명히 하기 위해 별도 PR(또는 별도 커밋)로 분리하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/konkuk/thip/common/persistence/FlushOrderTest.java` around lines 20 - 40, The FlushOrderTest (class FlushOrderTest) is unrelated to this PR’s scope and should be removed from this change set and submitted separately; revert or remove the file/changes that add or modify FlushOrderTest from this branch (or move them to a new branch/PR) so the current PR only contains the StatusFilterAspect, FilterContextHolder, open-in-view, and Query Persistence Adapter transaction changes.
🤖 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/test/java/konkuk/thip/common/persistence/FlushOrderTest.java`:
- Around line 71-137: The tests
update_then_insert_in_code__insert_first_at_flush,
update_then_force_flush_then_insert, and
insert_then_update_in_code__insert_first_at_flush lack assertions and only rely
on manual SQL log inspection; add automated verification by registering a
Hibernate StatementInspector (hibernate.session_factory.statement_inspector) in
the test configuration to capture executed SQL statements during em.flush(),
then assert ordering (e.g. assertThat(indexOf("insert ...
신규유저")).isLessThan(indexOf("update ... 수정된유저"))) for the first and third tests
and assert the two separate flush phases for update_then_force_flush_then_insert
(or alternatively enable hibernate.generate_statistics and use Statistics +
OutputCaptureExtension to capture SQL lines and assert their order); update the
test setup to install this inspector or statistics capture and add the
corresponding assertThat checks in each test method to fail CI when ordering
regresses.
- Around line 20-31: Update the class Javadoc on FlushOrderTest to remove the
absolute claim that "INSERT always runs before UPDATE at flush" and instead
state that INSERT runs before UPDATE only when they are in the same flush
operation; explicitly call out that Test 2 demonstrates the different behavior
when an explicit flush() separates operations (in that case UPDATE is executed
in the first flush and INSERT in the subsequent flush). Adjust the summary
sentence and add a short note describing the three test cases (1: same-flush
UPDATE→INSERT => INSERT first, 2: UPDATE→flush()→INSERT => separated flushes
follow code order, 3: INSERT→UPDATE => baseline) so the Javadoc matches the
tests' intent.
- Around line 50-52: The test relies on sequence-style ID allocation but
currently uses a bare `@GeneratedValue` on the id field in FlushOrderTest, which
behaves differently across dialects; change the annotation to
`@GeneratedValue`(strategy = GenerationType.SEQUENCE) on the private Long id field
to guarantee SEQUENCE allocation during tests, and add a short Javadoc on the
test class or test method explaining that this assertion depends on SEQUENCE
semantics and will not hold on IDENTITY-based databases (e.g., MySQL), so the
behavior is environment-specific.
---
Nitpick comments:
In `@src/test/java/konkuk/thip/common/persistence/FlushOrderTest.java`:
- Around line 20-40: The FlushOrderTest (class FlushOrderTest) is unrelated to
this PR’s scope and should be removed from this change set and submitted
separately; revert or remove the file/changes that add or modify FlushOrderTest
from this branch (or move them to a new branch/PR) so the current PR only
contains the StatusFilterAspect, FilterContextHolder, open-in-view, and Query
Persistence Adapter transaction changes.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 78539dd0-adf2-4fb5-a567-6846db6416ce
📒 Files selected for processing (1)
src/test/java/konkuk/thip/common/persistence/FlushOrderTest.java
Comment on lines
+20
to
+31
| /** | ||
| * flush 시점에 INSERT가 UPDATE보다 항상 먼저 실행되는지 검증하는 테스트. | ||
| * | ||
| * 로그에서 직접 SQL 순서를 눈으로 확인할 것. | ||
| * application-test.yml 또는 test/resources에서 | ||
| * spring.jpa.show-sql=true 또는 logging.level.org.hibernate.SQL=debug 설정 필요. | ||
| * | ||
| * [테스트 3종] | ||
| * 1. 코드 순서 UPDATE → INSERT : flush 시 INSERT 먼저 (핵심 케이스) | ||
| * 2. 코드 순서 UPDATE → flush() → INSERT : 강제 flush로 분리된 케이스 | ||
| * 3. 코드 순서 INSERT → UPDATE : 자연스러운 순서 기준선 | ||
| */ |
There was a problem hiding this comment.
클래스 Javadoc 요약이 테스트 2와 모순됩니다.
상단 주석은 "flush 시점에 INSERT가 UPDATE보다 항상 먼저 실행되는지 검증"이라 단언하지만, 테스트 2는 사이에 강제 flush()를 끼워 첫 flush에서 UPDATE만, 두 번째 flush에서 INSERT가 나가는 케이스(즉 UPDATE가 먼저 실행되는 시나리오)입니다. "동일 flush 내"라는 조건을 명시하거나, 테스트 2를 "강제 flush로 분리되면 코드 순서대로 나간다"는 별개 명제로 표현해 주세요.
제안 diff
-/**
- * flush 시점에 INSERT가 UPDATE보다 항상 먼저 실행되는지 검증하는 테스트.
+/**
+ * 동일 flush 내에서 Hibernate ActionQueue가 INSERT를 UPDATE보다 먼저 실행하는지 검증하는 테스트.
+ * (단, 중간에 강제 flush()가 들어가면 각 flush 경계 단위로 분리되어 처리됨 — 테스트 2 참고)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/konkuk/thip/common/persistence/FlushOrderTest.java` around
lines 20 - 31, Update the class Javadoc on FlushOrderTest to remove the absolute
claim that "INSERT always runs before UPDATE at flush" and instead state that
INSERT runs before UPDATE only when they are in the same flush operation;
explicitly call out that Test 2 demonstrates the different behavior when an
explicit flush() separates operations (in that case UPDATE is executed in the
first flush and INSERT in the subsequent flush). Adjust the summary sentence and
add a short note describing the three test cases (1: same-flush UPDATE→INSERT =>
INSERT first, 2: UPDATE→flush()→INSERT => separated flushes follow code order,
3: INSERT→UPDATE => baseline) so the Javadoc matches the tests' intent.
Comment on lines
+50
to
+52
| @Id | ||
| @GeneratedValue | ||
| private Long id; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 운영/테스트 DB 방언 및 ddl-auto, identity 사용 여부 확인
fd -e yml -e yaml . src/main/resources src/test/resources 2>/dev/null | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'
rg -nP '@GeneratedValue|GenerationType\.' --type=java -C1Repository: THIP-TextHip/THIP-Server
Length of output: 6227
🏁 Script executed:
#!/bin/bash
# Find and display test configuration files
find src/test/resources src/main/resources -name "application*.yml" -o -name "application*.yaml" -o -name "application*.properties" 2>/dev/null | head -20Repository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Read the FlushOrderTest to understand what it's testing
cat -n src/test/java/konkuk/thip/common/persistence/FlushOrderTest.javaRepository: THIP-TextHip/THIP-Server
Length of output: 5399
🏁 Script executed:
#!/bin/bash
# Search for test configuration files in all common locations
find src/test/resources -type f \( -name "*.yml" -o -name "*.yaml" -o -name "*.properties" \) 2>/dev/null
find . -maxdepth 3 -path "*test*" -name "application*" 2>/dev/null | head -20Repository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check if there's a pom.xml or build.gradle to see test database dependency
if [ -f "pom.xml" ]; then
echo "=== pom.xml (searching for DB dependencies) ==="
grep -A2 -B2 "h2\|mysql\|mariadb\|postgresql" pom.xml | head -40
fi
if [ -f "build.gradle" ]; then
echo "=== build.gradle (searching for DB dependencies) ==="
grep -A2 -B2 "h2\|mysql\|mariadb\|postgresql" build.gradle | head -40
fiRepository: THIP-TextHip/THIP-Server
Length of output: 744
🏁 Script executed:
#!/bin/bash
# Search more thoroughly for test configuration
find . -name "*.yml" -o -name "*.yaml" -o -name "*.properties" | grep -E "(test|application)" | head -30Repository: THIP-TextHip/THIP-Server
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check src/test/resources directory structure
ls -la src/test/resources/ 2>/dev/null || echo "src/test/resources not found"
find src/test/resources -type f 2>/dev/null | head -20Repository: THIP-TextHip/THIP-Server
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Check main resources for application configuration
ls -la src/main/resources/ 2>/dev/null || echo "src/main/resources not found"
find src/main/resources -type f 2>/dev/null | head -20Repository: THIP-TextHip/THIP-Server
Length of output: 881
🏁 Script executed:
#!/bin/bash
# Read application.yml if it exists
if [ -f "src/main/resources/application.yml" ]; then
echo "=== application.yml ==="
cat -n src/main/resources/application.yml
fiRepository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for test configuration classes or profiles
rg -l "@TestConfiguration|@ActiveProfiles|@DataJpaTest" --type=java src/test | head -10
rg "spring.jpa.database|hibernate.dialect" --type-list --type=yaml --type=xml | head -20Repository: THIP-TextHip/THIP-Server
Length of output: 1242
🏁 Script executed:
#!/bin/bash
# Check for application.yml and related files
find src/main/resources -name "application*" -type f 2>/dev/nullRepository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for dialect configuration in the codebase
rg "hibernate.dialect|spring.jpa.database" --type=yaml --type=properties --type=java | head -20Repository: THIP-TextHip/THIP-Server
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Check if application.yml exists in root or elsewhere
find . -maxdepth 4 -name "application.yml" -o -name "application.yaml" 2>/dev/null | head -10Repository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Look for all yaml files in the repo
find . -name "*.yml" -o -name "*.yaml" | grep -v ".git" | head -30Repository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check the structure of the entire repository
ls -la src/
ls -la src/main/Repository: THIP-TextHip/THIP-Server
Length of output: 499
🏁 Script executed:
#!/bin/bash
# Search for Spring configuration in source code
rg "spring.jpa" --type=java | head -20
rg "DataSource|JpaRepository" --type=java | head -10Repository: THIP-TextHip/THIP-Server
Length of output: 1730
🏁 Script executed:
#!/bin/bash
# Check for any Spring configuration or properties in Java code
rg "@Configuration|@Bean.*DataSource|@Bean.*EntityManagerFactory" --type=java -A3 | head -40Repository: THIP-TextHip/THIP-Server
Length of output: 2547
🏁 Script executed:
#!/bin/bash
# Check build.gradle for any test configuration details
head -100 build.gradleRepository: THIP-TextHip/THIP-Server
Length of output: 2963
🏁 Script executed:
#!/bin/bash
# Search for any profile-specific configuration or application.yml with profiles
find . -name "application-test*" -o -name "application.yml" -o -name "application.yaml" 2>/dev/null | grep -v ".git"Repository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check if there are any resource files with database or spring.jpa configuration
find . -type f \( -name "*.yml" -o -name "*.yaml" -o -name "*.properties" \) 2>/dev/null | xargs grep -l "spring.jpa\|hibernate\|datasource" 2>/dev/null | head -10Repository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check for any Spring Boot configuration in src/main
cat src/main/java/konkuk/thip/ThipServerApplication.javaRepository: THIP-TextHip/THIP-Server
Length of output: 666
🏁 Script executed:
#!/bin/bash
# Check the complete directory structure to find config files
find . -type f -name "*.yml" -o -name "*.yaml" | head -20Repository: THIP-TextHip/THIP-Server
Length of output: 428
🏁 Script executed:
#!/bin/bash
# Look at JpaRepositoryMethodTest to see if it has similar issues
cat -n src/test/java/konkuk/thip/common/persistence/JpaRepositoryMethodTest.java | head -80Repository: THIP-TextHip/THIP-Server
Length of output: 3235
🏁 Script executed:
#!/bin/bash
# Check Hibernate and Spring documentation patterns - check for any explicit configuration in test files
rg "@DataJpaTest|@TestPropertySource|application.properties" --type=java src/test | head -20Repository: THIP-TextHip/THIP-Server
Length of output: 897
🏁 Script executed:
#!/bin/bash
# Check the complete FlushOrderTest to see if it has assertions or is just for log inspection
cat -n src/test/java/konkuk/thip/common/persistence/FlushOrderTest.java | tail -50Repository: THIP-TextHip/THIP-Server
Length of output: 2026
🏁 Script executed:
#!/bin/bash
# Look at the `@DataJpaTest` documentation behavior - check Spring Boot version
grep -A2 "org.springframework.boot" build.gradle | head -5Repository: THIP-TextHip/THIP-Server
Length of output: 257
🏁 Script executed:
#!/bin/bash
# Check if H2 is available at test runtime and verify default dialect
rg "H2Dialect|datasource.url" --type=java | head -10Repository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify H2 is being used for `@DataJpaTest` by checking Spring Boot default behavior
# Check if there's any explicit database configuration
rg "spring.datasource|spring.jpa.database|@TestPropertySource" --type=java src/test | head -20Repository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Look at how Spring Boot 3.5.0 configures `@DataJpaTest` with H2
# Check if there's any additional DataSource or test configuration
rg "TestConfiguration|@Bean|DataSource|datasource" --type=java src/test | head -30Repository: THIP-TextHip/THIP-Server
Length of output: 520
🏁 Script executed:
#!/bin/bash
# Verify that production code consistently uses IDENTITY and check if there's any mixed usage
rg "@GeneratedValue" --type=java src/main/java | wc -l
rg "@GeneratedValue\(strategy = GenerationType.IDENTITY\)" --type=java src/main/java | wc -lRepository: THIP-TextHip/THIP-Server
Length of output: 74
🏁 Script executed:
#!/bin/bash
# Verify that H2 is the default test database by checking Spring Data JPA behavior
# H2 with AUTO `@GeneratedValue` uses SEQUENCE, MySQL uses IDENTITY
# Let's confirm what `@DataJpaTest` actually does in Spring Boot 3.5.0 docs context
# Check if there are any other test profiles or database configs we missed
find . -type f \( -name "*.properties" -o -name "*.yml" -o -name "*.yaml" \) 2>/dev/null | xargs grep -l "database\|datasource\|dialect" 2>/dev/null | head -10Repository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify production runs on MySQL and test runs on H2
# Check the migration scripts which suggest what DB schema is expected
head -50 src/main/resources/db/migration/V1__BaseLine.sql | grep -i "auto_increment\|sequence" | head -5Repository: THIP-TextHip/THIP-Server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Final verification: Check if there are any comments or documentation about test vs production DB
rg "H2|MySQL|MariaDB|IDENTITY|SEQUENCE" --type=java src/test | grep -i "test\|prod\|mysql\|h2" | head -10Repository: THIP-TextHip/THIP-Server
Length of output: 50
테스트의 전제를 명시적으로 보장하기 위해 @GeneratedValue 전략을 지정하세요.
현재 bare @GeneratedValue는 데이터베이스 방언에 따라 다르게 동작합니다:
- H2(테스트 DB):
AUTO→SEQUENCE→ ID는 persist 시 할당되고, INSERT는 flush 시까지 지연 - MySQL(운영 DB):
AUTO→IDENTITY→ persist 시점에 즉시 INSERT 실행
테스트 주석 "flush 시 INSERT가 먼저"라는 전제는 SEQUENCE 동작에 의존하므로, ActionQueue 순서 검증 의도를 명확히 하려면 strategy = GenerationType.SEQUENCE를 명시하고, 운영 환경이 MySQL/IDENTITY라면 해당 환경에서 이 보장이 적용되지 않음을 Javadoc에 기록하는 것이 좋습니다.
제안 diff
`@Id`
- `@GeneratedValue`
+ `@GeneratedValue`(strategy = GenerationType.SEQUENCE)
private Long id;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Id | |
| @GeneratedValue | |
| private Long id; | |
| `@Id` | |
| `@GeneratedValue`(strategy = GenerationType.SEQUENCE) | |
| private Long id; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/konkuk/thip/common/persistence/FlushOrderTest.java` around
lines 50 - 52, The test relies on sequence-style ID allocation but currently
uses a bare `@GeneratedValue` on the id field in FlushOrderTest, which behaves
differently across dialects; change the annotation to `@GeneratedValue`(strategy =
GenerationType.SEQUENCE) on the private Long id field to guarantee SEQUENCE
allocation during tests, and add a short Javadoc on the test class or test
method explaining that this assertion depends on SEQUENCE semantics and will not
hold on IDENTITY-based databases (e.g., MySQL), so the behavior is
environment-specific.
Comment on lines
+71
to
+137
| @Test | ||
| @DisplayName("[코드 순서: UPDATE → INSERT] flush 시 INSERT가 항상 먼저 실행된다") | ||
| void update_then_insert_in_code__insert_first_at_flush() { | ||
| /** | ||
| * 코드 작성 순서는 UPDATE → INSERT 이지만, | ||
| * flush 시점에는 Hibernate ActionQueue가 INSERT를 먼저 처리한다. | ||
| * | ||
| * 예상 SQL 로그: | ||
| * insert into test_user (name) values ('신규유저') | ||
| * update test_user set name='수정된유저' where id=? | ||
| */ | ||
|
|
||
| // UPDATE 대상 설정 (코드 상 먼저) | ||
| TestUser existing = em.find(TestUser.class, existingId); | ||
| existing.setName("수정된유저"); // dirty → flush 시 UPDATE | ||
|
|
||
| // INSERT 대상 설정 (코드 상 나중) | ||
| em.persist(new TestUser("신규유저")); | ||
|
|
||
| em.flush(); // ← 여기서 SQL 로그 확인: INSERT(신규유저) 가 UPDATE(수정된유저) 보다 먼저 나와야 함 | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("[코드 순서: UPDATE → flush() → INSERT] 강제 flush 이후 INSERT는 별도 flush에서 실행된다") | ||
| void update_then_force_flush_then_insert() { | ||
| /** | ||
| * 강제 flush를 중간에 끼워 넣으면, 그 시점까지 누적된 변경사항만 반영된다. | ||
| * INSERT 대상은 강제 flush 이후에 등록되었으므로 다음 flush에서 처리된다. | ||
| * | ||
| * 예상 SQL 로그: | ||
| * [첫 번째 flush] update test_user set name='수정된유저' where id=? | ||
| * [두 번째 flush] insert into test_user (name) values ('신규유저') | ||
| */ | ||
|
|
||
| // UPDATE 대상 설정 | ||
| TestUser existing = em.find(TestUser.class, existingId); | ||
| existing.setName("수정된유저"); | ||
|
|
||
| em.flush(); // ← 첫 번째 flush: UPDATE(수정된유저) 만 나감 | ||
|
|
||
| // INSERT 대상 설정 (강제 flush 이후) | ||
| em.persist(new TestUser("신규유저")); | ||
|
|
||
| em.flush(); // ← 두 번째 flush: INSERT(신규유저) 만 나감 | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("[코드 순서: INSERT → UPDATE] 자연스러운 코드 순서에서도 INSERT가 먼저 실행된다 (기준선)") | ||
| void insert_then_update_in_code__insert_first_at_flush() { | ||
| /** | ||
| * 코드 작성 순서가 INSERT → UPDATE 인 자연스러운 케이스. | ||
| * 테스트 1과 결과가 동일해야 한다. | ||
| * | ||
| * 예상 SQL 로그: | ||
| * insert into test_user (name) values ('신규유저') | ||
| * update test_user set name='수정된유저' where id=? | ||
| */ | ||
|
|
||
| // INSERT 대상 설정 (코드 상 먼저) | ||
| em.persist(new TestUser("신규유저")); | ||
|
|
||
| // UPDATE 대상 설정 (코드 상 나중) | ||
| TestUser existing = em.find(TestUser.class, existingId); | ||
| existing.setName("수정된유저"); | ||
|
|
||
| em.flush(); // ← 여기서 SQL 로그 확인: INSERT(신규유저) 가 UPDATE(수정된유저) 보다 먼저 나와야 함 | ||
| } |
There was a problem hiding this comment.
자동 검증 누락 — 테스트가 항상 통과합니다.
세 테스트 모두 assert* 호출이 없고 "SQL 로그를 눈으로 확인"하는 방식이라, ActionQueue 동작이 깨지거나 누군가 코드 순서를 바꿔도 CI는 절대 실패를 잡아내지 못합니다. 사실상 회귀 방지 기능이 없는 문서성 코드입니다. 다음 중 하나로 자동 검증을 추가해 주세요.
- 권장: Hibernate
StatementInspector(hibernate.session_factory.statement_inspector)를 테스트 설정에 등록해 실행 SQL을 캡처하고,assertThat(insertIdx).isLessThan(updateIdx)형태로 순서를 단언. - 또는
@DataJpaTest로깅에 의존하지 말고,Statistics(hibernate.generate_statistics=true) + 로그 캡처(OutputCaptureExtension의CapturedOutput)로 SQL 라인 순서를 검증.
자동 검증이 어렵다면, 이 클래스는 테스트가 아니라 docs/ 또는 README의 예제로 옮기는 것이 명확합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/konkuk/thip/common/persistence/FlushOrderTest.java` around
lines 71 - 137, The tests update_then_insert_in_code__insert_first_at_flush,
update_then_force_flush_then_insert, and
insert_then_update_in_code__insert_first_at_flush lack assertions and only rely
on manual SQL log inspection; add automated verification by registering a
Hibernate StatementInspector (hibernate.session_factory.statement_inspector) in
the test configuration to capture executed SQL statements during em.flush(),
then assert ordering (e.g. assertThat(indexOf("insert ...
신규유저")).isLessThan(indexOf("update ... 수정된유저"))) for the first and third tests
and assert the two separate flush phases for update_then_force_flush_then_insert
(or alternatively enable hibernate.generate_statistics and use Statistics +
OutputCaptureExtension to capture SQL lines and assert their order); update the
test setup to install this inspector or statistics capture and add the
corresponding assertThat checks in each test method to fail CI when ordering
regresses.
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
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.
#️⃣ 연관된 이슈
📝 작업 내용
1. Hibernate Filter 조건 단순화
status IN (:statuses)→status = 'ACTIVE'로 변경@IncludeInactive어노테이션 제거,FilterMode를ACTIVE_ONLY/UNFILTERED2가지로 단순화2. FilterContextHolder (ThreadLocal) 도입
@Unfiltered와@Transactional의 결합 문제 해소@UnfilteredAOP 는 ThreadLocal 에UNFILTERED모드만 기록 (Session 접근 없음)@Transactional경계의StatusFilterAspect가 ThreadLocal 을 읽어 filter 활성 여부 결정@Unfiltered가@Transactional없이도 독립적으로 동작3. Query Persistence Adapter 에
@Transactional(readOnly = true)명시SimpleJpaRepository는 Spring AOP 가 intercept 하지 못하는 구조 → 서비스에@Transactional이 없으면 filter 미적용 경로 존재@Transactional유무와 무관하게 filter 동작 보장4. AOP 실행 순서 명시적 보장
StatusFilterAspect가em.unwrap(Session.class)호출 시 트랜잭션이 먼저 열려있어야 함@EnableTransactionManagement(order = LOWEST_PRECEDENCE - 1)로 TransactionInterceptor 를 outer 로 고정@Order(LOWEST_PRECEDENCE)로 StatusFilterAspect 를 inner 로 명시5. 테스트 정비
StatusFilterTest재작성: adapter 경계 기준으로 filter 동작 검증application-test.yml에open-in-view: false추가하여 production 과 동일한 환경에서 검증📸 스크린샷
💬 리뷰 요구사항
📌 PR 진행 시 이러한 점들을 참고해 주세요
Summary by CodeRabbit
릴리스 노트
리팩터링
테스트