koji/794.patch

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', {})