btsk-115: 로컬 개발 환경 Docker 전환 및 전체 버그 수정 - #150
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
WalkthroughThis pull request adds idempotency support (annotation, aspect, exception handler), switches Gemini integration to streaming, introduces Nginx SSL configuration and local cert docs, updates repository query for question sets, extends error codes, adjusts cookie path logic, enables Spring Retry, and includes several related tests and Docker/Compose edits. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IdempotencyAspect
participant Redis
participant Service
rect rgb(200,230,255)
Note over Client,Service: First request with Idempotency-Key
Client->>IdempotencyAspect: POST /... (Idempotency-Key)
IdempotencyAspect->>Redis: GET idempotency:hash
Redis-->>IdempotencyAspect: not found
IdempotencyAspect->>Redis: SETNX idempotency:hash (lock, TTL)
Redis-->>IdempotencyAspect: success
IdempotencyAspect->>Service: proceed()
Service-->>IdempotencyAspect: result
IdempotencyAspect->>Redis: SET idempotency:hash = "DONE" (TTL)
IdempotencyAspect-->>Client: 200 OK + result
end
rect rgb(255,230,220)
Note over Client,IdempotencyAspect: Duplicate within TTL
Client->>IdempotencyAspect: POST /... (same Idempotency-Key)
IdempotencyAspect->>Redis: GET idempotency:hash
Redis-->>IdempotencyAspect: "DONE"
IdempotencyAspect-->>Client: 409 CONFLICT (DuplicateRequestException)
end
sequenceDiagram
participant Caller
participant GeminiClient
participant GeminiAPI
participant Stream
Note over Caller,GeminiAPI: Old (sync) flow
Caller->>GeminiClient: getLlmGeneratedQuestionContent()
GeminiClient->>GeminiAPI: generateContent(request)
GeminiAPI-->>GeminiClient: GenerateContentResponse
GeminiClient->>GeminiClient: validate & parse
GeminiClient-->>Caller: Response
Note over Caller,GeminiAPI: New (streaming) flow
Caller->>GeminiClient: getLlmGeneratedQuestionContent()
GeminiClient->>GeminiAPI: generateContentStream(request)
GeminiAPI-->>Stream: ResponseStream<GenerateContentResponse>
loop stream items
Stream-->>GeminiClient: chunk
GeminiClient->>GeminiClient: aggregate & validate finishReason
end
GeminiClient->>GeminiClient: parse aggregated text
GeminiClient-->>Caller: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Areas to review closely:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)src/main/java/kr/it/pullit/modules/questionset/client/GeminiClient.java (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (7)
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 |
Hyeonjun0527
changed the title
![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
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/kr/it/pullit/modules/projection/learnstats/domain/LearnStats.java (1)
63-68: Add test coverage for the breaking change in calculateLearningProgress().Verification confirms the semantic breaking change: the calculation numerator changed from
totalSolvedQuestionCount(attempted questions) tototalCorrectQuestionCount(correct questions). This is a significant business logic change that measures a different metric:
- Before: Progress = (attempted / total) × 100 — coverage metric
- After: Progress = (correct / total) × 100 — accuracy/mastery metric
Critical issues:
- No test coverage exists for
calculateLearningProgress()(noLearnStatsTest.javafile found)- Method definition exists but has no test assertions validating the new behavior
- Change lacks regression protection
Required actions:
- Add unit tests for
calculateLearningProgress()validating the accuracy calculation- Verify all consumers of this method (if/when integrated) are aware of the semantic change
- Document the business intent for this metric change
🧹 Nitpick comments (1)
src/test/java/kr/it/pullit/modules/questionset/client/GeminiClientTest.java (1)
63-69: Consider extracting stream mock setup to a helper method.The ResponseStream mock configuration is repeated across multiple tests. A helper method could reduce duplication and improve maintainability.
Example:
private ResponseStream<GenerateContentResponse> mockResponseStream(List<GenerateContentResponse> responses) { ResponseStream<GenerateContentResponse> mockStream = Mockito.mock(ResponseStream.class); when(mockStream.iterator()).thenReturn(responses.iterator()); when(mockStream.spliterator()).thenReturn(responses.spliterator()); return mockStream; }Then use it like:
ResponseStream<GenerateContentResponse> mockResponseStream = mockResponseStream(responses); when(models.generateContentStream(...)).thenReturn(mockResponseStream);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.gitignore(1 hunks)compose.yaml(6 hunks)nginx/certs/README.md(1 hunks)nginx/conf.d/default.conf(1 hunks)src/main/java/kr/it/pullit/modules/learningsource/source/service/SourceService.java(1 hunks)src/main/java/kr/it/pullit/modules/projection/learnstats/domain/LearnStats.java(1 hunks)src/main/java/kr/it/pullit/modules/projection/outbox/domain/OutboxEvent.java(1 hunks)src/main/java/kr/it/pullit/modules/questionset/client/GeminiClient.java(3 hunks)src/main/java/kr/it/pullit/modules/questionset/repository/QuestionSetRepository.java(1 hunks)src/main/java/kr/it/pullit/modules/questionset/repository/QuestionSetRepositoryImpl.java(1 hunks)src/main/java/kr/it/pullit/modules/questionset/repository/adapter/jpa/QuestionSetJpaRepository.java(1 hunks)src/main/java/kr/it/pullit/modules/questionset/service/QuestionService.java(1 hunks)src/main/java/kr/it/pullit/modules/questionset/service/QuestionSetService.java(3 hunks)src/main/java/kr/it/pullit/modules/questionset/web/QuestionSetController.java(2 hunks)src/main/java/kr/it/pullit/platform/security/config/SecurityConfig.java(1 hunks)src/main/java/kr/it/pullit/platform/web/cookie/CookieManager.java(1 hunks)src/main/java/kr/it/pullit/shared/error/CommonErrorCode.java(1 hunks)src/main/java/kr/it/pullit/shared/idempotency/DuplicateRequestException.java(1 hunks)src/main/java/kr/it/pullit/shared/idempotency/IdempotencyAspect.java(1 hunks)src/main/java/kr/it/pullit/shared/idempotency/IdempotencyExceptionHandler.java(1 hunks)src/main/java/kr/it/pullit/shared/idempotency/Idempotent.java(1 hunks)src/main/resources/application.yml(2 hunks)src/test/java/kr/it/pullit/modules/auth/repository/RefreshTokenRepositoryTest.java(1 hunks)src/test/java/kr/it/pullit/modules/questionset/client/GeminiClientTest.java(5 hunks)src/test/java/kr/it/pullit/modules/questionset/service/QuestionSetServiceTest.java(3 hunks)src/test/java/kr/it/pullit/platform/web/cookie/CookieManagerTest.java(1 hunks)src/test/resources/application-test.yml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/test/java/kr/it/pullit/modules/questionset/service/QuestionSetServiceTest.java (3)
src/test/java/kr/it/pullit/platform/web/cookie/CookieManagerTest.java (1)
MockitoUnitTest(20-212)src/test/java/kr/it/pullit/platform/security/jwt/filter/DevAuthenticationFilterTest.java (1)
MockitoUnitTest(19-47)src/test/java/kr/it/pullit/platform/security/jwt/filter/JwtAuthenticationFilterTest.java (1)
MockitoUnitTest(28-105)
src/main/java/kr/it/pullit/platform/security/config/SecurityConfig.java (2)
src/main/java/kr/it/pullit/platform/security/jwt/filter/DevAuthenticationFilter.java (1)
Profile(15-33)src/main/java/kr/it/pullit/platform/security/jwt/handler/LocalAuthenticationHandler.java (1)
Profile(12-47)
src/main/java/kr/it/pullit/modules/questionset/client/GeminiClient.java (2)
src/main/java/kr/it/pullit/modules/questionset/client/exception/LlmResponseParseException.java (1)
LlmResponseParseException(5-14)src/main/java/kr/it/pullit/modules/questionset/client/exception/LlmException.java (1)
LlmException(5-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: code_check
🔇 Additional comments (28)
src/main/java/kr/it/pullit/modules/projection/outbox/domain/OutboxEvent.java (1)
51-53: Verify the rationale and migration strategy for the column type change.Changing from
jsontolongtexttrades database-level features for broader compatibility:
- Lost: MySQL's automatic JSON validation, JSON-specific query functions, and potential storage optimizations
- Gained: Portability across MySQL versions (pre-5.7.8) and other database systems
Please confirm:
- Is this change driven by database compatibility requirements in the Docker environment?
- Is there a migration script to handle existing data if the
outbox_eventtable already contains records?- Does the application handle JSON validation elsewhere to compensate for the loss of database-level validation?
src/main/java/kr/it/pullit/platform/security/config/SecurityConfig.java (1)
30-30: Now I need to check if other beans depend on these security components in the worker context:I need to check the SecurityConfig file itself to understand the complete picture:
Based on my verification of Spring Framework's @Profile behavior and codebase analysis, I can now provide the rewritten review comment.
The
@Profile("!worker")annotation correctly prevents SecurityConfig from loading in worker profile.When a @configuration class is marked with @Profile, all of the @bean methods and @import annotations associated with that class will be bypassed unless one or more of the specified profiles are active. This means SecurityConfig's security filter chain definitions won't be created in worker profile, fixing the infinite restart issue.
However, the security-related beans (CustomOAuth2UserService, OAuth2AuthenticationSuccessHandler, JwtAuthenticationFilter, JwtAuthenticationEntryPoint, OAuth2AuthorizationRequestRepository) are independent
@Component/@Serviceclasses without@Profileannotations. When a bean is excluded due to profile conditions, its dependencies are also ignored, unless they are required by another active bean. These beans will still be registered but remain unused in worker profile.Consider applying
@Profile("!worker")to these security component beans to reduce unnecessary bean initialization in worker profile:
- CustomOAuth2UserService
- OAuth2AuthenticationSuccessHandler
- JwtAuthenticationFilter
- JwtAuthenticationEntryPoint
- OAuth2AuthorizationRequestRepository
This is a minor optimization—the current implementation is functionally correct and addresses the reported issue.
.gitignore (2)
48-48: LGTM - Fixed htmlReport entry.The removal of the leading dash corrects the ignore pattern.
50-52: Critical: Typo in directory path will fail to ignore SSL certificates.The path uses "ngnix" but should be "nginx" to match the actual directory structure referenced in compose.yaml (line 10:
./nginx/certs) and the README.md. This typo means private key files will NOT be ignored and could be committed to the repository, defeating the security objective stated in the PR description.Apply this diff:
# 로컬 개발용 SSL 인증서 개인 키 제외 -nginx/certs/*.key -nginx/certs/*.crt +nginx/certs/*.key +nginx/certs/*.crtLikely an incorrect or invalid review comment.
compose.yaml (4)
2-14: LGTM - Nginx service properly configured.The nginx service is correctly set up as an SSL termination proxy with appropriate volume mounts, port exposure, and service dependencies.
20-20: LGTM - Port exposure correctly removed.The API service no longer exposes ports directly since nginx now serves as the entry point on port 443.
91-95: MariaDB healthcheck properly configured.The healthcheck correctly validates database connectivity with appropriate intervals and retries. Note that the password is visible in the command, but this is acceptable for local development where the password is already defined in environment variables.
137-137: LGTM - Healthcheck improvements.The changes improve healthcheck accuracy and efficiency:
- RabbitMQ now uses
check_runninginstead ofping, which is more appropriate for validating service readiness- Reduced timeouts optimize startup time without compromising reliability
Also applies to: 149-151
nginx/conf.d/default.conf (3)
1-11: LGTM - SSL configuration appropriate for local development.The SSL setup uses modern protocols (TLSv1.2/1.3) and secure cipher suites. Certificate paths correctly match the volume mounts defined in compose.yaml.
12-28: SSE configuration correctly disables buffering and caching.The configuration properly supports Server-Sent Events with:
- Buffering and caching disabled for immediate event delivery
- HTTP/1.1 with empty Connection header to maintain keep-alive
- 1-hour read timeout for long-lived connections
Note: The
keepalive_timeout 7200s(2 hours) is quite long. Verify this aligns with your SSE connection lifetime requirements.
30-38: LGTM - Default proxy configuration with WebSocket support.The configuration correctly proxies requests to the backend service with appropriate headers for protocol upgrades (WebSocket) and client information forwarding.
nginx/certs/README.md (1)
1-52: LGTM - Comprehensive SSL setup documentation.The README provides clear, step-by-step instructions for SSL certificate generation across multiple platforms (Linux, macOS, Windows). The security note about Git exclusion of private keys and the file renaming instructions are particularly helpful for developers setting up their environment.
src/main/resources/application.yml (2)
24-26: LGTM! Proper schema management configuration.Disabling Hibernate DDL auto-generation in favor of schema.sql provides better control over database schema changes and is appropriate for production-like environments.
110-110: LGTM! Fixes OAuth2 redirect URI scheme issue.Setting
forward-headers-strategy: frameworkenables proper handling of X-Forwarded-* headers from Nginx, ensuring that OAuth2 redirect URIs are generated with the correct https scheme instead of http.src/main/java/kr/it/pullit/platform/web/cookie/CookieManager.java (1)
49-49: LGTM! Proper cookie expiration path handling.The
determinePathForCookiehelper ensures that cookies are expired with the same path they were created with, which is essential for proper cookie deletion. The refresh token cookie uses its specific path (/auth/refresh), while other cookies default to/.Also applies to: 63-68
src/main/java/kr/it/pullit/modules/questionset/service/QuestionService.java (1)
140-156: LGTM! Simplified LLM request construction.The refactoring to use
DEFAULT_MODEL_NAMEconstant reduces parameter passing and simplifies the code. The model name is now managed in a single location.src/test/resources/application-test.yml (1)
2-4: LGTM! Appropriate test environment schema management.Using
ddl-auto: create-dropfor tests ensures a clean schema for each test run, providing proper test isolation. This correctly contrasts with the production configuration that uses schema.sql.src/main/java/kr/it/pullit/modules/questionset/repository/QuestionSetRepository.java (1)
14-14: LGTM! Efficient eager loading method.The
findByIdWithQuestionsmethod provides a clear API for eagerly loading a QuestionSet with its questions, avoiding N+1 query issues. The method name clearly communicates its intent.src/test/java/kr/it/pullit/platform/web/cookie/CookieManagerTest.java (1)
118-135: LGTM! Comprehensive test coverage for cookie path handling.The test verifies that refresh_token cookies are expired with the correct path (
REFRESH_TOKEN_COOKIE_PATH), ensuring proper cookie deletion behavior. This provides good coverage for the CookieManager changes.src/main/java/kr/it/pullit/modules/questionset/web/QuestionSetController.java (1)
14-14: LGTM! Good addition of idempotency support.Adding the
@Idempotentannotation to thecreateQuestionSetendpoint prevents duplicate submissions, which is essential for reliable operation in distributed systems or with unreliable networks.Also applies to: 87-87
src/main/java/kr/it/pullit/modules/questionset/repository/QuestionSetRepositoryImpl.java (1)
25-28: LGTM!The delegation to the JPA repository is clean and follows the established pattern in this class.
src/test/java/kr/it/pullit/modules/auth/repository/RefreshTokenRepositoryTest.java (1)
11-14: Good test isolation improvement.The Redis host override ensures tests can run reliably in local environments without Docker dependencies.
src/main/java/kr/it/pullit/modules/questionset/repository/adapter/jpa/QuestionSetJpaRepository.java (1)
16-24: LGTM!The query correctly uses LEFT JOIN FETCH to eagerly load questions and properly filters soft-deleted records. This prevents N+1 query issues when accessing questions.
src/main/java/kr/it/pullit/shared/idempotency/IdempotencyExceptionHandler.java (1)
9-16: LGTM!HTTP 409 CONFLICT is the appropriate status code for duplicate requests, and the response structure is clear and consistent.
src/main/java/kr/it/pullit/shared/idempotency/Idempotent.java (1)
10-18: LGTM!The annotation design is clean with sensible defaults. The 10-minute TTL for idempotency keys provides a good balance between safety and resource usage.
src/main/java/kr/it/pullit/shared/idempotency/DuplicateRequestException.java (1)
6-14: LGTM!The exception follows the established pattern in the codebase and provides both parameterized and default constructor options.
src/test/java/kr/it/pullit/modules/questionset/client/GeminiClientTest.java (1)
57-78: LGTM!The test correctly validates the streaming API with chunked responses and properly aggregates them into the final DTO.
src/main/java/kr/it/pullit/modules/questionset/service/QuestionSetService.java (1)
302-314: Excellent optimization to prevent N+1 queries.Switching to
findByIdWithQuestionseagerly loads the questions collection, which is necessary for the soft delete operation on line 313. This prevents lazy loading exceptions and eliminates multiple round trips to the database.
1 participant
PR 설명
로컬 개발 환경 Docker Compose로 전환 및 전체 버그 수정
OAuth2 로그인 시 redirect_uri가 http로 생성되는 문제 해결
Nginx 리버스 프록시 환경에서 SSE 실시간 알림이 동작하지 않는 문제 수정
worker 컨테이너가 웹 보안 설정 충돌로 무한 재시작하는 버그 수정
테스트 환경에서 ddl-auto 및 Redis 설정 오류로 통합 테스트가 실패하는 문제 해결
Git에 포함된 개인 키 파일을 제거하고, .gitignore 및 인증서 생성 가이드(README) 추가
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores