Repository: Azure/WALinuxAgent
Pull request #252 (2016-06-21 21:19:48)

Corrected FlexibleVersion comparison of prerelease tags
Signed-off-by: Brendan Dixon <brendand@microsoft.com>

msftclas: Hi __@brendandixon__, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
    <span>You've already signed the contribution license agreement. Thanks!</span>
        <p>The agreement was validated by Microsoft and real humans are currently evaluating your PR.</p>

TTYL, MSBOT;

hglkrijger: lgtm

Patch:
diff --git a/azurelinuxagent/common/utils/flexible_version.py b/azurelinuxagent/common/utils/flexible_version.py
index ab0d0ac6..bcac06b0 100644
--- a/azurelinuxagent/common/utils/flexible_version.py
+++ b/azurelinuxagent/common/utils/flexible_version.py
@@ -30,9 +30,7 @@ class FlexibleVersion(version.Version):
 
         self.sep = sep
         self.prerel_sep = ''
-        self.prerel_tags = list(prerel_tags)
-        self.prerel_tags.sort()
-        self.prerel_tags = tuple(self.prerel_tags)
+        self.prerel_tags = tuple(prerel_tags) if prerel_tags is not None else ()
 
         self._compile_pattern()
 
@@ -73,6 +71,9 @@ class FlexibleVersion(version.Version):
 
     def __sub__(self, decrement):
         version = list(self.version)
+        if version[-1] <= 0:
+            raise ArithmeticError("Cannot decrement final numeric component of {0} below zero" \
+                .format(self))
         version[-1] -= decrement
         vstring = self._assemble(version, self.sep, self.prerel_sep, self.prerelease)
         return FlexibleVersion(vstring=vstring, sep=self.sep, prerel_tags=self.prerel_tags)
@@ -83,7 +84,7 @@ class FlexibleVersion(version.Version):
                 cls=self.__class__.__name__,
                 vstring=str(self),
                 sep=self.sep,
-                prerel_tags = list(self.prerel_tags.keys()))
+                prerel_tags=self.prerel_tags)
 
     def __str__(self):
         return self._assemble(self.version, self.sep, self.prerel_sep, self.prerelease)
@@ -109,8 +110,8 @@ class FlexibleVersion(version.Version):
         if self.prerelease is None and that.prerelease is not None:
             return False
 
-        this_index = self.prerel_tags[self.prerelease[0]]
-        that_index = self.prerel_tags[that.prerelease[0]]
+        this_index = self.prerel_tags_set[self.prerelease[0]]
+        that_index = self.prerel_tags_set[that.prerelease[0]]
         if this_index == that_index:
             return self.prerelease[1] < that.prerelease[1]
 
@@ -122,12 +123,10 @@ class FlexibleVersion(version.Version):
         if this_version != that_version:
             return False
 
-        if self.prerelease is None and that.prerelease is None:
-            return True
+        if self.prerelease != that.prerelease:
+            return False
 
-        this_index = self.prerel_tags[self.prerelease[0]]
-        that_index = that.prerel_tags[that.prerelease[0]]
-        return self.prerelease[1] == that.prerelease[1] and this_index == that_index
+        return True
 
     def _assemble(self, version, sep, prerel_sep, prerelease):
         s = sep.join(map(str, version))
@@ -144,7 +143,7 @@ class FlexibleVersion(version.Version):
 
         if self.prerel_tags:
             tags = '|'.join(re.escape(tag) for tag in self.prerel_tags)
-            self.prerel_tags = dict(zip(self.prerel_tags, range(len(self.prerel_tags))))
+            self.prerel_tags_set = dict(zip(self.prerel_tags, range(len(self.prerel_tags))))
             release_re = '(?:{prerel_sep}(?P<{tn}>{tags})(?P<{nn}>\d*))?'.format(
                         prerel_sep=self._re_prerel_sep,
                         tags=tags,


Test patch:
diff --git a/tests/utils/test_flexible_version.py b/tests/utils/test_flexible_version.py
index 9ac62e90..6db4d76e 100644
--- a/tests/utils/test_flexible_version.py
+++ b/tests/utils/test_flexible_version.py
@@ -214,6 +214,62 @@ class TestFlexibleVersion(unittest.TestCase):
                 "test: {0} expected: {1} ".format(test, expectation))
         return
 
+    def test_ensure_compatible_separators(self):
+        v1 = FlexibleVersion('1.2.3')
+        v2 = FlexibleVersion('1-2-3', sep='-')
+        try:
+            v1 == v2
+            self.assertTrue(False, "Incompatible separators failed to raise an exception")
+        except ValueError:
+            pass
+        except Exception as e:
+            t = e.__class__.__name__
+            self.assertTrue(False, "Incompatible separators raised an unexpected exception: {0}" \
+                .format(t))
+        return
+
+    def test_ensure_compatible_prerel(self):
+        v1 = FlexibleVersion('1.2.3', prerel_tags=('alpha', 'beta', 'rc'))
+        v2 = FlexibleVersion('1.2.3', prerel_tags=('a', 'b', 'c'))
+        try:
+            v1 == v2
+            self.assertTrue(False, "Incompatible prerel_tags failed to raise an exception")
+        except ValueError:
+            pass
+        except Exception as e:
+            t = e.__class__.__name__
+            self.assertTrue(False, "Incompatible prerel_tags raised an unexpected exception: {0}" \
+                .format(t))
+        return
+
+    def test_ensure_compatible_prerel_length(self):
+        v1 = FlexibleVersion('1.2.3', prerel_tags=('a', 'b', 'c'))
+        v2 = FlexibleVersion('1.2.3', prerel_tags=('a', 'b'))
+        try:
+            v1 == v2
+            self.assertTrue(False, "Incompatible prerel_tags failed to raise an exception")
+        except ValueError:
+            pass
+        except Exception as e:
+            t = e.__class__.__name__
+            self.assertTrue(False, "Incompatible prerel_tags raised an unexpected exception: {0}" \
+                .format(t))
+        return
+
+    def test_ensure_compatible_prerel_order(self):
+        v1 = FlexibleVersion('1.2.3', prerel_tags=('a', 'b'))
+        v2 = FlexibleVersion('1.2.3', prerel_tags=('b', 'a'))
+        try:
+            v1 == v2
+            self.assertTrue(False, "Incompatible prerel_tags failed to raise an exception")
+        except ValueError:
+            pass
+        except Exception as e:
+            t = e.__class__.__name__
+            self.assertTrue(False, "Incompatible prerel_tags raised an unexpected exception: {0}" \
+                .format(t))
+        return
+
     def test_parse(self):
         tests = {
             "1.2.3.4": ((1, 2, 3, 4), None),
@@ -235,6 +291,17 @@ class TestFlexibleVersion(unittest.TestCase):
             self.assertEqual(i, src_v.version[-1] - dst_v.version[-1])
         return
 
+    def test_decrement_disallows_below_zero(self):
+        try:
+            FlexibleVersion('1.0') - 1
+            self.assertTrue(False, "Decrement failed to raise an exception")
+        except ArithmeticError:
+            pass
+        except Exception as e:
+            t = e.__class__.__name__
+            self.assertTrue(False, "Decrement raised an unexpected exception: {0}".format(t))
+        return
+
     def test_increment(self):
         src_v = FlexibleVersion('1.0.0.0.0')
         dst_v = FlexibleVersion(str(src_v))
@@ -298,7 +365,7 @@ class TestFlexibleVersion(unittest.TestCase):
 
     def test_repr(self):
         v = FlexibleVersion('1,2,3rc4', ',', ['lol', 'rc'])
-        expected = "FlexibleVersion ('1,2,3rc4', ',', ['lol', 'rc'])"
+        expected = "FlexibleVersion ('1,2,3rc4', ',', ('lol', 'rc'))"
         self.assertEqual(expected, repr(v))
 
     def test_order(self):