From b80a54fa6a29ffb6e5421c1f63e333d8126066b1 Mon Sep 17 00:00:00 2001 From: Jason Roselander Date: Thu, 27 Jan 2022 16:09:19 -0800 Subject: [PATCH] Updated schema tool to work with full semantic versions, i.e., MAJOR.MINOR.PATCH (#2417) * Added UnitTestLogger which sends log messages to the testing.T.Log API. * Updated schema tool to work with full semantic versions, i.e., MAJOR.MINOR.PATCH * Responded to PR comments --- tools/common/schema/handler.go | 4 +- tools/common/schema/updatetask.go | 173 ++++++++++++++----------- tools/common/schema/updatetask_test.go | 104 +++++++++++---- tools/common/schema/version.go | 33 +---- tools/common/schema/version_test.go | 55 +++----- 5 files changed, 196 insertions(+), 173 deletions(-) diff --git a/tools/common/schema/handler.go b/tools/common/schema/handler.go index 36d6c2e5778..85626d5b76c 100644 --- a/tools/common/schema/handler.go +++ b/tools/common/schema/handler.go @@ -82,7 +82,7 @@ func validateSetupConfig(config *SetupConfig) error { flag(CLIOptVersion) + " but not both must be specified") } if !config.DisableVersioning { - ver, err := parseValidateVersion(config.InitialVersion) + ver, err := normalizeVersionString(config.InitialVersion) if err != nil { return NewConfigError("invalid " + flag(CLIOptVersion) + " argument:" + err.Error()) } @@ -96,7 +96,7 @@ func validateUpdateConfig(config *UpdateConfig) error { return NewConfigError("missing " + flag(CLIOptSchemaDir) + " argument ") } if len(config.TargetVersion) > 0 { - ver, err := parseValidateVersion(config.TargetVersion) + ver, err := normalizeVersionString(config.TargetVersion) if err != nil { return NewConfigError("invalid " + flag(CLIOptTargetVersion) + " argument:" + err.Error()) } diff --git a/tools/common/schema/updatetask.go b/tools/common/schema/updatetask.go index ae1597b2e3f..0bcb3acb4a7 100644 --- a/tools/common/schema/updatetask.go +++ b/tools/common/schema/updatetask.go @@ -33,9 +33,12 @@ import ( "encoding/json" "fmt" "os" + "regexp" "sort" "strings" + "github.com/blang/semver/v4" + "go.temporal.io/server/common/log" "go.temporal.io/server/common/log/tag" ) @@ -67,11 +70,6 @@ type ( manifest *manifest cqlStmts []string } - - // byVersion is a comparator type - // for sorting a set of version - // strings - byVersion []string ) const ( @@ -80,6 +78,7 @@ const ( var ( whitelistedCQLPrefixes = [4]string{"CREATE", "ALTER", "INSERT", "DROP"} + versionDirectoryRegex = regexp.MustCompile(`^v[\d.]+`) ) // NewUpdateSchemaTask returns a new instance of UpdateTask @@ -98,7 +97,7 @@ func (task *UpdateTask) Run() error { task.logger.Info("UpdateSchemeTask started", tag.NewAnyTag("config", config)) if config.IsDryRun { - if err := task.setupDryrunDatabase(); err != nil { + if err := task.setupDryRunDatabase(); err != nil { return fmt.Errorf("error creating dryrun database:%v", err.Error()) } } @@ -179,11 +178,13 @@ func (task *UpdateTask) buildChangeSet(currVer string) ([]changeSet, error) { config := task.config - verDirs, err := readSchemaDir(config.SchemaDir, currVer, config.TargetVersion) + verDirs, err := readSchemaDir(config.SchemaDir, currVer, config.TargetVersion, task.logger) if err != nil { return nil, fmt.Errorf("error listing schema dir:%v", err.Error()) } + task.logger.Debug(fmt.Sprintf("Schema Dirs: %s", verDirs)) + var result []changeSet for _, vd := range verDirs { @@ -196,8 +197,10 @@ func (task *UpdateTask) buildChangeSet(currVer string) ([]changeSet, error) { } if m.CurrVersion != dirToVersion(vd) { - return nil, fmt.Errorf("manifest version doesn't match with dirname, dir=%v,manifest.version=%v", - vd, m.CurrVersion) + return nil, fmt.Errorf( + "manifest version doesn't match with dirname, dir=%v,manifest.version=%v", + vd, m.CurrVersion, + ) } stmts, e := task.parseSQLStmts(dirPath, m) @@ -226,6 +229,7 @@ func (task *UpdateTask) parseSQLStmts(dir string, manifest *manifest) ([]string, for _, file := range manifest.SchemaUpdateCqlFiles { path := dir + "/" + file + task.logger.Info("Processing schema file: " + path) stmts, err := ParseFile(path) if err != nil { return nil, fmt.Errorf("error parsing file %v, err=%v", path, err) @@ -270,13 +274,13 @@ func readManifest(dirPath string) (*manifest, error) { return nil, err } - currVer, err := parseValidateVersion(manifest.CurrVersion) + currVer, err := normalizeVersionString(manifest.CurrVersion) if err != nil { return nil, fmt.Errorf("invalid CurrVersion in manifest") } manifest.CurrVersion = currVer - minVer, err := parseValidateVersion(manifest.MinCompatibleVersion) + minVer, err := normalizeVersionString(manifest.MinCompatibleVersion) if err != nil { return nil, err } @@ -297,88 +301,119 @@ func readManifest(dirPath string) (*manifest, error) { return &manifest, nil } -// readSchemaDir returns a sorted list of subdir names that hold -// the schema changes for versions in the range startVer < ver <= endVer -// when endVer is empty this method returns all subdir names that are greater than startVer -// this method has an assumption that the subdirs containing the -// schema changes will be of the form vx.x, where x.x is the version -// returns error when -// - startVer < endVer -// - endVer is empty and no subdirs have version >= startVer -// - endVer is non-empty and subdir with version == endVer is not found -func readSchemaDir(dir string, startVer string, endVer string) ([]string, error) { - - subdirs, err := os.ReadDir(dir) +// sortAndFilterVersions returns a sorted list of semantic versions the fall within the range +// (startVerExcl, endVerIncl]. If endVerIncl is not specified, returns all versions > startVerExcl. +// If endVerIncl is specified, it must be present in the list of versions. +func sortAndFilterVersions(versions []string, startVerExcl string, endVerIncl string, logger log.Logger) ([]string, error) { + + startVersionExclusive, err := semver.ParseTolerant(startVerExcl) if err != nil { return nil, err } - hasEndVer := len(endVer) > 0 - - if hasEndVer && cmpVersion(startVer, endVer) > 0 { - return nil, fmt.Errorf("startVer (%v) must be less than or equal to endVer (%v)", startVer, endVer) + var endVersionInclusive *semver.Version + if len(endVerIncl) > 0 { + evi, err := semver.ParseTolerant(endVerIncl) + if err != nil { + return nil, err + } + endVersionInclusive = &evi + + cmp := startVersionExclusive.Compare(*endVersionInclusive) + if cmp > 0 { + return nil, fmt.Errorf("start version '%s' must be less than end version '%s'", startVerExcl, endVerIncl) + } else if cmp == 0 { + logger.Warn( + fmt.Sprintf( + "Start version '%s' is equal to end version '%s'. Returning empty version list", + startVerExcl, + endVerIncl, + ), + ) + return []string{}, nil + } } - var endFound bool - var highestVer string - var result []string + var retVersions []string - for _, dir := range subdirs { + foundEndVer := false - if !dir.IsDir() { + for _, version := range versions { + semVer, err := semver.ParseTolerant(version) + if err != nil { + logger.Warn(fmt.Sprintf("Input '%s' is not a valid semver", version)) continue } - dirname := dir.Name() - - if !versionStrRegex.MatchString(dirname) { + if startVersionExclusive.Compare(semVer) >= 0 { continue } - ver := dirToVersion(dirname) - - if len(highestVer) == 0 { - highestVer = ver - } else if cmpVersion(ver, highestVer) > 0 { - highestVer = ver + if endVersionInclusive != nil && endVersionInclusive.Compare(semVer) < 0 { + continue } - highcmp := 0 - lowcmp := cmpVersion(ver, startVer) - if hasEndVer { - highcmp = cmpVersion(ver, endVer) - endFound = endFound || (highcmp == 0) + if endVersionInclusive != nil && endVersionInclusive.Compare(semVer) == 0 { + foundEndVer = true } - if lowcmp <= 0 || highcmp > 0 { - continue // out of range + retVersions = append(retVersions, version) + } + + if endVersionInclusive != nil && !foundEndVer { + return nil, fmt.Errorf("end version '%s' specified but not found. existing versions: %s", endVerIncl, versions) + } + + sort.Slice(retVersions, func(i, j int) bool { + verI, err := semver.ParseTolerant(retVersions[i]) + if err != nil { + panic(err) + } + verJ, err := semver.ParseTolerant(retVersions[j]) + if err != nil { + panic(err) } + return verI.Compare(verJ) < 0 + }) - result = append(result, dirname) + return retVersions, nil +} + +// readSchemaDir returns a sorted list of subdir names that hold +// the schema changes for versions in the range startVer < ver <= endVer +// when endVer is empty this method returns all subdir names that are greater than startVer +func readSchemaDir(dir string, startVer string, endVer string, logger log.Logger) ([]string, error) { + + subDirs, err := os.ReadDir(dir) + if err != nil { + return nil, err } - // when endVer is specified, atleast one result MUST be found - if hasEndVer && !endFound { - return nil, fmt.Errorf("version dir not found for target version %v", endVer) + if len(subDirs) == 0 { + return nil, fmt.Errorf("directory '%s' contains no subDirs", dir) } - // when endVer is empty and no result is found, then the highest version - // found must be equal to startVer, else return error - if !hasEndVer && len(result) == 0 { - if len(highestVer) == 0 || cmpVersion(startVer, highestVer) != 0 { - return nil, fmt.Errorf("no subdirs found with version >= %v", startVer) + dirNames := make([]string, 0, len(subDirs)) + for _, d := range subDirs { + if !d.IsDir() { + logger.Warn("not a directory: " + d.Name()) + continue } - return result, nil - } - sort.Sort(byVersion(result)) + if !versionDirectoryRegex.MatchString(d.Name()) { + logger.Warn("invalid directory name: " + d.Name()) + continue + } - return result, nil + dirNames = append(dirNames, d.Name()) + } + + return sortAndFilterVersions(dirNames, startVer, endVer, logger) } // sets up a temporary dryrun database for // executing the cassandra schema update -func (task *UpdateTask) setupDryrunDatabase() error { +func (task *UpdateTask) setupDryRunDatabase() error { setupConfig := &SetupConfig{ Overwrite: true, InitialVersion: "0.0", @@ -390,17 +425,3 @@ func (task *UpdateTask) setupDryrunDatabase() error { func dirToVersion(dir string) string { return dir[1:] } - -func (v byVersion) Len() int { - return len(v) -} - -func (v byVersion) Less(i, j int) bool { - v1 := dirToVersion(v[i]) - v2 := dirToVersion(v[j]) - return cmpVersion(v1, v2) < 0 -} - -func (v byVersion) Swap(i, j int) { - v[i], v[j] = v[j], v[i] -} diff --git a/tools/common/schema/updatetask_test.go b/tools/common/schema/updatetask_test.go index e0d25042834..31c9c5e3e1f 100644 --- a/tools/common/schema/updatetask_test.go +++ b/tools/common/schema/updatetask_test.go @@ -28,6 +28,10 @@ import ( "os" "testing" + "github.com/stretchr/testify/assert" + "go.temporal.io/server/common/log" + "go.uber.org/zap/zaptest" + "go.temporal.io/server/tests/testhelper" "github.com/stretchr/testify/require" @@ -35,55 +39,97 @@ import ( ) type UpdateTaskTestSuite struct { - *require.Assertions // override suite.Suite.Assertions with require.Assertions; this means that s.NotNil(nil) will stop the test, not merely log an error + *require.Assertions // override suite.Suite.Assertions with require.Assertions; this means that s.NotNil(n) will stop the test, not merely log an error suite.Suite + versionsDir string + emptyDir string + logger log.Logger } func TestUpdateTaskTestSuite(t *testing.T) { suite.Run(t, new(UpdateTaskTestSuite)) } +var updateTaskTestData = struct { + versions []string +}{ + versions: []string{"v0.5", "v1.5", "v2.5", "v2.5.1", "v2.5.2", "v2.7.17", "v3.5", "v10.2", "abc", "2.0", "3.0"}, +} + func (s *UpdateTaskTestSuite) SetupSuite() { s.Assertions = require.New(s.T()) + + s.versionsDir = testhelper.MkdirTemp(s.T(), "", "update_schema_test") + + for _, d := range updateTaskTestData.versions { + s.NoError(os.Mkdir(s.versionsDir+"/"+d, os.FileMode(0444))) + } + + s.emptyDir = testhelper.MkdirTemp(s.T(), "", "update_schema_test_empty") + + s.logger = log.NewZapLogger(zaptest.NewLogger(s.T())) } func (s *UpdateTaskTestSuite) TestReadSchemaDir() { - emptyDir := testhelper.MkdirTemp(s.T(), "", "update_schema_test_empty") - tmpDir := testhelper.MkdirTemp(s.T(), "", "update_schema_test") + ans, err := readSchemaDir(s.versionsDir, "0.5", "1.5", s.logger) + s.NoError(err) + s.Equal([]string{"v1.5"}, ans) - subDirs := []string{"v0.5", "v1.5", "v2.5", "v3.5", "v10.2", "abc", "2.0", "3.0"} - for _, d := range subDirs { - s.NoError(os.Mkdir(tmpDir+"/"+d, os.FileMode(0444))) - } + ans, err = readSchemaDir(s.versionsDir, "0.4", "10.2", s.logger) + s.NoError(err) + s.Equal([]string{"v0.5", "v1.5", "v2.5", "v2.5.1", "v2.5.2", "v2.7.17", "v3.5", "v10.2"}, ans) - _, err := readSchemaDir(tmpDir, "11.0", "11.2") - s.Error(err) - _, err = readSchemaDir(tmpDir, "0.5", "10.3") - s.Error(err) - _, err = readSchemaDir(tmpDir, "1.5", "0.5") - s.Error(err) - _, err = readSchemaDir(tmpDir, "10.3", "") - s.Error(err) - _, err = readSchemaDir(emptyDir, "11.0", "") - s.Error(err) - _, err = readSchemaDir(emptyDir, "10.1", "") - s.Error(err) + ans, err = readSchemaDir(s.versionsDir, "0.5", "3.5", s.logger) + s.NoError(err) + s.Equal([]string{"v1.5", "v2.5", "v2.5.1", "v2.5.2", "v2.7.17", "v3.5"}, ans) - ans, err := readSchemaDir(tmpDir, "1.5", "1.5") + // Start version found, no later versions. Return nothing. + ans, err = readSchemaDir(s.versionsDir, "10.2", "", s.logger) s.NoError(err) s.Equal(0, len(ans)) - ans, err = readSchemaDir(tmpDir, "0.4", "10.2") + // Start version not found, no later versions. Return nothing. + ans, err = readSchemaDir(s.versionsDir, "10.3", "", s.logger) s.NoError(err) - s.Equal([]string{"v0.5", "v1.5", "v2.5", "v3.5", "v10.2"}, ans) + s.Equal(0, len(ans)) - ans, err = readSchemaDir(tmpDir, "0.5", "3.5") + ans, err = readSchemaDir(s.versionsDir, "2.5.2", "", s.logger) s.NoError(err) - s.Equal([]string{"v1.5", "v2.5", "v3.5"}, ans) + s.Equal([]string{"v2.7.17", "v3.5", "v10.2"}, ans) +} - ans, err = readSchemaDir(tmpDir, "10.2", "") +func (s *UpdateTaskTestSuite) TestSortAndFilterVersionsWithEndLessThanStart_ReturnsError() { + _, err := sortAndFilterVersions(updateTaskTestData.versions, "1.5", "0.5", s.logger) + s.Error(err) + assert.Containsf(s.T(), err.Error(), "less than end version", "Unexpected error message") +} + +func (s *UpdateTaskTestSuite) TestReadSchemaDirWithEndVersion_ReturnsErrorWhenNotFound() { + // No versions in range + _, err := readSchemaDir(s.versionsDir, "11.0", "11.2", s.logger) + s.Error(err) + assert.Containsf(s.T(), err.Error(), "specified but not found", "Unexpected error message") + + // Versions in range, but nothing for v10.3 + _, err = readSchemaDir(s.versionsDir, "0.5", "10.3", s.logger) + s.Error(err) + assert.Containsf(s.T(), err.Error(), "specified but not found", "Unexpected error message") +} + +func (s *UpdateTaskTestSuite) TestReadSchemaDirWithSameStartAndEnd_ReturnsEmptyList() { + ans, err := readSchemaDir(s.versionsDir, "1.7", "1.7", s.logger) s.NoError(err) - s.Equal(0, len(ans)) + assert.Equal(s.T(), 0, len(ans)) +} + +func (s *UpdateTaskTestSuite) TestReadSchemaDirWithEmptyDir_ReturnsError() { + _, err := readSchemaDir(s.emptyDir, "11.0", "", s.logger) + s.Error(err) + assert.Containsf(s.T(), err.Error(), "contains no subDirs", "Unexpected error message") + + _, err = readSchemaDir(s.emptyDir, "10.1", "", s.logger) + s.Error(err) + assert.Containsf(s.T(), err.Error(), "contains no subDirs", "Unexpected error message") } func (s *UpdateTaskTestSuite) TestReadManifest() { @@ -139,8 +185,10 @@ func (s *UpdateTaskTestSuite) TestReadManifest() { } } -func (s *UpdateTaskTestSuite) runReadManifestTest(dir, input, currVer, minVer, desc string, - files []string, isErr bool) { +func (s *UpdateTaskTestSuite) runReadManifestTest( + dir, input, currVer, minVer, desc string, + files []string, isErr bool, +) { file := dir + "/manifest.json" err := os.WriteFile(file, []byte(input), os.FileMode(0644)) diff --git a/tools/common/schema/version.go b/tools/common/schema/version.go index 10adc1adb29..2c25afe36b9 100644 --- a/tools/common/schema/version.go +++ b/tools/common/schema/version.go @@ -25,39 +25,16 @@ package schema import ( - "fmt" - "regexp" - "github.com/blang/semver/v4" ) -// represents names of the form vx.x where x.x is a (major, minor) version pair -var versionStrRegex = regexp.MustCompile(`^v\d+(\.\d+)?$`) - -// represents names of the form x.x where minor version is always single digit -var versionNumRegex = regexp.MustCompile(`^\d+(\.\d+)?$`) - -// cmpVersion compares two version strings -// returns 0 if a == b -// returns < 0 if a < b -// returns > 0 if a > b -func cmpVersion(a, b string) int { - aParsed, _ := semver.ParseTolerant(a) - bParsed, _ := semver.ParseTolerant(b) - return aParsed.Compare(bParsed) -} - -// parseValidateVersion validates that the given input conforms to either of vx.x or x.x and -// returns x.x on success -func parseValidateVersion(ver string) (string, error) { - if len(ver) == 0 { - return "", fmt.Errorf("version is empty") +// normalizeVersionString take a valid semver string and returns the input as-is with the 'v' prefix removed if present +func normalizeVersionString(ver string) (string, error) { + if _, err := semver.ParseTolerant(ver); err != nil { + return "", err } - if versionStrRegex.MatchString(ver) { + if ver[0] == 'v' { return ver[1:], nil } - if !versionNumRegex.MatchString(ver) { - return "", fmt.Errorf("invalid version, expected format is x.x") - } return ver, nil } diff --git a/tools/common/schema/version_test.go b/tools/common/schema/version_test.go index 3f8301874eb..b2fdaee6174 100644 --- a/tools/common/schema/version_test.go +++ b/tools/common/schema/version_test.go @@ -46,52 +46,29 @@ func (s *VersionTestSuite) SetupTest() { s.Assertions = require.New(s.T()) // Have to define our overridden assertions in the test setup. If we did it earlier, s.T() will return nil } -func (s *VersionTestSuite) TestCmpVersion() { +func (s *VersionTestSuite) TestNormalizeVersionStringSuccess() { - s.Equal(0, cmpVersion("0", "0")) - s.Equal(0, cmpVersion("999", "999")) - s.Equal(0, cmpVersion("0.0", "0.0")) - s.Equal(0, cmpVersion("0.999", "0.999")) - s.Equal(0, cmpVersion("99.888", "99.888")) + validInputs := []string{"0", "1000", "9999", "0.1", "0.9", "99.9", "100.8"} + for _, in := range validInputs { + ans, err := normalizeVersionString(in) + s.NoError(err, in) + s.Equal(in, ans, in) - s.True(cmpVersion("0.1", "0") > 0) - s.True(cmpVersion("0.5", "0.1") > 0) - s.True(cmpVersion("1.1", "0.1") > 0) - s.True(cmpVersion("1.1", "0.9") > 0) - s.True(cmpVersion("1.1", "1.0") > 0) - - s.True(cmpVersion("0", "0.1") < 0) - s.True(cmpVersion("0.1", "0.5") < 0) - s.True(cmpVersion("0.1", "1.1") < 0) - s.True(cmpVersion("0.9", "1.1") < 0) - s.True(cmpVersion("1.0", "1.1") < 0) + withPrefix := "v" + in + ans, err = normalizeVersionString(withPrefix) + s.NoError(err, in) + s.Equal(in, ans, in) + } - s.True(cmpVersion("0.1a", "0.5") < 0) - s.True(cmpVersion("0.1", "0.5a") > 0) - s.True(cmpVersion("ab", "cd") == 0) } -func (s *VersionTestSuite) TestParseValidateVersion() { - - inputs := []string{"0", "1000", "9999", "0.1", "0.9", "99.9", "100.8"} - for _, in := range inputs { - s.execParseValidateTest(in, in, false) - s.execParseValidateTest("v"+in, in, false) - } +func (s *VersionTestSuite) TestNormalizeVersionStringInvalidInput() { errInputs := []string{"1.2a", "ab", "5.11a"} for _, in := range errInputs { - s.execParseValidateTest(in, "", true) - s.execParseValidateTest("v"+in, "", true) - } -} - -func (s *VersionTestSuite) execParseValidateTest(input string, output string, isErr bool) { - ver, err := parseValidateVersion(input) - if isErr { - s.NotNil(err) - return + _, err := normalizeVersionString(in) + s.Error(err, in) + _, err = normalizeVersionString("v" + in) + s.Errorf(err, in) } - s.Nil(err) - s.Equal(output, ver) }