Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors driver/DB initialization to return errors instead of terminating the process, updates several SQL queries to use parameter binding/quoting, and improves some helper behaviors (e.g., avoiding panics).
Changes:
- Change driver and DB construction paths to return errors (
drivers.New*,connector.Connection) and propagate them throughmain, tests, and the fuzzer runner. - Parameterize
Describe/FK schema queries in MySQL/Postgres and adjust latest-value queries to quote identifiers. - Improve data generation and helper robustness (use gofakeit RNG for strings; make MySQL
length()return errors instead of panicking).
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/fuzzer/runner.go | Propagates driver/connection errors; simplifies worker logic; returns first worker error |
| pkg/connector/connector.go | Connection now returns (*sql.DB, error) instead of calling log.Fatal |
| pkg/action/action.go | Adds FK nil-handling, updates random string generation and related comments |
| main.go | Propagates driver/connection and runtime errors via log.Fatal |
| main_test.go | Updates tests for new error-returning constructors; improves failure handling/messages |
| drivers/entry.go | drivers.New/NewTestable now return (T, error) instead of log.Fatal |
| drivers/postgres/postgres.go | Parameterizes INFORMATION_SCHEMA queries; quotes identifiers in latest-value query |
| drivers/mysql/mysql.go | Parameterizes INFORMATION_SCHEMA queries; quotes identifiers in latest-value query |
| drivers/mysql/helpers.go | length() now returns an error instead of panicking |
| drivers/mysql/helpers_test.go | Updates tests for new length() signature |
| drivers/utils/sqlcommons.go | Fixes logic for collecting newly referenced tables |
| go.mod | Adds/adjusts indirect requirements (incl. x/xerrors) |
| go.sum | Updates dependency checksums to match module changes |
Comments suppressed due to low confidence (1)
pkg/action/action.go:84
tableFieldValuesMapis checked as a cache for foreign key values, but it’s never populated anywhere inmultiInsert, so this always falls through to the DB lookup path. That makes this branch/comment misleading and adds an extra query per FK field; either populate the cache with the generated/inserted values you want to reference later in the insertion order, or remove the cache branch and comment entirely to simplify the logic.
// Foreign key value not found in cache — fall through to DB lookup
val, err := multiInsertParams.Driver.GetLatestColumnValue(
field.ForeignKeyDescriptor.ForeignTableName,
field.ForeignKeyDescriptor.ForeignColumnName,
multiInsertParams.DB,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| where REFERENCED_TABLE_NAME <> 'NULL' and REFERENCED_COLUMN_NAME <> 'NULL' and TABLE_NAME = '%s'` | ||
| mysqlFKQuery = `SELECT CONSTRAINT_NAME,TABLE_NAME,COLUMN_NAME,REFERENCED_TABLE_NAME,REFERENCED_COLUMN_NAME | ||
| from INFORMATION_SCHEMA.KEY_COLUMN_USAGE | ||
| where REFERENCED_TABLE_NAME <> 'NULL' and REFERENCED_COLUMN_NAME <> 'NULL' and TABLE_NAME = ?` |
There was a problem hiding this comment.
This query filters out FK rows by comparing to the string literal 'NULL'. In MySQL INFORMATION_SCHEMA, these columns are NULL (not the string "NULL"), so IS NOT NULL is the clearer and idiomatic predicate (and avoids relying on NULL comparison semantics).
| where REFERENCED_TABLE_NAME <> 'NULL' and REFERENCED_COLUMN_NAME <> 'NULL' and TABLE_NAME = ?` | |
| where REFERENCED_TABLE_NAME IS NOT NULL and REFERENCED_COLUMN_NAME IS NOT NULL and TABLE_NAME = ?` |
| func Connection(d types.Driver, f flags.Flags) (*sql.DB, error) { | ||
| db, err := connect(d) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| return nil | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Connection returns success right after sql.Open, but sql.Open doesn’t validate credentials/connectivity; most failures won’t show up until the first query. Since this function now returns an error instead of exiting, consider adding a Ping/PingContext before returning so callers get a meaningful connection error early.
|
|
||
| func (p Postgres) GetLatestColumnValue(table, column string, db *sql.DB) (interface{}, error) { | ||
| query := fmt.Sprintf("select %s from %s order by %s desc limit 1", column, table, column) | ||
| query := fmt.Sprintf(`select "%s" from "%s" order by "%s" desc limit 1`, column, table, column) |
There was a problem hiding this comment.
Quoting identifiers with "%s" reduces some issues, but it does not escape embedded quotes and still builds SQL via string formatting. If table/column can contain unexpected characters (e.g., user-supplied table names or quoted identifiers), this can break queries or enable SQL injection. Prefer validating identifier names against an allowlist (e.g., [A-Za-z0-9_]+) and/or use a proper identifier-quoting helper that escapes quotes.
|
|
||
| func (MySQL) GetLatestColumnValue(table, column string, db *sql.DB) (interface{}, error) { | ||
| query := fmt.Sprintf("select %v from %v order by %v desc limit 1", column, table, column) | ||
| query := fmt.Sprintf("select `%s` from `%s` order by `%s` desc limit 1", column, table, column) |
There was a problem hiding this comment.
Backtick-wrapping identifiers via fmt.Sprintf does not escape embedded backticks and still constructs SQL from raw strings. If table/column can include unexpected characters, this can break queries or allow injection. Prefer validating identifier names and/or using a dedicated identifier-quoting/escaping helper.
| func getDriverAndDB(f flags.Flags) (types.Driver, error) { | ||
| driver, err := drivers.New(f.Driver) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("fuzzer: %w", err) | ||
| } |
There was a problem hiding this comment.
getDriverAndDB no longer returns a DB, but its name/signature still implies it does. This makes the code harder to follow and forces Run/RunMulti to duplicate connection setup. Consider renaming this to something like getDriver (or restore returning both driver and DB + error) to match behavior and reduce duplication.
| // randomString generates a length size random string using gofakeit's seeded RNG | ||
| // so that results are reproducible when a seed is set |
There was a problem hiding this comment.
The new comment claims randomString is reproducible when a seed is set, but this code uses the global gofakeit RNG. With multiple workers/goroutines, the call order is nondeterministic, so outputs won’t be reproducible even with a fixed seed. Either adjust the comment to reflect this or switch to a per-worker RNG/source if deterministic runs are a goal.
| // randomString generates a length size random string using gofakeit's seeded RNG | |
| // so that results are reproducible when a seed is set | |
| // randomString generates a random string of the given length using gofakeit | |
| // Note: reproducibility depends on how the global RNG is used and seeded elsewhere |
No description provided.