diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 85e6cbd3f..acb580f86 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -486,6 +486,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). @@ -517,8 +518,6 @@ func (s *SQL) Create(key string, rel release.Releaser) error { } if _, err := transaction.Exec(insertQuery, args...); err != nil { - defer transaction.Rollback() - selectQuery, args, buildErr := s.statementBuilder. Select(sqlReleaseTableKeyColumn). From(sqlReleaseTableName). @@ -558,18 +557,20 @@ func (s *SQL) Create(key string, rel release.Releaser) error { ).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() 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 SQL transaction", slog.Any("error", err)) + return fmt.Errorf("error committing transaction: %w", err) + } return nil } @@ -624,6 +625,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). @@ -646,10 +648,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). @@ -686,8 +686,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.Exec(deleteCustomLabelsQuery, args...); err != nil { + s.Logger().Debug("failed to perform delete Labels query", slog.Any("error", err)) + return release, err + } + + if err := transaction.Commit(); err != nil { + s.Logger().Debug("failed to commit SQL 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 adcc780aa..6283ab506 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -288,6 +288,59 @@ func TestSqlCreateAlreadyExists(t *testing.T) { assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") } +func TestSqlCreateCommitFailure(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("commit failed")) + + err := sqlDriver.Create(key, rel) + require.ErrorContains(t, err, "commit failed") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + func TestSqlUpdate(t *testing.T) { vers := 1 name := "smug-pigeon" @@ -498,6 +551,132 @@ func TestSqlDelete(t *testing.T) { assert.Truef(t, reflect.DeepEqual(rel, deletedRelease), "Expected release {%v}, got {%v}", rel, deletedRelease) } +func TestSqlDeleteCommitFailure(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("commit failed")) + + _, err := sqlDriver.Delete(key) + require.ErrorContains(t, err, "commit failed") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + +func TestSqlDeleteLabelsFailureRollsBack(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). + WillReturnError(errors.New("labels delete failed")) + + mock.ExpectRollback() + + _, err := sqlDriver.Delete(key) + require.ErrorContains(t, err, "labels delete failed") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + func mockGetReleaseCustomLabels(mock sqlmock.Sqlmock, key string, namespace string, labels map[string]string) { query := fmt.Sprintf( regexp.QuoteMeta("SELECT %s, %s FROM %s WHERE %s = $1 AND %s = $2"),