From d159350fff1b17a1e41f30c7d682451364905638 Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Fri, 20 Oct 2017 23:05:10 +1000 Subject: [PATCH] Default to reverse ordering by ID in APIs Adjusts the affected tests to assume the exact current set of default test data. These tests were also testing the exact same query URL, so they've been consolidated into a single test. --- README.rst | 24 +++++----- module_build_service/utils.py | 54 ++++++++++++---------- tests/test_views/test_views.py | 83 ++++++++++++---------------------- 3 files changed, 70 insertions(+), 91 deletions(-) diff --git a/README.rst b/README.rst index a595c007..837163a6 100644 --- a/README.rst +++ b/README.rst @@ -208,9 +208,9 @@ module builds are displayed. These parameters are: value defaults to 1. - ``per_page`` - Specifies how many items per page should be displayed (e.g. ``per_page=20``). This value defaults to 10. -- ``order_by`` - a database column to order the API by in ascending order. This defaults to +- ``order_by`` - a database column to order the API by in ascending order. +- ``order_desc_by`` - a database column to order the API by in descending order. This defaults to ``id``. -- ``order_desc_by`` - a database column to order the API by in descending order. An example of querying the "module-builds" resource with the "per_page" and the "page" parameters:: @@ -226,7 +226,7 @@ parameters:: { "items": [ { - "id": 123, + "id": 124, "koji_tag": "module-de66baf89b40367c", "name": "testmodule", "owner": "mprahl", @@ -257,7 +257,7 @@ parameters:: "version": "20171005183359" }, { - "id": 124, + "id": 123, "koji_tag": "module-4620ad476f3d2b5c", "name": "testmodule", "owner": "mprahl", @@ -293,10 +293,10 @@ parameters:: "last": "http://mbs.fedoraproject.org/module-build-service/1/module-builds/?per_page=2&page=340", "next": "http://mbs.fedoraproject.org/module-build-service/1/module-builds/?per_page=2&page=2", "page": 1, - "pages": 340, + "pages": 60, "per_page": 2, "prev": null, - "total": 1020 + "total": 120 } } @@ -319,7 +319,7 @@ parameters:: 57047, 57048 ], - "id": 123, + "id": 124, "koji_tag": "module-de66baf89b40367c", "modulemd": "...." "name": "testmodule", @@ -365,7 +365,7 @@ parameters:: 57045, 57046 ], - "id": 124, + "id": 123, "koji_tag": "module-4620ad476f3d2b5c", "modulemd": "...." "name": "testmodule", @@ -412,10 +412,10 @@ parameters:: "last": "http://mbs.fedoraproject.org/module-build-service/1/module-builds/?verbose=true&per_page=2&page=340", "next": "http://mbs.fedoraproject.org/module-build-service/1/module-builds/?verbose=true&per_page=2&page=2", "page": 1, - "pages": 340, + "pages": 120, "per_page": 2, "prev": null, - "total": 1020 + "total": 60 } } @@ -460,7 +460,7 @@ and the "submitted_before" parameters:: { "items": [ { - "id": 1, + "id": 3, "state": 3, ... }, @@ -470,7 +470,7 @@ and the "submitted_before" parameters:: ... }, { - "id": 3, + "id": 1, "state": 3, ... } diff --git a/module_build_service/utils.py b/module_build_service/utils.py index 89fc1be5..2c899eef 100644 --- a/module_build_service/utils.py +++ b/module_build_service/utils.py @@ -404,6 +404,34 @@ def pagination_metadata(p_query, request_args): return pagination_data +def _add_order_by_clause(flask_request, query, column_source): + """ + Orders the given SQLAlchemy query based on the GET arguments provided + :param flask_request: a Flask request object + :param query: a SQLAlchemy query object + :param column_source: a SQLAlchemy database model + :return: a SQLAlchemy query object + """ + colname = "id" + descending = True + order_desc_by = flask_request.args.get("order_desc_by", None) + if order_desc_by: + colname = order_desc_by + else: + order_by = flask_request.args.get("order_by", None) + if order_by: + colname = order_by + descending = False + + column = getattr(column_source, colname, None) + if not column: + raise ValidationError('An invalid order_by or order_desc_by key ' + 'was supplied') + if descending: + column = column.desc() + return query.order_by(column) + + def filter_component_builds(flask_request): """ Returns a flask_sqlalchemy.Pagination object based on the request parameters @@ -433,18 +461,7 @@ def filter_component_builds(flask_request): if search_query: query = query.filter_by(**search_query) - # Order the results by any column in the ModuleBuild table but default to id. - order_by = flask_request.args.get('order_by', 'id') - order_desc_by = flask_request.args.get('order_desc_by', None) - if order_by or order_desc_by: - column = getattr(models.ComponentBuild, order_desc_by or order_by, None) - if column: - if order_desc_by: - column = column.desc() - query = query.order_by(column) - else: - raise ValidationError('An invalid order_by or order_desc_by key ' - 'was supplied') + query = _add_order_by_clause(flask_request, query, models.ComponentBuild) page = flask_request.args.get('page', 1, type=int) per_page = flask_request.args.get('per_page', 10, type=int) @@ -507,18 +524,7 @@ def filter_module_builds(flask_request): elif context == 'before': query = query.filter(column <= item_datetime) - # Order the results by any column in the ModuleBuild table. - order_by = flask_request.args.get("order_by", None) - order_desc_by = flask_request.args.get("order_desc_by", None) - if order_by or order_desc_by: - column = getattr(models.ModuleBuild, order_desc_by or order_by, None) - if column: - if order_desc_by: - column = column.desc() - query = query.order_by(column) - else: - raise ValidationError('An invalid order_by or order_desc_by key ' - 'was supplied') + query = _add_order_by_clause(flask_request, query, models.ModuleBuild) page = flask_request.args.get('page', 1, type=int) per_page = flask_request.args.get('per_page', 10, type=int) diff --git a/tests/test_views/test_views.py b/tests/test_views/test_views.py index 4032ca1d..3edc79f9 100644 --- a/tests/test_views/test_views.py +++ b/tests/test_views/test_views.py @@ -224,75 +224,48 @@ class TestViews(unittest.TestCase): self.assertEquals(meta_data['page'], 2) def test_query_builds(self): - rv = self.client.get('/module-build-service/1/module-builds/?per_page=2') - item = json.loads(rv.data)['items'][0] - self.assertEquals(item['id'], 1) - self.assertEquals(item['name'], 'nginx') - self.assertEquals(item['owner'], 'Moe Szyslak') - self.assertEquals(item['state'], 3) - self.assertDictEqual(item['tasks'], { - 'rpms': { - 'module-build-macros': { - 'task_id': 12312321, - 'state': 1, - 'state_reason': None, - 'nvr': 'module-build-macros-01-1.module_nginx_1_2', - }, - 'nginx': { - 'task_id': 12312345, - 'state': 1, - 'state_reason': None, - 'nvr': 'nginx-1.10.1-2.module_nginx_1_2', - }, - }, - }) - self.assertEquals(item['time_completed'], '2016-09-03T11:25:32Z') - self.assertEquals(item['time_modified'], '2016-09-03T11:25:32Z') - self.assertEquals(item['time_submitted'], '2016-09-03T11:23:20Z') - - def test_query_builds_not_verbose(self): rv = self.client.get('/module-build-service/1/module-builds/?per_page=2') items = json.loads(rv.data)['items'] expected = [ { - 'id': 1, - 'koji_tag': 'module-nginx-1.2', - 'name': 'nginx', - 'owner': 'Moe Szyslak', - 'scmurl': ('git://pkgs.domain.local/modules/nginx?#ba95886c7a443b36a9ce31abda1f9b' - 'ef22f2f8c9'), - 'state': 3, - 'state_name': 'done', + 'id': 30, + 'koji_tag': None, + 'name': 'testmodule', + 'owner': 'some_other_user', + 'scmurl': ('git://pkgs.domain.local/modules/testmodule?' + '#ca95886c7a443b36a9ce31abda1f9bef22f2f8c9'), + 'state': 1, + 'state_name': 'wait', 'state_reason': None, - 'stream': '1', + 'stream': '4.3.43', 'tasks': { 'rpms': { 'module-build-macros': { - 'nvr': 'module-build-macros-01-1.module_nginx_1_2', + 'nvr': 'module-build-macros-01-1.module_postgresql_1_2', 'state': 1, 'state_reason': None, - 'task_id': 12312321 + 'task_id': 47384002 }, - 'nginx': { - 'nvr': 'nginx-1.10.1-2.module_nginx_1_2', - 'state': 1, + 'rubygem-rails': { + 'nvr': 'postgresql-9.5.3-4.module_postgresql_1_2', + 'state': 3, 'state_reason': None, - 'task_id': 12312345 + 'task_id': 2433442 } } }, - 'time_completed': '2016-09-03T11:25:32Z', - 'time_modified': '2016-09-03T11:25:32Z', - 'time_submitted': '2016-09-03T11:23:20Z', - 'version': '2' + 'time_completed': None, + 'time_modified': '2016-09-03T13:58:40Z', + 'time_submitted': '2016-09-03T13:58:33Z', + 'version': '6' }, - { - 'id': 2, + { + 'id': 29, 'koji_tag': 'module-postgressql-1.2', 'name': 'postgressql', 'owner': 'some_user', - 'scmurl': ('git://pkgs.domain.local/modules/postgressql?#aa95886c7a443b36a' - '9ce31abda1f9bef22f2f8c9'), + 'scmurl': ('git://pkgs.domain.local/modules/postgressql?' + '#aa95886c7a443b36a9ce31abda1f9bef22f2f8c9'), 'state': 3, 'state_name': 'done', 'state_reason': None, @@ -303,19 +276,19 @@ class TestViews(unittest.TestCase): 'nvr': 'module-build-macros-01-1.module_postgresql_1_2', 'state': 1, 'state_reason': None, - 'task_id': 47383993 + 'task_id': 47384002 }, 'postgresql': { 'nvr': 'postgresql-9.5.3-4.module_postgresql_1_2', 'state': 1, 'state_reason': None, - 'task_id': 2433433 + 'task_id': 2433442 } } }, - 'time_completed': '2016-09-03T11:27:19Z', - 'time_modified': '2016-09-03T12:27:19Z', - 'time_submitted': '2016-09-03T12:25:33Z', + 'time_completed': '2016-09-03T12:57:19Z', + 'time_modified': '2016-09-03T13:57:19Z', + 'time_submitted': '2016-09-03T13:55:33Z', 'version': '2' } ]