From 7a09f727f3027e4f0d54d99f1579f6595720d094 Mon Sep 17 00:00:00 2001 From: EdLeafe Date: Thu, 27 Aug 2015 21:54:12 +0000 Subject: [PATCH] Replace os.path.join() for URLs Since os.path.join() is OS-dependent, it should not be used for creating URLs. This patch replaces the use of os.path.join() in nova/api/openstack with common.url_join(), which uses the more correct "/".join(), while preserving the behavior of removing duplicate slashes inside the URL and adding a trailing slash with an empty final element. It also adds some tests to ensure that the generate_href() method in nova/api/openstack/compute/views/versions.py works after the refactoring to use common.url_join() Closes-Bug: 1489627 Change-Id: I32948dd1fcf0839b34e446d9e4b08f9c39d17c8f --- nova/api/openstack/common.py | 41 ++++++++++++------- nova/api/openstack/compute/create_backup.py | 5 +-- .../legacy_v2/contrib/admin_actions.py | 4 +- .../openstack/compute/legacy_v2/servers.py | 9 ++-- nova/api/openstack/compute/views/versions.py | 8 +--- .../api/openstack/compute/test_versions.py | 16 ++++++++ nova/tests/unit/api/openstack/test_common.py | 32 +++++++++++++++ 7 files changed, 85 insertions(+), 30 deletions(-) diff --git a/nova/api/openstack/common.py b/nova/api/openstack/common.py index 00693efbf026..e32ac94a7bf5 100644 --- a/nova/api/openstack/common.py +++ b/nova/api/openstack/common.py @@ -16,7 +16,6 @@ import collections import functools import itertools -import os import re from oslo_config import cfg @@ -299,8 +298,7 @@ def remove_trailing_version_from_href(href): LOG.debug('href %s does not contain version' % href) raise ValueError(_('href %s does not contain version') % href) - new_path = '/'.join(url_parts) - + new_path = url_join(*url_parts) parsed_url = list(parsed_url) parsed_url[2] = new_path return urlparse.urlunsplit(parsed_url) @@ -398,6 +396,21 @@ def check_snapshots_enabled(f): return inner +def url_join(*parts): + """Convenience method for joining parts of a URL + + Any leading and trailing '/' characters are removed, and the parts joined + together with '/' as a separator. If last element of 'parts' is an empty + string, the returned URL will have a trailing slash. + """ + parts = parts or [""] + clean_parts = [part.strip("/") for part in parts if part] + if not parts[-1]: + # Empty last element should add a trailing slash + clean_parts.append("") + return "/".join(clean_parts) + + class ViewBuilder(object): """Model API responses as dictionaries.""" @@ -427,27 +440,27 @@ class ViewBuilder(object): params = request.params.copy() params["marker"] = identifier prefix = self._update_compute_link_prefix(request.application_url) - url = os.path.join(prefix, - self._get_project_id(request), - collection_name) + url = url_join(prefix, + self._get_project_id(request), + collection_name) return "%s?%s" % (url, urlparse.urlencode(params)) def _get_href_link(self, request, identifier, collection_name): """Return an href string pointing to this object.""" prefix = self._update_compute_link_prefix(request.application_url) - return os.path.join(prefix, - self._get_project_id(request), - collection_name, - str(identifier)) + return url_join(prefix, + self._get_project_id(request), + collection_name, + str(identifier)) def _get_bookmark_link(self, request, identifier, collection_name): """Create a URL that refers to a specific resource.""" base_url = remove_trailing_version_from_href(request.application_url) base_url = self._update_compute_link_prefix(base_url) - return os.path.join(base_url, - self._get_project_id(request), - collection_name, - str(identifier)) + return url_join(base_url, + self._get_project_id(request), + collection_name, + str(identifier)) def _get_collection_links(self, request, diff --git a/nova/api/openstack/compute/create_backup.py b/nova/api/openstack/compute/create_backup.py index 44b83a1cef57..b78f2a584cd1 100644 --- a/nova/api/openstack/compute/create_backup.py +++ b/nova/api/openstack/compute/create_backup.py @@ -13,8 +13,6 @@ # License for the specific language governing permissions and limitations # under the License. -import os.path - import webob from nova.api.openstack import common @@ -80,7 +78,8 @@ class CreateBackupController(wsgi.Controller): # build location of newly-created image entity if rotation is not zero if rotation > 0: image_id = str(image['id']) - image_ref = os.path.join(req.application_url, 'images', image_id) + image_ref = common.url_join(req.application_url, 'images', + image_id) resp.headers['Location'] = image_ref return resp diff --git a/nova/api/openstack/compute/legacy_v2/contrib/admin_actions.py b/nova/api/openstack/compute/legacy_v2/contrib/admin_actions.py index 367b83198a11..19d0043d7866 100644 --- a/nova/api/openstack/compute/legacy_v2/contrib/admin_actions.py +++ b/nova/api/openstack/compute/legacy_v2/contrib/admin_actions.py @@ -12,7 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -import os.path import traceback from oslo_log import log as logging @@ -292,7 +291,8 @@ class AdminActionsController(wsgi.Controller): # build location of newly-created image entity if rotation is not zero if rotation > 0: image_id = str(image['id']) - image_ref = os.path.join(req.application_url, 'images', image_id) + image_ref = common.url_join(req.application_url, 'images', + image_id) resp.headers['Location'] = image_ref return resp diff --git a/nova/api/openstack/compute/legacy_v2/servers.py b/nova/api/openstack/compute/legacy_v2/servers.py index 72dbfd4559fa..4431def6479b 100644 --- a/nova/api/openstack/compute/legacy_v2/servers.py +++ b/nova/api/openstack/compute/legacy_v2/servers.py @@ -15,7 +15,6 @@ # under the License. import base64 -import os import re import sys @@ -1113,10 +1112,10 @@ class Controller(wsgi.Controller): image_id = str(image['id']) url_prefix = self._view_builder._update_glance_link_prefix( req.application_url) - image_ref = os.path.join(url_prefix, - context.project_id, - 'images', - image_id) + image_ref = common.url_join(url_prefix, + context.project_id, + 'images', + image_id) resp = webob.Response(status_int=202) resp.headers['Location'] = image_ref diff --git a/nova/api/openstack/compute/views/versions.py b/nova/api/openstack/compute/views/versions.py index 0ee96bf0be83..6ce0d11c2952 100644 --- a/nova/api/openstack/compute/views/versions.py +++ b/nova/api/openstack/compute/views/versions.py @@ -14,7 +14,6 @@ # under the License. import copy -import os from nova.api.openstack import common @@ -92,8 +91,5 @@ class ViewBuilder(common.ViewBuilder): else: version_number = 'v2' - if path: - path = path.strip('/') - return os.path.join(self.prefix, version_number, path) - else: - return os.path.join(self.prefix, version_number) + '/' + path = path or '' + return common.url_join(self.prefix, version_number, path) diff --git a/nova/tests/unit/api/openstack/compute/test_versions.py b/nova/tests/unit/api/openstack/compute/test_versions.py index 7a85ecdcbfae..22a8581094ab 100644 --- a/nova/tests/unit/api/openstack/compute/test_versions.py +++ b/nova/tests/unit/api/openstack/compute/test_versions.py @@ -419,6 +419,22 @@ class VersionsViewBuilderTests(test.NoDBTestCase): self.assertEqual(expected, actual) + def test_generate_href_with_path(self): + path = "random/path" + base_url = "http://example.org/app/" + expected = "http://example.org/app/v2/%s" % path + builder = views.versions.ViewBuilder(base_url) + actual = builder.generate_href("v2", path) + self.assertEqual(actual, expected) + + def test_generate_href_with_empty_path(self): + path = "" + base_url = "http://example.org/app/" + expected = "http://example.org/app/v2/" + builder = views.versions.ViewBuilder(base_url) + actual = builder.generate_href("v2", path) + self.assertEqual(actual, expected) + # NOTE(oomichi): Now version API of v2.0 covers "/"(root). # So this class tests "/v2.1" only for v2.1 API. diff --git a/nova/tests/unit/api/openstack/test_common.py b/nova/tests/unit/api/openstack/test_common.py index e368918690a3..246affbd196d 100644 --- a/nova/tests/unit/api/openstack/test_common.py +++ b/nova/tests/unit/api/openstack/test_common.py @@ -579,6 +579,38 @@ class LinkPrefixTest(test.NoDBTestCase): result) +class UrlJoinTest(test.NoDBTestCase): + def test_url_join(self): + pieces = ["one", "two", "three"] + joined = common.url_join(*pieces) + self.assertEqual("one/two/three", joined) + + def test_url_join_extra_slashes(self): + pieces = ["one/", "/two//", "/three/"] + joined = common.url_join(*pieces) + self.assertEqual("one/two/three", joined) + + def test_url_join_trailing_slash(self): + pieces = ["one", "two", "three", ""] + joined = common.url_join(*pieces) + self.assertEqual("one/two/three/", joined) + + def test_url_join_empty_list(self): + pieces = [] + joined = common.url_join(*pieces) + self.assertEqual("", joined) + + def test_url_join_single_empty_string(self): + pieces = [""] + joined = common.url_join(*pieces) + self.assertEqual("", joined) + + def test_url_join_single_slash(self): + pieces = ["/"] + joined = common.url_join(*pieces) + self.assertEqual("", joined) + + class ViewBuilderLinkTest(test.NoDBTestCase): project_id = "fake" api_version = "2.1"