From 3b45898f254ffa06f088ff7c17b1ccde1872b483 Mon Sep 17 00:00:00 2001 From: Dan Callaghan Date: Apr 24 2018 02:46:37 +0000 Subject: remove 'results' filter from GET /api/v1.0/waivers/ Strictly this is a compatibility break, but nothing we know of is using this parameter, and it is quite awkward to use. Replace it with simple 'subject' and 'testcase' filter parameters, matching the other existing parameters. --- diff --git a/tests/test_api_v10.py b/tests/test_api_v10.py index 35afade..95d61c5 100644 --- a/tests/test_api_v10.py +++ b/tests/test_api_v10.py @@ -262,102 +262,40 @@ def test_get_obsolete_waivers(client, session): assert res_data['data'][1]['id'] == old_waiver.id -def test_filtering_waivers_by_subject_and_testcase(client, session): +def test_filtering_waivers_by_subject(client, session): create_waiver(session, subject={'subject.test1': 'subject1'}, - testcase='testcase1', username='foo-1', product_version='foo-1') + testcase='testcase', username='foo-1', product_version='foo-1') create_waiver(session, subject={'subject.test2': 'subject2'}, - testcase='testcase2', username='foo-2', product_version='foo-1') + testcase='testcase', username='foo-2', product_version='foo-1') - param = json.dumps([{'subject': {'subject.test1': 'subject1'}, 'testcase': 'testcase1'}]) - r = client.get('/api/v1.0/waivers/?results=%s' % param) + r = client.get('/api/v1.0/waivers/?subject=%s' % json.dumps({'subject.test1': 'subject1'})) res_data = json.loads(r.get_data(as_text=True)) assert r.status_code == 200 assert len(res_data['data']) == 1 assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'} - assert res_data['data'][0]['testcase'] == 'testcase1' -def test_filtering_waivers_by_subject_and_testcase_with_other_json_order(client, session): - create_waiver(session, subject={'subject.test1': 'subject1'}, - testcase='testcase1', username='foo-1', product_version='foo-1') - - param = json.dumps([{'testcase': 'testcase1', 'subject': {'subject.test1': 'subject1'}}]) - r = client.get('/api/v1.0/waivers/?results=%s' % param) +def test_filtering_waivers_with_invalid_json_subject(client, session): + r = client.get('/api/v1.0/waivers/?subject=[') + assert r.status_code == 400 res_data = json.loads(r.get_data(as_text=True)) - assert r.status_code == 200 - assert len(res_data['data']) == 1 - assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'} - assert res_data['data'][0]['testcase'] == 'testcase1' + assert res_data['message']['subject'] == \ + 'Invalid JSON: Expecting value: line 1 column 2 (char 1)' -def test_filtering_waivers_by_multiple_results_subjects_and_testcases(client, session): +def test_filtering_waivers_by_testcase(client, session): create_waiver(session, subject={'subject.test1': 'subject1'}, testcase='testcase1', username='foo-1', product_version='foo-1') - create_waiver(session, subject={'subject.test2': 'subject2'}, - testcase='testcase2', username='foo-2', product_version='foo-1') - create_waiver(session, subject={'subject.test3': 'subject3'}, - testcase='testcase3', username='foo-2', product_version='foo-1') - param = json.dumps([{'subject': {'subject.test1': 'subject1'}, 'testcase': 'testcase1'}, - {'subject': {'subject.test2': 'subject2'}, 'testcase': 'testcase2'}]) - 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']) == 2 - assert res_data['data'][0]['subject'] == {'subject.test2': 'subject2'} - assert res_data['data'][0]['testcase'] == 'testcase2' - assert res_data['data'][1]['subject'] == {'subject.test1': 'subject1'} - assert res_data['data'][1]['testcase'] == 'testcase1' - - -def test_filtering_waivers_by_subject_without_testcase(client, session): create_waiver(session, subject={'subject.test1': 'subject1'}, - testcase='testcase1', username='foo-1', product_version='foo-1') - create_waiver(session, subject={'subject.test2': 'subject2'}, testcase='testcase2', username='foo-2', product_version='foo-1') - param = json.dumps([{'subject': {'subject.test1': 'subject1'}}]) - r = client.get('/api/v1.0/waivers/?results=%s' % param) + r = client.get('/api/v1.0/waivers/?testcase=testcase1') res_data = json.loads(r.get_data(as_text=True)) assert r.status_code == 200 assert len(res_data['data']) == 1 - assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'} 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') @@ -482,6 +420,35 @@ def test_get_waivers_with_post_request(client, session): assert all(w['product_version'].startswith('foo-') for w in res_data['data']) +@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): + data = {'results': results} + r = client.post('/api/v1.0/waivers/+by-subjects-and-testcases', data=json.dumps(data), + content_type='application/json') + 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') + data = {'results': results} + r = client.post('/api/v1.0/waivers/+by-subjects-and-testcases', data=json.dumps(data), + content_type='application/json') + res_data = json.loads(r.get_data(as_text=True)) + assert r.status_code == 200 + assert len(res_data['data']) == 1 + + def test_get_waivers_with_post_malformed_since(client, session): create_waiver(session, subject={'subject.test1': 'subject1'}, testcase='testcase1', username='foo', product_version='foo-1') diff --git a/waiverdb/api_v1.py b/waiverdb/api_v1.py index fce492e..48c855d 100644 --- a/waiverdb/api_v1.py +++ b/waiverdb/api_v1.py @@ -25,6 +25,16 @@ def valid_dict(value): return value +def json_dict(value): + try: + value = json.loads(value) + except ValueError as e: + raise ValueError("Invalid JSON: %s" % e) + if not isinstance(value, dict): + raise ValueError("Must be a valid dict, not %r" % value) + return value + + def get_resultsdb_result(result_id): response = requests_session.request('GET', '{0}/results/{1}'.format( current_app.config['RESULTSDB_API_URL'], result_id), @@ -60,7 +70,8 @@ RP['create_waiver'].add_argument('comment', type=str, default=None, location='js RP['create_waiver'].add_argument('username', type=str, default=None, location='json') RP['get_waivers'] = reqparse.RequestParser() -RP['get_waivers'].add_argument('results', location='args') +RP['get_waivers'].add_argument('subject', type=json_dict, location='args') +RP['get_waivers'].add_argument('testcase', location='args') RP['get_waivers'].add_argument('product_version', location='args') RP['get_waivers'].add_argument('username', location='args') RP['get_waivers'].add_argument('include_obsolete', type=bool, default=False, location='args') @@ -108,8 +119,8 @@ class WaiversResource(Resource): :query int page: The page to get. :query int limit: Limit the number of items returned. - :query string results: Filter the waivers by result. Accepts a list of - dictionaries, with one key 'subject' and one key 'testcase'. + :query dict subject: Only include waivers for the given subject. + :query string testcase: Only include waivers for the given test case name. :query string product_version: Filter the waivers by product version. :query string username: Filter the waivers by username. :query string proxied_by: Filter the waivers by the users who are @@ -126,13 +137,10 @@ class WaiversResource(Resource): args = RP['get_waivers'].parse_args() query = Waiver.query.order_by(Waiver.timestamp.desc()) - if args['results']: - 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['subject']: + query = query.filter(Waiver.subject == args['subject']) + if args['testcase']: + query = query.filter(Waiver.testcase == args['testcase']) if args['product_version']: query = query.filter(Waiver.product_version == args['product_version']) if args['username']: