fix: bad marker behaves like non-existing marker

From the point of view of the interface, a marker should has only
two states: returning some messages, or returning no messages.
Currently a malformed message marker results in a 400 response,
which makes the marker no longer opaque.

This patch makes the transport respond 204 for a malformed marker
by making storage return an empty iterator in such a case:

  GET /queues/q/messages?marker=malformed  ->  204

This patch also include some clean up on test code.

Change-Id: I1ac7d4bb42af1644bd16d6bca6b7c50a1d700e10
Closes-Bug: #1212389

marconi/storage/exceptions.py

@@ -34,14 +34,6 @@ class Conflict(Exception):
     """
 
 
-class MalformedID(ValueError):
-    """ID was malformed."""
-
-
-class MalformedMarker(ValueError):
-    """Pagination marker was malformed."""
-
-
 class MessageConflict(Conflict):
 
     def __init__(self, queue, project, message_ids):

marconi/storage/mongodb/messages.py

@@ -417,7 +417,7 @@ class MessageController(storage.MessageBase):
             try:
                 marker = int(marker)
             except ValueError:
-                raise exceptions.MalformedMarker()
+                yield iter([])
 
         messages = self._list(queue_name, marker, echo, client_uuid,
                               include_claimed=include_claimed, project=project)
@@ -467,7 +467,7 @@ class MessageController(storage.MessageBase):
     def bulk_get(self, queue_name, message_ids, project=None):
         message_ids = [mid for mid in map(utils.to_oid, message_ids) if mid]
         if not message_ids:
-            return ()
+            return iter([])
 
         now = timeutils.utcnow()
 

marconi/storage/sqlite/utils.py

@@ -66,7 +66,7 @@ def marker_decode(id):
         return int(id, 8) ^ 0x3c96a355
 
     except ValueError:
-        raise exceptions.MalformedMarker()
+        return None
 
 
 def cid_encode(id):

marconi/tests/storage/base.py

@@ -355,49 +355,41 @@ class MessageControllerTest(ControllerBaseTest):
         self.assertEquals(countof['messages']['free'], 0)
 
     def test_bad_id(self):
-        # A malformed ID should result in an error. This
-        # doesn't hurt anything, since an attacker could just
-        # read the source code anyway to find out how IDs are
-        # implemented. Plus, if someone is just trying to
-        # get a message that they don't own, they would
-        # more likely just list the messages, not try to
-        # guess an ID of an arbitrary message.
-
         # NOTE(cpp-cabrera): A malformed ID should result in an empty
         # query. Raising an exception for validating IDs makes the
         # implementation more verbose instead of taking advantage of
         # the Maybe/Optional protocol, particularly when dealing with
         # bulk operations.
-        queue = 'foo'
-        project = '480924'
-        self.queue_controller.create(queue, project)
-
         bad_message_id = 'xyz'
-        self.controller.delete(queue, bad_message_id, project)
+        self.controller.delete(self.queue_name,
+                               bad_message_id,
+                               project=self.project)
+
         with testing.expect(exceptions.MessageDoesNotExist):
-            self.controller.get(queue, bad_message_id, project)
+            self.controller.get(self.queue_name,
+                                bad_message_id,
+                                project=self.project)
 
     def test_bad_claim_id(self):
-        self.queue_controller.create('unused', '480924')
-        [msgid] = self.controller.post('unused',
+        [msgid] = self.controller.post(self.queue_name,
                                        [{'body': {}, 'ttl': 10}],
-                                       project='480924',
-                                       client_uuid='unused')
+                                       project=self.project,
+                                       client_uuid='my_uuid')
 
         bad_claim_id = '; DROP TABLE queues'
-        self.controller.delete('unused', msgid,
-                               project='480924',
+        self.controller.delete(self.queue_name,
+                               msgid,
+                               project=self.project,
                                claim=bad_claim_id)
 
     def test_bad_marker(self):
-        queue = 'foo'
-        project = '480924'
-        self.queue_controller.create(queue, project)
-
         bad_marker = 'xyz'
-        func = self.controller.list
-        results = func(queue, project, marker=bad_marker)
-        self.assertRaises(exceptions.MalformedMarker, results.next)
+        interaction = self.controller.list(self.queue_name,
+                                           project=self.project,
+                                           marker=bad_marker)
+        messages = list(next(interaction))
+
+        self.assertEquals(messages, [])
 
 
 class ClaimControllerTest(ControllerBaseTest):
@@ -552,12 +544,20 @@ class ClaimControllerTest(ControllerBaseTest):
     def test_illformed_id(self):
         # any ill-formed IDs should be regarded as non-existing ones.
 
-        self.queue_controller.create('unused', '480924')
-        self.controller.delete('unused', 'illformed', '480924')
+        self.controller.delete(self.queue_name,
+                               'illformed',
+                               project=self.project)
 
         with testing.expect(exceptions.DoesNotExist):
-            self.controller.update('unused', 'illformed',
-                                   {'ttl': 40}, '480924')
+            self.controller.get(self.queue_name,
+                                'illformed',
+                                project=self.project)
+
+        with testing.expect(exceptions.DoesNotExist):
+            self.controller.update(self.queue_name,
+                                   'illformed',
+                                   {'ttl': 40},
+                                   project=self.project)
 
 
 def _insert_fixtures(controller, queue_name, project=None,

marconi/tests/transport/wsgi/test_messages.py

@@ -289,7 +289,7 @@ class MessagesBaseTest(base.TestBase):
                           query_string=query_string,
                           headers=self.headers)
 
-        self.assertEqual(self.srmock.status, falcon.HTTP_400)
+        self.assertEqual(self.srmock.status, falcon.HTTP_204)
 
     def test_no_uuid(self):
         path = self.queue_path + '/messages'

marconi/transport/wsgi/messages.py

@@ -102,17 +102,6 @@ class CollectionResource(object):
         except storage_exceptions.DoesNotExist:
             raise falcon.HTTPNotFound()
 
-        except storage_exceptions.MalformedMarker:
-            title = _(u'Invalid query string parameter')
-            description = _(u'The value for the query string '
-                            u'parameter "marker" could not be '
-                            u'parsed. We recommend using the '
-                            u'"next" URI from a previous '
-                            u'request directly, rather than '
-                            u'constructing the URI manually. ')
-
-            raise falcon.HTTPBadRequest(title, description)
-
         except Exception as ex:
             LOG.exception(ex)
             description = _(u'Messages could not be listed.')