[FEAT] 즐겨찾기 API 구현 #17
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new feature for managing user likes related to enterprises in the application. This includes the creation of a Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 6
🧹 Outside diff range and nitpick comments (8)
src/main/java/danpoong/soenter/domain/likes/service/LikesService.java (2)
7-11: Add Javadoc documentation to improve code clarity.The interface and its methods should be documented with Javadoc to describe their purpose, parameters, return values, and possible exceptions.
Apply this diff:
+/** + * Service interface for managing user likes/favorites for enterprises. + */ public interface LikesService { + /** + * Adds a like/favorite relationship between a user and an enterprise. + * + * @param userId The unique identifier of the user + * @param enterpriseId The unique identifier of the enterprise + * @throws IllegalArgumentException if either userId or enterpriseId is invalid + */ void addLike(String userId, Long enterpriseId); + + /** + * Retrieves all likes/favorites for a specific user. + * + * @param userId The unique identifier of the user + * @return List of liked enterprises as GetLikeResponse objects + */ List<GetLikeResponse> getLikes(String userId); + + /** + * Removes a like/favorite relationship between a user and an enterprise. + * + * @param userId The unique identifier of the user + * @param enterpriseId The unique identifier of the enterprise + * @throws IllegalArgumentException if either userId or enterpriseId is invalid + */ void removeLike(String userId, Long enterpriseId);
8-10: Consider adding parameter validation annotations.The methods should include validation annotations to ensure parameter integrity before reaching the implementation.
Apply this diff:
+import javax.validation.constraints.NotBlank; +import javax.validation.constraints.NotNull; + public interface LikesService { - void addLike(String userId, Long enterpriseId); - List<GetLikeResponse> getLikes(String userId); - void removeLike(String userId, Long enterpriseId); + void addLike(@NotBlank String userId, @NotNull Long enterpriseId); + List<GetLikeResponse> getLikes(@NotBlank String userId); + void removeLike(@NotBlank String userId, @NotNull Long enterpriseId);src/main/java/danpoong/soenter/domain/likes/converter/LikesConverter.java (1)
6-16: Add method documentation and null safety checksWhile the implementation is clean and follows good practices like the builder pattern, consider these improvements:
- Add Javadoc to document the method's purpose, parameters, and return value
- Consider adding null safety checks for the enterprise parameter and its getter methods
Here's a suggested improvement:
public class LikesConverter { + /** + * Converts an Enterprise entity to a GetLikeResponse DTO + * + * @param enterprise the enterprise entity to convert + * @return GetLikeResponse containing the enterprise details + * @throws IllegalArgumentException if enterprise is null + */ public static GetLikeResponse toGetLikeResponse(Enterprise enterprise) { + if (enterprise == null) { + throw new IllegalArgumentException("Enterprise cannot be null"); + } + return GetLikeResponse.builder() .enterpriseId(enterprise.getEnterpriseId()) .enterpriseName(enterprise.getName()) .city(enterprise.getCity()) .district(enterprise.getDistrict()) .website(enterprise.getWebsite()) .build(); } }src/main/java/danpoong/soenter/domain/likes/dto/LikesDTO.java (1)
6-26: Consider API versioning and pagination support.For better API maintainability and scalability:
- Consider moving these DTOs to a versioned package (e.g.,
v1/dto) to support API versioning.- Consider adding pagination support to
GetLikeResponseby wrapping it in aPage<T>or custom pagination response.src/main/java/danpoong/soenter/domain/likes/controller/LikesController.java (2)
1-20: LGTM! Consider internationalization for API documentation.The class setup follows Spring best practices with proper dependency injection and API documentation. However, since this is a backend API, consider using English for API documentation to ensure better maintainability and accessibility for international developers.
Consider updating the Tag description to English:
-@Tag(name = "Likes Controller", description = "즐겨찾기(좋아요) 관련 API") +@Tag(name = "Likes Controller", description = "API endpoints for managing favorites/likes")
16-20: Consider implementing rate limiting.To prevent potential abuse, consider adding rate limiting to these endpoints. This is especially important for the POST and DELETE operations.
You could implement this using Spring's
@RateLimiterannotation or a custom interceptor. For example:@RateLimiter(name = "likes") @RestController @RequestMapping("/api/likes")Configuration could be added to
application.properties:resilience4j.ratelimiter.instances.likes.limitForPeriod=10 resilience4j.ratelimiter.instances.likes.limitRefreshPeriod=1msrc/main/java/danpoong/soenter/domain/likes/service/LikesServiceImpl.java (2)
64-64: Check for Non-Existing Likes Before DeletionThe
removeLikemethod attempts to delete a like without checking if it exists. If the like does not exist, the operation may silently fail. Consider verifying the existence of the like before deletion and handle the case where it does not exist.Apply this diff to add a check:
@@ -64,2 +64,6 @@ + if (!likesRepository.existsByUserAndEnterprise(user, enterprise)) { + throw new RuntimeException("즐겨찾기에 존재하지 않는 기업입니다."); + } + likesRepository.deleteByUserAndEnterprise(user, enterprise);Alternatively, modify the
deleteByUserAndEnterprisemethod to return the number of records deleted and handle accordingly.
23-24: Consider Constructor Injection for Better TestabilityUsing
@RequiredArgsConstructoris great for dependency injection. However, explicitly defining a constructor can improve readability and testability, especially if you need to inject mocks during unit testing.Example:
@@ -18,2 +18,6 @@ @Service -@RequiredArgsConstructor +public class LikesServiceImpl implements LikesService { + + public LikesServiceImpl(LikesRepository likesRepository, UserRepository userRepository, EnterpriseRepository enterpriseRepository) { + this.likesRepository = likesRepository; + this.userRepository = userRepository; + this.enterpriseRepository = enterpriseRepository; + } private final LikesRepository likesRepository;This makes dependencies explicit and can help with testing frameworks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/main/java/danpoong/soenter/domain/likes/controller/LikesController.java(1 hunks)src/main/java/danpoong/soenter/domain/likes/converter/LikesConverter.java(1 hunks)src/main/java/danpoong/soenter/domain/likes/dto/LikesDTO.java(1 hunks)src/main/java/danpoong/soenter/domain/likes/repository/LikesRepository.java(1 hunks)src/main/java/danpoong/soenter/domain/likes/service/LikesService.java(1 hunks)src/main/java/danpoong/soenter/domain/likes/service/LikesServiceImpl.java(1 hunks)
🔇 Additional comments (9)
src/main/java/danpoong/soenter/domain/likes/service/LikesService.java (2)
7-11: Verify authorization checks in the implementation.
Ensure that the implementation includes proper authorization checks to prevent unauthorized access to likes functionality.
Let's verify the security implementation:
#!/bin/bash
# Description: Check for security configurations and implementations
# Expected: Find security-related configurations and authorization checks
# Search for Spring Security configuration
echo "Checking for Security Configuration..."
fd -e java -x grep -l "SecurityConfig"
# Search for authorization annotations in the implementation
echo "Checking for authorization in implementation..."
ast-grep --pattern 'class LikesServiceImpl implements LikesService {
$$$
@PreAuthorize($$$)
$$$
}'7-11: Consider using a proper User entity type instead of String userId.
Using raw String for userId might lead to maintainability issues. Consider creating a proper User entity or using a specialized ID type.
Let's check if there's an existing User entity in the codebase:
src/main/java/danpoong/soenter/domain/likes/repository/LikesRepository.java (2)
1-9: LGTM! Clean package structure and imports.
The package structure follows domain-driven design principles, and imports are well-organized and necessary.
10-14: Consider adding pagination and enterprise-specific query methods.
While the current implementation is correct, consider the following improvements:
- Add pagination support to
findByUserto handle large datasets efficiently:
Page<Likes> findByUser(User user, Pageable pageable);- Consider adding a method to fetch likes by enterprise:
List<Likes> findByEnterprise(Enterprise enterprise);- Consider adding an index on the user and enterprise columns in the Likes table to optimize these queries.
Additionally, verify that the delete operation is properly handled in the service layer with transaction management.
✅ Verification successful
Verified suggestions for repository improvements
The review suggestions are valid and beneficial:
- The codebase currently has no pagination support, which could lead to performance issues with large datasets.
- There's no enterprise-specific query method, which could be useful for analytics or admin features.
- No database indexes are defined, which could impact query performance.
- The delete operation is properly handled with
@Transactionalannotation inLikesServiceImpl, ensuring data consistency.
src/main/java/danpoong/soenter/domain/likes/repository/LikesRepository.java: Add suggested pagination and enterprise methodssrc/main/java/danpoong/soenter/domain/likes/entity/Likes.java: Add database indexes for user and enterprise columns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if @Transactional is used in the service layer for delete operations
ast-grep --pattern 'class $_ implements LikesService {
$$$
@Transactional
$$$
removeLike($$$) {
$$$
}
$$$
}'
Length of output: 126
Script:
#!/bin/bash
# Let's first find the service implementation file
fd -e java likes.*service -t f
# Then let's check for any delete/remove operations in the service layer
rg -l "delete.*Like|remove.*Like" --type java
# Check for @Transactional annotations in service files
rg "@Transactional" --type java -C 2
Length of output: 6664
Script:
#!/bin/bash
# Let's check the content of LikesServiceImpl.java to verify the implementation details
rg -A 10 "removeLike" src/main/java/danpoong/soenter/domain/likes/service/LikesServiceImpl.java
# Let's also check if there are any existing pagination implementations
rg "Pageable|Page<" --type java
# Check if there are any database indexes defined
fd -e java -e sql -e xml -e yaml -e properties | xargs rg -l "index|@Index"
Length of output: 643
src/main/java/danpoong/soenter/domain/likes/converter/LikesConverter.java (2)
1-5: LGTM! Clean package structure and imports
The package structure follows Java conventions, and imports are specific and necessary for the functionality.
4-4: Verify the DTO structure alignment
Let's verify that the GetLikeResponse DTO structure aligns with all the fields we're setting in the converter.
✅ Verification successful
Let me gather more information about the DTO structure and its usage.
DTO structure and converter implementation are properly aligned
The verification shows perfect alignment between:
- GetLikeResponse DTO fields: enterpriseId, enterpriseName, city, district, website
- LikesConverter implementation: correctly maps all Enterprise entity fields to the corresponding DTO fields
- Service layer: properly uses the converter to transform Enterprise entities to DTOs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the GetLikeResponse DTO structure and its usage
# Find and display the GetLikeResponse DTO definition
ast-grep --pattern 'class GetLikeResponse {
$$$
}'
# Find usages of GetLikeResponse to ensure consistent field usage
rg -l "GetLikeResponse"
Length of output: 434
Script:
#!/bin/bash
# Get the DTO definition and its fields
rg -A 10 "GetLikeResponse" src/main/java/danpoong/soenter/domain/likes/dto/LikesDTO.java
# Check converter implementation
rg -A 10 "GetLikeResponse" src/main/java/danpoong/soenter/domain/likes/converter/LikesConverter.java
# Check service implementation
rg -A 10 "GetLikeResponse" src/main/java/danpoong/soenter/domain/likes/service/LikesServiceImpl.java
Length of output: 2402
src/main/java/danpoong/soenter/domain/likes/dto/LikesDTO.java (2)
1-5: LGTM! Clean package structure and minimal imports.
The package naming and import statements follow best practices.
15-26: 🛠️ Refactor suggestion
Enhance response DTO with validation, documentation, and security measures.
The response DTO needs several improvements for better robustness and security:
public static class LikesResponse {
@Getter
@Builder
+ @NoArgsConstructor
+ @AllArgsConstructor
+ @ToString
public static class GetLikeResponse {
+ /** The unique identifier of the liked enterprise */
- private Long enterpriseId;
+ private final Long enterpriseId;
+ /** The name of the enterprise */
+ @NotBlank(message = "Enterprise name must not be blank")
- private String enterpriseName;
+ private final String enterpriseName;
+ /** The city where the enterprise is located */
+ @NotBlank(message = "City must not be blank")
- private String city;
+ private final String city;
+ /** The district within the city */
+ @NotBlank(message = "District must not be blank")
- private String district;
+ private final String district;
+ /** The enterprise's website URL */
+ @URL(message = "Website must be a valid URL")
- private String website;
+ private final String website;
}
}The changes:
- Add field validation
- Add JavaDoc for all fields
- Add necessary constructors
- Make fields final
- Add URL validation for website field
- Add toString() for logging
Additionally:
src/main/java/danpoong/soenter/domain/likes/controller/LikesController.java (1)
24-31: 🛠️ Refactor suggestion
Consider adding explicit error handling for duplicate likes.
While the basic implementation is correct, consider these improvements:
- Add explicit error handling for cases where a user tries to like the same enterprise multiple times
- Consider adding a response that indicates whether the like was newly created or already existed
Let's check if duplicate handling exists in the service layer:
Consider updating the endpoint to handle duplicates:
@PostMapping
public ApiResponse<Void> addLike(@RequestBody @Valid AddLikeRequest request, Authentication authentication) {
- likesService.addLike(authentication.getName(), request.getEnterpriseId());
- return ApiResponse.onSuccess(null);
+ boolean isNewLike = likesService.addLike(authentication.getName(), request.getEnterpriseId());
+ return ApiResponse.onSuccess(null, isNewLike ? "Like added successfully" : "Like already exists");
}| public static class LikesRequest { | ||
| @Getter | ||
| @Builder | ||
| public static class AddLikeRequest { | ||
| private Long enterpriseId; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation, documentation, and constructor annotations.
Consider the following improvements for better robustness and maintainability:
public static class LikesRequest {
@Getter
@Builder
+ @NoArgsConstructor
+ @AllArgsConstructor
public static class AddLikeRequest {
+ @NotNull(message = "Enterprise ID must not be null")
+ @Positive(message = "Enterprise ID must be positive")
+ /** The unique identifier of the enterprise to be liked */
- private Long enterpriseId;
+ private final Long enterpriseId;
}
}The changes:
- Add validation to prevent null/invalid enterprise IDs
- Add documentation for better API understanding
- Add necessary constructors for proper Builder pattern usage
- Make field final since it's immutable
📝 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 static class LikesRequest { | |
| @Getter | |
| @Builder | |
| public static class AddLikeRequest { | |
| private Long enterpriseId; | |
| } | |
| } | |
| public static class LikesRequest { | |
| @Getter | |
| @Builder | |
| @NoArgsConstructor | |
| @AllArgsConstructor | |
| public static class AddLikeRequest { | |
| @NotNull(message = "Enterprise ID must not be null") | |
| @Positive(message = "Enterprise ID must be positive") | |
| /** The unique identifier of the enterprise to be liked */ | |
| private final Long enterpriseId; | |
| } | |
| } |
| @DeleteMapping("/{enterpriseId}") | ||
| @Operation(summary = "즐겨찾기 삭제 API", description = "특정 기업을 즐겨찾기에서 삭제합니다.") | ||
| @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "COMMON200", description = "OK, 성공") | ||
| @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "COMMON400", description = "잘못된 요청입니다.") | ||
| public ApiResponse<Void> removeLike(@PathVariable Long enterpriseId, Authentication authentication) { | ||
| likesService.removeLike(authentication.getName(), enterpriseId); | ||
| return ApiResponse.onSuccess(null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for like existence before removal.
The current implementation doesn't verify if the like exists before attempting removal. This could lead to successful response codes even when no like was actually deleted.
Consider updating the service call to return a boolean indicating whether a like was actually removed:
@DeleteMapping("/{enterpriseId}")
public ApiResponse<Void> removeLike(@PathVariable Long enterpriseId, Authentication authentication) {
- likesService.removeLike(authentication.getName(), enterpriseId);
- return ApiResponse.onSuccess(null);
+ boolean removed = likesService.removeLike(authentication.getName(), enterpriseId);
+ return ApiResponse.onSuccess(null, removed ? "Like removed successfully" : "Like not found");
}Committable suggestion skipped: line range outside the PR's diff.
| @GetMapping | ||
| @Operation(summary = "즐겨찾기 조회 API", description = "사용자의 즐겨찾기 기업을 조회합니다.") | ||
| @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "COMMON200", description = "OK, 성공") | ||
| @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "COMMON400", description = "잘못된 요청입니다.") | ||
| public ApiResponse<List<GetLikeResponse>> getLikes(Authentication authentication) { | ||
| List<GetLikeResponse> response = likesService.getLikes(authentication.getName()); | ||
| return ApiResponse.onSuccess(response); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add pagination support for better performance.
The current implementation returns all likes without pagination, which could lead to performance issues as the number of favorites grows. Consider implementing pagination using Spring's Pageable interface.
Here's a suggested implementation:
@GetMapping
public ApiResponse<List<GetLikeResponse>> getLikes(Authentication authentication) {
- List<GetLikeResponse> response = likesService.getLikes(authentication.getName());
- return ApiResponse.onSuccess(response);
+ @PageableDefault(size = 20) Pageable pageable,
+ Authentication authentication
+) {
+ Page<GetLikeResponse> response = likesService.getLikes(authentication.getName(), pageable);
+ return ApiResponse.onSuccess(response);
}Committable suggestion skipped: line range outside the PR's diff.
| User user = userRepository.findById(Long.valueOf(userId)) | ||
| .orElseThrow(() -> new RuntimeException("존재하지 않는 사용자입니다.")); |
There was a problem hiding this comment.
Handle potential NumberFormatException when converting userId to Long
Converting userId from String to Long using Long.valueOf(userId) can throw a NumberFormatException if the input is not a valid number. This may lead to unexpected runtime errors. Consider adding validation or handling this exception to ensure robustness.
Apply this diff to handle the potential exception:
@@ -28,2 +28,6 @@
@Transactional
public void addLike(String userId, Long enterpriseId) {
+ Long userIdLong;
+ try {
+ userIdLong = Long.valueOf(userId);
+ } catch (NumberFormatException e) {
+ throw new RuntimeException("유효하지 않은 사용자 ID입니다.", e);
+ }
User user = userRepository.findById(userIdLong)Repeat similar changes for the getLikes and removeLike methods:
@@ -47,2 +47,6 @@
@Transactional(readOnly = true)
public List<GetLikeResponse> getLikes(String userId) {
+ Long userIdLong;
+ try {
+ userIdLong = Long.valueOf(userId);
+ } catch (NumberFormatException e) {
+ throw new RuntimeException("유효하지 않은 사용자 ID입니다.", e);
+ }
User user = userRepository.findById(userIdLong)@@ -59,2 +59,6 @@
@Transactional
public void removeLike(String userId, Long enterpriseId) {
+ Long userIdLong;
+ try {
+ userIdLong = Long.valueOf(userId);
+ } catch (NumberFormatException e) {
+ throw new RuntimeException("유효하지 않은 사용자 ID입니다.", e);
+ }
User user = userRepository.findById(userIdLong)Also applies to: 47-48, 59-60
| @Transactional | ||
| public void addLike(String userId, Long enterpriseId) { | ||
| User user = userRepository.findById(Long.valueOf(userId)) | ||
| .orElseThrow(() -> new RuntimeException("존재하지 않는 사용자입니다.")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use custom exceptions instead of RuntimeException for clarity
Throwing generic RuntimeException may obscure the specific cause of the error. By using custom exception classes, you can provide more meaningful error handling and improve code readability.
Define custom exceptions:
// src/main/java/danpoong/soenter/domain/exception/UserNotFoundException.java
package danpoong.soenter.domain.exception;
public class UserNotFoundException extends RuntimeException {
public UserNotFoundException(String message) {
super(message);
}
}
// src/main/java/danpoong/soenter/domain/exception/EnterpriseNotFoundException.java
package danpoong.soenter.domain.exception;
public class EnterpriseNotFoundException extends RuntimeException {
public EnterpriseNotFoundException(String message) {
super(message);
}
}
// src/main/java/danpoong/soenter/domain/exception/DuplicateLikeException.java
package danpoong.soenter.domain.exception;
public class DuplicateLikeException extends RuntimeException {
public DuplicateLikeException(String message) {
super(message);
}
}Update the service methods to use these exceptions:
@@ -29,2 +29,2 @@
User user = userRepository.findById(userIdLong)
- .orElseThrow(() -> new RuntimeException("존재하지 않는 사용자입니다."));
+ .orElseThrow(() -> new UserNotFoundException("존재하지 않는 사용자입니다."));
@@ -30,2 +30,2 @@
Enterprise enterprise = enterpriseRepository.findById(enterpriseId)
- .orElseThrow(() -> new RuntimeException("존재하지 않는 기업입니다."));
+ .orElseThrow(() -> new EnterpriseNotFoundException("존재하지 않는 기업입니다."));
@@ -34,2 +34,2 @@
throw new RuntimeException("이미 즐겨찾기에 추가된 기업입니다.");
+ throw new DuplicateLikeException("이미 즐겨찾기에 추가된 기업입니다.");Similarly, update the exceptions in getLikes and removeLike methods:
@@ -48,2 +48,2 @@
User user = userRepository.findById(userIdLong)
- .orElseThrow(() -> new RuntimeException("존재하지 않는 사용자입니다."));
+ .orElseThrow(() -> new UserNotFoundException("존재하지 않는 사용자입니다."));@@ -60,2 +60,2 @@
User user = userRepository.findById(userIdLong)
- .orElseThrow(() -> new RuntimeException("존재하지 않는 사용자입니다."));
+ .orElseThrow(() -> new UserNotFoundException("존재하지 않는 사용자입니다."));
@@ -62,2 +62,2 @@
Enterprise enterprise = enterpriseRepository.findById(enterpriseId)
- .orElseThrow(() -> new RuntimeException("존재하지 않는 기업입니다."));
+ .orElseThrow(() -> new EnterpriseNotFoundException("존재하지 않는 기업입니다."));Also applies to: 31-31, 34-34, 48-48, 62-62
| return likes.stream() | ||
| .map(like -> LikesConverter.toGetLikeResponse(like.getEnterprise())) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Possible N+1 Query Issue in getLikes Method
The getLikes method may suffer from the N+1 selects problem when accessing like.getEnterprise(). This can lead to performance issues due to multiple database queries. Consider optimizing the query to fetch all necessary data in a single operation.
Modify the LikesRepository to fetch Enterprise entities eagerly:
// In LikesRepository.java
@Query("SELECT l FROM Likes l JOIN FETCH l.enterprise WHERE l.user = :user")
List<Likes> findByUserWithEnterprise(@Param("user") User user);Update the service method to use the new repository method:
@@ -50,3 +50,3 @@
- List<Likes> likes = likesRepository.findByUser(user);
+ List<Likes> likes = likesRepository.findByUserWithEnterprise(user);This ensures that all Enterprise data is loaded with the Likes entities in a single query.
Related Issue 🍀
Key Changes 🔑
To Reviewers 🙏🏻
Summary by CodeRabbit
New Features
Bug Fixes
Documentation