From b41dce6c349aafe8c2389564c1f7b151918e578c Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Mar 05 2019 12:12:35 +0000 Subject: [PATCH 1/3] createEmptyBuild errors for non-existent user Until now, error was raised from postgres insert. Now is user's existence checked before running insert. Fixes: https://pagure.io/koji/issue/1163 --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 51255f2..22fde61 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -5105,6 +5105,9 @@ def new_build(data): if not name: raise koji.GenericError("No name or package id provided for build") data['pkg_id'] = new_package(name, strict=False) + if data.get('owner'): + # check, that user exists + get_user(data['owner'], strict=True) for f in ('version', 'release', 'epoch'): if f not in data: raise koji.GenericError("No %s value for build" % f) diff --git a/tests/test_hub/test_new_build.py b/tests/test_hub/test_new_build.py new file mode 100644 index 0000000..f7033e7 --- /dev/null +++ b/tests/test_hub/test_new_build.py @@ -0,0 +1,161 @@ +from __future__ import absolute_import +import mock +try: + import unittest2 as unittest +except ImportError: + import unittest + +import koji +import kojihub + +IP = kojihub.InsertProcessor + +class TestNewBuild(unittest.TestCase): + def setUp(self): + self.get_rpm = mock.patch('kojihub.get_rpm').start() + self.get_external_repo_id = mock.patch('kojihub.get_external_repo_id').start() + self.nextval = mock.patch('kojihub.nextval').start() + self.Savepoint = mock.patch('kojihub.Savepoint').start() + self.InsertProcessor = mock.patch('kojihub.InsertProcessor', + side_effect=self.getInsert).start() + self.inserts = [] + self.insert_execute = mock.MagicMock() + self.lookup_package = mock.patch('kojihub.lookup_package').start() + self.new_package = mock.patch('kojihub.new_package').start() + self.get_user = mock.patch('kojihub.get_user').start() + self.get_build = mock.patch('kojihub.get_build').start() + self.recycle_build = mock.patch('kojihub.recycle_build').start() + self.context = mock.patch('kojihub.context').start() + self._singleValue = mock.patch('kojihub._singleValue').start() + + def tearDown(self): + mock.patch.stopall() + + def getInsert(self, *args, **kwargs): + insert = IP(*args, **kwargs) + insert.execute = self.insert_execute + self.inserts.append(insert) + return insert + + def test_valid(self): + self.context.session.user_id = 123456 + self.get_build.return_value = None + self._singleValue.return_value = 65 # free build id + self.new_package.return_value = 54 + data = { + 'name': 'test_name', + 'version': 'test_version', + 'release': 'test_release', + 'epoch': 'test_epoch', + 'owner': 'test_owner', + 'extra': {'extra_key': 'extra_value'}, + } + + kojihub.new_build(data) + + self.assertEqual(len(self.inserts), 1) + insert = self.inserts[0] + self.assertEqual(insert.table, 'build') + self.assertEqual(insert.data, { + 'completion_time': 'NOW', + 'epoch': 'test_epoch', + 'extra': '{"extra_key": "extra_value"}', + 'id': 65, + 'owner': 'test_owner', + 'pkg_id': 54, + 'release': 'test_release', + 'source': None, + 'start_time': 'NOW', + 'state': 1, + 'task_id': None, + 'version': 'test_version', + 'volume_id': 0 + }) + + def test_empty_data(self): + with self.assertRaises(koji.GenericError): + kojihub.new_build({}) + self.assertEqual(len(self.inserts), 0) + + def test_wrong_pkg_id(self): + self.lookup_package.side_effect = koji.GenericError + data = { + 'pkg_id': 444, + 'name': 'test_name', + 'version': 'test_version', + 'release': 'test_release', + 'epoch': 'test_epoch', + 'owner': 'test_owner', + 'extra': {'extra_key': 'extra_value'}, + } + + with self.assertRaises(koji.GenericError): + kojihub.new_build(data) + + self.assertEqual(len(self.inserts), 0) + + def test_missing_pkg_id_name(self): + data = { + 'version': 'test_version', + 'release': 'test_release', + 'epoch': 'test_epoch', + 'owner': 'test_owner', + 'extra': {'extra_key': 'extra_value'}, + } + + with self.assertRaises(koji.GenericError): + kojihub.new_build(data) + + self.assertEqual(len(self.inserts), 0) + + def test_wrong_owner(self): + self.get_user.side_effect = koji.GenericError + data = { + 'owner': 123456, + 'name': 'test_name', + 'version': 'test_version', + 'release': 'test_release', + 'epoch': 'test_epoch', + 'extra': {'extra_key': 'extra_value'}, + } + + with self.assertRaises(koji.GenericError): + kojihub.new_build(data) + + self.assertEqual(len(self.inserts), 0) + + def test_missing_vre(self): + self.get_user.side_effect = koji.GenericError + data = { + 'name': 'test_name', + 'version': 'test_version', + 'release': 'test_release', + 'epoch': 'test_epoch', + } + + for item in ('version', 'release', 'epoch'): + d = data.copy() + del d[item] + with self.assertRaises(koji.GenericError): + kojihub.new_build(d) + + self.assertEqual(len(self.inserts), 0) + + def test_wrong_extra(self): + # extra dict contains unserializable data + class CantDoJSON(object): + pass + + data = { + 'owner': 123456, + 'name': 'test_name', + 'version': 'test_version', + 'release': 'test_release', + 'epoch': 'test_epoch', + 'extra': {'extra_key': CantDoJSON()}, + } + + with self.assertRaises(koji.GenericError): + kojihub.new_build(data) + + self.assertEqual(len(self.inserts), 0) From 7c7468e1b39bd7d6628c8259810c6c3b9aa71f33 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Mar 07 2019 13:19:57 +0000 Subject: [PATCH 2/3] convert username to user_id --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 22fde61..e944048 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -5106,8 +5106,8 @@ def new_build(data): raise koji.GenericError("No name or package id provided for build") data['pkg_id'] = new_package(name, strict=False) if data.get('owner'): - # check, that user exists - get_user(data['owner'], strict=True) + # check, that user exists (and convert name to id) + data['owner'] = get_user(data['owner'], strict=True)['id'] for f in ('version', 'release', 'epoch'): if f not in data: raise koji.GenericError("No %s value for build" % f) From b95019267bc68a1f09878a3c648437064721d45d Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Mar 07 2019 13:25:43 +0000 Subject: [PATCH 3/3] fix test for numeric user_id --- diff --git a/tests/test_hub/test_new_build.py b/tests/test_hub/test_new_build.py index f7033e7..fea39c3 100644 --- a/tests/test_hub/test_new_build.py +++ b/tests/test_hub/test_new_build.py @@ -38,10 +38,10 @@ class TestNewBuild(unittest.TestCase): return insert def test_valid(self): - self.context.session.user_id = 123456 self.get_build.return_value = None self._singleValue.return_value = 65 # free build id self.new_package.return_value = 54 + self.get_user.return_value = {'id': 123} data = { 'name': 'test_name', 'version': 'test_version', @@ -61,7 +61,7 @@ class TestNewBuild(unittest.TestCase): 'epoch': 'test_epoch', 'extra': '{"extra_key": "extra_value"}', 'id': 65, - 'owner': 'test_owner', + 'owner': 123, 'pkg_id': 54, 'release': 'test_release', 'source': None,