From 0217496d1f01778c017b146677ea6c0d08beb11b Mon Sep 17 00:00:00 2001 From: mprahl Date: Apr 23 2019 12:37:12 +0000 Subject: Refactor the API ordering to accept multiple order keyword arguments of the same direction This also improves how a column is determined to be valid for ordering. --- diff --git a/README.rst b/README.rst index 654f157..bbaaf8a 100644 --- a/README.rst +++ b/README.rst @@ -280,9 +280,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. -- ``order_desc_by`` - a database column to order the API by in descending order. This defaults to - ``id``. +- ``order_by`` - a database column to order the API by in ascending order. Multiple can be provided. +- ``order_desc_by`` - a database column to order the API by in descending order. Multiple can be + provided. This defaults to ``id``. An example of querying the "module-builds" resource with the "per_page" and the "page" parameters:: diff --git a/module_build_service/utils/views.py b/module_build_service/utils/views.py index f46eb06..7c9ef33 100644 --- a/module_build_service/utils/views.py +++ b/module_build_service/utils/views.py @@ -30,6 +30,7 @@ from datetime import datetime from flask import request, url_for, Response from sqlalchemy.sql.sqltypes import Boolean as sqlalchemy_boolean from sqlalchemy.orm import aliased +import sqlalchemy from module_build_service import models, api_version, conf from module_build_service.errors import ValidationError, NotFound @@ -96,30 +97,44 @@ def pagination_metadata(p_query, api_version, request_args): def _add_order_by_clause(flask_request, query, column_source): """ - Orders the given SQLAlchemy query based on the GET arguments provided + 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" + order_by = flask_request.args.getlist('order_by') + order_desc_by = flask_request.args.getlist('order_desc_by') + # Default to ordering by ID in descending order 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) + requested_order = ['id'] + + if order_by and order_desc_by: + raise ValidationError('You may not specify both order_by and order_desc_by') + elif order_by: + descending = False + requested_order = order_by + elif order_desc_by: + descending = True + requested_order = order_desc_by + + column_dict = dict(column_source.__table__.columns) + order_args = [] + for column_name in requested_order: + if column_name not in column_dict: + raise ValidationError( + 'An invalid ordering key of "{}" was supplied'.format(column_name)) + column = column_dict[column_name] + # If the version column is provided, cast it as an integer so the sorting is correct + if column_name == 'version': + column = sqlalchemy.cast(column, sqlalchemy.BigInteger) + if descending: + column = column.desc() + + order_args.append(column) + + return query.order_by(*order_args) def str_to_bool(value): diff --git a/tests/test_views/test_views.py b/tests/test_views/test_views.py index 273870d..9d19f57 100644 --- a/tests/test_views/test_views.py +++ b/tests/test_views/test_views.py @@ -663,6 +663,20 @@ class TestViews: assert items[0]['name'] == 'candy' assert items[1]['name'] == 'nginx' + def test_query_builds_order_by_multiple(self): + init_data(data_size=1, multiple_stream_versions=True) + platform_f28 = db.session.query(module_build_service.models.ModuleBuild).get(1) + platform_f28.version = '150' + db.session.add(platform_f28) + db.session.commit() + rv = self.client.get( + '/module-build-service/1/module-builds/?order_desc_by=stream_version' + '&order_desc_by=version') + items = json.loads(rv.data)['items'] + expected_ids = [8, 6, 4, 1, 2, 12, 3, 5, 7, 9] + actual_ids = [item['id'] for item in items] + assert actual_ids == expected_ids + def test_query_builds_order_desc_by(self): rv = self.client.get('/module-build-service/1/module-builds/?' 'per_page=10&order_desc_by=id') @@ -685,23 +699,28 @@ class TestViews: def test_query_builds_order_by_order_desc_by(self): """ - Test that when both order_by and order_desc_by is set, - we prefer order_desc_by. + Test that when both order_by and order_desc_by are set, an error is returned. """ rv = self.client.get('/module-build-service/1/module-builds/?' 'per_page=10&order_desc_by=id&order_by=name') - items = json.loads(rv.data)['items'] - # Check that the id is items[0]["id"], items[0]["id"] - 1, ... - for idx, item in enumerate(items): - assert item["id"] == items[0]["id"] - idx + error = json.loads(rv.data) + expected = { + 'error': 'Bad Request', + 'message': 'You may not specify both order_by and order_desc_by', + 'status': 400 + } + assert error == expected def test_query_builds_order_by_wrong_key(self): rv = self.client.get('/module-build-service/1/module-builds/?' 'per_page=10&order_by=unknown') - data = json.loads(rv.data) - assert data['status'] == 400 - assert data['error'] == 'Bad Request' - assert data['message'] == 'An invalid order_by or order_desc_by key was supplied' + error = json.loads(rv.data) + expected = { + 'error': 'Bad Request', + 'message': 'An invalid ordering key of "unknown" was supplied', + 'status': 400, + } + assert error == expected def test_query_base_module_br_filters(self): reuse_component_init_data()