From 80655cb362aacb3e2e4a1d49c396cccfaf0b9c3c Mon Sep 17 00:00:00 2001 From: Feodor Tersin Date: Mon, 13 Apr 2015 11:09:35 +0300 Subject: [PATCH] Fix and rename get_item_ids 1 Rename to get_items_ids 2 Fix a bug of filtering 3 Add a test on it 4 Add a test on ec2utils.os_id_to_ec2_id Change-Id: I0ae4b0d2cd937ffa810f7cea3d818a72e3fac845 --- ec2api/api/ec2utils.py | 2 +- ec2api/db/api.py | 4 +- ec2api/db/sqlalchemy/api.py | 4 +- ec2api/tests/unit/base.py | 4 +- ec2api/tests/unit/test_db_api.py | 21 ++++++-- ec2api/tests/unit/test_ec2utils.py | 74 ++++++++++++++++++++++++++ ec2api/tests/unit/test_metadata.py | 2 +- ec2api/tests/unit/test_metadata_api.py | 2 +- ec2api/tests/unit/tools.py | 8 +-- 9 files changed, 105 insertions(+), 16 deletions(-) diff --git a/ec2api/api/ec2utils.py b/ec2api/api/ec2utils.py index 0c1b2c49..a98b0c05 100644 --- a/ec2api/api/ec2utils.py +++ b/ec2api/api/ec2utils.py @@ -282,7 +282,7 @@ def os_id_to_ec2_id(context, kind, os_id, items_by_os_id=None, item = items_by_os_id.get(os_id) if item: return item['id'] - ids = db_api.get_item_ids(context, kind, (os_id,)) + ids = db_api.get_items_ids(context, kind, (os_id,)) if len(ids): item_id, _os_id = ids[0] else: diff --git a/ec2api/db/api.py b/ec2api/db/api.py index 34d87343..8fca882b 100644 --- a/ec2api/db/api.py +++ b/ec2api/db/api.py @@ -115,8 +115,8 @@ def get_public_items(context, kind, item_ids=None): return IMPL.get_public_items(context, kind, item_ids) -def get_item_ids(context, kind, os_ids): - return IMPL.get_item_ids(context, kind, os_ids) +def get_items_ids(context, kind, os_ids): + return IMPL.get_items_ids(context, kind, os_ids) def add_tags(context, tags): diff --git a/ec2api/db/sqlalchemy/api.py b/ec2api/db/sqlalchemy/api.py index 631550dc..329e0393 100644 --- a/ec2api/db/sqlalchemy/api.py +++ b/ec2api/db/sqlalchemy/api.py @@ -219,11 +219,11 @@ def get_public_items(context, kind, item_ids=None): @require_context -def get_item_ids(context, kind, os_ids): +def get_items_ids(context, kind, os_ids): query = (model_query(context, models.Item). filter(models.Item.id.like('%s-%%' % kind))) if os_ids: - query = query.filter(models.Item.id.in_(os_ids)) + query = query.filter(models.Item.os_id.in_(os_ids)) return [(item['id'], item['os_id']) for item in query.all()] diff --git a/ec2api/tests/unit/base.py b/ec2api/tests/unit/base.py index 687350d5..f10e37e8 100644 --- a/ec2api/tests/unit/base.py +++ b/ec2api/tests/unit/base.py @@ -110,8 +110,8 @@ class ApiTestCase(test_base.BaseTestCase): tools.get_db_api_get_item_by_id(*self._db_items)) self.db_api.get_items_by_ids.side_effect = ( tools.get_db_api_get_items_by_ids(*self._db_items)) - self.db_api.get_item_ids.side_effect = ( - tools.get_db_api_get_item_ids(*self._db_items)) + self.db_api.get_items_ids.side_effect = ( + tools.get_db_api_get_items_ids(*self._db_items)) def add_mock_db_items(self, *items): merged_items = items + tuple(item for item in self._db_items diff --git a/ec2api/tests/unit/test_db_api.py b/ec2api/tests/unit/test_db_api.py index 7445658b..cc9537d7 100644 --- a/ec2api/tests/unit/test_db_api.py +++ b/ec2api/tests/unit/test_db_api.py @@ -187,12 +187,13 @@ class DbApiTestCase(test_base.BaseTestCase): def _setup_items(self): db_api.add_item(self.context, 'fake', {}) db_api.add_item(self.context, 'fake', {'is_public': True}) - db_api.add_item(self.context, 'fake1', {}) + db_api.add_item(self.context, 'fake1', {'os_id': fakes.random_os_id()}) db_api.add_item(self.other_context, 'fake', {}) db_api.add_item(self.other_context, 'fake', {'is_public': False}) db_api.add_item(self.other_context, 'fake', {'is_public': True}) - db_api.add_item_id(self.other_context, 'fake', fakes.random_os_id()) - db_api.add_item(self.other_context, 'fake1', {'is_public': True}) + db_api.add_item(self.other_context, 'fake1', + {'is_public': False, + 'os_id': fakes.random_os_id()}) def test_get_items(self): self._setup_items() @@ -249,6 +250,20 @@ class DbApiTestCase(test_base.BaseTestCase): (item_id, fakes.random_ec2_id('fake'))) self.assertEqual(1, len(items)) + def test_get_items_ids(self): + self._setup_items() + item = db_api.get_items(self.context, 'fake1')[0] + other_item = db_api.get_items(self.other_context, 'fake1')[0] + items_ids = db_api.get_items_ids(self.context, 'fake1', + [item['os_id'], other_item['os_id']]) + self.assertThat(items_ids, + matchers.ListMatches( + [(item['id'], item['os_id']), + (other_item['id'], other_item['os_id'])], + orderless_lists=True)) + items_ids = db_api.get_items_ids(self.context, 'fake', [item['os_id']]) + self.assertEqual(0, len(items_ids)) + def test_get_public_items(self): self._setup_items() items = db_api.get_public_items(self.context, 'fake') diff --git a/ec2api/tests/unit/test_ec2utils.py b/ec2api/tests/unit/test_ec2utils.py index 3814ca5e..3d959e6c 100644 --- a/ec2api/tests/unit/test_ec2utils.py +++ b/ec2api/tests/unit/test_ec2utils.py @@ -149,6 +149,80 @@ class EC2UtilsTestCase(testtools.TestCase): self.assertEqual(conv('remove'), 'remove') self.assertEqual(conv(''), '') + @mock.patch('ec2api.db.api.IMPL') + def test_os_id_to_ec2_id(self, db_api): + fake_context = mock.Mock(service_catalog=[{'type': 'fake'}]) + fake_id = fakes.random_ec2_id('fake') + fake_os_id = fakes.random_os_id() + + # no cache, item is found + db_api.get_items_ids.return_value = [(fake_id, fake_os_id)] + item_id = ec2utils.os_id_to_ec2_id(fake_context, 'fake', fake_os_id) + self.assertEqual(fake_id, item_id) + db_api.get_items_ids.assert_called_once_with( + fake_context, 'fake', (fake_os_id,)) + self.assertFalse(db_api.add_item_id.called) + + # no cache, item isn't found + db_api.get_items_ids.return_value = [] + db_api.add_item_id.return_value = fake_id + item_id = ec2utils.os_id_to_ec2_id(fake_context, 'fake', fake_os_id) + self.assertEqual(fake_id, item_id) + db_api.add_item_id.assert_called_once_with( + fake_context, 'fake', fake_os_id) + + # no item in cache, item isn't found + db_api.reset_mock() + ids_cache = {fakes.random_os_id(): fakes.random_ec2_id('fake')} + item_id = ec2utils.os_id_to_ec2_id(fake_context, 'fake', fake_os_id, + ids_by_os_id=ids_cache) + self.assertEqual(fake_id, item_id) + self.assertIn(fake_os_id, ids_cache) + self.assertEqual(fake_id, ids_cache[fake_os_id]) + db_api.add_item_id.assert_called_once_with( + fake_context, 'fake', fake_os_id) + + # no item in cache, item is found + db_api.reset_mock() + db_api.get_items_ids.return_value = [(fake_id, fake_os_id)] + ids_cache = {} + item_id = ec2utils.os_id_to_ec2_id(fake_context, 'fake', fake_os_id, + ids_by_os_id=ids_cache) + self.assertEqual(fake_id, item_id) + self.assertEqual({fake_os_id: fake_id}, ids_cache) + self.assertFalse(db_api.add_item_id.called) + + # item in cache + db_api.reset_mock() + ids_cache = {fake_os_id: fake_id} + item_id = ec2utils.os_id_to_ec2_id(fake_context, 'fake', fake_os_id, + ids_by_os_id=ids_cache) + self.assertEqual(fake_id, item_id) + self.assertEqual({fake_os_id: fake_id}, ids_cache) + self.assertFalse(db_api.get_items_ids.called) + self.assertFalse(db_api.add_item_id.called) + + # item in items dict + items_dict = {fake_os_id: {'id': fake_id, + 'os_id': fake_os_id}} + ids_cache = {} + item_id = ec2utils.os_id_to_ec2_id(fake_context, 'fake', fake_os_id, + items_by_os_id=items_dict, + ids_by_os_id=ids_cache) + self.assertEqual(fake_id, item_id) + self.assertFalse(db_api.get_items_ids.called) + self.assertFalse(db_api.add_item_id.called) + self.assertEqual({}, ids_cache) + + # item not in items dict, item is found + items_dict = {fake_os_id: {'id': fake_id, + 'os_id': fake_os_id}} + db_api.get_items_ids.return_value = [(fake_id, fake_os_id)] + item_id = ec2utils.os_id_to_ec2_id(fake_context, 'fake', fake_os_id, + items_by_os_id=items_dict) + self.assertEqual(fake_id, item_id) + self.assertFalse(db_api.add_item_id.called) + @mock.patch('glanceclient.client.Client') @mock.patch('ec2api.db.api.IMPL') def test_get_os_image(self, db_api, glance): diff --git a/ec2api/tests/unit/test_metadata.py b/ec2api/tests/unit/test_metadata.py index f87cfe9f..6eb2f089 100644 --- a/ec2api/tests/unit/test_metadata.py +++ b/ec2api/tests/unit/test_metadata.py @@ -350,7 +350,7 @@ class ProxyTestCase(test_base.BaseTestCase): keypair = mock.Mock(public_key=fakes.PUBLIC_KEY_KEY_PAIR) keypair.configure_mock(name=fakes.NAME_KEY_PAIR) nova.return_value.keypairs.get.return_value = keypair - db_api.get_item_ids.return_value = [ + db_api.get_items_ids.return_value = [ (fakes.ID_EC2_INSTANCE_1, fakes.ID_OS_INSTANCE_1)] instance_api.describe_instances.return_value = { 'reservationSet': [fakes.EC2_RESERVATION_1]} diff --git a/ec2api/tests/unit/test_metadata_api.py b/ec2api/tests/unit/test_metadata_api.py index 6391530b..9f021f1a 100644 --- a/ec2api/tests/unit/test_metadata_api.py +++ b/ec2api/tests/unit/test_metadata_api.py @@ -91,7 +91,7 @@ class MetadataApiTestCase(base.ApiTestCase): api.get_metadata_item, self.fake_context, ['9999-99-99'], fakes.ID_OS_INSTANCE_1, fakes.IP_NETWORK_INTERFACE_2) - self.db_api.get_item_ids.assert_called_with( + self.db_api.get_items_ids.assert_called_with( self.fake_context, 'i', (fakes.ID_OS_INSTANCE_1,)) self.instance_api.describe_instances.assert_called_with( self.fake_context, [fakes.ID_EC2_INSTANCE_1]) diff --git a/ec2api/tests/unit/tools.py b/ec2api/tests/unit/tools.py index dab7fdd9..0a899404 100644 --- a/ec2api/tests/unit/tools.py +++ b/ec2api/tests/unit/tools.py @@ -92,15 +92,15 @@ def get_db_api_get_items_by_ids(*items): return db_api_get_items_by_ids -def get_db_api_get_item_ids(*items): - """Generate db_api.get_item_ids mock function.""" +def get_db_api_get_items_ids(*items): + """Generate db_api.get_items_ids mock function.""" - def db_api_get_item_ids(context, kind, item_os_ids): + def db_api_get_items_ids(context, kind, item_os_ids): return [(item['id'], item['os_id']) for item in items if (item['os_id'] in item_os_ids and ec2utils.get_ec2_id_kind(item['id']) == kind)] - return db_api_get_item_ids + return db_api_get_items_ids def get_neutron_create(kind, os_id, addon={}):