feat: added support to mssql for execute query #6200
Conversation
WalkthroughA new method named Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant MSSQLClient
participant NetworkPolicy
participant Database
participant Utils
Caller->>MSSQLClient: ExecuteQuery(host, port, username, password, dbName, query)
MSSQLClient->>NetworkPolicy: Validate host and port
MSSQLClient->>Database: Open connection
Database-->>MSSQLClient: Connection object
MSSQLClient->>Database: Execute query
Database-->>MSSQLClient: Query result rows
MSSQLClient->>Utils: Unmarshal rows into SQLResult
Utils-->>MSSQLClient: SQLResult or error (with possible partial data)
MSSQLClient->>Database: Close connection
MSSQLClient-->>Caller: Return SQLResult or error
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/js/libs/mssql/mssql.go (1)
147-162: Consider extracting common connection setup code.This validation and connection string setup logic is duplicated from the existing
connectfunction. Consider refactoring to extract this common code into a reusable helper function.+func buildConnectionString(host string, port int, username, password, dbName string) (string, error) { + if host == "" || port <= 0 { + return "", fmt.Errorf("invalid host or port") + } + if !protocolstate.IsHostAllowed(host) { + // host is not valid according to network policy + return "", protocolstate.ErrHostDenied.Msgf(host) + } + + target := net.JoinHostPort(host, fmt.Sprintf("%d", port)) + + connString := fmt.Sprintf("sqlserver://%s:%s@%s?database=%s&connection+timeout=30", + url.PathEscape(username), + url.PathEscape(password), + target, + dbName) + + return connString, nil +} // In ExecuteQuery: - if host == "" || port <= 0 { - return nil, fmt.Errorf("invalid host or port") - } - if !protocolstate.IsHostAllowed(host) { - // host is not valid according to network policy - return nil, protocolstate.ErrHostDenied.Msgf(host) - } - - target := net.JoinHostPort(host, fmt.Sprintf("%d", port)) - - connString := fmt.Sprintf("sqlserver://%s:%s@%s?database=%s&connection+timeout=30", - url.PathEscape(username), - url.PathEscape(password), - target, - dbName) + connString, err := buildConnectionString(host, port, username, password, dbName) + if err != nil { + return nil, err + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/js/generated/ts/mssql.tsis excluded by!**/generated/**
📒 Files selected for processing (1)
pkg/js/libs/mssql/mssql.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
🔇 Additional comments (4)
pkg/js/libs/mssql/mssql.go (4)
14-14: Import looks good.The addition of the utils package is appropriate for the new ExecuteQuery functionality, specifically for unmarshaling SQL query results.
137-146: Documentation is clear and helpful.The JSDoc-style comments and example usage provide good guidance for developers using this method.
177-183: Clarify the handling of partial results.The current implementation returns partial data when unmarshaling fails. While this might be useful in some scenarios, it could also lead to confusion or incorrect behavior. Consider:
- Adding a comment explaining this behavior
- Returning a warning along with the partial data
- Making this behavior configurable
Is this behavior of returning partial results intentional and are there scenarios where this is beneficial? If so, consider documenting this behavior in the function's comments.
137-185: Overall implementation looks good.The new ExecuteQuery method successfully implements MSSQL query execution capability with proper connection management and error handling. It's well-documented with clear examples and follows the pattern established by other methods in this file.
| rows, err := db.Query(query) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit rows.Close() to ensure proper resource cleanup.
Although many SQL drivers automatically close rows when the connection is closed, it's a best practice to explicitly close the rows after they're no longer needed. This prevents resource leaks, especially in case of errors.
rows, err := db.Query(query)
if err != nil {
return nil, err
}
+ defer rows.Close()| rows, err := db.Query(query) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using parameterized queries for security.
The current implementation passes the query string directly to the database, which could potentially lead to SQL injection if the query contains user input. Consider adding a parameter validation mechanism or using parameterized queries.
- rows, err := db.Query(query)
+ // For queries without parameters
+ rows, err := db.Query(query)
+
+ // For queries with parameters, consider using:
+ // rows, err := db.Query(query, param1, param2, ...)📝 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.
| rows, err := db.Query(query) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // For queries without parameters | |
| rows, err := db.Query(query) | |
| // For queries with parameters, consider using: | |
| // rows, err := db.Query(query, param1, param2, ...) | |
| if err != nil { | |
| return nil, err | |
| } |
Proposed changes
Support for execute to mssql
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
Closes #6156
Failing functional tests tracked at #6157