optimize sql driver connections.

[hchen2020]

PR Type

Enhancement

---

Description

• Refactor SQL driver connection management to use centralized data source configuration

• Replace multiple database-specific connection string properties with unified Connections array

• Add data source name parameter to SQL query execution requests and function arguments

• Implement new endpoint to retrieve connection settings with masked connection strings

• Update all database query methods to accept connection string as parameter instead of retrieving from settings

---

Diagram Walkthrough

flowchart LR
  A["SqlDriverSetting<br/>Connections Array"] -- "lookup by name" --> B["DataSourceSetting<br/>DbType, Name, ConnectionString"]
  C["SqlQueryRequest<br/>+ DataSource"] -- "passes to" --> D["ExecuteQueryFn"]
  D -- "retrieves connection" --> A
  E["GetConnectionSettings<br/>Endpoint"] -- "returns masked" --> B
  D -- "passes to" --> F["Database Query Methods<br/>RunQueryInMySql, etc."]

Loading

File Walkthrough

Relevant files

Enhancement

15 files IText2SqlHook.cs Make GetConnectionString return type nullable                        +1/-1      SqlDriverController.cs Add data source parameter and new connection settings endpoint +22/-1    SqlQueryRequest.cs Add DataSource property to SQL query request model              +4/-0      ExecuteQueryFn.cs Refactor to use connection string parameter and data source lookup +10/-9    GetTableDefinitionFn.cs Update database methods to accept connection string parameter +8/-3      SqlSelect.cs Refactor query methods to use passed connection string parameter +18/-17  VerifyDictionaryTerm.cs Use SQL hook to retrieve connection string instead of settings +3/-2      SqlDriverPlanningHook.cs Simplify to use single required SQL hook instance                +2/-2      ExecuteQueryArgs.cs Add DataSource and ConnectionString properties to execute query arguments +4/-0      DbKnowledgeService.cs Update connection initialization to use empty string placeholder +3/-2      DataSourceSetting.cs Create new data source configuration model class                  +9/-0      SqlDriverSetting.cs Replace individual connection strings with unified Connections array +1/-8      GetTableDefinitionFn.cs Update connection initialization to use empty string placeholder +3/-3      SqlSelect.cs Refactor query methods to use passed connection string parameter +20/-17  VerifyDictionaryTerm.cs Use SQL hook to retrieve connection string instead of settings +3/-2

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Information exposure Description: The new unauthenticated-looking GET /sql-driver/connections endpoint returns internal database metadata (DbType and Name) which can enable environment reconnaissance and targeting even though the ConnectionString is masked. SqlDriverController.cs [88-102] Referred Code [HttpGet] [Route("/sql-driver/connections")] public IActionResult GetConnectionSettings() { var settings = _services.GetRequiredService<SqlDriverSetting>(); var connections = settings.Connections.Select(x => new DataSourceSetting { DbType = x.DbType, Name = x.Name, ConnectionString = "**********" }).ToArray(); return Ok(connections); }
	SSRF via connection string Description: dbConnectionString can be sourced from IText2SqlHook.GetConnectionString(message) and then used to instantiate DB clients, so if the hook reads user-influenced state this can enable server-side request forgery (SSRF) / arbitrary outbound connections to attacker-controlled hosts via crafted connection strings. ExecuteQueryFn.cs [32-34] Referred Code var connectionString = _setting.Connections.FirstOrDefault(x => x.Name.Equals(args.DataSource, StringComparison.OrdinalIgnoreCase))?.ConnectionString; var dbConnectionString = dbHook.GetConnectionString(message) ?? connectionString ?? throw new Exception("database connection is not found");
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🔴	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: 🏷️ Missing audit context: The new /sql-driver/connections endpoint returns security-relevant configuration data without any audit log containing user identity, action, and outcome. Referred Code [HttpGet] [Route("/sql-driver/connections")] public IActionResult GetConnectionSettings() { var settings = _services.GetRequiredService<SqlDriverSetting>(); var connections = settings.Connections.Select(x => new DataSourceSetting { DbType = x.DbType, Name = x.Name, ConnectionString = "**********" }).ToArray(); return Ok(connections); } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: 🏷️ Misspelled error text: Newly added exception messages contain misspellings (e.g., "database connectdion") that reduce clarity and professionalism of self-documenting code. Referred Code var dbConnectionString = dbHook.GetConnectionString(message) ?? _settings.Connections.FirstOrDefault(c => c.DbType == dbType)?.ConnectionString ?? throw new Exception("database connectdion is not found"); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: 🏷️ Invalid connection usage: New code instantiates database connections with string.Empty, which will fail at runtime and is not guarded by validation or actionable error handling. Referred Code using var connection = new MySqlConnection(string.Empty); var sql = $"select table_name from information_schema.tables where table_schema = @tableSchema"; var results = connection.Query(sql, new { Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: 🏷️ Logs raw SQL: Newly added logging records full SQL statements via LogInformation, which can leak sensitive literals and is not masked or minimized. Referred Code // Print all the SQL statements for debugging _logger.LogInformation("Executing SQL Statements: {SqlStatements}", string.Join("\r\n", args.SqlStatements)); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: 🏷️ Unvalidated SQL execution: The controller accepts SqlStatement and DataSource from the request and routes them to execution without validation/authorization controls shown in the diff, enabling unsafe arbitrary query execution. Referred Code public async Task<IActionResult> ExecuteSqlQuery([FromRoute] string conversationId, [FromBody] SqlQueryRequest sqlQueryRequest) { var match = Regex.Match(sqlQueryRequest.SqlStatement, @"```sql\s*([\s\S]*?)\s*```", RegexOptions.IgnoreCase); if (match.Success) { sqlQueryRequest.SqlStatement = match.Groups[1].Value.Trim(); } var fn = _services.GetRequiredService<IRoutingService>(); var conv = _services.GetRequiredService<IConversationService>(); conv.SetConversationId(conversationId, [ new MessageState("database_type", sqlQueryRequest.DbType), new MessageState("data_source_name", sqlQueryRequest.DataSource), ]); var msg = new RoleDialogModel(AgentRole.User, sqlQueryRequest.SqlStatement) { CurrentAgentId = sqlQueryRequest.AgentId }; ... (clipped 8 lines) Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: 🏷️ Exception may surface: The new throw new Exception("database connection is not found") may propagate to user-facing responses depending on global exception handling, which requires verification that errors are sanitized externally. Referred Code var connectionString = _setting.Connections.FirstOrDefault(x => x.Name.Equals(args.DataSource, StringComparison.OrdinalIgnoreCase))?.ConnectionString; var dbConnectionString = dbHook.GetConnectionString(message) ?? connectionString ?? throw new Exception("database connection is not found"); Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix empty database connection string Initialize MySqlConnection with a valid connection string from settings instead of an empty string to prevent a runtime exception. src/Plugins/BotSharp.Plugin.SqlDriver/Services/DbKnowledgeService.cs [36] -using var connection = new MySqlConnection(string.Empty); +var connectionString = sqlDriverSettings.Connections.FirstOrDefault(c => c.DbType.Equals("mysql", StringComparison.OrdinalIgnoreCase))?.ConnectionString; +using var connection = new MySqlConnection(connectionString); Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical bug introduced in the PR where a MySqlConnection is initialized with an empty string, which would cause a runtime crash.	High
	Handle invalid function arguments properly Throw an ArgumentException if ExecuteQueryArgs deserialization fails instead of creating a new empty object to prevent executing an invalid query. src/Plugins/BotSharp.Plugin.SqlDriver/Functions/ExecuteQueryFn.cs [28] -var args = JsonSerializer.Deserialize<ExecuteQueryArgs>(message.FunctionArgs) ?? new(); +var args = JsonSerializer.Deserialize<ExecuteQueryArgs>(message.FunctionArgs) + ?? throw new ArgumentException("Function arguments are missing or invalid."); Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that creating a new empty ExecuteQueryArgs on deserialization failure can lead to silent errors and recommends throwing an exception, which is a more robust error-handling strategy.	Medium
	Use specific data source from state Retrieve the connection string using the specific data_source_name from the conversation state instead of just the first matching DbType to ensure the correct database is used. src/Plugins/BotSharp.Plugin.SqlDriver/Functions/GetTableDefinitionFn.cs [49-50] -var connectionString = settings.Connections.FirstOrDefault(x => x.DbType == "mysql")?.ConnectionString; +var state = _services.GetRequiredService<IConversationStateService>(); +var dataSourceName = state.GetState("data_source_name"); +var connectionString = settings.Connections.FirstOrDefault(x => x.Name.Equals(dataSourceName, StringComparison.OrdinalIgnoreCase))?.ConnectionString ?? + settings.Connections.FirstOrDefault(x => x.DbType == "mysql")?.ConnectionString; using var connection = new MySqlConnection(connectionString); Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that fetching the connection by DbType is not robust and proposes a better approach using the data_source_name from the conversation state, which aligns with the PR's intent.	Medium
Learned best practice	Add null checks for connections GetConnectionString is now nullable, so validate it (or fall back to configured data sources) before constructing MySqlConnection to avoid runtime exceptions and clearer failures. src/Plugins/BotSharp.Plugin.SqlDriver/Functions/VerifyDictionaryTerm.cs [51-53] var sqlHook = _services.GetRequiredService<IText2SqlHook>(); -var connectionString = sqlHook.GetConnectionString(message); +var connectionString = sqlHook.GetConnectionString(message) + ?? throw new InvalidOperationException("Database connection string is not configured."); using var connection = new MySqlConnection(connectionString); Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Improve defensive coding with precise null/state checks to prevent crashes and incorrect behavior (e.g., avoid passing null into connection constructors).	Low
General	Strengthen connection string lookup Improve the connection string lookup by adding a fallback to match by DbType if the DataSource name is not found, and throw a more specific exception on failure. src/Plugins/BotSharp.Plugin.SqlDriver/Functions/ExecuteQueryFn.cs [32-33] -var connectionString = _setting.Connections.FirstOrDefault(x => x.Name.Equals(args.DataSource, StringComparison.OrdinalIgnoreCase))?.ConnectionString; -var dbConnectionString = dbHook.GetConnectionString(message) ?? connectionString ?? throw new Exception("database connection is not found"); +var dbConnectionString = dbHook.GetConnectionString(message) + ?? _setting.Connections + .FirstOrDefault(c => + c.Name.Equals(args.DataSource, StringComparison.OrdinalIgnoreCase) || + c.DbType.Equals(dbType, StringComparison.OrdinalIgnoreCase)) + ?.ConnectionString + ?? throw new InvalidOperationException($"Connection for data source '{args.DataSource}' not found."); Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion improves the connection string lookup logic by adding a fallback to match by DbType, making the code more resilient if a DataSource name match fails.	Low
More