Add support for specifying a custom table name for storing chat messages#5
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for specifying a custom table name for storing chat messages in the MS SQL-backed short-term memory store, addressing issue #1976. The implementation includes proper input validation to prevent SQL injection vulnerabilities while maintaining backward compatibility through a default table name value.
Key Changes:
- Added a
tableNameparameter toShortTermMemoryStore.init()with validation and a default value of "ChatMessages" - Implemented a placeholder-based table name replacement mechanism for all SQL queries
- Version bumped from 1.0.x to 1.1.0, reflecting a minor feature addition
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| gradle.properties | Version bump to 1.1.0-SNAPSHOT for the new feature |
| ballerina/Ballerina.toml | Package version updated to 1.1.0 |
| ballerina/Dependencies.toml | Added lang.regexp dependency and updated package version; also includes transitive dependency updates (http 2.14.8, mime 2.12.1) |
| ballerina/store.bal | Core implementation: added tableName parameter with regex validation, introduced replaceTableNamePlaceholder function, and updated all SQL queries to use the dynamic table name |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ballerina/store.bal
Outdated
|
|
||
| if tableExists is sql:Error { | ||
| return error("Failed to check existence of the ChatMessages table: " + tableExists.message(), | ||
| return error(string `Failed to check existence of the $_tableName_$ table: ` + tableExists.message(), |
| } | ||
| } | ||
|
|
||
| isolated function replaceTableNamePlaceholder(sql:ParameterizedQuery query, string tableName) returns sql:ParameterizedQuery { |
There was a problem hiding this comment.
Can fix later, but technically we are creating two objects for each DB query. Also the table name is fixed at initialization. Wonder if we can absorb some pain and improve this, given that having to change the table name may not be common.
There was a problem hiding this comment.
Are we creating two objects for each query? I believe we're creating a single object and updating the same one.
I agree with the second point, but I don't think we have a cleaner way to handle this since the SQL library doesn’t provide functionality for this kind of scenario, AFAIK. we’ll proceed with this approach for now.
There was a problem hiding this comment.
Yeah, my bad. We can still avoid the repeated array creation though. We can do this separately.
There was a problem hiding this comment.
One approach would be to use function pointers (with a closure for when the table name is a variable).
| } | ||
| } | ||
|
|
||
| isolated function replaceTableNamePlaceholder(sql:ParameterizedQuery query, string tableName) returns sql:ParameterizedQuery { |
There was a problem hiding this comment.
Yeah, my bad. We can still avoid the repeated array creation though. We can do this separately.
ballerina/store.bal
Outdated
|
|
||
| if tableExists is sql:Error { | ||
| return error("Failed to check existence of the ChatMessages table: " + tableExists.message(), | ||
| return error(string `Failed to check existence of the ${self.tableName} table: ` + tableExists.message(), |
There was a problem hiding this comment.
tableExists.message() can also be an interpolation?
| } | ||
| } | ||
|
|
||
| isolated function replaceTableNamePlaceholder(sql:ParameterizedQuery query, string tableName) returns sql:ParameterizedQuery { |
There was a problem hiding this comment.
One approach would be to use function pointers (with a closure for when the table name is a variable).
4 participants
Purpose
Fixes: wso2/product-ballerina-integrator#1976
Examples
Checklist