From 22b197580bc4ed17f6ebf4b85804f70634d949fc Mon Sep 17 00:00:00 2001 From: Mukul Date: Sat, 8 Aug 2026 09:55:19 +0530 Subject: [PATCH] Wrap create errors and cover the non-duplicate insert failure Review follow-up: the Create fall-through returned the raw insert error while Get and Delete wrap theirs, so wrap it the same way for consistent diagnostics across the driver. The 'insert failed and the release does not already exist' branch became reachable with the rollback fix but had no coverage; add a test asserting the original error is surfaced and that it is not ErrReleaseExists. Signed-off-by: Mukul --- pkg/storage/driver/sql.go | 4 +-- pkg/storage/driver/sql_test.go | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 8dff008f1..ec3566851 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -542,7 +542,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { ToSql() if buildErr != nil { s.Logger().Debug("failed to build select query", "error", buildErr) - return err + return fmt.Errorf("failed to create release %q: %w", key, err) } var record SQLReleaseWrapper @@ -552,7 +552,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { } s.Logger().Debug("failed to store release in SQL database", slog.String("key", key), slog.Any("error", err)) - return err + return fmt.Errorf("failed to create release %q: %w", key, err) } // Filtering labels before insert cause in SQL storage driver system releases are stored in separate columns of release table diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index f75ec879c..7ca845485 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -384,6 +384,63 @@ func TestSqlCreateAlreadyExists(t *testing.T) { assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") } +func TestSqlCreateInsertFailureNotAlreadyExists(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) + + insertQuery := 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, + ) + + // The insert fails for a reason unrelated to a duplicate key, e.g. the + // database is unreachable. + insertErr := errors.New("connection refused") + mock.ExpectBegin() + mock. + ExpectExec(regexp.QuoteMeta(insertQuery)). + WithArgs(key, sqlReleaseDefaultType, body, rel.Name, rel.Namespace, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp()). + WillReturnError(insertErr) + + selectQuery := fmt.Sprintf( + regexp.QuoteMeta("SELECT %s FROM %s WHERE %s = $1 AND %s = $2"), + sqlReleaseTableKeyColumn, + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + mock.ExpectRollback() + + // No row comes back, so the release does not already exist and the original + // insert error has to be surfaced instead of ErrReleaseExists. + mock. + ExpectQuery(selectQuery). + WithArgs(key, namespace). + WillReturnError(sql.ErrNoRows) + + err := sqlDriver.Create(key, rel) + require.Errorf(t, err, "expected Create to fail when the insert fails with key %s", key) + require.NotErrorIs(t, err, ErrReleaseExists) + require.ErrorIs(t, err, insertErr) + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + func TestSqlUpdate(t *testing.T) { vers := 1 name := "smug-pigeon"