openstack/deb-heat
Code Issues Proposed changes

Tidy up urlfetch.py exception handling

Browse Source 
IOError doesn't seem appropiate for things like invalid URL scheme
and template exceeding max size. And requests.RequestException means
an ambiguous exception occurred while handling requests, not
necessarily an IO error.

Change-Id: Ib8108a5b9bf6d6c99545a1c29735521720d3a75a
This commit is contained in:
Liang Chen 2014-04-08 13:56:50 +08:00
parent d46adc1d53
commit a2d04996ae
4 changed files with 27 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
heat/common/urlfetch.py
View File

@ -21,6 +21,7 @@ from requests import exceptions
from six.moves import urllib from six.moves import urllib
from heat.common import exception
from heat.openstack.common.gettextutils import _ from heat.openstack.common.gettextutils import _
from heat.openstack.common import log as logging from heat.openstack.common import log as logging
@ -29,6 +30,10 @@ cfg.CONF.import_opt('max_template_size', 'heat.common.config')
logger = logging.getLogger(__name__) logger = logging.getLogger(__name__)
class URLFetchError(exception.Error, IOError):
pass
def get(url, allowed_schemes=('http', 'https')): def get(url, allowed_schemes=('http', 'https')):
''' '''
Get the data at the specifier URL. Get the data at the specifier URL.
@ -43,13 +48,13 @@ def get(url, allowed_schemes=('http', 'https')):
components = urllib.parse.urlparse(url) components = urllib.parse.urlparse(url)
if components.scheme not in allowed_schemes: if components.scheme not in allowed_schemes:
raise IOError(_('Invalid URL scheme %s') % components.scheme) raise URLFetchError(_('Invalid URL scheme %s') % components.scheme)
if components.scheme == 'file': if components.scheme == 'file':
try: try:
return urllib.request.urlopen(url).read() return urllib.request.urlopen(url).read()
except urllib.error.URLError as uex: except urllib.error.URLError as uex:
raise IOError(_('Failed to retrieve template: %s') % uex) raise URLFetchError(_('Failed to retrieve template: %s') % uex)
try: try:
resp = requests.get(url, stream=True) resp = requests.get(url, stream=True)
@ -68,9 +73,9 @@ def get(url, allowed_schemes=('http', 'https')):
for chunk in reader: for chunk in reader:
result += chunk result += chunk
if len(result) > cfg.CONF.max_template_size: if len(result) > cfg.CONF.max_template_size:
raise IOError("Template exceeds maximum allowed size (%s " raise URLFetchError("Template exceeds maximum allowed size (%s"
"bytes)" % cfg.CONF.max_template_size) " bytes)" % cfg.CONF.max_template_size)
return result return result
except exceptions.RequestException as ex: except exceptions.RequestException as ex:
raise IOError(_('Failed to retrieve template: %s') % ex) raise URLFetchError(_('Failed to retrieve template: %s') % ex)

3
heat/tests/test_nested_stack.py
View File

@ -629,7 +629,8 @@ Resources:
@mock.patch.object(urlfetch, 'get') @mock.patch.object(urlfetch, 'get')
def test_child_template_when_io_error(self, mock_get): def test_child_template_when_io_error(self, mock_get):
mock_get.side_effect = IOError() msg = 'Failed to retrieve template'
mock_get.side_effect = urlfetch.URLFetchError(msg)
t = template_format.parse(self.test_template) t = template_format.parse(self.test_template)
stack = self.parse_stack(t) stack = self.parse_stack(t)
nested_stack = stack['the_nested'] nested_stack = stack['the_nested']

9
heat/tests/test_provider_template.py
View File

@ -394,7 +394,8 @@ class ProviderTemplateTest(HeatTestCase):
self.assertTrue(test_templ, "Empty test template") self.assertTrue(test_templ, "Empty test template")
self.m.StubOutWithMock(urlfetch, "get") self.m.StubOutWithMock(urlfetch, "get")
urlfetch.get(test_templ_name, urlfetch.get(test_templ_name,
allowed_schemes=('file',)).AndRaise(IOError) allowed_schemes=('file',))\
.AndRaise(urlfetch.URLFetchError(_('Failed to retrieve template')))
urlfetch.get(test_templ_name, urlfetch.get(test_templ_name,
allowed_schemes=('http', 'https')).AndReturn(test_templ) allowed_schemes=('http', 'https')).AndReturn(test_templ)
parsed_test_templ = template_format.parse(test_templ) parsed_test_templ = template_format.parse(test_templ)
@ -499,7 +500,8 @@ class ProviderTemplateTest(HeatTestCase):
self.m.StubOutWithMock(urlfetch, "get") self.m.StubOutWithMock(urlfetch, "get")
urlfetch.get(test_templ_name, urlfetch.get(test_templ_name,
allowed_schemes=('http', 'https', allowed_schemes=('http', 'https',
'file')).AndRaise(IOError) 'file'))\
.AndRaise(urlfetch.URLFetchError(_('Failed to retrieve template')))
self.m.ReplayAll() self.m.ReplayAll()
temp_res = template_resource.TemplateResource('test_t_res', temp_res = template_resource.TemplateResource('test_t_res',
@ -524,7 +526,8 @@ class ProviderTemplateTest(HeatTestCase):
self.m.StubOutWithMock(urlfetch, "get") self.m.StubOutWithMock(urlfetch, "get")
urlfetch.get(test_templ_name, urlfetch.get(test_templ_name,
allowed_schemes=('http', 'https')).AndRaise(IOError) allowed_schemes=('http', 'https'))\
.AndRaise(urlfetch.URLFetchError(_('Failed to retrieve template')))
self.m.ReplayAll() self.m.ReplayAll()
temp_res = template_resource.TemplateResource('test_t_res', temp_res = template_resource.TemplateResource('test_t_res',

15
heat/tests/test_urlfetch.py
View File

@ -41,7 +41,8 @@ class UrlFetchTest(HeatTestCase):
def test_file_scheme_default_behaviour(self): def test_file_scheme_default_behaviour(self):
self.m.ReplayAll() self.m.ReplayAll()
self.assertRaises(IOError, urlfetch.get, 'file:///etc/profile') self.assertRaises(urlfetch.URLFetchError,
urlfetch.get, 'file:///etc/profile')
self.m.VerifyAll() self.m.VerifyAll()
def test_file_scheme_supported(self): def test_file_scheme_supported(self):
@ -62,7 +63,8 @@ class UrlFetchTest(HeatTestCase):
urllib.request.urlopen(url).AndRaise(urllib.error.URLError('oops')) urllib.request.urlopen(url).AndRaise(urllib.error.URLError('oops'))
self.m.ReplayAll() self.m.ReplayAll()
self.assertRaises(IOError, urlfetch.get, url, allowed_schemes=['file']) self.assertRaises(urlfetch.URLFetchError,
urlfetch.get, url, allowed_schemes=['file'])
self.m.VerifyAll() self.m.VerifyAll()
def test_http_scheme(self): def test_http_scheme(self):
@ -89,7 +91,7 @@ class UrlFetchTest(HeatTestCase):
requests.get(url, stream=True).AndRaise(exceptions.HTTPError()) requests.get(url, stream=True).AndRaise(exceptions.HTTPError())
self.m.ReplayAll() self.m.ReplayAll()
self.assertRaises(IOError, urlfetch.get, url) self.assertRaises(urlfetch.URLFetchError, urlfetch.get, url)
self.m.VerifyAll() self.m.VerifyAll()
def test_non_exist_url(self): def test_non_exist_url(self):
@ -98,12 +100,12 @@ class UrlFetchTest(HeatTestCase):
requests.get(url, stream=True).AndRaise(exceptions.Timeout()) requests.get(url, stream=True).AndRaise(exceptions.Timeout())
self.m.ReplayAll() self.m.ReplayAll()
self.assertRaises(IOError, urlfetch.get, url) self.assertRaises(urlfetch.URLFetchError, urlfetch.get, url)
self.m.VerifyAll() self.m.VerifyAll()
def test_garbage(self): def test_garbage(self):
self.m.ReplayAll() self.m.ReplayAll()
self.assertRaises(IOError, urlfetch.get, 'wibble') self.assertRaises(urlfetch.URLFetchError, urlfetch.get, 'wibble')
self.m.VerifyAll() self.m.VerifyAll()
def test_max_fetch_size_okay(self): def test_max_fetch_size_okay(self):
@ -123,6 +125,7 @@ class UrlFetchTest(HeatTestCase):
cfg.CONF.set_override('max_template_size', 5) cfg.CONF.set_override('max_template_size', 5)
requests.get(url, stream=True).AndReturn(response) requests.get(url, stream=True).AndReturn(response)
self.m.ReplayAll() self.m.ReplayAll()
exception = self.assertRaises(IOError, urlfetch.get, url) exception = self.assertRaises(urlfetch.URLFetchError,
urlfetch.get, url)
self.assertIn("Template exceeds", str(exception)) self.assertIn("Template exceeds", str(exception))
self.m.VerifyAll() self.m.VerifyAll()