Properly compare versions in APIVersionManager

Since the APIVersionManager stored the version numbers as ints and
floats, it was not possible to distinguish between version 1.2 and 1.20,
and also version 1.2 would be considered higher than version 1.13. With
the introduction of microversioning in some services, the version
numbers inflate quickly, making this a problem.

This patch wraps the version numbers in a Version object, that stores
the information as a semantic_version and correctly compares with other
Version objects, as well as ints, floats and strings.

It also removes the check for version being specified as a string, so
that it's possible to specify versions such as 1.20 without having to
explicitly create Version objects.

Change-Id: I0b0d87582d617290f08359ad181216cb99edb768
Closes-Bug: #1649819

openstack_dashboard/api/base.py

@@ -17,11 +17,13 @@
 #    under the License.
 
 from collections import Sequence  # noqa
+import functools
 
 from django.conf import settings
 
 from horizon import exceptions
 
+import semantic_version
 import six
 
 
@@ -29,6 +31,27 @@ __all__ = ('APIResourceWrapper', 'APIDictWrapper',
            'get_service_from_catalog', 'url_for',)
 
 
+@functools.total_ordering
+class Version(object):
+    def __init__(self, version):
+        self.version = semantic_version.Version(str(version), partial=True)
+
+    def __eq__(self, other):
+        return self.version == Version(other).version
+
+    def __lt__(self, other):
+        return self.version < Version(other).version
+
+    def __repr__(self):
+        return "Version('%s')" % self.version
+
+    def __str__(self):
+        return str(self.version)
+
+    def __hash__(self):
+        return hash(self.version)
+
+
 class APIVersionManager(object):
     """Object to store and manage API versioning data and utility methods."""
 
@@ -54,6 +77,7 @@ class APIVersionManager(object):
         return self._active
 
     def load_supported_version(self, version, data):
+        version = Version(version)
         self.supported[version] = data
 
     def get_active_version(self):
@@ -65,21 +89,15 @@ class APIVersionManager(object):
             # the setting in as a way of overriding the latest available
             # version.
             key = self.preferred
-        # Since we do a key lookup in the supported dict the type matters,
+        version = Version(key)
-        # let's ensure people know if they use a string when the key isn't.
-        if isinstance(key, six.string_types):
-            msg = ('The version "%s" specified for the %s service should be '
-                   'either an integer or a float, not a string.' %
-                   (key, self.service_type))
-            raise exceptions.ConfigurationError(msg)
         # Provide a helpful error message if the specified version isn't in the
         # supported list.
-        if key not in self.supported:
+        if version not in self.supported:
             choices = ", ".join(str(k) for k in six.iterkeys(self.supported))
             msg = ('%s is not a supported API version for the %s service, '
-                   ' choices are: %s' % (key, self.service_type, choices))
+                   ' choices are: %s' % (version, self.service_type, choices))
             raise exceptions.ConfigurationError(msg)
-        self._active = key
+        self._active = version
         return self.supported[self._active]
 
     def clear_active_cache(self):

openstack_dashboard/api/rest/config.py

@@ -41,7 +41,7 @@ class Settings(generic.View):
     url_regex = r'settings/$'
     SPECIALS = {
         'HORIZON_IMAGES_UPLOAD_MODE': api.glance.get_image_upload_mode(),
-        'HORIZON_ACTIVE_IMAGE_VERSION': api.glance.VERSIONS.active,
+        'HORIZON_ACTIVE_IMAGE_VERSION': str(api.glance.VERSIONS.active),
         'IMAGES_ALLOW_LOCATION': getattr(settings, 'IMAGES_ALLOW_LOCATION',
                                          False)
     }

openstack_dashboard/test/api_tests/base_tests.py

@@ -58,6 +58,51 @@ class APIDict(api_base.APIDictWrapper):
         return APIDict(innerDict)
 
 
+class APIVersionTests(test.TestCase):
+    def test_equal(self):
+        version = api_base.Version('1.0')
+        self.assertEqual(1, version)
+        self.assertEqual(1.0, version)
+        self.assertEqual('1', version)
+        self.assertEqual('1.0', version)
+        version = api_base.Version(1)
+        self.assertEqual(1, version)
+        self.assertEqual(1.0, version)
+        self.assertEqual('1', version)
+        self.assertEqual('1.0', version)
+        version = api_base.Version(1.0)
+        self.assertEqual(1, version)
+        self.assertEqual(1.0, version)
+        self.assertEqual('1', version)
+        self.assertEqual('1.0', version)
+        version = api_base.Version('1.0')
+        self.assertEqual(api_base.Version(1), version)
+        self.assertEqual(api_base.Version(1.0), version)
+        self.assertEqual(api_base.Version('1'), version)
+        self.assertEqual(api_base.Version('1.0'), version)
+
+    def test_greater(self):
+        version1 = api_base.Version('1.0')
+        version12 = api_base.Version('1.2')
+        version120 = api_base.Version('1.20')
+        self.assertGreater(version12, version1)
+        self.assertGreater(version120, version12)
+        self.assertEqual(version12, 1)  # sic!
+        self.assertGreater(1.2, version1)
+        self.assertGreater(version120, 1.2)
+        self.assertGreater('1.20', version12)
+
+    def test_dict(self):
+        version1 = api_base.Version('1.0')
+        version1b = api_base.Version('1.0')
+        self.assertIn(version1, {version1b: 1})
+
+    def test_text(self):
+        version1 = api_base.Version('1.0')
+        self.assertEqual("1.0", str(version1))
+        self.assertEqual("Version('1.0')", repr(version1))
+
+
 # Wrapper classes that only define _attrs don't need extra testing.
 class APIResourceWrapperTests(test.TestCase):
     def test_get_attribute(self):
@@ -175,8 +220,10 @@ class ApiVersionTests(test.TestCase):
         glance.VERSIONS.clear_active_cache()
 
     def test_invalid_versions(self):
-        with self.assertRaises(exceptions.ConfigurationError):
+        try:
             getattr(keystone.VERSIONS, 'active')
+        except exceptions.ConfigurationError:
+            self.fail("ConfigurationError raised inappropriately.")
         with self.assertRaises(exceptions.ConfigurationError):
             getattr(cinder.VERSIONS, 'active')
         try:

requirements.txt

@@ -36,6 +36,7 @@ python-novaclient!=7.0.0,>=6.0.0 # Apache-2.0
 python-swiftclient>=3.2.0 # Apache-2.0
 pytz>=2013.6 # MIT
 PyYAML>=3.10.0 # MIT
+semantic_version>=2.3.1 # BSD
 six>=1.9.0 # MIT
 XStatic>=1.0.0 # MIT License
 XStatic-Angular>=1.3.7 # MIT License