From c89a8569db3f6bc4e244a8890f150f0b5c70eaea Mon Sep 17 00:00:00 2001 From: Dmitry Sutyagin Date: Mon, 23 Jan 2017 15:48:31 -0800 Subject: [PATCH] Add limit, offset, order in collection GET Allow limiting the number of objects returned via GET by providing "limit" Example: api/notifications?limit=5 Allow offseting (skipping N first records) via "offset" Example: api/notifications?offset=100 Allow ordering of objects by providing "order_by" Example: api/notifications?order_by=-id Add helper functions/classes to: - get HTTP parameters (limit, offset, order_by) - get scoped collection query by applying 4 operations filter, order, offset, limit - set Conent-Range header if scope limits are present Make default NailgunCollection's GET utilize scoped query This makes default (parent) GET of child handlers support paging and ordering (overriden GET methods will not get this functionality automatically) NailgunCollection.GET is also an example of how to implement this new functionality. Helper functions/classes can be utilized in child handler methods to implement filters / ordering / paging Related-Bug: 1657348 Change-Id: I7760465f70b3f69791e7a0c558a26e8ba55c934a (cherry picked from commit 03aaca2deeb062e962a81d16c2a75fb5fccfc265) --- nailgun/nailgun/api/v1/handlers/base.py | 91 ++++++++++++++- nailgun/nailgun/objects/base.py | 127 +++++++++++++++++---- nailgun/nailgun/test/unit/test_handlers.py | 90 +++++++++++++++ 3 files changed, 283 insertions(+), 25 deletions(-) diff --git a/nailgun/nailgun/api/v1/handlers/base.py b/nailgun/nailgun/api/v1/handlers/base.py index bb4e8d64c1..e63775b52c 100644 --- a/nailgun/nailgun/api/v1/handlers/base.py +++ b/nailgun/nailgun/api/v1/handlers/base.py @@ -123,6 +123,15 @@ class BaseHandler(object): data=self.message ) + class _range_not_satisfiable(web.HTTPError): + message = 'Requested Range Not Satisfiable' + + def __init__(self): + super(_range_not_satisfiable, self).__init__( + status='416 Range Not Satisfiable', + data=self.message + ) + exc_status_map = { 200: web.ok, 201: web.created, @@ -141,6 +150,7 @@ class BaseHandler(object): 409: web.conflict, 410: web.gone, 415: web.unsupportedmediatype, + 416: _range_not_satisfiable, 500: web.internalerror, } @@ -445,12 +455,86 @@ class SingleHandler(BaseHandler): raise self.http(204) +class Pagination(object): + """Get pagination scope from init or HTTP request arguments""" + + def convert(self, x): + """ret. None if x=None, else ret. x as int>=0; else raise 400""" + + val = x + if val is not None: + if type(val) is not int: + try: + val = int(x) + except ValueError: + raise BaseHandler.http(400, 'Cannot convert "%s" to int' + % x) + # raise on negative values + if val < 0: + raise BaseHandler.http(400, 'Negative limit/offset not \ + allowed') + return val + + def get_order_by(self, order_by): + if order_by: + order_by = [s.strip() for s in order_by.split(',') if s.strip()] + return order_by if order_by else None + + def __init__(self, limit=None, offset=None, order_by=None): + if limit is not None or offset is not None or order_by is not None: + # init with provided arguments + self.limit = self.convert(limit) + self.offset = self.convert(offset) + self.order_by = self.get_order_by(order_by) + else: + # init with HTTP arguments + self.limit = self.convert(web.input(limit=None).limit) + self.offset = self.convert(web.input(offset=None).offset) + self.order_by = self.get_order_by(web.input(order_by=None) + .order_by) + + class CollectionHandler(BaseHandler): collection = None validator = BasicValidator eager = () + def get_scoped_query_and_range(self, pagination=None, filter_by=None): + """Get filtered+paged collection query and collection.ContentRange obj + + Return a scoped query, and if pagination is requested then also return + ContentRange object (see NailgunCollection.content_range) to allow to + set Content-Range header (outside of this functon). + If pagination is not set/requested, return query to all collection's + objects. + Allows getting object count without getting objects - via + content_range if pagination.limit=0. + + :param pagination: Pagination object + :param filter_by: filter dict passed to query.filter_by(\*\*dict) + :type filter_by: dict + :returns: SQLAlchemy query and ContentRange object + """ + pagination = pagination or Pagination() + query = None + content_range = None + if self.collection and self.collection.single.model: + query, content_range = self.collection.scope(pagination, filter_by) + if content_range: + if not content_range.valid: + raise self.http(416, 'Requested range "%s" cannot be ' + 'satisfied' % content_range) + return query, content_range + + def set_content_range(self, content_range): + """Set Content-Range header to indicate partial data + + :param content_range: NailgunCollection.content_range named tuple + """ + txt = 'objects {x.first}-{x.last}/{x.total}'.format(x=content_range) + web.header('Content-Range', txt) + @handle_errors @validate @serialize @@ -458,8 +542,13 @@ class CollectionHandler(BaseHandler): """:returns: Collection of JSONized REST objects. :http: * 200 (OK) + * 400 (Bad Request) + * 406 (requested range not satisfiable) """ - q = self.collection.eager(None, self.eager) + query, content_range = self.get_scoped_query_and_range() + if content_range: + self.set_content_range(content_range) + q = self.collection.eager(query, self.eager) return self.collection.to_list(q) @handle_errors diff --git a/nailgun/nailgun/objects/base.py b/nailgun/nailgun/objects/base.py index 71574d4072..98db8ba0aa 100644 --- a/nailgun/nailgun/objects/base.py +++ b/nailgun/nailgun/objects/base.py @@ -169,6 +169,30 @@ class NailgunCollection(object): #: Single object class single = NailgunObject + @classmethod + def content_range(cls, first, last, total, valid): + """Structure to set Content-Range header + + Defines structure necessary to implement paged requests. + "total" is needed to let client calculate how many pages are available. + "valid" is used to indicate that the requested page is valid + (contains data) or not (outside of data range). + Used in NailgunCollection.scope() + + :param first: first element (row) returned + :param last: last element (row) returned + :param total: total number of elements/rows (before pagination) + :param valid: whether the pagination is within data range or not + :returns: ContentRange object (collections.namedtuple) with 4 fields + """ + rng = collections.namedtuple('ContentRange', + ['first', 'last', 'total', 'valid']) + rng.first = first + rng.last = last + rng.total = total + rng.valid = valid + return rng + @classmethod def _is_iterable(cls, obj): return isinstance( @@ -191,6 +215,53 @@ class NailgunCollection(object): """ return db().query(cls.single.model) + @classmethod + def scope(cls, pagination=None, filter_by=None): + """Return a query to collection's objects and ContentRange object + + Return a filtered and paged query, according to the provided pagination + (see api.v1.handlers.base.Pagination) + Also return ContentRange - object with index of first element, last + element and total count of elements in query(after filtering), and + a 'valid' parameter to indicate that the paging scope (limit + offset) + is valid or not (resulted in no data while there was data to provide) + + :param pagination: Pagination object + :param filter_by: dict to filter objects {field1: value1, ...} + :returns: SQLAlchemy query and ContentRange object + """ + query = cls.all() + content_range = None + if filter_by: + query = query.filter_by(**filter_by) + query_full = query + if pagination: + if pagination.limit > 0 or pagination.limit is None: + if pagination.order_by: + query = cls.order_by(query, pagination.order_by) + if pagination.offset: + query = query.offset(pagination.offset) + if pagination.limit > 0: + query = query.limit(pagination.limit) + else: + # making an empty result + query = query.filter(False) + if pagination.offset or pagination.limit is not None: + total = query_full.count() + selected = query.count() if pagination.limit != 0 else 0 + # first element index=1 + first = pagination.offset + 1 if pagination.offset else 1 + if selected == 0 or pagination.limit == 0: + # no data, report first and last as 0 + first = last = 0 + elif pagination.limit > 0: + last = min(first + pagination.limit - 1, total) + else: + last = total + valid = selected > 0 or pagination.limit == 0 or total == 0 + content_range = cls.content_range(first, last, total, valid) + return query, content_range + @classmethod def _query_order_by(cls, query, order_by): """Adds order by clause into SQLAlchemy query @@ -235,6 +306,23 @@ class NailgunCollection(object): order_by=order_by_fields)) return sorted(iterable, key=key) + @classmethod + def get_iterable(cls, iterable, require=True): + """Return either iterable or cls.all() when possible + + :param iterable: model objects collection + :returns: original iterable or an SQLAlchemy query + """ + if iterable is not None: + if cls._is_iterable(iterable) or cls._is_query(iterable): + return iterable + else: + raise TypeError("'%s' object is not iterable" % type(iterable)) + elif cls.single.model: + return cls.all() + elif require: + raise ValueError('iterable not provided and single.model not set') + @classmethod def order_by(cls, iterable, order_by): """Order given iterable by specified order_by. @@ -244,15 +332,17 @@ class NailgunCollection(object): ORDER BY criterion to SQLAlchemy query. If name starts with '-' desc ordering applies, else asc. :type order_by: tuple of strings or string + :returns: ordered iterable (SQLAlchemy query) """ - if iterable is None or not order_by: + if not iterable or not order_by: return iterable + use_iterable = cls.get_iterable(iterable) if not isinstance(order_by, (list, tuple)): order_by = (order_by,) - if cls._is_query(iterable): - return cls._query_order_by(iterable, order_by) - else: - return cls._iterable_order_by(iterable, order_by) + if cls._is_query(use_iterable): + return cls._query_order_by(use_iterable, order_by) + elif cls._is_iterable(use_iterable): + return cls._iterable_order_by(use_iterable, order_by) @classmethod def filter_by(cls, iterable, **kwargs): @@ -266,10 +356,7 @@ class NailgunCollection(object): else asc. :returns: filtered iterable (SQLAlchemy query) """ - if iterable is not None: - use_iterable = iterable - else: - use_iterable = cls.all() + use_iterable = cls.get_iterable(iterable) if cls._is_query(use_iterable): return use_iterable.filter_by(**kwargs) elif cls._is_iterable(use_iterable): @@ -291,7 +378,7 @@ class NailgunCollection(object): :param iterable: iterable (SQLAlchemy query) :returns: filtered iterable (SQLAlchemy query) """ - use_iterable = iterable or cls.all() + use_iterable = cls.get_iterable(iterable) if cls._is_query(use_iterable): conditions = [] for key, value in six.iteritems(kwargs): @@ -306,8 +393,6 @@ class NailgunCollection(object): ), use_iterable ) - else: - raise TypeError("First argument should be iterable") @classmethod def lock_for_update(cls, iterable): @@ -318,15 +403,13 @@ class NailgunCollection(object): :param iterable: iterable (SQLAlchemy query) :returns: filtered iterable (SQLAlchemy query) """ - use_iterable = iterable or cls.all() + use_iterable = cls.get_iterable(iterable) if cls._is_query(use_iterable): return use_iterable.with_lockmode('update') elif cls._is_iterable(use_iterable): # we can't lock abstract iterable, so returning as is # for compatibility return use_iterable - else: - raise TypeError("First argument should be iterable") @classmethod def filter_by_list(cls, iterable, field_name, list_of_values, @@ -341,7 +424,7 @@ class NailgunCollection(object): :returns: filtered iterable (SQLAlchemy query) """ field_getter = operator.attrgetter(field_name) - use_iterable = iterable or cls.all() + use_iterable = cls.get_iterable(iterable) if cls._is_query(use_iterable): result = use_iterable.filter( field_getter(cls.single.model).in_(list_of_values) @@ -353,8 +436,6 @@ class NailgunCollection(object): lambda i: field_getter(i) in list_of_values, use_iterable ) - else: - raise TypeError("First argument should be iterable") @classmethod def filter_by_id_list(cls, iterable, uid_list): @@ -382,7 +463,7 @@ class NailgunCollection(object): :param options: list of sqlalchemy eagerload types :returns: iterable (SQLAlchemy query) """ - use_iterable = iterable or cls.all() + use_iterable = cls.get_iterable(iterable) if options: return use_iterable.options(*options) return use_iterable @@ -404,13 +485,11 @@ class NailgunCollection(object): @classmethod def count(cls, iterable=None): - use_iterable = iterable or cls.all() + use_iterable = cls.get_iterable(iterable) if cls._is_query(use_iterable): return use_iterable.count() elif cls._is_iterable(use_iterable): return len(list(iterable)) - else: - raise TypeError("First argument should be iterable") @classmethod def to_list(cls, iterable=None, fields=None, serializer=None): @@ -423,7 +502,7 @@ class NailgunCollection(object): :param serializer: the custom serializer :returns: collection of objects as a list of dicts """ - use_iterable = cls.all() if iterable is None else iterable + use_iterable = cls.get_iterable(iterable) return [ cls.single.to_dict(o, fields=fields, serializer=serializer) for o in use_iterable @@ -465,7 +544,7 @@ class NailgunCollection(object): :param options: list of sqlalchemy mapper options :returns: iterable (SQLAlchemy query) """ - use_iterable = iterable or cls.all() + use_iterable = cls.get_iterable(iterable) if args: return use_iterable.options(*args) return use_iterable diff --git a/nailgun/nailgun/test/unit/test_handlers.py b/nailgun/nailgun/test/unit/test_handlers.py index e53eb06411..6c80f4b351 100644 --- a/nailgun/nailgun/test/unit/test_handlers.py +++ b/nailgun/nailgun/test/unit/test_handlers.py @@ -20,9 +20,13 @@ import urllib import web from nailgun.api.v1.handlers.base import BaseHandler +from nailgun.api.v1.handlers.base import CollectionHandler from nailgun.api.v1.handlers.base import handle_errors +from nailgun.api.v1.handlers.base import Pagination from nailgun.api.v1.handlers.base import serialize +from nailgun.objects import NodeCollection + from nailgun.test.base import BaseIntegrationTest from nailgun.utils import reverse @@ -185,3 +189,89 @@ class TestHandlers(BaseIntegrationTest): def test_get_requested_default(self): self.check_get_requested_mime({}, 'application/json') + + def test_pagination_class(self): + # test empty query + web.ctx.env = {'REQUEST_METHOD': 'GET'} + pagination = Pagination() + self.assertEqual(pagination.limit, None) + self.assertEqual(pagination.offset, None) + self.assertEqual(pagination.order_by, None) + # test value retrieval from web + order_by cleanup + q = 'limit=1&offset=5&order_by=-id, timestamp , somefield ' + web.ctx.env['QUERY_STRING'] = q + pagination = Pagination() + self.assertEqual(pagination.limit, 1) + self.assertEqual(pagination.offset, 5) + self.assertEqual(set(pagination.order_by), + set(['-id', 'timestamp', 'somefield'])) + # test incorrect values raise 400 + web.ctx.env['QUERY_STRING'] = 'limit=qwe' + self.assertRaises(web.HTTPError, Pagination) + web.ctx.env['QUERY_STRING'] = 'offset=asd' + self.assertRaises(web.HTTPError, Pagination) + web.ctx.env['QUERY_STRING'] = 'limit=' + self.assertRaises(web.HTTPError, Pagination) + web.ctx.env['QUERY_STRING'] = 'offset=-2' + self.assertRaises(web.HTTPError, Pagination) + # test constructor, limit = 0 -> 0, offset '0' -> 0, bad order_by + pagination = Pagination(0, '0', ', ,,, ,') + self.assertEqual(pagination.limit, 0) + self.assertEqual(pagination.offset, 0) + self.assertEqual(pagination.order_by, None) + + def test_pagination_of_node_collection(self): + def assert_pagination_and_cont_rng(q, cr, sz, first, last, ttl, valid): + self.assertEqual(q.count(), sz) + self.assertEqual(cr.first, first) + self.assertEqual(cr.last, last) + self.assertEqual(cr.total, ttl) + self.assertEqual(cr.valid, valid) + + self.env.create_nodes(5) + # test pagination limited to 2 first items + pagination = Pagination(limit=2) + q, cr = NodeCollection.scope(pagination) + assert_pagination_and_cont_rng(q, cr, 2, 1, 2, 5, True) + # test invalid pagination + pagination = Pagination(offset=5) + q, cr = NodeCollection.scope(pagination) + assert_pagination_and_cont_rng(q, cr, 0, 0, 0, 5, False) + # test limit=0, offset ignored + pagination = Pagination(limit=0, offset=999) + q, cr = NodeCollection.scope(pagination) + assert_pagination_and_cont_rng(q, cr, 0, 0, 0, 5, True) + # test limit+offset+order_by + pagination = Pagination(limit=3, offset=1, order_by='-id') + q, cr = NodeCollection.scope(pagination) + assert_pagination_and_cont_rng(q, cr, 3, 2, 4, 5, True) + ids = sorted([i.id for i in self.env.nodes]) + n = q.all() + self.assertEqual(n[0].id, ids[3]) + self.assertEqual(n[1].id, ids[2]) + self.assertEqual(n[2].id, ids[1]) + + def test_collection_handler(self): + FakeHandler = CollectionHandler + # setting a collection is mandatory, CollectionHandler is not ready + # to use "as-is" + FakeHandler.collection = NodeCollection + urls = ("/collection_test", "collection_test") + app = web.application(urls, {'collection_test': FakeHandler}) + resp = app.request(urls[0]) + self.assertEqual(resp.status, '200 OK') + + def test_content_range_header(self): + self.env.create_nodes(5) + FakeHandler = CollectionHandler + FakeHandler.collection = NodeCollection + urls = ("/collection_test", "collection_test") + app = web.application(urls, {'collection_test': FakeHandler}) + # test paginated query + resp = app.request("/collection_test?limit=3&offset=1") + self.assertEqual(resp.status, '200 OK') + self.assertIn('Content-Range', resp.headers) + self.assertEqual(resp.headers['Content-Range'], 'objects 2-4/5') + # test invalid range (offset = 6 >= number of nodes ---> no data) + resp = app.request("/collection_test?limit=3&offset=5&order_by=id") + self.assertEqual(resp.status, '416 Range Not Satisfiable')