From 1d20ef03b85cfbdb116f548ab36684d8b31dd15b Mon Sep 17 00:00:00 2001
From: Dongfeng Huang <yingfeicaozhang100527@gmail.com>
Date: Tue, 16 May 2017 11:07:36 +0800
Subject: [PATCH] Fix bug for resource routing's filtering feature

1. What is the problem
For resource routing Admin API, when we call get_all(self, **kwargs)
function with an unsupported filter type specified to list the routing
information, the function will ignore this filter and return all the
routing information. It shouldn't happen. Instead, when unsupported
filter type is given, the function should response with 400 error
and notify user there exist unsupported filter types.

2. What is the solution to the problem
Response with 400 error when unsupported filter type is specified for
routing's get_all(self, **kwargs) function.

3. What the features need to be implemented to the Tricircle
to realize the solution
Modify the original resource routing's get_all(self, **kwargs)
function to return 400 error when met unsupported filter type.

Change-Id: I1bff9ebb7a6371a139d9ba547af417548100a2b4
Closes-Bug: #1690971
---
 tricircle/api/controllers/job.py              |  5 ++--
 tricircle/api/controllers/routing.py          | 26 ++++++++++++++-----
 .../unit/api/controllers/test_routing.py      | 15 ++++++-----
 3 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/tricircle/api/controllers/job.py b/tricircle/api/controllers/job.py
index a2263195..595b580b 100644
--- a/tricircle/api/controllers/job.py
+++ b/tricircle/api/controllers/job.py
@@ -202,8 +202,9 @@ class AsyncJobController(rest.RestController):
         is_valid_filter, filters = self._get_filters(kwargs)
 
         if not is_valid_filter:
-            msg = (_('Unsupported filter type: %(filter)s') %
-                   {'filter': [filter_name for filter_name in filters]})
+            msg = (_('Unsupported filter type: %(filters)s') % {
+                'filters': ', '.join([filter_name for filter_name in filters])
+            })
             return utils.format_api_error(400, msg)
 
         filters = [{'key': key,
diff --git a/tricircle/api/controllers/routing.py b/tricircle/api/controllers/routing.py
index 9b9995fb..3eead457 100644
--- a/tricircle/api/controllers/routing.py
+++ b/tricircle/api/controllers/routing.py
@@ -91,16 +91,21 @@ class RoutingController(rest.RestController):
         """Return a dictionary of query param filters from the request.
 
         :param params: the URI params coming from the wsgi layer
-        :return a dict of key/value filters
+        :return (flag, filters), flag indicates whether the filters are valid,
+        and the filters denote a list of key-value pairs.
         """
         filters = {}
-        for param in params:
-            if param in SUPPORTED_FILTERS:
+        unsupported_filters = {}
+        for filter_name in params:
+            if filter_name in SUPPORTED_FILTERS:
                 # map filter name
-                filter_name = param
-                filters[filter_name] = params.get(param)
+                filters[filter_name] = params.get(filter_name)
+            else:
+                unsupported_filters[filter_name] = params.get(filter_name)
 
-        return filters
+        if unsupported_filters:
+            return False, unsupported_filters
+        return True, filters
 
     @expose(generic=True, template='json')
     def get_all(self, **kwargs):
@@ -110,7 +115,14 @@ class RoutingController(rest.RestController):
             return utils.format_api_error(
                 403, _('Unauthorized to show all resource routings'))
 
-        filters = self._get_filters(kwargs)
+        is_valid_filter, filters = self._get_filters(kwargs)
+
+        if not is_valid_filter:
+            msg = (_('Unsupported filter type: %(filters)s') % {
+                'filters': ', '.join([filter_name for filter_name in filters])
+            })
+            return utils.format_api_error(400, msg)
+
         filters = [{'key': key,
                     'comparator': 'eq',
                     'value': value} for key, value in six.iteritems(filters)]
diff --git a/tricircle/tests/unit/api/controllers/test_routing.py b/tricircle/tests/unit/api/controllers/test_routing.py
index dfb4d998..9e4a7bc2 100644
--- a/tricircle/tests/unit/api/controllers/test_routing.py
+++ b/tricircle/tests/unit/api/controllers/test_routing.py
@@ -253,14 +253,15 @@ class RoutingControllerTest(unittest.TestCase):
         routings = self.controller.get_all(**kw_filter2)
         self.assertEqual([], routings['routings'])
 
-        # apply an illegal filter and it won't take effect
+        # failure case, use an unsupported filter type
         kw_filter3 = {'resource': 'port'}
-        routings = self.controller.get_all(**kw_filter3)
-        actual = [(routing['top_id'], routing['pod_id'])
-                  for routing in routings['routings']]
-        expect = [('c7f641c9-8462-4007-84b2-3035d8cfb7a3', pod_id1),
-                  ('b669a2da-ca95-47db-a2a9-ba9e546d82ee', pod_id2)]
-        six.assertCountEqual(self, expect, actual)
+        res = self.controller.get_all(**kw_filter3)
+        self._validate_error_code(res, 400)
+
+        kw_filter4 = {'pod_id': pod_id1,
+                      'resource': 'port'}
+        res = self.controller.get_all(**kw_filter4)
+        self._validate_error_code(res, 400)
 
         # failure case, only admin can show all resource routings
         self.context.is_admin = False