Repository: odin-detector/odin-control

Issue:
Proxy Adapter does not handle Metadata
The proxy adapter does not pass any request for metadata on to the Proxy Target. It should handle requests for metadata as if it were a normal local request

Discussion:

Patch:
diff --git a/src/odin/adapters/adapter.py b/src/odin/adapters/adapter.py
index 9bfe128..c20933d 100644
--- a/src/odin/adapters/adapter.py
+++ b/src/odin/adapters/adapter.py
@@ -199,7 +199,6 @@ def request_types(*oargs):
             """Inner method wrapper."""
             # Validate the Content-Type header in the request against allowed types
             if 'Content-Type' in request.headers:
-                logging.debug("Request content type: %s", request.headers['Content-Type'])
                 if request.headers['Content-Type'] not in oargs:
                     return ApiAdapterResponse(
                         'Request content type ({}) not supported'.format(
diff --git a/src/odin/adapters/proxy.py b/src/odin/adapters/proxy.py
index ed9b46a..8391f5f 100644
--- a/src/odin/adapters/proxy.py
+++ b/src/odin/adapters/proxy.py
@@ -10,9 +10,12 @@ import logging
 import time
 import tornado
 import tornado.httpclient
-from tornado.escape import json_decode
+from tornado.escape import json_encode
+from odin.util import decode_request_body
 
-from odin.adapters.adapter import ApiAdapter, ApiAdapterResponse, request_types, response_types
+from odin.adapters.adapter import (
+    ApiAdapter, ApiAdapterResponse,
+    request_types, response_types, wants_metadata)
 from odin.adapters.parameter_tree import ParameterTree, ParameterTreeError
 
 TIMEOUT_CONFIG_NAME = 'request_timeout'
@@ -44,18 +47,20 @@ class ProxyTarget(object):
         self.error_string = 'OK'
         self.last_update = 'unknown'
         self.data = {}
+        self.metadata = {}
         self.counter = 0
 
         # Build a parameter tree representation of the proxy target status
         self.status_param_tree = ParameterTree({
-            'url': (self._get_url, None),
-            'status_code': (self._get_status_code, None),
-            'error': (self._get_error_string, None),
-            'last_update': (self._get_last_update, None),
+            'url': (lambda: self.url, None),
+            'status_code': (lambda: self.status_code, None),
+            'error': (lambda: self.error_string, None),
+            'last_update': (lambda: self.last_update, None),
         })
 
         # Build a parameter tree representation of the proxy target data
-        self.data_param_tree = ParameterTree((self._get_data, None))
+        self.data_param_tree = ParameterTree((lambda: self.data, None))
+        self.meta_param_tree = ParameterTree((lambda: self.metadata, None))
 
         # Create an HTTP client instance and set up default request headers
         self.http_client = tornado.httpclient.HTTPClient()
@@ -63,10 +68,10 @@ class ProxyTarget(object):
             'Content-Type': 'application/json',
             'Accept': 'application/json',
         }
-
         self.remote_get()  # init the data tree
+        self.remote_get(get_metadata=True)  # init the metadata
 
-    def update(self, request, path):
+    def update(self, request, path, get_metadata=False):
         """
         Update the Proxy Target `ParameterTree` with data from the proxied adapter,
         after issuing a GET or a PUT request to it. It also updates the status code
@@ -87,27 +92,41 @@ class ProxyTarget(object):
             self.status_code = http_err.code
             self.error_string = http_err.message
             logging.error(
-                "Proxy target %s fetch failed: %d %s",
+                "HTTP Error: Proxy target %s fetch failed: %d %s Request: %s",
+                self.name,
+                self.status_code,
+                self.error_string,
+                request.body
+            )
+            self.last_update = tornado.httputil.format_timestamp(time.time())
+            return
+        except tornado.ioloop.TimeoutError as time_err:
+            self.status_code = 408
+            self.error_string = str(time_err)
+            logging.error(
+                "Timeout Error: Proxy Target %s fetch failed: %d %s",
                 self.name,
                 self.status_code,
                 self.error_string
-                )
+            )
             self.last_update = tornado.httputil.format_timestamp(time.time())
             return
-
         except IOError as other_err:
             self.status_code = 502
             self.error_string = str(other_err)
             logging.error(
-                "Proxy target %s fetch failed: %d %s",
+                "IO Error: Proxy Target %s fetch failed: %d %s",
                 self.name,
                 self.status_code,
                 self.error_string
-                )
+            )
             self.last_update = tornado.httputil.format_timestamp(time.time())
             return
 
-        data_ref = self.data  # reference for modification
+        if get_metadata:
+            data_ref = self.metadata
+        else:
+            data_ref = self.data  # reference for modification
         if path:
             # if the path exists, we need to split it so we can navigate the data
             path_elems = path.split('/')
@@ -120,17 +139,11 @@ class ProxyTarget(object):
         for key in response_body:
             new_elem = response_body[key]
             data_ref[key] = new_elem
-        logging.debug(
-            "Proxy target %s fetch succeeded: %d %s",
-            self.name,
-            self.status_code,
-            self.data_param_tree.get(path)
-            )
 
         # Update the timestamp of the last request in standard format
         self.last_update = tornado.httputil.format_timestamp(time.time())
 
-    def remote_get(self, path=''):
+    def remote_get(self, path='', get_metadata=False):
         """
         Get data from the remote target.
 
@@ -143,10 +156,12 @@ class ProxyTarget(object):
         request = tornado.httpclient.HTTPRequest(
             url=self.url + path,
             method="GET",
-            headers=self.request_headers,
+            headers=self.request_headers.copy(),
             request_timeout=self.request_timeout
         )
-        self.update(request, path)
+        if get_metadata:
+            request.headers["Accept"] += ";metadata=True"
+        self.update(request, path, get_metadata)
 
     def remote_set(self, path, data):
         """
@@ -157,6 +172,8 @@ class ProxyTarget(object):
         target data and status information according to the response.
         """
         # create request to PUT data, send to the target
+        if isinstance(data, dict):
+            data = json_encode(data)
         request = tornado.httpclient.HTTPRequest(
             url=self.url + path,
             method="PUT",
@@ -166,48 +183,6 @@ class ProxyTarget(object):
         )
         self.update(request, path)
 
-    def _get_status_code(self):
-        """
-        Get the target request status code.
-
-        This internal method is used to retrieve the status code
-        of the last target update request for use in the parameter
-        tree.
-        """
-        return self.status_code
-
-    def _get_error_string(self):
-        """
-        Get the target request error string.
-
-        This internal method is used to retrieve the error string
-        of the last target update request for use in the parameter
-        tree.
-        """
-        return self.error_string
-
-    def _get_last_update(self):
-        """
-        Get the target request last update timestamp.
-
-        This internal method is used to retrieve the timestamp
-        of the last target update request for use in the parameter
-        tree.
-        """
-        return self.last_update
-
-    def _get_data(self):
-        """
-        Get the target request data.
-
-        This internal method is used to retrieve the target updated during last call to update(),
-        for use in the parameter tree.
-        """
-        return self.data
-
-    def _get_url(self):
-        return self.url
-
 
 class ProxyAdapter(ApiAdapter):
     """
@@ -241,7 +216,7 @@ class ProxyAdapter(ApiAdapter):
                 logging.error(
                     "Illegal timeout specified for ProxyAdapter: %s",
                     self.options[TIMEOUT_CONFIG_NAME]
-                    )
+                )
 
         # Parse the list of target-URL pairs from the options, instantiating a ProxyTarget
         # object for each target specified.
@@ -261,13 +236,22 @@ class ProxyAdapter(ApiAdapter):
         else:
             logging.error("Failed to resolve targets for ProxyAdapter")
 
+        status_dict = {}
         # Construct the parameter tree returned by this adapter
-        tree = {'status': {}}
+        tree = {}
+        meta_tree = {}
         for target in self.targets:
-            tree['status'][target.name] = target.status_param_tree
+            status_dict[target.name] = target.status_param_tree
+
             tree[target.name] = target.data_param_tree
+            meta_tree[target.name] = target.meta_param_tree
+
+        self.status_tree = ParameterTree(status_dict)
+        tree['status'] = self.status_tree
+        meta_tree['status'] = self.status_tree.get("", True)
 
         self.param_tree = ParameterTree(tree)
+        self.meta_param_tree = ParameterTree(meta_tree)
 
     @request_types('application/json')
     @response_types('application/json', default='application/json')
@@ -281,6 +265,8 @@ class ProxyAdapter(ApiAdapter):
         :param request: HTTP request object
         :return: an ApiAdapterResponse object containing the appropriate response
         """
+
+        get_metadata = wants_metadata(request)
         # Update the target specified in the path, or all targets if none specified
         if "/" in path:
             path_elem, target_path = path.split('/', 1)
@@ -288,12 +274,19 @@ class ProxyAdapter(ApiAdapter):
             path_elem = path
             target_path = ""
         for target in self.targets:
-            if path_elem == '' or path_elem == target.name:
-                target.remote_get(target_path)
+            if path_elem == "" or path_elem == target.name:
+                target.remote_get(target_path, get_metadata)
 
         # Build the response from the adapter parameter tree
         try:
-            response = self.param_tree.get(path)
+            if get_metadata:
+                if path_elem == "" or path_elem == "status":
+                    # update status tree with metadata
+                    self.meta_param_tree.set('status', self.status_tree.get("", True))
+                response = self.meta_param_tree.get(path)
+
+            else:
+                response = self.param_tree.get(path)
             status_code = 200
         except ParameterTreeError as param_tree_err:
             response = {'error': str(param_tree_err)}
@@ -316,7 +309,7 @@ class ProxyAdapter(ApiAdapter):
         # Update the target specified in the path, or all targets if none specified
 
         try:
-            json_decode(request.body)  # ensure request body is JSON. Will throw a TypeError if not
+            body = decode_request_body(request)  # ensure request body is JSON. Will throw a TypeError if not
             if "/" in path:
                 path_elem, target_path = path.split('/', 1)
             else:
@@ -324,7 +317,8 @@ class ProxyAdapter(ApiAdapter):
                 target_path = ""
             for target in self.targets:
                 if path_elem == '' or path_elem == target.name:
-                    target.remote_set(target_path, request.body)
+                    target.remote_set(target_path, body)
+
             response = self.param_tree.get(path)
             status_code = 200
         except ParameterTreeError as param_tree_err:


Test patch:
diff --git a/tests/adapters/test_proxy.py b/tests/adapters/test_proxy.py
index c763091..4d7603c 100644
--- a/tests/adapters/test_proxy.py
+++ b/tests/adapters/test_proxy.py
@@ -10,12 +10,6 @@ import time
 
 import pytest
 
-if sys.version_info[0] == 3:  # pragma: no cover
-    from unittest.mock import Mock, patch
-    import asyncio
-else:                         # pragma: no cover
-    from mock import Mock, patch
-
 from tornado.testing import AsyncHTTPTestCase, bind_unused_port
 from tornado.ioloop import IOLoop
 from tornado.web import Application, RequestHandler
@@ -23,70 +17,69 @@ from tornado.httpserver import HTTPServer
 import tornado.gen
 
 from odin.adapters.proxy import ProxyTarget, ProxyAdapter
+from odin.adapters.parameter_tree import ParameterTree, ParameterTreeError
+from odin.adapters.adapter import wants_metadata
+from odin.util import convert_unicode_to_string
 from tests.utils import LogCaptureFilter, log_message_seen
 
+if sys.version_info[0] == 3:  # pragma: no cover
+    from unittest.mock import Mock, patch
+    import asyncio
+else:                         # pragma: no cover
+    from mock import Mock, patch
+
+
 class ProxyTestHandler(RequestHandler):
     """ Tornado request handler for use in test server needed for proxy tests."""
 
     # Data structure served by request handler
-    data = {'one': 1,
-            'two': 2.0,
-            'pi': 3.14,
-            'more':
-            {
-                'three': 3.0,
-                'replace': 'Replace Me!',
-                'even_more':{
-                    'extra_val': 5.5
-                }
+    data = {
+        'one': (1, None),  # this allows for auto generated metadata for testing purposes
+        'two': 2.0,
+        'pi': 3.14,
+        'more':
+        {
+            'three': 3.0,
+            'replace': 'Replace Me!',
+            'even_more': {
+                'extra_val': 5.5
             }
         }
+    }
+    param_tree = ParameterTree(data)
 
     def initialize(self, server):
         """Increment the server access count every time the request handler is invoked."""
         server.access_count += 1
 
     def get(self, path=''):
-        """Handle GET requests to the test 0server."""
+        """Handle GET requests to the test server."""
         try:
-            data_ref = self.data
-            if path:
-                path_elems = path.split('/')
-                for elem in path_elems[:-1]:
-                    data_ref = data_ref[elem]
-                self.write(data_ref)
-            else:
-                self.write(ProxyTestHandler.data)                
-        except KeyError:
+            data_ref = self.param_tree.get(path, wants_metadata(self.request))
+            self.write(data_ref)
+        except ParameterTreeError:
             self.set_status(404)
             self.write_error(404)
         except Exception as other_e:
-            logging.error("ProxyTestHandler GET failed:", str(other_e))
+            logging.error("ProxyTestHandler GET failed: %s", str(other_e))
             self.write_error(500)
-    
+
     def put(self, path):
         """Handle PUT requests to the test server."""
-        response_body = response_body = tornado.escape.json_decode(self.request.body)
+        response_body = convert_unicode_to_string(tornado.escape.json_decode(self.request.body))
         try:
-            data_ref = self.data
-            if path:
-                path_elems = path.split('/')
-                for elem in path_elems[:-1]:
-                    data_ref = data_ref[elem]
-                for key in response_body:
-                    new_elem = response_body[key]
-                    data_ref[key] = new_elem
-                self.write(data_ref)
-            else:
-                self.write(ProxyTestHandler.data)
-        except KeyError:
+            self.param_tree.set(path, response_body)
+            data_ref = self.param_tree.get(path)
+
+            self.write(data_ref)
+        except ParameterTreeError:
             self.set_status(404)
-            self.write_error(404)        
+            self.write_error(404)
         except Exception as other_e:
-            logging.error("ProxyTestHandler PUT failed:", str(other_e))
+            logging.error("ProxyTestHandler PUT failed: %s", str(other_e))
             self.write_error(500)
 
-                
+
 class ProxyTestServer(object):
     """ Tornado test server for use in proxy testing."""
     def __init__(self,):
@@ -150,16 +143,17 @@ class ProxyTargetTestFixture(object):
     def stop(self):
         """Stop the test server and proxy HTTP client."""
         self.proxy_target.http_client.close()
-        self.test_server.stop()       
+        self.test_server.stop()
 
 
-@pytest.fixture(scope="class")
+@pytest.fixture
 def test_proxy_target():
     """Fixture used in ProxyTarget test cases."""
     test_proxy_target = ProxyTargetTestFixture()
     yield test_proxy_target
     test_proxy_target.stop()
 
+
 class TestProxyTarget():
     """Test cases for the ProxyTarget class."""
 
@@ -174,7 +168,7 @@ class TestProxyTarget():
         test_proxy_target.proxy_target.last_update = ''
 
         test_proxy_target.proxy_target.remote_get()
-        assert test_proxy_target.proxy_target.data == ProxyTestHandler.data
+        assert test_proxy_target.proxy_target.data == ProxyTestHandler.param_tree.get("")
         assert test_proxy_target.proxy_target.status_code == 200
         assert test_proxy_target.proxy_target.last_update != ''
 
@@ -187,23 +181,37 @@ class TestProxyTarget():
     def test_proxy_target_http_get_error_404(self, test_proxy_target):
         """Test that a proxy target GET to a bad URL returns a 404 not found error."""
         bad_url = test_proxy_target.url + 'notfound/'
-        proxy_target = ProxyTarget(test_proxy_target.name, bad_url, 
-            test_proxy_target.request_timeout)
+        proxy_target = ProxyTarget(test_proxy_target.name, bad_url,
+                                   test_proxy_target.request_timeout)
         proxy_target.remote_get('notfound')
-        
+
         assert proxy_target.status_code == 404
         assert 'Not Found' in proxy_target.error_string
 
+    def test_proxy_target_timeout_error(self, test_proxy_target):
+        """Test that a porxy target GET request that times out is handled correctly"""
+        mock_fetch = Mock()
+        mock_fetch.side_effect = tornado.ioloop.TimeoutError('timeout')
+        proxy_target = ProxyTarget(test_proxy_target.name, test_proxy_target.url,
+                                   test_proxy_target.request_timeout)
+        proxy_target.http_client.fetch = mock_fetch
+
+        proxy_target.remote_get()
+
+        assert proxy_target.status_code == 408
+        assert 'timeout' in proxy_target.error_string
+
     def test_proxy_target_other_error(self, test_proxy_target):
         """Test that a proxy target GET request to a non-existing server returns a 502 error."""
         bad_url = 'http://127.0.0.1:{}'.format(test_proxy_target.port + 1)
         proxy_target = ProxyTarget(test_proxy_target.name, bad_url,
-            test_proxy_target.request_timeout)
+                                   test_proxy_target.request_timeout)
         proxy_target.remote_get()
 
         assert proxy_target.status_code == 502
         assert 'Connection refused' in proxy_target.error_string
 
+
 class ProxyAdapterTestFixture():
     """Container class used in fixtures for testing proxy adapters."""
 
@@ -233,7 +241,7 @@ class ProxyAdapterTestFixture():
         self.adapter = ProxyAdapter(**self.adapter_kwargs)
 
         self.path = ''
-        self.request = Mock
+        self.request = Mock()
         self.request.headers = {'Accept': 'application/json', 'Content-Type': 'application/json'}
         self.request.body = '{"pi":2.56}'
 
@@ -243,7 +251,7 @@ class ProxyAdapterTestFixture():
 
     def stop(self):
         """Stop the proxied test servers, ensuring any client connections to them are closed."""
-        for target in self.adapter.targets: 
+        for target in self.adapter.targets:
             target.http_client.close()
         for test_server in self.test_servers:
             test_server.stop()
@@ -253,14 +261,16 @@ class ProxyAdapterTestFixture():
         for test_server in self.test_servers:
             test_server.clear_access_count()
 
+
 @pytest.fixture(scope="class")
 def proxy_adapter_test():
     """Fixture used in testing the ProxyAdapter class."""
     proxy_adapter_test = ProxyAdapterTestFixture()
     yield proxy_adapter_test
-    
+
     proxy_adapter_test.stop()
 
+
 class TestProxyAdapter():
     """Test cases for testing the ProxyAdapter class."""
 
@@ -275,16 +285,37 @@ class TestProxyAdapter():
         """
         response = proxy_adapter_test.adapter.get(
             proxy_adapter_test.path, proxy_adapter_test.request)
-        
+
         assert 'status' in response.data
-        
-        assert len(response.data) ==  proxy_adapter_test.num_targets+1
-        
+
+        assert len(response.data) == proxy_adapter_test.num_targets + 1
+
         for tgt in range(proxy_adapter_test.num_targets):
             node_str = 'node_{}'.format(tgt)
             assert node_str in response.data
             assert response.data[node_str], ProxyTestHandler.data
-    
+
+    def test_adapter_get_metadata(self, proxy_adapter_test):
+        request = proxy_adapter_test.request
+        request.headers['Accept'] = "{};{}".format(request.headers['Accept'], "metadata=True")
+        response = proxy_adapter_test.adapter.get(proxy_adapter_test.path, request)
+
+        assert "status" in response.data
+        for target in range(proxy_adapter_test.num_targets):
+            node_str = 'node_{}'.format(target)
+            assert node_str in response.data
+            assert "one" in response.data[node_str]
+            assert "type" in response.data[node_str]['one']
+
+    def test_adapter_get_status_metadata(self, proxy_adapter_test):
+        request = proxy_adapter_test.request
+        request.headers['Accept'] = "{};{}".format(request.headers['Accept'], "metadata=True")
+        response = proxy_adapter_test.adapter.get(proxy_adapter_test.path, request)
+
+        assert 'status' in response.data
+        assert 'node_0' in response.data['status']
+        assert 'type' in response.data['status']['node_0']['error']
+
     def test_adapter_put(self, proxy_adapter_test):
         """
         Test that a PUT request to the proxy adapter returns the appropriate data for all
@@ -295,12 +326,12 @@ class TestProxyAdapter():
 
         assert 'status' in response.data
 
-        assert len(response.data) == proxy_adapter_test.num_targets+1
+        assert len(response.data) == proxy_adapter_test.num_targets + 1
 
         for tgt in range(proxy_adapter_test.num_targets):
             node_str = 'node_{}'.format(tgt)
             assert node_str in response.data
-            assert response.data[node_str] == ProxyTestHandler.data
+            assert convert_unicode_to_string(response.data[node_str]) == ProxyTestHandler.param_tree.get("")
 
     def test_adapter_get_proxy_path(self, proxy_adapter_test):
         """Test that a GET to a sub-path within a targer succeeds and return the correct data."""
@@ -314,30 +345,31 @@ class TestProxyAdapter():
 
     def test_adapter_get_proxy_path_trailing_slash(self, proxy_adapter_test):
         """
-        Test that a PUT to a sub-path with a trailing slash in the URL within a targer succeeds 
+        Test that a PUT to a sub-path with a trailing slash in the URL within a targer succeeds
         and returns the correct data.
         """
         node = proxy_adapter_test.adapter.targets[0].name
         path = "more/even_more/"
         response = proxy_adapter_test.adapter.get(
             "{}/{}".format(node, path), proxy_adapter_test.request)
-        
+
         assert response.data["even_more"] == ProxyTestHandler.data["more"]["even_more"]
         assert proxy_adapter_test.adapter.param_tree.get('')['status'][node]['status_code'] == 200
 
     def test_adapter_put_proxy_path(self, proxy_adapter_test):
         """
-        Test that a PUT to a sub-path withouta trailing slash in the URL within a targer succeeds 
+        Test that a PUT to a sub-path without a trailing slash in the URL within a targer succeeds
         and returns the correct data.
         """
         node = proxy_adapter_test.adapter.targets[0].name
-        path = "more/replace"
+        path = "more"
         proxy_adapter_test.request.body = '{"replace": "been replaced"}'
         response = proxy_adapter_test.adapter.put(
             "{}/{}".format(node, path), proxy_adapter_test.request)
 
+        logging.debug("Response: %s", response.data)
         assert proxy_adapter_test.adapter.param_tree.get('')['status'][node]['status_code'] == 200
-        assert response.data["replace"] == "been replaced"
+        assert convert_unicode_to_string(response.data["more"]["replace"]) == "been replaced"
 
     def test_adapter_get_bad_path(self, proxy_adapter_test):
         """Test that a GET to a bad path within a target returns the appropriate error."""