From 01ab1abd39c3a8dda2dc99745e782e840c7f5b10 Mon Sep 17 00:00:00 2001
From: Pete Vander Giessen <petevg@gmail.com>
Date: Tue, 7 Mar 2017 10:30:00 -0600
Subject: [PATCH] Raise errors in a centralized fashion.

Fixes error with annotations that's exposed when we actually run tests
with the "raise errors" fix.

Added tests to verify that errors get raised.
---
 juju/application.py              |  2 +-
 juju/client/connection.py        | 26 ++++++++++++-
 juju/model.py                    |  8 +---
 tests/integration/test_errors.py | 65 ++++++++++++++++++++++++++++++++
 4 files changed, 91 insertions(+), 10 deletions(-)
 create mode 100644 tests/integration/test_errors.py

diff --git a/juju/application.py b/juju/application.py
index 9863d88..6b1f8ab 100644
--- a/juju/application.py
+++ b/juju/application.py
@@ -253,7 +253,7 @@ class Application(model.ModelEntity):
         self.ann_facade.connect(self.connection)
 
         ann = client.EntityAnnotations(
-            entity=self.name,
+            entity=self.tag,
             annotations=annotations,
         )
         return await self.ann_facade.Set([ann])
diff --git a/juju/client/connection.py b/juju/client/connection.py
index 9e8cb8f..f408135 100644
--- a/juju/client/connection.py
+++ b/juju/client/connection.py
@@ -14,7 +14,7 @@ from http.client import HTTPSConnection
 import yaml
 
 from juju import tag
-from juju.errors import JujuAPIError, JujuConnectionError
+from juju.errors import JujuAPIError, JujuConnectionError, JujuError
 
 log = logging.getLogger("websocket")
 
@@ -91,8 +91,30 @@ class Connection:
         outgoing = json.dumps(msg, indent=2, cls=encoder)
         await self.ws.send(outgoing)
         result = await self.recv()
-        if result and 'error' in result:
+
+        if not result:
+            return result
+
+        if 'error' in result:
+            # API Error Response
             raise JujuAPIError(result)
+
+        if not 'response' in result:
+            # This may never happen
+            return result
+
+        if 'results' in result['response']:
+            # Check for errors in a result list.
+            errors = []
+            for res in result['response']['results']:
+                if res.get('error', {}).get('message'):
+                    errors.append(res['error']['message'])
+            if errors:
+                raise JujuError(errors)
+
+        elif result['response'].get('error', {}).get('message'):
+            raise JujuError(result['response']['error']['message'])
+
         return result
 
     def http_headers(self):
diff --git a/juju/model.py b/juju/model.py
index 1106a16..548fc03 100644
--- a/juju/model.py
+++ b/juju/model.py
@@ -1042,10 +1042,7 @@ class Model(object):
             )
             app.placement = placement
 
-            result = await app_facade.Deploy([app])
-            errors = [r.error.message for r in result.results if r.error]
-            if errors:
-                raise JujuError('\n'.join(errors))
+            await app_facade.Deploy([app])
             return await self._wait_for_new('application', application_name)
 
     def destroy(self):
@@ -1512,9 +1509,6 @@ class BundleHandler(object):
         self.plan = await self.client_facade.GetBundleChanges(
             yaml.dump(self.bundle))
 
-        if self.plan.errors:
-            raise JujuError('\n'.join(self.plan.errors))
-
     async def execute_plan(self):
         for step in self.plan.changes:
             method = getattr(self, step.method)
diff --git a/tests/integration/test_errors.py b/tests/integration/test_errors.py
new file mode 100644
index 0000000..4a95a27
--- /dev/null
+++ b/tests/integration/test_errors.py
@@ -0,0 +1,65 @@
+import pytest
+
+from .. import base
+
+MB = 1
+GB = 1024
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_juju_api_error(event_loop):
+    '''
+    Verify that we raise a JujuAPIError for responses with an error in
+    a top level key (for completely invalid requests).
+
+    '''
+    from juju.errors import JujuAPIError
+
+    async with base.CleanModel() as model:
+        with pytest.raises(JujuAPIError):
+            await model.add_machine(constraints={'mem': 'foo'})
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_juju_error_in_results_list(event_loop):
+    '''
+    Replicate the code that caused
+    https://github.com/juju/python-libjuju/issues/67, and verify that
+    we get a JujuError instead of passing silently by the failure.
+
+    (We don't raise a JujuAPIError, because the request isn't
+    completely invalid -- it's just passing a tag that doesn't exist.)
+
+    This also verifies that we will raise a JujuError any time there
+    is an error in one of a list of results.
+
+    '''
+    from juju.errors import JujuError
+    from juju.client import client
+
+    async with base.CleanModel() as model:
+        # Replicate
+        ann_facade = client.AnnotationsFacade()
+        ann_facade.connect(model.connection)
+
+        ann = client.EntityAnnotations(
+            entity='badtag',
+            annotations={'gui-x': '1', 'gui-y': '1'},
+        )
+        with pytest.raises(JujuError):
+            return await ann_facade.Set([ann])
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_juju_error_in_result(event_loop):
+    '''
+    Verify that we raise a JujuError when appropraite when we are
+    looking at a single result coming back.
+
+    # TODO: write this!
+
+    '''
+    pass
-- 
2.25.1