From df5904d88aed0b7d1d110f17297037811a0dafd4 Mon Sep 17 00:00:00 2001 From: Maxim Trofimov Date: Fri, 21 Apr 2023 10:15:22 +0300 Subject: [PATCH 1/5] add check if all migrations already applied Signed-off-by: Maxim Trofimov --- pkg/storage/driver/sql.go | 47 +++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 18f51f3fd..a9fa4c23f 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -94,8 +94,42 @@ func (s *SQL) Name() string { return SQLDriverName } +// Check if all migrations al +func (s *SQL) checkAlreadyApplied(migrations []*migrate.Migration) bool { + // make map (set) of ids for fast search + migrationsIds := make(map[string]struct{}) + for _, migration := range migrations { + migrationsIds[migration.Id] = struct{}{} + } + + // get list of applied migrations + migrate.SetDisableCreateTable(true) + records, err := migrate.GetMigrationRecords(s.db.DB, postgreSQLDialect) + migrate.SetDisableCreateTable(false) + if err != nil { + s.Log("checkAlreadyApplied: failed to get migration records: %v", err) + return false + } + + for _, record := range records { + if _, ok := migrationsIds[record.Id]; ok { + s.Log("checkAlreadyApplied: found previous migration (Id: %v) applied at %v", record.Id, record.AppliedAt) + delete(migrationsIds, record.Id) + } + } + + // check if all migrations appliyed + if len(migrationsIds) != 0 { + for id := range migrationsIds { + s.Log("checkAlreadyApplied: find unapplied migration (id: %v)", id) + } + return false + } + return true +} + func (s *SQL) ensureDBSetup() error { - // Populate the database with the relations we need if they don't exist yet + migrations := &migrate.MemoryMigrationSource{ Migrations: []*migrate.Migration{ { @@ -121,9 +155,9 @@ func (s *SQL) ensureDBSetup() error { CREATE INDEX ON %s (%s); CREATE INDEX ON %s (%s); CREATE INDEX ON %s (%s); - + GRANT ALL ON %s TO PUBLIC; - + ALTER TABLE %s ENABLE ROW LEVEL SECURITY; `, sqlReleaseTableName, @@ -200,6 +234,12 @@ func (s *SQL) ensureDBSetup() error { }, } + // Check that init migration already applied + if s.checkAlreadyApplied(migrations.Migrations) { + return nil + } + + // Populate the database with the relations we need if they don't exist yet _, err := migrate.Exec(s.db.DB, postgreSQLDialect, migrations, migrate.Up) return err } @@ -315,7 +355,6 @@ func (s *SQL) List(filter func(*rspb.Release) bool) ([]*rspb.Release, error) { s.Log("list: failed to list: %v", err) return nil, err } - var releases []*rspb.Release for _, record := range records { release, err := decodeRelease(record.Body) From b786cb40f09212a2b1c2c337f233a9b9c28122d9 Mon Sep 17 00:00:00 2001 From: Maxim Trofimov Date: Mon, 24 Apr 2023 17:27:22 +0300 Subject: [PATCH 2/5] fix Signed-off-by: Maxim Trofimov --- pkg/storage/driver/sql.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index a9fa4c23f..892f10bbb 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -355,6 +355,7 @@ func (s *SQL) List(filter func(*rspb.Release) bool) ([]*rspb.Release, error) { s.Log("list: failed to list: %v", err) return nil, err } + var releases []*rspb.Release for _, record := range records { release, err := decodeRelease(record.Body) From 199784f7116cd1949aacb6af0b3e1cd473227d75 Mon Sep 17 00:00:00 2001 From: Maxim Trofimov Date: Sat, 26 Aug 2023 20:09:06 +0300 Subject: [PATCH 3/5] fix conflict Signed-off-by: Maxim Trofimov --- pkg/storage/driver/sql_test.go | 36 ++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index 4c0c7b668..6caeed77f 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -21,6 +21,7 @@ import ( "time" sqlmock "github.com/DATA-DOG/go-sqlmock" + migrate "github.com/rubenv/sql-migrate" rspb "helm.sh/helm/v3/pkg/release" ) @@ -530,3 +531,38 @@ func mockGetReleaseCustomLabels(mock sqlmock.Sqlmock, key string, namespace stri } eq.WillReturnRows(returnRows).RowsWillBeClosed() } + +func TestCheckAlreadyAppliedFind(t *testing.T) { + sqlDriver, mock := newTestFixtureSQL(t) + initID := "init" + testMigrations := []*migrate.Migration{{Id: initID}} + mock. + ExpectQuery(""). + WillReturnRows( + sqlmock.NewRows([]string{"id", "applied_at"}). + AddRow(initID, time.Time{})) + mock.ExpectCommit() + + if !sqlDriver.checkAlreadyApplied(testMigrations) { + t.Errorf("Did not find init id: %v", initID) + } + +} + +func TestCheckAlreadyAppliedNotFind(t *testing.T) { + sqlDriver, mock := newTestFixtureSQL(t) + initID := "init" + testMigrations := []*migrate.Migration{{Id: initID}} + mock. + ExpectQuery(""). + WillReturnRows( + sqlmock.NewRows([]string{"id", "applied_at"}). + AddRow("1", time.Time{}). + AddRow("2", time.Time{})) + mock.ExpectCommit() + + if sqlDriver.checkAlreadyApplied(testMigrations) { + t.Errorf("Did find init id: %v, that not exists", initID) + } + +} From 6138e101aa4a468f4fdc169b1fda64b6cd32a111 Mon Sep 17 00:00:00 2001 From: Maxim Trofimov Date: Mon, 24 Apr 2023 20:38:13 +0300 Subject: [PATCH 4/5] add big tests Signed-off-by: Maxim Trofimov --- pkg/storage/driver/sql_test.go | 49 +++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index 6caeed77f..088c17076 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -562,7 +562,54 @@ func TestCheckAlreadyAppliedNotFind(t *testing.T) { mock.ExpectCommit() if sqlDriver.checkAlreadyApplied(testMigrations) { - t.Errorf("Did find init id: %v, that not exists", initID) + t.Errorf("Did find init id: %v, that does not exist", initID) + } + +} + +func TestCheckAlreadyAppliedBigFind(t *testing.T) { + sqlDriver, mock := newTestFixtureSQL(t) + + testMigrations := []*migrate.Migration{{Id: "init1"}, {Id: "init2"}, {Id: "init3"}} + mock. + ExpectQuery(""). + WillReturnRows( + sqlmock.NewRows([]string{"id", "applied_at"}). + AddRow("1", time.Time{}). + AddRow("2", time.Time{}). + AddRow("init1", time.Time{}). + AddRow("init2", time.Time{}). + AddRow("3", time.Time{}). + AddRow("init3", time.Time{}). + AddRow("4", time.Time{}). + AddRow("5", time.Time{})) + mock.ExpectCommit() + + if !sqlDriver.checkAlreadyApplied(testMigrations) { + t.Errorf("Did not find init ids, that exist") + } + +} + +func TestCheckAlreadyAppliedBigNotFind(t *testing.T) { + sqlDriver, mock := newTestFixtureSQL(t) + + testMigrations := []*migrate.Migration{{Id: "init1"}, {Id: "init2"}, {Id: "init3"}} + mock. + ExpectQuery(""). + WillReturnRows( + sqlmock.NewRows([]string{"id", "applied_at"}). + AddRow("1", time.Time{}). + AddRow("2", time.Time{}). + AddRow("init1", time.Time{}). + AddRow("3", time.Time{}). + AddRow("init2", time.Time{}). + AddRow("4", time.Time{}). + AddRow("5", time.Time{})) + mock.ExpectCommit() + + if sqlDriver.checkAlreadyApplied(testMigrations) { + t.Errorf("Did not find init id: init3, that does not exist") } } From 4944acb3410d9baf377a495f41df628115a3ce35 Mon Sep 17 00:00:00 2001 From: Maxim Trofimov Date: Sat, 26 Aug 2023 20:11:08 +0300 Subject: [PATCH 5/5] fix conflict Signed-off-by: Maxim Trofimov --- pkg/storage/driver/sql_test.go | 124 ++++++++++++--------------------- 1 file changed, 45 insertions(+), 79 deletions(-) diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index 088c17076..067b3cf47 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -532,84 +532,50 @@ func mockGetReleaseCustomLabels(mock sqlmock.Sqlmock, key string, namespace stri eq.WillReturnRows(returnRows).RowsWillBeClosed() } -func TestCheckAlreadyAppliedFind(t *testing.T) { - sqlDriver, mock := newTestFixtureSQL(t) - initID := "init" - testMigrations := []*migrate.Migration{{Id: initID}} - mock. - ExpectQuery(""). - WillReturnRows( - sqlmock.NewRows([]string{"id", "applied_at"}). - AddRow(initID, time.Time{})) - mock.ExpectCommit() - - if !sqlDriver.checkAlreadyApplied(testMigrations) { - t.Errorf("Did not find init id: %v", initID) - } - -} - -func TestCheckAlreadyAppliedNotFind(t *testing.T) { - sqlDriver, mock := newTestFixtureSQL(t) - initID := "init" - testMigrations := []*migrate.Migration{{Id: initID}} - mock. - ExpectQuery(""). - WillReturnRows( - sqlmock.NewRows([]string{"id", "applied_at"}). - AddRow("1", time.Time{}). - AddRow("2", time.Time{})) - mock.ExpectCommit() - - if sqlDriver.checkAlreadyApplied(testMigrations) { - t.Errorf("Did find init id: %v, that does not exist", initID) - } - -} - -func TestCheckAlreadyAppliedBigFind(t *testing.T) { - sqlDriver, mock := newTestFixtureSQL(t) - - testMigrations := []*migrate.Migration{{Id: "init1"}, {Id: "init2"}, {Id: "init3"}} - mock. - ExpectQuery(""). - WillReturnRows( - sqlmock.NewRows([]string{"id", "applied_at"}). - AddRow("1", time.Time{}). - AddRow("2", time.Time{}). - AddRow("init1", time.Time{}). - AddRow("init2", time.Time{}). - AddRow("3", time.Time{}). - AddRow("init3", time.Time{}). - AddRow("4", time.Time{}). - AddRow("5", time.Time{})) - mock.ExpectCommit() - - if !sqlDriver.checkAlreadyApplied(testMigrations) { - t.Errorf("Did not find init ids, that exist") - } - -} - -func TestCheckAlreadyAppliedBigNotFind(t *testing.T) { - sqlDriver, mock := newTestFixtureSQL(t) - - testMigrations := []*migrate.Migration{{Id: "init1"}, {Id: "init2"}, {Id: "init3"}} - mock. - ExpectQuery(""). - WillReturnRows( - sqlmock.NewRows([]string{"id", "applied_at"}). - AddRow("1", time.Time{}). - AddRow("2", time.Time{}). - AddRow("init1", time.Time{}). - AddRow("3", time.Time{}). - AddRow("init2", time.Time{}). - AddRow("4", time.Time{}). - AddRow("5", time.Time{})) - mock.ExpectCommit() - - if sqlDriver.checkAlreadyApplied(testMigrations) { - t.Errorf("Did not find init id: init3, that does not exist") +func TestSqlChechkAppliedMigrations(t *testing.T) { + cases := []struct { + migrationsToApply []*migrate.Migration + appliedMigrationsIds []string + expectedResult bool + errorExplanation string + }{ + { + migrationsToApply: []*migrate.Migration{{Id: "init1"}, {Id: "init2"}, {Id: "init3"}}, + appliedMigrationsIds: []string{"1", "2", "init1", "3", "init2", "4", "5"}, + expectedResult: false, + errorExplanation: "Has found one migration id \"init3\" as applied, that was not applied", + }, + { + migrationsToApply: []*migrate.Migration{{Id: "init1"}, {Id: "init2"}, {Id: "init3"}}, + appliedMigrationsIds: []string{"1", "2", "init1", "3", "init2", "4", "init3", "5"}, + expectedResult: true, + errorExplanation: "Has not found one or more migration ids, that was applied", + }, + { + migrationsToApply: []*migrate.Migration{{Id: "init"}}, + appliedMigrationsIds: []string{"1", "2", "3", "inits", "4", "tinit", "5"}, + expectedResult: false, + errorExplanation: "Has found single \"init\", that was not applied", + }, + { + migrationsToApply: []*migrate.Migration{{Id: "init"}}, + appliedMigrationsIds: []string{"1", "2", "init", "3", "init2", "4", "init3", "5"}, + expectedResult: true, + errorExplanation: "Has not found single migration id \"init\", that was applied", + }, + } + for i, c := range cases { + sqlDriver, mock := newTestFixtureSQL(t) + rows := sqlmock.NewRows([]string{"id", "applied_at"}) + for _, id := range c.appliedMigrationsIds { + rows.AddRow(id, time.Time{}) + } + mock. + ExpectQuery(""). + WillReturnRows(rows) + mock.ExpectCommit() + if sqlDriver.checkAlreadyApplied(c.migrationsToApply) != c.expectedResult { + t.Errorf("Test case: %v, Expected: %v, Have: %v, Explanation: %v", i, c.expectedResult, !c.expectedResult, c.errorExplanation) + } } - }