From de6ce0e1a6209dede876057f74dc767ee9a5ed94 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:06 +0000 Subject: [PATCH 1/14] add name_or_id_clause function --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 489de5d..5177a1d 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3222,6 +3222,45 @@ def get_build_target(info, event=None, strict=False): return None +def name_or_id_clause(info, table=None): + """Return query clause and values for lookup by name or id + + :param info: the name or id to look up + :type info: int or str or dict + :param str table: table name + :returns: a pair (clause, values) + + If info is an int, we are looking up by id + If info is a string, we are looking up by name + If info is a dict, we look for 'id' or 'name' fields to decide + + If table is given, then the query string will include it in the field name + """ + if not table: + prefix1 = prefix2 = '' + else: + prefix1 = f'{table}.' + prefix2 = f'{table}_' + if isinstance(info, dict): + if 'id' in info: + try: + info = int(info['id']) + except (ValueError, TypeError): + raise koji.ParameterError('Invalid name or id value: %r' % info) + elif 'name' in info: + info = info['name'] + if isinstance(info, int): + clause = f"({prefix1}id = %({prefix2}id)s)" + values = {f"{prefix2}id": info} + elif isinstance(info, str): + clause = f"({prefix1}name = %({prefix2}name)s)" + values = {f"{prefix2}name": info} + else: + raise koji.ParameterError('Invalid name or id value: %r' % info) + + return clause, values + + def lookup_name(table, info, strict=False, create=False): """Find the id and name in the table associated with info. @@ -3240,26 +3279,24 @@ def lookup_name(table, info, strict=False, create=False): create option will fail. """ fields = ('id', 'name') - if isinstance(info, int): - q = """SELECT id,name FROM %s WHERE id=%%(info)d""" % table - elif isinstance(info, str): - q = """SELECT id,name FROM %s WHERE name=%%(info)s""" % table + clause, values = name_or_id_clause(info, table=table) + query = QueryProcessor(columns=fields, tables=[table], + clauses=[clause], values=values) + ret = query.executeOne() + if ret is not None: + return ret + elif strict: + raise koji.GenericError('No such entry in table %s: %s' % (table, info)) + elif create: + if not isinstance(info, str): + raise koji.GenericError('Name must be a string') + new_id = nextval(f'{table}_id_seq') + insert = InsertProcessor(table) + insert.set(id=new_id, name=info) + return {'id': new_id, 'name': info} else: - raise koji.GenericError('Invalid type for id lookup: %s' % type(info)) - ret = _singleRow(q, locals(), fields, strict=False) - if ret is None: - if strict: - raise koji.GenericError('No such entry in table %s: %s' % (table, info)) - elif create: - if not isinstance(info, str): - raise koji.GenericError('Name must be a string') - id = _singleValue("SELECT nextval('%s_id_seq')" % table, strict=True) - q = """INSERT INTO %s(id,name) VALUES (%%(id)i,%%(info)s)""" % table - _dml(q, locals()) - return {'id': id, 'name': info} - else: - return ret - return ret + # no match and not strict + return None def get_id(table, info, strict=False, create=False): From 35809b56a8bc77fa5186ef5c7c3cea45b2868788 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:27 +0000 Subject: [PATCH 2/14] fix unit tests --- diff --git a/tests/test_hub/test_list_builds.py b/tests/test_hub/test_list_builds.py index 1b53b7c..a74c524 100644 --- a/tests/test_hub/test_list_builds.py +++ b/tests/test_hub/test_list_builds.py @@ -41,9 +41,10 @@ class TestListBuilds(unittest.TestCase): 'volume_id': 0, 'volume_name': 'DEFAULT'}] - def test_wrong_package(self): + @mock.patch('kojihub.get_package_id') + def test_wrong_package(self, get_package_id): package = 'test-package' - kojihub.get_package_id.return_value = None + get_package_id.return_value = None rv = self.exports.listBuilds(packageID=package) self.assertEquals(rv, []) diff --git a/tests/test_hub/test_list_tags.py b/tests/test_hub/test_list_tags.py index a0f4f8d..1523144 100644 --- a/tests/test_hub/test_list_tags.py +++ b/tests/test_hub/test_list_tags.py @@ -27,22 +27,21 @@ class TestListTags(unittest.TestCase): self.exports.listTags(build=build_name) self.assertEqual("No such build: %s" % build_name, str(cm.exception)) - def test_non_exist_package(self): - package_id = 999 + @mock.patch('kojihub.lookup_package') + def test_non_exist_package(self, lookup_package): self.cursor.fetchone.return_value = None self.context.cnx.cursor.return_value = self.cursor - kojihub.lookup_package.return_value = koji.GenericError + lookup_package.side_effect = koji.GenericError('Expected error') + + package_id = 999 with self.assertRaises(koji.GenericError) as cm: self.exports.listTags(package=package_id) - self.assertEqual("No such package: %s" % package_id, str(cm.exception)) + self.assertEqual('Expected error', str(cm.exception)) package_name = 'test-pkg' - self.cursor.fetchone.return_value = None - self.context.cnx.cursor.return_value = self.cursor - kojihub.lookup_package.return_value = koji.GenericError with self.assertRaises(koji.GenericError) as cm: self.exports.listTags(package=package_name) - self.assertEqual("No such package: %s" % package_name, str(cm.exception)) + self.assertEqual("Expected error", str(cm.exception)) def test_build_package_not_none(self): build_id = 999 From e50f3135c0c1ccd959a63ee052f6d2941574c38c Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:27 +0000 Subject: [PATCH 3/14] use name_or_id_clause in get_tag --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 5177a1d..9b13c93 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3475,21 +3475,15 @@ def get_tag(tagInfo, strict=False, event=None, blocked=False): 'tag_config.maven_support': 'maven_support', 'tag_config.maven_include_all': 'maven_include_all', } - data = {'tagInfo': tagInfo} - clauses = [] - if isinstance(tagInfo, int): - clauses.append("tag.id = %(tagInfo)i") - elif isinstance(tagInfo, str): - clauses.append("tag.name = %(tagInfo)s") - else: - raise koji.GenericError('Invalid type for tagInfo: %s' % type(tagInfo)) + clause, values = name_or_id_clause(tagInfo, table='tag') + clauses = [clause] if event == "auto": # find active event or latest create_event opts = {'order': '-create_event', 'limit': 1} query = QueryProcessor(tables=['tag_config'], columns=['create_event', 'revoke_event'], joins=['tag on tag.id = tag_config.tag_id'], - clauses=clauses, values=data, opts=opts) + clauses=clauses, values=values, opts=opts) try: event = query.executeOne(strict=True)['revoke_event'] except koji.GenericError: @@ -3506,7 +3500,7 @@ def get_tag(tagInfo, strict=False, event=None, blocked=False): fields, aliases = zip(*fields.items()) query = QueryProcessor(columns=fields, aliases=aliases, tables=tables, - joins=joins, clauses=clauses, values=data) + joins=joins, clauses=clauses, values=values) result = query.executeOne() if not result: if strict: diff --git a/tests/test_hub/test_tag_operations.py b/tests/test_hub/test_tag_operations.py index 7019e71..d15de00 100644 --- a/tests/test_hub/test_tag_operations.py +++ b/tests/test_hub/test_tag_operations.py @@ -257,7 +257,7 @@ class TestGetTag(unittest.TestCase): taginfo = {'test-tag': 'value'} with self.assertRaises(koji.GenericError) as ex: kojihub.get_tag(taginfo, strict=True) - self.assertEqual("Invalid type for tagInfo: %s" % type(taginfo), str(ex.exception)) + self.assertEqual("Invalid name or id value: %s" % taginfo, str(ex.exception)) def test_get_tag_non_exist_tag(self): taginfo = 'test-tag' From a5b6e19e89d37cd932150fac9e7cfa666dd26add Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:27 +0000 Subject: [PATCH 4/14] use name_or_id_clause in get_host --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 9b13c93..2cdbdbf 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -5458,17 +5458,12 @@ def get_host(hostInfo, strict=False, event=None): } clauses = [eventCondition(event, table='host_config')] - if isinstance(hostInfo, int): - clauses.append("host.id = %(hostInfo)i") - elif isinstance(hostInfo, str): - clauses.append("host.name = %(hostInfo)s") - else: - raise koji.GenericError('Invalid type for hostInfo: %s' % type(hostInfo)) + clause, values = name_or_id_clause(hostInfo, table='host') + clauses.append(clause) - data = {'hostInfo': hostInfo} fields, aliases = zip(*fields.items()) query = QueryProcessor(columns=fields, aliases=aliases, tables=tables, - joins=joins, clauses=clauses, values=data) + joins=joins, clauses=clauses, values=values) result = query.executeOne() if not result: if strict: diff --git a/tests/test_hub/test_get_host.py b/tests/test_hub/test_get_host.py index 59ed087..0c9ba3a 100644 --- a/tests/test_hub/test_get_host.py +++ b/tests/test_hub/test_get_host.py @@ -40,13 +40,13 @@ class TestSetHostEnabled(unittest.TestCase): joins = ['host ON host.id = host_config.host_id'] aliases = ['id', 'user_id', 'name', 'ready', 'task_load', 'arches', 'capacity', 'description', 'comment', 'enabled'] - clauses = ['(host_config.active = TRUE)', 'host.name = %(hostInfo)s'] - values = {'hostInfo': 'hostname'} + clauses = ['(host_config.active = TRUE)', '(host.name = %(host_name)s)'] + values = {'host_name': 'hostname'} self.assertEqual(query.tables, ['host_config']) self.assertEqual(query.joins, joins) self.assertEqual(set(query.columns), set(columns)) self.assertEqual(set(query.aliases), set(aliases)) - self.assertEqual(query.clauses, clauses) + self.assertEqual(set(query.clauses), set(clauses)) self.assertEqual(query.values, values) def test_get_host_by_id_event(self): @@ -63,13 +63,13 @@ class TestSetHostEnabled(unittest.TestCase): 'arches', 'capacity', 'description', 'comment', 'enabled'] clauses = ['(host_config.create_event <= 345 AND ( host_config.revoke_event IS NULL ' 'OR 345 < host_config.revoke_event ))', - 'host.id = %(hostInfo)i'] - values = {'hostInfo': 123} + '(host.id = %(host_id)s)'] + values = {'host_id': 123} self.assertEqual(query.tables, ['host_config']) self.assertEqual(query.joins, joins) self.assertEqual(set(query.columns), set(columns)) self.assertEqual(set(query.aliases), set(aliases)) - self.assertEqual(query.clauses, clauses) + self.assertEqual(set(query.clauses), set(clauses)) self.assertEqual(query.values, values) def getQueryMissing(self, *args, **kwargs): @@ -95,5 +95,5 @@ class TestSetHostEnabled(unittest.TestCase): host_info = {'host_id': 567} with self.assertRaises(koji.GenericError) as cm: self.exports.getHost(host_info) - self.assertEqual("Invalid type for hostInfo: %s" % type(host_info), str(cm.exception)) + self.assertEqual("Invalid name or id value: %s" % host_info, str(cm.exception)) self.assertEqual(len(self.queries), 0) From 8c89b2c95a979cec5a7c5a7844add51ead519b7a Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:27 +0000 Subject: [PATCH 5/14] use name_or_id_clause in get_build_targets --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 2cdbdbf..47ff7b7 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3191,22 +3191,22 @@ def get_build_targets(info=None, event=None, buildTagID=None, destTagID=None, qu 'tag AS tag1 ON build_target_config.build_tag = tag1.id', 'tag AS tag2 ON build_target_config.dest_tag = tag2.id'] clauses = [eventCondition(event)] + values = {} if info: - if isinstance(info, str): - clauses.append('build_target.name = %(info)s') - elif isinstance(info, int): - clauses.append('build_target.id = %(info)i') - else: - raise koji.GenericError('Invalid type for lookup: %s' % type(info)) + clause, c_values = name_or_id_clause(info, table='tag') + clauses.append(clause) + values.update(c_values) if buildTagID is not None: clauses.append('build_tag = %(buildTagID)i') + values['buildTagID'] = buildTagID if destTagID is not None: clauses.append('dest_tag = %(destTagID)i') + values['destTagID'] = destTagID query = QueryProcessor(columns=[f[0] for f in fields], aliases=[f[1] for f in fields], tables=['build_target_config'], joins=joins, clauses=clauses, - values=locals(), opts=queryOpts) + values=values, opts=queryOpts) return query.execute() From dd7867ecd431afda67a3c7ef35843a88c6dc03c9 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:27 +0000 Subject: [PATCH 6/14] use name_or_id_clause in get_external_repos --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 47ff7b7..d248270 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3768,19 +3768,18 @@ def get_external_repos(info=None, url=None, event=None, queryOpts=None): tables = ['external_repo'] joins = ['external_repo_config ON external_repo_id = id'] clauses = [eventCondition(event)] + values = {} if info is not None: - if isinstance(info, str): - clauses.append('name = %(info)s') - elif isinstance(info, int): - clauses.append('id = %(info)i') - else: - raise koji.GenericError('Invalid type for lookup: %s' % type(info)) + clause, c_values = name_or_id_clause(info, table='external_repo') + clauses.append(clause) + values.update(c_values) if url: clauses.append('url = %(url)s') + values['url'] = url query = QueryProcessor(columns=fields, tables=tables, joins=joins, clauses=clauses, - values=locals(), opts=queryOpts) + values=values, opts=queryOpts) return query.execute() diff --git a/tests/test_hub/test_get_external_repos.py b/tests/test_hub/test_get_external_repos.py index 27e12b8..25f09fa 100644 --- a/tests/test_hub/test_get_external_repos.py +++ b/tests/test_hub/test_get_external_repos.py @@ -17,15 +17,7 @@ class TestGetExternalRepos(DBQueryTestCase): joins=[ 'external_repo_config ON external_repo_id = id'], clauses=['(active = TRUE)'], - values={'clauses': ['(active = TRUE)'], - 'event': None, - 'fields': ['id', 'name', 'url'], - 'info': None, - 'joins': [ - 'external_repo_config ON external_repo_id = id'], - 'queryOpts': None, - 'tables': ['external_repo'], - 'url': None}, + values={}, opts={}) self.assertEqual(rv, [{'id': 1, 'name': 'ext_repo_1', @@ -46,19 +38,7 @@ class TestGetExternalRepos(DBQueryTestCase): '(create_event <= 1000' ' AND ( revoke_event IS NULL' ' OR 1000 < revoke_event ))'], - values={'clauses': [ - '(create_event <= 1000' - ' AND ( revoke_event IS NULL' - ' OR 1000 < revoke_event ))'], - 'event': 1000, - 'fields': ['id', 'name', 'url'], - 'info': None, - 'joins': [ - 'external_repo_config ON' - ' external_repo_id = id'], - 'queryOpts': None, - 'tables': ['external_repo'], - 'url': None}, + values={}, opts={}) self.assertEqual(rv, [{'id': 1, 'name': 'ext_repo_1', @@ -75,17 +55,8 @@ class TestGetExternalRepos(DBQueryTestCase): joins=[ 'external_repo_config ON external_repo_id = id'], clauses=['(active = TRUE)', - 'name = %(info)s'], - values={'clauses': ['(active = TRUE)', - 'name = %(info)s'], - 'event': None, - 'fields': ['id', 'name', 'url'], - 'info': 'ext_repo_1', - 'joins': [ - 'external_repo_config ON external_repo_id = id'], - 'queryOpts': None, - 'tables': ['external_repo'], - 'url': None}, + '(external_repo.name = %(external_repo_name)s)'], + values={'external_repo_name': 'ext_repo_1'}, opts={}) self.assertEqual(rv, [{'id': 1, 'name': 'ext_repo_1', @@ -102,17 +73,8 @@ class TestGetExternalRepos(DBQueryTestCase): joins=[ 'external_repo_config ON external_repo_id = id'], clauses=['(active = TRUE)', - 'id = %(info)i'], - values={'clauses': ['(active = TRUE)', - 'id = %(info)i'], - 'event': None, - 'fields': ['id', 'name', 'url'], - 'info': 1, - 'joins': [ - 'external_repo_config ON external_repo_id = id'], - 'queryOpts': None, - 'tables': ['external_repo'], - 'url': None}, + '(external_repo.id = %(external_repo_id)s)'], + values={'external_repo_id': 1}, opts={}) self.assertEqual(rv, [{'id': 1, 'name': 'ext_repo_1', @@ -130,16 +92,7 @@ class TestGetExternalRepos(DBQueryTestCase): 'external_repo_config ON external_repo_id = id'], clauses=['(active = TRUE)', 'url = %(url)s'], - values={'clauses': ['(active = TRUE)', - 'url = %(url)s'], - 'event': None, - 'fields': ['id', 'name', 'url'], - 'info': None, - 'joins': [ - 'external_repo_config ON external_repo_id = id'], - 'queryOpts': None, - 'tables': ['external_repo'], - 'url': 'http://example.com/repo/'}, + values={'url': 'http://example.com/repo/'}, opts={}) self.assertEqual(rv, [{'id': 1, 'name': 'ext_repo_1', @@ -149,4 +102,4 @@ class TestGetExternalRepos(DBQueryTestCase): info = {'info_key': 'info_value'} with self.assertRaises(koji.GenericError) as cm: kojihub.get_external_repos(info=info) - self.assertEqual("Invalid type for lookup: %s" % type(info), str(cm.exception)) + self.assertEqual("Invalid name or id value: %s" % info, str(cm.exception)) From fbe83e6b345feb9603c3b600af5c78fb2b634b23 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:27 +0000 Subject: [PATCH 7/14] use name_or_id_clause in get_channel --- diff --git a/hub/kojihub.py b/hub/kojihub.py index d248270..c485cf0 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -5527,16 +5527,10 @@ def get_channel(channelInfo, strict=False): For example, {'id': 20, 'name': 'container'} """ fields = ('id', 'name', 'description', 'enabled', 'comment') - query = """SELECT %s FROM channels - WHERE """ % ', '.join(fields) - if isinstance(channelInfo, int): - query += """id = %(channelInfo)i""" - elif isinstance(channelInfo, str): - query += """name = %(channelInfo)s""" - else: - raise koji.GenericError('Invalid type for channelInfo: %s' % type(channelInfo)) - - return _singleRow(query, locals(), fields, strict) + clause, values = name_or_id_clause(channelInfo, table='channels') + query = QueryProcessor(columns=fields, tables=['channels'], + clauses=[clause], values=values) + return query.executeOne(strict=strict) def query_buildroots(hostID=None, tagID=None, state=None, rpmID=None, archiveID=None, taskID=None, diff --git a/tests/test_hub/test_get_channel.py b/tests/test_hub/test_get_channel.py index 9b5e766..23cebf9 100644 --- a/tests/test_hub/test_get_channel.py +++ b/tests/test_hub/test_get_channel.py @@ -20,12 +20,12 @@ class TestGetChannel(unittest.TestCase): channel_info = {'channel': 'val'} with self.assertRaises(koji.GenericError) as cm: self.exports.getChannel(channel_info) - self.assertEqual('Invalid type for channelInfo: %s' % type(channel_info), + self.assertEqual('Invalid name or id value: %s' % channel_info, str(cm.exception)) # list channel_info = ['channel'] with self.assertRaises(koji.GenericError) as cm: self.exports.getChannel(channel_info) - self.assertEqual('Invalid type for channelInfo: %s' % type(channel_info), + self.assertEqual('Invalid name or id value: %s' % channel_info, str(cm.exception)) From c049b832c4912fe25d22718a46170d287efc21a9 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:27 +0000 Subject: [PATCH 8/14] expand get_channel unit tests --- diff --git a/tests/test_hub/test_get_channel.py b/tests/test_hub/test_get_channel.py index 23cebf9..880af65 100644 --- a/tests/test_hub/test_get_channel.py +++ b/tests/test_hub/test_get_channel.py @@ -6,9 +6,22 @@ import koji import kojihub +QP = kojihub.QueryProcessor + + class TestGetChannel(unittest.TestCase): + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = mock.MagicMock() + query.executeOne = mock.MagicMock() + self.queries.append(query) + return query + def setUp(self): + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] self.context = mock.patch('kojihub.context').start() self.exports = kojihub.RootExports() @@ -29,3 +42,37 @@ class TestGetChannel(unittest.TestCase): self.exports.getChannel(channel_info) self.assertEqual('Invalid name or id value: %s' % channel_info, str(cm.exception)) + + def test_query_by_name(self): + self.exports.getChannel('my_channel') + self.assertEqual(len(self.queries), 1) + query = self.queries[0] + clauses = ['(channels.name = %(channels_name)s)'] + values = {'channels_name': 'my_channel'} + self.assertEqual(query.tables, ['channels']) + self.assertEqual(query.joins, None) + self.assertEqual(set(query.clauses), set(clauses)) + self.assertEqual(query.values, values) + + + def test_query_by_id(self): + self.exports.getChannel(12345) + self.assertEqual(len(self.queries), 1) + query = self.queries[0] + clauses = ['(channels.id = %(channels_id)s)'] + values = {'channels_id': 12345} + self.assertEqual(query.tables, ['channels']) + self.assertEqual(query.joins, None) + self.assertEqual(set(query.clauses), set(clauses)) + self.assertEqual(query.values, values) + + def test_query_by_dict(self): + self.exports.getChannel({'id':12345, 'name': 'whatever'}) + self.assertEqual(len(self.queries), 1) + query = self.queries[0] + clauses = ['(channels.id = %(channels_id)s)'] + values = {'channels_id': 12345} + self.assertEqual(query.tables, ['channels']) + self.assertEqual(query.joins, None) + self.assertEqual(set(query.clauses), set(clauses)) + self.assertEqual(query.values, values) From b764e1d8056428be88f0c8a41b964341cfea4ad5 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:27 +0000 Subject: [PATCH 9/14] unit tests for lookup_name --- diff --git a/tests/test_hub/test_lookup_name.py b/tests/test_hub/test_lookup_name.py new file mode 100644 index 0000000..89b3753 --- /dev/null +++ b/tests/test_hub/test_lookup_name.py @@ -0,0 +1,131 @@ +import unittest + +import mock + +import koji +import kojihub + + +QP = kojihub.QueryProcessor +IP = kojihub.InsertProcessor + + +class TestLookupName(unittest.TestCase): + + def setUp(self): + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + self.query_executeOne = mock.MagicMock() + self.InsertProcessor = mock.patch('kojihub.InsertProcessor', + side_effect=self.getInsert).start() + self.inserts = [] + self.insert_execute = mock.MagicMock() + self.nextval = mock.patch('kojihub.nextval').start() + self.context = mock.patch('kojihub.context').start() + + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.executeOne = self.query_executeOne + self.queries.append(query) + return query + + def getInsert(self, *args, **kwargs): + insert = IP(*args, **kwargs) + insert.execute = self.insert_execute + self.inserts.append(insert) + return insert + + def tearDown(self): + mock.patch.stopall() + + def test_wrong_lookup_type(self): + bad_values = [ + {'foo': 'missing id and name fields'}, + ['something'], + set(), + ] + for value in bad_values: + with self.assertRaises(koji.GenericError) as cm: + kojihub.lookup_name('mytable', value) + self.assertEqual('Invalid name or id value: %s' % value, + str(cm.exception)) + self.assertEqual(len(self.queries), 0) + self.assertEqual(len(self.inserts), 0) + + def test_query_by_name(self): + kojihub.lookup_name('some_table', 'herbert') + self.assertEqual(len(self.queries), 1) + query = self.queries[0] + clauses = ['(some_table.name = %(some_table_name)s)'] + values = {'some_table_name': 'herbert'} + self.assertEqual(query.tables, ['some_table']) + self.assertEqual(query.joins, None) + self.assertEqual(set(query.clauses), set(clauses)) + self.assertEqual(query.values, values) + self.assertEqual(len(self.inserts), 0) + + + def test_query_by_id(self): + kojihub.lookup_name('some_table', 12345) + self.assertEqual(len(self.queries), 1) + query = self.queries[0] + clauses = ['(some_table.id = %(some_table_id)s)'] + values = {'some_table_id': 12345} + self.assertEqual(query.tables, ['some_table']) + self.assertEqual(query.joins, None) + self.assertEqual(set(query.clauses), set(clauses)) + self.assertEqual(query.values, values) + self.assertEqual(len(self.inserts), 0) + + def test_query_by_dict(self): + kojihub.lookup_name('some_table', {'id':12345, 'name': 'whatever'}) + self.assertEqual(len(self.queries), 1) + query = self.queries[0] + clauses = ['(some_table.id = %(some_table_id)s)'] + values = {'some_table_id': 12345} + self.assertEqual(query.tables, ['some_table']) + self.assertEqual(query.joins, None) + self.assertEqual(set(query.clauses), set(clauses)) + self.assertEqual(query.values, values) + self.assertEqual(len(self.inserts), 0) + + def test_lookup_name_no_match(self): + self.query_executeOne.return_value = None + result = kojihub.lookup_name('package', 'python') + self.assertEqual(len(self.queries), 1) + self.assertEqual(len(self.inserts), 0) + self.assertEqual(result, None) + + def test_lookup_name_strict(self): + self.query_executeOne.return_value = None + with self.assertRaises(koji.GenericError) as cm: + kojihub.lookup_name('package', 'python', strict=True) + self.assertEqual(len(self.queries), 1) + self.assertEqual(len(self.inserts), 0) + + def test_lookup_name_create(self): + self.query_executeOne.return_value = None + self.nextval.return_value = 999 + result = kojihub.lookup_name('package', 'python', create=True) + self.assertEqual(len(self.queries), 1) + self.assertEqual(len(self.inserts), 1) + expected = {'id': 999, 'name': 'python'} + self.assertEqual(result, expected) + insert = self.inserts[0] + self.assertEqual(insert.table, 'package') + self.assertEqual(insert.data, expected) + self.assertEqual(insert.rawdata, {}) + + def test_lookup_name_create_wrong_type(self): + self.query_executeOne.return_value = None + bad_values = [ + {'id': 100}, + 100 + ] + for value in bad_values: + with self.assertRaises(koji.GenericError) as cm: + kojihub.lookup_name('package', value, create=True) + self.assertEqual('Name must be a string', str(cm.exception)) + self.assertEqual(len(self.inserts), 0) + self.nextval.assert_not_called() From a7dd35b727bf8a4ad7b9217ca9ca2d9f2e3dd445 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:27 +0000 Subject: [PATCH 10/14] another unit test --- diff --git a/tests/test_hub/test_lookup_name.py b/tests/test_hub/test_lookup_name.py index 89b3753..8b630d4 100644 --- a/tests/test_hub/test_lookup_name.py +++ b/tests/test_hub/test_lookup_name.py @@ -42,6 +42,7 @@ class TestLookupName(unittest.TestCase): def test_wrong_lookup_type(self): bad_values = [ {'foo': 'missing id and name fields'}, + {'id': 'not a valid int'}, ['something'], set(), ] @@ -65,7 +66,6 @@ class TestLookupName(unittest.TestCase): self.assertEqual(query.values, values) self.assertEqual(len(self.inserts), 0) - def test_query_by_id(self): kojihub.lookup_name('some_table', 12345) self.assertEqual(len(self.queries), 1) @@ -90,6 +90,18 @@ class TestLookupName(unittest.TestCase): self.assertEqual(query.values, values) self.assertEqual(len(self.inserts), 0) + def test_query_by_dict_with_name(self): + kojihub.lookup_name('some_table', {'name': 'whatever'}) + self.assertEqual(len(self.queries), 1) + query = self.queries[0] + clauses = ['(some_table.name = %(some_table_name)s)'] + values = {'some_table_name': 'whatever'} + self.assertEqual(query.tables, ['some_table']) + self.assertEqual(query.joins, None) + self.assertEqual(set(query.clauses), set(clauses)) + self.assertEqual(query.values, values) + self.assertEqual(len(self.inserts), 0) + def test_lookup_name_no_match(self): self.query_executeOne.return_value = None result = kojihub.lookup_name('package', 'python') From 91232f786bb4a943eeafee3352b68f95006cfb6d Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 10 2021 20:01:27 +0000 Subject: [PATCH 11/14] require table arg in name_or_id_clause() to simplify code --- diff --git a/hub/kojihub.py b/hub/kojihub.py index c485cf0..7a871af 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3194,7 +3194,7 @@ def get_build_targets(info=None, event=None, buildTagID=None, destTagID=None, qu values = {} if info: - clause, c_values = name_or_id_clause(info, table='tag') + clause, c_values = name_or_id_clause('tag', info) clauses.append(clause) values.update(c_values) if buildTagID is not None: @@ -3222,25 +3222,18 @@ def get_build_target(info, event=None, strict=False): return None -def name_or_id_clause(info, table=None): +def name_or_id_clause(table, info): """Return query clause and values for lookup by name or id + :param str table: table name :param info: the name or id to look up :type info: int or str or dict - :param str table: table name :returns: a pair (clause, values) If info is an int, we are looking up by id If info is a string, we are looking up by name If info is a dict, we look for 'id' or 'name' fields to decide - - If table is given, then the query string will include it in the field name """ - if not table: - prefix1 = prefix2 = '' - else: - prefix1 = f'{table}.' - prefix2 = f'{table}_' if isinstance(info, dict): if 'id' in info: try: @@ -3250,11 +3243,11 @@ def name_or_id_clause(info, table=None): elif 'name' in info: info = info['name'] if isinstance(info, int): - clause = f"({prefix1}id = %({prefix2}id)s)" - values = {f"{prefix2}id": info} + clause = f"({table}.id = %({table}_id)s)" + values = {f"{table}_id": info} elif isinstance(info, str): - clause = f"({prefix1}name = %({prefix2}name)s)" - values = {f"{prefix2}name": info} + clause = f"({table}.name = %({table}_name)s)" + values = {f"{table}_name": info} else: raise koji.ParameterError('Invalid name or id value: %r' % info) @@ -3279,7 +3272,7 @@ def lookup_name(table, info, strict=False, create=False): create option will fail. """ fields = ('id', 'name') - clause, values = name_or_id_clause(info, table=table) + clause, values = name_or_id_clause(table, info) query = QueryProcessor(columns=fields, tables=[table], clauses=[clause], values=values) ret = query.executeOne() @@ -3476,7 +3469,7 @@ def get_tag(tagInfo, strict=False, event=None, blocked=False): 'tag_config.maven_include_all': 'maven_include_all', } - clause, values = name_or_id_clause(tagInfo, table='tag') + clause, values = name_or_id_clause('tag', tagInfo) clauses = [clause] if event == "auto": # find active event or latest create_event @@ -3770,7 +3763,7 @@ def get_external_repos(info=None, url=None, event=None, queryOpts=None): clauses = [eventCondition(event)] values = {} if info is not None: - clause, c_values = name_or_id_clause(info, table='external_repo') + clause, c_values = name_or_id_clause('external_repo', info) clauses.append(clause) values.update(c_values) if url: @@ -5457,7 +5450,7 @@ def get_host(hostInfo, strict=False, event=None): } clauses = [eventCondition(event, table='host_config')] - clause, values = name_or_id_clause(hostInfo, table='host') + clause, values = name_or_id_clause('host', hostInfo) clauses.append(clause) fields, aliases = zip(*fields.items()) @@ -5527,7 +5520,7 @@ def get_channel(channelInfo, strict=False): For example, {'id': 20, 'name': 'container'} """ fields = ('id', 'name', 'description', 'enabled', 'comment') - clause, values = name_or_id_clause(channelInfo, table='channels') + clause, values = name_or_id_clause('channels', channelInfo) query = QueryProcessor(columns=fields, tables=['channels'], clauses=[clause], values=values) return query.executeOne(strict=strict) From 5acdb1e4d8136e20dcb35bcc6840d1bde4067a3e Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Dec 08 2021 15:54:48 +0000 Subject: [PATCH 12/14] fix table name --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 7a871af..0adc2ad 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3194,7 +3194,7 @@ def get_build_targets(info=None, event=None, buildTagID=None, destTagID=None, qu values = {} if info: - clause, c_values = name_or_id_clause('tag', info) + clause, c_values = name_or_id_clause('build_target', info) clauses.append(clause) values.update(c_values) if buildTagID is not None: From 9b499d1bc9b594524ff5840bbd5febb3b5736f03 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Dec 08 2021 16:07:55 +0000 Subject: [PATCH 13/14] add explicit error case --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 0adc2ad..8b18516 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3242,6 +3242,8 @@ def name_or_id_clause(table, info): raise koji.ParameterError('Invalid name or id value: %r' % info) elif 'name' in info: info = info['name'] + else: + raise koji.ParameterError('Invalid name or id value: %r' % info) if isinstance(info, int): clause = f"({table}.id = %({table}_id)s)" values = {f"{table}_id": info} From f22abd179212ef6fda377afa23287417bc04b969 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Dec 14 2021 19:31:27 +0000 Subject: [PATCH 14/14] actually execute the insert --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 8b18516..ea63e1c 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3288,6 +3288,7 @@ def lookup_name(table, info, strict=False, create=False): new_id = nextval(f'{table}_id_seq') insert = InsertProcessor(table) insert.set(id=new_id, name=info) + insert.execute() return {'id': new_id, 'name': info} else: # no match and not strict diff --git a/tests/test_hub/test_lookup_name.py b/tests/test_hub/test_lookup_name.py index 8b630d4..42346fa 100644 --- a/tests/test_hub/test_lookup_name.py +++ b/tests/test_hub/test_lookup_name.py @@ -20,7 +20,6 @@ class TestLookupName(unittest.TestCase): self.InsertProcessor = mock.patch('kojihub.InsertProcessor', side_effect=self.getInsert).start() self.inserts = [] - self.insert_execute = mock.MagicMock() self.nextval = mock.patch('kojihub.nextval').start() self.context = mock.patch('kojihub.context').start() @@ -32,7 +31,7 @@ class TestLookupName(unittest.TestCase): def getInsert(self, *args, **kwargs): insert = IP(*args, **kwargs) - insert.execute = self.insert_execute + insert.execute = mock.MagicMock() self.inserts.append(insert) return insert @@ -128,6 +127,7 @@ class TestLookupName(unittest.TestCase): self.assertEqual(insert.table, 'package') self.assertEqual(insert.data, expected) self.assertEqual(insert.rawdata, {}) + insert.execute.assert_called_once() def test_lookup_name_create_wrong_type(self): self.query_executeOne.return_value = None