From 8377fb9e18f96728b080e36b68582d5023b5225a Mon Sep 17 00:00:00 2001 From: Radomir Dopieralski Date: Wed, 14 Dec 2016 11:53:57 +0100 Subject: [PATCH] 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 | 38 ++++++++++---- openstack_dashboard/api/rest/config.py | 2 +- .../test/api_tests/base_tests.py | 49 ++++++++++++++++++- requirements.txt | 1 + 4 files changed, 78 insertions(+), 12 deletions(-) diff --git a/openstack_dashboard/api/base.py b/openstack_dashboard/api/base.py index 30974a2d89..9baa51f9cf 100644 --- a/openstack_dashboard/api/base.py +++ b/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, - # 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) + version = Version(key) # 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): diff --git a/openstack_dashboard/api/rest/config.py b/openstack_dashboard/api/rest/config.py index 27bcf8875e..a216cd3496 100644 --- a/openstack_dashboard/api/rest/config.py +++ b/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) } diff --git a/openstack_dashboard/test/api_tests/base_tests.py b/openstack_dashboard/test/api_tests/base_tests.py index 7780573df3..57b1baa651 100644 --- a/openstack_dashboard/test/api_tests/base_tests.py +++ b/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: diff --git a/requirements.txt b/requirements.txt index 2c8e46aa90..a8202dfa89 100644 --- a/requirements.txt +++ b/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