From b54d03a4c045037ba19d9d3ff678dc5655e9dfc9 Mon Sep 17 00:00:00 2001
From: Eric Fried <openstack@fried.cc>
Date: Tue, 24 Sep 2019 08:41:49 -0500
Subject: [PATCH] Make proxy honor raise_exc in REST primitives

Previously, invoking a REST primitive (.get(), .put(), etc.) on a proxy
would ignore the raise_exc kwarg and always use raise_exc=False, causing
any error coming from the actual service [1] to return a Response object
even for responses with status >=400.

With this change, the raise_exc kwarg is honored: when True, REST
primitives with status >=400 will cause an appropriate ksa exception to
be raised.

[1] as opposed to lower level e.g. communication errors, which would
still raise

Change-Id: I463e9a63760a6e61827ba957dd9e5d23bd79f4e8
---
 lower-constraints.txt                       |  1 +
 openstack/proxy.py                          |  2 +-
 openstack/tests/unit/test_placement_rest.py | 49 ++++++++++++++++-----
 test-requirements.txt                       |  1 +
 4 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/lower-constraints.txt b/lower-constraints.txt
index b66d850be..10cae5083 100644
--- a/lower-constraints.txt
+++ b/lower-constraints.txt
@@ -1,6 +1,7 @@
 appdirs==1.3.0
 coverage==4.0
 cryptography==2.1
+ddt==1.0.1
 decorator==3.4.0
 doc8==0.8.0
 dogpile.cache==0.6.2
diff --git a/openstack/proxy.py b/openstack/proxy.py
index 8849efb4c..724a816d6 100644
--- a/openstack/proxy.py
+++ b/openstack/proxy.py
@@ -93,7 +93,7 @@ class Proxy(adapter.Adapter):
                 global_request_id = conn._global_request_id
         response = super(Proxy, self).request(
             url, method,
-            connect_retries=connect_retries, raise_exc=False,
+            connect_retries=connect_retries, raise_exc=raise_exc,
             global_request_id=global_request_id,
             **kwargs)
         for h in response.history:
diff --git a/openstack/tests/unit/test_placement_rest.py b/openstack/tests/unit/test_placement_rest.py
index e0c046166..b82b31d52 100644
--- a/openstack/tests/unit/test_placement_rest.py
+++ b/openstack/tests/unit/test_placement_rest.py
@@ -12,29 +12,58 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import ddt
+from keystoneauth1 import exceptions
+
 from openstack.tests.unit import base
 
 
+@ddt.ddt
 class TestPlacementRest(base.TestCase):
 
     def setUp(self):
         super(TestPlacementRest, self).setUp()
         self.use_placement()
 
-    def test_discovery(self):
-        self.register_uris([
-            dict(method='GET',
-                 uri=self.get_mock_url(
-                     'placement', 'public', append=['allocation_candidates']),
-                 json={})
-        ])
-        rs = self.cloud.placement.get('/allocation_candidates')
-        self.assertEqual(200, rs.status_code)
+    def _register_uris(self, status_code=None):
+        uri = dict(
+            method='GET',
+            uri=self.get_mock_url(
+                'placement', 'public', append=['allocation_candidates']),
+            json={})
+        if status_code is not None:
+            uri['status_code'] = status_code
+        self.register_uris([uri])
+
+    def _validate_resp(self, resp, status_code):
+        self.assertEqual(status_code, resp.status_code)
         self.assertEqual(
             'https://placement.example.com/allocation_candidates',
-            rs.url)
+            resp.url)
         self.assert_calls()
 
+    @ddt.data({}, {'raise_exc': False}, {'raise_exc': True})
+    def test_discovery(self, get_kwargs):
+        self._register_uris()
+        # Regardless of raise_exc, a <400 response doesn't raise
+        rs = self.cloud.placement.get('/allocation_candidates', **get_kwargs)
+        self._validate_resp(rs, 200)
+
+    @ddt.data({}, {'raise_exc': False})
+    def test_discovery_err(self, get_kwargs):
+        self._register_uris(status_code=500)
+        # >=400 doesn't raise by default or with explicit raise_exc=False
+        rs = self.cloud.placement.get('/allocation_candidates', **get_kwargs)
+        self._validate_resp(rs, 500)
+
+    def test_discovery_exc(self):
+        self._register_uris(status_code=500)
+        # raise_exc=True raises a ksa exception appropriate to the status code
+        ex = self.assertRaises(
+            exceptions.InternalServerError,
+            self.cloud.placement.get, '/allocation_candidates', raise_exc=True)
+        self._validate_resp(ex.response, 500)
+
     def test_microversion_discovery(self):
         self.assertEqual(
             (1, 17),
diff --git a/test-requirements.txt b/test-requirements.txt
index feb8d781c..ce8b81d83 100644
--- a/test-requirements.txt
+++ b/test-requirements.txt
@@ -4,6 +4,7 @@
 hacking>=1.0,<1.2 # Apache-2.0
 
 coverage!=4.4,>=4.0 # Apache-2.0
+ddt>=1.0.1 # MIT
 extras>=1.0.0 # MIT
 fixtures>=3.0.0 # Apache-2.0/BSD
 jsonschema>=2.6.0 # MIT