From ba627bd5375cdcf8af3febadee635378089981a1 Mon Sep 17 00:00:00 2001 From: Lukas Holecek Date: Apr 23 2018 08:51:52 +0000 Subject: Validate "results" filter for waivers Fixes constructing invalid SQL query ("... WHERE () AND ..."). Additionally returns status code 400 if a dict in "results" contains unknown key. This can help users to fix some mistakes quickly. Fixes #162 --- diff --git a/tests/test_api_v10.py b/tests/test_api_v10.py index 21dbdcf..ad3eccf 100644 --- a/tests/test_api_v10.py +++ b/tests/test_api_v10.py @@ -324,6 +324,40 @@ def test_filtering_waivers_by_subject_without_testcase(client, session): assert res_data['data'][0]['testcase'] == 'testcase1' +@pytest.mark.parametrize("results", [ + [{'item': {'subject.test1': 'subject1'}}], # Unexpected key + [{'subject': 'subject1'}], # Unexpected key type +]) +def test_filtering_waivers_with_bad_key(client, session, results): + param = json.dumps(results) + r = client.get('/api/v1.0/waivers/?results=%s' % param) + res_data = json.loads(r.get_data(as_text=True)) + assert r.status_code == 400 + assert "'results' parameter should be a list of dictionaries with subject and testcase" \ + in res_data.get('message') + + +@pytest.mark.parametrize("results", [ + [], + [{}], +]) +def test_filtering_waivers_with_empty_results(client, session, results): + create_waiver(session, subject={'subject.test1': 'subject1'}, + testcase='testcase1', username='foo-1', product_version='foo-1') + param = json.dumps(results) + r = client.get('/api/v1.0/waivers/?results=%s' % param) + res_data = json.loads(r.get_data(as_text=True)) + assert r.status_code == 200 + assert len(res_data['data']) == 1 + + +def test_filtering_waivers_with_invalid_json(client, session): + r = client.get('/api/v1.0/waivers/?results=[') + res_data = json.loads(r.get_data(as_text=True)) + assert r.status_code == 400 + assert "'results' parameter should be in JSON format" in res_data.get('message') + + def test_filtering_waivers_by_product_version(client, session): create_waiver(session, subject={'subject.test1': 'subject1'}, testcase='testcase1', username='foo-1', product_version='release-1') diff --git a/waiverdb/api_v1.py b/waiverdb/api_v1.py index 10d6bd1..41d7155 100644 --- a/waiverdb/api_v1.py +++ b/waiverdb/api_v1.py @@ -34,6 +34,19 @@ def get_resultsdb_result(result_id): return response.json() +def _validate_results_filter(results): + expected = { + 'subject': dict, + 'testcase': str, + } + for item in results: + for k, v in item.items(): + if not (k in expected and isinstance(v, expected[k])): + raise BadRequest( + ("'results' parameter should be a list of dictionaries with" + " subject and testcase")) + + # RP contains request parsers (reqparse.RequestParser). # Parsers are added in each 'resource section' for better readability RP = {} @@ -114,16 +127,11 @@ class WaiversResource(Resource): query = Waiver.query.order_by(Waiver.timestamp.desc()) if args['results']: - results = json.loads(args['results']) - for d in results: - if d.get('subject', None): - if not isinstance(d.get('subject', None), dict): - raise BadRequest("'results' parameter should be a list \ - of dictionaries with subject and testcase") - if d.get('testcase', None): - if not isinstance(d.get('testcase', None), str): - raise BadRequest("'results' parameter should be a list \ - of dictionaries with subject and testcase") + try: + results = json.loads(args['results']) + except json.JSONDecodeError: + raise BadRequest("'results' parameter should be in JSON format") + _validate_results_filter(results) query = Waiver.by_results(query, results) if args['product_version']: query = query.filter(Waiver.product_version == args['product_version']) @@ -368,17 +376,10 @@ class GetWaiversBySubjectsAndTestcases(Resource): raise UnsupportedMediaType('No JSON payload in request') data = request.get_json() query = Waiver.query.order_by(Waiver.timestamp.desc()) - if data.get('results'): - for d in data['results']: - if d.get('subject', None): - if not isinstance(d.get('subject', None), dict): - raise BadRequest("'results' parameter should be a list \ - of dictionaries with subject and testcase") - if d.get('testcase', None): - if not isinstance(d.get('testcase', None), str): - raise BadRequest("'results' parameter should be a list \ - of dictionaries with subject and testcase") - query = Waiver.by_results(query, data['results']) + if 'results' in data: + results = data['results'] + _validate_results_filter(results) + query = Waiver.by_results(query, results) if 'product_version' in data: query = query.filter(Waiver.product_version == data['product_version']) if 'username' in data: diff --git a/waiverdb/models/waivers.py b/waiverdb/models/waivers.py index c0a2a39..59381b4 100644 --- a/waiverdb/models/waivers.py +++ b/waiverdb/models/waivers.py @@ -36,13 +36,27 @@ class Waiver(db.Model): @classmethod def by_results(cls, query, results): - return query.filter(or_(*[ - and_( - cls.subject == d['subject'], - cls.testcase == d['testcase'] - ) if d.get('testcase', None) else - and_( - cls.subject == d['subject'] - ) if d.get('subject', None) else - and_() for d in results - ])) + """ + Filter ``query`` by matching with at least one filter in ``results``. + + If ``results`` is empty, ``query`` is not filtered. + + Args: + query (flask_sqlalchemy.BaseQuery) + results (list): each item should be dict containing + "subject" (dict) and "testcase" (str), both optional + + Returns: + Filtered query. + """ + clauses = [] + for result in results: + subject = result.get('subject', None) + testcase = result.get('testcase', None) + if subject or testcase: + clauses.append(and_( + not subject or cls.subject == subject, + not testcase or cls.testcase == testcase + )) + + return query.filter(or_(*clauses))