Added tests back for RateLimitingMiddleware which now throw correctly serialized

errors with correct error codes.

Removed some error printing, and simplified some other parts of the code with
suggestions from teammates.

4 files changed, 148 insertions, 72 deletions

diff --git a/nova/api/openstack/faults.py b/nova/api/openstack/faults.py
index 6ed9322de..d05c61fc7 100644
--- a/nova/api/openstack/faults.py
+++ b/nova/api/openstack/faults.py
@@ -68,17 +68,31 @@ class OverLimitFault(webob.exc.HTTPException):
     Rate-limited request response.
     """
 
-    wrapped_exc = webob.exc.HTTPForbidden()
+    _serialization_metadata = {
+        "application/xml": {
+            "attributes": {
+                "overLimitFault": "code"
+            }
+        }
+    }
 
     def __init__(self, message, details, retry_time):
         """
         Initialize new `OverLimitFault` with relevant information.
         """
-        self.message = message
-        self.details = details
-        self.retry_time = retry_time
+        self.wrapped_exc = webob.exc.HTTPForbidden()
+        self.content = {
+            "overLimitFault": {
+                "code": self.wrapped_exc.status_int,
+                "message": message,
+                "details": details,
+            },
+        }
 
     @webob.dec.wsgify(RequestClass=wsgi.Request)
     def __call__(self, request):
         """Currently just return the wrapped exception."""
+        serializer = wsgi.Serializer(self._serialization_metadata)
+        content_type = request.best_match_content_type()
+        self.wrapped_exc.body = serializer.serialize(self.content, content_type)
         return self.wrapped_exc
diff --git a/nova/api/openstack/limits.py b/nova/api/openstack/limits.py
index 3ecd46377..c4e04e9d9 100644
--- a/nova/api/openstack/limits.py
+++ b/nova/api/openstack/limits.py
@@ -54,8 +54,8 @@ class LimitsController(Controller):
                 "limit": ["verb", "URI", "regex", "value", "unit",
                     "resetTime", "remaining", "name"],
             },
-            "plurals" : {
-                "rate" : "limit",
+            "plurals": {
+                "rate": "limit",
             },
         },
     }
@@ -215,7 +215,12 @@ class RateLimitingMiddleware(Middleware):
         """
         verb = req.method
         url = req.url
-        username = req.environ["nova.context"].user_id
+        context = req.environ.get("nova.context")
+
+        if context:
+            username = context.user_id
+        else:
+            username = None
 
         delay, error = self._limiter.check_for_delay(verb, url, username)
 
@@ -255,14 +260,12 @@ class Limiter(object):
 
         @return: Tuple of delay (in seconds) and error message (or None, None)
         """
-        def _get_delay_list():
-            """Yield limit delays."""
-            for limit in self.levels[username]:
-                delay = limit(verb, url)
-                if delay:
-                    yield delay, limit.error_message
+        delays = []
 
-        delays = list(_get_delay_list())
+        for limit in self.levels[username]:
+            delay = limit(verb, url)
+            if delay:
+                delays.append((delay, limit.error_message))
 
         if delays:
             delays.sort()
@@ -349,8 +352,6 @@ class WsgiLimiterProxy(object):
 
         resp = conn.getresponse()
 
-        print resp
-
         if 200 >= resp.status < 300:
             return None, None
 
diff --git a/nova/tests/api/openstack/test_limits.py b/nova/tests/api/openstack/test_limits.py
index 40178e671..d1db93a59 100644
--- a/nova/tests/api/openstack/test_limits.py
+++ b/nova/tests/api/openstack/test_limits.py
@@ -22,11 +22,11 @@ import json
 import StringIO
 import stubout
 import time
+import unittest
 import webob
 
 from xml.dom.minidom import parseString
 
-from nova import test
 from nova.api.openstack import limits
 from nova.api.openstack.limits import Limit
 
@@ -40,28 +40,34 @@ TEST_LIMITS = [
 ]
 
 
-class LimitsControllerTest(test.TestCase):
-    """
-    Tests for `limits.LimitsController` class.
-    """
+class BaseLimitTestSuite(unittest.TestCase):
+    """Base test suite which provides relevant stubs and time abstraction."""
 
     def setUp(self):
         """Run before each test."""
-        test.TestCase.setUp(self)
         self.time = 0.0
         self.stubs = stubout.StubOutForTesting()
         self.stubs.Set(limits.Limit, "_get_time", self._get_time)
-        self.controller = limits.LimitsController()
 
     def tearDown(self):
         """Run after each test."""
         self.stubs.UnsetAll()
-        test.TestCase.tearDown(self)
 
     def _get_time(self):
         """Return the "time" according to this test suite."""
         return self.time
 
+
+class LimitsControllerTest(BaseLimitTestSuite):
+    """
+    Tests for `limits.LimitsController` class.
+    """
+
+    def setUp(self):
+        """Run before each test."""
+        BaseLimitTestSuite.setUp(self)
+        self.controller = limits.LimitsController()
+
     def _get_index_request(self, accept_header="application/json"):
         """Helper to set routing arguments."""
         request = webob.Request.blank("/")
@@ -74,11 +80,11 @@ class LimitsControllerTest(test.TestCase):
 
     def _populate_limits(self, request):
         """Put limit info into a request."""
-        limits = [
+        _limits = [
             Limit("GET", "*", ".*", 10, 60).display(),
             Limit("POST", "*", ".*", 5, 60 * 60).display(),
         ]
-        request.environ["nova.limits"] = limits
+        request.environ["nova.limits"] = _limits
         return request
 
     def test_empty_index_json(self):
@@ -131,7 +137,7 @@ class LimitsControllerTest(test.TestCase):
         response = request.get_response(self.controller)
 
         expected = "<limits><rate/><absolute/></limits>"
-        body = response.body.replace("\n","").replace(" ", "")
+        body = response.body.replace("\n", "").replace(" ", "")
 
         self.assertEqual(expected, body)
 
@@ -144,7 +150,7 @@ class LimitsControllerTest(test.TestCase):
         expected = parseString("""
             <limits>
                 <rate>
-                    <limit URI="*" regex=".*" remaining="10" resetTime="0" 
+                    <limit URI="*" regex=".*" remaining="10" resetTime="0"
                         unit="MINUTE" value="10" verb="GET"/>
                     <limit URI="*" regex=".*" remaining="5" resetTime="0"
                         unit="HOUR" value="5" verb="POST"/>
@@ -157,28 +163,108 @@ class LimitsControllerTest(test.TestCase):
         self.assertEqual(expected.toxml(), body.toxml())
 
 
-class LimiterTest(test.TestCase):
+class LimitMiddlewareTest(BaseLimitTestSuite):
+    """
+    Tests for the `limits.RateLimitingMiddleware` class.
+    """
+
+    @webob.dec.wsgify
+    def _empty_app(self, request):
+        """Do-nothing WSGI app."""
+        pass
+
+    def setUp(self):
+        """Prepare middleware for use through fake WSGI app."""
+        BaseLimitTestSuite.setUp(self)
+        _limits = [
+            Limit("GET", "*", ".*", 1, 60),
+        ]
+        self.app = limits.RateLimitingMiddleware(self._empty_app, _limits)
+
+    def test_good_request(self):
+        """Test successful GET request through middleware."""
+        request = webob.Request.blank("/")
+        response = request.get_response(self.app)
+        self.assertEqual(200, response.status_int)
+
+    def test_limited_request_json(self):
+        """Test a rate-limited (403) GET request through middleware."""
+        request = webob.Request.blank("/")
+        response = request.get_response(self.app)
+        self.assertEqual(200, response.status_int)
+
+        request = webob.Request.blank("/")
+        response = request.get_response(self.app)
+        self.assertEqual(response.status_int, 403)
+
+        body = json.loads(response.body)
+        expected = "Only 1 GET request(s) can be made to * every minute."
+        value = body["overLimitFault"]["details"].strip()
+        self.assertEqual(value, expected)
+
+    def test_limited_request_xml(self):
+        """Test a rate-limited (403) response as XML"""
+        request = webob.Request.blank("/")
+        response = request.get_response(self.app)
+        self.assertEqual(200, response.status_int)
+
+        request = webob.Request.blank("/")
+        request.accept = "application/xml"
+        response = request.get_response(self.app)
+        self.assertEqual(response.status_int, 403)
+
+        root = parseString(response.body).childNodes[0]
+        expected = "Only 1 GET request(s) can be made to * every minute."
+
+        details = root.getElementsByTagName("details")
+        self.assertEqual(details.length, 1)
+
+        value = details.item(0).firstChild.data.strip()
+        self.assertEqual(value, expected)
+
+
+class LimitTest(BaseLimitTestSuite):
+    """
+    Tests for the `limits.Limit` class.
+    """
+
+    def test_GET_no_delay(self):
+        """Test a limit handles 1 GET per second."""
+        limit = Limit("GET", "*", ".*", 1, 1)
+        delay = limit("GET", "/anything")
+        self.assertEqual(None, delay)
+        self.assertEqual(0, limit.next_request)
+        self.assertEqual(0, limit.last_request)
+
+    def test_GET_delay(self):
+        """Test two calls to 1 GET per second limit."""
+        limit = Limit("GET", "*", ".*", 1, 1)
+        delay = limit("GET", "/anything")
+        self.assertEqual(None, delay)
+
+        delay = limit("GET", "/anything")
+        self.assertEqual(1, delay)
+        self.assertEqual(1, limit.next_request)
+        self.assertEqual(0, limit.last_request)
+
+        self.time += 4
+
+        delay = limit("GET", "/anything")
+        self.assertEqual(None, delay)
+        self.assertEqual(4, limit.next_request)
+        self.assertEqual(4, limit.last_request)
+
+
+class LimiterTest(BaseLimitTestSuite):
     """
     Tests for the in-memory `limits.Limiter` class.
     """
 
     def setUp(self):
         """Run before each test."""
-        test.TestCase.setUp(self)
-        self.time = 0.0
-        self.stubs = stubout.StubOutForTesting()
-        self.stubs.Set(limits.Limit, "_get_time", self._get_time)
+        BaseLimitTestSuite.setUp(self)
         self.limiter = limits.Limiter(TEST_LIMITS)
 
-    def tearDown(self):
-        """Run after each test."""
-        self.stubs.UnsetAll()
-        test.TestCase.tearDown(self)
-
-    def _get_time(self):
-        """Return the "time" according to this test suite."""
-        return self.time
-
     def _check(self, num, verb, url, username=None):
         """Check and yield results from checks."""
         for x in xrange(num):
@@ -311,28 +397,16 @@ class LimiterTest(test.TestCase):
         self.assertEqual(expected, results)
 
 
-class WsgiLimiterTest(test.TestCase):
+class WsgiLimiterTest(BaseLimitTestSuite):
     """
     Tests for `limits.WsgiLimiter` class.
     """
 
     def setUp(self):
         """Run before each test."""
-        test.TestCase.setUp(self)
-        self.time = 0.0
-        self.stubs = stubout.StubOutForTesting()
-        self.stubs.Set(limits.Limit, "_get_time", self._get_time)
+        BaseLimitTestSuite.setUp(self)
         self.app = limits.WsgiLimiter(TEST_LIMITS)
 
-    def tearDown(self):
-        """Run after each test."""
-        self.stubs.UnsetAll()
-        test.TestCase.tearDown(self)
-
-    def _get_time(self):
-        """Return the "time" according to this test suite."""
-        return self.time
-
     def _request_data(self, verb, path):
         """Get data decribing a limit request verb/path."""
         return json.dumps({"verb": verb, "path": path})
@@ -476,7 +550,7 @@ def wire_HTTPConnection_to_WSGI(host, app):
     httplib.HTTPConnection = HTTPConnectionDecorator(httplib.HTTPConnection)
 
 
-class WsgiLimiterProxyTest(test.TestCase):
+class WsgiLimiterProxyTest(BaseLimitTestSuite):
     """
     Tests for the `limits.WsgiLimiterProxy` class.
     """
@@ -486,23 +560,11 @@ class WsgiLimiterProxyTest(test.TestCase):
         Do some nifty HTTP/WSGI magic which allows for WSGI to be called
         directly by something like the `httplib` library.
         """
-        test.TestCase.setUp(self)
-        self.time = 0.0
-        self.stubs = stubout.StubOutForTesting()
-        self.stubs.Set(limits.Limit, "_get_time", self._get_time)
+        BaseLimitTestSuite.setUp(self)
         self.app = limits.WsgiLimiter(TEST_LIMITS)
         wire_HTTPConnection_to_WSGI("169.254.0.1:80", self.app)
         self.proxy = limits.WsgiLimiterProxy("169.254.0.1:80")
 
-    def tearDown(self):
-        """Run after each test."""
-        self.stubs.UnsetAll()
-        test.TestCase.tearDown(self)
-
-    def _get_time(self):
-        """Return the "time" according to this test suite."""
-        return self.time
-
     def test_200(self):
         """Successful request test."""
         delay = self.proxy.check_for_delay("GET", "/anything")
@@ -519,4 +581,4 @@ class WsgiLimiterProxyTest(test.TestCase):
         expected = ("60.00", "403 Forbidden\n\nOnly 1 GET request(s) can be "\
             "made to /delayed every minute.")
 
-        self.assertEqual((delay, error), expected) 
+        self.assertEqual((delay, error), expected)
diff --git a/nova/wsgi.py b/nova/wsgi.py
index 21aabd556..ba0819466 100644
--- a/nova/wsgi.py
+++ b/nova/wsgi.py
@@ -482,7 +482,6 @@ class Serializer(object):
 
     def _to_xml_node(self, doc, metadata, nodename, data):
         """Recursive method to convert data members to XML nodes."""
-        print "to_xml_node(%s, %s, %s, %s)" % (doc, metadata, nodename, data)
         result = doc.createElement(nodename)
         if type(data) is list:
             singular = metadata.get('plurals', {}).get(nodename, None)