Conversation
|
""" WalkthroughNew features were added to support article reminder retrieval, article existence checking by URL, and article deletion with ownership verification. This included updates to service, repository, controller layers, DTOs, exception handling, and comprehensive tests for these functionalities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Usecase
participant Service
participant Repository
Client->>Controller: GET /api/v1/articles/remind?now=...&pageNumber=...&pageSize=...
Controller->>Usecase: getRemindArticles(user, now, pageNumber, pageSize)
Usecase->>Service: findTodayRemind(userId, now, pageable)
Service->>Repository: findTodayRemind(userId, pageable, startAt, endAt)
Repository-->>Service: Page<Article>
Service-->>Usecase: Page<Article>
Usecase-->>Controller: RemindArticleResponse
Controller-->>Client: ResponseDto<RemindArticleResponse>
Client->>Controller: GET /api/v1/articles/saved?url=...
Controller->>Usecase: checkArticleExists(user, url)
Usecase->>Service: findByUrlAndUser(user, url)
Service->>Repository: findArticleByUserAndUrl(user, url)
Repository-->>Service: Optional<Article>
Service-->>Usecase: Optional<Article>
Usecase-->>Controller: ArticleDetailResponse or null
Controller-->>Client: ResponseDto<ArticleDetailResponse>
Client->>Controller: DELETE /api/v1/articles/{articleId}
Controller->>Usecase: delete(user, articleId)
Usecase->>Repository: findById(articleId)
Usecase->>Usecase: checkOwner(article, user)
Usecase->>Service: delete(article)
Service->>Repository: delete(article)
Usecase-->>Controller: void
Controller-->>Client: ResponseDto<Void>
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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)
✨ 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: 4
🧹 Nitpick comments (4)
src/main/java/com/pinback/pinback_server/domain/article/domain/repository/ArticleRepositoryCustom.java (1)
17-17: Consider renaming the method to better reflect its functionality.The method name
findTodayRemindis misleading since it accepts anystartAtandendAtdate range, not just "today's" reminders. Consider renaming it tofindRemindInRangeorfindRemindBetweento better reflect its actual functionality.- Page<Article> findTodayRemind(UUID userId, Pageable pageable, LocalDateTime startAt, LocalDateTime endAt); + Page<Article> findRemindInRange(UUID userId, Pageable pageable, LocalDateTime startAt, LocalDateTime endAt);src/main/java/com/pinback/pinback_server/domain/article/domain/repository/ArticleRepositoryCustomImpl.java (1)
94-101: Consider ordering by remindAt for better user experience.The query is well-implemented with proper fetch joins for performance. However, ordering by
createdAt.desc()might not be the most logical for reminder functionality. Consider ordering byremindAt.asc()to show the most urgent reminders first.- .orderBy(article.createdAt.desc()) + .orderBy(article.remindAt.asc())src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java (1)
85-103: Review next reminder time calculation for edge casesThe method implementation looks correct for the core functionality, but there are potential edge cases to consider:
- Time zone handling: The calculation doesn't account for time zones or daylight saving time changes
- Month boundary: The manual date construction could be simplified using
LocalDateTime.plusDays(1).withHour().withMinute()- Leap year handling: The current implementation should handle leap years correctly, but using built-in methods would be safer
public RemindArticleResponse getRemindArticles(User user, LocalDateTime now, int pageNumber, int pageSize) { - LocalDateTime nextDate = now.plusDays(1L); - LocalDateTime nextRemindTime = LocalDateTime.of(nextDate.getYear(), nextDate.getMonth(), - nextDate.getDayOfMonth(), - user.getRemindDefault().getHour(), user.getRemindDefault().getMinute()); + LocalDateTime nextRemindTime = now.plusDays(1) + .withHour(user.getRemindDefault().getHour()) + .withMinute(user.getRemindDefault().getMinute()) + .withSecond(0) + .withNano(0); Page<Article> articles = articleGetService.findTodayRemind(user.getId(), now, PageRequest.of(pageNumber, pageSize)); List<ArticlesResponse> articlesResponses = articles.stream() .map(ArticlesResponse::from) .toList(); return new RemindArticleResponse( articles.getTotalElements(), nextRemindTime, articlesResponses ); }src/test/java/com/pinback/pinback_server/domain/article/application/ArticleRemindTest.java (1)
36-104: Consider additional edge case testsThe current test coverage is good but could benefit from additional edge cases:
- Time zone boundaries: Test behavior around daylight saving time changes
- Leap year handling: Test February 29th scenarios
- Empty result pagination: Test pagination when no articles exist
- Large dataset pagination: Test pagination with many articles
@DisplayName("리마인드 시간이 윤년 2월 29일인 경우 다음 리마인드 날짜를 올바르게 계산한다.") @Test void getRemindWhenLeapYear() { //given User user = userRepository.save(user()); Category category = categoryRepository.save(category(user)); //when - 2024년 2월 29일 (윤년) RemindArticleResponse responses = articleManagementUsecase.getRemindArticles(user, LocalDateTime.of(2024, 2, 29, 9, 0, 0), 0, 5); //then assertThat(responses.nextRemind()) .isEqualTo(LocalDateTime.of(2024, 3, 1, 12, 0, 0)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java(3 hunks)src/main/java/com/pinback/pinback_server/domain/article/domain/repository/ArticleRepositoryCustom.java(1 hunks)src/main/java/com/pinback/pinback_server/domain/article/domain/repository/ArticleRepositoryCustomImpl.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/presentation/ArticleController.java(4 hunks)src/main/java/com/pinback/pinback_server/domain/article/presentation/dto/response/RemindArticleResponse.java(1 hunks)src/test/java/com/pinback/pinback_server/domain/article/application/ArticleRemindTest.java(1 hunks)src/test/java/com/pinback/pinback_server/domain/fixture/TestFixture.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleGetService.java (1)
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.748Z
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.
src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java (1)
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.748Z
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.
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java (1)
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.748Z
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.
🧬 Code Graph Analysis (1)
src/test/java/com/pinback/pinback_server/domain/article/application/ArticleRemindTest.java (1)
src/test/java/com/pinback/pinback_server/domain/fixture/TestFixture.java (1)
TestFixture(10-46)
🔇 Additional comments (9)
src/test/java/com/pinback/pinback_server/domain/fixture/TestFixture.java (1)
33-35: LGTM! Well-designed test fixture method.The new
articleWithDatemethod provides a clean way to create test articles with customizable reminder dates, which is essential for testing the time-based reminder functionality. The method signature is clear and follows the existing patterns in the TestFixture class.src/main/java/com/pinback/pinback_server/domain/article/presentation/dto/response/RemindArticleResponse.java (1)
6-10: LGTM! Clean and well-structured response DTO.The record structure is appropriate for the reminder article response with clear field names and correct types. The use of a record for immutable data transfer is a good practice.
src/main/java/com/pinback/pinback_server/domain/article/domain/repository/ArticleRepositoryCustomImpl.java (2)
91-92: LGTM! Proper QueryDSL condition construction.The boolean expression correctly combines user ID filtering with date range conditions using
goe(greater or equal) andloe(less or equal). The condition is well-structured and readable.
103-108: LGTM! Proper pagination implementation.The count query uses the same conditions as the main query, and
PageableExecutionUtils.getPageis used correctly for efficient pagination. The implementation follows best practices.src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java (1)
3-3: LGTM: Clean import additionsThe new imports for
LocalDateTimeandRemindArticleResponseare appropriate for the reminder functionality.Also applies to: 17-17
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java (1)
3-3: LGTM: Appropriate import additionsThe new imports for
LocalDateTime,Page, andRemindArticleResponseare correctly added for the reminder functionality.Also applies to: 6-6, 20-20
src/test/java/com/pinback/pinback_server/domain/article/application/ArticleRemindTest.java (3)
1-35: LGTM: Well-structured test class setupThe test class is properly configured with Spring Boot testing annotations and necessary dependencies. The use of
@Transactionalensures test isolation.
36-62: Excellent test coverage for the main functionalityThe test correctly verifies that articles within the 24-hour window are retrieved and that the next reminder time is calculated properly. The test scenario is clear and well-documented.
89-104: Good edge case coverage for month boundaryThe test correctly verifies that the next reminder date calculation works properly when crossing month boundaries. The expected result (August 1, 12:00 PM) aligns with the user's default reminder time.
| } | ||
|
|
||
| public Page<Article> findTodayRemind(UUID userId, LocalDateTime today, Pageable pageable) { | ||
| LocalDateTime startAt = today.minusDays(1L).plusMinutes(1L); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the time calculation logic.
The calculation today.minusDays(1L).plusMinutes(1L) seems counter-intuitive. It excludes articles with remindAt exactly at the boundary minute (e.g., if current time is 12:00 PM, it excludes articles from 12:00 PM yesterday).
Based on the PR description mentioning "24 hours prior to the current time", shouldn't this be simply today.minusDays(1L)?
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation that might explain this time calculation
rg -A 5 -B 5 "plusMinutes.*1L|minusDays.*1L"Length of output: 1329
I want to inspect how findTodayRemind is defined in the repository interface—especially its query boundaries. Let’s locate its declaration:
🏁 Script executed:
#!/bin/bash
# Find and show context for the repository method
rg -n -A5 -B5 "findTodayRemind" .Length of output: 5819
Clarify the 24-hour window boundary in findTodayRemind
The logic in ArticleGetService.java (line 44):
LocalDateTime startAt = today.minusDays(1L).plusMinutes(1L);will exclude any articles with remindAt exactly at (now − 24h). To cover the full 24-hour window as described, should this simply be:
LocalDateTime startAt = today.minusDays(1L);– or is the extra minute intentional?
Please confirm or adjust to ensure we don’t inadvertently skip the boundary timestamp.
• File: src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleGetService.java
• Line: 44
🤖 Prompt for AI Agents
In
src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleGetService.java
at line 44, the current calculation of startAt using
today.minusDays(1L).plusMinutes(1L) excludes articles with remindAt exactly 24
hours ago. To include the full 24-hour window as intended, remove the
plusMinutes(1L) so that startAt is set to today.minusDays(1L), ensuring no
boundary timestamps are skipped.
|
|
||
| @RestController | ||
| @RequestMapping("/articles") | ||
| @RequestMapping("api/v1/articles") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider the API versioning strategy impact
The base mapping change from /articles to api/v1/articles introduces API versioning, which is a good practice. However, ensure this change is consistent across all controllers and doesn't break existing clients.
🏁 Script executed:
#!/bin/bash
# Description: Check for inconsistent API versioning across controllers
# Expected: All controllers should use consistent versioning pattern
echo "Checking for controllers with inconsistent API versioning..."
rg -A 2 "@RequestMapping" --type java | grep -E "(RequestMapping|RestController)" | head -20Length of output: 593
Ensure consistent API versioning across all controllers
The @RequestMapping annotations are currently inconsistent:
- AuthController uses
/api/v1/auth - ArticleController uses
api/v1/articles(missing leading slash) - CategoryController still uses
/categories(no versioning)
Please align all controllers to the /api/v1 pattern and include the leading slash. For example:
• In ArticleController (src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java):
- @RequestMapping("api/v1/articles")
+ @RequestMapping("/api/v1/articles")• In CategoryController (src/main/java/com/pinback/pinback_server/domain/category/presentation/CategoryController.java):
- @RequestMapping("/categories")
+ @RequestMapping("/api/v1/categories")• Verify AuthController remains /api/v1/auth and update any clients referencing the old endpoints.
📝 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.
| @RequestMapping("api/v1/articles") | |
| // src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java | |
| @RequestMapping("/api/v1/articles") |
🤖 Prompt for AI Agents
In
src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java
at line 26, the @RequestMapping annotation is missing a leading slash and does
not follow the consistent API versioning pattern. Update the annotation to
include the leading slash and the version prefix, changing it to
"/api/v1/articles" to align with other controllers. Also, verify and update the
CategoryController to use "/api/v1/categories" and ensure AuthController remains
"/api/v1/auth". Finally, update any clients referencing old endpoints to use the
new consistent paths.
| @GetMapping("/remind") | ||
| public ResponseDto<RemindArticleResponse> getAllRemindArticles(@CurrentUser User user, | ||
| @RequestParam LocalDateTime now, | ||
| @RequestParam int pageNumber, | ||
| @RequestParam int pageSize) { | ||
|
|
||
| RemindArticleResponse response = articleManagementUsecase.getRemindArticles(user, now, pageNumber, | ||
| pageSize); | ||
| return ResponseDto.ok(response); | ||
| } |
There was a problem hiding this comment.
Validate LocalDateTime parameter and consider security implications
The endpoint accepts a LocalDateTime now parameter directly from the client, which could be a security concern. Consider:
- Input validation: Ensure the
nowparameter is within reasonable bounds - Business logic: Why allow clients to specify arbitrary time values? Consider using server-side current time instead
- Parameter naming: The parameter name
nowis misleading since it can be any datetime value
@GetMapping("/remind")
public ResponseDto<RemindArticleResponse> getAllRemindArticles(@CurrentUser User user,
- @RequestParam LocalDateTime now,
+ @RequestParam(required = false) LocalDateTime referenceTime,
@RequestParam int pageNumber,
@RequestParam int pageSize) {
+ LocalDateTime actualTime = referenceTime != null ? referenceTime : LocalDateTime.now();
+ // Add validation for reasonable time bounds
+ if (actualTime.isAfter(LocalDateTime.now().plusDays(1))) {
+ throw new IllegalArgumentException("Reference time cannot be in the future");
+ }
+
- RemindArticleResponse response = articleManagementUsecase.getRemindArticles(user, now, pageNumber,
+ RemindArticleResponse response = articleManagementUsecase.getRemindArticles(user, actualTime, pageNumber,
pageSize);
return ResponseDto.ok(response);
}📝 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.
| @GetMapping("/remind") | |
| public ResponseDto<RemindArticleResponse> getAllRemindArticles(@CurrentUser User user, | |
| @RequestParam LocalDateTime now, | |
| @RequestParam int pageNumber, | |
| @RequestParam int pageSize) { | |
| RemindArticleResponse response = articleManagementUsecase.getRemindArticles(user, now, pageNumber, | |
| pageSize); | |
| return ResponseDto.ok(response); | |
| } | |
| @GetMapping("/remind") | |
| public ResponseDto<RemindArticleResponse> getAllRemindArticles(@CurrentUser User user, | |
| @RequestParam(required = false) LocalDateTime referenceTime, | |
| @RequestParam int pageNumber, | |
| @RequestParam int pageSize) { | |
| LocalDateTime actualTime = referenceTime != null ? referenceTime : LocalDateTime.now(); | |
| // Add validation for reasonable time bounds | |
| if (actualTime.isAfter(LocalDateTime.now().plusDays(1))) { | |
| throw new IllegalArgumentException("Reference time cannot be in the future"); | |
| } | |
| RemindArticleResponse response = articleManagementUsecase.getRemindArticles(user, actualTime, pageNumber, | |
| pageSize); | |
| return ResponseDto.ok(response); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java
around lines 61 to 70, the method accepts a LocalDateTime parameter named 'now'
directly from the client, which poses security and validation risks. Remove the
'now' parameter from the request and instead obtain the current server time
within the method to ensure consistent and secure time handling. Also, update
the method signature and any related calls to reflect this change, eliminating
the misleading parameter name.
| @DisplayName("오늘 기준을 24시간 범위 외의 아티클들을 조회할 수 없다.") | ||
| @Test | ||
| void getRemindArticleOutRange() { | ||
| //given | ||
| User user = userRepository.save(user()); | ||
| Category category = categoryRepository.save(category(user)); | ||
|
|
||
| //리마인드 시간이 7월 7일 9시 0분, 7월 8일 9시 이라고 가정 | ||
| // 오늘이 7월 8일 9시라고 가정 | ||
| articleRepository.save( | ||
| articleWithDate(user, "article" + "1", category, LocalDateTime.of(2025, 7, 7, 9, 0, 0))); | ||
|
|
||
| articleRepository.save( | ||
| articleWithDate(user, "article" + "2", category, LocalDateTime.of(2025, 7, 8, 9, 1, 0))); | ||
|
|
||
| //when | ||
| RemindArticleResponse responses = articleManagementUsecase.getRemindArticles(user, | ||
| LocalDateTime.of(2025, 7, 8, 9, 0, 0), 0, 5); | ||
|
|
||
| //then | ||
| assertThat(responses.totalArticle()) | ||
| .isEqualTo(0); | ||
|
|
||
| } |
There was a problem hiding this comment.
Critical issue: Test logic appears incorrect
The test comments and setup suggest that articles outside the 24-hour range should not be retrieved, but the test data setup seems problematic:
- Article 1: July 7, 9:00 AM (exactly 24 hours before reference time)
- Article 2: July 8, 9:01 AM (1 minute after reference time)
- Reference time: July 8, 9:00 AM
Based on the PR description, the feature should retrieve articles from "24 hours prior to current time up to current time". This means:
- Article 1 should be included (exactly at the boundary)
- Article 2 should be excluded (in the future)
But the test expects 0 articles, which suggests the boundary logic might be exclusive of the exact 24-hour mark.
#!/bin/bash
# Description: Verify the actual implementation of findTodayRemind method to understand the boundary logic
# Expected: Find the exact time range logic used in the repository implementation
echo "Searching for findTodayRemind implementation..."
ast-grep --pattern 'findTodayRemind($_) {
$$$
}'
echo "Searching for time range queries..."
rg -A 10 "findTodayRemind" --type java🤖 Prompt for AI Agents
In
src/test/java/com/pinback/pinback_server/domain/article/application/ArticleRemindTest.java
between lines 64 and 87, the test incorrectly expects zero articles when
querying for articles within the last 24 hours up to the reference time. To fix
this, adjust the test expectation to include the article exactly 24 hours before
the reference time (July 7, 9:00 AM) and exclude the article after the reference
time (July 8, 9:01 AM). Update the assertion to expect one article instead of
zero, reflecting the inclusive boundary logic of the feature.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java(3 hunks)src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleDeleteService.java(1 hunks)src/main/java/com/pinback/pinback_server/domain/article/exception/ArticleNotOwnedException.java(1 hunks)src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java(4 hunks)src/main/java/com/pinback/pinback_server/global/exception/constant/ExceptionCode.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleDeleteService.java
- src/main/java/com/pinback/pinback_server/global/exception/constant/ExceptionCode.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/pinback/pinback_server/domain/article/presentation/ArticleController.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.748Z
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.
src/main/java/com/pinback/pinback_server/domain/article/exception/ArticleNotOwnedException.java (1)
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.748Z
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.
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java (1)
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.748Z
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.
🧬 Code Graph Analysis (1)
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java (2)
src/main/java/com/pinback/pinback_server/domain/article/domain/service/ArticleGetService.java (1)
Service(22-57)src/main/java/com/pinback/pinback_server/domain/article/exception/ArticleNotOwnedException.java (1)
ArticleNotOwnedException(6-10)
🔇 Additional comments (4)
src/main/java/com/pinback/pinback_server/domain/article/exception/ArticleNotOwnedException.java (1)
6-10: LGTM! Clean exception implementation.The exception follows the established pattern in the codebase by extending
ApplicationExceptionand using a specific error code. The implementation is simple and focused on its single responsibility.src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java (3)
89-107: LGTM! Well-structured reminder article retrieval.The method correctly implements the 24-hour reminder logic by:
- Calculating the next reminder time based on user's default settings
- Retrieving articles within the current reminder window
- Properly structuring the response with pagination data
The next reminder time calculation is accurate - it takes the next day and sets it to the user's configured reminder hour/minute.
109-113: LGTM! Appropriate null handling for article existence check.The method correctly returns
nullwhen an article doesn't exist, which aligns with the PR requirements. The implementation is clean and uses the appropriate service layer method.
115-120: LGTM! Well-structured delete operation with proper ownership verification.The method correctly:
- Retrieves the article first (which will throw if not found)
- Verifies ownership before deletion
- Uses proper transaction handling
- Delegates to the appropriate service for deletion
The ownership verification adds necessary security to prevent unauthorized deletions.
| public void checkOwner(Article article, User user) { | ||
| if (!(article.getUser() == user)) { | ||
| throw new ArticleNotOwnedException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix object equality check for entity comparison.
The reference equality check article.getUser() == user may not work correctly if the entities are loaded from different contexts or sessions. Entity comparisons should typically use ID-based equality.
- public void checkOwner(Article article, User user) {
- if (!(article.getUser() == user)) {
- throw new ArticleNotOwnedException();
- }
- }
+ public void checkOwner(Article article, User user) {
+ if (!article.getUser().getId().equals(user.getId())) {
+ throw new ArticleNotOwnedException();
+ }
+ }Let me verify how entity equality is handled elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash
# Check how User entity comparisons are done in other parts of the codebase
rg -A 3 -B 3 "getUser\(\)\s*=="
# Check if User entity has custom equals implementation
ast-grep --pattern 'class User {
$$$
equals($$$) {
$$$
}
$$$
}'Length of output: 941
Fix owner check to use ID-based comparison instead of reference equality
Since User does not override equals(), article.getUser() == user only checks object identity and can fail when entities come from different persistence contexts. Update the check to compare their IDs:
• File: src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java
Lines: 122–126
- public void checkOwner(Article article, User user) {
- if (!(article.getUser() == user)) {
- throw new ArticleNotOwnedException();
- }
- }
+ public void checkOwner(Article article, User user) {
+ if (!article.getUser().getId().equals(user.getId())) {
+ throw new ArticleNotOwnedException();
+ }
+ }🤖 Prompt for AI Agents
In
src/main/java/com/pinback/pinback_server/domain/article/application/ArticleManagementUsecase.java
around lines 122 to 126, the owner check uses reference equality (==) between
User objects, which can fail if they come from different persistence contexts.
Fix this by comparing the IDs of the users instead, using
article.getUser().getId().equals(user.getId()) to ensure correct ownership
validation.
|
|
||
| public RemindArticleResponse getRemindArticles(User user, LocalDateTime now, int pageNumber, int pageSize) { | ||
| LocalDateTime nextDate = now.plusDays(1L); | ||
| LocalDateTime nextRemindTime = LocalDateTime.of(nextDate.getYear(), nextDate.getMonth(), |
There was a problem hiding this comment.
명세서 상에는 다음 리마인드 시간을 나타내는 변수가 remindDateTime로 기재되어 있어서 통일해주시면 좋을 것 같습니당 👍
| if (!(article.getUser() == user)) { | ||
| throw new ArticleNotOwnedException(); | ||
| } |
There was a problem hiding this comment.
다른 트랜잭션/세션에서 User 객체를 로드한 경우에는 동일한 ID를 가진 User라고 하더라도 메모리상 다른 인스턴스이므로, == 비교는 false가 되는 문제가 발생할 수 있는 걸로 알고 있습니다!
현재 저희 서비스 로직 상 참조 비교 방식으로 구현했을 때 문제가 없을까용?
만약 문제가 있다면, .equals() 메서드를 사용하면 좋을 것 같습니다 ! 💪
There was a problem hiding this comment.
로직상 문제는 없었는데 그렇겠네요? 수정하겠습니당
|
|
||
| @RestController | ||
| @RequestMapping("/articles") | ||
| @RequestMapping("/api/v1/articles") |
2 participants
🚀 PR 요약
close #23
close #36
close #37
리마인드 아티클을 조회하는 기능을 추가했습니다.
아티클 조회 여부 기능을 추가했습니다.
✨ PR 상세 내용
리마인드 아티클 조회
아티클 조회여부 기능 추가
🚨 주의 사항
혹시 엣지케이스가 있다면 찾아주세요
✅ 체크 리스트
Summary by CodeRabbit
Summary by CodeRabbit