Merge "Refactor trait normalization"

placement/handlers/resource_provider.py

@@ -229,8 +229,6 @@ def list_resource_providers(req):
     elif want_version.matches((1, 3)):
         schema = rp_schema.GET_RPS_SCHEMA_1_3
 
-    allow_forbidden = want_version.matches((1, 22))
-
     util.validate_query_params(req, schema)
 
     filters = {}
@@ -240,15 +238,15 @@ def list_resource_providers(req):
         filters['member_of'], filters['forbidden_aggs'] = (
             util.normalize_member_of_qs_params(req))
 
-    qpkeys = ('uuid', 'name', 'in_tree', 'resources', 'required')
+    if 'required' in req.GET:
+        filters['required'] = util.normalize_traits_qs_params(req)
+
+    qpkeys = ('uuid', 'name', 'in_tree', 'resources')
     for attr in qpkeys:
         if attr in req.GET:
             value = req.GET[attr]
             if attr == 'resources':
                 value = util.normalize_resources_qs_param(value)
-            elif attr == 'required':
-                value = util.normalize_traits_qs_param(
-                    value, allow_forbidden=allow_forbidden)
             filters[attr] = value
     try:
         resource_providers = rp_obj.get_all_by_filters(context, filters)

placement/lib.py

@@ -94,7 +94,7 @@ class RequestGroup(object):
         return ret
 
     @staticmethod
-    def _parse_request_items(req, allow_forbidden, verbose_suffix):
+    def _parse_request_items(req, verbose_suffix):
         ret = {}
         pattern = _QS_KEY_PATTERN_1_33 if verbose_suffix else _QS_KEY_PATTERN
         for key, val in req.GET.items():
@@ -113,8 +113,8 @@ class RequestGroup(object):
                 request_group.resources = util.normalize_resources_qs_param(
                     val)
             elif prefix == _QS_REQUIRED:
-                request_group.required_traits = util.normalize_traits_qs_param(
-                    val, allow_forbidden=allow_forbidden)
+                request_group.required_traits = (
+                    util.normalize_traits_qs_params(req, suffix))
             elif prefix == _QS_MEMBER_OF:
                 # special handling of member_of qparam since we allow multiple
                 # member_of params at microversion 1.24.
@@ -304,8 +304,7 @@ class RequestGroup(object):
         # Control whether we want verbose suffixes
         verbose_suffix = want_version.matches((1, 33))
         # dict of the form: { suffix: RequestGroup } to be returned
-        by_suffix = cls._parse_request_items(
-            req, allow_forbidden, verbose_suffix)
+        by_suffix = cls._parse_request_items(req, verbose_suffix)
 
         if want_version.matches(SAME_SUBTREE_VERSION):
             resourceless_suffixes = set(

placement/tests/unit/test_util.py

@@ -24,6 +24,7 @@ from oslo_utils import timeutils
 import testtools
 import webob
 
+import placement
 from placement import context
 from placement import lib as pl
 from placement import microversion
@@ -431,6 +432,64 @@ class TestNormalizeTraitsQsParam(testtools.TestCase):
                               util.normalize_traits_qs_param, fmt % traits)
 
 
+class TestNormalizeTraitsQsParams(testtools.TestCase):
+    @staticmethod
+    def _get_req(qstring, version):
+        req = webob.Request.blank(
+            '?' + qstring,
+        )
+        mv_parsed = microversion_parse.Version(*version)
+        mv_parsed.max_version = microversion_parse.parse_version_string(
+            microversion.max_version_string()
+        )
+        mv_parsed.min_version = microversion_parse.parse_version_string(
+            microversion.min_version_string()
+        )
+        req.environ[placement.microversion.MICROVERSION_ENVIRON] = mv_parsed
+        return req
+
+    def test_suffix(self):
+        req = self._get_req('required=!BAZ&requiredX=FOO,BAR', (1, 38))
+
+        traits = util.normalize_traits_qs_params(req, suffix='')
+
+        self.assertEqual({'!BAZ'}, traits)
+
+        traits = util.normalize_traits_qs_params(req, suffix='X')
+
+        self.assertEqual({'FOO', 'BAR'}, traits)
+
+    def test_allow_forbidden_1_21(self):
+        req = self._get_req('required=!BAZ', (1, 21))
+
+        ex = self.assertRaises(
+            webob.exc.HTTPBadRequest,
+            util.normalize_traits_qs_params,
+            req,
+            suffix='',
+        )
+
+        self.assertIn(
+            "Invalid query string parameters: Expected 'required' parameter "
+            "value of the form: HW_CPU_X86_VMX,CUSTOM_MAGIC. Got: !BAZ",
+            str(ex),
+        )
+
+    def test_allow_forbidden_1_22(self):
+        req = self._get_req('required=!BAZ', (1, 22))
+
+        traits = util.normalize_traits_qs_params(req, suffix='')
+
+        self.assertEqual({'!BAZ'}, traits)
+
+    def test_repeated_param_1_38(self):
+        req = self._get_req('required=FOO,!BAR&required=BAZ', (1, 38))
+
+        traits = util.normalize_traits_qs_params(req, suffix='')
+
+        self.assertEqual({'BAZ'}, traits)
+
+
 class TestParseQsRequestGroups(testtools.TestCase):
 
     @staticmethod

placement/util.py

@@ -337,6 +337,33 @@ def normalize_traits_qs_param(val, allow_forbidden=False):
     return ret
 
 
+def normalize_traits_qs_params(req, suffix=''):
+    """Given a webob.Request object, validate and collect required querystring
+    parameters.
+
+    We begin supporting forbidden traits in microversion 1.22.
+
+    :param req: a webob.Request object to read the params from
+    :param suffix: the string suffix of the request group to read from the
+        request. If empty then the unnamed request group is processed.
+    :returns: a set of trait names, including forbidden traits with an '!'
+        prefix.
+    :raises webob.exc.HTTPBadRequest: if the format of the query param is not
+        valid
+    """
+    want_version = req.environ[placement.microversion.MICROVERSION_ENVIRON]
+    allow_forbidden = want_version.matches((1, 22))
+
+    traits = set()
+
+    # NOTE(gibi): This means if the same query param is repeated then only
+    # the last one will be considered
+    for value in req.GET.getall('required' + suffix):
+        traits = normalize_traits_qs_param(value, allow_forbidden)
+
+    return traits
+
+
 def normalize_member_of_qs_params(req, suffix=''):
     """Given a webob.Request object, validate that the member_of querystring
     parameters are correct. We begin supporting multiple member_of params in