diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 2b278f7cb..e07c82c0b 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -17,6 +17,7 @@ limitations under the License. package driver import ( + "context" "fmt" "log/slog" "maps" @@ -488,6 +489,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { s.Logger().Debug("failed to start SQL transaction", slog.Any("error", err)) return fmt.Errorf("error beginning transaction: %w", err) } + defer transaction.Rollback() insertQuery, args, err := s.statementBuilder. Insert(sqlReleaseTableName). @@ -518,9 +520,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { return err } - if _, err := transaction.Exec(insertQuery, args...); err != nil { - defer transaction.Rollback() - + if _, err := transaction.ExecContext(context.Background(), insertQuery, args...); err != nil { selectQuery, args, buildErr := s.statementBuilder. Select(sqlReleaseTableKeyColumn). From(sqlReleaseTableName). @@ -559,18 +559,20 @@ func (s *SQL) Create(key string, rel release.Releaser) error { v, ).ToSql() if err != nil { - defer transaction.Rollback() s.Logger().Debug("failed to build insert query", slog.Any("error", err)) return err } - if _, err := transaction.Exec(insertLabelsQuery, args...); err != nil { - defer transaction.Rollback() + if _, err := transaction.ExecContext(context.Background(), insertLabelsQuery, args...); err != nil { s.Logger().Debug("failed to write Labels", slog.Any("error", err)) return err } } - defer transaction.Commit() + + if err := transaction.Commit(); err != nil { + s.Logger().Debug("failed to commit release creation transaction", slog.Any("error", err)) + return fmt.Errorf("error committing transaction: %w", err) + } return nil } @@ -623,7 +625,7 @@ func (s *SQL) Update(key string, rel release.Releaser) error { return err } - if _, err := s.db.Exec(query, args...); err != nil { + if _, err := s.db.ExecContext(context.Background(), query, args...); err != nil { s.Logger().Debug("failed to update release in SQL database", slog.String("key", key), slog.Any("error", err)) return err } @@ -638,6 +640,7 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { s.Logger().Debug("failed to start SQL transaction", slog.Any("error", err)) return nil, fmt.Errorf("error beginning transaction: %w", err) } + defer transaction.Rollback() selectQuery, args, err := s.statementBuilder. Select(sqlReleaseTableBodyColumn). @@ -660,10 +663,8 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { release, err := decodeRelease(record.Body) if err != nil { s.Logger().Debug("failed to decode release", slog.String("key", key), slog.Any("error", err)) - transaction.Rollback() return nil, err } - defer transaction.Commit() deleteQuery, args, err := s.statementBuilder. Delete(sqlReleaseTableName). @@ -675,7 +676,7 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { return nil, err } - _, err = transaction.Exec(deleteQuery, args...) + _, err = transaction.ExecContext(context.Background(), deleteQuery, args...) if err != nil { s.Logger().Debug("failed perform delete query", slog.Any("error", err)) return release, err @@ -699,8 +700,17 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { s.Logger().Debug("failed to build delete Labels query", slog.Any("error", err)) return nil, err } - _, err = transaction.Exec(deleteCustomLabelsQuery, args...) - return release, err + if _, err = transaction.ExecContext(context.Background(), deleteCustomLabelsQuery, args...); err != nil { + s.Logger().Debug("failed to delete release custom labels", slog.String("key", key), slog.Any("error", err)) + return release, err + } + + if err := transaction.Commit(); err != nil { + s.Logger().Debug("failed to commit release deletion transaction", slog.Any("error", err)) + return release, fmt.Errorf("error committing transaction: %w", err) + } + + return release, nil } // Get release custom labels from database diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index e5fde405b..3fb893a9d 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -14,6 +14,7 @@ limitations under the License. package driver import ( + "database/sql" "database/sql/driver" "errors" "fmt" @@ -561,3 +562,158 @@ func TestSqlCheckAppliedMigrations(t *testing.T) { assert.Equal(t, c.expectedResult, sqlDriver.checkAlreadyApplied(c.migrationsToApply), "Test case: %v, Expected: %v, Have: %v, Explanation: %v", i, c.expectedResult, !c.expectedResult, c.errorExplanation) } } + +// TestSqlCreateCommitError verifies that a commit-time failure is surfaced to +// the caller. A deferred, error-discarding Commit would report success even +// though nothing was persisted. +func TestSqlCreateCommitError(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + rel := releaseStub(name, vers, namespace, common.StatusDeployed) + + sqlDriver, mock := newTestFixtureSQL(t) + body, _ := encodeRelease(rel) + + query := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s,%s,%s,%s,%s,%s) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9)", + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableTypeColumn, + sqlReleaseTableBodyColumn, + sqlReleaseTableNameColumn, + sqlReleaseTableNamespaceColumn, + sqlReleaseTableVersionColumn, + sqlReleaseTableStatusColumn, + sqlReleaseTableOwnerColumn, + sqlReleaseTableCreatedAtColumn, + ) + + mock.ExpectBegin() + mock. + ExpectExec(regexp.QuoteMeta(query)). + WithArgs( + key, + sqlReleaseDefaultType, + body, + rel.Name, + rel.Namespace, + int(rel.Version), + rel.Info.Status.String(), + sqlReleaseDefaultOwner, + recentUnixTimestamp(), + ). + WillReturnResult(sqlmock.NewResult(1, 1)) + + labelsQuery := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s) VALUES ($1,$2,$3,$4)", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + sqlCustomLabelsTableKeyColumn, + sqlCustomLabelsTableValueColumn, + ) + + mock.MatchExpectationsInOrder(false) + for k, v := range filterSystemLabels(rel.Labels) { + mock. + ExpectExec(regexp.QuoteMeta(labelsQuery)). + WithArgs(key, rel.Namespace, k, v). + WillReturnResult(sqlmock.NewResult(1, 1)) + } + mock.ExpectCommit().WillReturnError(errors.New("transaction commit failed")) + + err := sqlDriver.Create(key, rel) + require.Error(t, err, "expected Create to surface the commit error, got nil") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + +// TestSqlDeleteCommitError verifies that a commit-time failure during Delete is +// surfaced to the caller instead of being silently discarded. +func TestSqlDeleteCommitError(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + rel := releaseStub(name, vers, namespace, common.StatusDeployed) + body, _ := encodeRelease(rel) + + sqlDriver, mock := newTestFixtureSQL(t) + + selectQuery := fmt.Sprintf( + "SELECT %s FROM %s WHERE %s = $1 AND %s = $2", + sqlReleaseTableBodyColumn, + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + mock.ExpectBegin() + mock. + ExpectQuery(regexp.QuoteMeta(selectQuery)). + WithArgs(key, namespace). + WillReturnRows( + mock.NewRows([]string{sqlReleaseTableBodyColumn}).AddRow(body), + ).RowsWillBeClosed() + + deleteQuery := fmt.Sprintf( + "DELETE FROM %s WHERE %s = $1 AND %s = $2", + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + mock. + ExpectExec(regexp.QuoteMeta(deleteQuery)). + WithArgs(key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + + mockGetReleaseCustomLabels(mock, key, namespace, rel.Labels) + + deleteLabelsQuery := fmt.Sprintf( + "DELETE FROM %s WHERE %s = $1 AND %s = $2", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + ) + mock. + ExpectExec(regexp.QuoteMeta(deleteLabelsQuery)). + WithArgs(key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + + mock.ExpectCommit().WillReturnError(errors.New("transaction commit failed")) + + _, err := sqlDriver.Delete(key) + require.Error(t, err, "expected Delete to surface the commit error, got nil") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + +// TestSqlDeleteNotFoundReleasesTransaction verifies that the not-found path +// rolls back the transaction rather than leaking its pooled connection. +func TestSqlDeleteNotFoundReleasesTransaction(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + + sqlDriver, mock := newTestFixtureSQL(t) + + selectQuery := fmt.Sprintf( + "SELECT %s FROM %s WHERE %s = $1 AND %s = $2", + sqlReleaseTableBodyColumn, + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + mock.ExpectBegin() + mock. + ExpectQuery(regexp.QuoteMeta(selectQuery)). + WithArgs(key, namespace). + WillReturnError(sql.ErrNoRows) + mock.ExpectRollback() + + _, err := sqlDriver.Delete(key) + require.ErrorIs(t, err, ErrReleaseNotFound) + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +}