Repository: kudu Updated Branches: refs/heads/master 34b058b8b -> 8e40dfdb9
[util] update on ParseVersion() This changelist updates implementation of the kudu::ParseVersion() utility function to recognize and successfully parse version strings like '1.8.0-cdh6.x-SNAPSHOT'. Version::ToString() implementation changed to return 'canonical' representation of the parsed version string. Added corresponding unit tests. This patch also fixes KUDU-2511. This is a follow-up for c12e63528a2f2c265136e702a48cbc6c93b7f2fd. Change-Id: I98b200d0529cb4471e4e855b88bfdb2ff2f346bf Reviewed-on: http://gerrit.cloudera.org:8080/11060 Tested-by: Alexey Serbin <aser...@cloudera.com> Reviewed-by: Andrew Wong <aw...@cloudera.com> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/7e733a73 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/7e733a73 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/7e733a73 Branch: refs/heads/master Commit: 7e733a73652db68342859d03d02763ef5e3de79d Parents: 34b058b Author: Alexey Serbin <aser...@cloudera.com> Authored: Wed Jul 25 22:07:11 2018 -0700 Committer: Alexey Serbin <aser...@cloudera.com> Committed: Mon Jul 30 01:55:52 2018 +0000 ---------------------------------------------------------------------- src/kudu/util/version_util-test.cc | 88 +++++++++++++++++++++++++++------ src/kudu/util/version_util.cc | 29 ++++++----- src/kudu/util/version_util.h | 11 +++-- 3 files changed, 98 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/7e733a73/src/kudu/util/version_util-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/util/version_util-test.cc b/src/kudu/util/version_util-test.cc index 54e8e76..3cb39b9 100644 --- a/src/kudu/util/version_util-test.cc +++ b/src/kudu/util/version_util-test.cc @@ -14,53 +14,109 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. + #include "kudu/util/version_util.h" #include <string> +#include <utility> #include <vector> #include <gtest/gtest.h> #include "kudu/util/status.h" #include "kudu/util/test_macros.h" -#include "kudu/util/test_util.h" +#include "kudu/util/version_info.h" using std::string; +using std::pair; using std::vector; namespace kudu { -class VersionUtilTest : public KuduTest {}; - -TEST_F(VersionUtilTest, TestVersion) { - const vector<Version> good_test_cases = { - { "0.0.0", 0, 0, 0, "" }, - { "1.0.0", 1, 0, 0, "" }, - { "1.1.0", 1, 1, 0, "" }, - { "1.1.1", 1, 1, 1, "" }, - { "1.10.100-1000", 1, 10, 100, "1000" }, - { "1.2.3-SNAPSHOT", 1, 2, 3, "SNAPSHOT" }, +TEST(VersionUtilTest, TestVersion) { + const vector<pair<Version, string>> good_test_cases = { + { { "0.0.0", 0, 0, 0, "" }, "0.0.0" }, + { { "1.0.0", 1, 0, 0, "" }, "1.0.0" }, + { { "1.1.0", 1, 1, 0, "" }, "1.1.0" }, + { { "1.1.1", 1, 1, 1, "" }, "1.1.1" }, + { { "1.1.1-", 1, 1, 1, "" }, "1.1.1" }, + { { "1.1.1-0-1-2", 1, 1, 1, "0-1-2" }, "1.1.1-0-1-2" }, + { { "1.10.100-1000.0", 1, 10, 100, "1000.0" }, "1.10.100-1000.0" }, + { { "1.2.3-SNAPSHOT", 1, 2, 3, "SNAPSHOT" }, "1.2.3-SNAPSHOT" }, + { { "1.8.0-x-SNAPSHOT", 1, 8, 0, "x-SNAPSHOT" }, "1.8.0-x-SNAPSHOT" }, + { { "0.1.2-a-b-c-d", 0, 1, 2, "a-b-c-d" }, "0.1.2-a-b-c-d" }, + { { " 0 .1 .2 -x", 0, 1, 2, "x" }, "0.1.2-x" }, + { { " 0 . 1 . 2 -x ", 0, 1, 2, "x" }, "0.1.2-x" }, + { { "+1.+2.+3-6", 1, 2, 3, "6" }, "1.2.3-6" }, + { { " +1 . +2 . +3 -100 .. ", 1, 2, 3, "100 .." }, "1.2.3-100 .." }, + // no octals: leading zeros are just chopped off + { { "00.01.010", 0, 1, 10, "" }, "0.1.10" }, + { { " 0.1.2----suffix ", 0, 1, 2, "---suffix" }, "0.1.2----suffix" }, + { { "0.1.2- - -x- -y- ", 0, 1, 2, " - -x- -y-" }, "0.1.2- - -x- -y-" }, }; - Version v; for (const auto& test_case : good_test_cases) { - ASSERT_OK(ParseVersion(test_case.raw_version, &v)); - EXPECT_EQ(test_case, v); + const auto& version = test_case.first; + const auto& canonical_str = test_case.second; + Version v; + ASSERT_OK(ParseVersion(version.raw_version, &v)); + EXPECT_EQ(version, v); + EXPECT_EQ(canonical_str, v.ToString()); } const vector<string> bad_test_cases = { "", + " ", + "-", + " -", + "--", + " - - ", + "..", + "0.1", + "0.1.", + "0.1.+", + "+ 0.+ 1.+ 2", + "0.1.+-woops", + "0.1.2+", + "1..1", + ".0.1", + " . . ", + "-0.1.2-013", + "0.-1.2-013", + "0.1.-2-013", + "1000,000.2999,999.999", + "1e10.0.1", + "1e+10.0.1", + "1e-1.0.1", + "1E+1.2.3", + "1E-5.0.1", "foo", "foo.1.0", + "a.b.c", + "0.x1.x2", + "0x0.0x1.0x2", "1.bar.0", "1.0.foo", "1.0foo.bar", "foo5-1.4.3", }; - for (const auto& test_case : bad_test_cases) { - ASSERT_TRUE(ParseVersion(test_case, &v).IsInvalidArgument()); + for (const auto& input_str : bad_test_cases) { + Version v; + const auto s = ParseVersion(input_str, &v); + ASSERT_TRUE(s.IsInvalidArgument()) + << s.ToString() << ": " << input_str << " ---> " << v.ToString(); } } +// Sanity check: parse current Kudu version string and make sure the 'canonical' +// representation of the parsed version matches the 'raw' input as is. +TEST(VersionUtilTest, ParseCurrentKuduVersionString) { + const auto ver_string = VersionInfo::GetShortVersionInfo(); + Version v; + const auto s = ParseVersion(ver_string, &v); + ASSERT_TRUE(s.ok()) << s.ToString(); + EXPECT_EQ(ver_string, v.ToString()); +} + } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/7e733a73/src/kudu/util/version_util.cc ---------------------------------------------------------------------- diff --git a/src/kudu/util/version_util.cc b/src/kudu/util/version_util.cc index bd298f8..6715894 100644 --- a/src/kudu/util/version_util.cc +++ b/src/kudu/util/version_util.cc @@ -17,15 +17,17 @@ #include "kudu/util/version_util.h" -#include <iostream> +#include <iterator> #include <string> #include <utility> #include <vector> #include <glog/logging.h> +#include "kudu/gutil/strings/join.h" #include "kudu/gutil/strings/numbers.h" #include "kudu/gutil/strings/split.h" +#include "kudu/gutil/strings/strip.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/util/status.h" @@ -45,7 +47,9 @@ bool Version::operator==(const Version& other) const { } string Version::ToString() const { - return raw_version; + return extra.empty() + ? Substitute("$0.$1.$2", major, minor, maintenance) + : Substitute("$0.$1.$2-$3", major, minor, maintenance, extra); } ostream& operator<<(ostream& os, const Version& v) { @@ -54,29 +58,32 @@ ostream& operator<<(ostream& os, const Version& v) { Status ParseVersion(const string& version_str, Version* v) { - CHECK(v); + static const char* const kDelimiter = "-"; + + DCHECK(v); const Status invalid_ver_err = Status::InvalidArgument("invalid version string", version_str); - Version temp_v; - const vector<string> main_and_extra = Split(version_str, "-"); - if (main_and_extra.empty() || main_and_extra.size() > 2) { + auto v_str = version_str; + StripWhiteSpace(&v_str); + const vector<string> main_and_extra = Split(v_str, kDelimiter); + if (main_and_extra.empty()) { return invalid_ver_err; } - if (main_and_extra.size() == 2) { - temp_v.extra = main_and_extra[1]; - } - const auto& main_ver_str = main_and_extra[0]; - const vector<string> maj_min_maint = Split(main_ver_str, "."); + const vector<string> maj_min_maint = Split(main_and_extra.front(), "."); if (maj_min_maint.size() != 3) { return invalid_ver_err; } + Version temp_v; if (!SimpleAtoi(maj_min_maint[0], &temp_v.major) || !SimpleAtoi(maj_min_maint[1], &temp_v.minor) || !SimpleAtoi(maj_min_maint[2], &temp_v.maintenance)) { return invalid_ver_err; } + temp_v.extra = JoinStringsIterator(std::next(main_and_extra.begin()), + main_and_extra.end(), kDelimiter); temp_v.raw_version = version_str; *v = std::move(temp_v); + return Status::OK(); } http://git-wip-us.apache.org/repos/asf/kudu/blob/7e733a73/src/kudu/util/version_util.h ---------------------------------------------------------------------- diff --git a/src/kudu/util/version_util.h b/src/kudu/util/version_util.h index 5cde6cc..446934c 100644 --- a/src/kudu/util/version_util.h +++ b/src/kudu/util/version_util.h @@ -16,9 +16,10 @@ // under the License. #pragma once -#include <iostream> +#include <iosfwd> #include <string> +#include "kudu/gutil/port.h" #include "kudu/util/status.h" namespace kudu { @@ -27,7 +28,7 @@ namespace kudu { // // <major>.<minor>.<maintenance>[-<extra>] // -// e.g. 1.6.0 or 1.7.1-SNAPSHOT. +// e.g. 1.6.0, 1.7.1-SNAPSHOT, 1.8.0-RC1-SNAPSHOT, etc. // // This struct can be used with versions reported by ksck to determine if and // how certain tools should function depending on what versions are running in @@ -35,6 +36,10 @@ namespace kudu { struct Version { bool operator==(const Version& other) const; + // Return 'canonical' version string, i.e. the concatenation of the version + // components transformed back into the string representation. The parser + // implementation has its quirks, so the canonical version string does not + // always match the raw input string. std::string ToString() const; // The original version string. @@ -53,6 +58,6 @@ std::ostream& operator<<(std::ostream& os, const Version& v); // Parse 'version_str' into 'v'. 'v' must not be null. Status ParseVersion(const std::string& version_str, - Version* v); + Version* v) WARN_UNUSED_RESULT; } // namespace kudu
