Feature #8103 - Implemented functionality for retrieving database metadata and data with export options. #8174
Conversation
Moved apache.poi.version property to the main pom.xml; Added TableRowsDto and TablesMetadataDto; Added ExportSettingsRepo and ExportSettingsRepoImpl for receevin data about and from db; Created unit tests ExportSettingsRepoImplTest; Created ExportSettingsService interface and ExportSettingsServiceImpl for manipulation and preparing metadata and raw data from db; Added unit test ExportSettingsServiceImplTest; Created ExportToFileService interface and ExportToFileServiceImpl for creating file for export from data; Added unit tests ExportToFileServiceImplTest; Created DatabaseMetadataException, FileGenerationException, InvalidLimitException for export metadata and data from db functionality; Added necesary methods for new exceptiond in the CustomExceptionHandler; Added constant for SQL_ROW_LIMIT in the AppConstant; Added new error messages in the ErrorMessage; Added method for receiving TableMetadataDto and TableRowsDto in the ModelUtils classes; Added ExportSettingsController with endpoints for receiving all table names, select data by limmit, offset and table name, and endpoint for export in file data; Added unit tests ExportSettingsControllerTest;
Added String securityKey method argument in the ExportSettingsController; Refactored unit tests in ExportSettingsServiceImplTest and ExportSettingsControllerTest; Added endpoints from ExportSettingsController in the SecurityConfig;
WalkthroughThe changes remove an outdated Apache POI version property and introduce a comprehensive export settings feature across the project. New REST endpoints, service implementations, repository interfaces, DTO classes, and exception handling mechanisms are added to support metadata retrieval and data export to Excel. In addition, new constants and error message definitions help validate input parameters, while extensive tests cover the controller, service, and repository layers. The modifications span multiple modules, including core, dao, service-api, and service, ensuring robust export functionality and enhanced error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant EC as ExportSettingsController
participant ES as ExportSettingsServiceImpl
participant R as ExportSettingsRepoImpl
alt Retrieve Tables Metadata
C->>EC: GET /export/settings/tables (with secretKey)
EC->>ES: getTablesMetadata(secretKey)
ES->>R: getTablesMetadata()
R-->>ES: TablesMetadataDto
ES-->>EC: TablesMetadataDto
EC-->>C: ResponseEntity(TablesMetadataDto)
end
alt Download Excel File
C->>EC: GET /export/settings/download-table-data (with params)
EC->>ES: getExcelFileAsResource(tableName, limit, offset, secretKey)
ES->>R: selectPortionFromTable(tableName, limit, offset)
R-->>ES: TableRowsDto
ES-->>EC: InputStreamResource (Excel file)
EC-->>C: ResponseEntity(InputStreamResource)
end
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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: 5
🧹 Nitpick comments (15)
service-api/src/main/java/greencity/exception/exceptions/InvalidLimitException.java (1)
1-10: Good exception design with clear documentationThe exception class is well-designed using Lombok's
@StandardExceptionto reduce boilerplate code. The Javadoc clearly explains the purpose of the exception, which will help other developers understand when and why this exception is thrown.A minor suggestion would be to include a sample message that describes the actual limit violation to make troubleshooting easier for other developers.
@StandardException public class InvalidLimitException extends RuntimeException { + // Example usage: + // throw new InvalidLimitException("Limit value " + limit + " exceeds maximum allowed value of " + AppConstant.SQL_ROW_LIMIT); }service-api/src/main/java/greencity/exception/exceptions/FileGenerationException.java (1)
1-11: Appropriate exception for file generation failuresWell-structured exception that follows project conventions and uses Lombok to reduce boilerplate. The documentation clearly explains when this exception is thrown.
Consider adding a constructor that accepts and wraps the original IOException to preserve the stack trace and root cause information, which would be valuable for debugging.
@StandardException public class FileGenerationException extends RuntimeException { + /** + * Constructs a new FileGenerationException with the specified cause. + * + * @param cause the cause of the file generation failure + */ + public FileGenerationException(IOException cause) { + super("Failed to generate file: " + cause.getMessage(), cause); + } }service-api/src/main/java/greencity/dto/exportsettings/TablesMetadataDto.java (1)
1-12: Clean DTO design with appropriate Lombok annotationsThe TablesMetadataDto class is well-structured with appropriate Lombok annotations to reduce boilerplate. The Map structure is a suitable choice for representing table names and their associated columns.
Consider adding a brief class-level Javadoc comment to explain the purpose of this DTO and what the Map structure represents, which would improve the self-documentation of the code.
+/** + * Data Transfer Object that represents metadata about database tables. + * The 'tables' map contains table names as keys and lists of column names as values. + */ @Data @Builder public class TablesMetadataDto { private Map<String, List<String>> tables; }dao/src/main/java/greencity/repository/ExportSettingsRepo.java (1)
14-20: Missing parameter documentation in JavaDoc.While the method's purpose is well documented, the JavaDoc should include @param annotations for all parameters to improve code documentation.
Consider enhancing the JavaDoc by adding detailed parameter descriptions:
/** * Method for receiving data from table by name, limit and offset. * + * @param tableName the name of the table to select data from + * @param limit maximum number of rows to retrieve + * @param offset starting point for data retrieval (pagination) * @return dto {@link TableRowsDto} */service-api/src/main/java/greencity/service/ExportSettingsService.java (1)
13-13: Consider enhancing security by documenting secretKey validationThe
secretKeyparameter is used in all methods, which is good for security. However, there's no documentation about what happens if an invalid key is provided.Consider adding documentation about the validation process for the
secretKeyparameter and what exceptions might be thrown when validation fails. This would help implementers understand the expected behavior and improve the security aspect of the interface.Also applies to: 24-24, 36-36
core/src/test/java/greencity/ModelUtils.java (1)
668-677: Consider verifying table names and columns
Hardcoding the table name and columns is acceptable for demonstration, but it might be fragile in a dynamic environment. Verifying alignment with actual database schema or enabling parameterization could enhance maintainability.service/src/test/java/greencity/service/ExportToFileServiceImplTest.java (3)
35-35: Remove the extra semicolon
There's an extra semicolon afterexportToFileService. It's a minor style issue that can be removed for clarity.- private ExportToFileServiceImpl exportToFileService;; + private ExportToFileServiceImpl exportToFileService;
51-59: Consider additional range checks
The test covers an out-of-limit scenario but does not address negative limits or zero. Testing more edge cases can increase reliability.
61-76: Reflection approach is valid but can be simplified
Using reflection for testing private methods is sometimes necessary, yet a package-private scoping or a dedicated helper class might avoid reflection overhead. Otherwise, the exception handling flow is well tested.service/src/main/java/greencity/service/ExportToFileServiceImpl.java (1)
72-96: Avoid direct matching by header order
Currently, cells are matched to map entries by sequential header check. For large or reordered column sets, this can become error-prone. Consider mapping header keys to indexes to ensure consistent alignment.service/src/main/java/greencity/service/ExportSettingsServiceImpl.java (1)
44-54: Robust parameter validation with clear error messages.The validation logic is comprehensive, but consider consolidating the exception types. Currently, you're throwing
IllegalArgumentExceptionfor negative values butInvalidLimitExceptionfor exceeding limit values.Consider using a consistent exception type for all validation errors:
- if (limit < 0) { - throw new IllegalArgumentException(ErrorMessage.NEGATIVE_LIMIT); - } + if (limit < 0) { + throw new InvalidLimitException(ErrorMessage.NEGATIVE_LIMIT); + }And similarly for the offset validation:
- if (offset < 0) { - throw new IllegalArgumentException(ErrorMessage.NEGATIVE_OFFSET); - } + if (offset < 0) { + throw new InvalidLimitException(ErrorMessage.NEGATIVE_OFFSET); + }This would provide more consistent error handling throughout the application.
dao/src/test/java/greencity/repository/impl/ExportSettingsRepoImplTest.java (1)
72-87: Well-structured test for data selection functionality.The test effectively simulates the execution of a SQL query and verification of the result. Consider enhancing the assertion to validate the content of the returned DTO, not just that it's non-null.
Add assertions to validate the returned data content:
TableRowsDto result = settingsRepo.selectPortionFromTable(TABLE_NAME, LIMIT, OFFSET); assertNotNull(result); + assertNotNull(result.getRows()); + assertEquals(1, result.getRows().size()); + assertEquals(1, result.getRows().get(0).size()); + assertEquals("1", result.getRows().get(0).get("id"));dao/src/main/java/greencity/repository/impl/ExportSettingsRepoImpl.java (1)
30-59: Robust retrieval of table metadata with proper exception handling.
- Reading metadata with
DatabaseMetaDatais clearly structured. The code properly logs and wrapsSQLExceptioninDatabaseMetadataException.- Returning a custom DTO (
TablesMetadataDto) fosters a clean separation between the database logic and higher-level layers.Consider closing the
ResultSetfor tables explicitly or wrapping it in a try-with-resources block for clarity, although the current approach is implicitly covered by the upper-level try-with-resources statement.core/src/main/java/greencity/controller/ExportSettingsController.java (2)
70-98: Excel download implementation needs potential performance considerations.The implementation for downloading Excel files is well-structured, with appropriate headers for content disposition and type. However, there are some considerations:
- For large datasets, streaming the response would be more efficient than loading the entire file into memory
- The filename format could be improved to include timestamp or more descriptive information
- There's no indication of how large files are handled, which could impact server performance
Consider implementing streaming for large files and adding appropriate response timeout handling:
@GetMapping("/download-table-data") public ResponseEntity<InputStreamResource> downloadExcel( @RequestParam @Pattern(regexp = "^[A-Za-z_]+$") String tableName, - @RequestParam int limit, + @RequestParam @Min(value = 1) @Max(value = 10000) int limit, @RequestParam int offset, @RequestHeader String secretKey) { HttpHeaders headers = new HttpHeaders(); headers.add(HttpHeaders.CONTENT_DISPOSITION, - String.format("attachment; filename= %s(%d - %d).xlsx", tableName, offset, limit)); + String.format("attachment; filename= %s_%s(%d-%d).xlsx", + tableName, + LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMdd_HHmmss")), + offset, + limit)); headers.add(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_OCTET_STREAM_VALUE); return ResponseEntity.ok() .headers(headers) + .contentLength(/* Consider adding file size if available */) .body(new InputStreamResource( exportSettingsService.getExcelFileAsResource(tableName, limit, offset, secretKey))); }
28-99: Security considerations for the controller.The current implementation relies solely on a
secretKeyheader for authorization. While this provides basic protection, consider the following enhancements:
- Use Spring Security's more robust authentication mechanisms
- Add rate limiting to prevent abuse
- Consider implementing specific permissions for database access rather than a single key
- Add audit logging for sensitive data access operations
Consider enhancing the security model by integrating with Spring Security's authentication mechanisms and implementing more granular authorization checks based on user roles or permissions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/pom.xml(0 hunks)core/src/main/java/greencity/config/SecurityConfig.java(2 hunks)core/src/main/java/greencity/controller/ExportSettingsController.java(1 hunks)core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java(2 hunks)core/src/test/java/greencity/ModelUtils.java(3 hunks)core/src/test/java/greencity/controller/ExportSettingsControllerTest.java(1 hunks)dao/src/main/java/greencity/repository/ExportSettingsRepo.java(1 hunks)dao/src/main/java/greencity/repository/impl/ExportSettingsRepoImpl.java(1 hunks)dao/src/test/java/greencity/repository/impl/ExportSettingsRepoImplTest.java(1 hunks)pom.xml(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/exportsettings/TableRowsDto.java(1 hunks)service-api/src/main/java/greencity/dto/exportsettings/TablesMetadataDto.java(1 hunks)service-api/src/main/java/greencity/exception/exceptions/DatabaseMetadataException.java(1 hunks)service-api/src/main/java/greencity/exception/exceptions/FileGenerationException.java(1 hunks)service-api/src/main/java/greencity/exception/exceptions/InvalidLimitException.java(1 hunks)service-api/src/main/java/greencity/service/ExportSettingsService.java(1 hunks)service-api/src/main/java/greencity/service/ExportToFileService.java(1 hunks)service/pom.xml(1 hunks)service/src/main/java/greencity/service/ExportSettingsServiceImpl.java(1 hunks)service/src/main/java/greencity/service/ExportToFileServiceImpl.java(1 hunks)service/src/test/java/greencity/ModelUtils.java(3 hunks)service/src/test/java/greencity/service/ExportSettingsServiceImplTest.java(1 hunks)service/src/test/java/greencity/service/ExportToFileServiceImplTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- core/pom.xml
✅ Files skipped from review due to trivial changes (2)
- service-api/src/main/java/greencity/dto/exportsettings/TableRowsDto.java
- pom.xml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (38)
service-api/src/main/java/greencity/constant/AppConstant.java (1)
32-32: Well-defined constant for SQL row limitThe addition of
SQL_ROW_LIMITis a good practice for centralizing configuration values. Using a constant with a clear, descriptive name improves maintainability and makes it easier to adjust this limit across the application if needed.service-api/src/main/java/greencity/exception/exceptions/DatabaseMetadataException.java (1)
1-10: Well structured exception class with clear purpose.The DatabaseMetadataException is well designed as a runtime exception with appropriate documentation. The use of Lombok's @StandardException annotation eliminates boilerplate code and makes the class concise.
service-api/src/main/java/greencity/service/ExportToFileService.java (1)
5-17: Clean interface design with well-documented method.The ExportToFileService interface has a clear purpose and provides a well-documented method for Excel export functionality. The method signature with tableName, limit, and offset parameters allows for flexible data retrieval.
core/src/main/java/greencity/config/SecurityConfig.java (2)
88-88: Good addition of export settings constant.The SETTINGS_EXPORT constant follows the same naming convention and style as other endpoints in the class.
296-297: Properly secured export settings endpoint.The addition of the SETTINGS_EXPORT endpoint to the authorized requests matcher ensures that only authenticated users with appropriate roles (USER, ADMIN, MODERATOR, UBS_EMPLOYEE) can access the export functionality, maintaining security standards.
dao/src/main/java/greencity/repository/ExportSettingsRepo.java (1)
6-13: Well-designed repository interface with clear method.The getTablesMetadata method is properly documented and returns an appropriate DTO type.
service/pom.xml (1)
137-146:✅ Verification successful
Good addition of Apache POI dependencies for Excel file export functionality
The Apache POI dependencies (both core and OOXML libraries) are correctly added to support Excel file generation capabilities as described in the PR objectives. This will enable the export functionality that's being implemented.
Since I noticed the
apache.poi.versionproperty was removed from core/pom.xml according to the summary, let's verify it's properly defined in the parent pom.xml:
🏁 Script executed:
#!/bin/bash # Check if apache.poi.version is defined in the parent pom.xml grep -n "apache.poi.version" pom.xmlLength of output: 94
Verified: Apache POI Dependencies and Version Definition Confirmed
The dependencies added in
service/pom.xmlfor both Apache POI core and OOXML look correctly implemented for supporting Excel file export functionality. Additionally, the propertyapache.poi.versionis properly defined in the parentpom.xml(line 48, value5.4.0), ensuring consistency across modules.
- Dependencies at
service/pom.xml(lines 137-146) are set up correctly:<dependency> <groupId>org.apache.poi</groupId> <artifactId>poi</artifactId> <version>${apache.poi.version}</version> </dependency> <dependency> <groupId>org.apache.poi</groupId> <artifactId>poi-ooxml</artifactId> <version>${apache.poi.version}</version> </dependency>- Verification confirms the
apache.poi.versionproperty is defined in the parentpom.xml.Great work on implementing these changes! Everything appears to be in order and aligned with the PR objectives.
service/src/test/java/greencity/ModelUtils.java (3)
3534-3542: Well-implemented test utility method for table metadataThe
getTablesMetadataDto()method properly creates a test instance ofTablesMetadataDtowith sample data for the "users" table, which is useful for unit testing the new export functionality.The use of HashMap for storing table metadata is appropriate here since order is not critical for this test data.
3544-3557: Good implementation of test data for table rowsThe
getTableRowsDto()method creates a well-structured instance ofTableRowsDtowith realistic sample data, which will be valuable for testing the row retrieval and export functionality.I particularly like the use of LinkedHashMap and LinkedList to maintain the order of columns and rows, which is important for testing data exports where order may matter.
114-115: Appropriate imports for the new functionalityThe imports added for the new DTO classes and collection types are necessary and properly organized.
Also applies to: 235-238
core/src/test/java/greencity/ModelUtils.java (3)
46-47: Imports look consistent
These imports forTableRowsDtoandTablesMetadataDtoalign well with the newly introduced functionality.
86-87: No issues with additional imports
The addition ofLinkedListandLinkedHashMapsimplifies object creation for the new DTOs.
678-691: Validate date format consistency
Date/time strings are manually set here. Ensure they match database expectations or user locale formats if further processing is required.service/src/test/java/greencity/service/ExportToFileServiceImplTest.java (1)
40-49: Unit test validations look good
These assertions ensure correct retrieval of data and call verification. Nice job covering the happy path.service/src/main/java/greencity/service/ExportToFileServiceImpl.java (1)
98-109: File generation logic is well-organized
The usage of aByteArrayOutputStreamand structured exception handling demonstrates good coding practices and clarity.service-api/src/main/java/greencity/constant/ErrorMessage.java (1)
244-248: Well-defined error messages for export functionality.The newly added error message constants are clear, concise, and follow the established pattern in the class. They properly support the validation requirements for limit, offset, and other export-related operations.
service/src/main/java/greencity/service/ExportSettingsServiceImpl.java (3)
21-25: Comprehensive validation before data access.Good implementation of the
getTablesMetadatamethod with proper secret key validation before accessing sensitive database information.
27-34: Clear transactional boundary with proper validation.Excellent use of
@Transactional(readOnly = true)for the data retrieval operation. The method correctly validates both security and input parameters before accessing the database.
36-42: Secure implementation of file export functionality.The method correctly validates security credentials and input parameters before performing the export operation.
dao/src/test/java/greencity/repository/impl/ExportSettingsRepoImplTest.java (3)
50-63: Comprehensive test for successful table metadata retrieval.The test effectively mocks all necessary database components and verifies the expected behavior of the
getTablesMetadatamethod.
65-70: Good error handling test for database connection failures.The test properly verifies that a
SQLExceptionduring connection acquisition is converted to aDatabaseMetadataException.
89-95: Clear test for SQL exception during data selection.The test effectively verifies that a
SQLExceptionduring query execution is properly converted to aDatabaseMetadataException.core/src/test/java/greencity/controller/ExportSettingsControllerTest.java (4)
50-55: Good setup for controller testing with appropriate exception handling.The MockMvc setup includes the custom exception handler, which is essential for testing error scenarios properly.
57-68: Comprehensive test for tables metadata endpoint.The test effectively verifies that the controller properly returns the expected JSON response with the correct status code.
70-84: Well-structured test for data selection with valid parameters.Good validation of both the status code and the response content.
162-177: Thorough test for Excel file download endpoint.Good validation of status code, content type, and Content-Disposition header. The test effectively verifies that the controller returns the expected file.
core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java (4)
12-17: Imports introduced for new exceptions look consistent.The import statements for
DatabaseMetadataException,FileGenerationException, andInvalidLimitExceptionare properly placed and conform to naming conventions. Good job on maintaining consistency here.
611-628: Exception handler aligns with existing patterns.The
handleDatabaseMetadataExceptionmethod correctly logs the error, creates anExceptionResponse, and returns an appropriate HTTP 500 status. This handler approach appears consistent with other exception handling methods in the class.
630-645: Accurate status code selection for invalid limits.Using
BAD_REQUEST(400) forInvalidLimitExceptionis appropriate as it signals a client error. The implemented logic suitably leverages the existing structure to ensure a clean and unified exception handling strategy.
647-662: Consistent handling for file generation errors.The
handleFileGenerationExceptionmethod properly sets a500 - Internal Server Errorstatus, emphasizing the server-side nature of file generation failures. Including a clear log message helps with troubleshooting.dao/src/main/java/greencity/repository/impl/ExportSettingsRepoImpl.java (2)
1-27: Implementation and dependency injection setup look appropriate.
- The
ExportSettingsRepoImplclass is annotated with@Repositoryand usesDataSourcevia constructor injection. This is a robust approach for ensuring maintainability and testability.- Good usage of
@RequiredArgsConstructorfrom Lombok to reduce boilerplate code.
61-85: Validate table name to avoid potential SQL injection.
- The
String.formatusage for constructing the SQL query could be exploited if thetableNameis user-provided. Since table names cannot be parametrized in prepared statements, ensure the table name is validated against a known list or pattern before executing.- Otherwise, the logic for retrieving rows is efficient and well-structured, returning a
TableRowsDtothat remains consistent with the rest of the application design.Would you like to integrate a validation step or a whitelist approach for
tableNameto reduce injection risk?service/src/test/java/greencity/service/ExportSettingsServiceImplTest.java (4)
1-41: Test setup is coherent and well-structured.
- The use of JUnit 5’s
@ExtendWith(MockitoExtension.class)is aptly combined with@InjectMocksand@Mockto provide a clean, isolated testing environment.- The constants at lines 24–28 keep test values organized, aiding readability.
42-63: Validating secret key logic ingetTablesMetadataTest.
- Properly mocking
dotenvService.validateSecretKey(SECRET_KEY)ensures controlled test behavior.- Verifying that the repository is invoked exactly once gives confidence in the correctness of the service flow.
65-119: Thorough tests forselectFromTableedge cases.
- The negative values for limit or offset, and the exceeding limit threshold, are well-handled by distinct test methods. This is excellent coverage for boundary checks.
- Ensuring the repository call is never triggered for invalid parameters (lines 85, 96, 107, 118) confirms that errors are caught early in the service layer.
121-142: Valid test coverage for Excel file export.
- Valid parameters test ensures the service calls the export method and returns a non-null
InputStream.- Invalid secret key test properly throws
BadSecretKeyException, ensuring secure access control.core/src/main/java/greencity/controller/ExportSettingsController.java (2)
1-28: Well-structured controller setup with appropriate annotations and imports.The controller is properly organized with the necessary imports and annotations. The
@RequestMapping("/export/settings")provides a clear base path for all endpoints, and@RequiredArgsConstructoris effectively used for dependency injection.
31-46: Endpoint for table metadata is well-documented and implemented.Good use of OpenAPI annotations to document the endpoint's purpose and response types. The method correctly delegates to the service layer for retrieving metadata.
Consider adding validation or logging for the secretKey parameter, as it's currently only passed through to the service without any controller-level handling.
| public interface ExportSettingsService { | ||
| /** | ||
| * Method for receiving all DB tables short metadata. | ||
| * | ||
| * @return {@link TablesMetadataDto} instance. | ||
| */ | ||
| TablesMetadataDto getTablesMetadata(String secretKey); | ||
|
|
||
| /** | ||
| * Method for receiving rows from table by table name, limit and offset. | ||
| * | ||
| * @param tableName {@link String} | ||
| * @param limit is limit of rows to return. | ||
| * @param offset is offset for rows to return. | ||
| * | ||
| * @return {@link TablesMetadataDto} object with metadata. | ||
| */ | ||
| TableRowsDto selectFromTable(String tableName, int limit, int offset, String secretKey); | ||
|
|
||
| /** | ||
| * Method for receiving an excel file as InputStream with rows from DB by table | ||
| * name, limit, offset. | ||
| * | ||
| * @param tableName {@link String} | ||
| * @param limit is limit of rows to return. | ||
| * @param offset is offset for rows to return. | ||
| * | ||
| * @return {@link InputStream} InputStream with file. | ||
| */ | ||
| InputStream getExcelFileAsResource(String tableName, int limit, int offset, String secretKey); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Well-structured interface for database export functionality
The interface is clear, focused, and properly documented. Each method serves a specific purpose related to database metadata retrieval and data export, which aligns with the PR objectives.
There's a small inconsistency in the JavaDoc comment for the selectFromTable method:
The JavaDoc at line 22 mentions returning a TablesMetadataDto object, but the actual method signature returns a TableRowsDto. Consider updating the documentation to match the actual return type.
🏁 Script executed:
#!/bin/bash
# Check the return type documentation for selectFromTable method
grep -A 3 "@return" service-api/src/main/java/greencity/service/ExportSettingsService.java | grep TableRowsDtoLength of output: 203
Subject: Minor JavaDoc Inconsistency in selectFromTable Method
The interface is well-structured and clearly documented overall. However, there is an inconsistency in the JavaDoc for the selectFromTable method:
- The Javadoc annotation currently mentions returning a
TablesMetadataDto, but the method signature returns aTableRowsDto. - Please update the JavaDoc to reference
TableRowsDtoin the@returnsection to ensure consistency between the documentation and the implementation.
| public InputStream exportTableDataToExcel(String tableName, int limit, int offset) { | ||
| if (limit > AppConstant.SQL_ROW_LIMIT) { | ||
| log.warn("Table data exceeds limit: {}", limit); | ||
| throw new InvalidLimitException(String.format(ErrorMessage.EXCEED_LIMIT, limit)); | ||
| } | ||
|
|
||
| Workbook workbook = new XSSFWorkbook(); | ||
| Sheet sheet = workbook.createSheet(tableName); | ||
| TableRowsDto data = exportSettingsRepo.selectPortionFromTable(tableName, limit, offset); | ||
| createHeaderRow(workbook, sheet, data); | ||
| populateTableCells(workbook, sheet, data); | ||
| return convertWorkbookToInputStream(workbook); | ||
| } |
There was a problem hiding this comment.
Verify parameter ordering with the repository
The call selectPortionFromTable(tableName, limit, offset) appears reversed compared to test usage (selectPortionFromTable(TABLE_NAME, OFFSET, LIMIT)). Ensure the parameter order is consistent across interface, implementation, and tests.
| private void createHeaderRow(Workbook workbook, Sheet sheet, TableRowsDto data) { | ||
| CellStyle headerStyle = workbook.createCellStyle(); | ||
| headerStyle.setFillForegroundColor(IndexedColors.LIGHT_BLUE.getIndex()); | ||
| headerStyle.setFillPattern(FillPatternType.SOLID_FOREGROUND); | ||
|
|
||
| Row header = sheet.createRow(0); | ||
| XSSFFont font = ((XSSFWorkbook) workbook).createFont(); | ||
| font.setFontName("Arial"); | ||
| font.setFontHeightInPoints((short) 16); | ||
| font.setBold(true); | ||
|
|
||
| Set<String> raw = data.getTableData().getFirst().keySet(); | ||
| int cellIndex = 0; | ||
| for (String key : raw) { | ||
| Cell headerCell = header.createCell(cellIndex++); | ||
| headerCell.setCellValue(key); | ||
| headerCell.setCellStyle(headerStyle); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle empty data case
Reliance on data.getTableData().getFirst() can cause errors if the collection is empty. Adding a guard check or fallback ensures no runtime exceptions are thrown for empty results.
| @Test | ||
| public void getSelectedWithNonExistentTableNameTest() throws Exception { | ||
| doThrow(new DatabaseMetadataException(ErrorMessage.SQL_METADATA_EXCEPTION_MESSAGE + NOT_EXISTS_TABLE_NAME)) | ||
| .when(exportSettingsService) | ||
| .selectFromTable(NOT_EXISTS_TABLE_NAME, LIMIT, OFFSET, SECRET_KEY); | ||
|
|
||
| mockMvc.perform(get(SETTINGS_CONTROLLER_LINK + "/select") | ||
| .param("tableName", NOT_EXISTS_TABLE_NAME) | ||
| .param("limit", String.valueOf(LIMIT)) | ||
| .param("offset", String.valueOf(OFFSET)) | ||
| .header("secretKey", SECRET_KEY) | ||
| .accept(MediaType.APPLICATION_JSON)) | ||
| .andExpect(status().isInternalServerError()) // Expect 404 Not Found for non-existent table | ||
| .andReturn(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider appropriate status code for non-existent resources.
The test expects a 500 (Internal Server Error) status code for a non-existent table, but this is typically a client-side error rather than a server error.
Consider changing the expected status code and ensuring your CustomExceptionHandler properly maps DatabaseMetadataException to an appropriate status code like 404 (Not Found) when the error is due to a non-existent resource:
mockMvc.perform(get(SETTINGS_CONTROLLER_LINK + "/select")
.param("tableName", NOT_EXISTS_TABLE_NAME)
.param("limit", String.valueOf(LIMIT))
.param("offset", String.valueOf(OFFSET))
.header("secretKey", SECRET_KEY)
.accept(MediaType.APPLICATION_JSON))
- .andExpect(status().isInternalServerError()) // Expect 404 Not Found for non-existent table
+ .andExpect(status().isNotFound()) // 404 is more appropriate for non-existent resources
.andReturn();You'll also need to update the corresponding exception handler in CustomExceptionHandler class.
📝 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 | |
| public void getSelectedWithNonExistentTableNameTest() throws Exception { | |
| doThrow(new DatabaseMetadataException(ErrorMessage.SQL_METADATA_EXCEPTION_MESSAGE + NOT_EXISTS_TABLE_NAME)) | |
| .when(exportSettingsService) | |
| .selectFromTable(NOT_EXISTS_TABLE_NAME, LIMIT, OFFSET, SECRET_KEY); | |
| mockMvc.perform(get(SETTINGS_CONTROLLER_LINK + "/select") | |
| .param("tableName", NOT_EXISTS_TABLE_NAME) | |
| .param("limit", String.valueOf(LIMIT)) | |
| .param("offset", String.valueOf(OFFSET)) | |
| .header("secretKey", SECRET_KEY) | |
| .accept(MediaType.APPLICATION_JSON)) | |
| .andExpect(status().isInternalServerError()) // Expect 404 Not Found for non-existent table | |
| .andReturn(); | |
| } | |
| @Test | |
| public void getSelectedWithNonExistentTableNameTest() throws Exception { | |
| doThrow(new DatabaseMetadataException(ErrorMessage.SQL_METADATA_EXCEPTION_MESSAGE + NOT_EXISTS_TABLE_NAME)) | |
| .when(exportSettingsService) | |
| .selectFromTable(NOT_EXISTS_TABLE_NAME, LIMIT, OFFSET, SECRET_KEY); | |
| mockMvc.perform(get(SETTINGS_CONTROLLER_LINK + "/select") | |
| .param("tableName", NOT_EXISTS_TABLE_NAME) | |
| .param("limit", String.valueOf(LIMIT)) | |
| .param("offset", String.valueOf(OFFSET)) | |
| .header("secretKey", SECRET_KEY) | |
| .accept(MediaType.APPLICATION_JSON)) | |
| .andExpect(status().isNotFound()) // 404 is more appropriate for non-existent resources | |
| .andReturn(); | |
| } |
| /** | ||
| * Method for receiving rows from DB by table name, limit and offset. | ||
| * | ||
| * @return dto {@link TableRowsDto} | ||
| */ | ||
| @Operation(summary = "Get table rows by params.") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = HttpStatuses.OK, | ||
| content = @Content(schema = @Schema(implementation = TableRowsDto.class))), | ||
| @ApiResponse(responseCode = "403", description = HttpStatuses.FORBIDDEN, | ||
| content = @Content(examples = @ExampleObject(HttpStatuses.FORBIDDEN))), | ||
| @ApiResponse(responseCode = "400", description = HttpStatuses.BAD_REQUEST, | ||
| content = @Content(examples = @ExampleObject(HttpStatuses.BAD_REQUEST))) | ||
| }) | ||
| @GetMapping("/select") | ||
| public ResponseEntity<TableRowsDto> getSelected(@RequestParam @Pattern(regexp = "^[A-Za-z_]+$") String tableName, | ||
| @RequestParam int limit, | ||
| @RequestParam int offset, | ||
| @RequestHeader String secretKey) { | ||
| return ResponseEntity.ok(exportSettingsService.selectFromTable(tableName, limit, offset, secretKey)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Input validation for table selection needs enhancement.
While the regex validation on tableName is a good security practice, the limit and offset parameters should also be validated to prevent potential issues:
- Negative values could cause unexpected behavior
- Excessively large limit values might impact performance or memory usage
Consider adding validation annotations such as:
- public ResponseEntity<TableRowsDto> getSelected(@RequestParam @Pattern(regexp = "^[A-Za-z_]+$") String tableName,
- @RequestParam int limit,
- @RequestParam int offset,
+ public ResponseEntity<TableRowsDto> getSelected(
+ @RequestParam @Pattern(regexp = "^[A-Za-z_]+$") String tableName,
+ @RequestParam @Min(value = 1, message = "Limit must be positive")
+ @Max(value = 1000, message = "Limit cannot exceed 1000") int limit,
+ @RequestParam @Min(value = 0, message = "Offset cannot be negative") int offset,
@RequestHeader String 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.
| /** | |
| * Method for receiving rows from DB by table name, limit and offset. | |
| * | |
| * @return dto {@link TableRowsDto} | |
| */ | |
| @Operation(summary = "Get table rows by params.") | |
| @ApiResponses(value = { | |
| @ApiResponse(responseCode = "200", description = HttpStatuses.OK, | |
| content = @Content(schema = @Schema(implementation = TableRowsDto.class))), | |
| @ApiResponse(responseCode = "403", description = HttpStatuses.FORBIDDEN, | |
| content = @Content(examples = @ExampleObject(HttpStatuses.FORBIDDEN))), | |
| @ApiResponse(responseCode = "400", description = HttpStatuses.BAD_REQUEST, | |
| content = @Content(examples = @ExampleObject(HttpStatuses.BAD_REQUEST))) | |
| }) | |
| @GetMapping("/select") | |
| public ResponseEntity<TableRowsDto> getSelected(@RequestParam @Pattern(regexp = "^[A-Za-z_]+$") String tableName, | |
| @RequestParam int limit, | |
| @RequestParam int offset, | |
| @RequestHeader String secretKey) { | |
| return ResponseEntity.ok(exportSettingsService.selectFromTable(tableName, limit, offset, secretKey)); | |
| } | |
| /** | |
| * Method for receiving rows from DB by table name, limit and offset. | |
| * | |
| * @return dto {@link TableRowsDto} | |
| */ | |
| @Operation(summary = "Get table rows by params.") | |
| @ApiResponses(value = { | |
| @ApiResponse(responseCode = "200", description = HttpStatuses.OK, | |
| content = @Content(schema = @Schema(implementation = TableRowsDto.class))), | |
| @ApiResponse(responseCode = "403", description = HttpStatuses.FORBIDDEN, | |
| content = @Content(examples = @ExampleObject(HttpStatuses.FORBIDDEN))), | |
| @ApiResponse(responseCode = "400", description = HttpStatuses.BAD_REQUEST, | |
| content = @Content(examples = @ExampleObject(HttpStatuses.BAD_REQUEST))) | |
| }) | |
| @GetMapping("/select") | |
| public ResponseEntity<TableRowsDto> getSelected( | |
| @RequestParam @Pattern(regexp = "^[A-Za-z_]+$") String tableName, | |
| @RequestParam @Min(value = 1, message = "Limit must be positive") | |
| @Max(value = 1000, message = "Limit cannot exceed 1000") int limit, | |
| @RequestParam @Min(value = 0, message = "Offset cannot be negative") int offset, | |
| @RequestHeader String secretKey) { | |
| return ResponseEntity.ok(exportSettingsService.selectFromTable(tableName, limit, offset, secretKey)); | |
| } |
…gsControllerTest, ExportSettingsRepoImplTest, ExportSettingsServiceImplTest, ExportToFileServiceImplTest; Made constants in the listed above classes static;
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
core/src/test/java/greencity/controller/ExportSettingsControllerTest.java (6)
86-96: Test for invalid table name handling is concise.Test correctly verifies that invalid table names result in a 400 Bad Request status. Consider adding a check for the error message in the response to ensure the proper error is communicated to the client.
.andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.message").value(containsString("Invalid table name"))) .andReturn();
114-128: Good test for negative offset validation.The test correctly verifies that negative offset values result in a 400 Bad Request status. Consider adding an assertion for the error message to ensure proper feedback is provided to the client.
.andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.message").value(ErrorMessage.NEGATIVE_OFFSET)) .andReturn();
130-144: Good test for negative limit validation.Similar to the negative offset test, consider adding an assertion for the error message to provide additional validation.
.andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.message").value(ErrorMessage.NEGATIVE_LIMIT)) .andReturn();
146-160: Well-structured test for limit validation.Test properly verifies that exceeding the maximum limit results in a 400 Bad Request status. Consider adding an assertion for the error message to ensure proper feedback.
.andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.message").value(String.format(ErrorMessage.EXCEED_LIMIT, AppConstant.SQL_ROW_LIMIT))) .andReturn();
162-177: Excel download test is comprehensive.Good validation of content type, status, and header for file download. The test properly verifies that the Excel file is provided with the correct name format.
One consideration: the test's mock response uses a very small byte array (5 bytes), which isn't a realistic Excel file. Consider adding a test that verifies the actual content structure, perhaps by using a real (small) Excel file or at least verifying that the service method is called with the correct parameters.
34-178: Consider adding test coverage for additional scenarios.The test class has good coverage for the basic scenarios but could be enhanced with tests for:
- Missing or empty secretKey header
- Tests for other potential error conditions, such as service exceptions
- Edge cases, like maximum allowed values for limit and offset
- Authorization and authentication failures
These additional tests would ensure more comprehensive coverage of the controller's behavior in all scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/test/java/greencity/controller/ExportSettingsControllerTest.java(1 hunks)dao/src/test/java/greencity/repository/impl/ExportSettingsRepoImplTest.java(1 hunks)service/src/test/java/greencity/service/ExportSettingsServiceImplTest.java(1 hunks)service/src/test/java/greencity/service/ExportToFileServiceImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- service/src/test/java/greencity/service/ExportToFileServiceImplTest.java
- dao/src/test/java/greencity/repository/impl/ExportSettingsRepoImplTest.java
- service/src/test/java/greencity/service/ExportSettingsServiceImplTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
core/src/test/java/greencity/controller/ExportSettingsControllerTest.java (5)
1-49: Class setup and dependencies look well structured.The imports, constant definitions, and class structure follow good practices. Using static constants for test values enhances readability and maintainability. The mock setup is appropriate for testing a Spring MVC controller.
One suggestion would be to consider adding some Javadoc to explain the purpose of the class, which would help with understanding what's being tested at a high level.
50-55: Good setup for MockMvc with proper error handling.The MockMvc setup correctly includes the controller under test and the CustomExceptionHandler, which will ensure that exceptions are handled as they would be in the real application.
57-68: Valid test case for retrieving tables metadata.Good test structure with clear expectations and verification. The test properly mocks the service response and validates both the status code and response content.
70-84: Valid test case for table row selection.Test is well structured with clear mocking and assertions. The expected JSON verification ensures the controller returns the correct data from the service.
98-112: Improve status code for non-existent resources.The test expects a 500 (Internal Server Error) status code for a non-existent table, but this is typically a client-side error rather than a server error.
Consider changing the expected status code and ensuring your
CustomExceptionHandlerproperly mapsDatabaseMetadataExceptionto an appropriate status code like 404 (Not Found) when the error is due to a non-existent resource.mockMvc.perform(get(SETTINGS_CONTROLLER_LINK + "/select") .param("tableName", NOT_EXISTS_TABLE_NAME) .param("limit", String.valueOf(LIMIT)) .param("offset", String.valueOf(OFFSET)) .header("secretKey", SECRET_KEY) .accept(MediaType.APPLICATION_JSON)) - .andExpect(status().isInternalServerError()) // Expect 404 Not Found for non-existent table + .andExpect(status().isNotFound()) // 404 is more appropriate for non-existent resources .andReturn();
GreenCity PR
Issue Link 📋
#8103
Added
TableRowsDtoandTablesMetadataDto;ExportSettingsRepoandExportSettingsRepoImplfor receiving data about and from db;ExportSettingsRepoImplTest;ExportSettingsServiceinterface andExportSettingsServiceImplfor manipulation and preparing metadata and raw data from db;ExportToFileServiceinterface andExportToFileServiceImplfor creating files for export from data;DatabaseMetadataException,FileGenerationException,InvalidLimitExceptionfor export metadata and data from db functionality;ExportSettingsServiceImplTest;ExportToFileServiceImplTest;ExportSettingsControllerwith endpoints for receiving all table names, selecting data by limit, offset, and table name, and endpoint for exporting data in the file;Changed
pom.xml;CustomExceptionHandler;AppConstant;ErrorMessage;TableMetadataDtoandTableRowsDtoin theModelUtilsclasses;Summary Of Changes 🔥
Created functionality for receiving info about DB such as table names with column names, selecting rows by some criteria(table name, offset, limit), and downloading Excel files by the same criterias. All endpoints are secured by Spring security and additional security with the secret key based on .env functionality.
Summary by CodeRabbit
New Features
Chores
Bug Fixes
Tests