[FIX] 할 일 생성, 수정, 삭제 기능 수정 #59
Conversation
WalkthroughThis change introduces a new enum Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TodoRequestType
participant ExceptionHandler
Client->>TodoRequestType: fromString(input)
alt Valid Request Type
TodoRequestType-->>Client: Returns corresponding enum constant
else Invalid Request Type
TodoRequestType->>ExceptionHandler: throw InvalidTodoRequestTypeException
ExceptionHandler-->>Client: Error using INVALID_TODO_REQUEST_TYPE
end
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
🔭 Outside diff range comments (1)
backend/src/main/java/com/dubu/backend/todo/repository/CustomScheduleRepositoryImpl.java (1)
27-30: Remove unused helper method.The
applyJoinTodomethod is no longer used after removing the conditional fetch join logic.Apply this diff to remove the unused method:
- private JPAQuery<Schedule> applyJoinTodo(JPAQuery<Schedule> jpaQuery){ - return jpaQuery.leftJoin(schedule.todos, todo) - .fetchJoin(); - }
🧹 Nitpick comments (15)
backend/src/main/java/com/dubu/backend/todo/dto/response/TodoInfo.java (1)
19-22: Consider refactoring for better code reuse and null safety.The implementation could be improved by:
- Reusing the existing
fromEntitymethod- Adding null safety check
- Improving formatting for better readability
Here's a suggested improvement:
public static List<TodoInfo> fromEntities(List<Todo> todos){ + if (todos == null) { + return List.of(); + } - return todos.stream() - .map(todo -> new TodoInfo(todo.getId(), todo.getCategory(), todo.getDifficulty(), todo.getTitle(), todo.getMemo())).toList(); + return todos.stream() + .map(TodoInfo::fromEntity) + .toList(); }This refactoring:
- Handles null input gracefully
- Reuses the existing
fromEntitymethod, reducing code duplication- Improves readability with proper formatting
backend/src/main/java/com/dubu/backend/todo/controller/TodoController.java (2)
49-57: Check REST status code for patch requests.
Returning 201 (Created) when a tomorrow schedule is generated may be confusing for typical REST conventions (PATCH usually returns 200). However, this may be a valid requirement given the business logic.
60-68: Consider aligning deletion response status with REST conventions.
Using 201 for a resource deletion might be unusual. If your domain logic truly requires 201 in certain cases, ensure it’s well-documented. Otherwise, 200 (OK) or 204 (No Content) is more common for successful deletions.backend/src/main/java/com/dubu/backend/todo/service/impl/SaveTodoManagementService.java (1)
1-34: Class definition and dependencies.
All required repositories are properly injected. Check concurrency controls or potential indexing if large volumes of data are expected, as dynamic schedule lookups might become a bottleneck.backend/src/main/java/com/dubu/backend/todo/service/impl/TomorrowTodoManagementService.java (3)
68-98: Refactor duplicated logic between createTodo and createTodoFromArchived.
Both methods share checks for schedule existence, the “3 tasks limit,” and conditional creation of tomorrow’s schedule. To reduce maintenance and potential errors, consider extracting this logic into helper methods or a reusable service routine.
101-149: Efficiency concern in identifying the target todo within todayTodos.
Using an index-based search (lines 117-122) is adequate for small lists. However, as the list scales, repeated linear scans can become a performance bottleneck. If the list could grow larger, consider using a map keyed by Todo IDs or adjusting the query to directly fetch the needed entity.
177-186: Method naming might be misleading.
The method createTomorrowTodosFromTodayTodos can be called even if the original schedule is not actually today’s schedule. Consider renaming it to better reflect its generic “todo cloning” behavior.backend/src/main/java/com/dubu/backend/todo/dto/request/TodoCreateFromArchivedRequest.java (1)
3-4: Consider additional business validations for 'todoId'.
If the “todoId” must be positive or meet certain constraints, use validators or domain checks to enforce this. If the intention is to always reference a valid archived Todo, clarifying constraints helps ensure robust domain logic.backend/src/main/java/com/dubu/backend/todo/dto/response/TodoSuccessResponse.java (1)
3-4: Consistent naming for boolean fields.
Renaming “isTomorrowScheduleCreated” to “tomorrowScheduleCreated” can improve readability and match conventional Java naming for booleans. This is a minor style preference but can provide consistency throughout the codebase.backend/src/main/java/com/dubu/backend/todo/converter/StringToTodoRequestTypeConverter.java (1)
7-12: Consider adding explicit error handling.While the converter correctly implements the Spring Converter interface, it might benefit from explicit error handling for invalid type strings.
Consider wrapping the conversion in a try-catch block:
@Override public TodoRequestType convert(String type) { + try { return TodoRequestType.fromString(type); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Invalid todo request type: " + type); + } }backend/src/main/java/com/dubu/backend/todo/dto/request/TodoCreateRequest.java (1)
8-18: Add Javadoc to document the entity creation method.The
toEntitymethod would benefit from documentation explaining its parameters and behavior.Add Javadoc:
+ /** + * Creates a Todo entity from the request data. + * + * @param member The member creating the todo + * @param category The resolved category entity + * @param schedule The associated schedule + * @param type The type of todo + * @return A new Todo entity + */ public Todo toEntity(Member member, Category category, Schedule schedule, TodoType type) {backend/src/main/java/com/dubu/backend/todo/dto/enums/TodoRequestType.java (2)
8-16: Consider decoupling service names from the enum.The enum directly contains service bean names, which creates tight coupling. Consider moving these to a configuration file or using a more flexible mapping strategy.
Example approach using a configuration file:
@Configuration public class TodoServiceConfig { @Value("${todo.service.today}") private String todayServiceName; @Value("${todo.service.tomorrow}") private String tomorrowServiceName; @Value("${todo.service.save}") private String saveServiceName; // getters }
18-23: Enhance string conversion robustness.The current implementation:
- Only matches exact case-insensitive strings
- Doesn't handle null values
- Doesn't trim whitespace
Consider this more robust implementation:
public static TodoRequestType fromString(String value){ + if (value == null) { + throw new ConstraintViolationException("TodoRequestType value cannot be null", null); + } + String normalizedValue = value.trim().toUpperCase(); return Arrays.stream(values()) - .filter(type -> type.name().toLowerCase().equals(value)) + .filter(type -> type.name().equals(normalizedValue)) .findFirst() .orElseThrow(() -> new ConstraintViolationException("Invalid TodoRequestType value: " + value, null)); }backend/src/main/java/com/dubu/backend/todo/entity/Todo.java (2)
57-60: Remove or implement the commented Route entity relationship.The commented code suggests a planned feature. Either implement the Route entity relationship or remove the comments to maintain clean code.
- // Route 엔티티 구현되면 포함 - // @ManyToOne(fetch = FetchType.LAZY) - // @JoinColumn(name = "route_id") - // private Route route;
41-55: Consider indexing foreign key columns for better query performance.The entity has multiple
@ManyToOnerelationships with lazy loading. Consider adding indexes on foreign key columns to optimize the fetch join queries mentioned in the PR objectives.Add the following indexes to improve query performance:
@Table(uniqueConstraints = { @UniqueConstraint(columnNames = {"parent_id", "schedule_id"}) -}) +}, indexes = { + @Index(name = "idx_todo_member", columnList = "member_id"), + @Index(name = "idx_todo_category", columnList = "category_id"), + @Index(name = "idx_todo_parent", columnList = "parent_id"), + @Index(name = "idx_todo_schedule", columnList = "schedule_id") +})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
backend/src/main/java/com/dubu/backend/global/config/WebConfig.java(2 hunks)backend/src/main/java/com/dubu/backend/todo/controller/TodoController.java(2 hunks)backend/src/main/java/com/dubu/backend/todo/converter/StringToTodoRequestTypeConverter.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/dto/enums/TodoRequestType.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/dto/request/CreateTodoFromArchivedRequest.java(0 hunks)backend/src/main/java/com/dubu/backend/todo/dto/request/TodoCreateFromArchivedRequest.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/dto/request/TodoCreateRequest.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/dto/request/TodoUpdateRequest.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/dto/response/TodoInfo.java(2 hunks)backend/src/main/java/com/dubu/backend/todo/dto/response/TodoManageResult.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/dto/response/TodoSuccessResponse.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/entity/Schedule.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/entity/Todo.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/registry/TodoManagementServiceRegistry.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/repository/CustomScheduleRepository.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/repository/CustomScheduleRepositoryImpl.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/repository/ScheduleRepository.java(0 hunks)backend/src/main/java/com/dubu/backend/todo/repository/TodoRepository.java(2 hunks)backend/src/main/java/com/dubu/backend/todo/service/TodoManagementService.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/SaveTodoManagementService.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/TodayTodoManagementService.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/TodoManagementServiceImpl.java(0 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/TodoQueryServiceImpl.java(4 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/TomorrowTodoManagementService.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/support/ScheduledDateResolver.java(0 hunks)backend/src/test/java/com/dubu/backend/config/TestQueryDslConfig.java(1 hunks)backend/src/test/java/com/dubu/backend/todo/service/impl/TodoCreateFromArchivedTest.java(0 hunks)backend/src/test/java/com/dubu/backend/todo/service/impl/TodoCreateTest.java(0 hunks)
💤 Files with no reviewable changes (6)
- backend/src/main/java/com/dubu/backend/todo/dto/request/CreateTodoFromArchivedRequest.java
- backend/src/main/java/com/dubu/backend/todo/repository/ScheduleRepository.java
- backend/src/test/java/com/dubu/backend/todo/service/impl/TodoCreateTest.java
- backend/src/main/java/com/dubu/backend/todo/support/ScheduledDateResolver.java
- backend/src/test/java/com/dubu/backend/todo/service/impl/TodoCreateFromArchivedTest.java
- backend/src/main/java/com/dubu/backend/todo/service/impl/TodoManagementServiceImpl.java
✅ Files skipped from review due to trivial changes (2)
- backend/src/test/java/com/dubu/backend/config/TestQueryDslConfig.java
- backend/src/main/java/com/dubu/backend/todo/dto/request/TodoUpdateRequest.java
🔇 Additional comments (32)
backend/src/main/java/com/dubu/backend/todo/dto/response/TodoInfo.java (1)
1-24: Well-structured DTO implementation!The record implementation with its conversion methods aligns well with the PR objectives. It provides a clean interface for both single and bulk entity conversion, supporting the new response structure and facilitating the strategy pattern implementation for different task types.
backend/src/main/java/com/dubu/backend/todo/repository/CustomScheduleRepositoryImpl.java (1)
20-24: LGTM! Query optimization looks good.The simplified query correctly addresses the LIMIT and FETCH JOIN issue mentioned in the PR objectives by using fetchFirst() without fetch joins.
Let's verify the query performance and usage:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the method is used with proper error handling and verify if todos are accessed efficiently # Test: Search for method usage to verify error handling rg -A 5 "findLatestSchedule.*Member.*LocalDate" # Test: Search for potential N+1 issues by checking if todos are accessed after fetching the schedule ast-grep --pattern 'schedule.getTodos()'Length of output: 1463
Simplified Query Implementation Verified
The findLatestSchedule method now correctly retrieves the latest schedule using fetchFirst(). Our searches confirm:
- The repository method is declared and implemented as intended.
- The service layer accesses schedule.getTodos(), indicating that related todos are loaded (albeit lazily), which aligns with the optimized query approach.
As a next step, consider removing any unused helper methods (for example, applyJoinTodo) if they are indeed no longer required.
backend/src/main/java/com/dubu/backend/todo/repository/CustomScheduleRepository.java (1)
9-11: LGTM! Interface simplification aligns with query optimization goals.The simplified interface with a single focused method is a good improvement. The removal of the
includeJoinTodoparameter suggests that fetch join handling has been moved to the implementation layer where it belongs.Let's verify the implementation to ensure proper query optimization:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Find and analyze the implementation of CustomScheduleRepository # to verify proper handling of fetch joins and LIMIT # Search for the implementation class echo "Searching for CustomScheduleRepository implementation..." ast-grep --pattern 'class $_ implements CustomScheduleRepository { $$$ }' # Search for usage of fetch joins in the implementation echo "Checking fetch join usage..." rg -A 5 'fetch|join' --type javaLength of output: 8362
Verified: Implementation correctly applies fetch join and limit optimizations.
The implementation in CustomScheduleRepositoryImpl.java shows that the fetch join is now handled through the applyJoinTodo method (using leftJoin with fetchJoin) and the use of fetchFirst() aligns with the objective to optimize the query for retrieving the latest schedule. The interface simplification and corresponding implementation adjustments meet the query optimization goals.
backend/src/main/java/com/dubu/backend/todo/repository/TodoRepository.java (4)
16-17: LGTM! Good use of JOIN FETCH and clear naming.The use of JOIN FETCH is appropriate for eager loading of a single entity, and the method name clearly indicates what's being fetched.
19-20: LGTM! Appropriate use of JOIN and clear naming.The use of regular JOIN is correct here since schedule is only used in the WHERE clause. The method name clearly indicates the fetch strategy.
22-23: LGTM! Simple and efficient query.The query is straightforward and efficient, using direct property access without unnecessary joins.
31-32: Consider pagination requirements with JOIN FETCH.The use of JOIN FETCH with potentially large result sets could cause memory issues. If pagination is needed, be aware that FETCH JOIN doesn't work with LIMIT in JPA.
Consider these alternatives if pagination is needed:
- Use
@EntityGraphinstead of FETCH JOIN- Split the query into two parts: first fetch IDs with pagination, then fetch entities with those IDs
Would you like me to verify if pagination is used with this query in the codebase?
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for usage of findTodosWithCategoryBySchedule with Pageable or LIMIT rg -A 5 "findTodosWithCategoryBySchedule.*Page"Length of output: 49
Attention: Consider potential memory issues with unbounded JOIN FETCH usage.
Based on the search results, there’s no evidence that the method findTodosWithCategoryBySchedule is currently used with pagination (such as Pageable or LIMIT). As highlighted, if this query returns a large result set, the unbounded JOIN FETCH could lead to performance or memory issues. It’s worth considering alternatives like using an @entitygraph or splitting the query if pagination becomes necessary.
- No usage of Pageable or explicit LIMIT was detected with findTodosWithCategoryBySchedule.
- Continued use in an unbounded context could be problematic if the number of results grows.
backend/src/main/java/com/dubu/backend/todo/service/impl/TodoQueryServiceImpl.java (3)
49-49: LGTM! More specific exception type.The change from
MemberNotFoundExceptiontoMemberCategoryNotFoundExceptionbetter reflects the actual error condition, improving error clarity for API consumers.
44-44: Verify the behavior offindLatestSchedule.The change to
findLatestScheduleimproves readability and aligns with the query optimization objective. Please ensure that:
- The method maintains the same ordering (by date desc)
- The method correctly handles the date parameter
Also applies to: 73-73
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the implementation of findLatestSchedule # Search for the repository method implementation ast-grep --pattern 'findLatestSchedule($_, $_) { $$$ }' # Search for test coverage rg -A 5 'findLatestSchedule'Length of output: 9735
Verified Implementation of findLatestSchedule
The implementation in CustomScheduleRepositoryImpl sorts by date in descending order using orderBy(schedule.date.desc()) and correctly applies the date filter via schedule.date.loe(date). The method is being consistently used across various services (for today and tomorrow schedules), confirming that the change meets the intended design and functionality.
63-63: 🏁 Script executed:#!/bin/bash # Description: Verify the implementation and test coverage of findTodosWithCategoryBySchedule # Search for the repository method implementation to verify join fetch ast-grep --pattern 'findTodosWithCategoryBySchedule($_) { $$$ }' # Search for performance tests rg -A 5 'findTodosWithCategoryBySchedule.*performance'Length of output: 125
backend/src/main/java/com/dubu/backend/todo/controller/TodoController.java (5)
5-13: Imports look appropriate.
These new imports for request, response, and service registry classes align with the updated design. No immediate issues found.
18-18: Valid addition of ResponseEntity.
This import is necessary for the new return types across the controller methods.
28-28: Initialization of TodoManagementServiceRegistry.
No concerns here; using a final field for the registry is a good pattern.
32-36: Ensure the service lookup won't fail.
If the registry fails to find a matching service, consider throwing a user-friendly error or verifying the service’s existence before usage.
41-45: Archived todo creation flow.
The logic is consistent with the “postTodoFromArchived” approach. Returning a specialized response is in line with the changes in the PR.backend/src/main/java/com/dubu/backend/todo/service/impl/TodayTodoManagementService.java (4)
1-31: Class definition and field injection.
The service follows Spring’s standard patterns (@service, @requiredargsconstructor, @transactional). Ensure careful handling of concurrency for daily schedule lookups to prevent data race or exceeding the todo limit when multiple requests happen concurrently.
47-64: createTodoFromArchived: duplication checks.
This method properly throws an exception if the same archived todo is already added. Overall approach looks good, but be mindful of concurrency issues in the same daily schedule.
66-89: modifyTodo: clearing parent relationships.
Breaking parent-child relationships when users update basic fields is consistent with the stated requirements. Confirm that partial updates on memo or other attributes not covered by this logic remain desired.
91-99: removeTodo: straightforward removal.
The service deletes the todo and returns a result with no tomorrow schedule creation. Implementation appears correct.backend/src/main/java/com/dubu/backend/todo/service/impl/SaveTodoManagementService.java (4)
35-45: createTodo: no schedule lookup.
Saving a SAVE-type todo directly with no associated schedule is reasonable. Ensure the logic meets all business constraints around maximum items or concurrency edge cases.
47-57: createTodoFromArchived: duplication check for SAVE.
The check prevents archived items from being duplicated. Implementation is sound.
59-91: modifyTodo: clearing relationships on updating category/title/difficulty.
The approach is similar to TodayTodoManagementService. The logic to clear the parent relationship is consistent. Ensure partial update is tested thoroughly.
93-107: removeTodo logic.
The code removes parent references from potential child todos scheduled for today or tomorrow. This helps maintain integrity when removing favorites.backend/src/main/java/com/dubu/backend/todo/service/impl/TomorrowTodoManagementService.java (3)
127-131: Parent relationship reset when changing title, category, or difficulty.
Clearing the parent todo upon any modification might unintentionally remove desired archival references. Ensure this is the intended business logic, as it may cause data lineage issues.
152-175: Verify orphaned relations or children when removing a todo.
While this code removes the targeted todo and replicates the remainder if tomorrow’s schedule does not exist, ensure that child todos or references (e.g., archived, subtasks) are handled correctly. You might need to consider orphan removal or cascade rules in the underlying entity relationships to avoid data inconsistency.
37-65: 🏁 Script executed:#!/bin/bash # Searching the codebase for 'findLatestSchedule' and relevant methods rg "findLatestSchedule"Length of output: 2652
backend/src/main/java/com/dubu/backend/todo/dto/response/TodoManageResult.java (1)
3-8: LGTM! Well-structured response DTO.The generic record with a factory method effectively supports the new API response requirements, allowing flexible response types while tracking tomorrow's schedule creation status.
backend/src/main/java/com/dubu/backend/todo/service/TodoManagementService.java (1)
9-12: Consider specifying the generic type parameter.The methods return
TodoManageResult<?>with a wildcard type parameter. Consider specifying the exact type parameter if the return type is known, to provide better type safety and clarity.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for TodoManageResult usage to determine the actual type parameter rg -A 5 'TodoManageResult<[^?]'Length of output: 756
Actionable: Consider updating the return types to use a concrete type parameter
We've verified that the record in
backend/src/main/java/com/dubu/backend/todo/dto/response/TodoManageResult.java
is declared as a generic record (TodoManageResult), and the service interface in
backend/src/main/java/com/dubu/backend/todo/service/TodoManagementService.java
currently uses a wildcard (<?>). If the returned "info" is always expected to be a specific type (for example, a TodoDto or similar), then replacing the wildcard with that concrete type would improve type safety and clarity.
- The generic type parameter (D) in TodoManageResult is intended to be specified.
- Returning TodoManageResult<?> loses the benefit of compile-time type checks.
- If a consistent concrete type is in use, updating the method signatures (e.g. TodoManageResult) is recommended.
backend/src/main/java/com/dubu/backend/todo/entity/Schedule.java (1)
27-27: LGTM! Removingfinalenables dynamic list management.The removal of the
finalmodifier allows for list reference modification, which aligns with the PR objectives for dynamic schedule creation and task management.backend/src/main/java/com/dubu/backend/global/config/WebConfig.java (2)
28-33: Review CORS configuration for security.The current CORS configuration allows all origins (
*), which might be too permissive for a production environment.Consider restricting allowed origins to specific domains based on your deployment environment.
37-37: LGTM! Converter registration is properly placed.The
StringToTodoRequestTypeConverterregistration is correctly placed in theaddFormattersmethod.backend/src/main/java/com/dubu/backend/todo/entity/Todo.java (1)
15-17: Verify the impact of the unique constraint on schedule creation.The unique constraint on
parent_idandschedule_idensures data integrity by preventing duplicate entries. However, verify that this constraint doesn't interfere with the new schedule creation functionality for tomorrow's tasks.Run this script to check for potential conflicts:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for potential conflicts with the unique constraint # Search for schedule creation logic rg -A 10 "createSchedule|createTomorrowSchedule" --type java # Search for parent-child todo relationships ast-grep --pattern 'parentTodo = $_'Length of output: 7713
Unique Constraint is Effectively Integrated with Schedule Creation
- The search confirms that the schedule creation logic (using methods like createTomorrowSchedule and findByParentTodoAndSchedule) properly checks for duplicate TODO entries.
- The unique constraint on (parent_id, schedule_id) acts as an additional safeguard to prevent duplicate entries without interfering with schedule creation.
- There’s no indication that the constraint is causing conflicts in the existing functionality.
| @Query("SELECT t From Todo t WHERE t.member = :member AND t.parentTodo = :parentTodo AND t.type = :type") | ||
| Optional<Todo> findByMemberAndParentTodoAndType(@Param("member") Member member, @Param("parentTodo") Todo parentTod, @Param("type") TodoType type); |
There was a problem hiding this comment.
Fix typo in parameter name.
There's a typo in the parameter name "parentTod" which should be "parentTodo".
Apply this diff to fix the typo:
- Optional<Todo> findByMemberAndParentTodoAndType(@Param("member") Member member, @Param("parentTodo") Todo parentTod, @Param("type") TodoType type);
+ Optional<Todo> findByMemberAndParentTodoAndType(@Param("member") Member member, @Param("parentTodo") Todo parentTodo, @Param("type") TodoType type);📝 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.
| @Query("SELECT t From Todo t WHERE t.member = :member AND t.parentTodo = :parentTodo AND t.type = :type") | |
| Optional<Todo> findByMemberAndParentTodoAndType(@Param("member") Member member, @Param("parentTodo") Todo parentTod, @Param("type") TodoType type); | |
| @Query("SELECT t From Todo t WHERE t.member = :member AND t.parentTodo = :parentTodo AND t.type = :type") | |
| Optional<Todo> findByMemberAndParentTodoAndType(@Param("member") Member member, @Param("parentTodo") Todo parentTodo, @Param("type") TodoType type); |
| @Override | ||
| public TodoManageResult<?> createTodo(Long memberId, TodoCreateRequest todoCreateRequest) { | ||
| Member member = memberRepository.findById(memberId).orElseThrow(() -> new MemberNotFoundException(memberId)); | ||
| Category category = categoryRepository.findByName(todoCreateRequest.category()).orElseThrow(() -> new CategoryNotFoundException(todoCreateRequest.category())); | ||
| Schedule schedule = scheduleRepository.findLatestSchedule(member, LocalDate.now()).orElseThrow(ScheduleNotFoundException::new); | ||
|
|
||
| if(schedule.getTodos().size() == 3){ | ||
| throw new TodoLimitExceededException(); | ||
| } | ||
|
|
||
| Todo todo = todoCreateRequest.toEntity(member, category, schedule, TodoType.SCHEDULED); | ||
|
|
||
| return TodoManageResult.of(false, TodoInfo.fromEntity(todoRepository.save(todo))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
createTodo: concurrency check on 3-todo limit.
Using a simple size check may lead to race conditions if multiple calls occur in parallel. Consider enforcing an optimistic or pessimistic lock, or a database constraint, to ensure the limit.
| public TodoManagementService getService(String serviceName){ | ||
| return todoManagementServices.get(serviceName); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checking and logging for service lookup.
The service lookup could be more robust with null checking and logging for better debugging and error handling.
Consider this implementation:
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
@Component
@RequiredArgsConstructor
public class TodoManagementServiceRegistry {
+ private static final Logger logger = LoggerFactory.getLogger(TodoManagementServiceRegistry.class);
private final Map<String, TodoManagementService> todoManagementServices;
public TodoManagementService getService(String serviceName) {
+ logger.debug("Looking up service for name: {}", serviceName);
+ TodoManagementService service = todoManagementServices.get(serviceName);
+ if (service == null) {
+ throw new IllegalArgumentException("No service found for name: " + serviceName);
+ }
+ return service;
}
}| @@ -3,17 +3,17 @@ | |||
| import com.dubu.backend.member.domain.Member; | |||
| import com.dubu.backend.todo.entity.*; | |||
|
|
|||
| public record CreateTodoRequest(String title, String category, String difficulty, String memo){ | |||
| public record TodoCreateRequest(String title, String category, String difficulty, String memo){ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation annotations to request fields.
Consider adding validation annotations to ensure data integrity.
Add validation annotations:
+import jakarta.validation.constraints.NotBlank;
+import jakarta.validation.constraints.Pattern;
-public record TodoCreateRequest(String title, String category, String difficulty, String memo) {
+public record TodoCreateRequest(
+ @NotBlank(message = "Title is required")
+ String title,
+
+ @NotBlank(message = "Category is required")
+ String category,
+
+ @NotBlank(message = "Difficulty is required")
+ @Pattern(regexp = "EASY|MEDIUM|HARD", message = "Invalid difficulty level")
+ String difficulty,
+
+ String memo) {
| private final TodoQueryService todoQueryService; | ||
| private final TodoManagementServiceRegistry todoManagementServiceRegistry; |
There was a problem hiding this comment.
전략패턴을 통해서 케이스별로 복잡한 비즈니스 로직을 잘 풀어내신 것 같습니다👍
| return Arrays.stream(values()) | ||
| .filter(type -> type.name().toLowerCase().equals(value)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new ConstraintViolationException("Invalid TodoRequestType value: " + value, null)); |
There was a problem hiding this comment.
예외를 따로 만들어서 ERROR CODE를 반환하면 클라이언트가 처리하기 좋을 것 같아요!
There was a problem hiding this comment.
TodoRequestType 의 경우 path variable 이나 쿼리 파라미터로 전달된 값을 받는 enum 입니다.
따라서, 경로 변수나 쿼리 파라미터로 들어온 값을 enum 객체로 변화합니다. 이 때, enum 객체에 해당되는 것이 없으면 제가 설정한 예외를 발생시킵니다.
이 때, 어떤 예외를 던져도 conversionUtils 에서 잡아 MethodArgumentTypeMismatchException 를 다시 던집니다. 그래서, 응답의 이미지에서 처럼 옵니다.
이런 경우에도 예외를 따로 만들어서 처리하는 것이 나을까요? 동작 상 차이가 없다고 해도 의미 상 커스텀 예외를 정의해서 던지는 것이 나은 것 같긴 합니다.
class InvalidTodoRequestTypeException extends BadRequestException 로 구현할 생각입니다.
| @@ -24,7 +24,7 @@ public class Schedule extends BaseTimeEntity { | |||
| private LocalDate date; | |||
|
|
|||
| @OneToMany(mappedBy = "schedule", cascade = CascadeType.ALL, orphanRemoval = true) | |||
| private final List<Todo> todos = new ArrayList<>(); | |||
| private List<Todo> todos = new ArrayList<>(); | |||
There was a problem hiding this comment.
엔티티에서 final 을 사용하지 않는(못하는) 이유는 보통 엔티티는 기본 생성자를 무조건 정의해야 하는 특성 상 final 설정이 불가능합니다. 하지만, 일대다 관계에서 일 쪽의 엔티티가 다 쪽의 엔티티와 연관관계를 갖게 할 때 보통 리스트를 선언과 동시에 초기화를 진행합니다. 그래서 final 을 붙어도 괜찮지 않을까 생각합니다!!
코드 상 제거한 이유는 final 이 없어도 잘 동작해서 일단은 제거해 두었습니다. 프로젝트 시간이 촉박하여 완벽하게 공부하지 못하고 답변 드립니다. 나중에라도 공부하고 나누겠습니다!!
| Category category = categoryRepository.findByName(todoCreateRequest.category()).orElseThrow(() -> new CategoryNotFoundException(todoCreateRequest.category())); | ||
| Schedule schedule = scheduleRepository.findLatestSchedule(member, LocalDate.now()).orElseThrow(ScheduleNotFoundException::new); | ||
|
|
||
| if(schedule.getTodos().size() == 3){ |
There was a problem hiding this comment.
동시성 문제를 생각하면 Schedule 조회쿼리에 Pessimistic Lock을 적용해서 3이상으로 수정되는 것을 방지할 수 있을 것 같습니다. 그리고 범위도 >=3 으로 최대한 가능성을 없애는 방향으로 작성하면 좋을 것 같아요!
There was a problem hiding this comment.
희상님 코멘트의 동시성 문제가 잘 이해가 되지 않습니다. 해당 코드는 오늘/내일 할 일의 경우 최대 3개까지 저장하지 못하는 정책을 바탕으로 구현된 것입니다.
범위를 >=3를 하는 것은 동의합니다!! 수정하겠습니다.
| List<Todo> tomorrowTodos = null; | ||
|
|
||
| // 할 일 3개 이미 존재 | ||
| if(todos.size() == 3){ |
There was a problem hiding this comment.
코드 전반적으로 존재하는 것 같은데 변경하는데 리소스가 많이 들 것 같다면 나중에 리팩토링 하는 방안도 좋을 것 같습니다. 서비스 특성상 사용자 본인만 변경하는 거라 동시성 문제가 현실적으로 발생하기가 쉽지 않을 것 같긴 해요!
There was a problem hiding this comment.
반복된 코드는 동의합니다!! 나중에 리팩토링을 진행하겠습니다. 현재 생각 중인 방법은 추상 클래스를 도입하여 공통된 코드는 메소드로 리팩토링할 생각입니다!!
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 (3)
backend/src/main/java/com/dubu/backend/global/exception/ErrorCode.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/dto/enums/TodoRequestType.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/exception/InvalidTodoRequestTypeException.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/com/dubu/backend/todo/dto/enums/TodoRequestType.java
🔇 Additional comments (2)
backend/src/main/java/com/dubu/backend/todo/exception/InvalidTodoRequestTypeException.java (1)
12-15: LGTM! Clean implementation of error code handling.The getErrorCode implementation correctly returns the enum name for consistent error code handling.
backend/src/main/java/com/dubu/backend/global/exception/ErrorCode.java (1)
53-54: LGTM! Well-structured error code addition.The new error code is properly defined with:
- Appropriate HTTP status (BAD_REQUEST)
- Clear error message with type parameter
- Logical placement in the Todo section
| public InvalidTodoRequestTypeException(String message) { | ||
| super(message.formatted(message)); | ||
| } |
There was a problem hiding this comment.
Fix incorrect string formatting in constructor.
The current implementation message.formatted(message) is incorrect as it uses the message as both the format string and the argument. This could cause unexpected behavior if the message contains format specifiers.
Apply this diff to fix the string formatting:
public InvalidTodoRequestTypeException(String message) {
- super(message.formatted(message));
+ super(message);
}📝 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.
| public InvalidTodoRequestTypeException(String message) { | |
| super(message.formatted(message)); | |
| } | |
| public InvalidTodoRequestTypeException(String message) { | |
| super(message); | |
| } |
Issue Number
close #46
As-Is
할 일 변경(생성, 수정, 삭제) 기능을 수정한다.
To-Be
Check List
Test Screenshot
(Optional) Additional Description
LIMIT 절 과 FETCH JOIN 같이 사용하면 발생하는 문제
일대다 관계에서 다 쪽에 FETCH JOIN을 걸면 JPA는 몇 개의 데이터를 가져와야 할 지 모른다. 따라서, 해당되는 모든 데이터를 어플리케이션 메모리로 가져와서 메모리 단에서 LIMIT 을 수행한다.
Summary by CodeRabbit
New Features
Refactor