feature/7929/commit-info-dev #110
Conversation
WalkthroughThis pull request introduces enhancements for retrieving Git commit information in a Spring Boot application. Key changes include the addition of properties and dependencies for JGit and JSONPath, the creation of a new REST endpoint Changes
Sequence DiagramsequenceDiagram
Client->>CommitInfoController: GET /commit-info
CommitInfoController->>CommitInfoService: getLatestCommitInfo()
CommitInfoService->>GitRepository: Fetch latest commit
GitRepository-->>CommitInfoService: Return commit details
CommitInfoService-->>CommitInfoController: Return CommitInfoDto
CommitInfoController-->>Client: Respond with commit info
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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: 3
🔭 Outside diff range comments (1)
chat/pom.xml (1)
Line range hint
1-300: Update Java version to align with Spring Boot 3.xThe project uses Spring Boot 3.2.2 which requires Java 17+, but the maven-compiler-plugin is set to Java 10. This version mismatch could lead to runtime issues.
Apply this diff to update the Java version:
<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-compiler-plugin</artifactId> <configuration> - <source>10</source> - <target>10</target> + <source>17</source> + <target>17</target> </configuration> </plugin>
🧹 Nitpick comments (9)
chat/src/main/java/greencity/service/impl/CommitInfoServiceImpl.java (3)
37-40: Include Exception Details in Warning LogCurrently, when an
IOExceptionoccurs during repository initialization, only a generic warning message is logged. Including the exception details in the log can provide valuable information for debugging.Apply this diff to include the exception in the warning log:
- log.warn(ErrorMessage.WARNING_GIT_DIRECTORY_NOT_FOUND); + log.warn(ErrorMessage.WARNING_GIT_DIRECTORY_NOT_FOUND, e);
29-41: Consider Throwing an Exception if Repository Initialization FailsSetting the
repositorytonullwhen initialization fails could lead toNullPointerExceptionelsewhere if not properly checked. Consider throwing a custom exception in the constructor to handle the failure more explicitly.Alternatively, ensure all methods accessing
repositorycheck fornulland handle it appropriately, as done ingetLatestCommitInfo().
65-66: Handle Exceptions More SpecificallyWhen catching the
IOException, consider whether throwing aResourceNotFoundExceptionis the most appropriate action. AnIOExceptionmight indicate issues other than a missing resource, such as I/O errors.Consider defining a more specific exception or handling different types of exceptions separately to provide clearer error messaging to the client.
chat/src/main/java/greencity/dto/CommitInfoDto.java (1)
17-20: Consider adding size constraints and format validation.While the
@NotNullvalidation is good, consider:
- Adding
@Sizeconstraints to prevent extremely large values- Adding pattern validation for
commitDateto ensure it matches the expected format (dd/MM/yyyy HH:mm:ss)@NotNull + @Size(min = 40, max = 40) private String commitHash; @NotNull + @Size(max = 19) + @Pattern(regexp = "^\\d{2}/\\d{2}/\\d{4} \\d{2}:\\d{2}:\\d{2}$") private String commitDate;chat/src/main/java/greencity/constant/AppConstant.java (1)
4-5: Consider using UTC for timestamps.While using Ukraine timezone might be suitable for your current needs, consider:
- Using UTC for storing and transmitting timestamps to ensure consistency across different timezones
- Converting to local timezone only at the presentation layer
chat/src/test/java/greencity/controller/CommitInfoControllerTest.java (1)
41-41: Update the hard-coded date to use a dynamic or past date.The test uses a future date "16/12/2024" which might cause test failures. Consider using a past date or generating a dynamic date to make the test more maintainable.
- private static final String COMMIT_DATE = "16/12/2024 12:06:32"; + private static final String COMMIT_DATE = "16/12/2023 12:06:32";chat/src/main/java/greencity/exception/handlers/CustomExceptionHandler.java (1)
145-145: Consider using consistent logging level across exception handlers.Other exception handlers in this class use
log.trace(), but this handler useslog.error(). Consider using the same logging level for consistency unless there's a specific reason for the difference.- log.error(ex.getMessage(), ex); + log.trace(ex.getMessage(), ex);chat/src/test/java/greencity/service/impl/CommitInfoServiceImplTest.java (1)
116-116: Update the hard-coded date to use a past date.The test uses a future date "2024-12-14" which might cause test failures. Consider using a past date to make the test more maintainable.
- Instant expectedDate = Instant.parse("2024-12-14T16:30:00Z"); + Instant expectedDate = Instant.parse("2023-12-14T16:30:00Z");chat/pom.xml (1)
180-184: Consider adding exclusions for unused JGit modulesJGit pulls in several modules by default. To reduce the deployment size, consider excluding unused modules.
<dependency> <groupId>org.eclipse.jgit</groupId> <artifactId>org.eclipse.jgit</artifactId> <version>${org.eclipse.jgit.version}</version> + <exclusions> + <exclusion> + <groupId>org.eclipse.jgit</groupId> + <artifactId>org.eclipse.jgit.ssh.apache</artifactId> + </exclusion> + <exclusion> + <groupId>org.eclipse.jgit</groupId> + <artifactId>org.eclipse.jgit.gpg.bc</artifactId> + </exclusion> + </exclusions> </dependency>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
chat/pom.xml(2 hunks)chat/src/main/java/greencity/config/SecurityConfig.java(2 hunks)chat/src/main/java/greencity/constant/AppConstant.java(1 hunks)chat/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)chat/src/main/java/greencity/controller/CommitInfoController.java(1 hunks)chat/src/main/java/greencity/dto/CommitInfoDto.java(1 hunks)chat/src/main/java/greencity/exception/exceptions/ResourceNotFoundException.java(1 hunks)chat/src/main/java/greencity/exception/handlers/CustomExceptionHandler.java(2 hunks)chat/src/main/java/greencity/service/CommitInfoService.java(1 hunks)chat/src/main/java/greencity/service/impl/CommitInfoServiceImpl.java(1 hunks)chat/src/test/java/greencity/controller/CommitInfoControllerTest.java(1 hunks)chat/src/test/java/greencity/service/impl/CommitInfoServiceImplTest.java(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build
chat/src/main/java/greencity/config/SecurityConfig.java
[error] File has not been previously formatted. Please format file and commit before running validation!
🔇 Additional comments (11)
chat/src/main/java/greencity/exception/exceptions/ResourceNotFoundException.java (1)
1-10: LGTM!The
ResourceNotFoundExceptionis correctly defined and appropriately annotated with@ResponseStatus(HttpStatus.NOT_FOUND)for proper HTTP response handling.chat/src/main/java/greencity/service/CommitInfoService.java (1)
1-15: LGTM!The
CommitInfoServiceinterface is well-defined with clear documentation, adhering to best practices.chat/src/main/java/greencity/dto/CommitInfoDto.java (1)
1-21: Well-structured DTO with appropriate annotations!The class is well-organized with proper Lombok annotations and validation constraints.
chat/src/main/java/greencity/constant/ErrorMessage.java (1)
12-16: Well-defined error messages with clear descriptions!The error messages are descriptive and follow a consistent format.
chat/src/main/java/greencity/controller/CommitInfoController.java (1)
1-56: Well-structured controller with comprehensive API documentation!The controller is well-organized with thorough OpenAPI annotations and clear example responses.
chat/src/test/java/greencity/controller/CommitInfoControllerTest.java (2)
43-54: LGTM! Well-structured test for successful scenario.The test properly verifies:
- Response status
- Response content (commit hash and date)
- Service method invocation
56-64: LGTM! Comprehensive error scenario test.The test properly verifies:
- Error response status
- Service method invocation
chat/src/main/java/greencity/config/SecurityConfig.java (1)
109-111: LGTM! Appropriate security configuration for commit info endpoint.The configuration correctly allows public access to the commit information endpoint, which is appropriate for this use case.
🧰 Tools
🪛 GitHub Actions: Build
[error] File has not been previously formatted. Please format file and commit before running validation!
chat/src/test/java/greencity/service/impl/CommitInfoServiceImplTest.java (1)
54-59: LGTM! Excellent test coverage with comprehensive scenarios.The test class demonstrates:
- Well-organized test structure
- Clear test method names
- Thorough coverage of success and error scenarios
- Good use of helper methods for mock configuration
Also applies to: 61-159
chat/pom.xml (2)
30-31: Verify the JGit version compatibility with Java 10The project uses Java 10 (as per maven-compiler-plugin configuration), but JGit 7.1.0 is quite recent. Ensure compatibility with the project's Java version.
Run this script to check JGit's Java version requirements:
✅ Verification successful
JGit 7.1.0 is compatible with Java 10
The project's Java 10 configuration is well above JGit's minimum requirements, as JGit maintains compatibility with Java 8 and above. No compatibility issues are expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check JGit's Java version requirements from its POM curl -s "https://repo1.maven.org/maven2/org/eclipse/jgit/org.eclipse.jgit/7.1.0.202411261347-r/org.eclipse.jgit-7.1.0.202411261347-r.pom" | grep -A 2 "<maven.compiler"Length of output: 167
Script:
#!/bin/bash # Check Java version configuration in the project echo "=== Java Version Configuration ===" cat chat/pom.xml | grep -A 2 "<java.version" echo -e "\n=== Maven Compiler Plugin Configuration ===" cat chat/pom.xml | grep -A 5 "maven-compiler-plugin" echo -e "\n=== Search for existing JGit usage ===" rg "org\.eclipse\.jgit" --type xmlLength of output: 936
185-190:⚠️ Potential issueRedundant json-path dependency declaration
The
json-pathdependency is already excluded fromspring-boot-starter-test. Adding it back with the same scope seems redundant.Let's verify if this dependency is actually needed:
✅ Verification successful
Confirmed: json-path dependency is redundant
The dependency is indeed redundant because:
- It's explicitly excluded from spring-boot-starter-test
- No direct usage of JsonPath found in test files
- All JSON assertions use Spring Test's built-in facilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for direct usage of JsonPath in test files rg -l "import.*com\.jayway\.jsonpath" --type java --glob "src/test/**/*"Length of output: 223
Script:
#!/bin/bash # Check spring-boot-starter-test dependency declaration echo "=== Checking spring-boot-starter-test dependency ===" cat chat/pom.xml | grep -A 10 "spring-boot-starter-test" echo -e "\n=== Searching for JsonPath imports ===" # Search for JsonPath imports in test directory fd -e java -t f . chat/src/test | xargs rg "import.*jsonpath" echo -e "\n=== Searching for JsonPath usage patterns ===" # Search for common JsonPath usage patterns fd -e java -t f . chat/src/test | xargs rg "JsonPath\.|JSONPath\."Length of output: 919
Script:
#!/bin/bash # Search for json-path in the dependency tree echo "=== Checking dependency tree for json-path ===" cat chat/pom.xml | grep -B 5 -A 5 "json-path" # Search for potential JSON-related test utilities that might need json-path echo -e "\n=== Searching for JSON-related test patterns ===" fd -e java -t f . chat/src/test | xargs rg -l "ObjectMapper|JSONObject|JSONArray|parseJson|readJson"Length of output: 1719
Script:
#!/bin/bash # Check the content of ChatControllerTest.java for JSON-related usage echo "=== Examining ChatControllerTest.java ===" cat chat/src/test/java/greencity/controller/ChatControllerTest.java # Also check for any other test files that might contain json-path related patterns echo -e "\n=== Searching for specific JsonPath patterns ===" fd -e java -t f . chat/src/test | xargs rg -l "jsonPath\(|read\(|parse\("Length of output: 23084
Script:
#!/bin/bash # Check content of files that might use JsonPath echo "=== Examining CommitInfoServiceImplTest.java ===" cat chat/src/test/java/greencity/service/impl/CommitInfoServiceImplTest.java echo -e "\n=== Examining ChatMessageServiceImplTest.java ===" cat chat/src/test/java/greencity/service/impl/ChatMessageServiceImplTest.java echo -e "\n=== Examining CommitInfoControllerTest.java ===" cat chat/src/test/java/greencity/controller/CommitInfoControllerTest.javaLength of output: 31002
| @RestController | ||
| @RequestMapping("/commit-info") | ||
| @RequiredArgsConstructor | ||
| public class CommitInfoController { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding security measures.
The endpoint is publicly accessible. Consider:
- Adding authentication/authorization checks
- Implementing rate limiting to prevent abuse
@RestController
@RequestMapping("/commit-info")
@RequiredArgsConstructor
+@SecurityRequirement(name = "Bearer Authentication")
public class CommitInfoController {📝 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.
| @RestController | |
| @RequestMapping("/commit-info") | |
| @RequiredArgsConstructor | |
| public class CommitInfoController { | |
| @RestController | |
| @RequestMapping("/commit-info") | |
| @RequiredArgsConstructor | |
| @SecurityRequirement(name = "Bearer Authentication") | |
| public class CommitInfoController { |
| public ResponseEntity<CommitInfoDto> getCommitInfo() { | ||
| return ResponseEntity.ok(commitInfoService.getLatestCommitInfo()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit error handling.
The current implementation might throw uncaught exceptions. Consider:
- Adding try-catch block to handle potential service exceptions
- Using
@ExceptionHandleror global exception handler
@GetMapping
public ResponseEntity<CommitInfoDto> getCommitInfo() {
- return ResponseEntity.ok(commitInfoService.getLatestCommitInfo());
+ try {
+ return ResponseEntity.ok(commitInfoService.getLatestCommitInfo());
+ } catch (ResourceNotFoundException e) {
+ throw e; // Let global exception handler handle it
+ } catch (Exception e) {
+ log.error("Unexpected error while fetching commit info", e);
+ throw new ServiceException("Failed to fetch commit information");
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| AuthenticationConfiguration authenticationConfiguration, | ||
| @Value("${spring.messaging.stomp.websocket.allowed-origins}") String[] allowedOrigins) { |
There was a problem hiding this comment.
Fix code formatting to comply with project standards.
The pipeline failure indicates formatting issues. Please format the file before running validation.
- AuthenticationConfiguration authenticationConfiguration,
- @Value("${spring.messaging.stomp.websocket.allowed-origins}") String[] allowedOrigins) {
+ AuthenticationConfiguration authenticationConfiguration,
+ @Value("${spring.messaging.stomp.websocket.allowed-origins}") String[] allowedOrigins) {📝 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.
| AuthenticationConfiguration authenticationConfiguration, | |
| @Value("${spring.messaging.stomp.websocket.allowed-origins}") String[] allowedOrigins) { | |
| AuthenticationConfiguration authenticationConfiguration, | |
| @Value("${spring.messaging.stomp.websocket.allowed-origins}") String[] allowedOrigins) { |
🧰 Tools
🪛 GitHub Actions: Build
[error] File has not been previously formatted. Please format file and commit before running validation!
|
| @Data | ||
| @AllArgsConstructor | ||
| @NoArgsConstructor | ||
| @EqualsAndHashCode |
There was a problem hiding this comment.
you can drop @EqualsAndHashCode, it's included in @Data
| @NoArgsConstructor | ||
| @EqualsAndHashCode | ||
| public class CommitInfoDto { | ||
| @NotNull |
There was a problem hiding this comment.
could annotate with @NotEmpty as well (if business logic dictates so)
| * the exception message. | ||
| */ | ||
| @ExceptionHandler(ResourceNotFoundException.class) | ||
| public final ResponseEntity<Object> handleResourceNotFoundException(ResourceNotFoundException ex, |
There was a problem hiding this comment.
idk if ReponseEntity is the best return type. there are specific types for error in spring, you can look here:
| public CommitInfoServiceImpl() { | ||
| try { | ||
| repository = new FileRepositoryBuilder() | ||
| .setGitDir(new File(".git")) |
| private static final String COMMIT_INFO_URL = "/commit-info"; | ||
| private static final String COMMIT_HASH = "abc123"; | ||
| private static final String COMMIT_DATE = "16/12/2024 12:06:32"; |
There was a problem hiding this comment.
ideally, test class should not be concerned about test data, I suggest you to move it to ModelUtils, or something like that..
+ less code in test class
+ less duplication of test data
- you have to look up entity data with lsp
|
|
||
| mockMvc.perform(get(COMMIT_INFO_URL).accept(MediaType.APPLICATION_JSON)) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.commitHash").value(COMMIT_HASH)) |
There was a problem hiding this comment.
would be better to convert object through objectmapper and compare it by .equals(), but it is to your preference
| (builderMock, context) -> { | ||
| configureFileRepositoryBuilderMock(builderMock); | ||
| when(builderMock.build()).thenThrow(new IOException()); |
There was a problem hiding this comment.
duplicated code, probably should extract method
#7929
Summary by CodeRabbit
New Features
Dependencies
Constants
Error Handling
ResourceNotFoundExceptionfor handling resource-related errors.