Conversation
- content가 45자 이상이면 ... 으로 처리
- redis에서 url 삭제가 로직 이후에 되도록 수정
- addPhotoPoint 추가
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/test/java/com/hubo/gillajabi/HuboApplicationTests.java (2)
3-20: Remove unused imports to improve code clarity.Several imported classes are not used in the current implementation. Consider removing the following unused imports:
- import co.elastic.clients.elasticsearch._types.NodeStatistics; - import co.elastic.clients.elasticsearch.nodes.Stats; - import org.elasticsearch.client.Request; - import org.elasticsearch.client.Response; - import org.elasticsearch.client.RestClient; - import static org.junit.jupiter.api.Assertions.*;Also, note that
org.junit.jupiter.api.Testis imported twice (lines 8 and 18). Remove the duplicate import.
27-46: Consider expanding test coverage for Elasticsearch operations.While the current test checks the Elasticsearch connection, it would be beneficial to add more test methods to cover various Elasticsearch operations relevant to your application. For example:
- Test indexing a document
- Test searching for documents
- Test updating a document
- Test deleting a document
These additional tests would provide more comprehensive coverage of your Elasticsearch integration.
Would you like assistance in generating these additional test methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- BE-hubo-gillajabi-resources (1 hunks)
- src/test/java/com/hubo/gillajabi/HuboApplicationTests.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- BE-hubo-gillajabi-resources
| public void testElasticsearchConnection() { | ||
| logger.info("Starting Elasticsearch connection test"); | ||
| try { | ||
| var response = elasticsearchClient.info(); | ||
| logger.info("Elasticsearch info: {}", response); | ||
| } catch (Exception e) { | ||
| logger.error("Error connecting to Elasticsearch", e); | ||
| throw new RuntimeException("Failed to connect to Elasticsearch", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the Elasticsearch connection test method.
While the test method is well-structured, consider the following improvements:
- Add assertions to validate the Elasticsearch connection:
assertNotNull(response);
assertNotNull(response.version());
assertEquals("You know, for Search", response.tagline());- Use more specific exception handling:
} catch (IOException e) {
logger.error("Error connecting to Elasticsearch", e);
fail("Failed to connect to Elasticsearch: " + e.getMessage());
}-
Consider renaming the method to be more descriptive, e.g.,
testElasticsearchConnectionAndInfo(). -
Add a comment explaining the purpose of this test in the context of your application.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/main/java/com/hubo/gillajabi/search/infrastructure/config/ElasticsearchConfig.java (1)
30-30: Approve logging additions with a security suggestion.The addition of logging statements improves the visibility of the configuration process, which is beneficial for debugging and monitoring. However, there's a potential security concern:
- Logging the full 'uris' value (line 30) might expose sensitive information in the logs.
Consider masking part of the URI to avoid exposing full connection details in the logs.
Here's a suggested modification for line 30:
- log.info("uris: " + uris); + log.info("Configuring Elasticsearch with URI: {}", maskUri(uris));And add this helper method to the class:
private String maskUri(String uri) { // This is a simple masking. Adjust as needed for your specific URI format. return uri.replaceAll("(http://|https://)([^:]+)(:\\d+)?", "$1****$3"); }This change will log the URI while masking the host part, improving security.
Also applies to: 36-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- BE-hubo-gillajabi-resources (1 hunks)
- build.gradle (2 hunks)
- src/main/java/com/hubo/gillajabi/search/infrastructure/config/ElasticsearchConfig.java (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- BE-hubo-gillajabi-resources
- build.gradle
🔇 Additional comments (1)
src/main/java/com/hubo/gillajabi/search/infrastructure/config/ElasticsearchConfig.java (1)
3-4: Approve logging framework change and query profile annotation removal.The switch from Log4j to Slf4j is a good improvement. Slf4j provides a more flexible logging facade that allows for easier switching between different logging implementations.
However, I noticed that the
@Profile("!test")annotation has been removed. Can you confirm if this was intentional? This change might affect how this configuration is used in different environments, particularly in test scenarios.To verify the impact of removing the @Profile annotation, let's check for its usage in test files:
Also applies to: 16-16
✅ Verification successful
To ensure the removal of the
@Profile("!test")annotation doesn't affect other parts of the codebase, please run the following script to search for its usage across all Java files:
Verified removal of
@Profile("!test")annotation.The search across all Java files did not reveal any usage of
@Profile("!test"). Therefore, removing this annotation fromElasticsearchConfig.javashould not have unintended side effects on other parts of the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of @Profile("!test") in test files # Test: Search for @Profile("!test") in test files rg --type java -g '*Test.java' '@Profile\("!test"\)'Length of output: 54
Script:
#!/bin/bash # Description: Search for usage of @Profile("!test") in all Java files rg --type java '@Profile\("!test"\)'Length of output: 38
| ClientConfiguration clientConfiguration = ClientConfiguration.builder() | ||
| .connectedTo(uris.replace("http://", "")) | ||
| .withBasicAuth(username, password) | ||
| .build(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggest improvement for handling both HTTP and HTTPS URIs.
The current implementation removes the "http://" prefix from the URI. However, it doesn't account for potential "https://" URIs. To make the configuration more robust and secure, consider handling both HTTP and HTTPS protocols.
Here's a suggested modification:
- .connectedTo(uris.replace("http://", ""))
+ .connectedTo(uris.replaceFirst("^https?://", ""))This change will remove both "http://" and "https://" prefixes, making the configuration work correctly for both secure and non-secure connections.
📝 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.
| ClientConfiguration clientConfiguration = ClientConfiguration.builder() | |
| .connectedTo(uris.replace("http://", "")) | |
| .withBasicAuth(username, password) | |
| .build(); | |
| ClientConfiguration clientConfiguration = ClientConfiguration.builder() | |
| .connectedTo(uris.replaceFirst("^https?://", "")) | |
| .withBasicAuth(username, password) | |
| .build(); |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (1)
.github/workflows/opened-pr-test.yml (1)
207-214: Improved test logging and output captureThe addition of detailed logging and output capture for Gradle tests is a good improvement for debugging and troubleshooting.
This change will make it easier to diagnose issues when tests fail. However, be aware that it might slightly increase the workflow execution time due to the additional I/O operations.
Consider adding a condition to only enable detailed logging when needed:
- name: Test with Gradle and generate detailed logs run: | mkdir -p logs if [ "${{ github.event_name }}" == "pull_request" ]; then ./gradlew test -Dspring.profiles.active=test --info --stacktrace 2>&1 | tee logs/test_output.log else ./gradlew test -Dspring.profiles.active=test fi env: LOGGING_LEVEL_ROOT: ${{ github.event_name == 'pull_request' && 'DEBUG' || 'INFO' }} LOGGING_LEVEL_ORG_SPRINGFRAMEWORK: ${{ github.event_name == 'pull_request' && 'DEBUG' || 'INFO' }}This way, detailed logging is only enabled for pull requests, potentially saving time on other workflow runs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- .github/workflows/opened-pr-test.yml (2 hunks)
- BE-hubo-gillajabi-resources (1 hunks)
- src/main/java/com/hubo/gillajabi/review/domain/entity/PostSearchDocument.java (5 hunks)
- src/test/java/com/hubo/gillajabi/image/domain/service/ImageValidationServiceTest.java (5 hunks)
- src/test/java/com/hubo/gillajabi/point/application/resolver/UserPointSearchResolverTest.java (1 hunks)
- src/test/java/com/hubo/gillajabi/point/fixture/UserPointFixture.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- BE-hubo-gillajabi-resources
- src/main/java/com/hubo/gillajabi/review/domain/entity/PostSearchDocument.java
- src/test/java/com/hubo/gillajabi/image/domain/service/ImageValidationServiceTest.java
- src/test/java/com/hubo/gillajabi/point/application/resolver/UserPointSearchResolverTest.java
- src/test/java/com/hubo/gillajabi/point/fixture/UserPointFixture.java
🧰 Additional context used
🪛 actionlint
.github/workflows/opened-pr-test.yml
78-78: shellcheck reported issue in this script: SC2046:warning:1:13: Quote this to prevent word splitting
(shellcheck)
83-83: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
100-100: shellcheck reported issue in this script: SC2086:info:8:6: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/opened-pr-test.yml
[error] 61-61: trailing spaces
(trailing-spaces)
[error] 117-117: trailing spaces
(trailing-spaces)
[error] 120-120: trailing spaces
(trailing-spaces)
[error] 123-123: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 129-129: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/opened-pr-test.yml (2)
215-228: Excellent addition of log artifact uploadsThe new steps to upload test and Gradle logs as artifacts are a valuable addition to the workflow.
These changes will greatly improve the ability to debug failed tests by providing easy access to detailed logs. The use of
if: always()ensures that logs are uploaded even if the tests fail, which is crucial for troubleshooting.Good job on enhancing the observability of your CI process!
Line range hint
1-245: Overall improvements to the CI workflow with some areas for refinementThe changes to this workflow file significantly enhance the test environment setup and improve observability. Key improvements include:
- Addition of services (MySQL, Redis, Elasticsearch, MongoDB) for a more comprehensive test environment.
- Detailed setup and verification steps for each service.
- Enhanced logging and artifact uploads for better debugging capabilities.
While these changes are generally positive, there are some areas for further improvement:
- Security: Use separate credentials for different services and consider using non-root users where possible.
- Efficiency: Some setup steps could be optimized or combined.
- Code cleanliness: Remove commented-out code related to the old remote testing setup.
- Internationalization: Use English for all log messages to ensure clarity for international teams.
Addressing these points will further enhance the robustness and maintainability of your CI process.
🧰 Tools
🪛 actionlint
78-78: shellcheck reported issue in this script: SC2046:warning:1:13: Quote this to prevent word splitting
(shellcheck)
83-83: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
100-100: shellcheck reported issue in this script: SC2086:info:8:6: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 61-61: trailing spaces
(trailing-spaces)
[error] 117-117: trailing spaces
(trailing-spaces)
[error] 120-120: trailing spaces
(trailing-spaces)
[error] 123-123: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 129-129: trailing spaces
(trailing-spaces)
| options: >- | ||
| --health-cmd "mongosh --eval 'db.runCommand({ping:1})'" | ||
| --health-interval 10s | ||
| --health-timeout 5s | ||
| --health-retries 5 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more portable health check for MongoDB
The current health check uses mongosh, which might not be available in all environments. A more portable solution would be to use the mongo command-line tool, which is more commonly available.
Update the MongoDB health check to use mongo:
options: >-
--health-cmd "mongo --eval 'db.runCommand({ping:1})'"
--health-interval 10s
--health-timeout 5s
--health-retries 5This change ensures better compatibility across different environments.
| - name: Install Elasticsearch plugin | ||
| run: | | ||
| docker exec $(docker ps -q --filter "name=elasticsearch") \ | ||
| /usr/share/elasticsearch/bin/elasticsearch-plugin install --batch analysis-nori |
There was a problem hiding this comment.
Fix potential issue in Elasticsearch plugin installation
The current plugin installation step uses command substitution without quotes, which could lead to issues if the container name contains spaces or special characters.
Update the plugin installation command to use proper quoting:
- name: Install Elasticsearch plugin
run: |
docker exec "$(docker ps -q --filter "name=elasticsearch")" \
/usr/share/elasticsearch/bin/elasticsearch-plugin install --batch analysis-noriThis change ensures that the command works correctly regardless of the container name format.
🧰 Tools
🪛 actionlint
78-78: shellcheck reported issue in this script: SC2046:warning:1:13: Quote this to prevent word splitting
(shellcheck)
.github/workflows/opened-pr-test.yml
Outdated
| # - name: Start test environment on remote server | ||
| # uses: appleboy/ssh-action@master | ||
| # with: | ||
| # host: ${{ secrets.BE_HOST }} | ||
| # username: ${{ secrets.BE_USER }} | ||
| # port: ${{ secrets.BE_PORT }} | ||
| # password: ${{ secrets.BE_PW }} | ||
| # script: | | ||
| # cd ~/hubo/test | ||
| # ./start-test-docker-compose.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out remote server actions
The presence of commented-out code for remote server actions indicates a shift in testing strategy from remote to local testing. Keeping unused code, even as comments, can lead to confusion and make the workflow file harder to maintain.
Consider removing these commented-out sections entirely to improve the clarity and maintainability of the workflow file. If there's a possibility that you might need these steps in the future, consider moving them to a separate document or creating a git branch with these changes for future reference.
Also applies to: lines 229-239
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
src/test/java/com/hubo/gillajabi/image/domain/service/ImageValidationServiceTest.java (4)
31-39: Approve changes with a minor suggestionThe test method has been correctly updated to focus on validation without deletion, aligning with the new
validateImageUrlmethod. The use ofassertDoesNotThrowis appropriate for this scenario.Consider updating the test method name to match the new functionality:
- void 단일_이미지_url_검증_및_삭제_성공() { + void 단일_이미지_url_검증_성공() {
58-69: Approve changes with suggestions for consistencyThe test has been updated to reflect the new
deleteImageUrlsfunctionality. The verification steps now correctly usedeleteById, which aligns with the changes in the repository.For better consistency, consider updating the test method name and display name to remove references to validation:
- @DisplayName("여러 이미지 URL 검증 및 삭제 - 성공") + @DisplayName("여러 이미지 URL 삭제 - 성공") - void 여러_이미지_URL_검증_및_삭제_성공() { + void 여러_이미지_URL_삭제_성공() {
73-74: Approve new test with suggestions for consistencyThe addition of a separate test for single image URL deletion improves test coverage. However, there's an inconsistency between the display name and the method name.
Please update the method name to match the display name for consistency:
@DisplayName("단일 이미지 URL 삭제") - void 여러_이미지_URL_삭제_실패(){ + void 단일_이미지_URL_삭제_성공(){
76-83: Approve implementation with suggestion for additional testingThe implementation correctly tests the single image URL deletion functionality. The use of
deleteByIdin the verification step is consistent with other tests.Consider adding a test case for the failure scenario, as the current test only covers the success case. This could involve mocking a scenario where the deletion fails and verifying the appropriate exception is thrown.
Example:
@Test @DisplayName("단일 이미지 URL 삭제 - 실패") void 단일_이미지_URL_삭제_실패(){ // given String imageUrl = "http://example.com/non-existent-image.jpg"; doThrow(new RuntimeException("Deletion failed")).when(imageUploadUrlRepository).deleteById(imageUrl); // when & then assertThrows(RuntimeException.class, () -> imageValidationService.deleteImageUrl(imageUrl)); verify(imageUploadUrlRepository, times(1)).deleteById(imageUrl); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .github/workflows/opened-pr-test.yml (2 hunks)
- BE-hubo-gillajabi-resources (1 hunks)
- src/test/java/com/hubo/gillajabi/config/FixtureBase.java (1 hunks)
- src/test/java/com/hubo/gillajabi/image/domain/service/ImageValidationServiceTest.java (2 hunks)
- src/test/java/com/hubo/gillajabi/point/application/resolver/UserPointSearchResolverTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- BE-hubo-gillajabi-resources
- src/test/java/com/hubo/gillajabi/point/application/resolver/UserPointSearchResolverTest.java
🧰 Additional context used
🪛 actionlint
.github/workflows/opened-pr-test.yml
78-78: shellcheck reported issue in this script: SC2046:warning:1:13: Quote this to prevent word splitting
(shellcheck)
83-83: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
100-100: shellcheck reported issue in this script: SC2086:info:8:6: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/opened-pr-test.yml
[error] 61-61: trailing spaces
(trailing-spaces)
[error] 117-117: trailing spaces
(trailing-spaces)
[error] 120-120: trailing spaces
(trailing-spaces)
[error] 123-123: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 129-129: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
src/test/java/com/hubo/gillajabi/config/FixtureBase.java (1)
38-44: LGTM: Repository retrieval methods are well-implemented.The
getRepositoryandgetCrudRepositorymethods are well-designed:
- They provide flexible access to different types of repositories.
- The use of generics ensures type safety.
- The protected access modifier allows proper use by subclasses.
These methods will be very useful for setting up test fixtures.
src/test/java/com/hubo/gillajabi/image/domain/service/ImageValidationServiceTest.java (1)
51-51: Approve method name updateThe method call has been correctly updated to
validateImageUrl, which aligns with the changes in theImageValidationServiceclass. This change appropriately focuses the test on validation without deletion..github/workflows/opened-pr-test.yml (2)
33-60: LGTM! Redis and MongoDB setups are well-configuredThe Redis and MongoDB service configurations are properly set up. The MongoDB health check using
mongoshis a good practice for ensuring the service is ready before running tests.
195-207: LGTM! Artifact upload steps are well-configuredThe artifact upload steps for test logs and Gradle logs are properly set up. Using the
if: always()condition ensures that these logs are uploaded even if the test step fails, which is crucial for debugging. Good job on following best practices for CI/CD workflows.
| package com.hubo.gillajabi.config; | ||
|
|
||
| import com.hubo.gillajabi.login.application.dto.response.TokenResponse; | ||
| import com.hubo.gillajabi.login.domain.constant.RoleStatus; | ||
| import com.hubo.gillajabi.login.domain.entity.MemberAuthentication; | ||
| import com.hubo.gillajabi.login.infrastructure.security.TokenProvider; | ||
| import com.hubo.gillajabi.member.domain.entity.Member; | ||
| import com.hubo.gillajabi.member.infrastructure.persistence.MemberRepository; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.context.ApplicationContext; | ||
| import org.springframework.context.annotation.ComponentScan; | ||
| import org.springframework.context.annotation.Configuration; | ||
| import org.springframework.context.annotation.FilterType; | ||
| import org.springframework.context.annotation.Profile; | ||
| import org.springframework.data.jpa.repository.JpaRepository; | ||
| import org.springframework.data.repository.CrudRepository; | ||
|
|
||
| import java.time.LocalDateTime; | ||
|
|
||
|
|
||
| /* | ||
| repository 의존성을 넣는 abstract class 입니다 | ||
| ApplicationContext, TokenProvider를 함께 받습니다. | ||
| */ | ||
| @Profile("test") | ||
| @Configuration | ||
| @ComponentScan(basePackages = "com.hubo.gillajabi", | ||
| useDefaultFilters = false, | ||
| includeFilters = @ComponentScan.Filter(type = FilterType.ASSIGNABLE_TYPE, value = FixtureBase.class)) | ||
| public abstract class FixtureBase { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider broadening the @ComponentScan configuration.
The current @ComponentScan configuration is very specific, only including the FixtureBase class itself. This might limit the usefulness of the configuration. Consider if you need to include other classes or use a broader scanning strategy to make this configuration more flexible and reusable.
You might want to adjust the @ComponentScan annotation to include other relevant classes or packages. For example:
@ComponentScan(basePackages = "com.hubo.gillajabi.config",
useDefaultFilters = false,
includeFilters = @ComponentScan.Filter(type = FilterType.ASSIGNABLE_TYPE,
classes = {FixtureBase.class, OtherRelevantClass.class}))This would allow you to include other classes that might be relevant for your test fixtures.
| public MemberAuthentication createAndLoadMember(String username, RoleStatus roleStatus) { | ||
| MemberRepository memberRepository = getRepository(MemberRepository.class); | ||
|
|
||
| Member member = Member.builder() | ||
| .nickName(username) | ||
| .profileImageUrl("profile.com") | ||
| .lastLoginAt(LocalDateTime.now()) | ||
| .build(); | ||
| MemberAuthentication memberAuthentication = MemberAuthentication.builder() | ||
| .member(member) | ||
| .roleStatus(roleStatus) | ||
| .build(); | ||
|
|
||
| memberRepository.save(member); | ||
|
|
||
| return memberAuthentication; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making lastLoginAt configurable in createAndLoadMember.
The createAndLoadMember method is well-implemented, but setting lastLoginAt to the current time might cause issues in some tests where you need to control this value.
Consider modifying the method signature to accept a LocalDateTime parameter for lastLoginAt:
public MemberAuthentication createAndLoadMember(String username, RoleStatus roleStatus, LocalDateTime lastLoginAt) {
// ...
Member member = Member.builder()
.nickName(username)
.profileImageUrl("profile.com")
.lastLoginAt(lastLoginAt)
.build();
// ...
}This change would allow more flexibility in testing scenarios where the last login time is relevant.
| public TokenResponse createAndLoadTokenResponse(MemberAuthentication memberAuthentication) { | ||
| TokenProvider tokenProvider = context.getBean(TokenProvider.class); | ||
| return tokenProvider.createToken(memberAuthentication); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the injected TokenProvider instead of retrieving it from the context.
The createAndLoadTokenResponse method is retrieving the TokenProvider from the context, but there's already an injected TokenProvider field in the class.
Modify the method to use the injected TokenProvider:
public TokenResponse createAndLoadTokenResponse(MemberAuthentication memberAuthentication) {
return tokenProvider.createToken(memberAuthentication);
}This change will make the code more consistent and slightly more efficient by avoiding the context lookup.
| mysql: | ||
| image: mysql:8.0.33 | ||
| env: | ||
| MYSQL_DATABASE: ${{ secrets.MYSQL_DATABASE }} | ||
| MYSQL_ROOT_HOST: '%' | ||
| MYSQL_ROOT_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }} | ||
| TZ: Asia/Seoul | ||
| MYSQL_CHARACTER_SET_SERVER: utf8mb4 | ||
| MYSQL_COLLATION_SERVER: utf8mb4_unicode_ci | ||
| ports: | ||
| - 3306:3306 | ||
| options: >- | ||
| --health-cmd="mysqladmin ping" | ||
| --health-interval=10s | ||
| --health-timeout=5s | ||
| --health-retries=3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a non-root user for MySQL in the test environment
While using the root user is convenient for testing, it's a good practice to use a dedicated user with limited privileges, even in test environments. This helps to catch potential permission issues early and aligns better with security best practices.
Consider creating a dedicated test user for MySQL:
env:
MYSQL_DATABASE: ${{ secrets.MYSQL_DATABASE }}
MYSQL_USER: ${{ secrets.MYSQL_TEST_USER }}
MYSQL_PASSWORD: ${{ secrets.MYSQL_TEST_PASSWORD }}
MYSQL_ROOT_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }}Then, update your application configuration to use these non-root credentials.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/opened-pr-test.yml (2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/opened-pr-test.yml
78-78: shellcheck reported issue in this script: SC2046:warning:1:13: Quote this to prevent word splitting
(shellcheck)
83-83: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
100-100: shellcheck reported issue in this script: SC2086:info:8:6: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/opened-pr-test.yml
[error] 61-61: trailing spaces
(trailing-spaces)
[error] 117-117: trailing spaces
(trailing-spaces)
[error] 120-120: trailing spaces
(trailing-spaces)
[error] 123-123: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 129-129: trailing spaces
(trailing-spaces)
| services: | ||
| mysql: | ||
| image: mysql:8.0.33 | ||
| env: | ||
| MYSQL_DATABASE: ${{ secrets.MYSQL_DATABASE }} | ||
| MYSQL_ROOT_HOST: '%' | ||
| MYSQL_ROOT_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }} | ||
| TZ: Asia/Seoul | ||
| MYSQL_CHARACTER_SET_SERVER: utf8mb4 | ||
| MYSQL_COLLATION_SERVER: utf8mb4_unicode_ci | ||
| ports: | ||
| - 3306:3306 | ||
| options: >- | ||
| --health-cmd="mysqladmin ping" | ||
| --health-interval=10s | ||
| --health-timeout=5s | ||
| --health-retries=3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a non-root user for MySQL in the test environment
While using the root user is convenient for testing, it's a good practice to use a dedicated user with limited privileges, even in test environments. This helps to catch potential permission issues early and aligns better with security best practices.
Consider creating a dedicated test user for MySQL:
env:
MYSQL_DATABASE: ${{ secrets.MYSQL_DATABASE }}
MYSQL_USER: ${{ secrets.MYSQL_TEST_USER }}
MYSQL_PASSWORD: ${{ secrets.MYSQL_TEST_PASSWORD }}
MYSQL_ROOT_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }}Then, update your application configuration to use these non-root credentials.
| elasticsearch: | ||
| image: docker.elastic.co/elasticsearch/elasticsearch:8.5.3 | ||
| env: | ||
| discovery.type: single-node | ||
| xpack.security.enabled: true | ||
| ELASTIC_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }} |
There was a problem hiding this comment.
Use a separate secret for Elasticsearch password
Currently, the Elasticsearch password is set to the MySQL root password. This practice reduces security by reusing credentials across different services.
Create a separate secret for the Elasticsearch password and update the configuration:
env:
discovery.type: single-node
xpack.security.enabled: true
ELASTIC_PASSWORD: ${{ secrets.ELASTICSEARCH_PASSWORD }}Don't forget to add the new ELASTICSEARCH_PASSWORD secret to your GitHub repository secrets.
| - name: Wait for Elasticsearch | ||
| run: | | ||
| timeout 600 bash -c ' | ||
| while ! curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} -o /dev/null -w "%{http_code}" http://127.0.0.1:9200 | grep -q "200"; do | ||
| echo "Waiting for Elasticsearch..." | ||
| sleep 5 | ||
| done | ||
| ' | ||
|
|
||
| - name: Verify Elasticsearch Health | ||
| run: | | ||
| curl -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cluster/health | ||
|
|
||
| - name: Install Elasticsearch plugin | ||
| run: | | ||
| docker exec $(docker ps -q --filter "name=elasticsearch") \ | ||
| /usr/share/elasticsearch/bin/elasticsearch-plugin install --batch analysis-nori | ||
|
|
||
| - name: Restart Elasticsearch | ||
| run: | | ||
| docker restart $(docker ps -q --filter "name=elasticsearch") | ||
|
|
||
| - name: Wait for Elasticsearch after restart | ||
| run: | | ||
| timeout 600 bash -c ' | ||
| until curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cluster/health | grep -q "status\":\"[green|yellow]"; do | ||
| echo "Waiting for Elasticsearch to be operational..." | ||
| sleep 15 | ||
| done | ||
| ' | ||
|
|
||
| - name: Verify Elasticsearch Health | ||
| run: | | ||
| curl -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cluster/health | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Elasticsearch setup and verification process
The current Elasticsearch setup and verification process is thorough but can be streamlined for efficiency. Consider combining the wait, plugin installation, restart, and health check steps into a single step:
- name: Setup and verify Elasticsearch
run: |
# Wait for Elasticsearch to be ready
timeout 600 bash -c '
until curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} -o /dev/null -w "%{http_code}" http://localhost:9200 | grep -q "200"; do
echo "Waiting for Elasticsearch..."
sleep 5
done
'
# Install plugin
docker exec "$(docker ps -q --filter "name=elasticsearch")" \
/usr/share/elasticsearch/bin/elasticsearch-plugin install --batch analysis-nori
# Restart Elasticsearch
docker restart "$(docker ps -q --filter "name=elasticsearch")"
# Wait for Elasticsearch to be operational after restart
timeout 600 bash -c '
until curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cluster/health | grep -q "status\":\"[green|yellow]"; do
echo "Waiting for Elasticsearch to be operational..."
sleep 15
done
'
# Verify final health
curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cluster/health | jq .This combined step reduces the number of separate actions while maintaining the necessary checks and setup procedures.
🧰 Tools
🪛 actionlint
78-78: shellcheck reported issue in this script: SC2046:warning:1:13: Quote this to prevent word splitting
(shellcheck)
83-83: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
| - name: MySQL connection health check | ||
| run: | | ||
| for i in {30..0}; do | ||
| if mysqladmin ping -h 127.0.0.1 --silent; then | ||
| break | ||
| fi | ||
| echo 'MySQL을 기다리는 중...' | ||
| sleep 1 | ||
| done | ||
| if [ $i -eq 0 ]; then | ||
| echo 'MySQL이 30초 내에 시작되지 않았습니다.' | ||
| exit 1 | ||
| fi | ||
|
|
||
| - name: Verify MySQL connection and configure access | ||
| run: | | ||
| echo "Showing initial databases:" | ||
| mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "SHOW DATABASES;" || echo "Failed to show initial databases" | ||
|
|
||
| echo "Creating hubo database:" | ||
| mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "CREATE DATABASE IF NOT EXISTS hubo;" || echo "Failed to create hubo database" | ||
|
|
||
| echo "Creating root user:" | ||
| mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "CREATE USER IF NOT EXISTS 'root'@'%' IDENTIFIED BY '${{ secrets.MYSQL_ROOT_PASSWORD }}';" || echo "Failed to create root user" | ||
|
|
||
| echo "Granting privileges:" | ||
| mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "GRANT ALL PRIVILEGES ON *.* TO 'root'@'%' WITH GRANT OPTION;" || echo "Failed to grant privileges" | ||
|
|
||
| echo "Flushing privileges:" | ||
| mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "FLUSH PRIVILEGES;" || echo "Failed to flush privileges" | ||
|
|
||
| echo "Showing final databases:" | ||
| mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "SHOW DATABASES;" || echo "Failed to show final databases" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize MySQL setup and use English for messages
The current MySQL setup process is thorough but can be optimized. Additionally, using English for messages ensures better understanding across international teams.
Consider the following improvements:
- Use English for messages:
- echo 'MySQL을 기다리는 중...'
+ echo 'Waiting for MySQL...'
- echo 'MySQL이 30초 내에 시작되지 않았습니다.'
+ echo 'MySQL did not start within 30 seconds.'- Combine MySQL operations into a single connection for efficiency:
- name: Setup and verify MySQL
run: |
# Wait for MySQL
timeout 30 bash -c 'until mysqladmin ping -h"127.0.0.1" -P3306 -uroot -p"${{ secrets.MYSQL_ROOT_PASSWORD }}" --silent; do echo "Waiting for MySQL..."; sleep 1; done'
# Configure MySQL
mysql -h127.0.0.1 -P3306 -uroot -p"${{ secrets.MYSQL_ROOT_PASSWORD }}" << EOF
SHOW DATABASES;
CREATE DATABASE IF NOT EXISTS hubo;
CREATE USER IF NOT EXISTS 'root'@'%' IDENTIFIED BY '${{ secrets.MYSQL_ROOT_PASSWORD }}';
GRANT ALL PRIVILEGES ON *.* TO 'root'@'%' WITH GRANT OPTION;
FLUSH PRIVILEGES;
SHOW DATABASES;
EOFThese changes improve efficiency, readability, and internationalization of the MySQL setup process.
🧰 Tools
🪛 actionlint
100-100: shellcheck reported issue in this script: SC2086:info:8:6: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 117-117: trailing spaces
(trailing-spaces)
[error] 120-120: trailing spaces
(trailing-spaces)
[error] 123-123: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 129-129: trailing spaces
(trailing-spaces)
| - name: Debug Elasticsearch before tests | ||
| run: | | ||
| echo "Checking Elasticsearch status before running tests..." | ||
| curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cluster/health | jq . | ||
| echo "Listing Elasticsearch indices..." | ||
| curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cat/indices?v | ||
| echo "Checking Elasticsearch plugins..." | ||
| curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cat/plugins?v | ||
| echo "Checking Elasticsearch settings..." | ||
| curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cluster/settings?pretty | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security in Elasticsearch debug output
While the debug information is valuable for troubleshooting, it might expose sensitive data in CI logs.
Consider the following improvements:
- Mask sensitive information in the output:
- name: Debug Elasticsearch before tests
run: |
echo "Checking Elasticsearch status before running tests..."
curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cluster/health | jq 'del(.cluster_name, .cluster_uuid)' | sed 's/\("password":\s*\)"[^"]*"/\1"[REDACTED]"/'
echo "Listing Elasticsearch indices..."
curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cat/indices?v | sed 's/\(index\s*\)[^ ]*/\1[REDACTED]/'
echo "Checking Elasticsearch plugins..."
curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cat/plugins?v
echo "Checking Elasticsearch settings..."
curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cluster/settings?pretty | sed 's/\("password":\s*\)"[^"]*"/\1"[REDACTED]"/'- Consider moving detailed debug information to a separate step that only runs on failure or when explicitly requested.
These changes help protect sensitive information while still providing valuable debug output when needed.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
src/test/java/com/hubo/gillajabi/point/application/presenation/UserPointControllerTest.java (2)
82-115: Consider extracting the request JSON to a constant or helper methodThe test method is well-structured and comprehensive. However, the request JSON is defined inline, which can make the test less readable, especially for larger JSON payloads.
Consider extracting the request JSON to a constant or a helper method. This would improve readability and make it easier to reuse the same JSON structure in other tests if needed. Here's an example of how you could refactor this:
public class UserPointControllerTest { + private static final String VALID_USER_POINT_JSON = """ + { + "content": "여기는 좋은 장소입니다.", + "latitude": 37.5665, + "longitude": 126.9780, + "imageUrls": [ + "https://example.com/image1.jpg" + ] + } + """.replace("\n", "").replace("\r", ""); @Test void 사용자_포인트_저장_성공() throws Exception { // given - String requestJson = """ - { - "content": "여기는 좋은 장소입니다.", - "latitude": 37.5665, - "longitude": 126.9780, - "imageUrls": [ - "https://example.com/image1.jpg" - ] - } - """.replace("\n", "").replace("\r", ""); ImageUploadUrl imageUploadUrl = userPointFixture.createImageUploadUrl("https://example.com/image1.jpg"); // ... rest of the test method ... mockMvc.perform( post("/api/user-point") .header("Authorization", "Bearer " + tokenResponse.accessToken()) .contentType(MediaType.APPLICATION_JSON) - .content(requestJson)) + .content(VALID_USER_POINT_JSON)) .andDo(print()) .andExpect(status().isCreated()); // ... rest of the test method ... } }This change improves the readability of the test method and makes it easier to maintain the JSON structure across multiple tests if needed.
149-180: Remove unused variableThe test method is well-structured and clear. However, there's an unused variable
imageUploadUrlthat can be removed to improve code cleanliness.Consider removing the unused
imageUploadUrlvariable:@Test void 사용자가_트래킹_중이_아닌_경우() throws Exception { // given String requestJson = """ { "content": "여기는 좋은 장소입니다.", "latitude": 37.5665, "longitude": 126.9780, "imageUrls": [ "https://example.com/image1.jpg" ] } """.replace("\n", "").replace("\r", ""); MemberAuthentication memberAuthentication = userPointFixture.createAndLoadMember("testUser", RoleStatus.USER); TokenResponse tokenResponse = userPointFixture.createAndLoadTokenResponse(memberAuthentication); - ImageUploadUrl imageUploadUrl = userPointFixture.createImageUploadUrl("https://example.com/image1.jpg"); // ... rest of the test method ... }This change removes the unused variable, making the test more concise and easier to maintain.
src/test/java/com/hubo/gillajabi/point/fixture/UserPointFixture.java (1)
213-214: Consider removing commented-out code or explain its necessity.The commented-out code might not be needed anymore. Removing it can improve code readability. If it's still required for future reference or debugging, consider adding a comment explaining why it's kept.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- src/main/java/com/hubo/gillajabi/point/domain/entity/UserPoint.java (1 hunks)
- src/main/java/com/hubo/gillajabi/point/domain/service/UserPointService.java (1 hunks)
- src/main/java/com/hubo/gillajabi/track/application/dto/request/TrackStartRequest.java (1 hunks)
- src/main/java/com/hubo/gillajabi/track/domain/entity/TrackRecord.java (4 hunks)
- src/main/java/com/hubo/gillajabi/track/domain/entity/TrackStatus.java (1 hunks)
- src/test/java/com/hubo/gillajabi/point/application/presenation/UserPointControllerTest.java (1 hunks)
- src/test/java/com/hubo/gillajabi/point/fixture/UserPointFixture.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/hubo/gillajabi/point/domain/entity/UserPoint.java
- src/main/java/com/hubo/gillajabi/point/domain/service/UserPointService.java
🔇 Additional comments (7)
src/main/java/com/hubo/gillajabi/track/domain/entity/TrackStatus.java (1)
39-41: Approved: Improved parameter naming increateByMemberAndTrackIdmethodThe change from
idtotrackIdin both the method signature and its usage within the method body enhances code clarity and maintainability. This modification better reflects the purpose of the parameter and improves overall readability.To ensure this change doesn't introduce any issues, please verify that all calls to this method have been updated accordingly. Run the following script to check for any remaining usages of the old method signature:
If the script returns any results, those locations may need to be updated to use the new
trackIdparameter name.src/main/java/com/hubo/gillajabi/track/domain/entity/TrackRecord.java (4)
7-7: LGTM: Import statements updated appropriately.The changes to the import statements are appropriate:
- The addition of
java.util.ArrayListis necessary for the new implementation of thephotoPointsfield.- The more specific Lombok imports improve code clarity and reduce potential conflicts.
Also applies to: 11-11
19-19: LGTM: Builder pattern added to TrackRecord class.The addition of the
@Builderannotation is appropriate and consistent with the changes made to thephotoPointsfield and thecreateByMembermethod. This will provide a more flexible way to createTrackRecordinstances.
37-38: LGTM: Improved initialization of photoPoints field.The changes to the
photoPointsfield are well-implemented:
- The
@Builder.Defaultannotation ensures that the default value is used when the builder pattern is employed.- Initializing with an empty ArrayList prevents potential null pointer exceptions and is a good practice.
- This change is consistent with the addition of the
@Builderannotation to the class.
Line range hint
1-86: Overall assessment: Good improvements with minor suggestions.The changes to the
TrackRecordclass are generally positive:
- Improved import statements for better clarity.
- Added
@Builderannotation for more flexible instance creation.- Better initialization of the
photoPointsfield to prevent null pointer exceptions.- New
addPhotoPointmethod for adding multiple photo points.The suggested improvements to the
addPhotoPointmethod will further enhance the robustness and consistency of the class. Once these are addressed, the changes will significantly improve the overall quality of theTrackRecordclass.src/test/java/com/hubo/gillajabi/point/application/presenation/UserPointControllerTest.java (2)
182-201: Well-structured test for user point deletionThis test method is well-implemented and covers the successful scenario for user point deletion. It properly sets up the test data, performs the deletion operation, and verifies the result.
The test method is comprehensive and follows good testing practices. It creates the necessary test data, performs the deletion operation, and then verifies that the point has been removed from the database. Keep up the good work!
224-247: Well-implemented test for unauthorized point deletionThis test method effectively covers the scenario where a user attempts to delete a point that doesn't belong to them. It properly sets up two different users, creates a point for one user, and then attempts to delete it with the other user's credentials.
The test is well-structured and comprehensive. It correctly:
- Sets up two different users
- Creates a point for one user
- Attempts to delete the point using the other user's credentials
- Verifies that the operation fails with a 403 Forbidden status
- Checks the error message
This approach ensures that the system properly enforces access control for point deletion. Good job on covering this important security aspect!
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| @Getter | |||
| @Setter | |||
| @AllArgsConstructor(access = AccessLevel.PROTECTED) | |||
| @AllArgsConstructor | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Constructor Access Levels for DTOs
The @AllArgsConstructor annotation in TrackStartRequest.java has been changed to use the default public access level. However, the shell script analysis reveals that other DTOs within the codebase have varying access levels specified for @AllArgsConstructor. This inconsistency can lead to unintended instantiation from outside the package and may violate encapsulation principles.
- Recommendation:
- Standardize the access levels for all
@AllArgsConstructorannotations across DTOs. If public access is intended, explicitly specify@AllArgsConstructor(access = AccessLevel.PUBLIC)to enhance code readability and maintain consistency. - Review other DTO classes to ensure that their constructor access levels align with the project's design principles and encapsulation requirements.
- Standardize the access levels for all
🔗 Analysis chain
Consider the implications of changing the @AllArgsConstructor access level.
The @AllArgsConstructor annotation has been modified to remove the AccessLevel.PROTECTED specification. This change makes the all-args constructor public by default, which could have several implications:
- It increases the visibility of the constructor, potentially allowing unintended instantiation from outside the package.
- It may conflict with the principle of encapsulation, as DTOs are often designed to be immutable or have controlled construction.
- It could lead to inconsistencies with other similar DTOs in the codebase if they follow a different pattern.
Consider whether this change is intentional and aligns with your project's design principles. If the intention is to allow public access, it might be clearer to explicitly specify @AllArgsConstructor(access = AccessLevel.PUBLIC) for better code readability and to show that this is a deliberate choice.
To assess the impact and consistency of this change, let's verify the usage of @AllArgsConstructor across the codebase:
This will help us understand if this change is consistent with other DTOs and how it might affect the usage of TrackStartRequest.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of @AllArgsConstructor in other DTOs
# Search for @AllArgsConstructor usage in Java files
echo "Usage of @AllArgsConstructor in the codebase:"
rg --type java "@AllArgsConstructor" -C 2
# Check for other classes in the same package
echo "\nOther classes in the same package:"
fd -e java . "src/main/java/com/hubo/gillajabi/track/application/dto/request"
# Check for usage of TrackStartRequest
echo "\nUsage of TrackStartRequest:"
rg "TrackStartRequest" -t java -C 2
Length of output: 40018
| @@ -52,7 +52,7 @@ public class TrackRecord extends BaseEntity { | |||
| private Long calorie; | |||
|
|
|||
| public static TrackRecord createByMember(final Member member, final Course course) { | |||
| return new TrackRecord(null, null, null, null, null, member, course, 0L,BigDecimal.ZERO,0L); | |||
| return new TrackRecord(null, null, null, null, new ArrayList<>(), member, course, 0L,BigDecimal.ZERO,0L); | |||
There was a problem hiding this comment.
Improvements needed for consistency and robustness.
The changes to createByMember and the addition of addPhotoPoint are good steps, but there are some areas for improvement:
-
In
createByMember, the initialization ofphotoPointswithnew ArrayList<>()is consistent with the field changes. This is good. -
For the new
addPhotoPointmethod:
a. The parameter namephotoPointis singular while it's a List. Consider renaming tophotoPointsfor clarity.
b. Add a null check on the input list to prevent potential NullPointerExceptions.
c. For consistency with other methods likeaddPhotoPoints, consider using thePhotoPoint.addTrackRecordutility method.
Consider refactoring the addPhotoPoint method as follows:
public void addPhotoPoint(List<PhotoPoint> photoPoints) {
if (photoPoints != null) {
this.photoPoints.addAll(photoPoints);
PhotoPoint.addTrackRecord(new HashSet<>(photoPoints), this);
}
}This change improves consistency with other methods, adds null safety, and maintains the bidirectional relationship between TrackRecord and PhotoPoint.
Also applies to: 84-86
| @DisplayName("트래킹 중이 아닌경우 실패") | ||
| @Test | ||
| void 사용자가_트래킹_중이_아닌_경우() throws Exception { | ||
| // given | ||
| String requestJson = """ | ||
| { | ||
| "content": "여기는 좋은 장소입니다.", | ||
| "latitude": 37.5665, | ||
| "longitude": 126.9780, | ||
| "imageUrls": [ | ||
| "https://example.com/image1.jpg" | ||
| ] | ||
| } | ||
| """.replace("\n", "").replace("\r", ""); | ||
| MemberAuthentication memberAuthentication = userPointFixture.createAndLoadMember("testUser", RoleStatus.USER); | ||
| TokenResponse tokenResponse = userPointFixture.createAndLoadTokenResponse(memberAuthentication); | ||
| ImageUploadUrl imageUploadUrl = userPointFixture.createImageUploadUrl("https://example.com/image1.jpg"); | ||
|
|
||
| // when | ||
| String result = mockMvc.perform( | ||
| post("/api/user-point") | ||
| .header("Authorization", "Bearer " + tokenResponse.accessToken()) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content(requestJson)) | ||
| .andDo(print()) | ||
| .andExpect(status().isBadRequest()) | ||
| .andReturn().getResponse().getContentAsString(StandardCharsets.UTF_8); | ||
|
|
||
| // then | ||
| JsonNode rootNode = objectMapper.readTree(result); | ||
| assertThat(rootNode.get("message").asText()).isEqualTo("현재 트래킹중이 아닙니다."); | ||
| } | ||
|
|
||
| @DisplayName("사용자 포인트 삭제가 성공한다") | ||
| @Test | ||
| void 사용자_포인트_삭제_성공() throws Exception { | ||
| // given | ||
| MemberAuthentication memberAuthentication = userPointFixture.createAndLoadMember("testUser", RoleStatus.USER); | ||
| TokenResponse tokenResponse = userPointFixture.createAndLoadTokenResponse(memberAuthentication); | ||
| UserPointDocument userPointDocument = userPointFixture.createAndLoadUserPointDocument(memberAuthentication.getMember()); | ||
|
|
||
| // when | ||
| mockMvc.perform( | ||
| delete("/api/user-point/{id}", userPointDocument.getUserPointId()) | ||
| .header("Authorization", "Bearer " + tokenResponse.accessToken()) | ||
| .contentType(MediaType.APPLICATION_JSON)) | ||
| .andDo(print()) | ||
| .andExpect(status().isNoContent()); | ||
|
|
||
| // then | ||
| List<UserPointDocument> userPointDocuments = userPointDocumentRepository.findAll(); | ||
| assertThat(userPointDocuments).isEmpty(); | ||
| } | ||
|
|
||
| @DisplayName("사용자 포인트 삭제 시 해당 사용자의 포인트가 없을 경우 실패한다") | ||
| @Test | ||
| void 사용자_포인트_삭제시_해당_사용자의_포인트가_없을_경우_실패한다() throws Exception { | ||
| // given | ||
| MemberAuthentication memberAuthentication = userPointFixture.createAndLoadMember("testUser", RoleStatus.USER); | ||
| TokenResponse tokenResponse = userPointFixture.createAndLoadTokenResponse(memberAuthentication); | ||
|
|
||
| // when | ||
| String result = mockMvc.perform( | ||
| delete("/api/user-point/{id}", 1L) | ||
| .header("Authorization", "Bearer " + tokenResponse.accessToken()) | ||
| .contentType(MediaType.APPLICATION_JSON)) | ||
| .andDo(print()) | ||
| .andExpect(status().isNotFound()) | ||
| .andReturn().getResponse().getContentAsString(StandardCharsets.UTF_8); | ||
|
|
||
| // then | ||
| JsonNode rootNode = objectMapper.readTree(result); | ||
| assertThat(rootNode.get("message").asText()).isEqualTo("포인트를 찾을 수 없습니다."); | ||
| } | ||
|
|
||
| @DisplayName("사용자 포인트 삭제시 해당 사용자의 포인트가 아닐 경우 실패한다") | ||
| @Test | ||
| void 사용자_포인트_삭제시_해당_사용자의_포인트가_아닐_경우_실패() throws Exception { | ||
| // given | ||
| MemberAuthentication memberAuthentication = userPointFixture.createAndLoadMember("testUser", RoleStatus.USER); | ||
| TokenResponse tokenResponse = userPointFixture.createAndLoadTokenResponse(memberAuthentication); | ||
| UserPointDocument userPointDocument = userPointFixture.createAndLoadUserPointDocument(memberAuthentication.getMember()); | ||
| MemberAuthentication otherMemberAuthentication = userPointFixture.createAndLoadMember("otherUser", RoleStatus.USER); | ||
| TokenResponse otherTokenResponse = userPointFixture.createAndLoadTokenResponse(otherMemberAuthentication); | ||
|
|
||
| // when | ||
| String result = mockMvc.perform( | ||
| delete("/api/user-point/{id}", userPointDocument.getUserPointId()) | ||
| .header("Authorization", "Bearer " + otherTokenResponse.accessToken()) | ||
| .contentType(MediaType.APPLICATION_JSON)) | ||
| .andDo(print()) | ||
| .andExpect(status().isForbidden()) | ||
| .andReturn().getResponse().getContentAsString(StandardCharsets.UTF_8); | ||
|
|
||
| // then | ||
| JsonNode rootNode = objectMapper.readTree(result); | ||
| assertThat(rootNode.get("message").asText()).isEqualTo("포인트 소유자가 아닙니다."); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using @nested classes for better test organization
The test class is comprehensive and covers various scenarios well. To further improve the organization and readability of the tests, consider using JUnit 5's @Nested annotation to group related tests together.
Restructure the test class using @Nested inner classes to group related tests. This can make the test suite more organized and easier to understand. Here's an example of how you could refactor:
@AutoConfigureMockMvc
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.MOCK)
@Transactional
@ActiveProfiles("test")
class UserPointControllerTest {
// ... existing setup code ...
@Nested
@DisplayName("User Point Creation Tests")
class UserPointCreationTests {
@Test
void 사용자_포인트_저장_성공() throws Exception {
// ... existing test code ...
}
@Test
void 사용자의_포인트_저장_요청에서_이미지가_유효하지_않을_경우() throws Exception {
// ... existing test code ...
}
@Test
void 사용자가_트래킹_중이_아닌_경우() throws Exception {
// ... existing test code ...
}
}
@Nested
@DisplayName("User Point Deletion Tests")
class UserPointDeletionTests {
@Test
void 사용자_포인트_삭제_성공() throws Exception {
// ... existing test code ...
}
@Test
void 사용자_포인트_삭제시_해당_사용자의_포인트가_없을_경우_실패한다() throws Exception {
// ... existing test code ...
}
@Test
void 사용자_포인트_삭제시_해당_사용자의_포인트가_아닐_경우_실패() throws Exception {
// ... existing test code ...
}
}
}This structure groups related tests together, making the test suite more organized and easier to navigate. It also allows for shared setup methods within each nested class if needed.
| @AfterEach | ||
| void cleanupAfterTest() { | ||
| mongoTemplate.getCollectionNames().forEach(collectionName -> { | ||
| mongoTemplate.dropCollection(collectionName); | ||
| }); | ||
|
|
||
| RedisServerCommands serverCommands = Objects.requireNonNull(redisTemplate.getConnectionFactory()) | ||
| .getConnection() | ||
| .serverCommands(); | ||
| serverCommands.flushAll(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using @DirtiesContext for test isolation
The current cleanup method manually drops MongoDB collections and flushes Redis. While this approach works, it might be more efficient and cleaner to use Spring's @DirtiesContext annotation. This annotation ensures a clean application context for each test method, which is particularly useful when dealing with embedded databases or in-memory data stores.
Consider replacing the cleanupAfterTest method with the @DirtiesContext annotation at the class level:
@AutoConfigureMockMvc
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.MOCK)
@Transactional
@ActiveProfiles("test")
+@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD)
public class UserPointControllerTest {
// ... existing code ...
- @AfterEach
- void cleanupAfterTest() {
- mongoTemplate.getCollectionNames().forEach(collectionName -> {
- mongoTemplate.dropCollection(collectionName);
- });
-
- RedisServerCommands serverCommands = Objects.requireNonNull(redisTemplate.getConnectionFactory())
- .getConnection()
- .serverCommands();
- serverCommands.flushAll();
- }This change will ensure that the application context is reset after each test method, providing a clean state without the need for manual cleanup.
📝 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.
| @AfterEach | |
| void cleanupAfterTest() { | |
| mongoTemplate.getCollectionNames().forEach(collectionName -> { | |
| mongoTemplate.dropCollection(collectionName); | |
| }); | |
| RedisServerCommands serverCommands = Objects.requireNonNull(redisTemplate.getConnectionFactory()) | |
| .getConnection() | |
| .serverCommands(); | |
| serverCommands.flushAll(); | |
| } | |
| @AutoConfigureMockMvc | |
| @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.MOCK) | |
| @Transactional | |
| @ActiveProfiles("test") | |
| @DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD) | |
| public class UserPointControllerTest { | |
| // ... existing code ... | |
| } |
| @DisplayName("사용자 포인트 삭제 시 해당 사용자의 포인트가 없을 경우 실패한다") | ||
| @Test | ||
| void 사용자_포인트_삭제시_해당_사용자의_포인트가_없을_경우_실패한다() throws Exception { | ||
| // given | ||
| MemberAuthentication memberAuthentication = userPointFixture.createAndLoadMember("testUser", RoleStatus.USER); | ||
| TokenResponse tokenResponse = userPointFixture.createAndLoadTokenResponse(memberAuthentication); | ||
|
|
||
| // when | ||
| String result = mockMvc.perform( | ||
| delete("/api/user-point/{id}", 1L) | ||
| .header("Authorization", "Bearer " + tokenResponse.accessToken()) | ||
| .contentType(MediaType.APPLICATION_JSON)) | ||
| .andDo(print()) | ||
| .andExpect(status().isNotFound()) | ||
| .andReturn().getResponse().getContentAsString(StandardCharsets.UTF_8); | ||
|
|
||
| // then | ||
| JsonNode rootNode = objectMapper.readTree(result); | ||
| assertThat(rootNode.get("message").asText()).isEqualTo("포인트를 찾을 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a non-existent ID consistently
The test method is well-structured and tests the correct scenario. However, the use of 1L as a non-existent ID might lead to false positives if an ID with value 1 happens to exist in the future.
Consider using a method to generate a guaranteed non-existent ID. This will make the test more robust and less likely to produce false positives. Here's a suggested change:
+ private Long getNonExistentPointId() {
+ return userPointDocumentRepository.findAll().stream()
+ .mapToLong(UserPointDocument::getUserPointId)
+ .max()
+ .orElse(0L) + 1;
+ }
@Test
void 사용자_포인트_삭제시_해당_사용자의_포인트가_없을_경우_실패한다() throws Exception {
// given
MemberAuthentication memberAuthentication = userPointFixture.createAndLoadMember("testUser", RoleStatus.USER);
TokenResponse tokenResponse = userPointFixture.createAndLoadTokenResponse(memberAuthentication);
+ Long nonExistentId = getNonExistentPointId();
// when
String result = mockMvc.perform(
- delete("/api/user-point/{id}", 1L)
+ delete("/api/user-point/{id}", nonExistentId)
.header("Authorization", "Bearer " + tokenResponse.accessToken())
.contentType(MediaType.APPLICATION_JSON))
.andDo(print())
.andExpect(status().isNotFound())
.andReturn().getResponse().getContentAsString(StandardCharsets.UTF_8);
// ... rest of the test method ...
}This change ensures that the test always uses a non-existent ID, making it more reliable and less prone to false positives.
Committable suggestion was skipped due to low confidence.
src/test/java/com/hubo/gillajabi/point/application/presenation/UserPointControllerTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/test/java/com/hubo/gillajabi/crawl/domain/service/duru/RoadGpxInfoDuruServiceTest.java (1)
22-22: Improved mock API response generation.The changes to the
mockApiResponseServicemethod enhance the test's reliability and predictability:
- The method signature now correctly declares
throws JsonProcessingException, aligning with the use ofXmlMapper.writeValueAsString().- Using
fixtureMonkey.giveMeBuilder()instead offixtureMonkey.giveMeOne()allows for more precise control over the generated test data.- Constraining the size of
trksegandtrkptlists makes the tests more focused and predictable.These improvements will lead to more reliable and targeted tests. Great job!
Consider extracting the magic numbers (1 and 2) used for list sizes into named constants for better readability and maintainability. For example:
private static final int TRKSEG_SIZE = 1; private static final int TRKPT_SIZE = 2; // ... .size("trk.trkseg", TRKSEG_SIZE) .size("trk.trkseg[*].trkpt", TRKPT_SIZE)This makes it easier to understand the intent behind these specific sizes and allows for easy modification if needed in the future.
Also applies to: 54-57
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/test/java/com/hubo/gillajabi/crawl/domain/service/duru/RoadGpxInfoDuruServiceTest.java (2 hunks)
- src/test/java/com/hubo/gillajabi/point/application/resolver/UserPointSearchResolverTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/hubo/gillajabi/point/application/resolver/UserPointSearchResolverTest.java
PR 이슈번호
추후 작성
추가 되거나 변경된 내용
논의 해 볼 내용
래퍼런스
Summary by CodeRabbit
New Features
UserPointSearchResolverclass, covering various scenarios for retrieving user points.FixtureBaseclass to facilitate test data setup and authentication token management.UserPointFixtureclass with additional methods for creating and managing user points and related entities.UserPointServiceclass for managing user points, including saving and deleting functionalities.UserPointControllerTestclass to validate endpoints related to user points.Bug Fixes
Chores