MOSU-287 refactor: 내 질문 조회 시 답변도 같이 전달 #290
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. WalkthroughThis change updates the inquiry listing flow to return both inquiries and their associated answers. It introduces new DTOs ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant Repository
participant DB
Client->>Controller: GET /my-inquiries
Controller->>Service: getMyInquiry(userId, pageable)
Service->>Repository: searchMyInquiry(userId, pageable)
Repository->>DB: Query inquiries LEFT JOIN answers
DB-->>Repository: Inquiry + Answer rows
Repository->>Service: Page<InquiryListResponse>
Service->>Controller: Page<InquiryListResponse>
Controller->>Client: ApiResponseWrapper<Page<InquiryListResponse>>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @jbh010204, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've refactored the 'my inquiries' feature to now include the corresponding answers when a user views their questions. This change enhances the user experience by providing a more complete view of their past interactions without needing to fetch answers separately.
Highlights
- Enhanced Inquiry Listing: The
getMyInquirymethod across the service, repository, and controller layers now returns a newInquiryListResponseDTO, which encapsulates both the inquiry details and its associated answer. - New DTOs Introduced: Two new DTOs,
InquiryAnswerResponseandInquiryListResponse, have been added to structure the combined inquiry and answer data. - Query Optimization: A new
baseQueryWithAnswermethod has been implemented inInquiryJpaRepositoryImplto efficiently fetch inquiry and answer data using a left join, ensuring answers are included when available.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the inquiry retrieval functionality to include answers along with the questions, which is a valuable enhancement. The implementation is solid, using a LEFT JOIN to fetch optional answers and introducing new DTOs for a structured response. I particularly appreciate the decoupling of DTOs from JPA entities, which improves the application's architecture. My review includes a few suggestions for improving code consistency and removing redundant code.
| .offset(pageable.getOffset()) | ||
| .limit(pageable.getPageSize()); | ||
| JPAQuery<Tuple> query = baseQuery() | ||
| .where(buildStatusCondition(inquiry, status)); |
There was a problem hiding this comment.
The baseQuery method was refactored to use the class field inquiry instead of taking it as a parameter, which is a good change. For consistency, the buildStatusCondition method should also be updated to not take the QInquiryJpaEntity inquiry parameter and instead use the class field. This would align it with the implementation of buildOrderByCondition and improve code clarity.
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryListResponse.java
Show resolved
Hide resolved
![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
🧹 Nitpick comments (7)
src/main/java/life/mosu/mosuserver/domain/inquiry/repository/InquiryQueryRepository.java (1)
14-15: Domain repository returning presentation DTOs couples layers; consider decoupling.The domain repository interface now returns
Page<InquiryListResponse>(presentation DTO). This continues the cross-layer dependency that was already present withInquiryResponse.Consider:
- Returning a domain/query projection (interface or record) from the repository.
- Mapping to presentation DTOs in the service/presentation layer.
- Alternatively, keep projections in infra (JpaRepositoryImpl) and map there.
This reduces coupling and eases DTO evolution without repository interface churn.
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryControllerDocs.java (1)
35-43: Update summary/response docs to reflect answers included in the list.The endpoint now returns inquiries with answers. Consider clarifying in the summary/description and using a more specific schema example for
InquiryListResponse.Example:
- Update summary to: “내 문의글 및 답변 조회”
- Provide an example schema for
InquiryListResponseif you maintain example payloads.src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryListResponse.java (2)
3-14: Align field naming with DTO type (reply→answer) for consistency.
InquiryAnswerResponseis exposed asreply. Renaming improves clarity and consistency across layers.package life.mosu.mosuserver.presentation.inquiry.dto; -public record InquiryListResponse( - InquiryResponse inquiry, - InquiryAnswerResponse reply -) { +public record InquiryListResponse( + InquiryResponse inquiry, + InquiryAnswerResponse answer +) { - public static InquiryListResponse of( - InquiryResponse inquiry, - InquiryAnswerResponse reply - ) { - return new InquiryListResponse(inquiry, reply); + public static InquiryListResponse of( + InquiryResponse inquiry, + InquiryAnswerResponse answer + ) { + return new InquiryListResponse(inquiry, answer); } }
1-14: Add Swagger annotations to document the composite response.Improve API docs by annotating the record and its fields.
+import io.swagger.v3.oas.annotations.media.Schema; + - public record InquiryListResponse( - InquiryResponse inquiry, - InquiryAnswerResponse reply - ) { +@Schema(description = "내 문의글 목록 항목 (문의 + 답변)") +public record InquiryListResponse( + @Schema(description = "문의 정보") + InquiryResponse inquiry, + @Schema(description = "문의 답변 정보 (없을 수 있음)", nullable = true) + InquiryAnswerResponse reply +) {If you adopt the previous rename to
answer, update the field name here as well.src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerResponse.java (2)
3-9: Consider using temporal types instead of Strings for timestampsRepresent createdAt/updatedAt as LocalDateTime/Instant and let Jackson format them (e.g., via @jsonformat or global ObjectMapper). This preserves ordering/timezone semantics and avoids pre-formatting in the repository layer.
11-19: Remove redundant factory method or use it consistentlyThe of(...) method adds no behavior over the canonical record constructor and isn’t used in mappings. Prefer removing it to keep API minimal, or consistently use it everywhere for symmetry.
Apply this diff if you choose to remove it:
- public static InquiryAnswerResponse of( - String title, - String content, - String author, - String createdAt, - String updatedAt - ) { - return new InquiryAnswerResponse(title, content, author, createdAt, updatedAt); - }src/main/java/life/mosu/mosuserver/infra/persistence/jpa/InquiryJpaRepositoryImpl.java (1)
94-113: Join and projection look correct; ensure uniqueness constraints if 1:1Projection includes only needed fields. Left join on inquiryAnswer.inquiryId = inquiry.id is fine. If 1:1 is not enforced, consider redesign as noted earlier to prevent duplicates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/inquiry/repository/InquiryQueryRepository.java(2 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/jpa/InquiryJpaRepositoryImpl.java(6 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java(3 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryControllerDocs.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryListResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryResponse.java(1 hunks)
🔇 Additional comments (11)
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryResponse.java (1)
26-27: No lingering usages ofInquiryResponse.of(...)found
A thorough search – including invocation patterns and static imports – returned no references to the removed factory method. The removal is safe.src/main/java/life/mosu/mosuserver/domain/inquiry/repository/InquiryQueryRepository.java (1)
4-4: Import addition aligns with signature change.The added import is consistent with the updated return type for
searchMyInquiry. Ensure controller and service are aligned (they appear to be).src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java (2)
60-63: No N+1 risk insearchMyInquiry– one query with left joinThe
searchMyInquiryimplementation inInquiryJpaRepositoryImplusesbaseQueryWithAnswer(), which performs a single QueryDSL.select(…)over bothInquiryandInquiryAnswerfields with a.leftJoin(inquiryAnswer).on(inquiryAnswer.inquiryId.eq(inquiry.id)). The result tuples are then mapped toInquiryListResponsein one pass, so there is no N+1 query issue here. No further refactor is required.
60-63: Return types are consistent across service, controller, and docs
All call sites usePage<InquiryListResponse>:
- InquiryService.getMyInquiry(...) returns
Page<InquiryListResponse>.- InquiryController.getMyInquiries(...) wraps that in
ResponseEntity<ApiResponseWrapper<Page<InquiryListResponse>>>.- InquiryControllerDocs defines the same return type.
No further changes or versioning are needed.
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java (2)
10-10: Import update aligns with new response modelSwitch to InquiryListResponse is correct for enriched responses.
72-73: No functional changes hereLine breaks only; logic remains the same.
src/main/java/life/mosu/mosuserver/infra/persistence/jpa/InquiryJpaRepositoryImpl.java (5)
18-21: Imports align with new join and mapping
Looks correct.
35-35: Answer Q-type initialization looks good
Consistent with other Q fields.
45-56: Refactor improves readability and reuseUsing baseQuery() and a dedicated mapper is clean. Status filtering retained; pagination unchanged.
80-93: Base select is minimal and focused
Good separation for reuse and counting.
151-166: No NPE risk:formatDateis already null-safe
BaseTimeEntity.formatDate(LocalDateTime)guards against null and returns null if the input is null, so calling it ontuple.get(inquiryAnswer.updatedAt)cannot throw an NPE. You can safely keep the existing code unchanged.Likely an incorrect or invalid review comment.
| @Override | ||
| public Page<InquiryResponse> searchMyInquiry(Long userId, Pageable pageable) { | ||
| JPAQuery<Tuple> query = baseQuery(inquiry) | ||
| public Page<InquiryListResponse> searchMyInquiry(Long userId, Pageable pageable) { | ||
| JPAQuery<Tuple> query = baseQueryWithAnswer() | ||
| .where(inquiry.userId.eq(userId)); | ||
|
|
||
| long total = getTotalCount(query, inquiry.count()); | ||
| long total = getTotalCount(query, inquiry.countDistinct()); | ||
|
|
||
| List<InquiryResponse> content = query | ||
| List<InquiryListResponse> content = query | ||
| .orderBy(inquiry.createdAt.desc()) | ||
| .offset(pageable.getOffset()) | ||
| .limit(pageable.getPageSize()) | ||
| .fetch().stream() | ||
| .map(this::mapToResponse) | ||
| .map(this::mapToInquiryListResponse) | ||
| .toList(); | ||
|
|
||
| return new PageImpl<>(content, pageable, total); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use id-based distinct count and validate 1:1 assumption to avoid paging errors
- Prefer countDistinct on the id expression to avoid dialect-specific issues:
- long total = getTotalCount(query, inquiry.countDistinct());
+ long total = getTotalCount(query, inquiry.id.countDistinct());- If an inquiry can have multiple answers (1:N), the current content query will duplicate inquiries within a page, causing missing unique items and inconsistent paging. If the domain guarantees 1:1 (Inquiry↔Answer), please confirm this is enforced at the JPA mapping and DB level (unique constraint/index). Otherwise, consider a 2-step fetch (page inquiries, then batch-load answers by IN clause) to guarantee uniqueness without relying on joins.
Script to verify relationship and constraints, and confirm service/docs alignment:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect InquiryAnswer entity for 1:1 mapping or uniqueness
fd -a "InquiryAnswerJpaEntity.java" | xargs -r -I{} rg -n --no-heading -A3 -B3 "@OneToOne|@ManyToOne|unique|UniqueConstraint|inquiryId|inquiry" {}
# Check BaseTimeEntity.formatDate implementation for null-safety
fd -a "BaseTimeEntity.java" | xargs -r -I{} rg -n --no-heading -A2 -B2 "static .*formatDate|DateTimeFormatter|null" {}
# Confirm service method return type
rg -n "interface\\s+InquiryQueryRepository|class\\s+InquiryService" -A 50 -S | rg -n "searchMyInquiry\\(|getMyInquiry\\(" -n -SLength of output: 2736
Use id-based distinct count and prevent duplicate paging issues
In InquiryJpaRepositoryImpl.searchMyInquiry, switch from inquiry.countDistinct() to inquiry.id.countDistinct() so that the total count reflects unique inquiry IDs and avoids dialect-specific quirks:
- long total = getTotalCount(query, inquiry.countDistinct());
+ long total = getTotalCount(query, inquiry.id.countDistinct());Currently, InquiryAnswerJpaEntity defines inquiryId as a simple @Column (no @OneToOne or unique constraint), so an inquiry with multiple answers will produce duplicate rows in content (and incorrect page sizes). To address this, choose one of the following:
- Enforce 1:1: Add a unique constraint on
inquiry_idin theinquiry_answertable (via JPA@Table(uniqueConstraints=…)or a DB migration) and map it with@OneToOnein the entity. - Use a two-step fetch:
- Query only distinct inquiry IDs (paged).
- Batch-load answers for those IDs and map to
InquiryListResponse.
Either approach will ensure unique items per page and consistent paging.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/infra/persistence/jpa/InquiryJpaRepositoryImpl.java
around lines 61 to 77, the total count query uses inquiry.countDistinct(), which
can cause duplicate counts due to multiple answers per inquiry. Change this to
use inquiry.id.countDistinct() to count unique inquiry IDs. Additionally, to
prevent duplicate rows in the paged content caused by multiple answers per
inquiry, either enforce a one-to-one relationship by adding a unique constraint
on inquiry_id in the inquiry_answer table and mapping it with @OneToOne, or
implement a two-step fetch: first query distinct inquiry IDs with pagination,
then batch-load their answers and map to InquiryListResponse. This will ensure
unique items per page and correct paging behavior.
| public ResponseEntity<ApiResponseWrapper<Page<InquiryListResponse>>> getMyInquiries( | ||
| @UserId Long userId, | ||
| @PageableDefault(size = 10) Pageable pageable | ||
| ) { | ||
| Page<InquiryResponse> inquiries = inquiryService.getMyInquiry(userId, pageable); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "내 질문 목록 조회 성공", inquiries)); | ||
| Page<InquiryListResponse> inquiries = inquiryService.getMyInquiry(userId, pageable); | ||
| return ResponseEntity.ok( | ||
| ApiResponseWrapper.success(HttpStatus.OK, "내 질문 목록 조회 성공", inquiries)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Return type change looks good; ensure upstream/downstream alignment
The switch to Page matches the PR goal. Minor consistency nit: other endpoints use PrincipalDetails.user() while this one uses @userid Long — consider standardizing.
Run to verify signatures/docs are updated:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Service method signature
rg -n "getMyInquiry\\s*\\(" -A 5 -S
# Docs interface return type alignment
fd -a "InquiryControllerDocs.java" | xargs -r rg -n "getMyInquiries|InquiryListResponse|Page<"
# DTO usage across layers
rg -n "InquiryListResponse" -SLength of output: 4965
Return type change verified across layers
The switch to Page<InquiryListResponse> is applied consistently in:
- Controller (src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java)
- Service (src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java)
- Repository (src/main/java/life/mosu/mosuserver/infra/persistence/jpa/InquiryJpaRepositoryImpl.java)
- API docs (src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryControllerDocs.java)
Minor consistency nit: most controllers inject the current user via PrincipalDetails user, whereas getMyInquiries uses @UserId Long userId. Consider standardizing on one approach for clarity and uniformity.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java
around lines 57 to 64, the method getMyInquiries currently injects the user
identifier using @UserId Long userId, while most other controllers use
PrincipalDetails user for the current user injection. To maintain consistency
and clarity across the codebase, refactor getMyInquiries to accept
PrincipalDetails user as a parameter instead of @UserId Long userId, then
extract the user ID from PrincipalDetails within the method before calling the
service layer.
| ResponseEntity<ApiResponseWrapper<Page<InquiryListResponse>>> getMyInquiries( | ||
| @Parameter(description = "사용자 ID", required = true) Long userId, | ||
| @Parameter(description = "페이지 정보") Pageable pageable | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security: don’t accept userId from clients for “my” resources; use PrincipalDetails.
“My inquiries” should derive the user id from the authenticated principal, not a request parameter. Accepting userId enables IDOR.
Proposed docs interface update:
- ResponseEntity<ApiResponseWrapper<Page<InquiryListResponse>>> getMyInquiries(
- @Parameter(description = "사용자 ID", required = true) Long userId,
- @Parameter(description = "페이지 정보") Pageable pageable
- );
+ ResponseEntity<ApiResponseWrapper<Page<InquiryListResponse>>> getMyInquiries(
+ @Parameter(hidden = true) PrincipalDetails principalDetails,
+ @Parameter(description = "페이지 정보") Pageable pageable
+ );Ensure the controller implementation uses principalDetails.getUser().getId() (or equivalent) and ignores any user id from client input.
📝 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.
| ResponseEntity<ApiResponseWrapper<Page<InquiryListResponse>>> getMyInquiries( | |
| @Parameter(description = "사용자 ID", required = true) Long userId, | |
| @Parameter(description = "페이지 정보") Pageable pageable | |
| ); | |
| ResponseEntity<ApiResponseWrapper<Page<InquiryListResponse>>> getMyInquiries( | |
| @Parameter(hidden = true) PrincipalDetails principalDetails, | |
| @Parameter(description = "페이지 정보") Pageable pageable | |
| ); |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryControllerDocs.java
around lines 40 to 43, the method getMyInquiries currently accepts a userId
parameter from the client, which poses a security risk. Remove the userId
parameter and instead update the method signature to accept the authenticated
user's details via PrincipalDetails or a similar security principal object.
Ensure the controller implementation retrieves the user ID from the
authenticated principal rather than from client input to prevent IDOR
vulnerabilities.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor