Feature/sql command #293
Feature/sql command #293
Conversation
…cted database Modified: moved helper functions that are utilised more often to Utils classes Added: Docs for new command
📝 WalkthroughWalkthroughThe changes introduce a new CLI command "sql" that enables direct SQL query execution against configured data sources. This is supported by new utility classes for query execution and CSV output handling, plus refactoring of existing query logic into reusable components. Documentation and integration tests are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Cli
participant QueryService as QueryService
participant QueryUtils as QueryUtils
participant FileUtils as FileUtils
participant DB as Database
User->>CLI: sql -s source -q "SELECT ..." -l 200
activate CLI
CLI->>CLI: Resolve source connection
CLI->>CLI: Prepare output directory
CLI->>QueryService: executeQuery(query, source, limit, ...)
deactivate CLI
activate QueryService
QueryService->>QueryUtils: executeQueryAndGetRecords(query, source, limit)
activate QueryUtils
QueryUtils->>DB: Execute SQL query
DB-->>QueryUtils: Result set
QueryUtils->>QueryUtils: Map results to List<Map>
QueryUtils-->>QueryService: Query results
deactivate QueryUtils
QueryService->>FileUtils: createCSVFile(data, filename, ...)
activate FileUtils
FileUtils->>FileUtils: Write CSV from records
FileUtils-->>QueryService: CSV file path
deactivate FileUtils
QueryService->>FileUtils: generateTablePreview(csvFile, limit)
activate FileUtils
FileUtils->>FileUtils: Format ASCII table
FileUtils-->>QueryService: Table preview string
deactivate FileUtils
QueryService->>QueryService: Build response message
QueryService-->>CLI: Response with CSV path & preview
activate CLI
CLI->>User: Log result message
deactivate CLI
deactivate QueryService
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
queryhelper/src/main/java/queryhelper/service/AIService.java (1)
36-41: Dead code:errorResponseis created but never returned.The
errorResponseobject is created when the query is not a SELECT statement, but the method continues execution regardless. This appears to be incomplete logic - either the response should be returned, or this block should be removed.🔎 Proposed fix - return early for non-SELECT statements
boolean selectStatement = isSelectStatement(query); if (!selectStatement) { GenericResponse errorResponse = new GenericResponse(); errorResponse.setMessage("Generated query, execute on your own will: " + query); errorResponse.setStatusCode(200); + return errorResponse; }queryhelper/src/main/java/queryhelper/utils/FileUtils.java (1)
50-69: CSV generation doesn't properly escape values.The
convertToCSVmethod doesn't handle values containing commas, quotes, or newlines per RFC 4180. If a column value contains a comma (e.g., "Smith, John"), the CSV will be malformed andgenerateTablePreviewwill parse it incorrectly.🔎 Proposed fix - add proper CSV escaping
public static void convertToCSV(String fileName, List<Map<String, Object>> list) { try (FileWriter csvWriter = new FileWriter(fileName)) { if (!list.isEmpty()) { Set<String> headers = list.get(0).keySet(); StringBuilder csvContent = new StringBuilder(); - csvContent.append(String.join(",", headers)).append("\n"); + csvContent.append(headers.stream().map(FileUtils::escapeCsvValue).collect(Collectors.joining(","))).append("\n"); for (Map<String, Object> map : list) { for (String header : headers) { - csvContent.append(map.getOrDefault(header, "")).append(","); + Object value = map.getOrDefault(header, ""); + csvContent.append(escapeCsvValue(String.valueOf(value))).append(","); } csvContent.setLength(csvContent.length() - 1); csvContent.append("\n"); } csvWriter.write(csvContent.toString()); } } catch (IOException e) { e.printStackTrace(); } } + private static String escapeCsvValue(String value) { + if (value == null) return ""; + if (value.contains(",") || value.contains("\"") || value.contains("\n")) { + return "\"" + value.replace("\"", "\"\"") + "\""; + } + return value; + }
🤖 Fix all issues with AI Agents
In @cli/src/test/java/integration/SqlserverIntegrationTest.java:
- Around line 38-41: The DATABASE constant is unused; either remove the DATABASE
field or set the SQL Server container to use it by adding
.withDatabaseName(DATABASE) to the MSSQLServerContainer initialization (the code
that constructs the container in SqlserverIntegrationTest), ensuring you
reference the existing IMAGE, USERNAME, PASSWORD constants and keep the
MSSQLServerContainer configuration consistent with other tests.
In @docs/markdowns/sql.md:
- Around line 2-3: Update the documentation in sql.md to fix the grammar (change
"you" to "your" in the two occurrences flagged) and align the CLI option name
with the implementation by changing the synopsis flag from `-q, --sql` to `-q,
--query` (or vice versa if you prefer changing the code in Cli.java to `--sql`);
ensure the example and any references in sql.md match the unique option name
used by the CLI implementation (look for the `-q`/`--sql`/`--query` mention and
the `sql` command description) so docs and the Cli.java option names are
consistent.
- Line 10: Fix the typo in the SQL flag description by changing "you" to "your"
so the line reads: q --sql "SQL Query Code" | specify the query you want to run
in your connected DB; update the string in the docs/markdowns/sql.md entry for
the SQL flag accordingly.
In @queryhelper/src/main/java/queryhelper/utils/FileUtils.java:
- Around line 71-109: In FileUtils.generateTablePreview the current
String.split(",") mis-parses quoted CSV fields; replace the naive split with a
real CSV parser (e.g., OpenCSV's CSVReader/CSVParser or Apache Commons CSV
CSVFormat) to correctly handle quoted fields and embedded commas, update imports
and dependencies accordingly, read each record via the parser into String[] rows
(keeping the rest of the table width/formatting logic unchanged), and ensure
IOException handling remains in place for parser errors.
- Around line 28-47: The filename sanitization in FileUtils.createCSVFile is too
weak (only replaces whitespace via csvFileName.replaceAll("\\s+", "_")), so
update that logic to strip or replace all filesystem-invalid characters (e.g., /
\ : * ? " < > | ; and control characters) and collapse runs into a safe subset
(letters, numbers, hyphen, underscore, dot), truncate excessively long names,
and fallback to a timestamp-only name if the sanitized result is empty; apply
this sanitization before building fileName and keep the rest of createCSVFile
(including the timestamp suffix) unchanged.
In @queryhelper/src/main/java/queryhelper/utils/QueryUtils.java:
- Around line 16-29: In executeQueryAndGetRecords, jdbcConnection and statement
are never closed and setMaxRows(showRowLimit) will NPE when showRowLimit is
null; fix by using try-with-resources to open the java.sql.Connection
jdbcConnection and Statement statement so both are automatically closed (wrap
driver.connect(...) and jdbcConnection.createStatement() in a try-with-resources
block), only call statement.setMaxRows(showRowLimit) when showRowLimit != null
(and optionally > 0), and then call QueryHelper.select(statement, query) inside
that try block; keep the existing SQLException handling but ensure resources are
closed even on exceptions.
🧹 Nitpick comments (3)
cli/src/test/java/integration/SqlserverIntegrationTest.java (1)
38-38: Consider pinning to a specific version for reproducibility.The
:2022-latesttag will pull the latest patch version of SQL Server 2022, which could vary across test runs and environments. For maximum test reproducibility, consider pinning to a specific version likemcr.microsoft.com/mssql/server:2022-CU10-ubuntu-22.04.queryhelper/src/main/java/queryhelper/service/QueryService.java (1)
18-44: Consider adding input validation and error handling.The method lacks validation for null or empty
queryparameter and doesn't handle the case whereexecuteQueryAndGetRecordsmight throw aRuntimeException(which wrapsSQLException). The caller may benefit from more specific error messages.cli/src/main/java/com/adaptivescale/rosetta/cli/Cli.java (1)
753-778: Significant code duplication with thequerycommand.The path resolution logic (lines 765-773) is nearly identical to lines 733-741 in the
querycommand. Consider extracting this into a helper method to reduce duplication and improve maintainability.🔎 Proposed helper method extraction
// Add this helper method to the Cli class private PathInfo resolveOutputPaths(String sourceName, Path output) throws IOException { Path sourceWorkspace = Paths.get("./", sourceName); Path dataDirectory = output != null ? output : sourceWorkspace.resolve(DEFAULT_OUTPUT_DIRECTORY); Path outputFile = dataDirectory.getFileName().toString().contains(".") ? dataDirectory.getFileName() : null; dataDirectory = dataDirectory.getFileName().toString().contains(".") ? (dataDirectory.getParent() != null ? dataDirectory.getParent() : Paths.get(".")) : dataDirectory; if (!dataDirectory.toFile().exists()) { Files.createDirectories(dataDirectory); } return new PathInfo(dataDirectory, outputFile); } // Simple record/class to hold the result private record PathInfo(Path dataDirectory, Path outputFile) {}Then both
sqlandquerycommands can use:PathInfo paths = resolveOutputPaths(sourceName, output);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.mdcli/src/main/java/com/adaptivescale/rosetta/cli/Cli.javacli/src/test/java/integration/SqlserverIntegrationTest.javadocs/markdowns/sql.mdqueryhelper/src/main/java/queryhelper/service/AIService.javaqueryhelper/src/main/java/queryhelper/service/QueryService.javaqueryhelper/src/main/java/queryhelper/utils/FileUtils.javaqueryhelper/src/main/java/queryhelper/utils/QueryUtils.java
🧰 Additional context used
🧬 Code graph analysis (5)
queryhelper/src/main/java/queryhelper/utils/QueryUtils.java (4)
common/src/main/java/com/adaptivescale/rosetta/common/DriverManagerDriverProvider.java (1)
DriverManagerDriverProvider(9-14)common/src/main/java/com/adaptivescale/rosetta/common/JDBCUtils.java (1)
JDBCUtils(7-25)common/src/main/java/com/adaptivescale/rosetta/common/models/input/Connection.java (1)
Connection(9-91)source/src/main/java/com/adataptivescale/rosetta/source/common/QueryHelper.java (1)
QueryHelper(17-116)
queryhelper/src/main/java/queryhelper/service/QueryService.java (4)
queryhelper/src/main/java/queryhelper/pojo/GenericResponse.java (1)
GenericResponse(3-38)queryhelper/src/main/java/queryhelper/pojo/QueryDataResponse.java (1)
QueryDataResponse(6-50)queryhelper/src/main/java/queryhelper/utils/FileUtils.java (1)
FileUtils(15-123)queryhelper/src/main/java/queryhelper/utils/QueryUtils.java (1)
QueryUtils(15-30)
queryhelper/src/main/java/queryhelper/utils/FileUtils.java (3)
queryhelper/src/main/java/queryhelper/pojo/GenericResponse.java (1)
GenericResponse(3-38)queryhelper/src/main/java/queryhelper/pojo/QueryDataResponse.java (1)
QueryDataResponse(6-50)queryhelper/src/main/java/queryhelper/utils/ErrorUtils.java (1)
ErrorUtils(9-56)
queryhelper/src/main/java/queryhelper/service/AIService.java (2)
queryhelper/src/main/java/queryhelper/utils/FileUtils.java (1)
FileUtils(15-123)queryhelper/src/main/java/queryhelper/utils/QueryUtils.java (1)
QueryUtils(15-30)
cli/src/main/java/com/adaptivescale/rosetta/cli/Cli.java (1)
queryhelper/src/main/java/queryhelper/service/QueryService.java (1)
QueryService(16-45)
🪛 LanguageTool
docs/markdowns/sql.md
[grammar] ~3-~3: Ensure spelling is correct
Context: ...NECTION_NAME] [-q, --sql "Write SQL for you Schema"] [--output "Output DIRECTORY or...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~10-~10: Ensure spelling is correct
Context: ... | specify the query you want to run in you connected DB. -l --limit Response Row l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/markdowns/sql.md
2-2: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
22-22: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (6)
cli/src/test/java/integration/SqlserverIntegrationTest.java (1)
172-174: LGTM! Good use of the IMAGE constant.The container instantiation correctly uses the parameterized constructor with the IMAGE constant, which centralizes the configuration and makes it easier to update the image version.
README.md (1)
143-144: LGTM!The documentation entry for the new SQL command is consistent with the existing command list format.
queryhelper/src/main/java/queryhelper/service/AIService.java (1)
18-21: LGTM on the refactoring to use shared utilities.The static imports properly delegate to the new utility classes.
cli/src/main/java/com/adaptivescale/rosetta/cli/Cli.java (1)
45-45: LGTM on the import addition.queryhelper/src/main/java/queryhelper/utils/FileUtils.java (2)
3-9: LGTM on the import additions.
111-121: LGTM on the row separator builder.The implementation correctly generates ASCII table borders based on column widths.
| private static String IMAGE = "mcr.microsoft.com/mssql/server:2022-latest"; | ||
| private static String USERNAME = "SA"; | ||
| private static String PASSWORD = "123abcD!"; | ||
| private static String DATABASE = "Northwind"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cli/src/test/java/integration/SqlserverIntegrationTest.javaRepository: rosettadb/rosetta
Length of output: 26183
🏁 Script executed:
rg -n "DATABASE" cli/src/test/java/integration/ --type=java -C 2Repository: rosettadb/rosetta
Length of output: 6599
🏁 Script executed:
rg -n "withDatabaseName" cli/src/test/java/integration/ --type=javaRepository: rosettadb/rosetta
Length of output: 437
Remove unused DATABASE constant or add .withDatabaseName(DATABASE) to container initialization.
The DATABASE constant defined on line 41 is never used. Other integration tests (MySQL, DB2, Oracle) properly configure their database names with .withDatabaseName(DATABASE), but SqlserverIntegrationTest is missing this configuration. Either add .withDatabaseName(DATABASE) to the MSSQLServerContainer initialization on line 172, or remove the unused constant.
🤖 Prompt for AI Agents
In @cli/src/test/java/integration/SqlserverIntegrationTest.java around lines 38
- 41, The DATABASE constant is unused; either remove the DATABASE field or set
the SQL Server container to use it by adding .withDatabaseName(DATABASE) to the
MSSQLServerContainer initialization (the code that constructs the container in
SqlserverIntegrationTest), ensuring you reference the existing IMAGE, USERNAME,
PASSWORD constants and keep the MSSQLServerContainer configuration consistent
with other tests.
| The sql commands allows the user to write SQL queries directly to the connected Database of his choice. | ||
| rosetta [-c, --config CONFIG_FILE] sql [-h, --help] [-s, --source CONNECTION_NAME] [-q, --sql "Write SQL for you Schema"] [--output "Output DIRECTORY or FILE"] |
There was a problem hiding this comment.
Fix grammar: "you" should be "your".
Based on static analysis hints, there are grammar issues on lines 3 and 10 where "you" should be "your".
Also, the command synopsis shows -q, --sql but the CLI implementation in Cli.java uses -q, --query. Please align the documentation with the actual CLI option names.
🔎 Proposed fix
-The sql commands allows the user to write SQL queries directly to the connected Database of his choice.
- rosetta [-c, --config CONFIG_FILE] sql [-h, --help] [-s, --source CONNECTION_NAME] [-q, --sql "Write SQL for you Schema"] [--output "Output DIRECTORY or FILE"]
+The sql command allows the user to write SQL queries directly to the connected database of their choice.
+ rosetta [-c, --config CONFIG_FILE] sql [-h, --help] [-s, --source CONNECTION_NAME] [-q, --query "Your SQL Query"] [-l, --limit ROW_LIMIT] [--no-limit] [--output "Output DIRECTORY or FILE"]Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Ensure spelling is correct
Context: ...NECTION_NAME] [-q, --sql "Write SQL for you Schema"] [--output "Output DIRECTORY or...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
2-2: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
In @docs/markdowns/sql.md around lines 2 - 3, Update the documentation in sql.md
to fix the grammar (change "you" to "your" in the two occurrences flagged) and
align the CLI option name with the implementation by changing the synopsis flag
from `-q, --sql` to `-q, --query` (or vice versa if you prefer changing the code
in Cli.java to `--sql`); ensure the example and any references in sql.md match
the unique option name used by the CLI implementation (look for the
`-q`/`--sql`/`--query` mention and the `sql` command description) so docs and
the Cli.java option names are consistent.
| -h, --help | Show the help message and exit. | ||
| -c, --config CONFIG_FILE | YAML config file. If none is supplied it will use main.conf in the current directory if it exists. | ||
| -s, --source CONNECTION_NAME | The source connection is used to specify which models and connection to use. | ||
| -q --sql "SQL Query Code" | specify the query you want to run in you connected DB. |
There was a problem hiding this comment.
Fix grammar: "you" should be "your".
🔎 Proposed fix
--q --sql "SQL Query Code" | specify the query you want to run in you connected DB.
+-q, --query "SQL Query Code" | Specify the query you want to run in your connected DB.🧰 Tools
🪛 LanguageTool
[grammar] ~10-~10: Ensure spelling is correct
Context: ... | specify the query you want to run in you connected DB. -l --limit Response Row l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In @docs/markdowns/sql.md at line 10, Fix the typo in the SQL flag description
by changing "you" to "your" so the line reads: q --sql "SQL Query Code" |
specify the query you want to run in your connected DB; update the string in the
docs/markdowns/sql.md entry for the SQL flag accordingly.
| response.setData(data); | ||
| response.setStatusCode(200); | ||
|
|
||
| String csvFile = createCSVFile(data, query, dataDirectory, outputFileName); |
There was a problem hiding this comment.
SQL query as filename may contain invalid characters.
Using the raw SQL query as the CSV filename base can result in invalid filenames. SQL queries often contain characters like ;, *, /, \, ", ', etc., which are problematic for file systems. The createCSVFile method only replaces whitespace with underscores.
🔎 Proposed fix - sanitize the filename
- String csvFile = createCSVFile(data, query, dataDirectory, outputFileName);
+ // Sanitize query for use as filename - keep only alphanumeric chars and limit length
+ String sanitizedQuery = query.replaceAll("[^a-zA-Z0-9]", "_").replaceAll("_+", "_");
+ if (sanitizedQuery.length() > 50) {
+ sanitizedQuery = sanitizedQuery.substring(0, 50);
+ }
+ String csvFile = createCSVFile(data, sanitizedQuery, dataDirectory, outputFileName);📝 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.
| String csvFile = createCSVFile(data, query, dataDirectory, outputFileName); | |
| // Sanitize query for use as filename - keep only alphanumeric chars and limit length | |
| String sanitizedQuery = query.replaceAll("[^a-zA-Z0-9]", "_").replaceAll("_+", "_"); | |
| if (sanitizedQuery.length() > 50) { | |
| sanitizedQuery = sanitizedQuery.substring(0, 50); | |
| } | |
| String csvFile = createCSVFile(data, sanitizedQuery, dataDirectory, outputFileName); |
| public static String createCSVFile(QueryDataResponse queryDataResponse, String csvFileName, Path dataDirectory, Path outputFileName) { | ||
| try { | ||
| if (outputFileName == null) { | ||
| String timestamp = new SimpleDateFormat("yyyyMMdd_HHmmss").format(new Date()); | ||
| String fileName = csvFileName.replaceAll("\\s+", "_") + "_" + timestamp + ".csv"; | ||
| Path csvFilePath = dataDirectory.resolve(fileName); | ||
| FileUtils.convertToCSV(csvFilePath.toString(), queryDataResponse.getRecords()); | ||
|
|
||
| return csvFilePath.toString(); | ||
| } | ||
|
|
||
| Path csvFilePath = dataDirectory.resolve(outputFileName.toString()); | ||
| FileUtils.convertToCSV(csvFilePath.toString(), queryDataResponse.getRecords()); | ||
| return csvFilePath.toString(); | ||
|
|
||
| } catch (Exception e) { | ||
| GenericResponse genericResponse = ErrorUtils.csvFileError(e); | ||
| throw new RuntimeException(genericResponse.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Filename sanitization is insufficient.
The replaceAll("\\s+", "_") only handles whitespace. If csvFileName contains other filesystem-invalid characters (;, *, /, \, ", <, >, |, etc.), file creation may fail or produce unexpected results.
🔎 Proposed fix
if (outputFileName == null) {
String timestamp = new SimpleDateFormat("yyyyMMdd_HHmmss").format(new Date());
- String fileName = csvFileName.replaceAll("\\s+", "_") + "_" + timestamp + ".csv";
+ String sanitized = csvFileName.replaceAll("[^a-zA-Z0-9._-]", "_").replaceAll("_+", "_");
+ if (sanitized.length() > 100) {
+ sanitized = sanitized.substring(0, 100);
+ }
+ String fileName = sanitized + "_" + timestamp + ".csv";
Path csvFilePath = dataDirectory.resolve(fileName);🤖 Prompt for AI Agents
In @queryhelper/src/main/java/queryhelper/utils/FileUtils.java around lines 28 -
47, The filename sanitization in FileUtils.createCSVFile is too weak (only
replaces whitespace via csvFileName.replaceAll("\\s+", "_")), so update that
logic to strip or replace all filesystem-invalid characters (e.g., / \ : * ? " <
> | ; and control characters) and collapse runs into a safe subset (letters,
numbers, hyphen, underscore, dot), truncate excessively long names, and fallback
to a timestamp-only name if the sanitized result is empty; apply this
sanitization before building fileName and keep the rest of createCSVFile
(including the timestamp suffix) unchanged.
| public static String generateTablePreview(String csvFile, int rowLimit) { | ||
| List<String[]> rows = new ArrayList<>(); | ||
| try (BufferedReader reader = new BufferedReader(new FileReader(csvFile))) { | ||
| String line; | ||
| int rowCount = 0; | ||
| while ((line = reader.readLine()) != null && rowCount < rowLimit) { | ||
| String[] columns = line.split(","); | ||
| rows.add(columns); | ||
| rowCount++; | ||
| } | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Error reading CSV file", e); | ||
| } | ||
|
|
||
| if (rows.isEmpty()) { | ||
| return "No data available to display."; | ||
| } | ||
| int maxColumns = rows.stream().mapToInt(row -> row.length).max().orElse(0); | ||
| int[] columnWidths = new int[maxColumns]; | ||
| for (String[] row : rows) { | ||
| for (int i = 0; i < row.length; i++) { | ||
| columnWidths[i] = Math.max(columnWidths[i], row[i].length()); | ||
| } | ||
| } | ||
| StringBuilder table = new StringBuilder(); | ||
| String rowSeparator = buildRowSeparator(columnWidths); | ||
|
|
||
| table.append(rowSeparator); | ||
| for (String[] row : rows) { | ||
| table.append("|"); | ||
| for (int i = 0; i < maxColumns; i++) { | ||
| String cell = (i < row.length) ? row[i] : ""; | ||
| table.append(" ").append(String.format("%-" + columnWidths[i] + "s", cell)).append(" |"); | ||
| } | ||
| table.append("\n").append(rowSeparator); | ||
| } | ||
|
|
||
| return table.toString(); | ||
| } |
There was a problem hiding this comment.
CSV parsing with split(",") doesn't handle quoted values.
If CSV values contain commas (which is valid CSV when quoted), the simple split(",") will incorrectly split the value. This will cause misaligned columns in the table preview.
Consider using a proper CSV parser or at minimum handling quoted values.
🤖 Prompt for AI Agents
In @queryhelper/src/main/java/queryhelper/utils/FileUtils.java around lines 71 -
109, In FileUtils.generateTablePreview the current String.split(",") mis-parses
quoted CSV fields; replace the naive split with a real CSV parser (e.g.,
OpenCSV's CSVReader/CSVParser or Apache Commons CSV CSVFormat) to correctly
handle quoted fields and embedded commas, update imports and dependencies
accordingly, read each record via the parser into String[] rows (keeping the
rest of the table width/formatting logic unchanged), and ensure IOException
handling remains in place for parser errors.
| public static List<Map<String, Object>> executeQueryAndGetRecords(String query, Connection source, Integer showRowLimit) { | ||
| try { | ||
| DriverManagerDriverProvider driverManagerDriverProvider = new DriverManagerDriverProvider(); | ||
| Driver driver = driverManagerDriverProvider.getDriver(source); | ||
| Properties properties = JDBCUtils.setJDBCAuth(source); | ||
| java.sql.Connection jdbcConnection = driver.connect(source.getUrl(), properties); | ||
| Statement statement = jdbcConnection.createStatement(); | ||
| statement.setMaxRows(showRowLimit); | ||
| List<Map<String, Object>> select = QueryHelper.select(statement, query); | ||
| return select; | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: JDBC resources are never closed, causing resource leaks.
The jdbcConnection and statement are never closed, which will exhaust database connection pools and cause memory leaks. Additionally, if showRowLimit is null, setMaxRows(showRowLimit) will throw a NullPointerException due to auto-unboxing.
🔎 Proposed fix using try-with-resources
public class QueryUtils {
public static List<Map<String, Object>> executeQueryAndGetRecords(String query, Connection source, Integer showRowLimit) {
+ DriverManagerDriverProvider driverManagerDriverProvider = new DriverManagerDriverProvider();
try {
- DriverManagerDriverProvider driverManagerDriverProvider = new DriverManagerDriverProvider();
Driver driver = driverManagerDriverProvider.getDriver(source);
Properties properties = JDBCUtils.setJDBCAuth(source);
- java.sql.Connection jdbcConnection = driver.connect(source.getUrl(), properties);
- Statement statement = jdbcConnection.createStatement();
- statement.setMaxRows(showRowLimit);
- List<Map<String, Object>> select = QueryHelper.select(statement, query);
- return select;
+ try (java.sql.Connection jdbcConnection = driver.connect(source.getUrl(), properties);
+ Statement statement = jdbcConnection.createStatement()) {
+ if (showRowLimit != null && showRowLimit > 0) {
+ statement.setMaxRows(showRowLimit);
+ }
+ return QueryHelper.select(statement, query);
+ }
} catch (SQLException e) {
throw new RuntimeException(e);
}
}
}📝 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 static List<Map<String, Object>> executeQueryAndGetRecords(String query, Connection source, Integer showRowLimit) { | |
| try { | |
| DriverManagerDriverProvider driverManagerDriverProvider = new DriverManagerDriverProvider(); | |
| Driver driver = driverManagerDriverProvider.getDriver(source); | |
| Properties properties = JDBCUtils.setJDBCAuth(source); | |
| java.sql.Connection jdbcConnection = driver.connect(source.getUrl(), properties); | |
| Statement statement = jdbcConnection.createStatement(); | |
| statement.setMaxRows(showRowLimit); | |
| List<Map<String, Object>> select = QueryHelper.select(statement, query); | |
| return select; | |
| } catch (SQLException e) { | |
| throw new RuntimeException(e); | |
| } | |
| } | |
| public static List<Map<String, Object>> executeQueryAndGetRecords(String query, Connection source, Integer showRowLimit) { | |
| DriverManagerDriverProvider driverManagerDriverProvider = new DriverManagerDriverProvider(); | |
| try { | |
| Driver driver = driverManagerDriverProvider.getDriver(source); | |
| Properties properties = JDBCUtils.setJDBCAuth(source); | |
| try (java.sql.Connection jdbcConnection = driver.connect(source.getUrl(), properties); | |
| Statement statement = jdbcConnection.createStatement()) { | |
| if (showRowLimit != null && showRowLimit > 0) { | |
| statement.setMaxRows(showRowLimit); | |
| } | |
| return QueryHelper.select(statement, query); | |
| } | |
| } catch (SQLException e) { | |
| throw new RuntimeException(e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @queryhelper/src/main/java/queryhelper/utils/QueryUtils.java around lines 16
- 29, In executeQueryAndGetRecords, jdbcConnection and statement are never
closed and setMaxRows(showRowLimit) will NPE when showRowLimit is null; fix by
using try-with-resources to open the java.sql.Connection jdbcConnection and
Statement statement so both are automatically closed (wrap driver.connect(...)
and jdbcConnection.createStatement() in a try-with-resources block), only call
statement.setMaxRows(showRowLimit) when showRowLimit != null (and optionally >
0), and then call QueryHelper.select(statement, query) inside that try block;
keep the existing SQLException handling but ensure resources are closed even on
exceptions.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.