mysql support #536
mysql support #536
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds MySQL support for config and logs stores: adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Factory as NewConfigStore
participant MySQLCtor as newMySQLConfigStore
participant GORM as GORM(MySQL)
participant Migrator as triggerMigrations
App->>Factory: Request store (type: "mysql", config: MySQLConfig)
alt type == "mysql" and config ok
Factory->>MySQLCtor: init with MySQLConfig
MySQLCtor->>GORM: gorm.Open(mysql, DSN)
GORM-->>MySQLCtor: db / error
alt open ok
MySQLCtor->>Migrator: triggerMigrations(ctx, db)
Migrator-->>MySQLCtor: ok / error
alt migrations ok
MySQLCtor-->>Factory: RDBConfigStore
Factory-->>App: ConfigStore
else migrations error
MySQLCtor-->>Factory: error
Factory-->>App: error
end
else open error
MySQLCtor-->>Factory: error
Factory-->>App: error
end
else invalid config
Factory-->>App: error ("invalid mysql config")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
akshaydeo
force-pushed
the
10-01-posgres_support
branch
from
5114e45 to
b8e4d4d
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
41734f2 to
e817121
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
framework/configstore/mysql.go (2)
12-20: Consider adding validation for configuration fields.The MySQLConfig struct lacks validation for required fields and sensible defaults. Missing validation could lead to cryptic connection errors.
Consider adding a validation method:
// Validate checks if the MySQL configuration is valid. func (c *MySQLConfig) Validate() error { if c.Username == "" { return fmt.Errorf("username is required") } if c.Host == "" { return fmt.Errorf("host is required") } if c.Port <= 0 || c.Port > 65535 { return fmt.Errorf("port must be between 1 and 65535") } if c.DBName == "" { return fmt.Errorf("db_name is required") } return nil }Then call it in
newMySQLConfigStorebefore constructing the DSN.
22-34: Code duplication with postgres.go.The
newMySQLConfigStorefunction follows nearly identical logic tonewPostgresConfigStore(DSN construction, gorm.Open, RDBConfigStore creation, triggerMigrations). Consider extracting a shared helper to reduce duplication.Example refactor:
// Helper function (add to rdb.go or a new helpers.go) func newRDBConfigStore(ctx context.Context, dialector gorm.Dialector, logger schemas.Logger) (ConfigStore, error) { db, err := gorm.Open(dialector, &gorm.Config{}) if err != nil { return nil, fmt.Errorf("failed to open database: %w", err) } d := &RDBConfigStore{db: db, logger: logger} if err := triggerMigrations(ctx, db); err != nil { return nil, fmt.Errorf("failed to run migrations: %w", err) } return d, nil } // Then simplify both constructors: func newMySQLConfigStore(ctx context.Context, config *MySQLConfig, logger schemas.Logger) (ConfigStore, error) { dsn := buildMySQLDSN(config) return newRDBConfigStore(ctx, mysql.Open(dsn), logger) }framework/configstore/postgres.go (1)
14-19: Consider adding validation for configuration fields.Similar to MySQLConfig, PostgresConfig lacks validation for required fields. This could lead to cryptic connection errors.
Add a validation method:
// Validate checks if the Postgres configuration is valid. func (c *PostgresConfig) Validate() error { if c.Username == "" { return fmt.Errorf("username is required") } if c.Host == "" { return fmt.Errorf("host is required") } if c.Port <= 0 || c.Port > 65535 { return fmt.Errorf("port must be between 1 and 65535") } if c.DBName == "" { return fmt.Errorf("db_name is required") } // SSLMode validation for Postgres validSSLModes := []string{"disable", "require", "verify-ca", "verify-full"} if c.SSLMode != "" { valid := false for _, mode := range validSSLModes { if c.SSLMode == mode { valid = true break } } if !valid { return fmt.Errorf("invalid ssl_mode: %s (valid: disable, require, verify-ca, verify-full)", c.SSLMode) } } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/postgres.go(2 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
framework/configstore/postgres.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/config.go (1)
Config(19-23)
framework/configstore/mysql.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/rdb.go (1)
RDBConfigStore(16-19)
framework/configstore/store.go (2)
framework/configstore/config.go (2)
ConfigStoreTypeMySQL(15-15)Config(19-23)framework/configstore/mysql.go (1)
MySQLConfig(13-20)
framework/configstore/config.go (1)
framework/configstore/mysql.go (1)
MySQLConfig(13-20)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (5)
framework/configstore/store.go (1)
126-130: LGTM!The MySQL case follows the existing pattern for SQLite and Postgres, with appropriate type assertion and error handling.
framework/configstore/config.go (2)
15-15: LGTM!The MySQL constant is properly declared alongside the existing config store types.
62-67: LGTM!The MySQL unmarshal case follows the established pattern for Postgres and SQLite, with appropriate error handling.
framework/configstore/postgres.go (1)
14-19: Breaking change: Postgres configuration refactored from DSN to individual fields.This is a documented breaking change that improves consistency with the new MySQL config. Existing Postgres configurations using the
dsnfield must migrate to individual connection parameters.Consider documenting a migration example in the PR description or a migration guide:
// Before: { "type": "postgres", "config": { "dsn": "host=localhost user=myuser password=mypass dbname=mydb port=5432 sslmode=disable" } } // After: { "type": "postgres", "config": { "username": "myuser", "password": "mypass", "host": "localhost", "port": 5432, "db_name": "mydb", "ssl_mode": "disable" } }framework/go.mod (1)
19-20: Dependencies verified: no known vulnerabilitiesSecurity advisory checks against GitHub’s database show no issues for go-sql-driver/mysql v1.8.1 or gorm.io/driver/mysql v1.6.0. Proceed with these updates.
framework/configstore/mysql.go
Outdated
|
|
||
| // newMySQLConfigStore creates a new MySQL config store. | ||
| func newMySQLConfigStore(ctx context.Context, config *MySQLConfig, logger schemas.Logger) (ConfigStore, error) { | ||
| db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) |
There was a problem hiding this comment.
Critical: MySQL driver does not support sslmode parameter.
The DSN construction uses sslmode=%s, but the MySQL driver (go-sql-driver/mysql) expects tls=<config-name> for TLS/SSL connections, not sslmode. This will cause SSL connections to fail silently or be ignored.
Apply this diff to fix the SSL parameter:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{})
+ dsn := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local",
+ config.Username, config.Password, config.Host, config.Port, config.DBName)
+ // MySQL driver uses 'tls' parameter, common values: "true", "false", "skip-verify", or custom config name
+ if config.SSLMode != "" && config.SSLMode != "disable" {
+ if config.SSLMode == "require" {
+ dsn += "&tls=true"
+ } else {
+ dsn += "&tls=" + config.SSLMode
+ }
+ }
+ db, err := gorm.Open(mysql.Open(dsn), &gorm.Config{})See go-sql-driver/mysql documentation for TLS configuration: https://github.com/go-sql-driver/mysql#tls
📝 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.
| db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) | |
| dsn := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local", | |
| config.Username, config.Password, config.Host, config.Port, config.DBName) | |
| // MySQL driver uses 'tls' parameter, common values: "true", "false", "skip-verify", or custom config name | |
| if config.SSLMode != "" && config.SSLMode != "disable" { | |
| if config.SSLMode == "require" { | |
| dsn += "&tls=true" | |
| } else { | |
| dsn += "&tls=" + config.SSLMode | |
| } | |
| } | |
| db, err := gorm.Open(mysql.Open(dsn), &gorm.Config{}) |
🤖 Prompt for AI Agents
In framework/configstore/mysql.go around line 24 the DSN uses the unsupported
MySQL parameter `sslmode=%s`; change this to use the go-sql-driver/mysql
`tls=<name>` parameter instead and ensure any required TLS config is registered
with mysql.RegisterTLSConfig before opening the DB. Concretely, map your
existing SSLMode value to an appropriate tls name (e.g., "true"/"skip-verify" or
a custom name), register a tls.Config under that name via
mysql.RegisterTLSConfig, and replace `sslmode=%s` in the fmt.Sprintf with
`tls=%s` (passing the registered name). Refer to the go-sql-driver/mysql TLS
docs for examples and required registration steps.
Security: DSN with embedded credentials may be logged.
Constructing DSNs inline can expose credentials in logs if GORM or the driver logs the connection string. Consider wrapping errors to avoid credential leakage.
You can improve security by:
- Using
mysql.Configconstructor instead of DSN string:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{})
+ mysqlConfig := mysql.Config{
+ User: config.Username,
+ Passwd: config.Password,
+ Net: "tcp",
+ Addr: fmt.Sprintf("%s:%d", config.Host, config.Port),
+ DBName: config.DBName,
+ ParseTime: true,
+ Loc: time.Local,
+ // TLS configuration would go here based on SSLMode
+ }
+ db, err := gorm.Open(mysql.New(mysql.Config{
+ DSNConfig: &mysqlConfig,
+ }), &gorm.Config{})- Wrapping connection errors to avoid exposing the DSN:
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to connect to MySQL database: %w", err)
}Based on learnings.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In framework/configstore/mysql.go around line 24, the code builds a DSN inline
(fmt.Sprintf) which can leak credentials if the DSN is logged; replace the
inline DSN with the mysql.Config struct/builder (mysql.NewConfig or
&mysql.Config{User:, Passwd:, Net:"tcp", Addr: fmt.Sprintf("%s:%d", Host, Port),
DBName:, Params: map[string]string{...}}) and pass that config to mysql.Open to
avoid embedding credentials in a single string, and when handling gorm.Open
errors wrap or return a sanitized error message (e.g., "failed to open MySQL
connection") without including the DSN or raw config values so credentials are
never leaked in logs or error messages.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
framework/configstore/mysql.go (1)
24-24: Consider extracting DSN construction to reduce duplication.Both
mysql.goandpostgres.goinline DSN construction withfmt.Sprintf. Extracting each to a method would improve testability and maintainability.Consider adding a helper method:
func (c *MySQLConfig) DSN() string { return fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&tls=%s", c.Username, c.Password, c.Host, c.Port, c.DBName, c.TLSMode) }Then simplify the constructor:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&tls=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.TLSMode)), &gorm.Config{}) + db, err := gorm.Open(mysql.Open(config.DSN()), &gorm.Config{})Apply the same pattern to
PostgresConfigfor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/postgres.go(2 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
framework/configstore/postgres.go (2)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)
framework/configstore/mysql.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/rdb.go (1)
RDBConfigStore(16-19)
framework/configstore/config.go (1)
framework/configstore/mysql.go (1)
MySQLConfig(13-20)
framework/configstore/store.go (2)
framework/configstore/config.go (2)
ConfigStoreTypeMySQL(15-15)Config(19-23)framework/configstore/mysql.go (1)
MySQLConfig(13-20)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (5)
framework/configstore/store.go (1)
126-130: LGTM!The MySQL case follows the established pattern for SQLite and Postgres, with proper type assertion and error handling.
framework/configstore/config.go (2)
15-15: LGTM!MySQL constant added consistently with existing config store types.
62-67: LGTM!MySQL unmarshalling logic follows the established pattern for SQLite and Postgres with proper error handling.
framework/configstore/mysql.go (1)
13-20: LGTM!MySQLConfig struct fields are appropriately defined with correct JSON tags.
framework/configstore/postgres.go (1)
14-19: Remove DSN migration warning — no legacy DSN support exists
There are no references to a"dsn"field in code, tests, or documentation—PostgresConfig has always used discrete fields in this codebase.Likely an incorrect or invalid review comment.
| func newMySQLConfigStore(ctx context.Context, config *MySQLConfig, logger schemas.Logger) (ConfigStore, error) { | ||
| db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Add input validation for connection parameters.
The constructor does not validate required fields (username, host, dbname) or port range. Invalid configurations will fail at connection time with obscure errors rather than clear validation messages.
Add validation before attempting connection:
func newMySQLConfigStore(ctx context.Context, config *MySQLConfig, logger schemas.Logger) (ConfigStore, error) {
+ // Validate required fields
+ if config.Username == "" {
+ return nil, fmt.Errorf("mysql username is required")
+ }
+ if config.Host == "" {
+ return nil, fmt.Errorf("mysql host is required")
+ }
+ if config.DBName == "" {
+ return nil, fmt.Errorf("mysql database name is required")
+ }
+ if config.Port <= 0 || config.Port > 65535 {
+ return nil, fmt.Errorf("mysql port must be between 1 and 65535, got %d", config.Port)
+ }
+
db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&tls=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.TLSMode)), &gorm.Config{})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In framework/configstore/mysql.go around lines 23 to 27, the constructor calls
gorm.Open without validating connection parameters; add pre-checks to return
clear errors: validate that config.Username, config.Host and config.DBName are
non-empty, ensure config.Port is within 1-65535 (or non-zero positive int), and
optionally validate SSLMode if you have a set of allowed values; if any check
fails return a descriptive error (fmt.Errorf or errors.New) before calling
gorm.Open so callers get immediate, actionable validation messages.
framework/configstore/mysql.go
Outdated
|
|
||
| // newMySQLConfigStore creates a new MySQL config store. | ||
| func newMySQLConfigStore(ctx context.Context, config *MySQLConfig, logger schemas.Logger) (ConfigStore, error) { | ||
| db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) |
There was a problem hiding this comment.
Critical: Incorrect SSL parameter for MySQL DSN.
The MySQL driver does not recognize the sslmode parameter—that's PostgreSQL-specific. MySQL uses tls=<value> (e.g., tls=true, tls=skip-verify, or a custom TLS config name). This will either cause connection failures or silently ignore SSL settings, creating a security risk.
Apply this diff to use the correct MySQL TLS parameter:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{})
+ db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&tls=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{})Also consider renaming the SSLMode field in MySQLConfig to TLSMode for clarity:
type MySQLConfig struct {
Username string `json:"username"`
Password string `json:"password"`
Host string `json:"host"`
Port int `json:"port"`
DBName string `json:"db_name"`
- SSLMode string `json:"ssl_mode"`
+ TLSMode string `json:"tls_mode"`
}Then update the DSN construction accordingly:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&tls=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{})
+ db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&tls=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.TLSMode)), &gorm.Config{})🤖 Prompt for AI Agents
In framework/configstore/mysql.go around line 24, the DSN uses the
PostgreSQL-specific sslmode param which MySQL ignores; change the DSN to use the
MySQL TLS parameter (tls=<value>) instead of sslmode, update the DSN formatting
to inject config.TLSMode (or config.SSLMode -> renamed TLSMode) accordingly, and
rename the MySQLConfig field from SSLMode to TLSMode (plus update all
references/usages and any JSON/tags) so the connection uses the correct TLS
setting (e.g., tls=skip-verify or tls=true) when opening gorm/mysql.
| filippo.io/edwards25519 v1.1.0 // indirect | ||
| github.com/go-sql-driver/mysql v1.8.1 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
Verify MySQL driver versions and security advisories.
Ensure the MySQL driver dependencies are up-to-date and free from known vulnerabilities.
Run the following script to check for the latest versions and security advisories:
Also applies to: 98-98
🏁 Script executed:
#!/bin/bash
# Description: Check MySQL driver versions and security advisories
echo "=== Checking latest versions on pkg.go.dev ==="
echo "Current: gorm.io/driver/mysql v1.6.0"
curl -s "https://proxy.golang.org/gorm.io/driver/mysql/@latest" | jq -r '.Version'
echo -e "\nCurrent: github.com/go-sql-driver/mysql v1.8.1"
curl -s "https://proxy.golang.org/github.com/go-sql-driver/mysql/@latest" | jq -r '.Version'
echo -e "\n=== Checking for security advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/go-sql-driver/mysql") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 237
Update MySQL driver to v1.9.3
Bump github.com/go-sql-driver/mysql from v1.8.1 to v1.9.3 in framework/go.mod (lines 19–20); gorm.io/driver/mysql at line 98 remains at v1.6.0 (latest). No GO ecosystem security advisories detected.
🤖 Prompt for AI Agents
In framework/go.mod around lines 19 to 20, the MySQL driver is pinned to
github.com/go-sql-driver/mysql v1.8.1; update that module to v1.9.3 by changing
the version specifier, then run go mod tidy (and commit updated go.sum) to pull
the new version and clean dependencies; after updating, run unit/integration
tests (and verify gorm.io/driver/mysql v1.6.0 compatibility) and fix any minor
API or dependency issues surfaced by the new driver.
b8e4d4d to
bd9b247
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
e817121 to
b42cdce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
framework/configstore/postgres.go (1)
24-26: Past review issues remain unaddressed.The two major concerns previously flagged in past reviews have not been resolved:
Security: DSN with embedded credentials may be logged - Constructing DSNs inline can expose credentials in logs if GORM or the driver logs the connection string. Consider using the Postgres driver's Config constructor or pgx connection configuration directly to avoid DSN string construction.
Missing input validation - The constructor lacks validation for required fields (Username, Host, DBName, Password) and port range (1-65535). This results in obscure connection errors instead of clear validation messages. Add validation before attempting connection as suggested in the previous review.
🧹 Nitpick comments (1)
framework/configstore/postgres.go (1)
27-29: Consider wrapping connection errors for better debugging.Wrapping the connection error with additional context would help identify the source of failures more easily.
Apply this diff:
db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to connect to Postgres database: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/postgres.go(2 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/configstore/config.go
- framework/configstore/mysql.go
- framework/configstore/store.go
- framework/go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
framework/configstore/postgres.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/config.go (1)
Config(19-23)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
framework/configstore/postgres.go (1)
13-20: Breaking change: PostgresConfig refactored to individual connection fields.The struct definition is clear and aligns with the MySQL implementation for consistency. The breaking change from DSN string to individual parameters is acknowledged in the PR objectives.
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
b42cdce to
c8af440
Compare
akshaydeo
force-pushed
the
10-01-posgres_support
branch
from
bd9b247 to
57ce6b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
framework/configstore/postgres.go (1)
23-29: Add input validation and improve security posture.The constructor has two unresolved issues from previous reviews:
Missing input validation: No validation for required fields (Username, Host, DBName, Password) or port range (1-65535). Invalid configs will fail with obscure connection errors.
Security concern: Inline DSN construction can expose credentials in logs if GORM or the Postgres driver logs the connection string. Consider using
postgres.Configconstructor and wrapping errors.Apply validation before connection:
func newPostgresConfigStore(ctx context.Context, config *PostgresConfig, logger schemas.Logger) (ConfigStore, error) { + // Validate required fields + if config.Username == "" { + return nil, fmt.Errorf("postgres username is required") + } + if config.Host == "" { + return nil, fmt.Errorf("postgres host is required") + } + if config.DBName == "" { + return nil, fmt.Errorf("postgres database name is required") + } + if config.Port <= 0 || config.Port > 65535 { + return nil, fmt.Errorf("postgres port must be between 1 and 65535, got %d", config.Port) + } + // Optional: validate SSLMode against accepted values (disable, require, verify-ca, verify-full) + dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d sslmode=%s", config.Host, config.Username, config.Password, config.DBName, config.Port, config.SSLMode) db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to connect to Postgres database: %w", err) }Based on learnings.
framework/configstore/mysql.go (1)
23-34: Critical: Fix MySQL TLS parameter and add validation.Three unresolved issues from previous reviews:
CRITICAL - Incorrect SSL parameter (Line 24): MySQL does not recognize
sslmode— that's PostgreSQL-specific. The MySQL driver requirestls=<value>(e.g.,tls=true,tls=skip-verify). Usingsslmodewill cause the parameter to be silently ignored, creating a security risk.Missing input validation: No validation for required fields (Username, Host, DBName) or port range (1-65535). Invalid configs fail with obscure connection errors.
Security concern: Inline DSN construction can expose credentials in logs if GORM or the MySQL driver logs the connection string.
Apply these fixes:
func newMySQLConfigStore(ctx context.Context, config *MySQLConfig, logger schemas.Logger) (ConfigStore, error) { + // Validate required fields + if config.Username == "" { + return nil, fmt.Errorf("mysql username is required") + } + if config.Host == "" { + return nil, fmt.Errorf("mysql host is required") + } + if config.DBName == "" { + return nil, fmt.Errorf("mysql database name is required") + } + if config.Port <= 0 || config.Port > 65535 { + return nil, fmt.Errorf("mysql port must be between 1 and 65535, got %d", config.Port) + } + + // Build DSN with correct MySQL TLS parameter + dsn := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local", + config.Username, config.Password, config.Host, config.Port, config.DBName) + + // MySQL uses 'tls' parameter, not 'sslmode' + if config.SSLMode != "" && config.SSLMode != "disable" { + if config.SSLMode == "require" { + dsn += "&tls=true" + } else { + dsn += "&tls=" + config.SSLMode + } + } + - db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) + db, err := gorm.Open(mysql.Open(dsn), &gorm.Config{}) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to connect to MySQL database: %w", err) }If you rename
SSLModetoTLSMode(as suggested in the previous comment), update the references accordingly.Based on learnings.
🧹 Nitpick comments (1)
framework/configstore/mysql.go (1)
13-20: Consider renaming SSLMode to TLSMode for MySQL clarity.While the struct mirrors PostgresConfig for consistency, MySQL uses the
tlsparameter (notsslmode). Renaming this field toTLSModewould better reflect MySQL's TLS configuration and avoid confusion with PostgreSQL's sslmode.Apply this diff:
type MySQLConfig struct { Username string `json:"username"` Password string `json:"password"` Host string `json:"host"` Port int `json:"port"` DBName string `json:"db_name"` - SSLMode string `json:"ssl_mode"` + TLSMode string `json:"tls_mode"` }Then update the constructor accordingly (see next comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/postgres.go(2 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/configstore/config.go
- framework/configstore/store.go
- framework/go.mod
🧰 Additional context used
🧬 Code graph analysis (2)
framework/configstore/postgres.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/config.go (1)
Config(19-23)
framework/configstore/mysql.go (4)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/config.go (1)
Config(19-23)framework/configstore/rdb.go (1)
RDBConfigStore(16-19)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
framework/configstore/postgres.go (1)
13-20: LGTM! PostgresConfig structure is well-defined.The refactored PostgresConfig with individual connection parameters follows the standard pattern and improves clarity over a single DSN string. The field names and JSON tags are appropriate.
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
c8af440 to
a8be29e
Compare
akshaydeo
force-pushed
the
10-01-posgres_support
branch
from
57ce6b5 to
c0e98bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
framework/go.mod (1)
19-20: Update go-sql-driver/mysql to v1.9.3.v1.9.3 (released 2025-06-13) is the latest driver and ships connection stability fixes; remaining on v1.8.1 leaves those bugfixes behind. Please bump the module and refresh go.sum. (github.com)
framework/configstore/mysql.go (4)
19-19: RenameSSLModetoTLSModefor MySQL correctness.MySQL uses
tlsparameters, notsslmode(which is PostgreSQL-specific). The field name should reflect MySQL conventions to avoid confusion.Apply this diff:
- SSLMode string `json:"ssl_mode"` + TLSMode string `json:"tls_mode"`Then update the DSN construction in line 24 to use
config.TLSMode.
24-24: Critical: MySQL driver does not supportsslmodeparameter.The DSN uses
sslmode=%s, but MySQL driver expectstls=<value>(e.g.,true,false,skip-verify, or a custom TLS config name). This will cause SSL connections to fail or be silently ignored.Apply this diff to use the correct MySQL TLS parameter:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) + db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&tls=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.TLSMode)), &gorm.Config{})Refer to go-sql-driver/mysql documentation for valid TLS values: https://github.com/go-sql-driver/mysql#tls
23-27: Add input validation for connection parameters.Missing validation for required fields (username, host, dbname) and port range. Invalid configurations will fail at connection time with obscure GORM/driver errors.
Add validation before attempting connection:
func newMySQLConfigStore(ctx context.Context, config *MySQLConfig, logger schemas.Logger) (ConfigStore, error) { + // Validate required fields + if config.Username == "" { + return nil, fmt.Errorf("mysql username is required") + } + if config.Host == "" { + return nil, fmt.Errorf("mysql host is required") + } + if config.DBName == "" { + return nil, fmt.Errorf("mysql database name is required") + } + if config.Port <= 0 || config.Port > 65535 { + return nil, fmt.Errorf("mysql port must be between 1 and 65535, got %d", config.Port) + } + db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&tls=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.TLSMode)), &gorm.Config{})
24-27: DSN with embedded credentials may leak in logs.Inline DSN construction embeds credentials in a string that GORM or the driver may log on error. This creates a security risk.
Consider using
mysql.Configstruct to avoid credential leakage:- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&tls=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.TLSMode)), &gorm.Config{}) + mysqlConfig := mysql.Config{ + User: config.Username, + Passwd: config.Password, + Net: "tcp", + Addr: fmt.Sprintf("%s:%d", config.Host, config.Port), + DBName: config.DBName, + ParseTime: true, + // Add TLS configuration based on config.TLSMode + Params: map[string]string{"charset": "utf8mb4"}, + } + db, err := gorm.Open(mysql.New(mysql.Config{ + DSNConfig: &mysqlConfig, + }), &gorm.Config{}) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to connect to MySQL: %w", err) }Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/postgres.go(2 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/configstore/config.go
🧰 Additional context used
🧬 Code graph analysis (3)
framework/configstore/store.go (2)
framework/configstore/config.go (2)
ConfigStoreTypeMySQL(15-15)Config(19-23)framework/configstore/mysql.go (1)
MySQLConfig(13-20)
framework/configstore/postgres.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/config.go (1)
Config(19-23)
framework/configstore/mysql.go (4)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/config.go (1)
Config(19-23)framework/configstore/rdb.go (1)
RDBConfigStore(16-19)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
framework/configstore/mysql.go (1)
1-10: LGTM!Imports are appropriate for the MySQL config store implementation.
framework/configstore/postgres.go
Outdated
| dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d sslmode=%s", | ||
| config.Host, config.Username, config.Password, config.DBName, config.Port, config.SSLMode) | ||
| db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) |
There was a problem hiding this comment.
Escape Postgres DSN values before building the string.
Directly interpolating username/password/db into host=%s user=%s ... fails once a value carries spaces, quotes, or backslashes, because libpq requires those characters to be single-quoted and escaped; this regresses from the previous DSN field and will break valid credentials. (postgresql.org) Please assemble the DSN via url.URL/url.UserPassword (or pgconn.Config) so everything is escaped before handing it to GORM. For example:
+ u := &url.URL{
+ Scheme: "postgres",
+ Path: config.DBName,
+ }
+ if config.Username != "" {
+ if config.Password != "" {
+ u.User = url.UserPassword(config.Username, config.Password)
+ } else {
+ u.User = url.User(config.Username)
+ }
+ }
+ u.Host = net.JoinHostPort(config.Host, strconv.Itoa(config.Port))
+ q := u.Query()
+ if config.SSLMode != "" {
+ q.Set("sslmode", config.SSLMode)
+ }
+ u.RawQuery = q.Encode()
+ dsn := u.String()
- dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d sslmode=%s",
- config.Host, config.Username, config.Password, config.DBName, config.Port, config.SSLMode)Add the necessary imports (net, net/url, strconv) when applying this change.
📝 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.
| dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d sslmode=%s", | |
| config.Host, config.Username, config.Password, config.DBName, config.Port, config.SSLMode) | |
| db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) | |
| // Assemble a properly escaped Postgres DSN via url.URL instead of fmt.Sprintf | |
| u := &url.URL{ | |
| Scheme: "postgres", | |
| Path: config.DBName, | |
| } | |
| if config.Username != "" { | |
| if config.Password != "" { | |
| u.User = url.UserPassword(config.Username, config.Password) | |
| } else { | |
| u.User = url.User(config.Username) | |
| } | |
| } | |
| u.Host = net.JoinHostPort(config.Host, strconv.Itoa(config.Port)) | |
| q := u.Query() | |
| if config.SSLMode != "" { | |
| q.Set("sslmode", config.SSLMode) | |
| } | |
| u.RawQuery = q.Encode() | |
| dsn := u.String() | |
| db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) |
akshaydeo
force-pushed
the
10-01-posgres_support
branch
from
c0e98bb to
82eb715
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
a8be29e to
b463534
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
framework/go.mod (1)
19-20: Update MySQL driver to latest version.The past review correctly identified that
github.com/go-sql-driver/mysqlshould be updated from v1.8.1 to v1.9.3.framework/configstore/postgres.go (1)
24-26: Critical: DSN construction fails with special characters in credentials.The past review correctly identified that
fmt.Sprintfdoes not escape special characters in username, password, or database name. Credentials containing spaces, quotes, or backslashes will cause connection failures.framework/configstore/mysql.go (3)
23-27: Add input validation for connection parameters.The past review correctly identified that missing validation for required fields (username, host, dbname) and port range will result in obscure connection errors instead of clear validation messages.
24-24: Critical: MySQL driver does not supportsslmodeparameter.The past review correctly identified that MySQL uses
tls=<value>(e.g.,tls=true,tls=skip-verify) instead of the PostgreSQL-specificsslmodeparameter. This will cause SSL connections to fail or be silently ignored.Additionally, the DSN construction embeds credentials directly, which creates a security risk if the DSN is logged by GORM or the driver.
25-27: Wrap connection error to avoid credential leakage.Connection errors may include the DSN string with embedded credentials. Return a sanitized error message.
Apply this diff:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to connect to MySQL database: %w", err) }
🧹 Nitpick comments (1)
framework/configstore/mysql.go (1)
13-20: Rename SSLMode to TLSMode for MySQL clarity.MySQL uses TLS configuration, not SSL mode. The field name
SSLModeis misleading and inconsistent with MySQL'stlsparameter. Consider renaming toTLSModewithjson:"tls_mode"tag for accuracy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/postgres.go(2 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/configstore/store.go
🧰 Additional context used
🧬 Code graph analysis (3)
framework/configstore/config.go (1)
framework/configstore/mysql.go (1)
MySQLConfig(13-20)
framework/configstore/postgres.go (2)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)
framework/configstore/mysql.go (4)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/config.go (1)
Config(19-23)framework/configstore/rdb.go (1)
RDBConfigStore(16-19)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
framework/go.mod (1)
98-98: LGTM: MySQL GORM driver version is current.The
gorm.io/driver/mysql v1.6.0is the latest available version for the GORM MySQL driver.framework/configstore/config.go (2)
15-15: LGTM: MySQL constant added correctly.The new
ConfigStoreTypeMySQLconstant follows the established pattern for config store types.
62-67: LGTM: MySQL unmarshalling implemented correctly.The MySQL case in
UnmarshalJSONcorrectly follows the same pattern as SQLite and Postgres, properly unmarshalling intoMySQLConfigand handling errors consistently.framework/configstore/mysql.go (1)
28-33: LGTM: Store initialization and migration logic is correct.The RDBConfigStore initialization and migration triggering follow the established pattern used in PostgreSQL and SQLite implementations.
framework/configstore/postgres.go
Outdated
| Username string `json:"username"` | ||
| Password string `json:"password"` | ||
| Host string `json:"host"` | ||
| Port int `json:"port"` | ||
| DBName string `json:"db_name"` | ||
| SSLMode string `json:"ssl_mode"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Breaking change: Verify migration path for existing configs.
The PostgresConfig structure changed from a single DSN field to individual connection parameters. Ensure existing PostgreSQL configurations are migrated before deployment.
🏁 Script executed:
#!/bin/bash
# Locate the file containing PostgresConfig
rg -l "type PostgresConfig" -t go
# Show the definition of PostgresConfig
rg -n "type PostgresConfig" -C3 -t go
# Inspect DSN construction via fmt.Sprintf in that file
rg -n "fmt.Sprintf" -C3 -t go $(rg -l "type PostgresConfig" -t go)Length of output: 950
Escape DSN values and document migration path
- The new
PostgresConfigstruct removed the originalDSNfield (lines 14–19). Add documentation or a migration helper so existing configs aren’t broken on upgrade. - Building the DSN with
fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d sslmode=%s",…)(lines 24–25) doesn’t quote or escape special characters in credentials—wrap values in quotes or switch tourl.URL/driver-provided escaping to prevent parse errors or injection.
🤖 Prompt for AI Agents
In framework/configstore/postgres.go around lines 14 to 19, the new
PostgresConfig removed the original DSN field and builds a DSN with unescaped
values; add a migration note and helper to preserve old configs by accepting an
optional DSN (or populate DSN from existing fields if present) and document the
upgrade path, and update DSN construction to properly escape/quote credential
and identifier values — either build a URL via net/url (use url.UserPassword for
user/pass and set Host/Path/RawQuery) or ensure you wrap each value in quotes
and escape any internal quotes (e.g., replace ' with ''), so special characters
in username/password/dbname/host are handled safely and injection/parse errors
are avoided.
82eb715 to
66b4fba
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
b463534 to
caede8b
Compare
akshaydeo
force-pushed
the
10-01-posgres_support
branch
from
66b4fba to
75e8335
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
caede8b to
09f6b33
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
framework/configstore/mysql.go (4)
23-31: Validate required fields before opening the connectionAdd checks for username, host, db name, and port range to fail fast with clear errors.
func newMySQLConfigStore(ctx context.Context, config *MySQLConfig, logger schemas.Logger) (ConfigStore, error) { + if config.Username == "" { + return nil, fmt.Errorf("mysql username is required") + } + if config.Host == "" { + return nil, fmt.Errorf("mysql host is required") + } + if config.DBName == "" { + return nil, fmt.Errorf("mysql database name is required") + } + if config.Port <= 0 || config.Port > 65535 { + return nil, fmt.Errorf("mysql port must be between 1 and 65535, got %d", config.Port) + }
25-31: Avoid credential leakage and wrap errorsConstructing a raw DSN can leak secrets if logged. Prefer driver config and wrap errors.
Option A (wrap error only):
db, err := gorm.Open(mysql.Open(dsn), &gorm.Config{}) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to connect to MySQL database: %w", err) }Option B (recommended): use go-sql-driver/mysql config to avoid string DSN:
+ // import alias: mysqlDriver "github.com/go-sql-driver/mysql" + cfg := mysqlDriver.NewConfig() + cfg.User = config.Username + cfg.Passwd = config.Password + cfg.Net = "tcp" + cfg.Addr = fmt.Sprintf("%s:%d", config.Host, config.Port) + cfg.DBName = config.DBName + cfg.Params = map[string]string{"charset": "utf8mb4"} + cfg.ParseTime = true + // TLS mapping + switch config.SSLMode { + case "", "disable": + // no TLS + case "require": + cfg.TLSConfig = "true" + case "skip-verify": + cfg.TLSConfig = "skip-verify" + default: + cfg.TLSConfig = config.SSLMode // assume pre-registered custom config name + } + db, err := gorm.Open(mysql.Open(cfg.FormatDSN()), &gorm.Config{}) + if err != nil { + return nil, fmt.Errorf("failed to connect to MySQL database: %w", err) + }
12-20: Optional: RenameSSLModetoTLSModefor accuracyConsider
TLSMode stringjson:"ssl_mode"` to keep JSON stable but clarify code semantics.
24-31: Critical: MySQL DSN uses PostgressslmodeandparseTime=True
- MySQL driver ignores
sslmode; usetls=<value>.- Boolean must be
parseTime=true(lowercase).Apply this minimal fix:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) + dsn := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=true&loc=Local", + config.Username, config.Password, config.Host, config.Port, config.DBName) + if config.SSLMode != "" && config.SSLMode != "disable" { + switch config.SSLMode { + case "require": + dsn += "&tls=true" + case "skip-verify": + dsn += "&tls=skip-verify" + default: + dsn += "&tls=" + config.SSLMode // assume a registered custom TLS config name + } + } + db, err := gorm.Open(mysql.Open(dsn), &gorm.Config{})framework/go.mod (1)
19-21: Verify MySQL driver versions and advisories; consider bumpPlease confirm these are up to date and free of CVEs; bump if needed.
#!/bin/bash set -euo pipefail echo "Current in go.mod:" rg -nP 'gorm\.io/driver/mysql|github\.com/go-sql-driver/mysql' -n -N -C0 framework/go.mod echo -e "\nLatest versions from proxy.golang.org:" echo -n "gorm.io/driver/mysql latest: " curl -s https://proxy.golang.org/gorm.io/driver/mysql/@latest | jq -r '.Version' echo -n "github.com/go-sql-driver/mysql latest: " curl -s https://proxy.golang.org/github.com/go-sql-driver/mysql/@latest | jq -r '.Version' echo -e "\nGitHub advisories for go-sql-driver/mysql:" gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: GO, package: "github.com/go-sql-driver/mysql") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Also applies to: 98-99
framework/configstore/postgres.go (1)
24-26: Escape DSN properly; avoid raw fmt.Sprintf with credentialsUnescaped values (spaces, quotes) will break libpq parsing; build a URL DSN.
- dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d sslmode=%s", - config.Host, config.Username, config.Password, config.DBName, config.Port, config.SSLMode) - db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) + u := &url.URL{Scheme: "postgres", Path: config.DBName} + if config.Username != "" { + if config.Password != "" { + u.User = url.UserPassword(config.Username, config.Password) + } else { + u.User = url.User(config.Username) + } + } + u.Host = net.JoinHostPort(config.Host, strconv.Itoa(config.Port)) + q := u.Query() + if config.SSLMode != "" { + q.Set("sslmode", config.SSLMode) + } + u.RawQuery = q.Encode() + db, err := gorm.Open(postgres.Open(u.String()), &gorm.Config{})Add imports:
import ( "context" "fmt" "net" "net/url" "strconv" "github.com/maximhq/bifrost/core/schemas" "gorm.io/driver/postgres" "gorm.io/gorm" )
🧹 Nitpick comments (3)
framework/configstore/mysql.go (1)
28-34: Optional: Set connection pool and verify connectivityAfter open, set sane pools and ping.
d := &RDBConfigStore{db: db, logger: logger} // Run migrations + sqlDB, derr := db.DB() + if derr == nil { + sqlDB.SetMaxOpenConns(20) + sqlDB.SetMaxIdleConns(10) + sqlDB.SetConnMaxLifetime(0) + } + // Ensure connectivity early + if derr == nil { + if pingErr := sqlDB.PingContext(ctx); pingErr != nil { + return nil, fmt.Errorf("mysql ping failed: %w", pingErr) + } + } if err := triggerMigrations(ctx, db); err != nil { return nil, err }framework/configstore/postgres.go (2)
27-29: Wrap connection error (don’t leak DSN)db, err := gorm.Open(postgres.Open(u.String()), &gorm.Config{}) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to connect to Postgres database: %w", err) }
12-20: Breaking change: document migration path from DSN to fieldsPlease add upgrade notes and (ideally) a temporary backward-compatible path to accept old DSN configs, or a migration helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/postgres.go(2 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/configstore/config.go
🧰 Additional context used
🧬 Code graph analysis (3)
framework/configstore/store.go (2)
framework/configstore/config.go (2)
ConfigStoreTypeMySQL(15-15)Config(19-23)framework/configstore/mysql.go (1)
MySQLConfig(13-20)
framework/configstore/mysql.go (4)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/config.go (1)
Config(19-23)framework/configstore/rdb.go (1)
RDBConfigStore(16-19)
framework/configstore/postgres.go (2)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
framework/configstore/store.go (1)
126-130: MySQL wiring in factory looks goodType switch and constructor call mirror existing patterns.
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
09f6b33 to
3ac98ad
Compare
akshaydeo
force-pushed
the
10-01-posgres_support
branch
2 times, most recently
from
81df39c to
534b720
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
3ac98ad to
4621f1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
framework/go.mod (1)
19-20: Previous review comment remains valid.The past review correctly identified that
github.com/go-sql-driver/mysqlshould be updated from v1.8.1 to v1.9.3. This issue has already been flagged and requires action.framework/configstore/mysql.go (3)
13-20: Previous review comment on SSLMode naming remains valid.The past review correctly identified that MySQL uses
tlsparameter, notsslmode, and recommended renaming theSSLModefield toTLSModefor clarity. This naming issue has already been flagged.
24-24: Previous review comments on DSN construction remain valid.Past reviews correctly identified multiple critical issues with line 24:
- MySQL driver does not recognize
sslmodeparameter—should usetls=instead- Embedding credentials directly in DSN string risks exposure in logs
- No input validation before connection attempt
These issues have been thoroughly documented in previous reviews.
23-27: Previous review comment on input validation remains valid.The past review correctly identified that the constructor lacks validation for required fields (username, host, dbname) and port range, which would result in obscure connection errors rather than clear validation messages. This has already been flagged.
framework/configstore/postgres.go (2)
14-19: Previous review comment on breaking change remains valid.The past review correctly identified that removing the original
DSNfield in favor of individual connection parameters is a breaking change requiring a documented migration path for existing configurations. This has already been flagged.
24-26: Previous review comment on DSN escaping remains valid.The past review correctly identified that building the DSN with
fmt.Sprintfwithout escaping special characters in credentials, database names, or other parameters will cause parse errors or injection vulnerabilities. The recommendation to useurl.URL/url.UserPasswordor proper quoting/escaping has already been provided.
🧹 Nitpick comments (1)
framework/configstore/postgres.go (1)
23-36: Consider extracting shared logic to reduce duplication.The MySQL and PostgreSQL config store constructors follow nearly identical patterns (config struct, DSN construction, GORM open, wrapper, migrations). Extracting shared validation, error handling, and post-connection logic into helper functions would reduce duplication and improve maintainability.
Example approach:
// Shared validation helper func validateRDBConfig(username, host, dbname string, port int) error { if username == "" { return fmt.Errorf("username is required") } if host == "" { return fmt.Errorf("host is required") } if dbname == "" { return fmt.Errorf("database name is required") } if port <= 0 || port > 65535 { return fmt.Errorf("port must be between 1 and 65535, got %d", port) } return nil } // Shared post-connection setup func setupRDBStore(ctx context.Context, db *gorm.DB, logger schemas.Logger) (ConfigStore, error) { d := &RDBConfigStore{db: db, logger: logger} if err := triggerMigrations(ctx, db); err != nil { return nil, err } return d, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/postgres.go(2 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/configstore/config.go
- framework/configstore/store.go
🧰 Additional context used
🧬 Code graph analysis (2)
framework/configstore/postgres.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/config.go (1)
Config(19-23)
framework/configstore/mysql.go (4)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-104)framework/configstore/config.go (1)
Config(19-23)framework/configstore/rdb.go (1)
RDBConfigStore(16-19)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
feddb47 to
6110981
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
195b28d to
98a001b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
framework/configstore/mysql.go (4)
12-20: RenameSSLModetoTLSModefor MySQL clarity.The field name
SSLModeat line 19 is PostgreSQL-specific terminology. MySQL uses TLS configuration, not SSL mode. This naming inconsistency can confuse developers.Apply this diff to rename the field:
type MySQLConfig struct { User string `json:"user"` Password string `json:"password"` Host string `json:"host"` Port int `json:"port"` DBName string `json:"db_name"` - SSLMode string `json:"ssl_mode"` + TLSMode string `json:"tls_mode"` }Note: After renaming this struct field, you must also update the JSON schema in
transports/config.schema.jsonto usetls_modeinstead ofssl_mode, and update the DSN construction in line 24 to referenceconfig.TLSMode.
23-27: Add connection parameter validation.The constructor lacks validation for required fields and port range. Invalid configurations will fail at connection time with obscure GORM/driver errors rather than clear validation messages.
Add validation before attempting connection:
func newMySQLConfigStore(ctx context.Context, config *MySQLConfig, logger schemas.Logger) (ConfigStore, error) { + // Validate required fields + if config.User == "" { + return nil, fmt.Errorf("mysql user is required") + } + if config.Host == "" { + return nil, fmt.Errorf("mysql host is required") + } + if config.DBName == "" { + return nil, fmt.Errorf("mysql database name is required") + } + if config.Port <= 0 || config.Port > 65535 { + return nil, fmt.Errorf("mysql port must be between 1 and 65535, got %d", config.Port) + } + db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.User, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{})
24-24: Critical: MySQL driver does not supportsslmodeparameter.The DSN uses
sslmode=%s, which is PostgreSQL-specific. The MySQL driver (go-sql-driver/mysql) expectstls=<value>for TLS connections (e.g.,tls=true,tls=skip-verify, or a custom TLS config name). Usingsslmodewill cause the parameter to be silently ignored, creating a security risk where SSL/TLS may not be enabled as expected.Apply this diff to use the correct MySQL TLS parameter:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.User, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) + db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&tls=%s", config.User, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{})See go-sql-driver/mysql TLS documentation: https://github.com/go-sql-driver/mysql#tls
24-27: Security: DSN with embedded credentials may be logged.Constructing DSNs inline can expose credentials in logs if GORM or the driver logs the connection string during errors or debugging.
Consider using the
mysql.Configbuilder pattern to avoid embedding credentials in a single string:- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.User, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) + mysqlCfg := mysql.Config{ + User: config.User, + Passwd: config.Password, + Net: "tcp", + Addr: fmt.Sprintf("%s:%d", config.Host, config.Port), + DBName: config.DBName, + ParseTime: true, + Params: map[string]string{"charset": "utf8mb4"}, + // Add TLS config based on config.SSLMode + } + db, err := gorm.Open(mysql.New(mysql.Config{ + DSNConfig: &mysqlCfg, + }), &gorm.Config{}) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to connect to MySQL database: %w", err) }Based on learnings.
framework/go.mod (1)
19-20: Update MySQL driver to latest version.The MySQL driver version v1.8.1 is outdated. The past review identified v1.9.3 as the latest version and requested a security advisory check.
transports/config.schema.json (1)
254-300: Port type inconsistency across schemas.Line 270 defines
portas type"string"in theconfig_storeMySQL schema, but line 408 defines it as type"integer"in thelogs_storeMySQL schema. Port numbers should consistently be integers across all database configurations.Additionally, line 279's password description states "Leave empty if you want to use IAM role authentication" — IAM authentication is AWS RDS-specific and may not apply to generic MySQL deployments. Either clarify this is only for AWS RDS MySQL or remove the IAM reference.
Apply this diff to fix the port type:
"port": { - "type": "string", + "type": "integer", "description": "Database port" },Also update the password description to clarify scope:
"password": { "type": "string", - "description": "Database password. Leave empty if you want to use IAM role authentication." + "description": "Database password. For AWS RDS IAM authentication, leave empty." },
🧹 Nitpick comments (1)
transports/config.schema.json (1)
285-287: Add enum constraints forssl_mode.The
ssl_modefield lacks validation constraints. Consider adding an enum to restrict valid values and prevent configuration errors.Apply this diff to add enum constraints:
"ssl_mode": { "type": "string", + "enum": ["disable", "true", "false", "skip-verify", "preferred"], "description": "Database SSL mode" }Note: MySQL's go-sql-driver uses
tlsparameter values like"true","false","skip-verify", or custom config names. Coordinate this enum with the actual TLS parameter values after fixing the criticalsslmodevstlsissue inframework/configstore/mysql.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)transports/config.schema.json(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/configstore/store.go
- framework/configstore/config.go
🧰 Additional context used
🧬 Code graph analysis (1)
framework/configstore/mysql.go (4)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-111)framework/configstore/config.go (1)
Config(19-23)framework/configstore/rdb.go (1)
RDBConfigStore(16-19)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
transports/config.schema.json (1)
178-179: LGTM: MySQL added to store type enums.The addition of
"mysql"to bothconfig_store.typeandlogs_store.typeenums correctly extends the supported database backends.Also applies to: 318-319
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
98a001b to
8d14ffa
Compare
akshaydeo
changed the base branch from
09-15-seggreate_http_server_into_a_separate_struct
to
graphite-base/536
akshaydeo
force-pushed
the
graphite-base/536
branch
from
cec64bb to
6f55199
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
8d14ffa to
11249bf
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
11249bf to
6c7eaaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
framework/configstore/mysql.go (2)
12-20: Critical:SSLModefield name and usage don't match MySQL driver requirements.The struct field
SSLModeand JSON tagssl_modeare misleading because the MySQL driver (go-sql-driver/mysql) uses thetlsparameter, notsslmode. This inconsistency between the configuration field naming and the actual driver parameter can cause confusion and connection failures.This issue was already flagged in previous reviews. The
sslmodeparameter is PostgreSQL-specific and not recognized by the MySQL driver. As noted in past reviews, you should:
- Rename the field from
SSLModetoTLSModeand update the JSON tag totls_mode- Update the DSN construction in line 24 to use
tls=%sinstead ofsslmode=%s- Ensure valid TLS mode values are used (e.g., "true", "false", "skip-verify", or custom config names)
See the past review comments for detailed implementation suggestions.
22-34: Multiple critical and major issues remain unaddressed.This constructor has three significant issues that were flagged in previous reviews but remain unresolved:
Critical: Incorrect SSL parameter (line 24) - Uses
sslmode=%swhich MySQL driver doesn't recognize. Should usetls=%sparameter instead.Major: Security risk (line 24) - Inline DSN construction with embedded credentials can leak passwords in logs if GORM or the driver logs connection strings. Consider using
mysql.Configstruct builder instead of string formatting.Major: Missing input validation (lines 23-27) - No validation of required fields (User, Host, DBName) or port range before attempting connection. Invalid configurations will fail with obscure driver errors rather than clear validation messages.
All three issues have been detailed with specific code suggestions in previous review comments. Please refer to those comments for implementation guidance, including:
- How to map SSL/TLS modes correctly for MySQL
- Using
mysql.Configstruct to avoid credential leakage- Adding pre-connection validation with descriptive error messages
transports/config.schema.json (1)
254-300: Port type inconsistency and misleading field name for MySQL.The MySQL configuration schema for
config_storehas several issues:
Port type inconsistency (line 270) - Defines
portas type"string", but thelogs_storeMySQL schema at line 408 correctly defines it as type"integer". Port numbers should consistently be integers across all database configurations.IAM authentication scope unclear (line 279) - The password description states "Leave empty if you want to use IAM role authentication." This is AWS RDS-specific and may confuse users deploying to non-AWS MySQL instances. Consider clarifying this is for AWS RDS only or removing if not universally applicable.
ssl_modefield naming mismatch (lines 285-287) - The field is namedssl_mode, but the MySQL driver (go-sql-driver/mysql) actually uses thetlsparameter, notsslmode. This creates a disconnect between the configuration schema and the driver implementation. Consider renaming totls_modeand adding enum constraints for valid values like"true","false","skip-verify", or custom config names.The port type and IAM authentication concerns were flagged in previous reviews. The field naming issue (ssl_mode vs tls) is consistent with the same problem in the Go code (framework/configstore/mysql.go).
Apply this diff to fix the port type:
"port": { - "type": "string", + "type": "integer", "description": "Database port" },
🧹 Nitpick comments (1)
transports/config.schema.json (1)
392-437: IAM authentication note andssl_modefield naming concerns.The MySQL configuration schema for
logs_storehas two concerns:
IAM authentication scope unclear (line 417) - Same issue as in
config_store: the password description mentions IAM role authentication, which is AWS RDS-specific and may confuse users with non-AWS MySQL deployments.
ssl_modefield naming mismatch (lines 423-425) - The field is namedssl_mode, but this doesn't align with the MySQL driver'stlsparameter. Consider renaming totls_modefor consistency with the actual driver requirements and adding enum constraints for valid TLS mode values.While the port type is correctly defined as
"integer"here (line 408), ensure this naming and validation consistency is maintained across both the schema and the Go implementation in framework/configstore/mysql.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)transports/config.schema.json(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/configstore/config.go
- framework/configstore/store.go
- framework/go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
framework/configstore/mysql.go (4)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-111)framework/configstore/config.go (1)
Config(19-23)framework/configstore/rdb.go (1)
RDBConfigStore(16-19)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
6c7eaaf to
476c15d
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
476c15d to
e3deede
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
e3deede to
6c2a4d9
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
6c2a4d9 to
a80c464
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
a80c464 to
fb44a13
Compare
akshaydeo
force-pushed
the
v1.3.0
branch
2 times, most recently
from
959fc17 to
341c855
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
fb44a13 to
0e73c42
Compare
akshaydeo
force-pushed
the
10-01-mysql_support
branch
from
0e73c42 to
17c4d8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
framework/configstore/mysql.go (3)
24-24: CRITICAL: MySQL driver does not supportsslmodeparameter.The DSN uses
sslmode=%s, but MySQL's driver expectstls=<value>(e.g.,tls=true,tls=skip-verify), notsslmode. This will cause SSL connections to fail or be silently ignored.Apply this diff to use the correct MySQL TLS parameter:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.User, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) + dsn := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local", + config.User, config.Password, config.Host, config.Port, config.DBName) + if config.SSLMode != "" && config.SSLMode != "disable" { + if config.SSLMode == "require" { + dsn += "&tls=true" + } else { + dsn += "&tls=" + config.SSLMode + } + } + db, err := gorm.Open(mysql.Open(dsn), &gorm.Config{})Additionally, consider renaming the
SSLModefield toTLSModein the struct for clarity.
24-24: Security: Credentials in DSN may leak in logs.Building the DSN inline embeds credentials in a single string that may be logged by GORM or the driver, exposing sensitive information.
Prefer using
mysql.Configstruct to avoid credential leakage:- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.User, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) + mysqlConfig := mysql.Config{ + User: config.User, + Passwd: config.Password, + Net: "tcp", + Addr: fmt.Sprintf("%s:%d", config.Host, config.Port), + DBName: config.DBName, + Params: map[string]string{ + "charset": "utf8mb4", + "parseTime": "True", + "loc": "Local", + }, + } + // Handle TLS configuration based on SSLMode + if config.SSLMode != "" && config.SSLMode != "disable" { + if config.SSLMode == "require" { + mysqlConfig.Params["tls"] = "true" + } else { + mysqlConfig.Params["tls"] = config.SSLMode + } + } + db, err := gorm.Open(mysql.New(mysql.Config{ + DSNConfig: &mysqlConfig, + }), &gorm.Config{})Based on learnings.
23-27: Validate connection parameters before attempting connection.Missing validation for required fields (username, host, db_name) and port range will result in obscure connection errors rather than clear validation messages.
Add validation before gorm.Open:
func newMySQLConfigStore(ctx context.Context, config *MySQLConfig, logger schemas.Logger) (ConfigStore, error) { + if config.User == "" { + return nil, fmt.Errorf("mysql username is required") + } + if config.Host == "" { + return nil, fmt.Errorf("mysql host is required") + } + if config.DBName == "" { + return nil, fmt.Errorf("mysql database name is required") + } + if config.Port <= 0 || config.Port > 65535 { + return nil, fmt.Errorf("mysql port must be between 1 and 65535, got %d", config.Port) + } + db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.User, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{})
🧹 Nitpick comments (4)
framework/configstore/mysql.go (4)
14-14: Consider renamingUsertoUsernamefor consistency.The field is named
User, but PostgreSQL configuration likely usesUsername. Maintaining consistent field names across database configurations improves code clarity.Apply this diff to align field naming:
- User string `json:"user"` + Username string `json:"username"`Then update the DSN construction on line 24 accordingly:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.User, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) + db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.Username, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{})
24-24: Pass context to GORM for proper timeout and cancellation handling.The context parameter is received but not used when opening the database connection. This prevents proper timeout and cancellation propagation.
Apply this diff to use context-aware GORM operations:
- db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.User, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{}) + db, err := gorm.Open(mysql.Open(fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8mb4&parseTime=True&loc=Local&sslmode=%s", config.User, config.Password, config.Host, config.Port, config.DBName, config.SSLMode)), &gorm.Config{ + PrepareStmt: true, + }) if err != nil { return nil, err } + // Set the context for the connection + db = db.WithContext(ctx)Based on learnings.
24-24: Consider timezone implications ofloc=Local.Using
loc=Localin the DSN may cause inconsistent datetime handling across different server environments. Consider usingloc=UTCfor consistency or making the timezone configurable.
25-27: Wrap connection error with context.The error returned on connection failure doesn't provide context about what operation failed, making debugging harder.
Apply this diff:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to open MySQL connection: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
framework/configstore/config.go(2 hunks)framework/configstore/mysql.go(1 hunks)framework/configstore/store.go(1 hunks)framework/go.mod(2 hunks)transports/config.schema.json(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/configstore/store.go
- framework/configstore/config.go
- framework/go.mod
- transports/config.schema.json
🧰 Additional context used
🧬 Code graph analysis (1)
framework/configstore/mysql.go (3)
core/schemas/logger.go (1)
Logger(28-55)framework/configstore/store.go (1)
ConfigStore(15-111)framework/configstore/config.go (1)
Config(19-23)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
1 participant
Summary
Add MySQL support to the config store, allowing users to use MySQL as a database backend alongside the existing SQLite and PostgreSQL options.
Changes
ConfigStoreTypeMySQLconstant to support MySQL as a config store typemysql.gofile with MySQL-specific configuration and connection logicType of change
Affected areas
How to test
Test MySQL configuration with the following example:
Breaking changes
The PostgreSQL configuration format has changed from using a single DSN string to individual connection parameters. Users with existing PostgreSQL configurations will need to update their configuration.
Security considerations
The MySQL configuration includes credentials that should be properly secured. SSL mode can be configured for secure connections.
Checklist