[gutil] Disallow empty semantic version string. (sonic-net#181)

Co-authored-by: smolkaj <[email protected]> Co-authored-by: kishanps <[email protected]>
rkavitha-hcl · Jun 7, 2024 · aa191e2 · aa191e2
1 parent 46ab725
commit aa191e2
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 19 deletions.
diff --git a/gutil/version.cc b/gutil/version.cc
@@ -59,8 +59,6 @@ absl::StatusOr<Version> ParseVersion(absl::string_view version_string) {
   if (!RE2::FullMatch(version_string, *kSemanticVersionRegex,
                       &version.major_version, &version.minor_version,
                       &version.patch_version)) {
-    // TODO: Remove this hack once all P4Infos have versions.
-    if (version_string.empty()) return Version{0, 0, 0};
     return gutil::InvalidArgumentErrorBuilder()
            << "unable to parse '" << version_string
            << "' as a semantic version string; expected string of the form "

diff --git a/gutil/version_test.cc b/gutil/version_test.cc
@@ -21,8 +21,10 @@
 
 namespace gutil {
 namespace {
-using gutil::IsOkAndHolds;
-using testing::Eq;
+
+using ::gutil::IsOkAndHolds;
+using ::gutil::StatusIs;
+using ::testing::Eq;
 
 bool RoundTrips(const Version& version) {
   absl::StatusOr<Version> roundtripped_version =
@@ -39,17 +41,18 @@ TEST(ParseVersionTest, PositiveExamples) {
 
 TEST(ParseVersionTest, NegativeExamples) {
   EXPECT_THAT(ParseVersion("100"),
-              gutil::StatusIs(absl::StatusCode::kInvalidArgument));
+              StatusIs(absl::StatusCode::kInvalidArgument));
   EXPECT_THAT(ParseVersion("1.1"),
-              gutil::StatusIs(absl::StatusCode::kInvalidArgument));
+              StatusIs(absl::StatusCode::kInvalidArgument));
   EXPECT_THAT(ParseVersion("1.1.1."),
-              gutil::StatusIs(absl::StatusCode::kInvalidArgument));
+              StatusIs(absl::StatusCode::kInvalidArgument));
   EXPECT_THAT(ParseVersion("1.1.1.1"),
-              gutil::StatusIs(absl::StatusCode::kInvalidArgument));
+              StatusIs(absl::StatusCode::kInvalidArgument));
   EXPECT_THAT(ParseVersion("1.1.1,0"),
-              gutil::StatusIs(absl::StatusCode::kInvalidArgument));
+              StatusIs(absl::StatusCode::kInvalidArgument));
   EXPECT_THAT(ParseVersion("hello"),
-              gutil::StatusIs(absl::StatusCode::kInvalidArgument));
+              StatusIs(absl::StatusCode::kInvalidArgument));
+  EXPECT_THAT(ParseVersion(""), StatusIs(absl::StatusCode::kInvalidArgument));
 }
 
 TEST(VersionTest, VersionToStringAndParseVersionRoundTrip) {
@@ -66,10 +69,5 @@ TEST(ComparisonTest, OrderingIsLexicographic) {
   EXPECT_GT((Version{10, 0, 0}), (Version{2, 0, 0}));
 }
 
-// TODO: Remove this test once all P4Infos have versions.
-TEST(ParseVersionTest, EmptyStringForBackwardsCompatibility) {
-  EXPECT_THAT(ParseVersion(""), IsOkAndHolds(Eq(Version{0, 0, 0})));
-}
-
 }  // namespace
 }  // namespace gutil
diff --git a/sai_p4/instantiations/google/BUILD.bazel b/sai_p4/instantiations/google/BUILD.bazel
@@ -210,13 +210,13 @@ cc_library(
             ":clos_stage",
             ":instantiations",
             ":unioned_p4info_embed",
+            "//p4_pdpi:ir_cc_proto",
+            "//sai_p4/tools:p4info_tools",
             "@com_github_google_glog//:glog",
-            "@com_google_protobuf//:protobuf",
+            "@com_github_p4lang_p4runtime//:p4info_cc_proto",
             "@com_google_absl//absl/status",
             "@com_google_absl//absl/strings",
-            "@com_github_p4lang_p4runtime//:p4info_cc_proto",
-            "//p4_pdpi:ir_cc_proto",
-            "//sai_p4/tools:p4info_tools",
+            "@com_google_protobuf//:protobuf",
         ],
 )