feat: 중복 닉네임 검증 api 추가 #237
Conversation
|
|
||
| private final SiteUserRepository siteUserRepository; | ||
|
|
||
| public NicknameExistsResponse existsByNickname(String nickname) { |
There was a problem hiding this comment.
existsByNickname 이름이 다소 어색해보이는데, 컨벤션이 따로 있을까요? 간단하게 checkNicknameExists 등을 사용할 수 있을 것 같아서요
There was a problem hiding this comment.
JPA 쿼리 메서드 컨벤션 맞춰서 한거긴 한데 서비스나 컨트롤러쪽은 조금 어색하게 느껴지긴 하네요 바꾸는 게 좋을까요? @nayonsoso
| @GetMapping("/exists") | ||
| public ResponseEntity<NicknameExistsResponse> existsByNickname(@RequestParam("nickname") String nickname) { |
There was a problem hiding this comment.
[컨벤션]
컨트롤러의 인자는 파악하기 쉽게 시작과 끝에 개행하는 것 통일해주세요~
WalkthroughThis pull request updates several components to shift the focus from user profile management to nickname verification. It changes the source of a constant in error handling, introduces the new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MPC as MyPageController
participant MPS as MyPageService
U->>MPC: GET /my (retrieve profile info)
MPC->>MPS: getMyPageInfo(siteUser)
MPS-->>MPC: MyPageResponse
MPC-->>U: HTTP 200 with profile data
U->>MPC: PATCH /my (update profile)
MPC->>MPS: updateMyPageInfo(siteUser, imageFile, nickname)
MPS-->>MPC: Confirmation
MPC-->>U: HTTP 200 (update success)
sequenceDiagram
participant U as User
participant SUC as SiteUserController
participant SUS as SiteUserService
U->>SUC: GET /users/exists?nickname=someName
SUC->>SUS: checkNicknameExists(nickname)
SUS-->>SUC: NicknameExistsResponse
SUC-->>U: HTTP 200 with nickname existence flag
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (7)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (1)
256-265: Consider simplifying the test university creation.The method creates three liked universities and returns the count, but it could be simplified to avoid the additional database query.
private int createLikedUniversities(SiteUser testUser) { LikedUniversity likedUniversity1 = new LikedUniversity(null, 괌대학_A_지원_정보, testUser); LikedUniversity likedUniversity2 = new LikedUniversity(null, 메이지대학_지원_정보, testUser); LikedUniversity likedUniversity3 = new LikedUniversity(null, 코펜하겐IT대학_지원_정보, testUser); likedUniversityRepository.save(likedUniversity1); likedUniversityRepository.save(likedUniversity2); likedUniversityRepository.save(likedUniversity3); - return likedUniversityRepository.countBySiteUser_Id(testUser.getId()); + return 3; // We know we've added exactly 3 universities }Alternatively, you could save all entities at once and still use the database count if needed:
private int createLikedUniversities(SiteUser testUser) { LikedUniversity likedUniversity1 = new LikedUniversity(null, 괌대학_A_지원_정보, testUser); LikedUniversity likedUniversity2 = new LikedUniversity(null, 메이지대학_지원_정보, testUser); LikedUniversity likedUniversity3 = new LikedUniversity(null, 코펜하겐IT대학_지원_정보, testUser); - likedUniversityRepository.save(likedUniversity1); - likedUniversityRepository.save(likedUniversity2); - likedUniversityRepository.save(likedUniversity3); + likedUniversityRepository.saveAll(List.of(likedUniversity1, likedUniversity2, likedUniversity3)); return likedUniversityRepository.countBySiteUser_Id(testUser.getId()); }src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java (1)
31-39: Consider adding validation for the update endpoint inputs.The PATCH endpoint takes a file and nickname as inputs but doesn't appear to have any validation at the controller level. Consider adding validation to ensure the nickname meets your requirements and the file is of an acceptable type/size.
@PatchMapping public ResponseEntity<Void> updateMyPageInfo( @AuthorizedUser SiteUser siteUser, - @RequestParam("file") MultipartFile imageFile, - @RequestParam("nickname") String nickname + @RequestParam("file") MultipartFile imageFile, + @RequestParam("nickname") @Size(min = 2, max = 20) @Pattern(regexp = "^[a-zA-Z0-9가-힣]+$") String nickname ) { myPageService.updateMyPageInfo(siteUser, imageFile, nickname); return ResponseEntity.ok().build(); }Additionally, consider adding content type validation for the image file:
if (imageFile != null && !imageFile.getContentType().startsWith("image/")) { throw new CustomException(ErrorCode.INVALID_FILE_TYPE); }src/main/java/com/example/solidconnection/siteuser/controller/SiteUserController.java (1)
19-24: Consider validating the nickname parameter
While the current route is straightforward for checking if a nickname exists, consider adding parameter validation (e.g., empty or null check) to avoid potential edge cases.src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java (2)
30-32: Configurable constants suggestion
MIN_DAYS_BETWEEN_NICKNAME_CHANGESand the date format constant are fine but consider making them configurable or documented for future flexibility.
46-65: Robustness in updateMyPageInfo
The method includes necessary checks and handles previous image removal. Consider handling S3 failures more gracefully or providing rollback logic if the upload fails mid-transaction.src/main/java/com/example/solidconnection/siteuser/service/SiteUserService.java (2)
14-17: Method implementation aligns with the feature requirements.The
checkNicknameExistsmethod correctly:
- Delegates to the repository layer to check if the nickname exists
- Wraps the result in the appropriate response DTO using the factory method
- Has a clear, descriptive name that matches its functionality
Note: This implementation addresses the feature requirement mentioned in the PR objectives to add a nickname validation API.
However, I noticed in the past review comments there was a discussion about the naming convention of
existsByNickname. While the repository method follows JPA naming conventions, you might consider renaming this service method to better match API semantics (e.g.,validateNicknameAvailabilityinstead ofcheckNicknameExists) if it would better represent the business logic from the consumer's perspective.
1-18: Consider adding input validation.The current implementation doesn't validate the nickname input parameter. You might want to add validation to handle cases like null/empty nicknames or apply any business rules (length, allowed characters, etc.).
public NicknameExistsResponse checkNicknameExists(String nickname) { + if (nickname == null || nickname.trim().isEmpty()) { + throw new IllegalArgumentException("Nickname cannot be empty"); + } + // Optional: Add additional validation for nickname format, length, etc. boolean exists = siteUserRepository.existsByNickname(nickname); return NicknameExistsResponse.from(exists); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/com/example/solidconnection/custom/exception/ErrorCode.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/controller/SiteUserController.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/dto/NicknameExistsResponse.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/service/SiteUserService.java(1 hunks)src/main/java/com/example/solidconnection/university/controller/UniversityController.java(3 hunks)src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java(1 hunks)src/test/java/com/example/solidconnection/siteuser/service/SiteUserServiceTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/example/solidconnection/custom/exception/ErrorCode.java
🔇 Additional comments (32)
src/main/java/com/example/solidconnection/siteuser/dto/NicknameExistsResponse.java (1)
3-8: Well-structured DTO implementation.The record is a good choice for this simple response DTO, providing immutability and reducing boilerplate. The static factory method is a good practice for creating instances.
src/main/java/com/example/solidconnection/university/controller/UniversityController.java (4)
5-5: Appropriate service class update aligned with refactoring.The import change from
SiteUserServicetoMyPageServiceis consistent with the PR objective of renaming components.
35-35: Service injection updated appropriately.Field renamed from
siteUserServicetomyPageServiceto match the component renaming.
52-52: Method call updated correctly.The service method call has been updated to use the renamed service while maintaining the same functionality.
91-91: TODO comment needs addressing.This TODO comment mentions a required future update to the return type but is unrelated to the current PR changes.
Consider creating a separate ticket to track this TODO item to ensure it's not forgotten.
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (7)
43-57: Well-structured test class with proper dependency setup.The test class is properly annotated with
@DisplayNamefor better readability, extends the base integration test class, and includes all necessary dependencies.
58-77: Comprehensive test for user profile information retrieval.This test thoroughly validates that the
getMyPageInfomethod returns the correct user information by checking all relevant fields.
79-97: Thorough test for wish university list retrieval.This test correctly verifies both the size and content of the wish university list returned by the service method.
99-159: Well-organized tests for profile image update functionality.The nested class structure improves readability. The tests cover:
- Successful image updates
- First-time profile updates (no previous image deletion)
- Subsequent profile updates (previous image deletion)
- Empty image file validation
All test cases have clear assertions and properly mock external dependencies.
161-215: Comprehensive tests for nickname update scenarios.The tests thoroughly cover:
- Successful nickname updates
- Duplicate nickname validation
- Minimum waiting period between nickname changes
Good use of test fixtures and error message validation.
217-241: Helper methods for creating test users.These methods create test users with different configurations, improving code reuse and readability.
267-283: Well-structured test utility methods for image files.The methods for creating valid and empty image files are well-implemented and properly separated.
src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java (2)
16-19: Consider making the controller class public.The controller is currently package-private (no explicit access modifier). While this might be intentional, controllers are typically public since they represent the API interface.
@RequiredArgsConstructor @RequestMapping("/my") @RestController -class MyPageController { +public class MyPageController {Check your team's convention for controller visibility to ensure consistency across the codebase.
23-29: Profile information endpoint looks good.The endpoint is well-defined, using proper authorization and returning the appropriate response type.
src/test/java/com/example/solidconnection/siteuser/service/SiteUserServiceTest.java (4)
4-4: New import for NicknameExistsResponse
The added import ensures that the tests can referenceNicknameExistsResponsefor verifying nickname existence.
27-31: Properly setting up test data
Using a sharedSiteUserin the@BeforeEachmethod guarantees a consistent test fixture. This approach is fine for clarity and reusability.
35-44: Well-defined test for existing nicknames
This test verifies that the method returnstruewhen the nickname already exists. The naming is descriptive, and the scenario is clear. Consider edge cases (e.g., empty, special characters) if relevant.
47-53: Clear test for non-existent nicknames
This test ensures the method returnsfalsefor an unknown nickname, which is essential for negative scenario coverage. Also consider edge cases such as uppercase vs. lowercase.src/main/java/com/example/solidconnection/siteuser/controller/SiteUserController.java (3)
3-3: Import for NicknameExistsResponse
Necessary import for returning theNicknameExistsResponsein the newcheckNicknameExistsmethod.
13-13: Endpoint path updated to /users
Changing from "/my" to "/users" could impact existing consumers. Ensure documentation or compatibility measures are in place.
15-15: Class visibility for controller
Marking the controller aspublicis standard for Spring’s component scanning.src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java (8)
1-17: New service class imports and setup
The imports cover essential components (exception handling, S3 integration, repositories). The structure nicely sets the stage for profile management logic.
33-36: Constructor injection for dependencies
Injecting repositories and services via final fields is clean and follows Spring best practices.
37-45: Validate user argument in getMyPageInfo
This method correctly uses a read-only transaction and computes the liked universities count. However, consider verifying thatsiteUseris non-null or loaded to avoid potential NPE.
67-71: Unique nickname validation
The repository check is straightforward. Potentially account for unchanged nicknames if user re-enters the same nickname, but current logic is acceptable if that scenario isn’t required.
73-82: Preventing rapid nickname changes
This method enforces a minimum time interval before updates. It’s good for preventing spammy changes. Consider refining the logic if the updated nickname is identical to the old one.
84-88: Profile image necessity
The check ensures an image is provided. If removing or skipping an image is allowed at some point, further logic might be needed.
90-94: Potential confusion in isDefaultProfileImage
The logic interprets anullor non-profile/URL as default. Verify it aligns with actual default image standards; the naming could be confusing if the default image does start withprofile/.
95-105: Retrieving wish list universities
The method filters the user's liked universities and maps them to the appropriate response class. Straightforward and maintainable.src/main/java/com/example/solidconnection/siteuser/service/SiteUserService.java (3)
3-3: Added appropriate import for the new response DTO.The import for the new
NicknameExistsResponserecord is correctly added to support the nickname validation functionality.
1-18: Remove unused methods during code cleanup.The significant refactoring from
SiteUserServiceto the newMyPageServicestructure looks clean. All the old methods related to user profile management have been properly removed, streamlining this service to focus only on nickname validation functionality.
1-18:Details
❓ Verification inconclusive
Confirm the repository method has appropriate indexing.
The
existsByNicknamemethod in the repository will be performing lookups on thenicknamefield. For production performance, ensure this field is properly indexed in the database, especially if you expect frequent nickname validation checks.
🏁 Script executed:
#!/bin/bash # Check if the nickname field is indexed in any repository files or entity definitions echo "Checking for nickname field in entity classes..." rg -l "class.*SiteUser" --type java echo "Checking for nickname field definition..." rg "private\s+\w+\s+nickname" --type java -A 3 -B 1 echo "Checking for index annotations on nickname field..." rg "@.*Index.*nickname" --type javaLength of output: 3876
Performance Indexing Verification
The shell script results indicate that thenicknamefield in theSiteUserentity is annotated only with@Column(nullable = false, length = 100)and no explicit index annotation (or indexed definition via@Table(indexes = ...)) was detected in the codebase. This absence means that, as currently implemented, the database index on thenicknamecolumn might not exist unless it is defined externally (for example, in a migration script or database configuration).
- Confirm that a database index for the
nicknamecolumn is present—either defined within your entity via table annotations or externally through migration scripts.- If not already in place, consider adding an index on the
nicknamefield to ensure optimal performance of theexistsByNicknamelookup during frequent nickname validations in production.
3 participants
관련 이슈
작업 내용
Summary by CodeRabbit
New Features
Refactor