From b975594ce4ec1df300eccc261bedbd607e2c2aa0 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Feb 01 2018 15:12:52 +0000
Subject: [PATCH 1/4] recheck for duplicate external rpm on insertion errors
fixes: https://pagure.io/koji/issue/788
---
diff --git a/hub/kojihub.py b/hub/kojihub.py
index 7e664af..4500acd 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -5730,7 +5730,7 @@ def add_external_rpm(rpminfo, external_repo, strict=True):
# [!] Calling function should perform access checks
- #sanity check rpminfo
+ # sanity check rpminfo
dtypes = (
('name', basestring),
('version', basestring),
@@ -5746,37 +5746,47 @@ def add_external_rpm(rpminfo, external_repo, strict=True):
if not isinstance(rpminfo[field], allowed):
#this will catch unwanted NULLs
raise koji.GenericError("Invalid value for %s: %r" % (field, rpminfo[field]))
- #TODO: more sanity checks for payloadhash
+ # strip extra fields
+ rpminfo = dslice(rpminfo, [x[0] for x in dtypes])
+ # TODO: more sanity checks for payloadhash
- #Check to see if we have it
- data = rpminfo.copy()
- data['location'] = external_repo
- previous = get_rpm(data, strict=False)
+ def check_dup():
+ # Check to see if we have it
+ data = rpminfo.copy()
+ data['location'] = external_repo
+ previous = get_rpm(data, strict=False)
+ if previous:
+ disp = "%(name)s-%(version)s-%(release)s.%(arch)s@%(external_repo_name)s" % previous
+ if strict:
+ raise koji.GenericError("external rpm already exists: %s" % disp)
+ elif data['payloadhash'] != previous['payloadhash']:
+ raise koji.GenericError("hash changed for external rpm: %s (%s -> %s)" \
+ % (disp, previous['payloadhash'], data['payloadhash']))
+ else:
+ return previous
+
+ previous = check_dup()
if previous:
- disp = "%(name)s-%(version)s-%(release)s.%(arch)s@%(external_repo_name)s" % previous
- if strict:
- raise koji.GenericError("external rpm already exists: %s" % disp)
- elif data['payloadhash'] != previous['payloadhash']:
- raise koji.GenericError("hash changed for external rpm: %s (%s -> %s)" \
- % (disp, previous['payloadhash'], data['payloadhash']))
- else:
- return previous
+ return previous
- #add rpminfo entry
- rpminfo['external_repo_id'] = get_external_repo_id(external_repo, strict=True)
- rpminfo['id'] = _singleValue("""SELECT nextval('rpminfo_id_seq')""")
- q = """INSERT INTO rpminfo (id, build_id, buildroot_id,
- name, version, release, epoch, arch,
- external_repo_id,
- payloadhash, size, buildtime)
- VALUES (%(id)i, NULL, NULL,
- %(name)s, %(version)s, %(release)s, %(epoch)s, %(arch)s,
- %(external_repo_id)i,
- %(payloadhash)s, %(size)i, %(buildtime)i)
- """
- _dml(q, rpminfo)
+ # add rpminfo entry
+ data = rpminfo.copy()
+ data['external_repo_id'] = get_external_repo_id(external_repo, strict=True)
+ data['id'] = nextval('rpminfo_id_seq')
+ data['build_id'] = None
+ data['buildroot_id'] = None
+ insert = InsertProcessor('rpminfo', data=data)
+ try:
+ insert.execute()
+ except Exception:
+ # check for dup again to work around a race
+ # see: https://pagure.io/koji/issue/788
+ previous = check_dup()
+ if previous:
+ return previous
+ raise
- return get_rpm(rpminfo['id'])
+ return get_rpm(data['id'])
def import_build_log(fn, buildinfo, subdir=None):
"""Move a logfile related to a build to the right place"""
From 6dd0cb86ce5aa07d7fb199ea55e35cdec9d3d49d Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Feb 04 2018 13:01:27 +0000
Subject: [PATCH 2/4] use a savepoint
---
diff --git a/hub/kojihub.py b/hub/kojihub.py
index 4500acd..3da3def 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -5776,11 +5776,13 @@ def add_external_rpm(rpminfo, external_repo, strict=True):
data['build_id'] = None
data['buildroot_id'] = None
insert = InsertProcessor('rpminfo', data=data)
+ savepoint = Savepoint('pre_insert')
try:
insert.execute()
except Exception:
- # check for dup again to work around a race
+ # if this failed, it likely duplicates one just inserted
# see: https://pagure.io/koji/issue/788
+ savepoint.rollback()
previous = check_dup()
if previous:
return previous
@@ -7395,6 +7397,16 @@ def nextval(sequence):
return _singleValue("SELECT nextval(%(sequence)s)", data, strict=True)
+class Savepoint(object):
+
+ def __init__(self, name):
+ self.name = name
+ _dml("SAVEPOINT %s" % name, {})
+
+ def rollback(self):
+ _dml("ROLLBACK TO SAVEPOINT %s" % self.name, {})
+
+
def parse_json(value, desc=None, errstr=None):
if value is None:
return value
From 3c5c9f74d446ee6bca15cadceaf3c21b2f26be7c Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Feb 04 2018 15:08:00 +0000
Subject: [PATCH 3/4] unit test for add_external_rpm
---
diff --git a/tests/test_hub/test_add_external_rpm.py b/tests/test_hub/test_add_external_rpm.py
new file mode 100644
index 0000000..7aa928b
--- /dev/null
+++ b/tests/test_hub/test_add_external_rpm.py
@@ -0,0 +1,158 @@
+import mock
+import unittest
+
+import koji
+import kojihub
+
+
+IP = kojihub.InsertProcessor
+
+
+class FakeException(Exception):
+ pass
+
+
+class TestAddExternalRPM(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.rpminfo = {
+ 'name': 'NAME',
+ 'version': 'VERSION',
+ 'release': 'RELEASE',
+ 'epoch': None,
+ 'arch': 'noarch',
+ 'payloadhash': 'fakehash',
+ 'size': 42,
+ 'buildtime': 0,
+ }
+ self.repo = 'myrepo'
+
+ 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_add_ext_rpm(self):
+ self.get_rpm.return_value = None
+ self.get_external_repo_id.return_value = mock.sentinel.repo_id
+ self.nextval.return_value = mock.sentinel.rpm_id
+
+ # call it
+ kojihub.add_external_rpm(self.rpminfo, self.repo)
+
+ self.assertEqual(len(self.inserts), 1)
+ insert = self.inserts[0]
+ self.assertEqual(insert.data['external_repo_id'], mock.sentinel.repo_id)
+ self.assertEqual(insert.data['id'], mock.sentinel.rpm_id)
+ self.assertEqual(insert.table, 'rpminfo')
+
+ def test_add_ext_rpm_bad_data(self):
+ rpminfo = self.rpminfo.copy()
+ del rpminfo['size']
+
+ with self.assertRaises(koji.GenericError):
+ kojihub.add_external_rpm(rpminfo, self.repo)
+
+ self.get_rpm.assert_not_called()
+ self.nextval.assert_not_called()
+ self.assertEqual(len(self.inserts), 0)
+
+ rpminfo = self.rpminfo.copy()
+ rpminfo['size'] = ['invalid type']
+
+ with self.assertRaises(koji.GenericError):
+ kojihub.add_external_rpm(rpminfo, self.repo)
+
+ self.get_rpm.assert_not_called()
+ self.nextval.assert_not_called()
+ self.assertEqual(len(self.inserts), 0)
+
+ def test_add_ext_rpm_dup(self):
+ prev = self.rpminfo.copy()
+ prev['external_repo_id'] = mock.sentinel.repo_id
+ prev['external_repo_name'] = self.repo
+ self.get_rpm.return_value = prev
+ self.get_external_repo_id.return_value = mock.sentinel.repo_id
+
+ # call it (default is strict)
+ with self.assertRaises(koji.GenericError):
+ kojihub.add_external_rpm(self.rpminfo, self.repo)
+
+ self.assertEqual(len(self.inserts), 0)
+ self.nextval.assert_not_called()
+
+ # call it without strict
+ ret = kojihub.add_external_rpm(self.rpminfo, self.repo, strict=False)
+
+ self.assertEqual(ret, self.get_rpm.return_value)
+ self.assertEqual(len(self.inserts), 0)
+ self.nextval.assert_not_called()
+
+ # different hash
+ prev['payloadhash'] = 'different hash'
+ with self.assertRaises(koji.GenericError):
+ kojihub.add_external_rpm(self.rpminfo, self.repo, strict=False)
+
+ self.assertEqual(len(self.inserts), 0)
+ self.nextval.assert_not_called()
+
+ def test_add_ext_rpm_dup_late(self):
+ prev = self.rpminfo.copy()
+ prev['external_repo_id'] = mock.sentinel.repo_id
+ prev['external_repo_name'] = self.repo
+ self.get_rpm.side_effect = [None, prev]
+ self.get_external_repo_id.return_value = mock.sentinel.repo_id
+ self.insert_execute.side_effect = FakeException('insert failed')
+
+ # call it (default is strict)
+ with self.assertRaises(koji.GenericError):
+ kojihub.add_external_rpm(self.rpminfo, self.repo)
+
+ self.assertEqual(len(self.inserts), 1)
+ self.nextval.assert_called_once()
+
+ # call it without strict
+ self.inserts[:] = []
+ self.nextval.reset_mock()
+ self.get_rpm.side_effect = [None, prev]
+ ret = kojihub.add_external_rpm(self.rpminfo, self.repo, strict=False)
+
+ self.assertEqual(ret, prev)
+ self.assertEqual(len(self.inserts), 1)
+ self.nextval.assert_called_once()
+
+ # different hash
+ self.inserts[:] = []
+ self.nextval.reset_mock()
+ self.get_rpm.side_effect = [None, prev]
+ prev['payloadhash'] = 'different hash'
+ with self.assertRaises(koji.GenericError):
+ kojihub.add_external_rpm(self.rpminfo, self.repo, strict=False)
+
+ self.assertEqual(len(self.inserts), 1)
+ self.nextval.assert_called_once()
+
+ # no dup after failed insert
+ self.inserts[:] = []
+ self.nextval.reset_mock()
+ self.get_rpm.side_effect = [None, None]
+ with self.assertRaises(FakeException):
+ kojihub.add_external_rpm(self.rpminfo, self.repo, strict=False)
+
+ self.assertEqual(len(self.inserts), 1)
+ self.nextval.assert_called_once()
+
+
From a7e866cf8a39f56e694372cfd9d6099f7bc5be0d Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Feb 04 2018 15:15:40 +0000
Subject: [PATCH 4/4] unit test for Savepoint
---
diff --git a/tests/test_hub/test_savepoint.py b/tests/test_hub/test_savepoint.py
new file mode 100644
index 0000000..279729e
--- /dev/null
+++ b/tests/test_hub/test_savepoint.py
@@ -0,0 +1,22 @@
+import mock
+import unittest
+
+import kojihub
+
+
+class TestSavepoint(unittest.TestCase):
+
+ def setUp(self):
+ self.dml = mock.patch('kojihub._dml').start()
+
+ def tearDown(self):
+ mock.patch.stopall()
+
+ def test_savepoint(self):
+ sp = kojihub.Savepoint('some_name')
+ self.assertEqual(sp.name, 'some_name')
+ self.dml.assert_called_once_with('SAVEPOINT some_name', {})
+
+ self.dml.reset_mock()
+ sp.rollback()
+ self.dml.assert_called_once_with('ROLLBACK TO SAVEPOINT some_name', {})