From 90045e55f70e1c2dfb867cd1827c654a23a15acd Mon Sep 17 00:00:00 2001 From: Giulia Naponiello Date: Feb 12 2018 17:05:22 +0000 Subject: [PATCH 1/2] Backwards compatibility in the server for result_id argument. This is tricky, because the server has to accept either a `result_id` *or* a `subject`/`testcase` pair. We tried to decorate some blocks with comments indicating that they can be removed in a future release. --- diff --git a/tests/test_api_v10.py b/tests/test_api_v10.py index 3ca35b0..8908db9 100644 --- a/tests/test_api_v10.py +++ b/tests/test_api_v10.py @@ -3,8 +3,8 @@ import json from .utils import create_waiver import datetime -from requests import ConnectionError -from mock import patch +from requests import ConnectionError, HTTPError +from mock import patch, Mock from waiverdb import __version__ @@ -29,16 +29,95 @@ def test_create_waiver(mocked_get_user, client, session): assert res_data['comment'] == 'it broke' +@patch('waiverdb.api_v1.get_resultsdb_result') +@patch('waiverdb.auth.get_user', return_value=('foo', {})) +def test_create_waiver_legacy(mocked_get_user, mocked_resultsdb, client, session): + mocked_resultsdb.return_value = { + 'data': { + 'type': ['koji_build'], + 'item': ['somebuild'], + }, + 'testcase': {'name': 'sometest'} + } + + data = { + 'result_id': 123, + 'product_version': 'fool-1', + 'waived': True, + 'comment': 'it broke', + } + 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 == 201 + assert res_data['username'] == 'foo' + assert res_data['subject'] == {'type': 'koji_build', 'item': 'somebuild'} + assert res_data['testcase'] == 'sometest' + assert res_data['product_version'] == 'fool-1' + assert res_data['waived'] is True + assert res_data['comment'] == 'it broke' + + +@patch('waiverdb.api_v1.get_resultsdb_result') +@patch('waiverdb.auth.get_user', return_value=('foo', {})) +def test_create_waiver_with_original_spec_nvr_subject(mocked_get_user, mocked_resultsdb, client, + session): + mocked_resultsdb.return_value = { + 'data': { + 'original_spec_nvr': ['somedata'], + }, + 'testcase': {'name': 'sometest'} + } + + data = { + 'result_id': 123, + 'product_version': 'fool-1', + 'waived': True, + 'comment': 'it broke', + } + 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 == 201 + assert res_data['username'] == 'foo' + assert res_data['subject'] == {'original_spec_nvr': 'somedata'} + assert res_data['testcase'] == 'sometest' + assert res_data['product_version'] == 'fool-1' + assert res_data['waived'] is True + assert res_data['comment'] == 'it broke' + + +@patch('waiverdb.api_v1.get_resultsdb_result', side_effect=HTTPError(response=Mock(status=404))) +@patch('waiverdb.auth.get_user', return_value=('foo', {})) +def test_create_waiver_with_unknown_result_id(mocked_get_user, mocked_resultsdb, client, session): + data = { + 'result_id': 123, + 'product_version': 'fool-1', + 'waived': True, + 'comment': 'it broke', + } + mocked_resultsdb.return_value.status_code = 404 + 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 res_data['message'].startswith('Failed looking up result in Resultsdb:') + + @patch('waiverdb.auth.get_user', return_value=('foo', {})) def test_create_waiver_with_no_testcase(mocked_get_user, client): data = { 'subject': {'foo': 'bar'}, + 'waived': True, + 'product_version': 'the-best', } 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 'Missing required parameter in the JSON body' in res_data['message']['testcase'] + # XXX - when we ditch result_id and make subject and testcase required + # again, this assertion will change back to the original. +# assert 'Missing required parameter in the JSON body' in res_data['message']['testcase'] + assert 'Either result_id or subject/testcase are required arguments.' in res_data['message'] @patch('waiverdb.auth.get_user', return_value=('foo', {})) diff --git a/tests/test_cli.py b/tests/test_cli.py index 95fab1f..cacda99 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -237,7 +237,6 @@ def test_submit_waiver_with_id(tmpdir): "data": {"item": ["htop-1.0-1.fc22"], "type": ["bodhi_update"]}, "id": 15, "product_version": "Parrot", - "subject": {"subject.test": "test", "s": "t"}, "testcase": {"name": "test.testcase"}, "timestamp": "2017-010-16T17:42:04.209638", "username": "foo", @@ -266,8 +265,6 @@ def test_submit_waiver_with_multiple_ids(tmpdir): "comment": "It's dead!", "data": {"item": ["htop-1.0-1.fc22"], "type": ["bodhi_update"]}, "id": 15, - "product_version": "Parrot", - "subject": {"subject.test": "test", "s": "t"}, "testcase": {"name": "test.testcase"}, "timestamp": "2017-010-16T17:42:04.209638", "username": "foo", @@ -314,7 +311,6 @@ def test_submit_waiver_for_original_spec_nvr_result(tmpdir): "original_spec_nvr": "test", "id": 15, "product_version": "Parrot", - "subject": {"subject.test": "test", "s": "t"}, "testcase": {"name": "test.testcase"}, "timestamp": "2017-010-16T17:42:04.209638", "username": "foo", diff --git a/waiverdb/api_v1.py b/waiverdb/api_v1.py index 04a47f3..a3410fe 100644 --- a/waiverdb/api_v1.py +++ b/waiverdb/api_v1.py @@ -2,9 +2,10 @@ import json +import requests from flask import Blueprint, request, current_app from flask_restful import Resource, Api, reqparse, marshal_with, marshal -from werkzeug.exceptions import BadRequest, UnsupportedMediaType, Forbidden +from werkzeug.exceptions import BadRequest, UnsupportedMediaType, Forbidden, ServiceUnavailable from sqlalchemy.sql.expression import func from waiverdb import __version__ @@ -15,6 +16,7 @@ import waiverdb.auth api_v1 = (Blueprint('api_v1', __name__)) api = Api(api_v1) +requests_session = requests.Session() def valid_dict(value): @@ -23,13 +25,22 @@ def valid_dict(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), + headers={'Content-Type': 'application/json'}, + timeout=60) + response.raise_for_status() + return response.json() + + # 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('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('subject', type=valid_dict, location='json') +RP['create_waiver'].add_argument('testcase', type=str, location='json') +RP['create_waiver'].add_argument('result_id', type=int, 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') @@ -206,6 +217,40 @@ class WaiversResource(Resource): raise Forbidden('user %s does not have the proxyuser ability' % user) proxied_by = user user = args['username'] + + # XXX - remove this in a future release (it was for temp backwards compat) + if args['result_id']: + if args['subject'] or args['testcase']: + raise BadRequest('Only result_id or subject and ' + 'testcase are allowed. Not both.') + try: + result = get_resultsdb_result(args['result_id']) + except requests.HTTPError as e: + if e.response.status_code == 404: + raise BadRequest('Result id not found in Resultsdb') + else: + raise ServiceUnavailable('Failed looking up result in Resultsdb: %s' % e) + except Exception as e: + raise ServiceUnavailable('Failed looking up result in Resultsdb: %s' % e) + if 'original_spec_nvr' in result['data']: + subject = {'original_spec_nvr': result['data']['original_spec_nvr'][0]} + else: + if result['data']['type'][0] == 'koji_build' or \ + result['data']['type'][0] == 'bodhi_update': + SUBJECT_KEYS = ['item', 'type'] + subject = dict([(k, v[0]) for k, v in result['data'].items() + if k in SUBJECT_KEYS]) + else: + raise BadRequest('It is not possible to submit a waiver by ' + 'id for this result. Please try again specifying ' + 'a subject and a testcase.') + args['subject'] = subject + args['testcase'] = result['testcase']['name'] + + if not args['subject'] or not args['testcase']: + raise BadRequest('Either result_id or subject/testcase ' + 'are required arguments.') + waiver = Waiver(args['subject'], args['testcase'], user, args['product_version'], args['waived'], args['comment'], proxied_by) db.session.add(waiver) diff --git a/waiverdb/cli.py b/waiverdb/cli.py index 85ecf8d..d0398c6 100644 --- a/waiverdb/cli.py +++ b/waiverdb/cli.py @@ -35,9 +35,9 @@ def validate_config(config): raise click.ClickException(config_error.format('resultsdb_api_url')) -def check_response(resp, data): - if 'result_id' in data: - msg = 'for result with id {0}'.format(data['result_id']) +def check_response(resp, data, result_id=None): + if result_id: + msg = 'for result with id {0}'.format(result_id) else: msg = 'for result with subject {0} and testcase {1}'.format(json.dumps(data['subject']), data['testcase']) @@ -101,34 +101,7 @@ def cli(comment, waived, product_version, testcase, subject, result_id, config_f auth_method = config.get('waiverdb', 'auth_method') data_list = [] - if result_ids: - for result_id in result_ids: - result = requests.request('GET', '{0}/results/{1}'.format(config.get('waiverdb', - 'resultsdb_api_url'), result_id), - headers={'Content-Type': 'application/json'}, - timeout=60) - if 'original_spec_nvr' in result.json(): - subject = {'original_spec_nvr': result.json()['original_spec_nvr']} - else: - if result.json()['data']['type'][0] == 'koji_build' or \ - result.json()['data']['type'][0] == 'bodhi_update': - SUBJECT_KEYS = ['item', 'type'] - subject = dict([(k, v[0]) for k, v in result.json()['data'].items() - if k in SUBJECT_KEYS]) - else: - raise click.ClickException('It is not possible to submit a waiver by \ - id for this result. Please try again specifying \ - a subject and a testcase.') - - data_list.append({ - 'result_id': result_id, - 'subject': subject, - 'testcase': result.json()['testcase']['name'], - 'waived': waived, - 'product_version': product_version, - 'comment': comment - }) - else: + if not result_ids: data_list.append({ 'subject': json.loads(subject), 'testcase': testcase, @@ -137,6 +110,15 @@ def cli(comment, waived, product_version, testcase, subject, result_id, config_f 'comment': comment }) + # XXX - TODO - remove this in a future release. (for backwards compat) + for result_id in result_ids: + data_list.append({ + 'result_id': result_id, + '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 @@ -164,7 +146,7 @@ def cli(comment, waived, product_version, testcase, subject, result_id, config_f data=json.dumps(data), headers={'Content-Type': 'application/json'}, timeout=60) - check_response(resp, data) + check_response(resp, data, data.get('result_id', None)) elif auth_method == 'Kerberos': # Try to import this now so the user gets immediate feedback if # it isn't installed @@ -181,14 +163,14 @@ def cli(comment, waived, product_version, testcase, subject, result_id, config_f if resp.status_code == 401: raise click.ClickException('WaiverDB authentication using Kerberos failed. ' 'Make sure you have a valid Kerberos ticket.') - check_response(resp, data) + check_response(resp, data, data.get('result_id', None)) elif auth_method == 'dummy': for data in data_list: resp = requests.request('POST', '{0}/waivers/'.format(api_url.rstrip('/')), data=json.dumps(data), auth=('user', 'pass'), headers={'Content-Type': 'application/json'}, timeout=60) - check_response(resp, data) + check_response(resp, data, data.get('result_id', None)) if __name__ == '__main__': From 83f18928516637c0efc5d05631f51b5f0538e742 Mon Sep 17 00:00:00 2001 From: Giulia Naponiello Date: Feb 12 2018 17:05:26 +0000 Subject: [PATCH 2/2] Changes on schema migration for backward compatibility In changing waiverdb to record waivers against a subject+testcase, rather than a result_id, we wrote a schema migration which just drops the old column. Now that result_id it is used we needed to make some changes for backward compatibility. --- diff --git a/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py b/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py index 7d3537f..8238b1d 100644 --- a/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py +++ b/waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py @@ -8,6 +8,10 @@ Create Date: 2017-12-04 10:03:54.792758 from alembic import op import sqlalchemy as sa +import requests + +from waiverdb.api_v1 import get_resultsdb_result +from waiverdb.models import db, Waiver # revision identifiers, used by Alembic. @@ -15,23 +19,54 @@ revision = 'f2772c2c64a6' down_revision = '0a74cdab732a' +def convert_id_to_subject_and_testcase(result_id): + try: + result = get_resultsdb_result(result_id) + except requests.HTTPError as e: + if e.response.status_code == 404: + raise RuntimeError('Result id %s not found in Resultsdb' % (result_id)) + else: + raise RuntimeError('Failed looking up result in Resultsdb: %s' % e) + except Exception as e: + raise RuntimeError('Failed looking up result in Resultsdb: %s' % e) + if 'original_spec_nvr' in result['data']: + subject = {'original_spec_nvr': result['data']['original_spec_nvr'][0]} + else: + if result['data']['type'][0] == 'koji_build' or \ + result['data']['type'][0] == 'bodhi_update': + SUBJECT_KEYS = ['item', 'type'] + subject = dict([(k, v[0]) for k, v in result['data'].items() + if k in SUBJECT_KEYS]) + else: + raise RuntimeError('Unable to determine subject for result id %s' % (result_id)) + testcase = result['testcase']['name'] + return (subject, testcase) + + def upgrade(): 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)) + # querying resultsdb for the corresponding subject/testcase for each result_id + waivers = Waiver.query.all() + for waiver in waivers: + subject, testcase = convert_id_to_subject_and_testcase(waiver.result_id) + waiver.subject = subject + waiver.testcase = testcase + db.session.commit() + # 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('subject', nullable=False) batch_op.alter_column('testcase', nullable=False) - batch_op.drop_column('result_id') + batch_op.alter_column('result_id', nullable=True) 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('testcase') - batch_op.drop_column('subject') - batch_op.alter_column('result_id', nullable=False) + # It shouldn't be possible to downgrade this change. + # Because the result_id field will not be populated with data anymore. + # If the user tries to downgrade "result_id" should be not null once again + # like in the old version of the schema, but the value is not longer available + raise RuntimeError('Irreversible migration') diff --git a/waiverdb/models/waivers.py b/waiverdb/models/waivers.py index c45c2a5..094994f 100644 --- a/waiverdb/models/waivers.py +++ b/waiverdb/models/waivers.py @@ -8,6 +8,7 @@ from sqlalchemy_utils import JSONType class Waiver(db.Model): id = db.Column(db.Integer, primary_key=True) + result_id = db.Column(db.Integer, nullable=True) 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) @@ -28,9 +29,9 @@ class Waiver(db.Model): self.proxied_by = proxied_by def __repr__(self): - 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) + return '%s(result_id=%r, subject=%r, testcase=%r, username=%r, product_version=%r,\ + waived=%r)' % (self.__class__.__name__, self.result_id, self.subject, self.testcase, + self.username, self.product_version, self.waived) @classmethod def by_results(cls, query, results):