Comment on parse_limits(); expand an exception message; add unit tests; fix a minor discovered bug

2 files changed, 96 insertions, 3 deletions

diff --git a/nova/api/openstack/limits.py b/nova/api/openstack/limits.py
index 8642a28f9..bc76547d8 100644
--- a/nova/api/openstack/limits.py
+++ b/nova/api/openstack/limits.py
@@ -327,6 +327,11 @@ class Limiter(object):
 
         return None, None
 
+    # Note: This method gets called before the class is instantiated,
+    # so this must be either a static method or a class method.  It is
+    # used to develop a list of limits to feed to the constructor.  We
+    # put this in the class so that subclasses can override the
+    # default limit parsing.
     @staticmethod
     def parse_limits(limits):
         """
@@ -351,14 +356,16 @@ class Limiter(object):
         result = []
         for group in limits.split(';'):
             group = group.strip()
-            if group[0] != '(' or group[-1] != ')':
-                raise ValueError("Groups must be surrounded by parentheses")
+            if group[:1] != '(' or group[-1:] != ')':
+                raise ValueError("Limit rules must be surrounded by "
+                                 "parentheses")
             group = group[1:-1]
 
             # Extract the Limit arguments
             args = [a.strip() for a in group.split(',')]
             if len(args) != 5:
-                raise ValueError("Groups must contain exactly 5 elements")
+                raise ValueError("Limit rules must contain the following "
+                                 "arguments: verb, uri, regex, value, unit")
 
             # Pull out the arguments
             verb, uri, regex, value, unit = args
@@ -464,6 +471,11 @@ class WsgiLimiterProxy(object):
 
         return resp.getheader("X-Wait-Seconds"), resp.read() or None
 
+    # Note: This method gets called before the class is instantiated,
+    # so this must be either a static method or a class method.  It is
+    # used to develop a list of limits to feed to the constructor.
+    # This implementation returns an empty list, since all limit
+    # decisions are made by a remote server.
     @staticmethod
     def parse_limits(limits):
         """
diff --git a/nova/tests/api/openstack/test_limits.py b/nova/tests/api/openstack/test_limits.py
index eeacd2fca..e0368d5f5 100644
--- a/nova/tests/api/openstack/test_limits.py
+++ b/nova/tests/api/openstack/test_limits.py
@@ -500,6 +500,87 @@ class LimitTest(BaseLimitTestSuite):
         self.assertEqual(4, limit.last_request)
 
 
+class ParseLimitsTest(BaseLimitTestSuite):
+    """
+    Tests for the default limits parser in the in-memory
+    `limits.Limiter` class.
+    """
+
+    def test_invalid(self):
+        """Test that parse_limits() handles invalid input correctly."""
+        try:
+            limits.Limiter.parse_limits(';;;;;')
+        except ValueError:
+            return
+        assert False, "Failure to reject invalid input"
+
+    def test_bad_rule(self):
+        """Test that parse_limits() handles bad rules correctly."""
+        try:
+            limits.Limiter.parse_limits('GET, *, .*, 20, minute')
+        except ValueError:
+            return
+        assert False, "Failure to reject bad rule"
+
+    def test_missing_arg(self):
+        """Test that parse_limits() handles missing args correctly."""
+        try:
+            limits.Limiter.parse_limits('(GET, *, .*, 20)')
+        except ValueError:
+            return
+        assert False, "Failure to reject missing rule argument"
+
+    def test_bad_value(self):
+        """Test that parse_limits() handles bad values correctly."""
+        try:
+            limits.Limiter.parse_limits('(GET, *, .*, foo, minute)')
+        except ValueError:
+            return
+        assert False, "Failure to reject invalid value"
+
+    def test_bad_unit(self):
+        """Test that parse_limits() handles bad units correctly."""
+        try:
+            limits.Limiter.parse_limits('(GET, *, .*, 20, lightyears)')
+        except ValueError:
+            return
+        assert False, "Failure to reject invalid unit"
+
+    def test_multiple_rules(self):
+        """Test that parse_limits() handles multiple rules correctly."""
+        try:
+            l = limits.Limiter.parse_limits('(get, *, .*, 20, minute);'
+                                            '(PUT, /foo*, /foo.*, 10, hour);'
+                                            '(POST, /bar*, /bar.*, 5, second);'
+                                            '(Say, /derp*, /derp.*, 1, day)')
+        except ValueError, e:
+            assert False, str(e)
+
+        # Make sure the number of returned limits are correct
+        self.assertEqual(len(l), 4)
+
+        # Check all the verbs...
+        expected = ['GET', 'PUT', 'POST', 'SAY']
+        self.assertEqual([t.verb for t in l], expected)
+
+        # ...the URIs...
+        expected = ['*', '/foo*', '/bar*', '/derp*']
+        self.assertEqual([t.uri for t in l], expected)
+
+        # ...the regexes...
+        expected = ['.*', '/foo.*', '/bar.*', '/derp.*']
+        self.assertEqual([t.regex for t in l], expected)
+
+        # ...the values...
+        expected = [20, 10, 5, 1]
+        self.assertEqual([t.value for t in l], expected)
+
+        # ...and the units...
+        expected = [limits.PER_MINUTE, limits.PER_HOUR,
+                    limits.PER_SECOND, limits.PER_DAY]
+        self.assertEqual([t.unit for t in l], expected)
+
+
 class LimiterTest(BaseLimitTestSuite):
     """
     Tests for the in-memory `limits.Limiter` class.