Conversation
WalkthroughThis change adds support for retrieving detailed information about an article by its ID, including a new reminder time field. It introduces new DTOs for detailed responses, a new exception for not-found cases, and extends both the domain entity and service layers. Tests and fixtures are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ArticleController
participant ArticleManagementUsecase
participant ArticleGetService
participant ArticleRepository
Client->>ArticleController: GET /articles/details/{articleId}
ArticleController->>ArticleManagementUsecase: getArticleDetail(articleId)
ArticleManagementUsecase->>ArticleGetService: findById(articleId)
ArticleGetService->>ArticleRepository: findById(articleId)
ArticleRepository-->>ArticleGetService: Article or not found
ArticleGetService-->>ArticleManagementUsecase: Article or throw ArticleNotFoundException
ArticleManagementUsecase-->>ArticleController: ArticleDetailResponse
ArticleController-->>Client: ResponseDto<ArticleDetailResponse>
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java (1)
32-37: Consider using primitivelongfor consistency.The implementation looks good and follows the established controller pattern. However, consider using
longinstead ofLongfor the path variable to maintain consistency with the service layer and avoid unnecessary autoboxing.- public ResponseDto<ArticleDetailResponse> getArticleDetails(@PathVariable Long articleId) { + public ResponseDto<ArticleDetailResponse> getArticleDetails(@PathVariable long articleId) {src/test/java/com/pinback/pinback_server/domain/fixture/TestFixture.java (1)
26-26: Consider using relative dates for better test maintainability.The fixture updates properly support the new
remindAtfield in article creation. However, using a hardcoded future date (2025-07-07) might cause issues in tests that depend on relative time comparisons or when run in different years.Consider using a relative date approach:
- return Article.create("test", "testmemo", user, category, LocalDateTime.of(2025, 7, 7, 12, 0, 0)); + return Article.create("test", "testmemo", user, category, LocalDateTime.now().plusDays(1));This would make the test data more maintainable and prevent potential issues with time-based assertions.
Also applies to: 30-30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java(2 hunks)src/main/java/com/pinback/pinback_server/domain/article/domain/entity/Article.java(2 hunks)src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleGetService.java(2 hunks)src/main/java/com/pinback/pinback_server/domain/article/exception/ArticleNotFoundException.java(1 hunks)src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java(3 hunks)src/main/java/com/pinback/pinback_server/domain/article/presentation/dto/request/ArticleCreateRequest.java(1 hunks)src/main/java/com/pinback/pinback_server/domain/article/presentation/dto/response/ArticleDetailResponse.java(1 hunks)src/main/java/com/pinback/pinback_server/domain/category/presentation/dto/response/CategoryDetail.java(1 hunks)src/main/java/com/pinback/pinback_server/global/exception/constant/ExceptionCode.java(1 hunks)src/test/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecaseTest.java(3 hunks)src/test/java/com/pinback/pinback_server/domain/fixture/TestFixture.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleGetService.java (1)
src/main/java/com/pinback/pinback_server/domain/article/exception/ArticleNotFoundException.java (1)
ArticleNotFoundException(6-10)
⏰ 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: build
🔇 Additional comments (14)
src/main/java/com/pinback/pinback_server/domain/article/presentation/dto/request/ArticleCreateRequest.java (1)
16-16: Good change: Making remindTime optional aligns with business logic.Removing the
@NotNullvalidation annotation makes the reminder functionality optional, which is a logical design decision since not all articles need reminders.src/main/java/com/pinback/pinback_server/global/exception/constant/ExceptionCode.java (1)
22-22: LGTM: Exception code follows established patterns.The new
ARTICLE_NOT_FOUNDexception code correctly uses HTTP 404 status and follows the consistent error code naming convention.src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleGetService.java (1)
23-25: Clean implementation of article retrieval with proper exception handling.The
findByIdmethod correctly usesOptional.orElseThrowwith a method reference to handle the case when an article is not found, providing clear error handling.src/main/java/com/pinback/pinback_server/domain/article/domain/entity/Article.java (2)
54-55: Proper JPA mapping for the new remindAt field.The
remindAtfield is correctly mapped to the database columnremind_atand allows null values, which is consistent with the optional nature of reminders.
60-60: Factory method properly updated to handle the new field.The
createmethod signature and builder pattern are correctly updated to include theremindAtparameter, maintaining consistency in the entity creation process.Also applies to: 67-67
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java (1)
34-34: Correctly integrated remindTime parameter.The
createArticlemethod properly passes theremindTimefrom the command to theArticle.createmethod, completing the integration of the reminder functionality.src/main/java/com/pinback/pinback_server/domain/article/exception/ArticleNotFoundException.java (1)
1-10: LGTM! Clean exception implementation.The exception class follows the established pattern in the codebase, properly extending
ApplicationExceptionand using the predefined error code. The implementation is concise and appropriate for its purpose.src/main/java/com/pinback/pinback_server/domain/category/presentation/dto/response/CategoryDetail.java (1)
5-15: LGTM! Well-structured DTO record.The record class properly encapsulates category information with appropriate field types. The static factory method
of()follows the established pattern for creating DTOs from entities and correctly maps the entity fields.src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java (1)
21-21: LGTM! Improved RESTful endpoint naming.Changing from
/articleto/articlesfollows RESTful conventions using plural form for resource endpoints.src/main/java/com/pinback/pinback_server/domain/article/presentation/dto/response/ArticleDetailResponse.java (1)
8-23: LGTM! Well-designed response DTO.The record structure appropriately captures all necessary article details. The static factory method
from()correctly maps entity fields to DTO fields and properly handles the category conversion usingCategoryDetail.of(). The use ofLocalDateTimeforremindAtis appropriate for timestamp representation.src/test/java/com/pinback/pinback_server/domain/fixture/TestFixture.java (1)
3-3: LGTM! Proper import addition.The
LocalDateTimeimport is correctly added to support the new timestamp field in article creation.src/test/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecaseTest.java (3)
20-20: LGTM!The import addition for
ArticleDetailResponseis necessary for the new test method.
56-56: LGTM!The assertion correctly validates that the
remindAtfield is properly set from the command'sremindTimeparameter, ensuring the new reminder functionality works as expected.
74-93: Comprehensive test coverage for article detail retrieval.The test effectively validates all aspects of the article detail retrieval functionality:
- Tests the complete flow from setup to response validation
- Verifies all response fields (url, memo, category details, remindAt)
- Follows the AAA (Arrange-Act-Assert) pattern correctly
- Properly integrates with the existing test fixture methods
The test logic and assertions are well-structured and comprehensive.
| public ArticleDetailResponse getArticleDetail(long articleId) { | ||
| Article article = articleGetService.findById(articleId); | ||
| return ArticleDetailResponse.from(article); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding authorization checks for article access.
The getArticleDetail method correctly retrieves and transforms article data. However, there's no authorization check to ensure users can only access their own articles.
Consider adding a user parameter and verifying article ownership:
-public ArticleDetailResponse getArticleDetail(long articleId) {
+public ArticleDetailResponse getArticleDetail(long articleId, User user) {
Article article = articleGetService.findById(articleId);
+ if (!article.getUser().equals(user)) {
+ throw new UnauthorizedAccessException();
+ }
return ArticleDetailResponse.from(article);
}Run the following script to check if similar authorization patterns exist in the codebase:
🏁 Script executed:
#!/bin/bash
# Check for authorization patterns in other usecase methods
rg -A 5 -B 5 "getUser\(\)\.equals|\.getUser\(\)" --type javaLength of output: 62
I couldn’t find any existing ownership checks or security annotations with the initial pattern search. Let’s broaden the search to see if the codebase uses any custom exception, Spring Security hooks, or user‐fetching utilities:
🏁 Script executed:
#!/bin/bash
# 1. Check for custom unauthorized exception usage
rg -n "UnauthorizedAccessException" --type java
# 2. Search for Spring Security context usage
rg -n "SecurityContextHolder" --type java
# 3. Look for method‐level security annotations
rg -n "@PreAuthorize" --type java
# 4. Find utilities or services fetching the current user
rg -n "getCurrentUser" --type java
rg -n "Principal" --type javaLength of output: 1218
We still need to see how getArticleDetail is being called in your controllers so we can update both layers. Let’s locate all usages:
🏁 Script executed:
#!/bin/bash
# List every occurrence of getArticleDetail in the repo
rg -n "getArticleDetail" --type javaLength of output: 774
Let’s inspect how @CurrentUser is set up and how the controller method is defined so we can align our changes:
🏁 Script executed:
#!/bin/bash
# 1. Show the resolver implementation and annotation it looks for
sed -n '1,200p' src/main/java/com/pinback/pinback_server/global/config/resolver/CurrentUserArgumentResolver.java
# 2. View the controller method signature
sed -n '1,200p' src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java
# 3. Locate the CurrentUser annotation definition
rg -n "interface CurrentUser" --type javaLength of output: 3820
Enforce article‐ownership check in detail endpoint
The controller and use-case need to accept the authenticated user and verify ownership before returning article details:
• Update ArticleController#getArticleDetails to inject the current user:
@GetMapping("/details/{articleId}")
public ResponseDto<ArticleDetailResponse> getArticleDetails(
@PathVariable Long articleId,
@CurrentUser User user
) {
ArticleDetailResponse response = articleManagementUsecase.getArticleDetail(articleId, user);
return ResponseDto.ok(response);
}• Change ArticleManagementUsecase#getArticleDetail to accept User and throw an access‐denied exception if the user doesn’t own the article:
-public ArticleDetailResponse getArticleDetail(long articleId) {
- Article article = articleGetService.findById(articleId);
- return ArticleDetailResponse.from(article);
-}
+public ArticleDetailResponse getArticleDetail(long articleId, User user) {
+ Article article = articleGetService.findById(articleId);
+ if (!article.getUser().equals(user)) {
+ throw new org.springframework.security.access.AccessDeniedException(
+ "You do not have permission to view this article"
+ );
+ }
+ return ArticleDetailResponse.from(article);
+}• Add import in ArticleManagementUsecase:
import org.springframework.security.access.AccessDeniedException;These changes ensure only the owning user can retrieve article details.
🤖 Prompt for AI Agents
In
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java
around lines 38 to 41, modify the getArticleDetail method to accept a User
parameter representing the authenticated user. After retrieving the article by
ID, check if the article's owner matches the provided user; if not, throw an
AccessDeniedException. Also, add the import statement for
org.springframework.security.access.AccessDeniedException. This enforces
ownership verification before returning article details.
There was a problem hiding this comment.
아니 어차피 유저정보 안써서 손해야
There was a problem hiding this comment.
@rootTiket 네, 이해했습니다. 해당 프로젝트에서는 아티클 상세 조회에 유저 권한 확인이 필요하지 않다는 점 알겠습니다. 현재 구현이 요구사항에 맞다면 그대로 진행하시면 됩니다.
✏️ Learnings added
Learnt from: rootTiket
PR: Pinback-Team/pinback-server#18
File: src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java:38-41
Timestamp: 2025-07-07T15:33:02.697Z
Learning: In the Pinback server project, article detail retrieval (getArticleDetail method) does not require user authorization checks. Articles can be accessed by ID without ownership verification as per the project's design decision.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
...java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecaseTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecaseTest.java (1)
77-77: Improve test method naming.The test method name
test()is not descriptive and doesn't follow naming conventions. Consider a more descriptive name that explains what the test is verifying.- void test() { + void getArticleDetail() {src/test/java/com/pinback/pinback_server/domain/fixture/TestFixture.java (1)
26-26: Consider using a more flexible approach for reminder dates.The hard-coded date
LocalDateTime.of(2025, 7, 7, 12, 0, 0)works functionally but may impact test maintainability. Consider these improvements:
- Extract to a constant for better maintainability
- Use relative dates to avoid future date obsolescence
- Add parameterization for different test scenarios
Here's a suggested refactor to improve maintainability:
+import java.time.LocalDateTime; + public class TestFixture { + private static final LocalDateTime DEFAULT_REMIND_AT = LocalDateTime.of(2025, 7, 7, 12, 0, 0); + public static Article article(User user) { Category category = category(user); - return Article.create("test", "testmemo", user, category, LocalDateTime.of(2025, 7, 7, 12, 0, 0)); + return Article.create("test", "testmemo", user, category, DEFAULT_REMIND_AT); } public static Article articleWithCategory(User user, Category category) { - return Article.create("test", "testmemo", user, category, LocalDateTime.of(2025, 7, 7, 12, 0, 0)); + return Article.create("test", "testmemo", user, category, DEFAULT_REMIND_AT); } + + public static Article articleWithRemindAt(User user, Category category, LocalDateTime remindAt) { + return Article.create("test", "testmemo", user, category, remindAt); + } }This approach provides better maintainability while maintaining backward compatibility for existing tests.
Also applies to: 30-30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java(2 hunks)src/main/java/com/pinback/pinback_server/domain/article/domain/entity/Article.java(2 hunks)src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleGetService.java(2 hunks)src/main/java/com/pinback/pinback_server/domain/article/exception/ArticleNotFoundException.java(1 hunks)src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java(3 hunks)src/main/java/com/pinback/pinback_server/domain/article/presentation/dto/request/ArticleCreateRequest.java(1 hunks)src/main/java/com/pinback/pinback_server/domain/article/presentation/dto/response/ArticleDetailResponse.java(1 hunks)src/main/java/com/pinback/pinback_server/domain/category/presentation/dto/response/CategoryDetail.java(1 hunks)src/main/java/com/pinback/pinback_server/global/exception/constant/ExceptionCode.java(1 hunks)src/test/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecaseTest.java(3 hunks)src/test/java/com/pinback/pinback_server/domain/fixture/TestFixture.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleGetService.java (1)
src/main/java/com/pinback/pinback_server/domain/article/exception/ArticleNotFoundException.java (1)
ArticleNotFoundException(6-10)
🔇 Additional comments (19)
src/main/java/com/pinback/pinback_server/domain/category/presentation/dto/response/CategoryDetail.java (1)
5-15: LGTM! Clean DTO implementation with good factory pattern.The record class implementation is well-structured with appropriate field types and follows the static factory method pattern, which is a good practice for DTO creation from domain entities.
src/main/java/com/pinback/pinback_server/domain/article/presentation/dto/request/ArticleCreateRequest.java (1)
16-19: LGTM! Making remindTime optional is appropriate.Removing the
@NotNullannotation makes the reminder time field optional during validation, which aligns with the feature implementation where users may not always want to set a reminder.src/main/java/com/pinback/pinback_server/global/exception/constant/ExceptionCode.java (1)
22-22: LGTM! Exception code follows established patterns.The new
ARTICLE_NOT_FOUNDexception code is properly implemented with appropriate HTTP status (404), follows the sequential error code pattern (c40403), and has a clear Korean message consistent with other exception codes.src/main/java/com/pinback/pinback_server/domain/article/exception/ArticleNotFoundException.java (1)
6-10: LGTM! Standard exception implementation following application patterns.The exception class correctly extends
ApplicationExceptionand uses the appropriate exception code. The implementation follows the established pattern used throughout the application.src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleGetService.java (1)
23-25: LGTM! Proper implementation of repository pattern with exception handling.The
findByIdmethod correctly uses theOptional.orElseThrow()pattern and throws the appropriate domain-specific exception when an article is not found. The implementation follows established patterns in the codebase.src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java (3)
11-11: LGTM!Proper import addition for the new response DTO.
34-34: LGTM!Correct integration of the new
remindTimeparameter into the article creation flow.
38-41: LGTM!Clean implementation of the article detail retrieval method. The method properly delegates to the service layer and converts the entity to a response DTO.
src/main/java/com/pinback/pinback_server/domain/article/domain/entity/Article.java (3)
3-3: LGTM!Proper import addition for the new
LocalDateTimefield.
54-55: LGTM!Well-structured field addition with appropriate JPA mapping. The nullable nature allows for optional reminders.
60-60: LGTM!Proper integration of the new
remindAtparameter into the factory method and builder pattern.Also applies to: 67-67
src/test/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecaseTest.java (3)
20-20: LGTM!Proper import addition for the new response DTO.
56-56: LGTM!Good addition to verify the new
remindAtfield is properly saved.
75-92: LGTM!Comprehensive test coverage for the new
getArticleDetailfunctionality. The test properly verifies all fields of the response DTO.src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java (3)
3-4: LGTM!Proper import additions for the new endpoint functionality.
Also applies to: 12-12
21-21: LGTM!Good practice to use plural form for REST API endpoints.
32-36: Confirm SecurityConfig Secures /articles/details
Authentication is enforced globally bySecurityConfig(anyRequest().authenticated()), so/articles/details/{articleId}already requires a valid JWT. No@CurrentUserparameter is needed for authentication.If your business requirements differ:
- To make details public (no auth): add
in.requestMatchers("/articles/details/**").permitAll()SecurityConfig.filterChain(...).- To restrict access to article owners: inject the current user and enforce ownership, e.g.:
@GetMapping("/details/{articleId}") public ResponseDto<ArticleDetailResponse> getArticleDetails(@CurrentUser User user, @PathVariable Long articleId) { // verify user owns articleId before returning details }Key locations:
- src/main/java/com/pinback/pinback_server/global/config/security/SecurityConfig.java
- src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java
Likely an incorrect or invalid review comment.
src/main/java/com/pinback/pinback_server/domain/article/presentation/dto/response/ArticleDetailResponse.java (1)
1-23: LGTM!Well-structured DTO record with clean field definitions and proper entity mapping. The factory method correctly converts all Article fields and maintains consistency with the CategoryDetail pattern.
src/test/java/com/pinback/pinback_server/domain/fixture/TestFixture.java (1)
3-3: LGTM: Required import for LocalDateTime.The import is correctly added to support the new LocalDateTime parameter in the Article.create method calls.
1 participant
🚀 PR 요약
아티클 상세조회 API 를 구현했습니다.
✨ PR 상세 내용
🚨 주의 사항
없습니다
✅ 체크 리스트
close #17
Summary by CodeRabbit
New Features
Bug Fixes
Tests