From 40a6323236409bac7d464eb287366f569192ac80 Mon Sep 17 00:00:00 2001 From: Marek Siarkowicz Date: Thu, 26 Aug 2021 12:53:53 +0200 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Lili Cosic --- server/storage/schema/actions.go | 6 +-- server/storage/schema/migration.go | 28 ++++++++----- server/storage/schema/migration_test.go | 55 +++++++++++++++++++++++++ server/storage/schema/schema.go | 4 +- server/storage/schema/schema_test.go | 4 +- 5 files changed, 79 insertions(+), 18 deletions(-) diff --git a/server/storage/schema/actions.go b/server/storage/schema/actions.go index 103a2ef7ae68..20c8f1193aa5 100644 --- a/server/storage/schema/actions.go +++ b/server/storage/schema/actions.go @@ -73,7 +73,7 @@ func (as ActionList) unsafeExecute(lg *zap.Logger, tx backend.BatchTx) error { revert, err := a.unsafeDo(tx) if err != nil { - revertActions.unsafeExecuteInReverseOrder(lg, tx) + revertActions.unsafeExecuteInReversedOrder(lg, tx) return err } revertActions = append(revertActions, revert) @@ -81,9 +81,9 @@ func (as ActionList) unsafeExecute(lg *zap.Logger, tx backend.BatchTx) error { return nil } -// unsafeExecuteInReverseOrder executes actions in revered order. Will panic on +// unsafeExecuteInReversedOrder executes actions in revered order. Will panic on // action error. Should be used when reverting. -func (as ActionList) unsafeExecuteInReverseOrder(lg *zap.Logger, tx backend.BatchTx) { +func (as ActionList) unsafeExecuteInReversedOrder(lg *zap.Logger, tx backend.BatchTx) { for j := len(as) - 1; j >= 0; j-- { _, err := as[j].unsafeDo(tx) if err != nil { diff --git a/server/storage/schema/migration.go b/server/storage/schema/migration.go index 00bf6060582d..bac67bec5a7d 100644 --- a/server/storage/schema/migration.go +++ b/server/storage/schema/migration.go @@ -25,26 +25,28 @@ import ( type migrationPlan []migrationStep func newPlan(lg *zap.Logger, current semver.Version, target semver.Version) (p migrationPlan, err error) { - if current.Major != target.Major { - lg.Error("Changing major storage version is not supported", - zap.String("storage-version", current.String()), - zap.String("target-storage-version", target.String()), - ) - return nil, fmt.Errorf("Changing major storage version is not supported") - } // TODO(serathius): Implement downgrades - if current.Minor > target.Minor { + if target.LessThan(current) { lg.Error("Target version is lower than the current version, downgrades are not yet supported", zap.String("storage-version", current.String()), zap.String("target-storage-version", target.String()), ) return nil, fmt.Errorf("downgrades are not yet supported") } - return buildPlan(current, target) + return buildPlan(lg, current, target) } -func buildPlan(current semver.Version, target semver.Version) (plan migrationPlan, err error) { - for current.Minor != target.Minor { +func buildPlan(lg *zap.Logger, current semver.Version, target semver.Version) (plan migrationPlan, err error) { + current = trimToMinor(current) + target = trimToMinor(target) + if current.Major != target.Major { + lg.Error("Changing major storage version is not supported", + zap.String("storage-version", current.String()), + zap.String("target-storage-version", target.String()), + ) + return plan, fmt.Errorf("changing major storage version is not supported") + } + for !current.Equal(target) { isUpgrade := current.Minor < target.Minor changes, err := schemaChangesForVersion(current, isUpgrade) @@ -117,3 +119,7 @@ func (s migrationStep) unsafeExecute(lg *zap.Logger, tx backend.BatchTx) error { } return nil } + +func trimToMinor(ver semver.Version) semver.Version { + return semver.Version{Major: ver.Major, Minor: ver.Minor} +} diff --git a/server/storage/schema/migration_test.go b/server/storage/schema/migration_test.go index 24175b31002a..d81a6208dcac 100644 --- a/server/storage/schema/migration_test.go +++ b/server/storage/schema/migration_test.go @@ -26,6 +26,61 @@ import ( "go.uber.org/zap/zaptest" ) +var ( + V4_0 = semver.Version{Major: 4, Minor: 0} +) + +func TestNewPlan(t *testing.T) { + tcs := []struct { + name string + + current semver.Version + target semver.Version + + expectError bool + expectErrorMsg string + }{ + { + name: "Update v3.5 to v3.6 should work", + current: V3_5, + target: V3_6, + }, + { + name: "Downgrade v3.6 to v3.5 should fail as downgrades are not yet supported", + current: V3_6, + target: V3_5, + expectError: true, + expectErrorMsg: "downgrades are not yet supported", + }, + { + name: "Upgrade v3.6 to v3.7 should fail as v3.7 is unknown", + current: V3_6, + target: V3_7, + expectError: true, + expectErrorMsg: `version "3.7.0" is not supported`, + }, + { + name: "Upgrade v3.6 to v4.0 as major version changes are unsupported", + current: V3_6, + target: V4_0, + expectError: true, + expectErrorMsg: "changing major storage version is not supported", + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + lg := zaptest.NewLogger(t) + _, err := newPlan(lg, tc.current, tc.target) + if (err != nil) != tc.expectError { + t.Errorf("newPlan(lg, %q, %q) returned unexpected error (or lack thereof), expected: %v, got: %v", tc.current, tc.target, tc.expectError, err) + } + if err != nil && err.Error() != tc.expectErrorMsg { + t.Errorf("newPlan(lg, %q, %q) returned unexpected error message, expected: %q, got: %q", tc.current, tc.target, tc.expectErrorMsg, err.Error()) + } + }) + } +} + func TestMigrationStepExecute(t *testing.T) { recorder := &actionRecorder{} errorC := fmt.Errorf("error C") diff --git a/server/storage/schema/schema.go b/server/storage/schema/schema.go index 0fbdb7bb9f54..198b90f3e045 100644 --- a/server/storage/schema/schema.go +++ b/server/storage/schema/schema.go @@ -113,7 +113,7 @@ func schemaChangesForVersion(v semver.Version, isUpgrade bool) ([]schemaChange, } var ( - // schemaChanges list changes that were introduced in perticular version. + // schemaChanges list changes that were introduced in a particular version. // schema was introduced in v3.6 as so its changes were not tracked before. schemaChanges = map[semver.Version][]schemaChange{ V3_6: { @@ -121,6 +121,6 @@ var ( }, } // emptyStorageVersion is used for v3.6 Step for the first time, in all other version StoragetVersion should be set by migrator. - // Adding a addNewField for StorageVersion we can reuselogic to remove it when downgrading to v3.5 + // Adding a addNewField for StorageVersion we can reuse logic to remove it when downgrading to v3.5 emptyStorageVersion = []byte("") ) diff --git a/server/storage/schema/schema_test.go b/server/storage/schema/schema_test.go index bb4a0e3e9b47..11b4d23afa73 100644 --- a/server/storage/schema/schema_test.go +++ b/server/storage/schema/schema_test.go @@ -130,7 +130,7 @@ func TestMigrate(t *testing.T) { expectErrorMsg: `cannot detect storage schema version: missing term information`, }, { - name: `Upgrading v3.5 to v3.6 should be succeed all required fields are set`, + name: `Upgrading v3.5 to v3.6 should succeed; all required fields are set`, version: V3_5, targetVersion: V3_6, expectVersion: &V3_6, @@ -268,7 +268,7 @@ func testUnsafeMigrate(lg *zap.Logger, tx backend.BatchTx, target semver.Version if err != nil { return fmt.Errorf("cannot determine storage version: %w", err) } - plan, err := buildPlan(current, target) + plan, err := buildPlan(lg, current, target) if err != nil { return fmt.Errorf("cannot create migration plan: %w", err) }