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에 읽기 전용 트랜잭션 어노테이션을 추가하고, FilterContextHolder를 통한 스레드 로컬 필터 모드 관리를 도입하였으며, StatusFilterAspect를 간소화하고 BaseJpaEntity의 Hibernate 필터 정의를 단순화하였습니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 Results484 tests 484 ✅ 44s ⏱️ Results for commit 68d2fb4. |
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.
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
릴리스 노트
리팩토링
테스트