refactor: extract pkg/mysql from pkg/db by chronark · Pull Request #5287 · unkeyed/unkey
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/mysql/database.go`:
- Around line 150-166: The Close() method currently may call Close() twice on
the same *sql.DB when readReplica.db and writeReplica.db are the same; update
database.Close to detect shared connections and only close each underlying
*sql.DB once (e.g., compare d.readReplica.db == d.writeReplica.db or track a
boolean when assigning readReplica in NewDatabase) so you skip closing the same
DB object twice; reference the database.Close method and the
readReplica/writeReplica fields when making the change.
In `@pkg/mysql/handle_err_deadlock.go`:
- Around line 19-23: The CR_* numeric checks are ineffective because
go-sql-driver/mysql.MySQLError doesn't report client-side CR_* codes, so remove
the constants errServerGone and errServerLost and delete the isMySQLError(err,
errServerGone)/isMySQLError(err, errServerLost) branches inside the error
handling function (the same function that already checks mysql.ErrInvalidConn,
string-based messages, and net.Error); alternatively, if you prefer explicit
handling, replace those branches with checks that detect mysql.ErrInvalidConn,
inspect err.Error() text, or use net.Error assertions (keep isMySQLError for
real MySQLError codes only).
In `@pkg/mysql/handle_err_duplicate_key.go`:
- Around line 9-14: Replace the direct type assertion in IsDuplicateKeyError
with the package's existing errors.As-based helper to handle wrapped errors:
call isMySQLError (the helper in handle_err_deadlock.go) or use errors.As to
obtain *mysql.MySQLError and then check mysqlErr.Number == 1062; update
IsDuplicateKeyError to rely on that approach so wrapped MySQL errors are
detected consistently.
In `@pkg/mysql/replica.go`:
- Around line 174-175: The code currently appends "_tx" to the replica label by
calling WrapTxWithContext(tx, r.mode+"_tx", ctx), which changes metric labels;
change the call to pass r.mode unchanged (e.g., WrapTxWithContext(tx, r.mode,
ctx)) so the "replica" label stays stable, and if you need to distinguish begin
vs tx operations add a separate label/parameter such as "operation" (or a new
label name) rather than mutating r.mode; update any callers or metric tags that
expect the separate operation label accordingly.
In `@pkg/mysql/retry.go`:
- Around line 45-52: The current shouldRetryError is used for
TxWithResult/TxRetry and treats connection-class errors as retryable, which can
cause re-running a closure around Commit/Rollback and produce duplicate writes;
create a separate tx-specific predicate (e.g., shouldRetryTxError) and use it in
TxWithResult/TxRetry so only errors that guarantee the transaction was aborted
(e.g., serialization failures, deadlocks, explicit "transaction aborted"
semantics) are retried, while connection/disconnect errors and other
non-guaranteed-abort errors remain non-retryable; update calls in TxWithResult
and TxRetry to reference shouldRetryTxError instead of shouldRetryError and keep
shouldRetryError behavior for non-transactional retry paths.
In `@pkg/mysql/traced_tx.go`:
- Around line 38-58: The span for transaction methods (TracedTx.ExecContext,
PrepareContext, QueryContext, Rollback) is not marked as failed when an error
occurs, so traces remain "successful" even on errors; update each method to
record the error and set the span status on error (e.g., call
span.RecordError(err) and span.SetStatus(codes.Error, err.Error()) or the
equivalent in your tracing helper) in the error branch before updating
metrics/returning so the span reflects failures just like the corresponding
Replica methods.
In `@pkg/mysql/tx.go`:
- Around line 26-37: When rollback fails you currently return only the wrapped
rollbackErr and lose the original err from fn; change the error handling to
preserve both by joining or wrapping them together (e.g., use
errors.Join(rollbackErr, err) or wrap rollbackErr with fault.Wrap while
including err as the cause) and return that combined error instead of discarding
err; update the imports to include "errors" if you use errors.Join, and keep the
existing fault.Wrap call but ensure the original err is included in the returned
fault so callers can see both the transaction failure and the rollback failure
(refer to variables tx.Rollback(), rollbackErr, err and the returned t).
---
Nitpick comments:
In `@pkg/mysql/database.go`:
- Around line 140-146: The RO() method contains an unreachable nil check because
readReplica is always initialized in New() (either as a separate replica or set
to the write replica); simplify RO() by removing the conditional and returning
d.readReplica directly. Update the function implementation (database.RO) to be a
single return of d.readReplica and remove the now-redundant nil branch so the
method is clearer and shorter.
- Around line 36-39: The parseTime check in function open is case-sensitive and
will miss variants like "ParseTime=true"; update the validation to perform a
case-insensitive check on the DSN (e.g., normalize the DSN to lower-case before
searching or parse the DSN/query and check keys) so that any capitalization of
"parseTime=true" is accepted; locate the open(dsn string) function and replace
the strings.Contains(dsn, "parseTime=true") check with a case-insensitive or
proper-query-parameter check to ensure reliable detection.
- Around line 110-115: The current initialization sets readReplica.mode = "rw"
which mislabels fallback read traffic; update the Replica initialization
(variable readReplica created from writeReplica) to use a distinct read fallback
label such as "ro_fallback" (or "primary"/"ro" per preference) instead of "rw"
and ensure any metric collection that reads Replica.mode will emit that new
label so fallback reads are distinguishable from true write-capable replicas.
In `@pkg/mysql/tx.go`:
- Around line 53-57: The inline wrapper in Tx uses the built-in alias any in the
TxWithResult call; change it to use a zero-size placeholder type (e.g.,
struct{}) so you avoid any while keeping the internal void-returning behavior.
Specifically, update the anonymous inner function passed to TxWithResult in Tx
to return (struct{}, error) and return nil (or struct{}{}) from that closure,
and adapt the unpacking so Tx still returns the error; keep references to Tx and
TxWithResult unchanged.