8079 display log files #8148
8079 display log files #8148
Conversation
# Conflicts: # service/src/main/java/greencity/service/EcoNewsServiceImpl.java
WalkthroughThis pull request introduces multiple enhancements across the application. The changes include reformatting of the security configuration file, new constants and error messages, and the addition of a REST controller with endpoints for log file management. New exception classes and handlers have been added to improve error reporting. Logging configuration now supports file rotation and additional properties. Furthermore, new DTO records, filters, and service interfaces and implementations for managing dotenv and log files have been introduced along with comprehensive unit tests to validate these features. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant LFC as LogFileController
participant DS as DotenvService
participant LFS as LogFileService
C->>LFC: POST /logs/download (filename, secretKey)
LFC->>DS: validateSecretKey(secretKey)
DS-->>LFC: Valid key (or exception)
LFC->>LFS: generateDownloadLogFileUrl(filename, secretKey)
LFS-->>LFC: File Resource (or exception)
LFC->>C: Return HTTP Response with file/resource
sequenceDiagram
participant C as Client
participant LFC as LogFileController
participant DS as DotenvService
C->>LFC: POST /logs/delete-dotenv (secretKey)
LFC->>DS: validateSecretKey(secretKey)
DS-->>LFC: Valid key (or exception)
LFC->>DS: deleteDotenvFile(secretKey)
DS-->>LFC: Confirmation (or exception)
LFC->>C: Return HTTP Response
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 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)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (26)
service-api/src/main/java/greencity/exception/exceptions/BadSecretKeyException.java (1)
5-7: Add documentation and customize error message handling.While the implementation is functionally correct, consider these enhancements:
- Add Javadoc to describe the exception's purpose and when it's thrown
- Consider adding a custom constructor with a generic message to avoid potential information leakage
Here's a suggested implementation:
@StandardException +/** + * Exception thrown when secret key validation fails. + * Uses generic messages to avoid exposing sensitive information. + */ public class BadSecretKeyException extends RuntimeException { + public BadSecretKeyException() { + super("Invalid authentication credentials"); + } }service-api/src/main/java/greencity/dto/logs/filter/ByteSizeRange.java (2)
6-8: Consider adding upper bound validation.While the
@Min(0)validation prevents negative sizes, consider adding an upper bound validation to prevent unreasonably large values that could impact performance or cause memory issues.Example implementation:
public record ByteSizeRange( - @Min(value = 0, message = "Size cannot be negative") long from, - @Min(value = 0, message = "Size cannot be negative") long to) { + @Min(value = 0, message = "Size cannot be negative") + @Max(value = Long.MAX_VALUE, message = "Size exceeds maximum allowed value") long from, + @Min(value = 0, message = "Size cannot be negative") + @Max(value = Long.MAX_VALUE, message = "Size exceeds maximum allowed value") long to) {
9-13: Well-implemented range validation!The compact constructor effectively validates the range with a clear error message. The implementation is clean and follows best practices.
Consider adding
@NonNullannotations to the fields if they are not expected to be null, or add null checks if they can be null:public record ByteSizeRange( + @NonNull @Min(value = 0, message = "Size cannot be negative") long from, + @NonNull @Min(value = 0, message = "Size cannot be negative") long to) {service-api/src/main/java/greencity/exception/exceptions/FileReadException.java (1)
1-7: Consider adding Javadoc documentation.Adding documentation would help other developers understand:
- When this exception is thrown
- What conditions trigger it
- How to handle it appropriately
package greencity.exception.exceptions; import lombok.experimental.StandardException; +/** + * Exception thrown when a file cannot be read due to I/O errors or access issues. + * This is typically thrown during log file operations when the system + * encounters problems accessing or reading file content. + */ @StandardException public class FileReadException extends RuntimeException { }service/src/main/java/greencity/service/DotenvServiceImpl.java (3)
34-41: Consider caching env variables instead of reloading on each validation.Calling
reloadEnvFile()invalidateSecretKeyeach time might cause overhead if the method is frequently invoked. You could explore caching or lazy-loading the environment variables to reduce repeated file I/O operations.
47-64: Handle concurrency and user clarity for.envdeletion.Deleting the
.envfile can make dependent functionality unavailable if another thread or process is accessing it. Consider documenting or handling concurrent calls (e.g., by locking or providing a user-facing explanation in UI about the system state after deletion).
77-85: Ensure consistent error messaging during reload failures.
reloadEnvFile()throwsFunctionalityNotAvailableExceptionif the file cannot be loaded. Verify that this is the intended user-facing error message for all scenarios, including when the file is intentionally deleted for security concerns.core/src/main/java/greencity/controller/LogFileController.java (4)
37-37: Complete or remove the TODO comment about tests.The comment indicates the need for tests on this controller. Implementing them will help ensure each endpoint behaves correctly, particularly under error conditions or concurrency scenarios.
Do you want help creating a basic unit test outline?
54-63: Reconsider using POST for retrieving log file metadata.While using POST for data retrieval is valid, it can be less conventional than HTTP GET. Evaluate whether switching to GET could be more RESTful unless a payload-based filter strictly requires POST.
83-87: Manage potentially large file outputs when viewing logs.The file content is returned as a raw
String. This could be large and memory-intensive if logs grow sizable. Consider adding pagination, file streaming, or partial responses if needed for production scenarios.
125-130: Clarify user experience after removing.env.Deleting the
.envfile may disable subsequent calls that require it. Consider adding a response message or additional data in the response body to inform clients that environment-related functionalities are no longer available.service/src/main/java/greencity/service/LogFileServiceImpl.java (2)
59-73: Stream large log files carefully.Reading the entire file into memory might be expensive for extremely large logs. Consider streaming the file or providing chunked responses if performance becomes an issue in a production environment.
155-157: Use consistent locale for case-insensitive matches.When using
toLowerCase(), supply a locale (e.g.,Locale.ROOT) to avoid unexpected issues in certain locales.- filename.toLowerCase().contains(fileNameFilter.toLowerCase()) + filename.toLowerCase(Locale.ROOT).contains(fileNameFilter.toLowerCase(Locale.ROOT))Also applies to: 168-170
service-api/src/main/java/greencity/dto/logs/filter/DateRange.java (1)
5-5: Consider using modern date-time API.While the validation is well implemented, consider using
java.time.LocalDateTimeorjava.time.Instantinstead of the legacyjava.util.Dateclass. The modern date-time API provides better immutability and handling of time zones.-import java.util.Date; +import java.time.LocalDateTime; public record DateRange( - @NotNull(message = "From date cannot be null") Date from, - @NotNull(message = "To date cannot be null") Date to) { + @NotNull(message = "From date cannot be null") LocalDateTime from, + @NotNull(message = "To date cannot be null") LocalDateTime to) {Also applies to: 8-9
service-api/src/main/java/greencity/service/DotenvService.java (3)
3-19: Consider enhancing security measures for sensitive operationsThe interface handles critical operations on the
.envfile. Consider adding:
- Rate limiting for key validation to prevent brute force attempts
- Audit logging for tracking access attempts
- Documentation about key format requirements
- Recovery mechanism for accidental deletions
Would you like me to propose an enhanced version of the interface with these security measures?
10-10: Add validation requirements to method documentationThe
validateSecretKeymethod should document:
- Expected key format/requirements
- What exceptions it may throw
- Rate limiting behavior if implemented
18-18: Document potential risks of deleteDotenvFile operationThe method documentation should warn about:
- Irreversible nature of the operation
- Impact on system functionality
- Required permissions/preconditions
service-api/src/main/java/greencity/dto/logs/LogFileRequestDto.java (1)
9-26: Revise default JSON example for security and accuracyThe example JSON:
- Should not include a literal "string" as secretKey
- Uses future dates (2025) which may confuse users
- Should demonstrate realistic byte size ranges
public static final String defaultJson = """ { - "secretKey": "string", + "secretKey": "<your-secret-key-here>", "filterDto": { "fileNameQuery": "app.log", "fileContentQuery": "ERROR", "byteSizeRange": { "from": 0, - "to": 0 + "to": 1048576 }, "dateRange": { - "from": "2025-01-01T00:00:00.000Z", - "to": "2025-01-01T00:00:00.000Z" + "from": "2024-01-01T00:00:00.000Z", + "to": "2024-02-01T00:00:00.000Z" }, "logLevel": "INFO" } } """;service-api/src/main/java/greencity/dto/logs/LogFileMetadataDto.java (1)
14-28: Update example JSON with realistic valuesThe example JSON uses future dates and could be more descriptive:
{ - "filename": "string", + "filename": "application-2024-02-01.log", "size": 0, - "lastModified": "2025-01-01T00:00:00.000+00:00" + "lastModified": "2024-02-01T10:30:00.000Z" }service/src/main/java/greencity/config/DotenvConfig.java (1)
15-15: Consider making the env filename configurable.Instead of hardcoding the filename, consider making it configurable through system properties or Spring configuration to support different environments and improve security.
- private static final String DOTENV_FILENAME = "secretKeys.env"; + @Value("${dotenv.filename:secretKeys.env}") + private String dotenvFilename;service-api/src/main/java/greencity/service/LogFileService.java (1)
18-18: Consider adding validation annotations for parameters.Add validation annotations to ensure parameters meet security requirements before processing.
- PageableDto<LogFileMetadataDto> getLogFilesList(Pageable page, LogFileFilterDto filterDto, String secretKey); + PageableDto<LogFileMetadataDto> getLogFilesList(@NotNull Pageable page, LogFileFilterDto filterDto, @NotBlank String secretKey); - String getLogFileContent(String filename, String secretKey); + String getLogFileContent(@NotBlank @Pattern(regexp = "^[\\w.-]+\\.log$") String filename, @NotBlank String secretKey); - Resource getDownloadLogFileUrl(String filename, String secretKey); + Resource getDownloadLogFileUrl(@NotBlank @Pattern(regexp = "^[\\w.-]+\\.log$") String filename, @NotBlank String secretKey);Also applies to: 28-28, 37-37
service/src/test/java/greencity/service/DotenvServiceImplTest.java (1)
38-51: Consider using parameterized tests for secret key validation.Using parameterized tests would make it easier to test multiple scenarios with different secret keys.
+ @ParameterizedTest + @ValueSource(strings = {"invalid", "", " ", "wrongKey"}) + void validateSecretKeyShouldThrowExceptionForInvalidKeys(String secretKey) { DotenvServiceImpl spyService = spy(dotenvService); doNothing().when(spyService).reloadEnvFile(); when(dotenv.get("logs.secretKey")).thenReturn("validSecret"); when(passwordEncoder.matches(secretKey, "validSecret")).thenReturn(false); assertThrows(BadSecretKeyException.class, () -> spyService.validateSecretKey(secretKey)); }core/src/test/java/greencity/controller/LogFileControllerTest.java (1)
67-91: Add validation for pagination parameters.The test case verifies basic functionality but should also validate that the service receives correct pagination parameters.
Add assertions to verify pagination parameters:
verify(logFileService).getLogFilesList(page, filterDto, secretKey); +verify(page).getPageNumber(); +verify(page).getPageSize(); +assertEquals(5, pageNumber); +assertEquals(20, pageSize);core/src/main/java/greencity/config/SecurityConfig.java (1)
87-87: Consider more specific URL pattern.The current pattern
/logs/**is quite broad. Consider restricting it to specific operations.-public static final String LOGS = "/logs/**"; +public static final String LOGS = "/logs/{operation:list|view|download|delete-dotenv}/**";core/src/test/java/greencity/ModelUtils.java (1)
603-611: Add more test data variations.The current implementation provides basic test data. Consider adding factory methods for different filtering scenarios.
Add methods like:
public static LogFileFilterDto getLogFileFilterDtoWithDateRange() { return new LogFileFilterDto( "filename", "fileContent", null, new DateRange(LocalDateTime.now().minusDays(7), LocalDateTime.now()), LogLevel.INFO ); } public static LogFileFilterDto getLogFileFilterDtoWithSizeRange() { return new LogFileFilterDto( "filename", "fileContent", new ByteSizeRange(1024L, 2048L), null, LogLevel.INFO ); }core/src/main/resources/logback.xml (1)
32-35: Consider adding log retention policy.While the current configuration handles log rotation well, consider adding a retention policy to automatically clean up old log files.
Add the following within the rollingPolicy element:
<rollingPolicy class="ch.qos.logback.core.rolling.TimeBasedRollingPolicy"> <fileNamePattern>${LOG_PATH}/application-%d{yyyy-MM-dd}.%i.log</fileNamePattern> + <!-- Keep 30 days worth of history --> + <maxHistory>30</maxHistory> + <!-- Total size of all archive files, if total size > 1GB, it will delete old archived files --> + <totalSizeCap>1GB</totalSizeCap> <timeBasedFileNamingAndTriggeringPolicy class="ch.qos.logback.core.rolling.SizeAndTimeBasedFNATP">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
core/src/main/java/greencity/config/SecurityConfig.java(2 hunks)core/src/main/java/greencity/constant/HttpStatuses.java(1 hunks)core/src/main/java/greencity/controller/LogFileController.java(1 hunks)core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java(2 hunks)core/src/main/resources/logback.xml(1 hunks)core/src/test/java/greencity/ModelUtils.java(4 hunks)core/src/test/java/greencity/controller/LogFileControllerTest.java(1 hunks)secretKeys.env(1 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)service-api/src/main/java/greencity/dto/logs/LogFileMetadataDto.java(1 hunks)service-api/src/main/java/greencity/dto/logs/LogFileRequestDto.java(1 hunks)service-api/src/main/java/greencity/dto/logs/filter/ByteSizeRange.java(1 hunks)service-api/src/main/java/greencity/dto/logs/filter/DateRange.java(1 hunks)service-api/src/main/java/greencity/dto/logs/filter/LogFileFilterDto.java(1 hunks)service-api/src/main/java/greencity/exception/exceptions/BadSecretKeyException.java(1 hunks)service-api/src/main/java/greencity/exception/exceptions/FileReadException.java(1 hunks)service-api/src/main/java/greencity/exception/exceptions/FunctionalityNotAvailableException.java(1 hunks)service-api/src/main/java/greencity/service/DotenvService.java(1 hunks)service-api/src/main/java/greencity/service/LogFileService.java(1 hunks)service/pom.xml(1 hunks)service/src/main/java/greencity/config/DotenvConfig.java(1 hunks)service/src/main/java/greencity/config/PasswordEncoderConfig.java(1 hunks)service/src/main/java/greencity/service/DotenvServiceImpl.java(1 hunks)service/src/main/java/greencity/service/LogFileServiceImpl.java(1 hunks)service/src/test/java/greencity/service/DotenvServiceImplTest.java(1 hunks)service/src/test/java/greencity/service/LogFileServiceImplTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- secretKeys.env
🧰 Additional context used
🪛 GitHub Actions: CI/CD GreenCity
service-api/src/main/java/greencity/dto/logs/filter/LogFileFilterDto.java
[error] 4-4: Unused import - lombok.Builder.
🔇 Additional comments (18)
service-api/src/main/java/greencity/exception/exceptions/BadSecretKeyException.java (1)
1-4: Clean package organization and imports.The package structure follows the project conventions, and imports are kept minimal and relevant.
service-api/src/main/java/greencity/dto/logs/filter/ByteSizeRange.java (1)
1-5: Clean and well-organized imports!The imports are minimal, specific, and well-organized. Good use of Jakarta validation constraints.
service-api/src/main/java/greencity/exception/exceptions/FileReadException.java (2)
5-7: Well-structured exception class with Lombok integration!The use of
@StandardExceptionfrom Lombok is a great choice as it reduces boilerplate while providing all necessary exception constructors and methods.
5-7:❓ Verification inconclusive
Verify consistent exception handling across the codebase.
Let's ensure this exception is properly caught and handled wherever files are being read.
🏁 Script executed:
#!/bin/bash # Search for places where FileReadException is caught or thrown echo "Searching for FileReadException usage..." rg -l "FileReadException" # Search for potential file reading operations that might need to use this exception echo -e "\nSearching for potential file reading operations..." ast-grep --pattern 'new FileInputStream($_)' ast-grep --pattern 'Files.readAllBytes($_)' ast-grep --pattern 'Files.readString($_)'Length of output: 2143
Action: Verify and standardize file reading exception handling
The current implementation of FileReadException is used in the custom exception handler and in parts of the LogFileServiceImpl. However, our search revealed several file reading operations—such as those in
SocialNetworkImageServiceImpl.java(usingFileInputStream) and inMultipartBase64ImageMapper.java(usingFiles.readAllBytes)—where it's not clear if IOExceptions or similar errors are being caught and uniformly wrapped in a FileReadException.Please review the following:
File Definition & Exception Handler:
service-api/src/main/java/greencity/exception/exceptions/FileReadException.javacore/src/main/java/greencity/exception/handler/CustomExceptionHandler.javaFile Reading Operations:
service/src/main/java/greencity/service/LogFileServiceImpl.javaservice/src/main/java/greencity/service/SocialNetworkImageServiceImpl.javaservice/src/main/java/greencity/mapping/MultipartBase64ImageMapper.javaEnsure that any IO-related exceptions during file processing are either caught and rethrown as a FileReadException or appropriately handled so that error management remains consistent across the codebase.
service-api/src/main/java/greencity/exception/exceptions/FunctionalityNotAvailableException.java (1)
1-7: Well-structured exception class!Good use of Lombok's
@StandardExceptionto reduce boilerplate while maintaining a clean, focused implementation.service-api/src/main/java/greencity/dto/logs/filter/LogFileFilterDto.java (1)
7-13: Well-designed filter DTO!Good use of Java records for an immutable DTO and appropriate validation annotations for the range fields.
service/src/main/java/greencity/config/PasswordEncoderConfig.java (1)
8-14:✅ Verification successful
Secure password encoder configuration!
Good choice using BCryptPasswordEncoder for secure password hashing. The configuration follows Spring best practices.
Let's verify we're using the latest BCrypt implementation:
🌐 Web query:
What is the latest version of Spring Security BCryptPasswordEncoder and are there any known security vulnerabilities?💡 Result:
The latest version of Spring Security's
BCryptPasswordEncodershown in official documentation is v6.4.2 (as of February 2025)[1]. This implementation remains the recommended choice for secure password hashing using the bcrypt algorithm.Known Security Vulnerabilities
CVE-2022-22976 (Fixed)
- Impact: Integer overflow when using
BCryptPasswordEncoderwith maximum work factor (31), causing salt rounds to be skipped[2][6].- Affected Versions:
- Spring Security 5.5.x < 5.5.7
- Spring Security 5.6.x < 5.6.4
- Older unsupported versions
- Mitigation:
1️⃣ Avoid work factors ≥31 (computationally impractical)
2️⃣ Use OWASP-recommended strength=10[2][8]
3️⃣ Upgrade to fixed versions:
- ≥5.5.7
- ≥5.6.4
- ≥5.7[2][6]
Implementation Considerations
- Default configuration (strength=10) remains unaffected by this CVE[2].
- Work factors above ~20 become computationally infeasible on modern hardware[1][4].
- Avoid manual instantiation with deprecated constructors like
new BCryptPasswordEncoder(31)due to integer overflow risks[1][5].Current Best Practices
// Recommended configuration (Spring Security ≥6.x) @Bean public PasswordEncoder passwordEncoder() { return new BCryptPasswordEncoder(); // Defaults: $2a version + strength=10 }This ensures:
- Automatic salt generation[1][8]
- Compatibility with modern password storage standards[8]
- Protection against rainbow table attacks[4]
No active CVEs exist for
BCryptPasswordEncoderin properly configured environments using recent Spring Security versions (≥6.x)[1][8].Citations:
- 1: https://docs.spring.io/spring-security/reference/api/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoder.html
- 2: https://spring.io/security/cve-2022-22976
- 3: https://spring.io/security/page-5
- 4: https://www.aegissofttech.com/articles/bcrypt-password-encoding-spring-security.html
- 5: Spring Security Bcrypt with strength/log rounds = 31 results in 'Bad number of rounds' error although 31 should be ok spring-projects/spring-security#11470
- 6: https://nvd.nist.gov/vuln/search/results?form_type=Basic&results_type=overview&query=Spring+Security&queryType=phrase&search_type=all&isCpeNameSearch=false
- 7: https://docs.spring.io/spring-security/site/docs/4.2.4.RELEASE/apidocs/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoder.html
- 8: https://docs.spring.io/spring-security/reference/features/authentication/password-storage.html
- 9: https://cve.mitre.org/cgi-bin/cvekey.cgi
- 10: https://www.youtube.com/watch?v=RI3UpqUZq50
Secure password encoder configuration verified!
Great work on this configuration. Your use of
BCryptPasswordEncoderwith its default settings (providing a strength of 10) aligns perfectly with Spring Security best practices. The latest documentation (v6.4.2 as of February 2025) confirms that this approach remains secure, provided that recommended work factors are maintained. No vulnerabilities affect the default configuration, so your dedicated bean (DotenvPasswordEncoder) is both intentional and appropriate.service-api/src/main/java/greencity/dto/logs/filter/DateRange.java (1)
10-14: Great validation in the compact constructor!Excellent practice implementing the validation logic in the record's compact constructor to ensure date range consistency.
core/src/main/java/greencity/constant/HttpStatuses.java (1)
15-15: LGTM! Addition follows existing patternThe new constant follows the established naming convention and provides a clear message.
core/src/test/java/greencity/controller/LogFileControllerTest.java (1)
58-65: LGTM! Well-structured test setup.The test setup is comprehensive with proper mocking of dependencies and initialization of MockMvc with custom argument resolvers and exception handler.
service-api/src/main/java/greencity/constant/ErrorMessage.java (1)
238-244: LGTM! Well-defined error messages.The error messages are clear, descriptive, and follow the existing pattern in the file. They cover all necessary scenarios for log file operations:
- File not found cases
- Read errors
- Security validation
- Environment file operations
core/src/main/java/greencity/config/SecurityConfig.java (1)
349-350: LGTM! Appropriate security configuration.The logs endpoint is properly secured and accessible only to authenticated users with appropriate roles.
core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java (3)
560-577: LGTM! Well-structured exception handler for FileReadException.The implementation follows best practices with appropriate error logging and HTTP 503 status code for service unavailability.
579-596: LGTM! Well-implemented exception handler for BadSecretKeyException.The implementation correctly uses HTTP 403 Forbidden status code for invalid secret key scenarios.
598-607: LGTM! Proper implementation of FunctionalityNotAvailableException handler.The handler follows the established pattern and uses appropriate HTTP 403 status code.
core/src/main/resources/logback.xml (2)
4-5: LGTM! Well-defined logging properties.The LOG_PATH and LOG_MAX_SIZE properties are appropriately configured with reasonable values.
14-30: LGTM! Well-structured rolling file appender configuration.The implementation includes:
- Daily log rotation with size-based triggers
- Consistent log pattern with console appender
- Clear file naming pattern for easy log file identification
service/pom.xml (1)
131-135:✅ Verification successful
Verify the dotenv-java dependency version.
Let's check if there's a newer version available with potential security fixes or improvements.
🌐 Web query:
What is the latest version of dotenv-java library?💡 Result:
The latest version of the dotenv-java library is 3.1.0, as indicated by its GitHub repository and Maven Central listings[6][9]. This pure Java implementation allows loading environment variables from
.envfiles without Kotlin dependencies.Key details:
Maven:
<dependency> <groupId>io.github.cdimascio</groupId> <artifactId>dotenv-java</artifactId> <version>3.1.0</version> </dependency>Gradle:
implementation 'io.github.cdimascio:dotenv-java:3.1.0'The library supports Java 11+ with backward compatibility for Java 8 available in version 2.3.2[6]. It provides environment variable management through a simple API using
Dotenv.load()anddotenv.get("VAR_NAME")while respecting system environment variables that take precedence over.envfile values[6][9].Citations:
- 1: https://github.com/cdimascio/java-dotenv
- 2: https://www.dotenv.org/changelog/
- 3: https://javadoc.io/doc/io.github.cdimascio/dotenv-java/latest/index.html
- 4: https://www.npmjs.com/package/dotenv-cli
- 5: https://javadoc.io/doc/io.github.cdimascio/java-dotenv/latest/index.html
- 6: https://github.com/cdimascio/dotenv-java
- 7: https://mvnrepository.com/artifact/io.github.cdimascio/dotenv-java/3.0.0
- 8: https://central.sonatype.com/artifact/io.github.cdimascio/dotenv-java/2.2.3
- 9: https://central.sonatype.com/artifact/io.github.cdimascio/dotenv-java
- 10: https://mvnrepository.com/artifact/io.github.cdimascio/dotenv-java
dotenv-java Dependency Verified
The version declared in the
pom.xml(3.1.0) is indeed the latest available according to Maven Central and the GitHub repository. No changes are necessary.
| private boolean filterFileDto(LogFileMetadataDto fileDto, LogFileFilterDto filterDto, String secretKey) { | ||
| if (filterDto == null) { | ||
| return true; | ||
| } | ||
| String fileContent = getLogFileContent(fileDto.getFilename(), secretKey); | ||
| return matchesFileNameQuery(fileDto.getFilename(), filterDto.fileNameQuery()) | ||
| && matchesFileContentQuery(fileContent, filterDto.fileContentQuery()) | ||
| && matchesByteSize(fileDto.getByteSize(), filterDto.byteSizeRange()) | ||
| && matchesDateRange(fileDto.getLastModified(), filterDto.dateRange()) | ||
| && matchesLogLevel(fileContent, filterDto.logLevel()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid reading file content multiple times for filtering.
Currently, each file's content is read within filterFileDto. This can become costly if you have many large log files. One option is to parse only the file metadata first, and selectively read file content on demand to meet filter criteria.
| public PageableDto<LogFileMetadataDto> getLogFilesList(Pageable pageable, LogFileFilterDto filterDto, | ||
| String secretKey) { | ||
| dotEnvService.validateSecretKey(secretKey); | ||
| File[] logFiles = listLogFilesFromFolder(); | ||
|
|
||
| if (logFiles == null || logFiles.length == 0) { | ||
| throw new NotFoundException(ErrorMessage.LOG_FILES_NOT_FOUND); | ||
| } | ||
|
|
||
| List<LogFileMetadataDto> dtos = Arrays.stream(logFiles) | ||
| .map(file -> new LogFileMetadataDto(file.getName(), file.length(), new Date(file.lastModified()))) | ||
| .filter(fileDto -> filterFileDto(fileDto, filterDto, secretKey)) | ||
| .toList(); | ||
|
|
||
| return applyPagination(dtos, pageable); | ||
| } |
There was a problem hiding this comment.
Check offset bounds in pagination logic.
applyPagination is called after filtering. If pageable.getOffset() exceeds the total elements (e.g., due to library defaults or user input), subList() in applyPagination (line 120) can throw an IndexOutOfBoundsException. Consider adding safety checks or no-results handling.
int totalElements = dtos.size();
int start = (int) pageable.getOffset();
int end = Math.min((start + pageable.getPageSize()), dtos.size());
+ if (start >= totalElements) {
+ return new PageableDto<>(List.of(), totalElements, pageable.getPageNumber(), totalPages);
+ }Committable suggestion skipped: line range outside the PR's diff.
| package greencity.dto.logs.filter; | ||
|
|
||
| import jakarta.validation.Valid; | ||
| import lombok.Builder; |
There was a problem hiding this comment.
Remove unused import.
The lombok.Builder import is not being used in this class.
-import lombok.Builder;📝 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.
| import lombok.Builder; | |
| // (The unused import 'lombok.Builder' has been removed.) |
🧰 Tools
🪛 GitHub Actions: CI/CD GreenCity
[error] 4-4: Unused import - lombok.Builder.
| public record LogFileRequestDto( | ||
| String secretKey, | ||
| @Valid LogFileFilterDto filterDto) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation constraints for secretKey field
The secretKey field should have appropriate validation constraints:
- Minimum/maximum length
- Pattern matching for allowed characters
- Not null/blank validation
public record LogFileRequestDto(
+ @NotBlank(message = "Secret key must not be blank")
+ @Size(min = 32, max = 256, message = "Secret key must be between 32 and 256 characters")
+ @Pattern(regexp = "^[a-zA-Z0-9._-]+$", message = "Secret key contains invalid characters")
String secretKey,
@Valid LogFileFilterDto filterDto) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public record LogFileRequestDto( | |
| String secretKey, | |
| @Valid LogFileFilterDto filterDto) { | |
| public record LogFileRequestDto( | |
| @NotBlank(message = "Secret key must not be blank") | |
| @Size(min = 32, max = 256, message = "Secret key must be between 32 and 256 characters") | |
| @Pattern(regexp = "^[a-zA-Z0-9._-]+$", message = "Secret key contains invalid characters") | |
| String secretKey, | |
| @Valid LogFileFilterDto filterDto) { | |
| } |
service-api/src/main/java/greencity/dto/logs/LogFileMetadataDto.java
Outdated
Show resolved
Hide resolved
| @Bean | ||
| Dotenv dotenv() { | ||
| try { | ||
| return Dotenv.configure() | ||
| .filename(DOTENV_FILENAME) | ||
| .load(); | ||
| } catch (DotenvException ex) { | ||
| throw new FunctionalityNotAvailableException(ErrorMessage.FUNCTIONALITY_NOT_AVAILABLE); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add file permission validation for the env file.
The current implementation doesn't verify file permissions, which could lead to security vulnerabilities if the file is readable by unauthorized users.
🏁 Script executed:
#!/bin/bash
# Check if the env file has secure permissions
fd -t f 'secretKeys.env' -x stat -c '%a %n' {} \;Length of output: 88
Enhance File Permission Validation in DotenvConfig
In service/src/main/java/greencity/config/DotenvConfig.java (lines 17–26), please add a robust file permission validation for the environment file (e.g., secretKeys.env). The current implementation lacks any checks on file permissions, which risks exposing sensitive configuration data. Note that a prior automated check using the stat command failed with an error (Command not found: stat). This indicates that the current shell-based command isn’t reliable in this environment. Consider one of the following approaches:
- Java-based verification: Use Java APIs (for example,
java.nio.file.Files.getPosixFilePermissions) to programmatically verify that the file permissions restrict access appropriately. - Shell-based alternative: If a shell check is preferred, update the command (e.g., using
ls -linstead ofstat) and manually verify that the file permissions onsecretKeys.envare secure.
Please manually verify that any implementation meets your security requirements.
service-api/src/main/java/greencity/service/LogFileService.java
Outdated
Show resolved
Hide resolved
| @Test | ||
| void getLogFileContentShouldThrowNotFoundExceptionWhenFileDoesNotExist() { | ||
| String filename = "nonexistent.log"; | ||
| String secretKey = "secret"; | ||
|
|
||
| File mockFile = mock(File.class); | ||
| when(mockFile.exists()).thenReturn(false); | ||
|
|
||
| LogFileServiceImpl spyService = spy(logFileService); | ||
| doReturn(mockFile).when(spyService).getLogFile(filename); | ||
|
|
||
| assertThrows(NotFoundException.class, () -> spyService.getLogFileContent(filename, secretKey)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for file name validation.
Consider adding tests to verify that file names are properly validated to prevent path traversal attacks.
+ @Test
+ void getLogFileContentShouldPreventPathTraversal() {
+ String filename = "../../../etc/passwd";
+ String secretKey = "secret";
+
+ assertThrows(InvalidFileNameException.class,
+ () -> logFileService.getLogFileContent(filename, secretKey));
+ }📝 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.
| @Test | |
| void getLogFileContentShouldThrowNotFoundExceptionWhenFileDoesNotExist() { | |
| String filename = "nonexistent.log"; | |
| String secretKey = "secret"; | |
| File mockFile = mock(File.class); | |
| when(mockFile.exists()).thenReturn(false); | |
| LogFileServiceImpl spyService = spy(logFileService); | |
| doReturn(mockFile).when(spyService).getLogFile(filename); | |
| assertThrows(NotFoundException.class, () -> spyService.getLogFileContent(filename, secretKey)); | |
| } | |
| @Test | |
| void getLogFileContentShouldThrowNotFoundExceptionWhenFileDoesNotExist() { | |
| String filename = "nonexistent.log"; | |
| String secretKey = "secret"; | |
| File mockFile = mock(File.class); | |
| when(mockFile.exists()).thenReturn(false); | |
| LogFileServiceImpl spyService = spy(logFileService); | |
| doReturn(mockFile).when(spyService).getLogFile(filename); | |
| assertThrows(NotFoundException.class, () -> spyService.getLogFileContent(filename, secretKey)); | |
| } | |
| @Test | |
| void getLogFileContentShouldPreventPathTraversal() { | |
| String filename = "../../../etc/passwd"; | |
| String secretKey = "secret"; | |
| assertThrows(InvalidFileNameException.class, | |
| () -> logFileService.getLogFileContent(filename, secretKey)); | |
| } |
| @Test | ||
| void getLogFilesListShouldReturnLogFilesWhenTheyExist() { | ||
| String secretKey = "secret"; | ||
| File logFile1 = new File("test1.log"); | ||
| File logFile2 = new File("test2.log"); | ||
| File[] mockFiles = { logFile1, logFile2 }; | ||
|
|
||
| LogFileServiceImpl spyService = spy(logFileService); | ||
| doReturn(mockFiles).when(spyService).listLogFilesFromFolder(); | ||
| PageableDto<LogFileMetadataDto> result = spyService.getLogFilesList(PAGEABLE, null, secretKey); | ||
|
|
||
| assertNotNull(result); | ||
| assertEquals(2, result.getTotalElements()); | ||
| assertEquals("test1.log", result.getPage().get(0).getFilename()); | ||
| assertEquals("test2.log", result.getPage().get(1).getFilename()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for file size limits.
Consider adding tests to verify that large files are handled appropriately to prevent memory issues.
+ @Test
+ void getLogFileContentShouldHandleLargeFiles() {
+ String filename = "large.log";
+ String secretKey = "secret";
+ File mockFile = mock(File.class);
+ when(mockFile.exists()).thenReturn(true);
+ when(mockFile.length()).thenReturn(MAX_FILE_SIZE + 1);
+
+ LogFileServiceImpl spyService = spy(logFileService);
+ doReturn(mockFile).when(spyService).getLogFile(filename);
+
+ assertThrows(FileSizeLimitExceededException.class,
+ () -> spyService.getLogFileContent(filename, secretKey));
+ }📝 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.
| @Test | |
| void getLogFilesListShouldReturnLogFilesWhenTheyExist() { | |
| String secretKey = "secret"; | |
| File logFile1 = new File("test1.log"); | |
| File logFile2 = new File("test2.log"); | |
| File[] mockFiles = { logFile1, logFile2 }; | |
| LogFileServiceImpl spyService = spy(logFileService); | |
| doReturn(mockFiles).when(spyService).listLogFilesFromFolder(); | |
| PageableDto<LogFileMetadataDto> result = spyService.getLogFilesList(PAGEABLE, null, secretKey); | |
| assertNotNull(result); | |
| assertEquals(2, result.getTotalElements()); | |
| assertEquals("test1.log", result.getPage().get(0).getFilename()); | |
| assertEquals("test2.log", result.getPage().get(1).getFilename()); | |
| } | |
| @Test | |
| void getLogFilesListShouldReturnLogFilesWhenTheyExist() { | |
| String secretKey = "secret"; | |
| File logFile1 = new File("test1.log"); | |
| File logFile2 = new File("test2.log"); | |
| File[] mockFiles = { logFile1, logFile2 }; | |
| LogFileServiceImpl spyService = spy(logFileService); | |
| doReturn(mockFiles).when(spyService).listLogFilesFromFolder(); | |
| PageableDto<LogFileMetadataDto> result = spyService.getLogFilesList(PAGEABLE, null, secretKey); | |
| assertNotNull(result); | |
| assertEquals(2, result.getTotalElements()); | |
| assertEquals("test1.log", result.getPage().get(0).getFilename()); | |
| assertEquals("test2.log", result.getPage().get(1).getFilename()); | |
| } | |
| @Test | |
| void getLogFileContentShouldHandleLargeFiles() { | |
| String filename = "large.log"; | |
| String secretKey = "secret"; | |
| File mockFile = mock(File.class); | |
| when(mockFile.exists()).thenReturn(true); | |
| when(mockFile.length()).thenReturn(MAX_FILE_SIZE + 1); | |
| LogFileServiceImpl spyService = spy(logFileService); | |
| doReturn(mockFile).when(spyService).getLogFile(filename); | |
| assertThrows(FileSizeLimitExceededException.class, | |
| () -> spyService.getLogFileContent(filename, secretKey)); | |
| } |
| @Test | ||
| void getLogFileShouldReturnOkWhenRequestIsValid() throws Exception { | ||
| String filename = "logfile.log"; | ||
| String secretKey = "validSecret"; | ||
| String fileContent = "Log file content"; | ||
|
|
||
| when(logFileService.getLogFileContent(filename, secretKey)).thenReturn(fileContent); | ||
|
|
||
| mockMvc.perform(post(VIEW_LOG_FILE_LINK, filename) | ||
| .contentType(MediaType.TEXT_PLAIN) | ||
| .content(secretKey)) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(content().string(fileContent)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for error scenarios.
The test only covers the success scenario. Consider adding test cases for:
- Invalid secret key
- File not found
- File read errors
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
service/src/test/java/greencity/service/LogFileServiceImplTest.java (1)
87-140:⚠️ Potential issueAdd security-related test cases for file content retrieval.
The current tests cover basic functionality, but security-critical test cases are missing.
As mentioned in previous reviews, please add tests for:
- Path traversal prevention
- File size limits
These tests are essential for security and resource management.
🧹 Nitpick comments (10)
service/src/test/java/greencity/ModelUtils.java (1)
3521-3527: Consider adding more test data variations.While the current implementation is good, having additional factory methods with different combinations of filter parameters would help test edge cases and boundary conditions.
Example additions:
+ public static LogFileFilterDto getLogFileFilterDtoWithAllFields() { + return new LogFileFilterDto("test", + "error", + new ByteSizeRange(0, 1000), + new DateRange(LocalDateTime.now().minusDays(7), LocalDateTime.now()), + LogLevel.ERROR); + } + + public static LogFileFilterDto getLogFileFilterDtoWithNullFields() { + return new LogFileFilterDto(null, + null, + null, + null, + null); + }service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java (3)
1-38: Well-structured test class with clear test cases!The test class follows good testing practices:
- Clear test method names following behavior-driven naming
- Good use of Arrange-Act-Assert pattern
- Appropriate assertions for both positive and negative scenarios
Consider adding more test scenarios for better coverage.
Consider enhancing the test suite with:
- Edge case test where
fromequalsto- Tests with extreme values (e.g., Long.MAX_VALUE)
Here's a suggested implementation:
@Test void shouldCreateByteSizeRangeWhenFromEqualsTo() { // Arrange long value = 100L; // Act ByteSizeRange byteSizeRange = new ByteSizeRange(value, value); // Assert assertNotNull(byteSizeRange); assertEquals(value, byteSizeRange.from()); assertEquals(value, byteSizeRange.to()); } @Test void shouldHandleExtremeValues() { // Arrange long from = 0L; long to = Long.MAX_VALUE; // Act ByteSizeRange byteSizeRange = new ByteSizeRange(from, to); // Assert assertNotNull(byteSizeRange); assertEquals(from, byteSizeRange.from()); assertEquals(to, byteSizeRange.to()); }
10-23: Consider using @DisplayName for better test reporting.The test method has a descriptive name, but adding @DisplayName would improve test report readability.
@Test +@DisplayName("Should create ByteSizeRange when 'from' size is less than or equal to 'to' size") void shouldCreateByteSizeRangeWhenFromSizeIsLessThanOrEqualToToSize() {
25-37: Consider parameterizing the negative test case.The test could be parameterized to cover multiple invalid scenarios efficiently.
@ParameterizedTest @DisplayName("Should throw BadRequestException for invalid size ranges") @CsvSource({ "100, 10", "1000, 500", "Long.MAX_VALUE, 0" }) void shouldThrowBadRequestExceptionForInvalidSizeRanges(long from, long to) { // Act & Assert BadRequestException exception = assertThrows(BadRequestException.class, () -> new ByteSizeRange(from, to)); assertEquals("'from' size must be less or equal to 'to' size", exception.getMessage()); }service/src/test/java/greencity/service/DotenvServiceImplTest.java (2)
39-75: Consider adding edge cases for secret key validation.While the current tests cover the main scenarios well, consider adding tests for:
- Null or empty secret keys
- Secret keys with special characters
- Very long secret keys
Here's a suggested test case:
@Test void validateSecretKeyShouldThrowExceptionWhenKeyIsEmpty() { DotenvServiceImpl spyService = spy(dotenvService); doNothing().when(spyService).reloadEnvFile(); assertThrows(IllegalArgumentException.class, () -> spyService.validateSecretKey("")); assertThrows(IllegalArgumentException.class, () -> spyService.validateSecretKey(null)); }
76-134: Consider refactoring to reduce code duplication.The file deletion tests are comprehensive but contain repeated setup code. Consider:
- Extracting the common setup to a @beforeeach method
- Creating constants for repeated values
- Testing with different file paths
Here's a suggested refactor:
private static final String VALID_SECRET_KEY = "validSecret"; private DotenvServiceImpl spyService; @BeforeEach void setUp() { spyService = spy(dotenvService); doNothing().when(spyService).validateSecretKey(VALID_SECRET_KEY); } @Test void deleteDotenvFileShouldDeleteIfSecretKeyIsValid() { try (MockedStatic<Files> mockedFiles = mockStatic(Files.class)) { mockedFiles.when(() -> Files.exists(any())).thenReturn(true); mockedFiles.when(() -> Files.deleteIfExists(any())).thenReturn(true); assertDoesNotThrow(() -> spyService.deleteDotenvFile(VALID_SECRET_KEY)); } }service/src/test/java/greencity/service/LogFileServiceImplTest.java (2)
26-39: Consider enhancing test class setup with constants and documentation.The test class setup is well-structured, but could be improved by:
- Adding class-level documentation to describe the purpose and scope of these tests
- Moving test data like "secret" to constants to improve maintainability
@ExtendWith(MockitoExtension.class) +/** + * Unit tests for {@link LogFileServiceImpl} class. + * Tests cover log file listing, content retrieval, and download functionality. + */ class LogFileServiceImplTest { public static final PageRequest PAGEABLE = PageRequest.of(0, 10); + private static final String TEST_SECRET_KEY = "secret"; + private static final String TEST_FILE_NAME = "test.log";
41-85: Add test coverage for pagination edge cases.The file listing tests are well-structured, but consider adding tests for:
- Pagination with empty pages
- Handling large number of files
- Edge cases with max page size
+ @Test + void getLogFilesListShouldHandleEmptyPage() { + String secretKey = TEST_SECRET_KEY; + File[] mockFiles = new File[]{new File("test.log")}; + PageRequest emptyPage = PageRequest.of(1, 10); // Page beyond available data + + LogFileServiceImpl spyService = spy(logFileService); + doReturn(mockFiles).when(spyService).listLogFilesFromFolder(); + + PageableDto<LogFileMetadataDto> result = spyService.getLogFilesList(emptyPage, null, secretKey); + + assertNotNull(result); + assertEquals(0, result.getPage().size()); + assertEquals(1, result.getTotalElements()); + }service/src/main/java/greencity/service/LogFileServiceImpl.java (2)
29-30: Consider making the logs directory path configurable.The hardcoded logs directory path using
System.getProperty("user.dir")could be problematic in different environments. Consider making it configurable through application properties.- private static final String LOGS_DIRECTORY = - System.getProperty("user.dir") + File.separator + "logs" + File.separator; + @Value("${logging.file.path:${user.dir}/logs/}") + private String logsDirectory;
209-212: Enhance log level matching accuracy.The current implementation using
contains()might give false positives. For example, "ERROR" would match in "ERRORS_SUMMARY". Consider using a more precise pattern matching approach.- return logLevel == null - || fileContent.contains(logLevel.toString()); + return logLevel == null + || Pattern.compile("(?m)^.*" + Pattern.quote(logLevel.toString()) + ".*$") + .matcher(fileContent) + .find();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/src/test/java/greencity/ModelUtils.java(4 hunks)core/src/test/java/greencity/controller/LogFileControllerTest.java(1 hunks)service-api/src/main/java/greencity/dto/logs/filter/LogFileFilterDto.java(1 hunks)service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java(1 hunks)service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java(1 hunks)service/src/main/java/greencity/config/DotenvConfig.java(1 hunks)service/src/main/java/greencity/service/DotenvServiceImpl.java(1 hunks)service/src/main/java/greencity/service/LogFileServiceImpl.java(1 hunks)service/src/test/java/greencity/ModelUtils.java(3 hunks)service/src/test/java/greencity/service/DotenvServiceImplTest.java(1 hunks)service/src/test/java/greencity/service/LogFileServiceImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- service-api/src/main/java/greencity/dto/logs/filter/LogFileFilterDto.java
- service/src/main/java/greencity/config/DotenvConfig.java
- core/src/test/java/greencity/controller/LogFileControllerTest.java
- service/src/main/java/greencity/service/DotenvServiceImpl.java
- core/src/test/java/greencity/ModelUtils.java
🔇 Additional comments (7)
service/src/test/java/greencity/ModelUtils.java (1)
3521-3527: LGTM! Test data factory method follows established patterns.The implementation provides a good test fixture with a mix of null and non-null values, which helps test various scenarios.
service/src/test/java/greencity/service/DotenvServiceImplTest.java (2)
1-38: Well-structured test class with proper setup!The class follows testing best practices with clear organization, appropriate use of Mockito extensions, and proper dependency injection setup.
1-134: Excellent test coverage with room for minor improvements!The test class demonstrates solid testing practices with comprehensive coverage of both success and failure scenarios. The use of mocking and spying is particularly well done. While there are opportunities for minor improvements in code organization and edge case coverage, the overall implementation is robust and maintainable.
service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java (2)
1-10: LGTM! Well-structured test class setup.The imports are clean and well-organized, following best practices.
27-39: LGTM! Excellent exception testing.The test effectively validates both the exception type and its message, ensuring proper error handling. The error message is clear and user-friendly.
service/src/main/java/greencity/service/LogFileServiceImpl.java (2)
134-144: Optimize file content reading in filter operations.The current implementation reads the entire file content for each filter operation, which could be inefficient when dealing with multiple large log files.
Consider implementing a more efficient filtering strategy:
- Apply metadata filters first (filename, size, date)
- Only read file content if content-based filters are present
113-123: Add bounds checking in pagination logic.The current pagination implementation might throw IndexOutOfBoundsException if the start offset exceeds the list size.
| @Test | ||
| void shouldThrowBadRequestExceptionWhenFromDateIsNull() { | ||
| // Arrange | ||
| Date from = null; | ||
| Date to = new Date(2025, 12, 31); // December 31, 2025 | ||
|
|
||
| // Act & Assert | ||
| assertThrows(NullPointerException.class, () -> new DateRange(from, to)); | ||
| } | ||
|
|
||
| @Test | ||
| void shouldThrowBadRequestExceptionWhenToDateIsNull() { | ||
| // Arrange | ||
| Date from = new Date(2025, 1, 1); // January 1, 2025 | ||
| Date to = null; | ||
|
|
||
| // Act & Assert | ||
| assertThrows(NullPointerException.class, () -> new DateRange(from, to)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align test method names with actual behavior.
The test method names suggest they're expecting BadRequestException, but they're actually testing for NullPointerException. Consider renaming these methods to accurately reflect their behavior:
- void shouldThrowBadRequestExceptionWhenFromDateIsNull()
+ void shouldThrowNullPointerExceptionWhenFromDateIsNull()
- void shouldThrowBadRequestExceptionWhenToDateIsNull()
+ void shouldThrowNullPointerExceptionWhenToDateIsNull()📝 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.
| @Test | |
| void shouldThrowBadRequestExceptionWhenFromDateIsNull() { | |
| // Arrange | |
| Date from = null; | |
| Date to = new Date(2025, 12, 31); // December 31, 2025 | |
| // Act & Assert | |
| assertThrows(NullPointerException.class, () -> new DateRange(from, to)); | |
| } | |
| @Test | |
| void shouldThrowBadRequestExceptionWhenToDateIsNull() { | |
| // Arrange | |
| Date from = new Date(2025, 1, 1); // January 1, 2025 | |
| Date to = null; | |
| // Act & Assert | |
| assertThrows(NullPointerException.class, () -> new DateRange(from, to)); | |
| } | |
| @Test | |
| void shouldThrowNullPointerExceptionWhenFromDateIsNull() { | |
| // Arrange | |
| Date from = null; | |
| Date to = new Date(2025, 12, 31); // December 31, 2025 | |
| // Act & Assert | |
| assertThrows(NullPointerException.class, () -> new DateRange(from, to)); | |
| } | |
| @Test | |
| void shouldThrowNullPointerExceptionWhenToDateIsNull() { | |
| // Arrange | |
| Date from = new Date(2025, 1, 1); // January 1, 2025 | |
| Date to = null; | |
| // Act & Assert | |
| assertThrows(NullPointerException.class, () -> new DateRange(from, to)); | |
| } |
| @Test | ||
| void shouldCreateDateRangeWhenFromDateIsBeforeOrEqualToToDate() { | ||
| // Arrange | ||
| Date from = new Date(2025, 1, 1); // January 1, 2025 | ||
| Date to = new Date(2025, 12, 31); // December 31, 2025 | ||
|
|
||
| // Act | ||
| DateRange dateRange = new DateRange(from, to); | ||
|
|
||
| // Assert | ||
| assertNotNull(dateRange); | ||
| assertEquals(from, dateRange.from()); | ||
| assertEquals(to, dateRange.to()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using modern date-time API.
While the test structure is excellent with clear AAA pattern, the Date(year, month, day) constructor is deprecated. Consider using java.time.LocalDate or java.time.Instant for more reliable date handling.
Here's how you could refactor it:
- Date from = new Date(2025, 1, 1); // January 1, 2025
- Date to = new Date(2025, 12, 31); // December 31, 2025
+ Date from = Date.from(LocalDate.of(2025, 1, 1).atStartOfDay(ZoneId.systemDefault()).toInstant());
+ Date to = Date.from(LocalDate.of(2025, 12, 31).atStartOfDay(ZoneId.systemDefault()).toInstant());📝 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.
| @Test | |
| void shouldCreateDateRangeWhenFromDateIsBeforeOrEqualToToDate() { | |
| // Arrange | |
| Date from = new Date(2025, 1, 1); // January 1, 2025 | |
| Date to = new Date(2025, 12, 31); // December 31, 2025 | |
| // Act | |
| DateRange dateRange = new DateRange(from, to); | |
| // Assert | |
| assertNotNull(dateRange); | |
| assertEquals(from, dateRange.from()); | |
| assertEquals(to, dateRange.to()); | |
| } | |
| @Test | |
| void shouldCreateDateRangeWhenFromDateIsBeforeOrEqualToToDate() { | |
| // Arrange | |
| - Date from = new Date(2025, 1, 1); // January 1, 2025 | |
| - Date to = new Date(2025, 12, 31); // December 31, 2025 | |
| + Date from = Date.from(LocalDate.of(2025, 1, 1) | |
| + .atStartOfDay(ZoneId.systemDefault()) | |
| + .toInstant()); | |
| + Date to = Date.from(LocalDate.of(2025, 12, 31) | |
| + .atStartOfDay(ZoneId.systemDefault()) | |
| + .toInstant()); | |
| // Act | |
| DateRange dateRange = new DateRange(from, to); | |
| // Assert | |
| assertNotNull(dateRange); | |
| assertEquals(from, dateRange.from()); | |
| assertEquals(to, dateRange.to()); | |
| } |
service/src/test/java/greencity/service/LogFileServiceImplTest.java
Outdated
Show resolved
Hide resolved
| try { | ||
| String content = Files.readString(file.toPath()); | ||
| return content; | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using buffered reading for large files.
Reading the entire file content at once with Files.readString() could lead to memory issues with large log files. Consider using buffered reading or streaming for better memory management.
- String content = Files.readString(file.toPath());
- return content;
+ return Files.lines(file.toPath())
+ .collect(Collectors.joining(System.lineSeparator()));Committable suggestion skipped: line range outside the PR's diff.
…9-display-log-files # Conflicts: # service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java # service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java # service/src/test/java/greencity/ModelUtils.java
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
service/src/main/java/greencity/service/LogFileServiceImpl.java (2)
114-124:⚠️ Potential issueCheck offset bounds in pagination logic.
The pagination logic needs bounds checking to prevent IndexOutOfBoundsException when the offset exceeds the total elements.
private PageableDto<LogFileMetadataDto> applyPagination(List<LogFileMetadataDto> dtos, Pageable pageable) { long totalElements = dtos.size(); int totalPages = (int) Math.ceil((double) totalElements / pageable.getPageSize()); int start = (int) pageable.getOffset(); + if (start >= totalElements) { + return new PageableDto<>(List.of(), totalElements, pageable.getPageNumber(), totalPages); + } int end = Math.min((start + pageable.getPageSize()), dtos.size()); List<LogFileMetadataDto> paginatedList = dtos.subList(start, end);
135-150: 🛠️ Refactor suggestionAvoid reading file content multiple times for filtering.
The current implementation reads the file content for each filter operation, which could be inefficient for large files or many filters.
private boolean filterFileDto(LogFileMetadataDto fileDto, LogFileFilterDto filterDto, String secretKey) { if (filterDto == null) { return true; } - String fileContent = getLogFileContent(fileDto.getFilename(), secretKey); + // Read content only if needed for content-based filters + String fileContent = null; + if (filterDto.fileContentQuery() != null || filterDto.logLevel() != null) { + fileContent = getLogFileContent(fileDto.getFilename(), secretKey); + } return matchesFileNameQuery(fileDto.getFilename(), filterDto.fileNameQuery()) - && matchesFileContentQuery(fileContent, filterDto.fileContentQuery()) + && (filterDto.fileContentQuery() == null || matchesFileContentQuery(fileContent, filterDto.fileContentQuery())) && matchesByteSize(fileDto.getByteSize(), filterDto.byteSizeRange()) && matchesDateRange( fileDto.getLastModified() .toInstant() .atZone(ZoneId.systemDefault()) .toLocalDateTime(), filterDto.dateRange()) - && matchesLogLevel(fileContent, filterDto.logLevel()); + && (filterDto.logLevel() == null || matchesLogLevel(fileContent, filterDto.logLevel()));
🧹 Nitpick comments (3)
service-api/src/main/java/greencity/dto/logs/filter/DateRange.java (1)
7-9: Excellent use of records and validation annotations!The record is a perfect choice for this immutable DTO, and the validation annotations are well-placed with clear messages.
Consider adding
@Pastor@PastOrPresentvalidation for log file dates since logs are typically historical records:public record DateRange( - @NotNull(message = "From date cannot be null") LocalDateTime from, - @NotNull(message = "To date cannot be null") LocalDateTime to) { + @NotNull(message = "From date cannot be null") + @PastOrPresent(message = "From date must not be in the future") + LocalDateTime from, + @NotNull(message = "To date cannot be null") + @PastOrPresent(message = "To date must not be in the future") + LocalDateTime to) {service/src/main/java/greencity/service/LogFileServiceImpl.java (1)
31-32: Consider externalizing the logs directory path to configuration.The hardcoded logs directory path might not work consistently across different environments. Consider moving this to an application property or environment variable for better configurability.
- private static final String LOGS_DIRECTORY = - System.getProperty("user.dir") + File.separator + "logs" + File.separator; + @Value("${logging.file.path:${user.dir}/logs/}") + private String logsDirectory;core/src/main/java/greencity/controller/LogFileController.java (1)
102-110: Add Content-Length header for better download handlingThe download implementation is good but could be improved:
- Add Content-Length header for better client-side progress tracking
- Consider adding ETag support for caching
return ResponseEntity.ok() .contentType(MediaType.APPLICATION_OCTET_STREAM) .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + filename + "\"") + .header(HttpHeaders.CONTENT_LENGTH, String.valueOf(resource.contentLength())) + .header(HttpHeaders.ETAG, "\"" + resource.contentLength() + "\"") .body(logFileService.getDownloadLogFileUrl(filename, secretKey));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/src/main/java/greencity/controller/LogFileController.java(1 hunks)core/src/test/java/greencity/ModelUtils.java(4 hunks)core/src/test/java/greencity/controller/LogFileControllerTest.java(1 hunks)service-api/src/main/java/greencity/dto/logs/LogFileRequestDto.java(1 hunks)service-api/src/main/java/greencity/dto/logs/filter/DateRange.java(1 hunks)service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java(1 hunks)service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java(1 hunks)service/src/main/java/greencity/service/LogFileServiceImpl.java(1 hunks)service/src/test/java/greencity/ModelUtils.java(2 hunks)service/src/test/java/greencity/service/DotenvServiceImplTest.java(1 hunks)service/src/test/java/greencity/service/LogFileServiceImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- core/src/test/java/greencity/ModelUtils.java
- service-api/src/main/java/greencity/dto/logs/LogFileRequestDto.java
- service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java
- service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java
- service/src/test/java/greencity/service/LogFileServiceImplTest.java
- core/src/test/java/greencity/controller/LogFileControllerTest.java
- service/src/test/java/greencity/service/DotenvServiceImplTest.java
- service/src/test/java/greencity/ModelUtils.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
service-api/src/main/java/greencity/dto/logs/filter/DateRange.java (2)
1-6: Well-structured imports with modern Jakarta EE validation!Good choice using the Jakarta validation package and keeping imports clean and organized.
10-14:❓ Verification inconclusive
Good validation logic in the compact constructor!
The validation ensures a logical date range with a clear error message.
Consider these edge cases:
- Should we handle time zones explicitly? Verify if the application consistently uses the same time zone for all dates.
- While
@NotNullprevents null values, defensive null checks in the constructor might make the code more robust.
🏁 Script executed:
#!/bin/bash # Search for time zone handling patterns in the codebase rg -l "ZoneId|ZoneOffset|TimeZone" --type javaLength of output: 1355
Strong validation in the compact constructor!
The date range check is clear and produces an appropriate error when the “from” date is after the “to” date. A couple of potential improvements to further enhance robustness:
Time Zone Handling:
Considering that several files in the codebase explicitly work with time zones (e.g., through ZoneId, ZoneOffset, TimeZone), please verify that the dates used in this validation are normalized to a consistent time zone. This ensures that comparisons remain accurate across different parts of the application.Defensive Null Checks:
Even though @NotNull annotations might already prevent null values, adding defensive null checks directly in the constructor could provide an extra layer of safety against unexpected input.These suggestions are optional improvements if the rest of the application already manages these concerns adequately.
core/src/test/java/greencity/controller/LogFileControllerTest.java
Outdated
Show resolved
Hide resolved
service-api/src/main/java/greencity/dto/logs/LogFileMetadataDto.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/src/main/java/greencity/controller/LogFileController.java (3)
52-62: 🛠️ Refactor suggestionUse GET instead of POST for retrieving log files list
The endpoint uses POST for a read operation, which isn't RESTful. Consider:
- Change to GET and move filter parameters to query parameters
- Use request params for filtering instead of request body
124-129: 🛠️ Refactor suggestionAdd safeguards for critical .env file deletion
The current implementation lacks important safety measures:
- Add a confirmation mechanism
- Implement backup creation
- Add audit logging
32-35: 🛠️ Refactor suggestionConsider enhancing security measures
While the profile restriction is good, consider these security enhancements:
- Move secret key to request headers
- Add rate limiting for sensitive operations
- Consider IP whitelisting for additional security
Add rate limiting annotation:
@RestController @RequiredArgsConstructor @RequestMapping("/logs") @Profile({"dev", "test"}) +@RateLimiter(name = "logFileController") public class LogFileController {
🧹 Nitpick comments (20)
core/src/test/java/greencity/ModelUtils.java (1)
601-608: Consider enhancing test data coverage.The
getLogFileFilterDto()method provides a basic test fixture, but it could be enhanced to cover more test scenarios:
- The
byteSizeRangeanddateRangeparameters are set to null, which might miss testing these filter conditions.- The method only creates a filter with INFO level, but testing with other log levels would be valuable.
Consider expanding the method to include more test scenarios:
public static LogFileFilterDto getLogFileFilterDto() { return new LogFileFilterDto( "filename", "fileContent", - null, - null, + new ByteSizeRange(0L, 1000L), + new DateRange(LocalDateTime.now().minusDays(7), LocalDateTime.now()), LogLevel.INFO); } +public static LogFileFilterDto getLogFileFilterDtoWithDebugLevel() { + return new LogFileFilterDto( + "debug_file", + "debug_content", + new ByteSizeRange(0L, 500L), + new DateRange(LocalDateTime.now().minusDays(1), LocalDateTime.now()), + LogLevel.DEBUG); +}service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java (1)
10-20: Consider enhancing test coverage with edge cases.While the test is well-structured, consider adding these improvements:
- Test edge case where
fromequalsto- Test boundary values (0, Long.MAX_VALUE)
- Split assertions into separate test methods for better failure isolation
Example addition:
@Test void shouldCreateByteSizeRangeWhenFromEqualsTo() { long size = 100L; ByteSizeRange byteSizeRange = new ByteSizeRange(size, size); assertNotNull(byteSizeRange); assertEquals(size, byteSizeRange.from()); assertEquals(size, byteSizeRange.to()); }service/src/test/java/greencity/service/UserNotificationServiceImplTest.java (4)
130-150: Consider parameterizing language codes in test data providers.The test data providers for English and Ukrainian scenarios have duplicated structure but differ only in language-specific content. Consider refactoring to reduce duplication.
+ private static Stream<Arguments> getNotificationScenarios() { + return Stream.of( + Arguments.of( + "en", + "You have been tagged in the comment.", + "{user} tagged you in the event «{secondMessage}».", + List.of(getUser()), + List.of(1L), + List.of("Taras")), + Arguments.of( + "ua", + "Вас тегнули в коментарі", + "{user} позначив вас у події «{secondMessage}».", + List.of(getUser()), + List.of(1L), + List.of("Taras")) + // Add more test cases here + ); + }Also applies to: 270-303
622-693: Add test coverage for edge cases in like notification handling.The like notification tests cover basic scenarios but could benefit from additional edge cases:
- Multiple users liking the same content
- Concurrent like/unlike operations
- Invalid notification types
+ @Test + @DisplayName("createOrUpdateLikeNotification handles concurrent likes correctly") + void testCreateOrUpdateLikeNotification_ConcurrentLikes() { + UserVO targetUserVO = mock(UserVO.class); + List<UserVO> actionUsers = Arrays.asList( + mock(UserVO.class), + mock(UserVO.class), + mock(UserVO.class) + ); + // Test implementation + }
724-731: Consider adding assertions for empty result handling.While the test verifies the interactions, it would be beneficial to add explicit assertions about the behavior when no habit assigns are found.
@Test void checkLastDayOfHabitPrimaryDurationToMessageNoHabitAssigns() { when(habitAssignRepo.getHabitAssignsWithLastDayOfPrimaryDurationToMessage()).thenReturn(Collections.emptyList()); userNotificationService.checkLastDayOfHabitPrimaryDurationToMessage(); verify(habitAssignRepo).getHabitAssignsWithLastDayOfPrimaryDurationToMessage(); verifyNoMoreInteractions(modelMapper, notificationService); + // Add explicit assertions + verify(notificationService, never()).sendEmailNotification(any()); + verify(messagingTemplate, never()).convertAndSend(anyString(), any()); }
733-754: Add test coverage for error scenarios in habit duration notification.The test covers the happy path but should also verify error handling:
- Invalid habit assignments
- Null user references
- Failed notification delivery
+ @Test + void checkLastDayOfHabitPrimaryDurationToMessageShouldHandleErrors() { + Habit habit = getHabit().setHabitTranslations(Collections.emptyList()); // Invalid state + User user = getUser().setId(2L); + HabitAssign habitAssign = getHabitAssign(HabitAssignStatus.INPROGRESS) + .setUser(user) + .setHabit(habit); + + when(habitAssignRepo.getHabitAssignsWithLastDayOfPrimaryDurationToMessage()) + .thenReturn(List.of(habitAssign)); + // Test implementation for error handling + }core/src/main/resources/logback.xml (1)
14-30: RollingFile Appender and File Rotation Policy
The newly introducedrollingFileappender efficiently combines time-based and size-based triggering:
- The
<file>element directs logs to${LOG_PATH}/application.log.- The
<rollingPolicy>utilizes a daily rotation pattern with<fileNamePattern>${LOG_PATH}/application-%d{yyyy-MM-dd}.%i.log</fileNamePattern>, which supports multiple files per day.- The nested
<timeBasedFileNamingAndTriggeringPolicy>withSizeAndTimeBasedFNATPeffectively enforces the maximum file size using the${LOG_MAX_SIZE}property.Suggestion: It might be worthwhile to verify that
ch.qos.logback.core.rolling.SizeAndTimeBasedFNATPis fully supported in your current Logback version. In some contexts, the more recentSizeAndTimeBasedRollingPolicyis recommended. Consider reviewing the latest Logback documentation to confirm the best practice for your environment.core/src/test/java/greencity/controller/EventControllerTest.java (3)
84-111: Consider parameterizing the filter test.The test covers basic filtering functionality but could be enhanced with parameterized tests to cover different filter combinations.
+@ParameterizedTest +@MethodSource("filterTestCases") +void getAllEventsTest(Long userId, FilterEventDto filterEventDto, PageableAdvancedDto<EventDto> expected) { Pageable pageable = PageRequest.of(0, 20); - Long userId = 1L; - PageableAdvancedDto<EventDto> eventDtoPageableAdvancedDto = getEventDtoPageableAdvancedDto(pageable); - FilterEventDto filterEventDto = ModelUtils.getFilterEventDto(); ObjectMapper objectMapper = new ObjectMapper(); objectMapper.findAndRegisterModules(); - String expectedJson = objectMapper.writeValueAsString(eventDtoPageableAdvancedDto); + String expectedJson = objectMapper.writeValueAsString(expected); when(eventService.getEvents(pageable, filterEventDto, userId)) - .thenReturn(eventDtoPageableAdvancedDto); + .thenReturn(expected);
605-627: Consider using a test data builder pattern.The test data creation methods are quite lengthy. Consider implementing a builder pattern for test data to improve readability and maintainability.
@Builder public class EventTestDataBuilder { private String title = "string"; private String description = "stringstringstringstringstringstringstringstring"; private boolean open = true; // ... other fields public AddEventDtoRequest buildAddEventRequest() { // Build and return AddEventDtoRequest } public UpdateEventRequestDto buildUpdateEventRequest() { // Build and return UpdateEventRequestDto } }Also applies to: 629-653
1-790: Consider adding integration tests.While the unit tests are comprehensive, consider adding integration tests to verify the interaction between the controller and actual services.
service-api/src/main/java/greencity/dto/logs/LogFileMetadataDto.java (1)
10-24: Align JSON field names with DTO fieldsThe JSON example structure has some inconsistencies:
- The field name "size" in JSON should match "byteSize" from the record
- The date format includes timezone offset ("+00:00") which might be inconsistent with LocalDateTime
Consider updating the JSON structure:
- "size": 0, + "byteSize": 0, - "lastModified": "2025-01-01T00:00:00.000+00:00" + "lastModified": "2025-01-01T00:00:00.000"README.md (9)
15-18: Clear and Informative Project Description
The “About the project” section clearly describes GreenCity’s aim. Consider reviewing the phrasing "А user can start doing an environment-friendly habit" as the first character in "А user" appears unusual; a simple "A user" may improve consistency.
31-32: Refine Contribution Guidelines Wording
In the sentence “Though there are two GitHub projects (...) all of the issues are listed in the first one…” consider removing the word “of” (i.e. “all issues are listed…”) for conciseness.🧰 Tools
🪛 LanguageTool
[style] ~31-~31: Consider removing “of” to be more concise
Context: ...ts/GreenCityClient) for front-end part) all of the issues are listed in the first one - [G...(ALL_OF_THE)
33-33: Enhance Punctuation for Clarity
The note “NOTE: make sure that your code passes checkstyle. Otherwise your pull request will be declined” would read slightly better with a comma after “Otherwise” (i.e. “Otherwise, your pull request will be declined”).🧰 Tools
🪛 LanguageTool
[uncategorized] ~33-~33: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ... sure that your code passes checkstyle. Otherwise your pull request will be declined**. S...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
55-57: Maintain Consistent List Punctuation
The configuration list items (e.g., “*Name:GreenCityApplication.”, “*Use classpath of modules:core”, “*JRE:21.”) show slight inconsistencies in punctuation spacing. Consider standardizing the style (for example, remove extra spaces around colons) to improve readability.
63-64: Clarify Database Setup Instructions
In the step “Go todao->src->test->resources->sql, find fileinsert.sql, open it, pressCtrl + A->RMB->Execute. (that will run script which fill your db)”, consider rephrasing “fill your db” to “populate your database” for improved clarity and professionalism.🧰 Tools
🪛 LanguageTool
[uncategorized] ~63-~63: Loose punctuation mark.
Context: ...RE:21`.
Enviroment variables:
84-84: Grammar Improvement in Database Creation Step
The instruction “Create database called "greencity". Here's example with Docker CLI:” would read better as “Create a database called 'greencity'. Here's an example using Docker CLI:” to improve grammatical correctness.
90-90: Enhance Clarity in Environment Variable Note
In the note “If you start your project locally, don't forget to change DATASOURCE_URL environment variable so that it points to localhost:5432,” consider adding “the” before “DATASOURCE_URL environment variable” (i.e. “change the DATASOURCE_URL environment variable…”) for improved clarity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~90-~90: Possible missing article found.
Context: ...ontactproject members.
- Create database called "greencity". Here's example with...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~90-~90: Possible missing article found.
Context: ...ate database called "greencity". Here's example with Docker CLI:docker run ... (AI_HYDRA_LEO_MISSING_AN) </details> </details> --- `113-117`: **Refine User Role Update Instructions** The guidance “By default, new registered users have role *USER*. To overcome this you need to update record that corresponds to your user in the local database. For example, `UPDATE users SET role = 1 WHERE id = <your_user_id>;`” could be refined to: - “By default, newly registered users have the role *USER*. To grant administrative privileges, update the record corresponding to your user in the local database (e.g. `UPDATE users SET role = 1 WHERE id = <your_user_id>;`).” This adjustment helps with grammatical precision and clarity. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [style] ~115-~115: Using many exclamation marks might seem excessive (in this case: 10 exclamation marks for a text that’s 7013 characters long) Context: ...login, you will see "authorized" text.   </details> </details> --- `121-121`: **Correct Verb Agreement in Front-end Connection Section** The sentence “There is no special configurations required...” should be adjusted to “There are no special configurations required...” to correctly match the plural subject “configurations.” <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [typographical] ~121-~121: It seems that a comma is missing. Context: ...d users have role *USER*. To overcome this you need to update record that correspo... (IN_ORDER_TO_VB_COMMA) --- [uncategorized] ~121-~121: Possible missing article found. Context: .... To overcome this you need to update record that corresponds to your user in the ... (AI_HYDRA_LEO_MISSING_THE) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 36349c184625c301dc89b7c902c8b9a186b0b680 and a8733c4597c7cb299379bf251923c91246b38e87. </details> <details> <summary>📒 Files selected for processing (61)</summary> * `README.md` (1 hunks) * `core/src/main/java/greencity/config/SecurityConfig.java` (1 hunks) * `core/src/main/java/greencity/constant/HttpStatuses.java` (1 hunks) * `core/src/main/java/greencity/controller/EventController.java` (1 hunks) * `core/src/main/java/greencity/controller/LogFileController.java` (1 hunks) * `core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java` (1 hunks) * `core/src/main/resources/logback.xml` (1 hunks) * `core/src/test/java/greencity/ModelUtils.java` (1 hunks) * `core/src/test/java/greencity/controller/EventControllerTest.java` (1 hunks) * `core/src/test/java/greencity/controller/LogFileControllerTest.java` (1 hunks) * `core/src/test/java/greencity/webcontroller/ManagementEventControllerTest.java` (1 hunks) * `dao/src/main/java/greencity/entity/HabitInvitation.java` (1 hunks) * `dao/src/main/java/greencity/enums/InvitationStatus.java` (1 hunks) * `dao/src/main/java/greencity/enums/NotificationType.java` (1 hunks) * `dao/src/main/java/greencity/repository/UserRepo.java` (1 hunks) * `service-api/src/main/java/greencity/constant/AppConstant.java` (1 hunks) * `service-api/src/main/java/greencity/constant/ErrorMessage.java` (1 hunks) * `service-api/src/main/java/greencity/constant/FriendTupleConstant.java` (1 hunks) * `service-api/src/main/java/greencity/dto/event/EventDateInformationDto.java` (1 hunks) * `service-api/src/main/java/greencity/dto/event/EventInformationDto.java` (1 hunks) * `service-api/src/main/java/greencity/dto/event/EventResponseDto.java` (1 hunks) * `service-api/src/main/java/greencity/dto/logs/LogFileMetadataDto.java` (1 hunks) * `service-api/src/main/java/greencity/dto/logs/LogFileRequestDto.java` (1 hunks) * `service-api/src/main/java/greencity/dto/logs/filter/ByteSizeRange.java` (1 hunks) * `service-api/src/main/java/greencity/dto/logs/filter/DateRange.java` (1 hunks) * `service-api/src/main/java/greencity/dto/logs/filter/LogFileFilterDto.java` (1 hunks) * `service-api/src/main/java/greencity/dto/notification/NotificationDto.java` (1 hunks) * `service-api/src/main/java/greencity/dto/notification/NotificationInviteDto.java` (1 hunks) * `service-api/src/main/java/greencity/enums/InvitationStatus.java` (1 hunks) * `service-api/src/main/java/greencity/enums/NotificationType.java` (1 hunks) * `service-api/src/main/java/greencity/exception/exceptions/BadSecretKeyException.java` (1 hunks) * `service-api/src/main/java/greencity/exception/exceptions/FileReadException.java` (1 hunks) * `service-api/src/main/java/greencity/exception/exceptions/FunctionalityNotAvailableException.java` (1 hunks) * `service-api/src/main/java/greencity/service/DotenvService.java` (1 hunks) * `service-api/src/main/java/greencity/service/EventService.java` (1 hunks) * `service-api/src/main/java/greencity/service/HabitInvitationService.java` (1 hunks) * `service-api/src/main/java/greencity/service/LogFileService.java` (1 hunks) * `service-api/src/main/java/greencity/service/NotificationFriendService.java` (1 hunks) * `service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java` (1 hunks) * `service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java` (1 hunks) * `service/pom.xml` (1 hunks) * `service/src/main/java/greencity/config/DotenvConfig.java` (1 hunks) * `service/src/main/java/greencity/config/PasswordEncoderConfig.java` (1 hunks) * `service/src/main/java/greencity/mapping/events/EventDtoMapper.java` (1 hunks) * `service/src/main/java/greencity/mapping/events/EventResponseDtoMapper.java` (1 hunks) * `service/src/main/java/greencity/rating/constant/RatingPointsNames.java` (1 hunks) * `service/src/main/java/greencity/service/DotenvServiceImpl.java` (1 hunks) * `service/src/main/java/greencity/service/EcoNewsServiceImpl.java` (1 hunks) * `service/src/main/java/greencity/service/HabitInvitationServiceImpl.java` (1 hunks) * `service/src/main/java/greencity/service/LogFileServiceImpl.java` (1 hunks) * `service/src/main/java/greencity/service/NotificationFriendServiceImpl.java` (1 hunks) * `service/src/main/java/greencity/service/UserNotificationServiceImpl.java` (1 hunks) * `service/src/main/java/greencity/utils/EventUtils.java` (1 hunks) * `service/src/test/java/greencity/TestConst.java` (1 hunks) * `service/src/test/java/greencity/mapping/events/EventResponseDtoMapperTest.java` (1 hunks) * `service/src/test/java/greencity/service/DotenvServiceImplTest.java` (1 hunks) * `service/src/test/java/greencity/service/HabitInvitationServiceImplTest.java` (1 hunks) * `service/src/test/java/greencity/service/LogFileServiceImplTest.java` (1 hunks) * `service/src/test/java/greencity/service/NotificationFriendServiceImplTest.java` (1 hunks) * `service/src/test/java/greencity/service/UserNotificationServiceImplTest.java` (1 hunks) * `service/src/test/java/greencity/utils/EventUtilsTest.java` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (31)</summary> * service-api/src/main/java/greencity/enums/InvitationStatus.java * service-api/src/main/java/greencity/dto/event/EventInformationDto.java * service/src/main/java/greencity/rating/constant/RatingPointsNames.java * service-api/src/main/java/greencity/dto/event/EventResponseDto.java * service-api/src/main/java/greencity/dto/event/EventDateInformationDto.java * service/src/test/java/greencity/utils/EventUtilsTest.java * service-api/src/main/java/greencity/constant/AppConstant.java * service-api/src/main/java/greencity/service/HabitInvitationService.java * service/src/main/java/greencity/mapping/events/EventDtoMapper.java * service/src/test/java/greencity/TestConst.java * service-api/src/main/java/greencity/dto/notification/NotificationInviteDto.java * service-api/src/main/java/greencity/constant/FriendTupleConstant.java * dao/src/main/java/greencity/enums/InvitationStatus.java * service-api/src/main/java/greencity/service/NotificationFriendService.java * service/src/main/java/greencity/service/NotificationFriendServiceImpl.java * service-api/src/main/java/greencity/enums/NotificationType.java * service-api/src/main/java/greencity/dto/notification/NotificationDto.java * dao/src/main/java/greencity/entity/HabitInvitation.java * service/src/test/java/greencity/service/NotificationFriendServiceImplTest.java * service/src/main/java/greencity/service/EcoNewsServiceImpl.java * service/src/main/java/greencity/utils/EventUtils.java * service/src/main/java/greencity/mapping/events/EventResponseDtoMapper.java * dao/src/main/java/greencity/enums/NotificationType.java * service/src/test/java/greencity/mapping/events/EventResponseDtoMapperTest.java * service-api/src/main/java/greencity/service/EventService.java * service/src/main/java/greencity/service/HabitInvitationServiceImpl.java * service/src/test/java/greencity/service/HabitInvitationServiceImplTest.java * core/src/test/java/greencity/webcontroller/ManagementEventControllerTest.java * service/src/main/java/greencity/service/UserNotificationServiceImpl.java * dao/src/main/java/greencity/repository/UserRepo.java * core/src/main/java/greencity/controller/EventController.java </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (20)</summary> * core/src/main/java/greencity/constant/HttpStatuses.java * service-api/src/main/java/greencity/exception/exceptions/FunctionalityNotAvailableException.java * service-api/src/main/java/greencity/exception/exceptions/FileReadException.java * service/pom.xml * service/src/main/java/greencity/config/PasswordEncoderConfig.java * service-api/src/main/java/greencity/dto/logs/filter/ByteSizeRange.java * service-api/src/main/java/greencity/dto/logs/filter/DateRange.java * service-api/src/main/java/greencity/service/DotenvService.java * service-api/src/main/java/greencity/dto/logs/LogFileRequestDto.java * service/src/main/java/greencity/config/DotenvConfig.java * core/src/main/java/greencity/config/SecurityConfig.java * service-api/src/main/java/greencity/dto/logs/filter/LogFileFilterDto.java * service-api/src/main/java/greencity/exception/exceptions/BadSecretKeyException.java * core/src/test/java/greencity/controller/LogFileControllerTest.java * service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java * service-api/src/main/java/greencity/service/LogFileService.java * service/src/test/java/greencity/service/LogFileServiceImplTest.java * service/src/main/java/greencity/service/DotenvServiceImpl.java * service/src/test/java/greencity/service/DotenvServiceImplTest.java * service/src/main/java/greencity/service/LogFileServiceImpl.java </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>README.md</summary> [style] ~31-~31: Consider removing “of” to be more concise Context: ...ts/GreenCityClient) for front-end part) all of the issues are listed in the first one - [G... (ALL_OF_THE) --- [uncategorized] ~33-~33: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’. Context: ... sure that your code passes checkstyle. Otherwise your pull request will be declined**. S... (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA) --- [uncategorized] ~51-~51: Possible missing comma found. Context: ... new local branch "dev"). 3. After this `git pull origin dev` (for update last ... (AI_HYDRA_LEO_MISSING_COMMA) --- [uncategorized] ~59-~59: Loose punctuation mark. Context: ...n` -> `+` -> `Application`. * `Name` : `GreenCityApplication`. * `Use classpa... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~60-~60: Loose punctuation mark. Context: ...lication`. * `Use classpath of modules`:`core` * `JRE` : `21`. 6. `Enviromen... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~63-~63: Loose punctuation mark. Context: ...RE` : `21`. 6. `Enviroment variables`:  --- [uncategorized] ~90-~90: Possible missing article found. Context: ...ontact > project members. 1. Create database called "greencity". Here's example with... (AI_HYDRA_LEO_MISSING_A) --- [uncategorized] ~90-~90: Possible missing article found. Context: ...ate database called "greencity". Here's example with Docker CLI: ```shell docker run ... (AI_HYDRA_LEO_MISSING_AN) --- [grammar] ~103-~103: Possible agreement error. The noun ‘id’ seems to be countable. Context: ...e database to activated, using command `UPDATE users SET user_status = 2 WHERE id = <your_user_id>;`. Also, you will nee... (CD_NN) --- [style] ~115-~115: Using many exclamation marks might seem excessive (in this case: 10 exclamation marks for a text that’s 7013 characters long) Context: ...login, you will see "authorized" text.   --- [uncategorized] ~120-~120: Possible missing article found. Context: ... By default, new registered users have role *USER*. To overcome this you need to ... (AI_HYDRA_LEO_MISSING_THE) --- [typographical] ~121-~121: It seems that a comma is missing. Context: ...d users have role *USER*. To overcome this you need to update record that correspo... (IN_ORDER_TO_VB_COMMA) --- [uncategorized] ~121-~121: Possible missing article found. Context: .... To overcome this you need to update record that corresponds to your user in the ... (AI_HYDRA_LEO_MISSING_THE) --- [uncategorized] ~123-~123: It seems likely that a singular genitive (’s) apostrophe is missing. Context: ...r in the local database. For example, `UPDATE users SET role = 1 WHERE id = <your_user_id>;... (AI_HYDRA_LEO_APOSTROPHE_S_XS) --- [uncategorized] ~127-~127: The verb “is” doesn’t seem to fit in this context, “are” is probably more formally correct. Context: ...## 4.4. Connect with front-end There is no special configurations required. Jus... (AI_HYDRA_LEO_CPT_IS_ARE) </details> </details> </details> <details> <summary>🔇 Additional comments (16)</summary><blockquote> <details> <summary>core/src/test/java/greencity/ModelUtils.java (1)</summary> `44-44`: **LGTM! Well-organized imports.** The new imports for `LogFileFilterDto` and `LogLevel` are correctly placed in their respective groups (project-specific imports and Spring framework imports). Also applies to: 88-88 </details> <details> <summary>service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java (2)</summary> `1-7`: **Well-structured package and imports!** Good practices observed: - Clean package organization under test directory - Specific imports without wildcards - Appropriate use of static imports for assertions --- `22-32`: **Excellent exception testing!** The test demonstrates good practices: - Clear validation of the error scenario - Proper use of assertThrows - Verification of the error message </details> <details> <summary>service/src/test/java/greencity/service/UserNotificationServiceImplTest.java (2)</summary> `1-80`: **LGTM! Well-organized imports and package structure.** The imports are logically grouped and necessary for the test implementation. --- `81-102`: **LGTM! Clean test setup with proper mock declarations.** The test class is well-structured with clear mock declarations and constant definitions. </details> <details> <summary>core/src/main/resources/logback.xml (3)</summary> `3-6`: **Property Definitions and Logging Pattern** The definitions for `DATE_FORMAT`, `LOG_PATH`, `LOG_MAX_SIZE`, and `LOG_PATTERN` are clear and enhance configurability. Using a relative path (i.e., `./logs`) is acceptable in many environments, but please ensure that the application has write permissions in the target deployment context. --- `8-12`: **Console Appender Configuration** The console appender is set up correctly to use the defined `LOG_PATTERN`, ensuring uniform log message formatting. No changes are needed here. --- `32-35`: **Root Logger Configuration** The root logger now includes references to both the `console` and `rollingFile` appenders. This ensures that log messages are correctly sent to both outputs, supporting robust logging and easier troubleshooting. </details> <details> <summary>core/src/test/java/greencity/controller/EventControllerTest.java (3)</summary> `76-82`: **LGTM! Well-structured test setup.** The setup method correctly configures MockMvc with necessary argument resolvers. --- `761-789`: **LGTM! Comprehensive version API testing.** The tests for v2 API endpoints are well-structured, covering both success and failure scenarios. --- `196-213`: <details> <summary>❓ Verification inconclusive</summary> **Verify multipart file handling.** The test verifies basic multipart file handling but doesn't test scenarios with actual image files. --- 🏁 Script executed: ```shell #!/bin/bash # Check if the controller handles image file uploads in other test classes rg -A 5 "MockMultipartFile.*image" --type javaLength of output: 197
Verify Multipart Handling for Image Uploads
The current test in
core/src/test/java/greencity/controller/EventControllerTest.java(lines 196–213) confirms that the controller accepts a JSON payload via multipart file handling. However, it does not cover scenarios involving image file uploads. We haven't located evidence of additional tests handling image files, so please verify manually that image uploads are either tested elsewhere or are not required. If the controller supports image files, consider adding a dedicated test case that simulates an actual image file upload.service-api/src/main/java/greencity/dto/logs/LogFileMetadataDto.java (1)
5-8: Well-structured record DTO!Good choice using a record for this immutable DTO. The field types are appropriate, and using
LocalDateTimefor timestamp handling is a modern approach.service-api/src/main/java/greencity/constant/ErrorMessage.java (1)
238-244: Well-defined error messages!The new error messages are clear, consistent with existing patterns, and use proper placeholders for dynamic content.
core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java (1)
568-607: Well-implemented exception handlers!The new exception handlers for
FileReadException,BadSecretKeyException, andFunctionalityNotAvailableException:
- Follow consistent patterns
- Use appropriate HTTP status codes
- Include proper error logging
README.md (2)
1-12: Restored README Content Appears Consistent
The content restored at the top—including the project logo, license, and copyright—is comprehensive and well‑formatted. It faithfully reinstates the original informational context.
125-128: Overall Documentation for Setup Checkstyle Is Adequate
The “Setup Checkstyle” section provides clear links for further instructions. No major changes needed here, though ensuring consistency in punctuation throughout the document will further enhance professionalism.🧰 Tools
🪛 LanguageTool
[uncategorized] ~127-~127: The verb “is” doesn’t seem to fit in this context, “are” is probably more formally correct.
Context: ...## 4.4. Connect with front-endThere is no special configurations required. Jus...
(AI_HYDRA_LEO_CPT_IS_ARE)
README.md
Outdated
| * `Name` : `GreenCityApplication`. | ||
| * `Use classpath of modules`:`core` |
There was a problem hiding this comment.
Correct Typographical Error
The heading “6. Enviroment variables:” contains a typo. Please change “Enviroment” to “Environment”.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~59-~59: Loose punctuation mark.
Context: ...n->+->Application`.
Name:GreenCityApplication.- `Use classpa...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~60-~60: Loose punctuation mark.
Context: ...lication`.
Use classpath of modules:coreJRE:21.
- `Enviromen...
(UNLIKELY_OPENING_PUNCTUATION)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
service/src/main/java/greencity/service/LogFileServiceImpl.java (2)
31-32: Consider making the logs directory path configurable.The hard-coded logs directory path might be inflexible for different deployment environments. Consider making it configurable through application properties.
- private static final String LOGS_DIRECTORY = - System.getProperty("user.dir") + File.separator + "logs" + File.separator; + @Value("${logging.file.path:${user.dir}/logs/}") + private String logsDirectory;
215-218: Improve log level matching precision.The current implementation might match unintended text (e.g., "INFO" within "INFORMATION"). Consider using regex or word boundaries for exact matches.
- private boolean matchesLogLevel(String fileContent, LogLevel logLevel) { - return logLevel == null - || fileContent.contains(logLevel.toString()); + private boolean matchesLogLevel(String fileContent, LogLevel logLevel) { + if (logLevel == null) { + return true; + } + String pattern = "\\b" + logLevel.toString() + "\\b"; + return fileContent.matches("(?s).*" + pattern + ".*"); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/test/java/greencity/controller/LogFileControllerTest.java(1 hunks)service-api/src/main/java/greencity/dto/logs/LogFileMetadataDto.java(1 hunks)service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java(1 hunks)service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java(1 hunks)service/src/main/java/greencity/service/LogFileServiceImpl.java(1 hunks)service/src/test/java/greencity/service/DotenvServiceImplTest.java(1 hunks)service/src/test/java/greencity/service/LogFileServiceImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- service-api/src/main/java/greencity/dto/logs/LogFileMetadataDto.java
- service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java
- service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java
- service/src/test/java/greencity/service/DotenvServiceImplTest.java
- service/src/test/java/greencity/service/LogFileServiceImplTest.java
- core/src/test/java/greencity/controller/LogFileControllerTest.java
🔇 Additional comments (5)
service/src/main/java/greencity/service/LogFileServiceImpl.java (5)
39-60: Well-structured implementation with proper security checks!The implementation includes appropriate security validation, error handling, and efficient stream operations for processing log files.
74-76: Consider using buffered reading for large files.Reading the entire file content at once with
Files.readString()could lead to memory issues with large log files. Consider using buffered reading or streaming for better memory management.
140-150: Avoid reading file content multiple times for filtering.Currently, each file's content is read within
filterFileDto. This can become costly if you have many large log files.
119-129: Check offset bounds in pagination logic.If
pageable.getOffset()exceeds the total elements,subList()can throw anIndexOutOfBoundsException. Consider adding safety checks.
70-78: Excellent error handling implementation!The code uses custom exceptions with descriptive messages and proper validation checks throughout the implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
core/src/main/java/greencity/controller/EventController.java (5)
84-117: Consider consolidating versioned endpoints.Having two similar endpoints (
/eventsand/events/createV2) with different response types (EventDto vs EventResponseDto) might lead to:
- API inconsistency
- Maintenance overhead
- Client confusion
Consider using API versioning through headers or path prefixes (e.g.,
/v2/events) instead of method suffixes.
167-169: Consider moving ID validation to a custom validator.The path variable validation logic could be encapsulated in a custom validator annotation to promote reusability and maintain separation of concerns.
Example implementation:
@Target({ElementType.METHOD, ElementType.TYPE}) @Retention(RetentionPolicy.RUNTIME) @Constraint(validatedBy = EventIdMatchValidator.class) public @interface ValidEventIdMatch { String message() default "Event ID in path must match event DTO ID"; Class<?>[] groups() default {}; Class<? extends Payload>[] payload() default {}; }
242-246: Enhance error message for status validation.The error message for status validation could be more descriptive by including which statuses require a user ID.
- throw new BadRequestException(ErrorMessage.STATUSES_REQUIRE_USER_ID); + throw new BadRequestException(String.format( + "User ID is required for statuses: %s", + STATUSES_THAT_REQUIRE_USER_ID.stream() + .map(EventStatus::name) + .collect(Collectors.joining(", ")) + ));
351-369: Maintain consistent parameter ordering in like/dislike methods.The parameter order is inconsistent between like and dislike methods:
like(eventId, user)dislike(user, eventId)This could lead to confusion during maintenance.
- public void dislike(@PathVariable Long eventId, @Parameter(hidden = true) @CurrentUser UserVO user) { - eventService.dislike(user, eventId); + public void dislike(@PathVariable Long eventId, @Parameter(hidden = true) @CurrentUser UserVO user) { + eventService.dislike(eventId, user);
561-587: Consider more RESTful endpoint naming.The current endpoints could be more RESTful:
/addToRequested->/requests/removeFromRequested->/requestsUse HTTP methods (POST/DELETE) to differentiate the actions instead of verb-based URLs.
Example:
- @PostMapping("/{eventId}/addToRequested") + @PostMapping("/{eventId}/requests")- @DeleteMapping("/{eventId}/removeFromRequested") + @DeleteMapping("/{eventId}/requests")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/greencity/config/SecurityConfig.java(1 hunks)core/src/main/java/greencity/controller/EventController.java(1 hunks)core/src/test/java/greencity/controller/EventControllerTest.java(1 hunks)service-api/src/main/java/greencity/dto/event/EventResponseDto.java(1 hunks)service-api/src/main/java/greencity/service/EventService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- service-api/src/main/java/greencity/dto/event/EventResponseDto.java
- service-api/src/main/java/greencity/service/EventService.java
- core/src/main/java/greencity/config/SecurityConfig.java
- core/src/test/java/greencity/controller/EventControllerTest.java
🔇 Additional comments (2)
core/src/main/java/greencity/controller/EventController.java (2)
59-67: Well-structured controller class with clear responsibilities!The class follows best practices with proper annotations, dependency injection, and clear separation of concerns.
487-490: Well-implemented event attendance endpoints!The methods follow REST conventions with appropriate response types and clear endpoint naming.
| import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; | ||
| import org.springframework.security.crypto.password.PasswordEncoder; | ||
|
|
||
| @Configuration |
There was a problem hiding this comment.
Why do You need this config?
PasswordEncoder is already present in the context and it's not necessary to create one more.
And if You will decide to remove this config then You should remove the @qualifier annotation from the DotenvServiceImpl.
There was a problem hiding this comment.
It's a bit tricky. I know about the existing password encoder, but it is located in core module, and I cannot include core module into service module because of looped dependency, so I created another one. we may talk more about it on a daily meeting
service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java
Outdated
Show resolved
Hide resolved
service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java
Outdated
Show resolved
Hide resolved
|
According to Sonar, code test coverage should be improved. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
service-api/src/main/java/greencity/dto/logs/LogFileRequestDto.java (1)
8-8:⚠️ Potential issueEnhance validation constraints for secretKey field.
The secretKey field should have stronger validation constraints to ensure security.
core/src/main/java/greencity/controller/LogFileController.java (4)
124-129:⚠️ Potential issueAdd safeguards for critical .env file deletion.
The .env file deletion endpoint lacks important safety measures.
32-36: 🛠️ Refactor suggestionConsider adding rate limiting for security.
The controller handles sensitive operations but lacks rate limiting protection.
@RestController @RequiredArgsConstructor @RequestMapping("/logs") @Profile({"dev", "test"}) +@RateLimiter(name = "logFileController") public class LogFileController {
103-105: 🛠️ Refactor suggestionMove secretKey to request header for better security.
Similar to the previous endpoint, move secretKey to headers.
public ResponseEntity<Resource> downloadLogFile( - @RequestBody String secretKey, + @RequestHeader("X-Secret-Key") String secretKey, @PathVariable String filename) {
81-83: 🛠️ Refactor suggestionMove secretKey to request header for better security.
Sensitive credentials should be passed in headers rather than request body.
public ResponseEntity<String> getLogFile( - @RequestBody String secretKey, + @RequestHeader("X-Secret-Key") String secretKey, @PathVariable String filename) {
🧹 Nitpick comments (1)
service-api/src/main/java/greencity/dto/logs/LogFileRequestDto.java (1)
10-27: Consider removing the defaultJson constant.The defaultJson constant with hardcoded example values could be replaced with OpenAPI schema annotations in the controller. This would:
- Keep example values closer to their usage
- Reduce maintenance overhead
- Follow the single responsibility principle
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/src/main/java/greencity/controller/LogFileController.java(1 hunks)core/src/test/java/greencity/controller/LogFileControllerTest.java(1 hunks)service-api/src/main/java/greencity/constant/AppConstant.java(1 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)service-api/src/main/java/greencity/dto/logs/LogFileRequestDto.java(1 hunks)service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java(1 hunks)service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java(1 hunks)service/pom.xml(2 hunks)service/src/main/java/greencity/config/DotenvConfig.java(1 hunks)service/src/main/java/greencity/service/DotenvServiceImpl.java(1 hunks)service/src/test/java/greencity/service/DotenvServiceImplTest.java(1 hunks)service/src/test/java/greencity/service/LogFileServiceImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- service-api/src/test/java/greencity/dto/logs/filter/ByteSizeRangeTest.java
- service-api/src/test/java/greencity/dto/logs/filter/DateRangeTest.java
- service/pom.xml
- service/src/test/java/greencity/service/LogFileServiceImplTest.java
- core/src/test/java/greencity/controller/LogFileControllerTest.java
- service/src/main/java/greencity/config/DotenvConfig.java
- service/src/main/java/greencity/service/DotenvServiceImpl.java
- service/src/test/java/greencity/service/DotenvServiceImplTest.java
- service-api/src/main/java/greencity/constant/ErrorMessage.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
service-api/src/main/java/greencity/constant/AppConstant.java (2)
31-31: Well-structured constant addition for environment configuration.The constant follows the established naming conventions and clearly indicates its purpose.
31-31:❓ Verification inconclusive
Consider security implications of exposing sensitive filenames.
While the constant is well-named, exposing the filename of sensitive environment variables in the codebase could potentially aid malicious actors. Consider:
- Using a more generic filename
- Making the filename configurable through system properties
- Adding documentation about secure file permissions
Let's check if this filename is exposed in any public endpoints or documentation:
🏁 Script executed:
#!/bin/bash # Search for potential exposure of the filename in controllers or documentation rg -i "secretKeys.env" --type java --glob "**/controller/**" rg -i "secretKeys.env" --glob "**/*.md"Length of output: 100
Security Review: Sensitive Filename Exposure
The automated search did not reveal any public exposure of
"secretKeys.env"in controllers or markdown documentation, which is a positive sign. However, exposing sensitive filenames in code can still provide attackers with clues. I suggest:
- Abstracting the Filename: Consider using a generic filename or making it configurable via system properties or environment variables.
- Secure File Handling: Add documentation about secure file permissions to mitigate risks in case the file is accessed.
- Manual Verification: Please double-check that this filename is not indirectly exposed elsewhere in the codebase or documentation.
| @PathVariable String filename) { | ||
| return ResponseEntity.ok() | ||
| .contentType(MediaType.APPLICATION_OCTET_STREAM) | ||
| .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + filename + "\"") |
There was a problem hiding this comment.
Sanitize filename in Content-Disposition header.
The filename should be sanitized to prevent HTTP header injection.
- .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + filename + "\"")
+ .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + sanitizeFilename(filename) + "\"")Add this helper method:
private String sanitizeFilename(String filename) {
return filename.replaceAll("[^a-zA-Z0-9._-]", "_");
}
Warded120
force-pushed
the
8079-display-log-files
branch
from
1cb4c4c to
f1aa89b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
service/src/main/java/greencity/service/DotenvServiceImpl.java (3)
22-24: Consider reordering constructor parameters for consistency.The parameter ordering in the constructor differs from the field declarations. Consider matching the order for better readability:
- public DotenvServiceImpl(PasswordEncoder passwordEncoder, - Dotenv dotenv) { + public DotenvServiceImpl(Dotenv dotenv, + PasswordEncoder passwordEncoder) {
31-39: Consider adding security audit logging.For security-sensitive operations like secret key validation, it would be beneficial to add audit logging to track validation attempts:
@Override public void validateSecretKey(String secretKey) { + log.info("Attempting to validate secret key"); reloadEnvFile(); String actualKey = dotenv.get("logs.secretKey"); if (actualKey == null || !passwordEncoder.matches(secretKey, actualKey)) { + log.warn("Invalid secret key attempt detected"); throw new BadSecretKeyException(ErrorMessage.BAD_SECRET_KEY); } + log.info("Secret key validated successfully"); }
71-79: Consider making reloadEnvFile private.Since this method is only used internally within the class, consider making it private to better encapsulate the implementation:
- void reloadEnvFile() { + private void reloadEnvFile() {service/src/test/java/greencity/service/LogFileServiceImplSanitizeFilenameTest.java (1)
20-53: Add a test case for null filenames.Current tests thoroughly check various forbidden characters and whitespace scenarios, which is commendable. A quick addition could cover a
nullfilename scenario to ensure no exceptions are thrown or to confirm expected behavior.@Test public void sanitizeFilenameShouldHandleNullFilenameTest() { + String filename = null; + String result = logFileService.sanitizeFilename(filename); + assertEquals("", result); // Or whichever behavior you deem appropriate }service/src/test/java/greencity/service/LogFileServiceImplTest.java (1)
52-90: Consider adding edge cases for listLogFiles tests.The current tests cover the happy path well. Consider adding these scenarios:
- Empty filter criteria
- Invalid page parameters
- Maximum page size handling
+ @Test + void listLogFilesListShouldHandleEmptyFilterTest() { + String secretKey = "secret"; + File[] mockFiles = {new File("test.log")}; + + LogFileServiceImpl spyService = spy(logFileService); + doReturn(mockFiles).when(spyService).listLogFilesFromFolder(); + + PageableDto<LogFileMetadataDto> result = spyService.listLogFiles( + PageRequest.of(0, Integer.MAX_VALUE), + null, + secretKey + ); + + assertNotNull(result); + assertEquals(1, result.getTotalElements()); + }core/src/main/java/greencity/controller/LogFileController.java (1)
80-86: Improve endpoint design for viewing log content.Consider these improvements:
- Use GET method for retrieving content
- Move secret key to request header
- Add Content-Type header for proper text rendering
-@PostMapping("/view/{filename}") +@GetMapping("/view/{filename}") public ResponseEntity<String> viewLogFileContent( - @RequestBody String secretKey, + @RequestHeader("X-Secret-Key") String secretKey, @PathVariable String filename) { return ResponseEntity.status(HttpStatus.OK) + .contentType(MediaType.TEXT_PLAIN) .body(logFileService.viewLogFileContent(logFileService.sanitizeFilename(filename), secretKey)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/greencity/controller/LogFileController.java(1 hunks)core/src/test/java/greencity/controller/LogFileControllerTest.java(1 hunks)service-api/src/main/java/greencity/service/LogFileService.java(1 hunks)service/src/main/java/greencity/service/DotenvServiceImpl.java(1 hunks)service/src/main/java/greencity/service/LogFileServiceImpl.java(1 hunks)service/src/test/java/greencity/service/LogFileServiceImplSanitizeFilenameTest.java(1 hunks)service/src/test/java/greencity/service/LogFileServiceImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/greencity/controller/LogFileControllerTest.java
🔇 Additional comments (15)
service/src/main/java/greencity/service/DotenvServiceImpl.java (2)
16-17: Great use of profile restrictions!Limiting this service to dev and test environments is a good security practice, as it prevents unauthorized access to environment variables in production.
48-48:❓ Verification inconclusive
Verify path traversal protection.
The file path construction using
System.getProperty("user.dir")could potentially be vulnerable to path traversal ifAppConstant.DOTENV_FILENAMEis not properly sanitized:
🏁 Script executed:
#!/bin/bash # Check if DOTENV_FILENAME constant is properly defined and sanitized ast-grep --pattern 'class AppConstant { $$$ DOTENV_FILENAME = $value; $$$ }'Length of output: 84
Security Concern: Validate DOTENV_FILENAME Sanitization
The file path construction using
System.getProperty("user.dir")concatenated withAppConstant.DOTENV_FILENAMEmight expose the application to path traversal risks ifDOTENV_FILENAMEisn’t strictly defined as a trusted, constant value. Although our initial AST grep query didn’t return output, this could be due to the search pattern rather than a guarantee of proper sanitization.Please manually verify that:
AppConstant.DOTENV_FILENAMEis declared as a constant with a static, safe value.- No external or dynamic inputs can modify its value, ensuring it isn’t inadvertently exposed to manipulation that could lead to path traversal.
Once you confirm these points, we can be more confident in the security of the file path construction.
service/src/main/java/greencity/service/LogFileServiceImpl.java (5)
41-62: Offer a null-safe check forlogFiles.The code correctly handles the scenario when no log files are found by throwing
NotFoundException. However, iflogFilesis null or empty, applying stream operations can be prone to NullPointerException if missed. You've already handled it with a null check, so this looks good. Keep an eye on concurrency if multiple threads might modify the logs directory concurrently.
76-77: Switch to buffered or streaming reads for large files.You already received a similar recommendation before. Reliance on
Files.readString(file.toPath())can strain memory if files are significantly large. Consider a buffered streaming approach to enhance performance.- return Files.readString(file.toPath()); + return Files.lines(file.toPath()) + .collect(Collectors.joining(System.lineSeparator()));
91-93: Prevent path traversal in file access.A past review suggested path normalization and checking for “..” sequences before constructing the file path to mitigate potential path traversal attacks. Here’s an example:
File file = new File(filename); String normalizedPath = file.toPath().normalize().toString(); if (normalizedPath.contains("..")) { throw new IllegalArgumentException("Path traversal detected"); } return new File(LOGS_DIRECTORY + filename);
119-139: Check pagination offset bounds to avoid IndexOutOfBoundsException.In the
applyPaginationmethod, ifstart >= totalElements,subList(start, end)might throw an IndexOutOfBoundsException. Here’s how you can handle it:int totalElements = dtos.size(); int start = (int) pageable.getOffset(); int end = Math.min((start + pageable.getPageSize()), dtos.size()); +if (start >= totalElements) { + return new PageableDto<>(List.of(), totalElements, pageable.getPageNumber(), totalPages); +} List<LogFileMetadataDto> paginatedList = dtos.subList(start, end);
155-182: Consolidate file reads to avoid multiple stream creations.Multiple invocations for
linesStreamSupplier.get()cause the file to be read repeatedly for each filter. Combining or short-circuiting filters in a single pass could improve performance for large log files.service-api/src/main/java/greencity/service/LogFileService.java (1)
9-57: Consider applying rate limiting on log file operations.Previously suggested as a security best practice to mitigate excessive file I/O from untrusted sources. For example:
+ @RateLimiter(name = "logFileService") public interface LogFileService { // ... }service/src/test/java/greencity/service/LogFileServiceImplTest.java (4)
25-35: Replace wildcard imports with explicit imports.For better code maintainability and to avoid potential naming conflicts, replace static wildcard imports with explicit imports.
-import static org.junit.jupiter.api.Assertions.*; -import static org.mockito.ArgumentMatchers.*; -import static org.mockito.Mockito.*; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when;
37-50: Well-structured test setup!Good use of Mockito extension, dependency injection, and test initialization. The
@BeforeEachmethod effectively handles the common setup for secret key validation.
101-154: Add security-focused test cases for file content access.Consider adding tests for:
- Path traversal attempts
- Non-log file access attempts
- Large file handling
+ @Test + void viewLogFileContentShouldPreventPathTraversalTest() { + String filename = "../../../etc/passwd"; + String secretKey = "secret"; + + assertThrows(InvalidFileNameException.class, + () -> logFileService.viewLogFileContent(filename, secretKey)); + }
156-173: Enhance download URL test coverage.The current test only covers the happy path. Consider adding tests for:
- Invalid file types (non-log files)
- Directory access attempts
- Resource cleanup
+ @Test + void generateDownloadLogFileUrlShouldThrowExceptionForInvalidFileType() { + String filename = "test.exe"; + String secretKey = "secret"; + File mockFile = mock(File.class); + + LogFileServiceImpl spyService = spy(logFileService); + doReturn(mockFile).when(spyService).getLogFile(filename); + + when(mockFile.exists()).thenReturn(true); + when(mockFile.isFile()).thenReturn(true); + + assertThrows(InvalidFileTypeException.class, + () -> spyService.generateDownloadLogFileUrl(filename, secretKey)); + }core/src/main/java/greencity/controller/LogFileController.java (3)
32-39: Consider enhancing security measures for sensitive operations.While the profile restriction is a good start, consider these security enhancements:
- Move secret key to request headers instead of request body for better security
- Add rate limiting to prevent brute force attempts
- Consider adding IP whitelisting for additional security
@RestController @RequiredArgsConstructor @RequestMapping("/logs") @Profile({"dev", "test"}) +@RateLimiter(name = "logFileController") public class LogFileController { private final LogFileService logFileService; private final DotenvService dotenvService;
52-62: Use GET instead of POST for retrieving log files list.The endpoint uses POST for a read operation, which isn't RESTful. Consider:
- Change to GET and move filter parameters to query parameters
- Use request params for filtering instead of request body
-@PostMapping +@GetMapping public ResponseEntity<PageableDto<LogFileMetadataDto>> listLogFiles( @Schema( description = "Filters for logs", name = "LogFileFilterDto", type = "object", - example = LogFileRequestDto.defaultJson) @RequestBody @Valid LogFileRequestDto requestDto, + example = LogFileRequestDto.defaultJson) @Valid LogFileFilterDto filterDto, + @RequestHeader("X-Secret-Key") String secretKey, @Parameter(hidden = true) Pageable page) {
126-131: Add safeguards for critical .env file deletion.The current implementation of .env file deletion lacks important safety measures:
- Add a confirmation mechanism to prevent accidental deletions
- Implement backup creation before deletion
- Add audit logging for this critical operation
@PostMapping("/delete-dotenv") public ResponseEntity<Object> deleteDotenvFile( - @RequestBody String secretKey) { + @RequestBody DeleteDotenvRequest request, + @RequestHeader("X-Secret-Key") String secretKey) { + if (!request.getConfirmation().equals("I understand the consequences")) { + throw new InvalidConfirmationException("Please confirm the deletion"); + } + // Create backup + dotenvService.backupDotenvFile(secretKey); + // Delete with audit logging dotenvService.deleteDotenvFile(secretKey); + log.warn("Dotenv file deleted by request"); return ResponseEntity.ok().build(); } +@Data +class DeleteDotenvRequest { + @NotBlank(message = "Confirmation is required") + private String confirmation; +}
…9-display-log-files # Conflicts: # service/src/main/java/greencity/service/DotenvServiceImpl.java # service/src/test/java/greencity/service/LogFileServiceImplTest.java
Warded120
force-pushed
the
8079-display-log-files
branch
from
f1aa89b to
a5f8aba
Compare
| int start = (int) pageable.getOffset(); | ||
| int end = Math.min((start + pageable.getPageSize()), dtos.size()); | ||
|
|
||
| List<LogFileMetadataDto> paginatedList = dtos.subList(start, end); |
There was a problem hiding this comment.
In this line, I have an error when trying to receive a list of log files.
The error occurs when I pass in the request page and size, which exceeds the number of logs(The user doesn't know how many log files are present).
For example: At this moment we have 2 log files but I pass the request with size=20 and page=1. In this case, we have an error "fromIndex(20) > toIndex(2)". It is not supposed to work in such way.
In my opinion, You should return an empty result instead of such error.
It is difficult to explain, so I attached the screenshot with the request and error.
There was a problem hiding this comment.
I'll investigate it, thanks
|
In case we delete the |
| * Checks if the log file's content contains the given query filter. | ||
| * | ||
| * @param fileContentStream The name of the log file. | ||
| * @param nameFilter The filter to match against the file content. Can be |
There was a problem hiding this comment.
There is no parameter with the name nameFilter in the method arguments.
There was a problem hiding this comment.
the method was updated, but I forgot to update javaDoc, I'll tend to it
| /** | ||
| * Checks if the log file contains the specified log level. | ||
| * | ||
| * @param fileDto The log file metadata object. |
There was a problem hiding this comment.
There is no parameter with the name fileDto in the method arguments.
Warded120
force-pushed
the
8079-display-log-files
branch
from
b38949b to
cceb682
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
service/src/test/java/greencity/service/LogFileServiceImplSanitizeFilenameTest.java (4)
20-21: Remove unused mock dependency.The
DotenvServicemock is not utilized in any of the test cases for filename sanitization. Consider removing it to maintain clean and focused test code.- @Mock - private DotenvService dotenvService;
23-30: Enhance test coverage with additional edge cases.The test cases are well-structured but could be more comprehensive. Consider adding the following scenarios:
- Filenames with leading/trailing spaces
- Very long filenames (to test length limits)
- Special Unicode characters
private static Stream<Arguments> filenameProvider() { return Stream.of( Arguments.of("valid_filename123.txt", "valid_filename123.txt"), Arguments.of("invalid@filename#.txt", "invalid_filename_.txt"), Arguments.of("", ""), Arguments.of("@#$.txt", "___.txt"), - Arguments.of("file name with spaces.txt", "file_name_with_spaces.txt")); + Arguments.of("file name with spaces.txt", "file_name_with_spaces.txt"), + Arguments.of(" leading_space.txt ", "leading_space.txt"), + Arguments.of("very_long_filename_that_exceeds_typical_limits.txt", "very_long_filename_that_exceeds_typical_limits.txt"), + Arguments.of("unicode_χαρακτήρες.txt", "unicode_________.txt"));
32-37: Improve test method clarity with better parameter naming and display name.The test method could be more descriptive for better readability and test reporting.
@ParameterizedTest @MethodSource("filenameProvider") + @DisplayName("sanitizeFilename should handle various filename formats correctly") - void sanitizeFilenameShouldReturnSanitizedFilenameTest(String input, String expected) { + void sanitizeFilenameShouldReturnSanitizedFilenameTest( + @Description("Input filename") String inputFilename, + @Description("Expected sanitized filename") String expectedFilename) { - String result = logFileService.sanitizeFilename(input); - assertEquals(expected, result); + String result = logFileService.sanitizeFilename(inputFilename); + assertEquals(expectedFilename, result, + String.format("Failed to sanitize filename. Input: %s", inputFilename)); }
14-15: Add class-level documentation.Consider adding Javadoc to describe the purpose of this test class and its relationship with
LogFileServiceImpl.@ExtendWith(MockitoExtension.class) +/** + * Unit tests for the filename sanitization functionality in {@link LogFileServiceImpl}. + * These tests verify that filenames are properly sanitized by removing or replacing + * invalid characters while preserving valid ones. + */ class LogFileServiceImplSanitizeFilenameTest {service/src/main/java/greencity/service/DotenvServiceImpl.java (2)
34-41: Consider security implications of validation logic.While the implementation is functional, consider these security enhancements:
- Use a constant-time comparison to prevent timing attacks
- Avoid different error messages that could reveal key existence
@Override public void validateSecretKey(String secretKey) { reloadEnvFile(); String actualKey = dotenv.get("logs.secretKey"); - if (actualKey == null || !passwordEncoder.matches(secretKey, actualKey)) { + if (!isValidKey(secretKey, actualKey)) { throw new BadSecretKeyException(ErrorMessage.BAD_SECRET_KEY); } } +private boolean isValidKey(String secretKey, String actualKey) { + return actualKey != null && passwordEncoder.matches(secretKey, actualKey); +}
73-81: Add logging for better debugging.Consider adding debug logging to help track env file reloading attempts and failures.
+import lombok.extern.slf4j.Slf4j; +@Slf4j public class DotenvServiceImpl implements DotenvService { void reloadEnvFile() { try { + log.debug("Attempting to reload .env file"); dotenv = Dotenv.configure() .filename(AppConstant.DOTENV_FILENAME) .load(); + log.debug(".env file reloaded successfully"); } catch (DotenvException ex) { + log.error("Failed to reload .env file", ex); throw new FunctionalityNotAvailableException(ErrorMessage.FUNCTIONALITY_NOT_AVAILABLE); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/greencity/controller/LogFileController.java(1 hunks)core/src/test/java/greencity/controller/LogFileControllerTest.java(1 hunks)service-api/src/main/java/greencity/service/LogFileService.java(1 hunks)service/src/main/java/greencity/config/DotenvConfig.java(1 hunks)service/src/main/java/greencity/service/DotenvServiceImpl.java(1 hunks)service/src/main/java/greencity/service/LogFileServiceImpl.java(1 hunks)service/src/test/java/greencity/service/LogFileServiceImplSanitizeFilenameTest.java(1 hunks)service/src/test/java/greencity/service/LogFileServiceImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- service/src/main/java/greencity/config/DotenvConfig.java
- service/src/test/java/greencity/service/LogFileServiceImplTest.java
- core/src/main/java/greencity/controller/LogFileController.java
- core/src/test/java/greencity/controller/LogFileControllerTest.java
- service/src/main/java/greencity/service/LogFileServiceImpl.java
🔇 Additional comments (3)
service/src/main/java/greencity/service/DotenvServiceImpl.java (2)
17-20: Well-structured service configuration!Good practice using @Profile to restrict this sensitive functionality to development and test environments.
21-28: Clean dependency injection pattern!The non-final dotenv field is correctly designed to support reloading functionality.
service-api/src/main/java/greencity/service/LogFileService.java (1)
9-56: Add rate limiting to prevent DoS attacks.As suggested in the previous review, add rate limiting to protect against potential DoS attacks through excessive file operations.
+@RateLimiter(name = "logFileService") public interface LogFileService { // ... existing methods ... }Consider additional security measures.
Given the sensitive nature of log files and the critical issue reported about .env file deletion:
- Consider using Spring Security's method-level security annotations
- Add proper validation for the secret key format
- Implement audit logging for file access
+@PreAuthorize("@logFileSecurityService.validateSecretKey(#secretKey)") public interface LogFileService { // ... existing methods ... }
…9-display-log-files # Conflicts: # service/src/main/java/greencity/service/LogFileServiceImpl.java
|
Issue link: 8079
changed:
Summary by CodeRabbit
New Features
Chores