From 0ec47f9eb4347983606c81c41838fff1eb226346 Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Tue, 6 Dec 2016 13:30:12 +0100 Subject: [PATCH] objects: add delete_objects public method Sometimes we don't need to construct objects before deleting them, and it's a waste to do so. It may also be easier to use a single delete_objects call instead of a loop with get_objects and delete. This patch introduces a method that will delete all matching resources without constructing full blown versioned objects. before: for obj in cls.get_objects(...): obj.delete() after: cls.delete_objects(...) delete_objects also returns the number of entries deleted from DB. The default non-db implementation relies on get_objects and delete methods, meaning it still constructs objects. Co-Authored-By: Artur Korzeniewski Partially-Implements: blueprint adopt-oslo-versioned-objects-for-db Change-Id: I94a79d244b80421f77b714c94248d8ec55c95946 --- neutron/db/allowedaddresspairs_db.py | 14 ++--- neutron/db/db_base_plugin_common.py | 4 -- neutron/db/ipam_backend_mixin.py | 10 +-- neutron/db/servicetype_db.py | 4 +- neutron/objects/base.py | 24 ++++++++ neutron/objects/db/api.py | 8 ++- neutron/objects/network.py | 5 +- neutron/objects/subnetpool.py | 6 +- neutron/tests/unit/objects/test_base.py | 82 +++++++++++++++++++++++-- 9 files changed, 117 insertions(+), 40 deletions(-) diff --git a/neutron/db/allowedaddresspairs_db.py b/neutron/db/allowedaddresspairs_db.py index d7083bbc8be..665a2a88a83 100644 --- a/neutron/db/allowedaddresspairs_db.py +++ b/neutron/db/allowedaddresspairs_db.py @@ -58,14 +58,10 @@ class AllowedAddressPairsMixin(object): return allowed_address_pairs def get_allowed_address_pairs(self, context, port_id): - pairs = self._get_allowed_address_pairs_objs(context, port_id) - return [self._make_allowed_address_pairs_dict(pair.db_obj) - for pair in pairs] - - def _get_allowed_address_pairs_objs(self, context, port_id): pairs = obj_addr_pair.AllowedAddressPair.get_objects( context, port_id=port_id) - return pairs + return [self._make_allowed_address_pairs_dict(pair.db_obj) + for pair in pairs] def _extend_port_dict_allowed_address_pairs(self, port_res, port_db): # If port_db is provided, allowed address pairs will be accessed via @@ -82,10 +78,8 @@ class AllowedAddressPairsMixin(object): attr.PORTS, ['_extend_port_dict_allowed_address_pairs']) def _delete_allowed_address_pairs(self, context, id): - pairs = self._get_allowed_address_pairs_objs(context, port_id=id) - with context.session.begin(subtransactions=True): - for pair in pairs: - pair.delete() + obj_addr_pair.AllowedAddressPair.delete_objects( + context, port_id=id) @staticmethod def _make_allowed_address_pairs_dict(allowed_address_pairs, diff --git a/neutron/db/db_base_plugin_common.py b/neutron/db/db_base_plugin_common.py index a20605b181f..864c90d30e9 100644 --- a/neutron/db/db_base_plugin_common.py +++ b/neutron/db/db_base_plugin_common.py @@ -224,10 +224,6 @@ class DbBasePluginCommon(common_db_mixin.CommonDbMixin): raise n_exc.PortNotFound(port_id=id) return port - def _get_dns_by_subnet(self, context, subnet_id): - return subnet_obj.DNSNameServer.get_objects(context, - subnet_id=subnet_id) - def _get_route_by_subnet(self, context, subnet_id): return subnet_obj.Route.get_objects(context, subnet_id=subnet_id) diff --git a/neutron/db/ipam_backend_mixin.py b/neutron/db/ipam_backend_mixin.py index 88c22ae987b..8921a81e568 100644 --- a/neutron/db/ipam_backend_mixin.py +++ b/neutron/db/ipam_backend_mixin.py @@ -146,14 +146,12 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon): return new_routes def _update_subnet_dns_nameservers(self, context, id, s): - old_dns_list = self._get_dns_by_subnet(context, id) new_dns_addr_list = s["dns_nameservers"] # NOTE(changzhi) delete all dns nameservers from db # when update subnet's DNS nameservers. And store new # nameservers with order one by one. - for dns in old_dns_list: - dns.delete() + subnet_obj.DNSNameServer.delete_objects(context, subnet_id=id) for order, server in enumerate(new_dns_addr_list): dns = subnet_obj.DNSNameServer(context, @@ -182,10 +180,8 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon): return result_pools def _update_subnet_service_types(self, context, subnet_id, s): - old_types = subnet_obj.SubnetServiceType.get_objects( - context, subnet_id=subnet_id) - for service_type in old_types: - service_type.delete() + subnet_obj.SubnetServiceType.delete_objects(context, + subnet_id=subnet_id) updated_types = s.pop('service_types') for service_type in updated_types: new_type = subnet_obj.SubnetServiceType(context, diff --git a/neutron/db/servicetype_db.py b/neutron/db/servicetype_db.py index 3cc497e6e78..a7aff905ffc 100644 --- a/neutron/db/servicetype_db.py +++ b/neutron/db/servicetype_db.py @@ -103,10 +103,8 @@ class ServiceTypeManager(object): def del_resource_associations(self, context, resource_ids): if not resource_ids: return - objs = servicetype_obj.ProviderResourceAssociation.get_objects( + servicetype_obj.ProviderResourceAssociation.delete_objects( context, resource_id=resource_ids) - for obj in objs: - obj.delete() _deprecate._MovedGlobals() diff --git a/neutron/objects/base.py b/neutron/objects/base.py index 5f4e7858027..f429a04d5cc 100644 --- a/neutron/objects/base.py +++ b/neutron/objects/base.py @@ -175,6 +175,13 @@ class NeutronObject(obj_base.VersionedObject, **kwargs): raise NotImplementedError() + @classmethod + def delete_objects(cls, context, validate_filters=True, **kwargs): + objs = cls.get_objects(context, validate_filters, **kwargs) + for obj in objs: + obj.delete() + return len(objs) + def create(self): raise NotImplementedError() @@ -430,6 +437,23 @@ class NeutronDbObject(NeutronObject): ) return [cls._load_object(context, db_obj) for db_obj in db_objs] + @classmethod + def delete_objects(cls, context, validate_filters=True, **kwargs): + """ + Delete objects that match filtering criteria from DB. + + :param context: + :param validate_filters: Raises an error in case of passing an unknown + filter + :param kwargs: multiple keys defined by key=value pairs + :return: Number of entries deleted + """ + if validate_filters: + cls.validate_filters(**kwargs) + with db_api.autonested_transaction(context.session): + return obj_db_api.delete_objects( + context, cls.db_model, **cls.modify_fields_to_db(kwargs)) + @classmethod def is_accessible(cls, context, db_obj): return (context.is_admin or diff --git a/neutron/objects/db/api.py b/neutron/objects/db/api.py index 3ac1c9e8440..a98fb9d6c02 100644 --- a/neutron/objects/db/api.py +++ b/neutron/objects/db/api.py @@ -87,13 +87,17 @@ def delete_object(context, model, **kwargs): context.session.delete(db_obj) -# TODO(ihrachys): expose through objects API def delete_objects(context, model, **kwargs): - '''Delete matching objects, if any. + '''Delete matching objects, if any. Return number of deleted objects. This function does not raise exceptions if nothing matches. + + :param model: SQL model + :param kwargs: multiple filters defined by key=value pairs + :return: Number of entries deleted ''' with context.session.begin(subtransactions=True): db_objs = get_objects(context, model, **kwargs) for db_obj in db_objs: context.session.delete(db_obj) + return len(db_objs) diff --git a/neutron/objects/network.py b/neutron/objects/network.py index da917cbbfe4..7274d019157 100644 --- a/neutron/objects/network.py +++ b/neutron/objects/network.py @@ -167,10 +167,7 @@ class Network(rbac_db.NeutronRbacObject): self.obj_reset_changes(['qos_policy_id']) def _set_dns_domain(self, dns_domain): - objs = NetworkDNSDomain.get_objects(self.obj_context, - network_id=self.id) - for obj in objs: - obj.delete() + NetworkDNSDomain.delete_objects(self.obj_context, network_id=self.id) if dns_domain: NetworkDNSDomain(self.obj_context, network_id=self.id, dns_domain=dns_domain).create() diff --git a/neutron/objects/subnetpool.py b/neutron/objects/subnetpool.py index c4fdc81ad69..87fa3df629d 100644 --- a/neutron/objects/subnetpool.py +++ b/neutron/objects/subnetpool.py @@ -119,10 +119,8 @@ class SubnetPool(base.NeutronDbObject): super(SubnetPool, self).update() if synthetic_changes: if 'prefixes' in synthetic_changes: - old = SubnetPoolPrefix.get_objects( - self.obj_context, subnetpool_id=self.id) - for prefix in old: - prefix.delete() + SubnetPoolPrefix.delete_objects(self.obj_context, + subnetpool_id=self.id) for prefix in self.prefixes: prefix_obj = SubnetPoolPrefix(self.obj_context, subnetpool_id=self.id, diff --git a/neutron/tests/unit/objects/test_base.py b/neutron/tests/unit/objects/test_base.py index 3d6999d73ed..c6f983779cc 100644 --- a/neutron/tests/unit/objects/test_base.py +++ b/neutron/tests/unit/objects/test_base.py @@ -476,8 +476,11 @@ class _BaseObjectTestCase(object): for fields in self.obj_fields ] + invalid_fields = ( + set(self._test_class.synthetic_fields).union(set(TIMESTAMP_FIELDS)) + ) valid_field = [f for f in self._test_class.fields - if f not in self._test_class.synthetic_fields][0] + if f not in invalid_fields][0] self.valid_field_filter = {valid_field: self.obj_fields[-1][valid_field]} self.obj_registry = self.useFixture( @@ -757,6 +760,34 @@ class BaseObjectIfaceTestCase(_BaseObjectTestCase, test_base.BaseTestCase): [get_obj_db_fields(obj) for obj in self.objs], [get_obj_db_fields(obj) for obj in objs]) + def test_delete_objects(self): + '''Test that delete_objects calls to underlying db_api.''' + with mock.patch.object( + obj_db_api, 'delete_objects', return_value=0 + ) as delete_objects_mock: + self.assertEqual(0, self._test_class.delete_objects(self.context)) + delete_objects_mock.assert_any_call( + self.context, self._test_class.db_model) + + def test_delete_objects_valid_fields(self): + '''Test that a valid filter does not raise an error.''' + with mock.patch.object(obj_db_api, 'delete_objects', return_value=0): + self._test_class.delete_objects(self.context, + **self.valid_field_filter) + + def test_delete_objects_invalid_fields(self): + with mock.patch.object(obj_db_api, 'delete_objects'): + self.assertRaises(n_exc.InvalidInput, + self._test_class.delete_objects, self.context, + fake_field='xxx') + + def test_delete_objects_without_validate_filters(self): + with mock.patch.object( + obj_db_api, 'delete_objects'): + self._test_class.delete_objects(self.context, + validate_filters=False, + unknown_filter='value') + def test_count(self): if not isinstance(self._test_class, base.NeutronDbObject): self.skipTest('Class %s does not inherit from NeutronDbObject' % @@ -783,7 +814,10 @@ class BaseObjectIfaceTestCase(_BaseObjectTestCase, test_base.BaseTestCase): self.assertEqual(expected, self._test_class.count(self.context, validate_filters=False, fake_field='xxx')) - def test_create(self): + # Adding delete_objects mock because some objects are using delete_objects + # while calling create(), Port for example + @mock.patch.object(obj_db_api, 'delete_objects') + def test_create(self, *mocks): with mock.patch.object(obj_db_api, 'create_object', return_value=self.db_objs[0]) as create_mock: with mock.patch.object(obj_db_api, 'get_objects', @@ -797,7 +831,10 @@ class BaseObjectIfaceTestCase(_BaseObjectTestCase, test_base.BaseTestCase): self._test_class.modify_fields_to_db( get_obj_db_fields(self.objs[0]))) - def test_create_updates_from_db_object(self): + # Adding delete_objects mock because some objects are using delete_objects + # while calling create(), Port for example + @mock.patch.object(obj_db_api, 'delete_objects') + def test_create_updates_from_db_object(self, *mocks): with mock.patch.object(obj_db_api, 'create_object', return_value=self.db_objs[0]): with mock.patch.object(obj_db_api, 'get_objects', @@ -805,7 +842,10 @@ class BaseObjectIfaceTestCase(_BaseObjectTestCase, test_base.BaseTestCase): self.objs[1].create() self._check_equal(self.objs[0], self.objs[1]) - def test_create_duplicates(self): + # Adding delete_objects mock because some objects are using delete_objects + # while calling create(), Port for example + @mock.patch.object(obj_db_api, 'delete_objects') + def test_create_duplicates(self, delete_object): with mock.patch.object(obj_db_api, 'create_object', side_effect=obj_exc.DBDuplicateEntry): obj = self._test_class(self.context, **self.obj_fields[0]) @@ -876,8 +916,11 @@ class BaseObjectIfaceTestCase(_BaseObjectTestCase, test_base.BaseTestCase): project_id = self.obj_fields[0]['project_id'] self.assertEqual(project_id, obj.tenant_id) + # Adding delete_objects mock because some objects are using delete_objects + # while calling update(), Port for example + @mock.patch.object(obj_db_api, 'delete_objects') @mock.patch.object(obj_db_api, 'update_object') - def test_update_changes(self, update_mock): + def test_update_changes(self, update_mock, del_mock): fields_to_update = self.get_updatable_fields( self._test_class.modify_fields_from_db(self.db_objs[0])) if not fields_to_update: @@ -914,7 +957,10 @@ class BaseObjectIfaceTestCase(_BaseObjectTestCase, test_base.BaseTestCase): obj = self._test_class(self.context, **self.obj_fields[0]) self.assertRaises(o_exc.NeutronObjectUpdateForbidden, obj.update) - def test_update_updates_from_db_object(self): + # Adding delete_objects mock because some objects are using delete_objects + # while calling update(), Port and Network for example + @mock.patch.object(obj_db_api, 'delete_objects') + def test_update_updates_from_db_object(self, *mocks): with mock.patch.object(obj_db_api, 'update_object', return_value=self.db_objs[0]): with mock.patch.object(obj_db_api, 'get_objects', @@ -1515,6 +1561,30 @@ class BaseDbObjectTestCase(_BaseObjectTestCase, self.assertTrue(self._test_class.objects_exist( self.context, validate_filters=False, fake_filter='xxx')) + def test_delete_objects(self): + for fields in self.obj_fields: + self._make_object(fields).create() + + objs = self._test_class.get_objects( + self.context, **self.valid_field_filter) + for k, v in self.valid_field_filter.items(): + self.assertEqual(v, objs[0][k]) + + count = self._test_class.delete_objects( + self.context, **self.valid_field_filter) + + self.assertEqual(len(objs), count) + + new_objs = self._test_class.get_objects(self.context) + self.assertEqual(len(self.obj_fields) - len(objs), len(new_objs)) + for obj in new_objs: + for k, v in self.valid_field_filter.items(): + self.assertNotEqual(v, obj[k]) + + def test_delete_objects_nothing_to_delete(self): + self.assertEqual( + 0, self._test_class.delete_objects(self.context)) + def test_db_obj(self): obj = self._make_object(self.obj_fields[0]) self.assertIsNone(obj.db_obj)