From 3662ec2e90dcb2925549af48d48f9c4f0a4f360d Mon Sep 17 00:00:00 2001 From: Giulia Naponiello Date: Jan 23 2018 14:00:55 +0000 Subject: [PATCH 1/2] Provide a way to waive the absence of a test result This change is needed to submit a new waiver (using the API) for tests which do not have a result in ResultDB. To achieve this we replace the result_id with a ResultDB subject and a ResultDB testcase The changes include: * api changes * database migration of older waivers * changes to tests so they worked with new api --- diff --git a/requirements.txt b/requirements.txt index 14951b7..7a19292 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,6 +11,7 @@ SQLAlchemy kerberos >= 1.1.1 flask-oidc systemd +sqlalchemy-utils # packages for the unit tests pytest >= 2.4.2 mock diff --git a/tests/test_api_v10.py b/tests/test_api_v10.py index b25f666..8da3d90 100644 --- a/tests/test_api_v10.py +++ b/tests/test_api_v10.py @@ -11,7 +11,8 @@ from waiverdb import __version__ @patch('waiverdb.auth.get_user', return_value=('foo', {})) def test_create_waiver(mocked_get_user, client, session): data = { - 'result_id': 123, + 'subject': {'subject.test': 'subject'}, + 'testcase': 'testcase1', 'product_version': 'fool-1', 'waived': True, 'comment': 'it broke', @@ -21,28 +22,43 @@ def test_create_waiver(mocked_get_user, client, session): res_data = json.loads(r.get_data(as_text=True)) assert r.status_code == 201 assert res_data['username'] == 'foo' - assert res_data['result_id'] == 123 + assert res_data['subject'] == {'subject.test': 'subject'} + assert res_data['testcase'] == 'testcase1' assert res_data['product_version'] == 'fool-1' assert res_data['waived'] is True assert res_data['comment'] == 'it broke' @patch('waiverdb.auth.get_user', return_value=('foo', {})) -def test_create_waiver_with_malformed_data(mocked_get_user, client): +def test_create_waiver_with_no_testcase(mocked_get_user, client): data = { - 'result_id': 'wrong id', + 'subject': {'foo': 'bar'}, } r = client.post('/api/v1.0/waivers/', data=json.dumps(data), content_type='application/json') res_data = json.loads(r.get_data(as_text=True)) assert r.status_code == 400 - assert 'invalid literal for int()' in res_data['message']['result_id'] + assert 'Missing required parameter in the JSON body' in res_data['message']['testcase'] + + +@patch('waiverdb.auth.get_user', return_value=('foo', {})) +def test_create_waiver_with_malformed_subject(mocked_get_user, client): + data = { + 'subject': 'asd', + 'testcase': 'qqq', + } + r = client.post('/api/v1.0/waivers/', data=json.dumps(data), + content_type='application/json') + res_data = json.loads(r.get_data(as_text=True)) + assert r.status_code == 400 + assert 'Must be a valid dict' in res_data['message']['subject'] @patch('waiverdb.auth.get_user', return_value=('foo', {})) def test_non_superuser_cannot_create_waiver_for_other_users(mocked_get_user, client): data = { - 'result_id': 123, + 'subject': {'subject.test': 'subject'}, + 'testcase': 'testcase1', 'product_version': 'fool-1', 'waived': True, 'comment': 'it broke', @@ -58,7 +74,8 @@ def test_non_superuser_cannot_create_waiver_for_other_users(mocked_get_user, cli @patch('waiverdb.auth.get_user', return_value=('bodhi', {})) def test_superuser_can_create_waiver_for_other_users(mocked_get_user, client, session): data = { - 'result_id': 123, + 'subject': {'subject.test': 'subject'}, + 'testcase': 'testcase1', 'product_version': 'fool-1', 'waived': True, 'comment': 'it broke', @@ -75,13 +92,15 @@ def test_superuser_can_create_waiver_for_other_users(mocked_get_user, client, se def test_get_waiver(client, session): # create a new waiver - waiver = create_waiver(session, result_id=123, username='foo', + waiver = create_waiver(session, subject={'subject.test': 'subject'}, + testcase='testcase1', username='foo', product_version='foo-1', comment='bla bla bla') r = client.get('/api/v1.0/waivers/%s' % waiver.id) res_data = json.loads(r.get_data(as_text=True)) assert r.status_code == 200 assert res_data['username'] == waiver.username - assert res_data['result_id'] == waiver.result_id + assert res_data['subject'] == waiver.subject + assert res_data['testcase'] == waiver.testcase assert res_data['product_version'] == waiver.product_version assert res_data['waived'] is True assert res_data['comment'] == waiver.comment @@ -107,7 +126,8 @@ def test_500(mocked, client, session): def test_get_waivers(client, session): for i in range(0, 10): - create_waiver(session, result_id=i, username='foo %d' % i, + create_waiver(session, subject={"subject%d" % i: "%d" % i}, + testcase="case %d" % i, username='foo %d' % i, product_version='foo-%d' % i, comment='bla bla bla') r = client.get('/api/v1.0/waivers/') res_data = json.loads(r.get_data(as_text=True)) @@ -117,7 +137,8 @@ def test_get_waivers(client, session): def test_pagination_waivers(client, session): for i in range(0, 30): - create_waiver(session, result_id=i, username='foo %d' % i, + create_waiver(session, subject={"subject%d" % i: "%d" % i}, + testcase="case %d" % i, username='foo %d' % i, product_version='foo-%d' % i, comment='bla bla bla') r = client.get('/api/v1.0/waivers/?page=2') res_data = json.loads(r.get_data(as_text=True)) @@ -130,9 +151,11 @@ def test_pagination_waivers(client, session): def test_obsolete_waivers_are_excluded_by_default(client, session): - create_waiver(session, result_id=123, username='foo', + create_waiver(session, subject={'subject.test': 'subject'}, + testcase='testcase1', username='foo', product_version='foo-1') - new_waiver = create_waiver(session, result_id=123, username='foo', + new_waiver = create_waiver(session, subject={'subject.test': 'subject'}, + testcase='testcase1', username='foo', product_version='foo-1', waived=False) r = client.get('/api/v1.0/waivers/') res_data = json.loads(r.get_data(as_text=True)) @@ -143,9 +166,11 @@ def test_obsolete_waivers_are_excluded_by_default(client, session): def test_get_obsolete_waivers(client, session): - old_waiver = create_waiver(session, result_id=123, username='foo', + old_waiver = create_waiver(session, subject={'subject.test': 'subject'}, + testcase='testcase1', username='foo', product_version='foo-1') - new_waiver = create_waiver(session, result_id=123, username='foo', + new_waiver = create_waiver(session, subject={'subject.test': 'subject'}, + testcase='testcase1', username='foo', product_version='foo-1', waived=False) r = client.get('/api/v1.0/waivers/?include_obsolete=1') res_data = json.loads(r.get_data(as_text=True)) @@ -155,31 +180,60 @@ def test_get_obsolete_waivers(client, session): assert res_data['data'][1]['id'] == old_waiver.id -def test_filtering_waivers_by_result_id(client, session): - create_waiver(session, result_id=123, username='foo-1', product_version='foo-1') - create_waiver(session, result_id=234, username='foo-2', product_version='foo-1') - r = client.get('/api/v1.0/waivers/?result_id=123') +def test_filtering_waivers_by_subject_and_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'}, 'testcase': 'testcase1'}]) + 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 - assert res_data['data'][0]['result_id'] == 123 + assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'} + assert res_data['data'][0]['testcase'] == 'testcase1' + + +def test_filtering_waivers_by_multiple_results_subjects_and_testcases(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_multiple_result_ids(client, session): - create_waiver(session, result_id=123, username='foo-1', product_version='foo-1') - create_waiver(session, result_id=234, username='foo-2', product_version='foo-1') - create_waiver(session, result_id=345, username='foo-2', product_version='foo-1') - r = client.get('/api/v1.0/waivers/?result_id=123,345') +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) 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]['result_id'] == 345 - assert res_data['data'][1]['result_id'] == 123 + 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_product_version(client, session): - create_waiver(session, result_id=123, username='foo-1', product_version='release-1') - create_waiver(session, result_id=124, username='foo-1', product_version='release-2') + create_waiver(session, subject={'subject.test1': 'subject1'}, + testcase='testcase1', username='foo-1', product_version='release-1') + create_waiver(session, subject={'subject.test2': 'subject2'}, + testcase='testcase2', username='foo-1', product_version='release-2') r = client.get('/api/v1.0/waivers/?product_version=release-1') res_data = json.loads(r.get_data(as_text=True)) assert r.status_code == 200 @@ -188,8 +242,10 @@ def test_filtering_waivers_by_product_version(client, session): def test_filtering_waivers_by_username(client, session): - create_waiver(session, result_id=123, username='foo', product_version='foo-1') - create_waiver(session, result_id=124, username='bar', product_version='foo-2') + create_waiver(session, subject={'subject.test1': 'subject1'}, + testcase='testcase1', username='foo', product_version='foo-1') + create_waiver(session, subject={'subject.test2': 'subject2'}, + testcase='testcase2', username='bar', product_version='foo-2') r = client.get('/api/v1.0/waivers/?username=foo') res_data = json.loads(r.get_data(as_text=True)) assert r.status_code == 200 @@ -201,18 +257,21 @@ def test_filtering_waivers_by_since(client, session): before1 = (datetime.datetime.utcnow() - datetime.timedelta(seconds=100)).isoformat() before2 = (datetime.datetime.utcnow() - datetime.timedelta(seconds=99)).isoformat() after = (datetime.datetime.utcnow() + datetime.timedelta(seconds=100)).isoformat() - create_waiver(session, result_id=123, username='foo', product_version='foo-1') + create_waiver(session, subject={'subject.test1': 'subject1'}, + testcase='testcase1', username='foo', product_version='foo-1') r = client.get('/api/v1.0/waivers/?since=%s' % before1) 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]['result_id'] == 123 + assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'} + assert res_data['data'][0]['testcase'] == 'testcase1' r = client.get('/api/v1.0/waivers/?since=%s,%s' % (before1, after)) 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]['result_id'] == 123 + assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'} + assert res_data['data'][0]['testcase'] == 'testcase1' r = client.get('/api/v1.0/waivers/?since=%s' % (after)) res_data = json.loads(r.get_data(as_text=True)) @@ -225,19 +284,33 @@ def test_filtering_waivers_by_since(client, session): assert len(res_data['data']) == 0 +def test_filtering_waivers_by_malformed_since(client, session): + create_waiver(session, subject={'subject.test1': 'subject1'}, + testcase='testcase1', username='foo', product_version='foo-1') + wrong_since = 123 + r = client.get('/api/v1.0/waivers/?since=%s' % wrong_since) + res_data = json.loads(r.get_data(as_text=True)) + assert r.status_code == 400 + assert res_data['message'] == "'since' parameter not in ISO8601 format" + + def test_filtering_waivers_by_proxied_by(client, session): - create_waiver(session, result_id=123, username='foo-1', product_version='foo-1', + create_waiver(session, subject={'subject.test1': 'subject1'}, + testcase='testcase1', username='foo-1', product_version='foo-1', proxied_by='bodhi') - create_waiver(session, result_id=234, username='foo-2', product_version='foo-1') + create_waiver(session, subject={'subject.test2': 'subject2'}, + testcase='testcase2', username='foo-2', product_version='foo-1') r = client.get('/api/v1.0/waivers/?proxied_by=bodhi') 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]['result_id'] == 123 + assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'} + assert res_data['data'][0]['testcase'] == 'testcase1' def test_jsonp(client, session): - waiver = create_waiver(session, result_id=123, username='foo', product_version='foo-1') + waiver = create_waiver(session, subject={'subject.test1': 'subject1'}, + testcase='testcase1', username='foo', product_version='foo-1') r = client.get('/api/v1.0/waivers/%s?callback=jsonpcallback' % waiver.id) assert r.mimetype == 'application/javascript' assert 'jsonpcallback' in r.get_data(as_text=True) @@ -252,23 +325,45 @@ def test_healthcheck(client): def test_get_waivers_with_post_request(client, session): """ This tests that users can get waivers by sending a POST request with a long - list of result ids. + list of result subject/testcase. """ - result_ids = [] + results = [] for i in range(1, 51): - result_ids.append(str(i)) - create_waiver(session, result_id=i, username='foo', product_version='foo-1') + results.append({'subject': {'subject%d' % i: '%d' % i}, + 'testcase': 'case %d' % i}) + create_waiver(session, subject={"subject%d" % i: "%d" % i}, + testcase="case %d" % i, username='foo %d' % i, + product_version='foo-%d' % i, comment='bla bla bla') data = { - 'result_ids': result_ids + 'results': results } - r = client.post('/api/v1.0/waivers/+by-result-ids', data=json.dumps(data), + 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']) == 50 - assert set([w['result_id'] for w in res_data['data']]) == set(range(1, 51)) - assert all(w['username'] == 'foo' for w in res_data['data']) - assert all(w['product_version'] == 'foo-1' for w in res_data['data']) + subjects = [] + testcases = [] + for i in reversed(range(1, 51)): + subjects.append({'subject%d' % i: '%d' % i}) + testcases.append('case %d' % i) + assert [w['subject'] for w in res_data['data']] == subjects + assert set([w['testcase'] for w in res_data['data']]) == set(testcases) + assert all(w['username'].startswith('foo') for w in res_data['data']) + assert all(w['product_version'].startswith('foo-') for w in res_data['data']) + + +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') + data = { + 'since': 123 + } + 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 res_data['message'] == "'since' parameter not in ISO8601 format" def test_about_endpoint(client): diff --git a/tests/test_auth.py b/tests/test_auth.py index 71a896d..3091db5 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -38,7 +38,8 @@ class TestKerberosAuthentication(object): client, monkeypatch, session): monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab') data = { - 'result_id': 123, + 'subject': {'subject.test': 'subject'}, + 'testcase': 'testcase1', 'product_version': 'fool-1', 'waived': True, 'comment': 'it broke', diff --git a/tests/test_events.py b/tests/test_events.py index 7193749..2b201bd 100644 --- a/tests/test_events.py +++ b/tests/test_events.py @@ -9,7 +9,8 @@ from waiverdb.models import Waiver @mock.patch('waiverdb.events.fedmsg') def test_publish_new_waiver_with_fedmsg(mock_fedmsg, session): waiver = Waiver( - result_id=1, + subject={'subject.test': 'subject'}, + testcase='testcase1', username='jcline', product_version='something', waived=True, @@ -22,7 +23,8 @@ def test_publish_new_waiver_with_fedmsg(mock_fedmsg, session): topic='waiver.new', msg={ 'id': waiver.id, - 'result_id': 1, + 'subject': {'subject.test': 'subject'}, + 'testcase': 'testcase1', 'username': 'jcline', 'proxied_by': None, 'product_version': 'something', @@ -36,7 +38,8 @@ def test_publish_new_waiver_with_fedmsg(mock_fedmsg, session): @mock.patch('waiverdb.events.fedmsg') def test_publish_new_waiver_with_fedmsg_for_proxy_user(mock_fedmsg, session): waiver = Waiver( - result_id=1, + subject={'subject.test': 'subject'}, + testcase='testcase1', username='jcline', product_version='something', waived=True, @@ -50,7 +53,8 @@ def test_publish_new_waiver_with_fedmsg_for_proxy_user(mock_fedmsg, session): topic='waiver.new', msg={ 'id': waiver.id, - 'result_id': 1, + 'subject': {'subject.test': 'subject'}, + 'testcase': 'testcase1', 'username': 'jcline', 'proxied_by': 'bodhi', 'product_version': 'something', diff --git a/tests/utils.py b/tests/utils.py index 77d76e3..ef8d9a2 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -3,9 +3,10 @@ from waiverdb.models import Waiver -def create_waiver(session, result_id, username, product_version, waived=True, +def create_waiver(session, subject, testcase, username, product_version, waived=True, comment=None, proxied_by=None): - waiver = Waiver(result_id, username, product_version, waived, comment, proxied_by) + waiver = Waiver(subject, testcase, username, product_version, waived, comment, + proxied_by) session.add(waiver) session.flush() return waiver diff --git a/waiverdb/api_v1.py b/waiverdb/api_v1.py index 674da1d..d502881 100644 --- a/waiverdb/api_v1.py +++ b/waiverdb/api_v1.py @@ -1,8 +1,10 @@ # SPDX-License-Identifier: GPL-2.0+ +import json + from flask import Blueprint, request, current_app from flask_restful import Resource, Api, reqparse, marshal_with, marshal -from werkzeug.exceptions import BadRequest, NotFound, UnsupportedMediaType, Forbidden +from werkzeug.exceptions import BadRequest, UnsupportedMediaType, Forbidden from sqlalchemy.sql.expression import func from waiverdb import __version__ @@ -14,19 +16,27 @@ import waiverdb.auth api_v1 = (Blueprint('api_v1', __name__)) api = Api(api_v1) + +def valid_dict(value): + if not isinstance(value, dict): + raise ValueError("Must be a valid dict, not %r" % value) + return value + + # RP contains request parsers (reqparse.RequestParser). # Parsers are added in each 'resource section' for better readability RP = {} RP['create_waiver'] = reqparse.RequestParser() -RP['create_waiver'].add_argument('result_id', type=int, required=True, location='json') +RP['create_waiver'].add_argument('subject', type=valid_dict, required=True, location='json') +RP['create_waiver'].add_argument('testcase', type=str, required=True, location='json') RP['create_waiver'].add_argument('waived', type=bool, required=True, location='json') RP['create_waiver'].add_argument('product_version', type=str, required=True, location='json') RP['create_waiver'].add_argument('comment', type=str, default=None, location='json') RP['create_waiver'].add_argument('username', type=str, default=None, location='json') RP['get_waivers'] = reqparse.RequestParser() -RP['get_waivers'].add_argument('result_id', location='args') +RP['get_waivers'].add_argument('results', 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') @@ -74,8 +84,8 @@ class WaiversResource(Resource): :query int page: The page to get. :query int limit: Limit the number of items returned. - :query int result_id: Filter the waivers by result ID. Accepts one or - more result IDs separated by commas. + :query string results: Filter the waivers by result. Accepts a list of + dictionaries, with one key 'subject' and one key 'testcase'. :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 @@ -91,8 +101,19 @@ class WaiversResource(Resource): """ args = RP['get_waivers'].parse_args() query = Waiver.query.order_by(Waiver.timestamp.desc()) - if args['result_id']: - query = query.filter(Waiver.result_id.in_(args['result_id'].split(','))) + + 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), basestring): + raise BadRequest("'results' parameter should be a list \ + of dictionaries with subject and testcase") + query = Waiver.by_results(query, results) if args['product_version']: query = query.filter(Waiver.product_version == args['product_version']) if args['username']: @@ -102,15 +123,19 @@ class WaiversResource(Resource): if args['since']: try: since_start, since_end = reqparse_since(args['since']) - except: + except ValueError: + raise BadRequest("'since' parameter not in ISO8601 format") + except TypeError: raise BadRequest("'since' parameter not in ISO8601 format") if since_start: query = query.filter(Waiver.timestamp >= since_start) if since_end: query = query.filter(Waiver.timestamp <= since_end) if not args['include_obsolete']: - subquery = db.session.query(func.max(Waiver.id)).group_by(Waiver.result_id) + subquery = db.session.query(func.max(Waiver.id)).group_by(Waiver.subject, + Waiver.testcase) query = query.filter(Waiver.id.in_(subquery)) + query = query.order_by(Waiver.timestamp.desc()) return json_collection(query, args['page'], args['limit']) @jsonp @@ -133,7 +158,8 @@ class WaiversResource(Resource): Content-Length: 91 { - "result_id": 1, + "subject": {"productmd.compose.id": "Fedora-9000-19700101.n.18"}, + "testcase": "compose.install_no_user", "waived": false, "product_version": "Parrot", "comment": "It's dead!" @@ -155,20 +181,23 @@ class WaiversResource(Resource): "comment": "It's dead!", "id": 15, "product_version": "Parrot", - "result_id": 1, + "subject": {"productmd.compose.id": "Fedora-9000-19700101.n.18"}, + "testcase": "compose.install_no_user", "timestamp": "2017-03-16T17:42:04.209638", "username": "jcline", "waived": false, "proxied_by": null } - :json int result_id: The result ID for the waiver. + :json object subject: The result subject for the waiver. + :json string testcase: The result testcase for the waiver. :json boolean waived: Whether or not the result is waived. :json string product_version: The product version string. :json string comment: A comment explaining the waiver. :json string username: Username on whose behalf the caller is proxying. :statuscode 201: The waiver was successfully created. """ + user, headers = waiverdb.auth.get_user(request) args = RP['create_waiver'].parse_args() proxied_by = None @@ -177,8 +206,8 @@ class WaiversResource(Resource): raise Forbidden('user %s does not have the proxyuser ability' % user) proxied_by = user user = args['username'] - waiver = Waiver(args['result_id'], user, args['product_version'], args['waived'], - args['comment'], proxied_by) + waiver = Waiver(args['subject'], args['testcase'], user, + args['product_version'], args['waived'], args['comment'], proxied_by) db.session.add(waiver) db.session.commit() return waiver, 201, headers @@ -198,24 +227,25 @@ class WaiverResource(Resource): """ try: return Waiver.query.get_or_404(waiver_id) - except: + except Exception as NotFound: raise NotFound('Waiver not found') -class GetWaiversByResultIDs(Resource): +class GetWaiversBySubjectsAndTestcases(Resource): @jsonp def post(self): """ - Return a list of waivers by filtering the waivers with a list of result ids. - This accepts POST requests in order to handle a special case where a - GET /waivers/ request has a long query string with many result ids that - could cause 413 erros. + Return a list of waivers by filtering the waivers with a + list of result subjects and testcases. + This accepts POST requests in order to handle a special + case where a GET /waivers/ request has a long query string with many + result subjects/testcases that could cause 413 errors. **Sample request**: .. sourcecode:: http - POST /api/v1.0/waivers/+by-result-ids HTTP/1.1 + POST /api/v1.0/waivers/+by-subjects-and-testcases HTTP/1.1 Host: localhost:5004 Accept-Encoding: gzip, deflate Accept: application/json @@ -225,7 +255,16 @@ class GetWaiversByResultIDs(Resource): Content-Length: 40 { - "result_ids": [1,2] + "results": [ + { + "subject": {"productmd.compose.id": "Fedora-9000-19700101.n.18"}, + "testcase": "compose.install_no_user" + }, + { + "subject": {"item": "gzip-1.9-1.fc28", "type": "koji_build"}, + "testcase": "dist.rpmlint" + } + ] } **Sample response**: @@ -244,7 +283,8 @@ class GetWaiversByResultIDs(Resource): "comment": "It's dead!", "id": 5, "product_version": "Parrot", - "result_id": 2, + "subject": {"productmd.compose.id": "Fedora-9000-19700101.n.18"}, + "testcase": "compose.install_no_user", "timestamp": "2017-09-21T04:55:53.343368", "username": "dummy", "waived": true, @@ -254,7 +294,8 @@ class GetWaiversByResultIDs(Resource): "comment": "It's dead!", "id": 4, "product_version": "Parrot", - "result_id": 1, + "subject": {"item": "gzip-1.9-1.fc28", "type": "koji_build"}, + "testcase": "dist.rpmlint", "timestamp": "2017-09-21T04:55:51.936658", "username": "dummy", "waived": true, @@ -263,7 +304,8 @@ class GetWaiversByResultIDs(Resource): ] } - :jsonparam array result_ids: Filter the waivers by a list of result IDs. + :jsonparam array results: Filter the waivers by a list of dictionaries + with result subjects and testcase. :jsonparam string product_version: Filter the waivers by product version. :jsonparam string username: Filter the waivers by username. :jsonparam string proxied_by: Filter the waivers by the users who are @@ -280,8 +322,17 @@ class GetWaiversByResultIDs(Resource): raise UnsupportedMediaType('No JSON payload in request') data = request.get_json() query = Waiver.query.order_by(Waiver.timestamp.desc()) - if 'result_ids' in data and data['result_ids']: - query = query.filter(Waiver.result_id.in_(data['result_ids'])) + 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), basestring): + raise BadRequest("'results' parameter should be a list \ + of dictionaries with subject and testcase") + query = Waiver.by_results(query, data['results']) if 'product_version' in data: query = query.filter(Waiver.product_version == data['product_version']) if 'username' in data: @@ -291,15 +342,20 @@ class GetWaiversByResultIDs(Resource): if 'since' in data: try: since_start, since_end = reqparse_since(data['since']) - except: + except ValueError: + raise BadRequest("'since' parameter not in ISO8601 format") + except TypeError: raise BadRequest("'since' parameter not in ISO8601 format") if since_start: query = query.filter(Waiver.timestamp >= since_start) if since_end: query = query.filter(Waiver.timestamp <= since_end) if not data.get('include_obsolete', False): - subquery = db.session.query(func.max(Waiver.id)).group_by(Waiver.result_id) + subquery = db.session.query(func.max(Waiver.id)).group_by(Waiver.subject, + Waiver.testcase) query = query.filter(Waiver.id.in_(subquery)) + + query = query.order_by(Waiver.timestamp.desc()) return {'data': marshal(query.all(), waiver_fields)} @@ -333,5 +389,5 @@ class AboutResource(Resource): # set up the Api resource routing here api.add_resource(WaiversResource, '/waivers/') api.add_resource(WaiverResource, '/waivers/') -api.add_resource(GetWaiversByResultIDs, '/waivers/+by-result-ids') +api.add_resource(GetWaiversBySubjectsAndTestcases, '/waivers/+by-subjects-and-testcases') api.add_resource(AboutResource, '/about') diff --git a/waiverdb/events.py b/waiverdb/events.py index 372ab48..5596ebb 100644 --- a/waiverdb/events.py +++ b/waiverdb/events.py @@ -48,7 +48,8 @@ def publish_new_waiver(session): "waived": true, "timestamp": "2017-03-16T17:42:04.209638", "product_version": "Satellite 6.3", - "result_id": 1, + "subject": "{\"a.nice.example\": \"this-is-a-really-nice-example\"}", + "testcase": "t.e.s.t.case", "proxied_by": null, "id": 15 } diff --git a/waiverdb/fields.py b/waiverdb/fields.py index b724b12..baf7198 100644 --- a/waiverdb/fields.py +++ b/waiverdb/fields.py @@ -2,9 +2,11 @@ from flask_restful import fields + waiver_fields = { 'id': fields.Integer, - 'result_id': fields.Integer, + 'subject': fields.Raw, + 'testcase': fields.String, 'username': fields.String, 'proxied_by': fields.String, 'product_version': fields.String, diff --git a/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py b/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py new file mode 100644 index 0000000..d4a23f8 --- /dev/null +++ b/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py @@ -0,0 +1,37 @@ +"""waive absence of result + +Revision ID: f2772c2c64a6 +Revises: 0a74cdab732a +Create Date: 2017-12-04 10:03:54.792758 + +""" + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'f2772c2c64a6' +down_revision = '0a74cdab732a' + + +def upgrade(): + op.add_column('waiver', sa.Column('result_subject', sa.Text(), nullable=True, index=True)) + op.add_column('waiver', sa.Column('result_testcase', sa.Text(), nullable=True, index=True)) + + # SQLite has some problem in dropping/altering columns. + # So in this way Alembic should do some behind the scenes + # with: make new table - copy data - drop old table - rename new table + with op.batch_alter_table('waiver') as batch_op: + batch_op.alter_column('result_subject', nullable=False) + batch_op.alter_column('result_testcase', nullable=False) + batch_op.drop_column('result_id') + + +def downgrade(): + op.add_column('waiver', sa.Column('result_id', sa.INTEGER(), nullable=True)) + + with op.batch_alter_table('waiver') as batch_op: + batch_op.drop_column('result_testcase') + batch_op.drop_column('result_subject') + batch_op.alter_column('result_id', nullable=False) diff --git a/waiverdb/models/waivers.py b/waiverdb/models/waivers.py index 317badf..c45c2a5 100644 --- a/waiverdb/models/waivers.py +++ b/waiverdb/models/waivers.py @@ -2,11 +2,14 @@ import datetime from .base import db +from sqlalchemy import or_, and_ +from sqlalchemy_utils import JSONType class Waiver(db.Model): id = db.Column(db.Integer, primary_key=True) - result_id = db.Column(db.Integer, nullable=False) + subject = db.Column(JSONType, nullable=False, index=True) + testcase = db.Column(db.Text, nullable=False, index=True) username = db.Column(db.String(255), nullable=False) proxied_by = db.Column(db.String(255)) product_version = db.Column(db.String(200), nullable=False) @@ -14,9 +17,10 @@ class Waiver(db.Model): comment = db.Column(db.Text) timestamp = db.Column(db.DateTime, default=datetime.datetime.utcnow) - def __init__(self, result_id, username, product_version, waived=False, comment=None, - proxied_by=None): - self.result_id = result_id + def __init__(self, subject, testcase, username, product_version, waived=False, + comment=None, proxied_by=None): + self.subject = subject + self.testcase = testcase self.username = username self.product_version = product_version self.waived = waived @@ -24,6 +28,19 @@ class Waiver(db.Model): self.proxied_by = proxied_by def __repr__(self): - return '%s(result_id=%r, username=%r, product_version=%r, waived=%r)' % \ - (self.__class__.__name__, self.result_id, self.username, self.product_version, - self.waived) + return '%s(subject=%r, testcase=%r, username=%r, product_version=%r, waived=%r)' % \ + (self.__class__.__name__, self.subject, self.testcase, self.username, + self.product_version, self.waived) + + @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 + ])) From 68e2b2e778e962d4caceabd36432cd75ff46dea9 Mon Sep 17 00:00:00 2001 From: Giulia Naponiello Date: Jan 23 2018 14:00:55 +0000 Subject: [PATCH 2/2] Added dummy auth for CLI The dummy authentication for the CLI is needed for developing and testing purposes. --- diff --git a/tests/test_api_v10.py b/tests/test_api_v10.py index 8da3d90..3ca35b0 100644 --- a/tests/test_api_v10.py +++ b/tests/test_api_v10.py @@ -195,6 +195,19 @@ def test_filtering_waivers_by_subject_and_testcase(client, session): 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) + 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_multiple_results_subjects_and_testcases(client, session): create_waiver(session, subject={'subject.test1': 'subject1'}, testcase='testcase1', username='foo-1', product_version='foo-1') diff --git a/tests/test_cli.py b/tests/test_cli.py index 0f28716..12c4b9a 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -95,7 +95,7 @@ oidc_scopes= assert result.output == 'Error: Please specify product version\n' -def test_no_result_id(tmpdir): +def test_no_subject(tmpdir): p = tmpdir.join('client.conf') p.write(""" [waiverdb] @@ -110,7 +110,25 @@ oidc_scopes= args = ['-C', p.strpath, '-p', 'fedora-26'] result = runner.invoke(waiverdb_cli, args) assert result.exit_code == 1 - assert result.output == 'Error: Please specify one or more result ids to waive\n' + assert result.output == 'Error: Please specify one subject\n' + + +def test_no_testcase(tmpdir): + p = tmpdir.join('client.conf') + p.write(""" +[waiverdb] +auth_method=OIDC +api_url=http://localhost:5004/api/v1.0 +oidc_id_provider=https://id.stg.fedoraproject.org/openidc/ +oidc_client_id=waiverdb +oidc_scopes= + openid + """) + runner = CliRunner() + args = ['-C', p.strpath, '-p', 'fedora-26', '-s', 'subject'] + result = runner.invoke(waiverdb_cli, args) + assert result.exit_code == 1 + assert result.output == 'Error: Please specify testcase\n' def test_oidc_auth_is_enabled(tmpdir): @@ -123,7 +141,8 @@ def test_oidc_auth_is_enabled(tmpdir): "comment": "It's dead!", "id": 15, "product_version": "Parrot", - "result_id": 123, + "subject": {"subject.test": "test", "s": "t"}, + "testcase": "test.testcase", "timestamp": "2017-010-16T17:42:04.209638", "username": "foo", "waived": True @@ -140,10 +159,12 @@ oidc_scopes= openid """) runner = CliRunner() - args = ['-C', p.strpath, '-p', 'Parrot', '-r', 123, '-c', "It's dead!"] + args = ['-C', p.strpath, '-p', 'Parrot', '-s', '{"subject.test": "test", "s": "t"}', + '-t', 'test.testcase', '-c', "It's dead!"] result = runner.invoke(waiverdb_cli, args) exp_json = { - 'result_id': 123, + "subject": {"subject.test": "test", "s": "t"}, + "testcase": "test.testcase", 'waived': True, 'product_version': 'Parrot', 'comment': "It's dead!" @@ -155,7 +176,7 @@ oidc_scopes= timeout=60, headers={'Content-Type': 'application/json'}) assert result.exit_code == 0 - assert result.output == 'Created waiver 15 for result 123\n' + assert result.output == 'Created waiver 15 for result with subject {"subject.test": "test", "s": "t"} and testcase test.testcase\n' # noqa def test_kerberos_is_enabled(tmpdir): @@ -168,7 +189,8 @@ def test_kerberos_is_enabled(tmpdir): "comment": "It's dead!", "id": 15, "product_version": "Parrot", - "result_id": 123, + "subject": {"subject.test": "test", "s": "t"}, + "testcase": "test.testcase", "timestamp": "2017-010-16T17:42:04.209638", "username": "foo", "waived": True @@ -181,7 +203,8 @@ auth_method=Kerberos api_url=http://localhost:5004/api/v1.0 """) runner = CliRunner() - args = ['-C', p.strpath, '-p', 'Parrot', '-r', 123, '-c', "It's dead!"] + args = ['-C', p.strpath, '-p', 'Parrot', '-s', '{"subject.test": "test", "s": "t"}', + '-t', 'test.testcase', '-c', "It's dead!"] result = runner.invoke(waiverdb_cli, args) mock_request.assert_called_once() - assert result.output == 'Created waiver 15 for result 123\n' + assert result.output == 'Created waiver 15 for result with subject {"subject.test": "test", "s": "t"} and testcase test.testcase\n' # noqa diff --git a/waiverdb/cli.py b/waiverdb/cli.py index 53518b2..ff417c6 100644 --- a/waiverdb/cli.py +++ b/waiverdb/cli.py @@ -20,7 +20,7 @@ def validate_config(config): if not config.has_option('waiverdb', 'auth_method'): raise click.ClickException(config_error.format('auth_method')) auth_method = config.get('waiverdb', 'auth_method') - if auth_method not in ['OIDC', 'Kerberos']: + if auth_method not in ['OIDC', 'Kerberos', 'dummy']: raise click.ClickException('The WaiverDB authentication mechanism of ' '"{0}" is not supported'.format(auth_method)) required_configs = ['api_url'] @@ -37,36 +37,48 @@ def validate_config(config): @click.option('--config-file', '-C', default='/etc/waiverdb/client.conf', type=click.Path(exists=True), help='Specify a config file to use') -@click.option('--result-id', '-r', multiple=True, type=int, - help='Specify one or more results to be waived') +@click.option('--subject', '-s', + help='Specify one subject for a result to waive') +@click.option('--testcase', '-t', + help='Specify a testcase for the subject') @click.option('--product-version', '-p', help='Specify one of PDC\'s product version identifiers.') @click.option('--waived/--no-waived', default=True, help='Whether or not the result is waived') @click.option('--comment', '-c', help='A comment explaining why the result is waived') -def cli(comment, waived, product_version, result_id, config_file): +def cli(comment, waived, product_version, testcase, subject, config_file): """ Creates new waivers against test results. Examples: - waiverdb-cli -r 123 -r 456 -p "fedora-26" -c "It's dead!" + waiverdb-cli -t dist.rpmlint -s '{"item": "python-requests-1.2.3-1.fc26", + "type": "koji_build"}' + -p "fedora-26" -c "It's dead!" """ config = configparser.SafeConfigParser() + config.read(config_file) validate_config(config) if not product_version: raise click.ClickException('Please specify product version') - # This name makes more sense for the values of result_id, where as --result-id - # makes more sense from the cli perspective. - result_ids = result_id - if not result_ids: - raise click.ClickException('Please specify one or more result ids to waive') + if not subject: + raise click.ClickException('Please specify one subject') + if not testcase: + raise click.ClickException('Please specify testcase') auth_method = config.get('waiverdb', 'auth_method') + data = { + 'subject': json.loads(subject), + 'testcase': testcase, + 'waived': waived, + 'product_version': product_version, + 'comment': comment + } + api_url = config.get('waiverdb', 'api_url') if auth_method == 'OIDC': # Try to import this now so the user gets immediate feedback if # it isn't installed @@ -84,31 +96,13 @@ def cli(comment, waived, product_version, result_id, config_file): {'Token': 'Token', 'Authorization': 'Authorization'}, config.get('waiverdb', 'oidc_client_id'), oidc_client_secret) - for result_id in result_ids: - data = { - 'result_id': result_id, - 'waived': waived, - 'product_version': product_version, - 'comment': comment - } - api_url = config.get('waiverdb', 'api_url') - scopes = config.get('waiverdb', 'oidc_scopes').strip().splitlines() - resp = oidc.send_request( - scopes=scopes, - url='{0}/waivers/'.format(api_url.rstrip('/')), - data=json.dumps(data), - headers={'Content-Type': 'application/json'}, - timeout=60) - if not resp.ok: - try: - error_msg = resp.json()['message'] - except (ValueError, KeyError): - error_msg = resp.text - raise click.ClickException( - 'Failed to create waiver for result {0}:\n{1}' - .format(result_id, error_msg)) - click.echo('Created waiver {0} for result {1}'.format( - resp.json()['id'], result_id)) + scopes = config.get('waiverdb', 'oidc_scopes').strip().splitlines() + resp = oidc.send_request( + scopes=scopes, + url='{0}/waivers/'.format(api_url.rstrip('/')), + data=json.dumps(data), + headers={'Content-Type': 'application/json'}, + timeout=60) elif auth_method == 'Kerberos': # Try to import this now so the user gets immediate feedback if # it isn't installed @@ -117,31 +111,28 @@ def cli(comment, waived, product_version, result_id, config_file): except ImportError: raise click.ClickException('python-requests-kerberos needs to be installed') auth = requests_kerberos.HTTPKerberosAuth(mutual_authentication=requests_kerberos.OPTIONAL) - for result_id in result_ids: - data = { - 'result_id': result_id, - 'waived': waived, - 'product_version': product_version, - 'comment': comment - } - api_url = config.get('waiverdb', 'api_url') - resp = requests.request('POST', '{0}/waivers/'.format(api_url.rstrip('/')), - data=json.dumps(data), auth=auth, - headers={'Content-Type': 'application/json'}, - timeout=60) - if resp.status_code == 401: - raise click.ClickException('WaiverDB authentication using Kerberos failed. ' - 'Make sure you have a valid Kerberos ticket.') - if not resp.ok: - try: - error_msg = resp.json()['message'] - except (ValueError, KeyError): - error_msg = resp.text - raise click.ClickException( - 'Failed to create waiver for result {0}:\n{1}' - .format(result_id, error_msg)) - click.echo('Created waiver {0} for result {1}'.format( - resp.json()['id'], result_id)) + resp = requests.request('POST', '{0}/waivers/'.format(api_url.rstrip('/')), + data=json.dumps(data), auth=auth, + headers={'Content-Type': 'application/json'}, + timeout=60) + if resp.status_code == 401: + raise click.ClickException('WaiverDB authentication using Kerberos failed. ' + 'Make sure you have a valid Kerberos ticket.') + elif auth_method == 'dummy': + resp = requests.request('POST', '{0}/waivers/'.format(api_url.rstrip('/')), + data=json.dumps(data), auth=('user', 'pass'), + headers={'Content-Type': 'application/json'}, + timeout=60) + if not resp.ok: + try: + error_msg = resp.json()['message'] + except (ValueError, KeyError): + error_msg = resp.text + raise click.ClickException( + 'Failed to create waiver for result with subject {0} and testcase {1}:\n{2}' + .format(subject, testcase, error_msg)) + click.echo('Created waiver {0} for result with subject {1} and testcase {2}'.format( + resp.json()['id'], subject, testcase)) if __name__ == '__main__': diff --git a/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py b/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py index d4a23f8..7d3537f 100644 --- a/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py +++ b/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py @@ -16,15 +16,15 @@ down_revision = '0a74cdab732a' def upgrade(): - op.add_column('waiver', sa.Column('result_subject', sa.Text(), nullable=True, index=True)) - op.add_column('waiver', sa.Column('result_testcase', sa.Text(), nullable=True, index=True)) + op.add_column('waiver', sa.Column('subject', sa.Text(), nullable=True, index=True)) + op.add_column('waiver', sa.Column('testcase', sa.Text(), nullable=True, index=True)) # SQLite has some problem in dropping/altering columns. # So in this way Alembic should do some behind the scenes # with: make new table - copy data - drop old table - rename new table with op.batch_alter_table('waiver') as batch_op: - batch_op.alter_column('result_subject', nullable=False) - batch_op.alter_column('result_testcase', nullable=False) + batch_op.alter_column('subject', nullable=False) + batch_op.alter_column('testcase', nullable=False) batch_op.drop_column('result_id') @@ -32,6 +32,6 @@ def downgrade(): op.add_column('waiver', sa.Column('result_id', sa.INTEGER(), nullable=True)) with op.batch_alter_table('waiver') as batch_op: - batch_op.drop_column('result_testcase') - batch_op.drop_column('result_subject') + batch_op.drop_column('testcase') + batch_op.drop_column('subject') batch_op.alter_column('result_id', nullable=False)