From 2db63c7b8c4dac49087ccc604bcca74896c8a65b Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Nov 23 2022 07:41:50 +0000 Subject: [PATCH 1/2] Replace _multiRow, _singleRow, _singleValue with QP Fixes: https://pagure.io/koji/issue/3581 --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 680f9d3..b675d68 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -75,7 +75,7 @@ from koji.util import ( multi_fnmatch, safer_move, ) -from koji.db import ( +from koji.db import ( # noqa: F401 BulkInsertProcessor, InsertProcessor, QueryProcessor, @@ -84,11 +84,12 @@ from koji.db import ( _applyQueryOpts, _dml, _fetchSingle, - _multiRow, - _singleRow, + _multiRow, # needed for backward compatibility, will removed in Koji 1.36 + _singleRow, # needed for backward compatibility, will removed in Koji 1.36 _singleValue, get_event, nextval, + currval, ) @@ -735,7 +736,7 @@ def make_task(method, arglist, **opts): idata['host_id'] = opts['assign'] insert = InsertProcessor('task', data=idata) insert.execute() - task_id = _singleValue("SELECT currval('task_id_seq')", strict=True) + task_id = currval('task_id_seq') opts['id'] = task_id koji.plugin.run_callbacks( 'postTaskStateChange', attribute='state', old=None, new='FREE', info=opts) @@ -2284,15 +2285,15 @@ def get_tag_groups(tag, event=None, inherit=True, incl_pkgs=True, incl_reqs=True evcondition = eventCondition(event) # First get the list of groups - fields = ('name', 'group_id', 'tag_id', 'blocked', 'exported', 'display_name', - 'is_default', 'uservisible', 'description', 'langonly', 'biarchonly',) - q = """ - SELECT %s FROM group_config JOIN groups ON group_id = id - WHERE %s AND tag_id = %%(tagid)s - """ % (",".join(fields), evcondition) + columns = ['name', 'group_id', 'tag_id', 'blocked', 'exported', 'display_name', + 'is_default', 'uservisible', 'description', 'langonly', 'biarchonly'] groups = {} for tagid in taglist: - for group in _multiRow(q, locals(), fields): + query = QueryProcessor(tables=['group_config'], columns=columns, + joins=['groups ON group_id = id'], + clauses=[evcondition, 'tag_id = %(tagid)s'], + values={'tagid': tagid}) + for group in query.execute(): grp_id = group['group_id'] # we only take the first entry for group as we go through inheritance groups.setdefault(grp_id, group) @@ -2300,13 +2301,12 @@ def get_tag_groups(tag, event=None, inherit=True, incl_pkgs=True, incl_reqs=True if incl_pkgs: for group in groups.values(): group['packagelist'] = {} - fields = ('group_id', 'tag_id', 'package', 'blocked', 'type', 'basearchonly', 'requires') - q = """ - SELECT %s FROM group_package_listing - WHERE %s AND tag_id = %%(tagid)s - """ % (",".join(fields), evcondition) + columns = ['group_id', 'tag_id', 'package', 'blocked', 'type', 'basearchonly', 'requires'] for tagid in taglist: - for grp_pkg in _multiRow(q, locals(), fields): + query = QueryProcessor(tables=['group_package_listing'], columns=columns, + clauses=[evcondition, 'tag_id = %(tagid)s'], + values={'tagid': tagid}) + for grp_pkg in query.execute(): grp_id = grp_pkg['group_id'] if grp_id not in groups: # tag does not have this group @@ -2322,12 +2322,13 @@ def get_tag_groups(tag, event=None, inherit=True, incl_pkgs=True, incl_reqs=True # and now the group reqs for group in groups.values(): group['grouplist'] = {} - fields = ('group_id', 'tag_id', 'req_id', 'blocked', 'type', 'is_metapkg', 'name') - q = """SELECT %s FROM group_req_listing JOIN groups on req_id = id - WHERE %s AND tag_id = %%(tagid)s - """ % (",".join(fields), evcondition) + columns = ['group_id', 'tag_id', 'req_id', 'blocked', 'type', 'is_metapkg', 'name'] for tagid in taglist: - for grp_req in _multiRow(q, locals(), fields): + query = QueryProcessor(tables=['group_req_listing'], columns=columns, + joins=['groups on req_id = id'], + clauses=[evcondition, 'tag_id = %(tagid)s'], + values={'tagid': tagid}) + for grp_req in query.execute(): grp_id = grp_req['group_id'] if grp_id not in groups: # tag does not have this group @@ -2991,7 +2992,7 @@ def repo_info(repo_id, strict=False): :returns: dict (id, state, create_event, creation_time, tag_id, tag_name, dist) """ - fields = ( + fields = [ ('repo.id', 'id'), ('repo.state', 'state'), ('repo.task_id', 'task_id'), @@ -3001,12 +3002,12 @@ def repo_info(repo_id, strict=False): ('repo.tag_id', 'tag_id'), ('tag.name', 'tag_name'), ('repo.dist', 'dist'), - ) - q = """SELECT %s FROM repo - JOIN tag ON tag_id=tag.id - JOIN events ON repo.create_event = events.id - WHERE repo.id = %%(repo_id)s""" % ','.join([f[0] for f in fields]) - return _singleRow(q, locals(), [f[1] for f in fields], strict=strict) + ] + columns, aliases = zip(*fields) + joins = ['tag ON tag_id=tag.id', 'events ON repo.create_event = events.id'] + query = QueryProcessor(tables=['repo'], columns=columns, aliases=aliases, joins=joins, + clauses=['repo.id = %(repo_id)s'], values={'repo_id': repo_id}) + return query.executeOne(strict=strict) def repo_ready(repo_id): @@ -7412,9 +7413,9 @@ def add_archive_type(name, description, extensions, compression_type=None): for ext in extensions.split(' '): if not ext.replace('.', '').isalnum(): raise koji.GenericError(f'No such {ext} file extension') - select = r"""SELECT id FROM archivetypes - WHERE extensions ~* E'(\\s|^)%s(\\s|$)'""" % ext - results = _multiRow(select, {}, ('id',)) + query = QueryProcessor(tables=['archivetypes'], columns=['id'], + clauses=[f"extensions ~* E'(\\s|^){ext}(\\s|$)'"], values={}) + results = query.execute() if len(results) > 0: raise koji.GenericError(f'file extension {ext} already exists') insert = InsertProcessor('archivetypes', data=data) @@ -8336,10 +8337,16 @@ def build_references(build_id, limit=None, lazy=False): ret = {} # find tags - q = """SELECT tag_id, tag.name FROM tag_listing JOIN tag on tag_id = tag.id - WHERE build_id = %(build_id)i AND active = TRUE""" - ret['tags'] = _multiRow(q, locals(), ('id', 'name')) - + fields = { + 'tag_id': 'tag_id', + 'tag.name': 'name', + } + columns, aliases = zip(*fields.items()) + query = QueryProcessor(tables=['tag_listing'], columns=columns, aliases=aliases, + joins=['tag on tag_id = tag.id'], + clauses=['build_id = %(build_id)i', 'active = TRUE'], + values={'build_id': build_id}) + ret['tags'] = query.execute() if lazy and ret['tags']: return ret @@ -8403,20 +8410,29 @@ def build_references(build_id, limit=None, lazy=False): return ret # find archives whose buildroots we were in - fields = ('id', 'type_id', 'type_name', 'build_id', 'filename') + fields = { + 'archiveinfo.id': 'id', + 'archiveinfo.type_id': 'type_id', + 'archivetypes.name': 'type_name', + 'archiveinfo.build_id': 'build_id', + 'archiveinfo.filename': 'filename', + } + columns, aliases = zip(*fields.items()) idx = {} - q = """SELECT archiveinfo.id, archiveinfo.type_id, archivetypes.name, archiveinfo.build_id, - archiveinfo.filename - FROM buildroot_archives - JOIN archiveinfo ON archiveinfo.buildroot_id = buildroot_archives.buildroot_id - JOIN build ON archiveinfo.build_id = build.id - JOIN archivetypes ON archivetypes.id = archiveinfo.type_id - WHERE buildroot_archives.archive_id = %(archive_id)i - AND build.state = %(st_complete)i""" + opts = {} if limit is not None: - q += "\nLIMIT %(limit)i" + opts = {'limit': limit} for (archive_id,) in build_archive_ids: - for row in _multiRow(q, locals(), fields): + query = QueryProcessor(tables=['buildroot_archives'], columns=columns, aliases=aliases, + joins=['archiveinfo ON archiveinfo.buildroot_id = ' + 'buildroot_archives.buildroot_id', + 'build ON archiveinfo.build_id = build.id', + 'archivetypes ON archivetypes.id = archiveinfo.type_id'], + clauses=['buildroot_archives.archive_id = %(archive_id)i', + 'build.state = %(st_complete)i'], + values={'archive_id': archive_id, 'st_complete': st_complete}, + opts=opts) + for row in query.execute(): idx.setdefault(row['id'], row) if limit is not None and len(idx) > limit: break @@ -10329,10 +10345,14 @@ class RootExports(object): If no event with the given id exists, an error will be raised. """ - fields = ('id', 'ts') - values = {'id': id} - q = """SELECT id, date_part('epoch', time) FROM events WHERE id = %(id)i""" - return _singleRow(q, values, fields, strict=True) + fields = { + 'id': 'id', + "date_part('epoch', time)": 'ts', + } + columns, aliases = zip(*fields.items()) + query = QueryProcessor(tables=['events'], columns=columns, aliases=aliases, + clauses=['id = %(id)i'], values={'id': id}) + return query.executeOne(strict=True) def getLastEvent(self, before=None): """ @@ -10351,18 +10371,24 @@ class RootExports(object): When trying to find information about a specific event, the getEvent() method should be used. """ - fields = ('id', 'ts') + fields = { + 'id': 'id', + "date_part('epoch', time)": 'ts', + } + columns, aliases = zip(*fields.items()) values = {} - q = """SELECT id, date_part('epoch', time) FROM events""" + clauses = [] if before is not None: if not isinstance(before, NUMERIC_TYPES): raise koji.GenericError('Invalid type for before: %s' % type(before)) # use the repr() conversion because it retains more precision than the # string conversion - q += """ WHERE date_part('epoch', time) < %(before)r""" + clauses = ["date_part('epoch', time) < %(before)r"] values['before'] = before - q += """ ORDER BY id DESC LIMIT 1""" - return _singleRow(q, values, fields, strict=True) + opts = {'order': '-id', 'limit': 1} + query = QueryProcessor(tables=['events'], columns=columns, aliases=aliases, + clauses=clauses, values=values, opts=opts) + return query.executeOne(strict=True) evalPolicy = staticmethod(eval_policy) @@ -11882,19 +11908,17 @@ class RootExports(object): st_complete = koji.BUILD_STATES['COMPLETE'] # we need to filter out builds without tasks (imports) as they'll reduce # time average. CG imported builds often contain *_koji_task_id instead. - query = """SELECT date_part('epoch', avg(build.completion_time - events.time)) - FROM build - JOIN events ON build.create_event = events.id - WHERE build.pkg_id = %(packageID)i - AND build.state = %(st_complete)i - AND ( - build.task_id IS NOT NULL OR - build.extra LIKE '%%' || 'koji_task_id' || '%%' - )""" + clauses = ['build.pkg_id = %(packageID)i', 'build.state = %(st_complete)i', + "build.task_id IS NOT NULL OR build.extra LIKE '%' || 'koji_task_id' || '%'"] if age is not None: - query += " AND build.completion_time > NOW() - '%s months'::interval" % int(age) - - return _singleValue(query, locals()) + clauses.append(f"build.completion_time > NOW() - '{int(age)} months'::interval") + query = QueryProcessor(tables=['build'], + columns=["date_part('epoch', " + "avg(build.completion_time - events.time))"], + joins=['events ON build.create_event = events.id'], + clauses=clauses, + values={'packageID': packageID, 'st_complete': st_complete}) + return query.singleValue() packageListAdd = staticmethod(pkglist_add) packageListRemove = staticmethod(pkglist_remove) @@ -13073,14 +13097,13 @@ class RootExports(object): The timestamp represents the last time the host with the given ID contacted the hub. Returns None if the host has never contacted - the hub.""" - query = """SELECT update_time FROM sessions - JOIN host ON sessions.user_id = host.user_id - WHERE host.id = %(hostID)i - ORDER BY update_time DESC - LIMIT 1 - """ - date = _singleValue(query, locals(), strict=False) + the hub.""" + opts = {'order': '-update_time', 'limit': 1} + query = QueryProcessor(tables=['sessions'], columns=['update_time'], + joins=['host ON sessions.user_id = host.user_id'], + clauses=['host.id = %(hostID)i'], values={'hostID': hostID}, + opts=opts) + date = query.singleValue(strict=False) if ts and date is not None: return date.timestamp() else: diff --git a/koji/db.py b/koji/db.py index 6e0b549..7a47d9a 100644 --- a/koji/db.py +++ b/koji/db.py @@ -315,6 +315,12 @@ def nextval(sequence): return _singleValue("SELECT nextval(%(sequence)s)", data, strict=True) +def currval(sequence): + """Get the current value for the given sequence""" + data = {'sequence': sequence} + return _singleValue("SELECT currval(%(sequence)s)", data, strict=True) + + class Savepoint(object): def __init__(self, name): diff --git a/tests/test_hub/test_add_archivetype.py b/tests/test_hub/test_add_archivetype.py index 41dd944..e37e0d9 100644 --- a/tests/test_hub/test_add_archivetype.py +++ b/tests/test_hub/test_add_archivetype.py @@ -6,10 +6,10 @@ import koji import kojihub IP = kojihub.InsertProcessor +QP = kojihub.QueryProcessor class TestAddArchiveType(unittest.TestCase): - def setUp(self): self.context = mock.patch('kojihub.context').start() @@ -23,7 +23,10 @@ class TestAddArchiveType(unittest.TestCase): self.insert_execute = mock.MagicMock() self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start() self.get_archive_type = mock.patch('kojihub.get_archive_type').start() - self._multiRow = mock.patch('kojihub._multiRow').start() + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + self.query_execute = mock.MagicMock() def tearDown(self): mock.patch.stopall() @@ -34,7 +37,14 @@ class TestAddArchiveType(unittest.TestCase): self.inserts.append(insert) return insert + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = self.query_execute + self.queries.append(query) + return query + def test_add_archive_type_valid_empty_compression_type(self): + self.query_execute.side_effect = [[]] self.verify_name_internal.return_value = None self.get_archive_type.return_value = None kojihub.add_archive_type('deb', 'Debian package', 'deb') @@ -50,6 +60,7 @@ class TestAddArchiveType(unittest.TestCase): self.context.session.assertPerm.assert_called_with('admin') def test_add_archive_type_valid_with_compression_type(self): + self.query_execute.side_effect = [[]] self.verify_name_internal.return_value = None self.get_archive_type.return_value = None kojihub.add_archive_type('jar', 'Jar package', 'jar', 'zip') diff --git a/tests/test_hub/test_get_last_host_update.py b/tests/test_hub/test_get_last_host_update.py index ee2a6b6..46827d1 100644 --- a/tests/test_hub/test_get_last_host_update.py +++ b/tests/test_hub/test_get_last_host_update.py @@ -5,14 +5,24 @@ import sys import kojihub +QP = kojihub.QueryProcessor + class TestGetLastHostUpdate(unittest.TestCase): + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.singleValue = self.query_singleValue + self.queries.append(query) + return query def setUp(self): self.exports = kojihub.RootExports() + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + self.query_singleValue = mock.MagicMock() - @mock.patch('kojihub._singleValue') - def test_valid_ts(self, _singleValue): + def test_valid_ts(self): expected = 1615875554.862938 if sys.version_info[1] <= 6: dt = datetime.datetime.strptime( @@ -20,19 +30,17 @@ class TestGetLastHostUpdate(unittest.TestCase): else: dt = datetime.datetime.strptime( "2021-03-16T06:19:14.862938+00:00", "%Y-%m-%dT%H:%M:%S.%f%z") - _singleValue.return_value = dt + self.query_singleValue.return_value = dt rv = self.exports.getLastHostUpdate(1, ts=True) self.assertEqual(rv, expected) - @mock.patch('kojihub._singleValue') - def test_valid_datetime(self, _singleValue): + def test_valid_datetime(self): if sys.version_info[1] <= 6: dt = datetime.datetime.strptime( "2021-03-16T06:19:14.862938+0000", "%Y-%m-%dT%H:%M:%S.%f%z") else: dt = datetime.datetime.strptime( "2021-03-16T06:19:14.862938+00:00", "%Y-%m-%dT%H:%M:%S.%f%z") - expected = "2021-03-16T06:19:14.862938+00:00" - _singleValue.return_value = dt + self.query_singleValue.return_value = dt rv = self.exports.getLastHostUpdate(1) self.assertEqual(rv, dt) diff --git a/tests/test_hub/test_new_build.py b/tests/test_hub/test_new_build.py index bfc8e1c..3b215b2 100644 --- a/tests/test_hub/test_new_build.py +++ b/tests/test_hub/test_new_build.py @@ -23,7 +23,6 @@ class TestNewBuild(unittest.TestCase): 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() diff --git a/tests/test_hub/test_repos.py b/tests/test_hub/test_repos.py index 2e2faf4..97f7fbd 100644 --- a/tests/test_hub/test_repos.py +++ b/tests/test_hub/test_repos.py @@ -27,6 +27,7 @@ class TestRepoFunctions(unittest.TestCase): self._dml = mock.patch('kojihub._dml').start() self.exports = kojihub.RootExports() self.get_tag = mock.patch('kojihub.get_tag').start() + self.query_executeOne = mock.MagicMock() def tearDown(self): mock.patch.stopall() @@ -34,6 +35,7 @@ class TestRepoFunctions(unittest.TestCase): def getQuery(self, *args, **kwargs): query = QP(*args, **kwargs) query.execute = mock.MagicMock() + query.executeOne = self.query_executeOne self.queries.append(query) return query @@ -78,8 +80,7 @@ class TestRepoFunctions(unittest.TestCase): if 'dist = %(dist)s' not in update.clauses: raise Exception('Missing dist condition') - @mock.patch('kojihub._singleRow') - def test_repo_info(self, _singleRow): + def test_repo_info(self): repo_row = {'id': 10, 'state': 0, 'task_id': 15, @@ -90,7 +91,7 @@ class TestRepoFunctions(unittest.TestCase): 'tag_id': 3, 'tag_name': 'test-tag', 'dist': False} - _singleRow.return_value = repo_row + self.query_executeOne.return_value = repo_row rv = kojihub.repo_info(3) self.assertEqual(rv, repo_row) diff --git a/tests/test_hub/test_user_groups.py b/tests/test_hub/test_user_groups.py index 78c09bb..2dd67d3 100644 --- a/tests/test_hub/test_user_groups.py +++ b/tests/test_hub/test_user_groups.py @@ -259,8 +259,7 @@ class TestGrouplist(unittest.TestCase): self.assertEqual(u.values['user_id'], uid) self.assertEqual(u.values['group_id'], gid) - @mock.patch('kojihub._multiRow') - def test_get_group_members(self, _multiRow): + def test_get_group_members(self): group, gid = 'test_group', 1 # no permission @@ -300,4 +299,3 @@ class TestGrouplist(unittest.TestCase): self.assertEqual(len(self.queries), 1) self.assertEqual(len(self.inserts), 0) self.assertEqual(len(self.updates), 0) - _multiRow.assert_not_called() From a95d04607f95930857596d9aacc6ddb542dc248d Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Dec 02 2022 10:33:29 +0000 Subject: [PATCH 2/2] Fix fields type and dict of query.execute --- diff --git a/hub/kojihub.py b/hub/kojihub.py index b675d68..310cef2 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3059,12 +3059,13 @@ def repo_expire_older(tag_id, event_id, dist=None): def repo_references(repo_id): """Return a list of buildroots that reference the repo""" - fields = { - 'buildroot_id': 'id', - 'host_id': 'host_id', - 'create_event': 'create_event', - 'state': 'state'} - fields, aliases = zip(*fields.items()) + fields = [ + ('buildroot_id', 'id'), + ('host_id', 'host_id'), + ('create_event', 'create_event'), + ('state', 'state'), + ] + fields, aliases = zip(*fields) query = QueryProcessor( tables=['standard_buildroot'], columns=fields, aliases=aliases, @@ -3559,15 +3560,16 @@ def get_tag(tagInfo, strict=False, event=None, blocked=False): tables = ['tag_config'] joins = ['tag ON tag.id = tag_config.tag_id', 'LEFT OUTER JOIN permissions ON tag_config.perm_id = permissions.id'] - fields = {'tag.id': 'id', - 'tag.name': 'name', - 'tag_config.perm_id': 'perm_id', - 'permissions.name': 'perm', - 'tag_config.arches': 'arches', - 'tag_config.locked': 'locked', - 'tag_config.maven_support': 'maven_support', - 'tag_config.maven_include_all': 'maven_include_all', - } + fields = [ + ('tag.id', 'id'), + ('tag.name', 'name'), + ('tag_config.perm_id', 'perm_id'), + ('permissions.name', 'perm'), + ('tag_config.arches', 'arches'), + ('tag_config.locked', 'locked'), + ('tag_config.maven_support', 'maven_support'), + ('tag_config.maven_include_all', 'maven_include_all') + ] clause, values = name_or_id_clause('tag', tagInfo) clauses = [clause] @@ -3585,13 +3587,13 @@ def get_tag(tagInfo, strict=False, event=None, blocked=False): # query point instantly before the revoke_event # (to get latest tag_config before deletion) event -= 1 - fields['tag_config.revoke_event'] = 'revoke_event' + fields.append(('tag_config.revoke_event', 'revoke_event')) else: # if tag is not deleted, query event=None pass clauses.append(eventCondition(event, table='tag_config')) - fields, aliases = zip(*fields.items()) + fields, aliases = zip(*fields) query = QueryProcessor(columns=fields, aliases=aliases, tables=tables, joins=joins, clauses=clauses, values=values) result = query.executeOne() @@ -4091,17 +4093,17 @@ def get_tag_external_repos(tag_info=None, repo_info=None, event=None): joins = ['tag ON tag_external_repos.tag_id = tag.id', 'external_repo ON tag_external_repos.external_repo_id = external_repo.id', 'external_repo_config ON external_repo.id = external_repo_config.external_repo_id'] - fields = { - 'external_repo.id': 'external_repo_id', - 'external_repo.name': 'external_repo_name', - 'priority': 'priority', - 'tag.id': 'tag_id', - 'tag.name': 'tag_name', - 'url': 'url', - 'merge_mode': 'merge_mode', - 'arches': 'arches', - } - columns, aliases = zip(*fields.items()) + fields = [ + ('external_repo.id', 'external_repo_id'), + ('external_repo.name', 'external_repo_name'), + ('priority', 'priority'), + ('tag.id', 'tag_id'), + ('tag.name', 'tag_name'), + ('url', 'url'), + ('merge_mode', 'merge_mode'), + ('arches', 'arches'), + ] + columns, aliases = zip(*fields) clauses = [eventCondition(event, table='tag_external_repos'), eventCondition(event, table='external_repo_config')] @@ -4861,8 +4863,8 @@ def get_image_build(buildInfo, strict=False): build_id = find_build_id(buildInfo, strict=strict) if not build_id: return None - query = QueryProcessor(tables=('image_builds',), columns=('build_id',), - clauses=('build_id = %(build_id)i',), + query = QueryProcessor(tables=['image_builds'], columns=['build_id'], + clauses=['build_id = %(build_id)i'], values={'build_id': build_id}) result = query.executeOne() if strict and not result: @@ -5491,23 +5493,24 @@ def get_host(hostInfo, strict=False, event=None): tables = ['host_config'] joins = ['host ON host.id = host_config.host_id'] - fields = {'host.id': 'id', - 'host.user_id': 'user_id', - 'host.name': 'name', - 'host.ready': 'ready', - 'host.task_load': 'task_load', - 'host_config.arches': 'arches', - 'host_config.capacity': 'capacity', - 'host_config.description': 'description', - 'host_config.comment': 'comment', - 'host_config.enabled': 'enabled', - } + fields = [ + ('host.id', 'id'), + ('host.user_id', 'user_id'), + ('host.name', 'name'), + ('host.ready', 'ready'), + ('host.task_load', 'task_load'), + ('host_config.arches', 'arches'), + ('host_config.capacity', 'capacity'), + ('host_config.description', 'description'), + ('host_config.comment', 'comment'), + ('host_config.enabled', 'enabled'), + ] clauses = [eventCondition(event, table='host_config')] clause, values = name_or_id_clause('host', hostInfo) clauses.append(clause) - fields, aliases = zip(*fields.items()) + fields, aliases = zip(*fields) query = QueryProcessor(columns=fields, aliases=aliases, tables=tables, joins=joins, clauses=clauses, values=values) result = query.executeOne() @@ -5730,9 +5733,14 @@ def list_channels(hostID=None, event=None, enabled=None): [{'comment': 'test channel', 'description': 'container channel', 'enabled': True, 'id': 20, 'name': 'container', 'container channel' }] """ - fields = {'channels.id': 'id', 'channels.name': 'name', 'channels.description': 'description', - 'channels.enabled': 'enabled', 'channels.comment': 'comment'} - columns, aliases = zip(*fields.items()) + fields = [ + ('channels.id', 'id'), + ('channels.name', 'name'), + ('channels.description', 'description'), + ('channels.enabled', 'enabled'), + ('channels.comment', 'comment'), + ] + columns, aliases = zip(*fields) if enabled is not None: if enabled: enable_clause = 'enabled IS TRUE' @@ -7489,8 +7497,8 @@ def new_image_build(build_info): # We don't have to worry about updating an image build because the id is # the only thing we care about, and that should never change if a build # fails first and succeeds later on a resubmission. - query = QueryProcessor(tables=('image_builds',), columns=('build_id',), - clauses=('build_id = %(build_id)i',), + query = QueryProcessor(tables=['image_builds'], columns=['build_id'], + clauses=['build_id = %(build_id)i'], values={'build_id': build_info['id']}) result = query.executeOne() if not result: @@ -7505,11 +7513,9 @@ def new_typed_build(build_info, btype): """Mark build as a given btype""" btype_id = lookup_name('btype', btype, strict=True)['id'] - query = QueryProcessor(tables=('build_types',), columns=('build_id',), - clauses=('build_id = %(build_id)i', - 'btype_id = %(btype_id)i'), - values={'build_id': build_info['id'], - 'btype_id': btype_id}) + query = QueryProcessor(tables=['build_types'], columns=['build_id'], + clauses=['build_id = %(build_id)i', 'btype_id = %(btype_id)i'], + values={'build_id': build_info['id'], 'btype_id': btype_id}) result = query.executeOne() if not result: insert = InsertProcessor('build_types') @@ -8337,11 +8343,11 @@ def build_references(build_id, limit=None, lazy=False): ret = {} # find tags - fields = { - 'tag_id': 'tag_id', - 'tag.name': 'name', - } - columns, aliases = zip(*fields.items()) + fields = [ + ('tag_id', 'tag_id'), + ('tag.name', 'name') + ] + columns, aliases = zip(*fields) query = QueryProcessor(tables=['tag_listing'], columns=columns, aliases=aliases, joins=['tag on tag_id = tag.id'], clauses=['build_id = %(build_id)i', 'active = TRUE'], @@ -8354,11 +8360,11 @@ def build_references(build_id, limit=None, lazy=False): query = QueryProcessor(tables=['rpminfo'], columns=['id'], clauses=['build_id=%(build_id)i'], values={'build_id': build_id}, opts={'asList': True}) - build_rpm_ids = query.execute() + build_rpm_ids = [(i,) for i in query.execute()] query = QueryProcessor(tables=['archiveinfo'], columns=['id'], clauses=['build_id=%(build_id)i'], values={'build_id': build_id}, opts={'asList': True}) - build_archive_ids = query.execute() + build_archive_ids = [(i,) for i in query.execute()] if not build_archive_ids: build_archive_ids = [] @@ -8378,7 +8384,7 @@ def build_references(build_id, limit=None, lazy=False): AND build.state = %(st_complete)i""" if limit is not None: q += "\nLIMIT %(limit)i" - for (rpm_id,) in build_rpm_ids: + for rpm_id in build_rpm_ids: for row in _multiRow(q, locals(), fields): idx.setdefault(row['id'], row) if limit is not None and len(idx) > limit: @@ -8401,7 +8407,7 @@ def build_references(build_id, limit=None, lazy=False): qopts['limit'] = limit query = QueryProcessor(columns=fields, tables=['archive_rpm_components'], clauses=clauses, joins=joins, values=values, opts=qopts) - for (rpm_id,) in build_rpm_ids: + for rpm_id in build_rpm_ids: query.values['rpm_id'] = rpm_id archive_ids = [i[0] for i in query.execute()] ret['component_of'].extend(archive_ids) @@ -8410,19 +8416,19 @@ def build_references(build_id, limit=None, lazy=False): return ret # find archives whose buildroots we were in - fields = { - 'archiveinfo.id': 'id', - 'archiveinfo.type_id': 'type_id', - 'archivetypes.name': 'type_name', - 'archiveinfo.build_id': 'build_id', - 'archiveinfo.filename': 'filename', - } - columns, aliases = zip(*fields.items()) + fields = [ + ('archiveinfo.id', 'id'), + ('archiveinfo.type_id', 'type_id'), + ('archivetypes.name', 'type_name'), + ('archiveinfo.build_id', 'build_id'), + ('archiveinfo.filename', 'filename') + ] + columns, aliases = zip(*fields) idx = {} opts = {} if limit is not None: opts = {'limit': limit} - for (archive_id,) in build_archive_ids: + for archive_id in build_archive_ids: query = QueryProcessor(tables=['buildroot_archives'], columns=columns, aliases=aliases, joins=['archiveinfo ON archiveinfo.buildroot_id = ' 'buildroot_archives.buildroot_id', @@ -8453,7 +8459,7 @@ def build_references(build_id, limit=None, lazy=False): qopts['limit'] = limit query = QueryProcessor(columns=fields, tables=['archive_components'], clauses=clauses, joins=joins, values=values, opts=qopts) - for (archive_id,) in build_archive_ids: + for archive_id in build_archive_ids: query.values['archive_id'] = archive_id archive_ids = [i[0] for i in query.execute()] ret['component_of'].extend(archive_ids) @@ -8930,9 +8936,9 @@ def add_group_member(group, user, strict=True): # check to see if user is already a member data = {'user_id': uinfo['id'], 'group_id': ginfo['id']} table = 'user_groups' - clauses = ('user_id = %(user_id)i', 'group_id = %(group_id)s') query = QueryProcessor(columns=['user_id'], tables=[table], - clauses=('active = TRUE',) + clauses, + clauses=['active = TRUE', 'user_id = %(user_id)i', + 'group_id = %(group_id)s'], values=data, opts={'rowlock': True}) row = query.executeOne() if row: @@ -9005,9 +9011,12 @@ def list_cgs(): are permitted to import for this content generator. """ - fields = {'content_generator.id': 'id', 'content_generator.name': 'name', - 'users.name': 'user_name'} - columns, aliases = zip(*fields.items()) + fields = [ + ('content_generator.id', 'id'), + ('content_generator.name', 'name'), + ('users.name', 'user_name'), + ] + columns, aliases = zip(*fields) tables = ['cg_users'] joins = ['content_generator ON content_generator.id = cg_users.cg_id', 'users ON users.id = cg_users.user_id'] @@ -10284,16 +10293,17 @@ class RootExports(object): if not context.session.logged_in: return None clauses = ['expired is FALSE'] - fields = {'user_id': 'user_id', - 'expired': 'expired', - 'master': 'master', - 'authtype': 'authtype', - 'callnum': 'callnum', - "date_part('epoch', start_time)": 'start_time', - 'update_time': 'update_time', - 'exclusive': 'exclusive', - } - columns, aliases = zip(*fields.items()) + fields = [ + ('user_id', 'user_id'), + ('expired', 'expired'), + ('master', 'master'), + ('authtype', 'authtype'), + ('callnum', 'callnum'), + ("date_part('epoch', start_time)", 'start_time'), + ('update_time', 'update_time'), + ('exclusive', 'exclusive'), + ] + columns, aliases = zip(*fields) if details: columns += ('hostip', 'id') aliases += ('hostip', 'id') @@ -10345,11 +10355,11 @@ class RootExports(object): If no event with the given id exists, an error will be raised. """ - fields = { - 'id': 'id', - "date_part('epoch', time)": 'ts', - } - columns, aliases = zip(*fields.items()) + fields = [ + ('id', 'id'), + ("date_part('epoch', time)", 'ts') + ] + columns, aliases = zip(*fields) query = QueryProcessor(tables=['events'], columns=columns, aliases=aliases, clauses=['id = %(id)i'], values={'id': id}) return query.executeOne(strict=True) @@ -10371,11 +10381,11 @@ class RootExports(object): When trying to find information about a specific event, the getEvent() method should be used. """ - fields = { - 'id': 'id', - "date_part('epoch', time)": 'ts', - } - columns, aliases = zip(*fields.items()) + fields = [ + ('id', 'id'), + ("date_part('epoch', time)", 'ts') + ] + columns, aliases = zip(*fields) values = {} clauses = [] if before is not None: @@ -11909,7 +11919,7 @@ class RootExports(object): # we need to filter out builds without tasks (imports) as they'll reduce # time average. CG imported builds often contain *_koji_task_id instead. clauses = ['build.pkg_id = %(packageID)i', 'build.state = %(st_complete)i', - "build.task_id IS NOT NULL OR build.extra LIKE '%' || 'koji_task_id' || '%'"] + "build.task_id IS NOT NULL OR build.extra LIKE '%%' || 'koji_task_id' || '%%'"] if age is not None: clauses.append(f"build.completion_time > NOW() - '{int(age)} months'::interval") query = QueryProcessor(tables=['build'], @@ -12549,7 +12559,7 @@ class RootExports(object): else: raise koji.GenericError('queryOpts.group is not available for this API') query = QueryProcessor(columns=fields, aliases=aliases, - tables=('users',), joins=joins, clauses=clauses, + tables=['users'], joins=joins, clauses=clauses, values=locals(), opts=queryOpts, enable_group=True, transform=xform_user_krb) return query.execute() @@ -13075,19 +13085,20 @@ class RootExports(object): userID = get_user(userID, strict=True)['id'] clauses.append('user_id = %(userID)i') - fields = {'host.id': 'id', - 'host.user_id': 'user_id', - 'host.name': 'name', - 'host.ready': 'ready', - 'host.task_load': 'task_load', - 'host_config.arches': 'arches', - 'host_config.capacity': 'capacity', - 'host_config.description': 'description', - 'host_config.comment': 'comment', - 'host_config.enabled': 'enabled', - } + fields = [ + ('host.id', 'id'), + ('host.user_id', 'user_id'), + ('host.name', 'name'), + ('host.ready', 'ready'), + ('host.task_load', 'task_load'), + ('host_config.arches', 'arches'), + ('host_config.capacity', 'capacity'), + ('host_config.description', 'description'), + ('host_config.comment', 'comment'), + ('host_config.enabled', 'enabled'), + ] tables = ['host_config'] - fields, aliases = zip(*fields.items()) + fields, aliases = zip(*fields) query = QueryProcessor(columns=fields, aliases=aliases, tables=tables, joins=joins, clauses=clauses, values=locals()) return query.execute() diff --git a/tests/test_hub/test_get_build_type.py b/tests/test_hub/test_get_build_type.py index 0ab137a..9631446 100644 --- a/tests/test_hub/test_get_build_type.py +++ b/tests/test_hub/test_get_build_type.py @@ -3,43 +3,55 @@ import unittest import kojihub +QP = kojihub.QueryProcessor + class TestGetBuildType(unittest.TestCase): - @mock.patch('kojihub.get_build') - @mock.patch('kojihub.QueryProcessor') - def test_no_build(self, QueryProcessor, get_build): - get_build.return_value = None + def setUp(self): + self.maxDiff = None + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + self.query_execute = mock.MagicMock() + self.get_build = mock.patch('kojihub.get_build').start() + self.get_maven_build = mock.patch('kojihub.get_maven_build').start() + self.get_win_build = mock.patch('kojihub.get_win_build').start() + self.get_image_build = mock.patch('kojihub.get_image_build').start() + + def tearDown(self): + mock.patch.stopall() + + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = self.query_execute + self.queries.append(query) + return query + + def test_no_build(self): + self.get_build.return_value = None # strict on kojihub.get_build_type('mytestbuild-1-1', strict=True) - QueryProcessor.assert_not_called() - get_build.assert_called_with('mytestbuild-1-1', strict=True) - + self.assertEqual(len(self.queries), 0) + self.get_build.assert_called_with('mytestbuild-1-1', strict=True) - @mock.patch('kojihub.get_maven_build') - @mock.patch('kojihub.get_win_build') - @mock.patch('kojihub.get_image_build') - @mock.patch('kojihub.get_build') - @mock.patch('kojihub.QueryProcessor') - def test_has_build(self, QueryProcessor, get_build, get_image_build, - get_win_build, get_maven_build): + def test_has_build(self): typeinfo = {'maven': {'maven': 'foo'}, 'win': {'win': 'foo'}, 'image': {'image': 'foo'}, 'new_type': {'bar': 42}} - binfo = {'id' : 1, 'extra' : {'typeinfo': {'new_type': typeinfo['new_type']}}} - get_build.return_value = binfo - get_maven_build.return_value = typeinfo['maven'] - get_win_build.return_value = typeinfo['win'] - get_image_build.return_value = typeinfo['image'] + binfo = {'id': 1, 'extra': {'typeinfo': {'new_type': typeinfo['new_type']}}} + self.get_build.return_value = binfo + self.get_maven_build.return_value = typeinfo['maven'] + self.get_win_build.return_value = typeinfo['win'] + self.get_image_build.return_value = typeinfo['image'] - query = QueryProcessor.return_value - query.execute.return_value = [['new_type']] + self.query_execute.return_value = [['new_type']] ret = kojihub.get_build_type('mytestbuild-1-1', strict=True) assert ret == typeinfo - get_build.assert_called_with('mytestbuild-1-1', strict=True) - get_maven_build.assert_called_with(binfo['id'], strict=False) - get_win_build.assert_called_with(binfo['id'], strict=False) - get_image_build.assert_called_with(binfo['id'], strict=False) + self.get_build.assert_called_with('mytestbuild-1-1', strict=True) + self.get_maven_build.assert_called_with(binfo['id'], strict=False) + self.get_win_build.assert_called_with(binfo['id'], strict=False) + self.get_image_build.assert_called_with(binfo['id'], strict=False)