diff --git a/trove/instance/service.py b/trove/instance/service.py index 4fae31e7ca..883c1cd3d8 100644 --- a/trove/instance/service.py +++ b/trove/instance/service.py @@ -495,6 +495,7 @@ class InstanceController(wsgi.Controller): if configuration_ref: configuration_id = utils.get_id_from_href(configuration_ref) return configuration_id + return "" def _modify_instance(self, context, req, instance, **kwargs): if 'detach_replica' in kwargs and kwargs['detach_replica']: @@ -502,8 +503,6 @@ class InstanceController(wsgi.Controller): context, request=req) with StartNotification(context, instance_id=instance.id): instance.detach_replica() - - instance.update_db(**kwargs) elif 'configuration_id' in kwargs: if kwargs['configuration_id']: context.notification = ( @@ -519,8 +518,6 @@ class InstanceController(wsgi.Controller): request=req)) with StartNotification(context, instance_id=instance.id): instance.detach_configuration() - - instance.update_db(**kwargs) elif 'datastore_version' in kwargs: datastore_version = ds_models.DatastoreVersion.load( instance.datastore, kwargs['datastore_version']) @@ -529,8 +526,6 @@ class InstanceController(wsgi.Controller): with StartNotification(context, instance_id=instance.id, datastore_version_id=datastore_version.id): instance.upgrade(datastore_version) - - instance.update_db(**kwargs) elif 'access' in kwargs: instance.update_access(kwargs['access']) @@ -538,6 +533,7 @@ class InstanceController(wsgi.Controller): """Updates the instance. - attach/detach configuration. + - detach from primary. - access information. """ LOG.info("Updating database instance '%(instance_id)s' for tenant " @@ -548,8 +544,8 @@ class InstanceController(wsgi.Controller): context = req.environ[wsgi.CONTEXT_KEY] name = body['instance'].get('name') - if ((name and len(body['instance']) != 2) or - (not name and len(body['instance']) == 2)): + if ((name and len(body['instance'].keys()) > 2) or + (not name and len(body['instance'].keys()) >= 2)): raise exception.BadRequest("Only one attribute (except 'name') is " "allowed to update.") @@ -566,7 +562,7 @@ class InstanceController(wsgi.Controller): args['detach_replica'] = detach_replica configuration_id = self._configuration_parse(context, body) - if configuration_id: + if configuration_id is not None: args['configuration_id'] = configuration_id if 'access' in body['instance']: @@ -576,31 +572,11 @@ class InstanceController(wsgi.Controller): return wsgi.Result(None, 202) def edit(self, req, id, body, tenant_id): + """Updates the instance to set or unset one or more attributes. + + Deprecated. Use update method instead. """ - Updates the instance to set or unset one or more attributes. - """ - LOG.info("Editing instance for tenant id %s.", tenant_id) - LOG.debug("req: %s", strutils.mask_password(req)) - LOG.debug("body: %s", strutils.mask_password(body)) - context = req.environ[wsgi.CONTEXT_KEY] - - instance = models.Instance.load(context, id) - self.authorize_instance_action(context, 'edit', instance) - - args = {} - args['detach_replica'] = ('replica_of' in body['instance'] or - 'slave_of' in body['instance']) - - if 'name' in body['instance']: - args['name'] = body['instance']['name'] - if 'configuration' in body['instance']: - args['configuration_id'] = self._configuration_parse(context, body) - if 'datastore_version' in body['instance']: - args['datastore_version'] = body['instance'].get( - 'datastore_version') - - self._modify_instance(context, req, instance, **args) - return wsgi.Result(None, 202) + self.update(req, id, body, tenant_id) def configuration(self, req, tenant_id, id): """ diff --git a/trove/tests/api/configurations.py b/trove/tests/api/configurations.py index a8f540879c..92a61fa66e 100644 --- a/trove/tests/api/configurations.py +++ b/trove/tests/api/configurations.py @@ -686,14 +686,13 @@ class DeleteConfigurations(ConfigurationsTestBase): @time_out(30) def test_unassign_configuration_from_instances(self): """test to unassign configuration from instance""" - instance_info.dbaas.instances.modify(configuration_instance.id, - configuration="") + instance_info.dbaas.instances.update(configuration_instance.id, + remove_configuration=True) resp, body = instance_info.dbaas.client.last_response assert_equal(resp.status, 202) - instance_info.dbaas.instances.get(configuration_instance.id) - # test that config group is not removed - instance_info.dbaas.instances.modify(instance_info.id, - configuration=None) + + instance_info.dbaas.instances.update(instance_info.id, + remove_configuration=True) resp, body = instance_info.dbaas.client.last_response assert_equal(resp.status, 202) instance_info.dbaas.instances.get(instance_info.id) @@ -704,10 +703,12 @@ class DeleteConfigurations(ConfigurationsTestBase): return False else: return True + inst_info = instance_info poll_until(result_has_no_configuration) inst_info = configuration_instance poll_until(result_has_no_configuration) + instance = instance_info.dbaas.instances.get(instance_info.id) assert_equal('RESTART_REQUIRED', instance.status) @@ -780,17 +781,17 @@ class DeleteConfigurations(ConfigurationsTestBase): report.log("instance new name:%s" % new_name) saved_name = instance_info.name config_id = configuration_info.id - instance_info.dbaas.instances.edit(instance_info.id, - configuration=config_id, - name=new_name) + instance_info.dbaas.instances.update(instance_info.id, + configuration=config_id, + name=new_name) assert_equal(202, instance_info.dbaas.last_http_code) check = instance_info.dbaas.instances.get(instance_info.id) assert_equal(200, instance_info.dbaas.last_http_code) assert_equal(check.name, new_name) # restore instance name - instance_info.dbaas.instances.edit(instance_info.id, - name=saved_name) + instance_info.dbaas.instances.update(instance_info.id, + name=saved_name) assert_equal(202, instance_info.dbaas.last_http_code) instance = instance_info.dbaas.instances.get(instance_info.id) @@ -813,14 +814,14 @@ class DeleteConfigurations(ConfigurationsTestBase): # already has an assigned configuration with patch config_id = configuration_info.id assert_raises(exceptions.BadRequest, - instance_info.dbaas.instances.edit, + instance_info.dbaas.instances.update, instance_info.id, configuration=config_id) @test(runs_after=[test_assign_config_and_name_to_instance_using_patch]) def test_unassign_configuration_after_patch(self): """Remove the configuration from the instance""" - instance_info.dbaas.instances.edit(instance_info.id, - remove_configuration=True) + instance_info.dbaas.instances.update(instance_info.id, + remove_configuration=True) assert_equal(202, instance_info.dbaas.last_http_code) instance = instance_info.dbaas.instances.get(instance_info.id) assert_equal('RESTART_REQUIRED', instance.status) @@ -847,8 +848,8 @@ class DeleteConfigurations(ConfigurationsTestBase): # test unassign config group from an invalid instance invalid_id = "invalid-inst-id" try: - instance_info.dbaas.instances.edit(invalid_id, - remove_configuration=True) + instance_info.dbaas.instances.update(invalid_id, + remove_configuration=True) except exceptions.NotFound: resp, body = instance_info.dbaas.client.last_response assert_equal(resp.status, 404) diff --git a/trove/tests/api/instances.py b/trove/tests/api/instances.py index 06e87c8852..f0b4517a81 100644 --- a/trove/tests/api/instances.py +++ b/trove/tests/api/instances.py @@ -790,7 +790,7 @@ class CreateInstance(object): # Check these attrs only are returned in create response allowed_attrs = ['created', 'flavor', 'addresses', 'id', 'links', 'name', 'status', 'updated', 'datastore', 'fault', - 'region', 'service_status_updated'] + 'region', 'service_status_updated', 'access'] if ROOT_ON_CREATE: allowed_attrs.append('password') if VOLUME_SUPPORT: @@ -933,7 +933,7 @@ class TestGetInstances(object): def test_index_list(self): allowed_attrs = ['id', 'links', 'name', 'status', 'flavor', 'datastore', 'ip', 'hostname', 'replica_of', - 'region', 'addresses'] + 'region', 'addresses', 'access'] if VOLUME_SUPPORT: allowed_attrs.append('volume') instances = dbaas.instances.list() @@ -955,7 +955,7 @@ class TestGetInstances(object): allowed_attrs = ['created', 'databases', 'flavor', 'hostname', 'id', 'links', 'name', 'status', 'updated', 'ip', 'datastore', 'fault', 'region', - 'service_status_updated', 'addresses'] + 'service_status_updated', 'addresses', 'access'] if VOLUME_SUPPORT: allowed_attrs.append('volume') instances = dbaas.instances.list(detailed=True) @@ -975,7 +975,7 @@ class TestGetInstances(object): allowed_attrs = ['created', 'databases', 'flavor', 'hostname', 'id', 'links', 'name', 'status', 'updated', 'ip', 'datastore', 'fault', 'region', - 'service_status_updated', 'addresses'] + 'service_status_updated', 'addresses', 'access'] if VOLUME_SUPPORT: allowed_attrs.append('volume') else: @@ -1054,7 +1054,8 @@ class TestGetInstances(object): 'flavor', 'guest_status', 'host', 'hostname', 'id', 'name', 'root_enabled_at', 'root_enabled_by', 'server_state_description', 'status', 'datastore', - 'updated', 'users', 'volume', 'fault', 'region'] + 'updated', 'users', 'volume', 'fault', 'region', + 'access'] with CheckInstance(result._info) as check: check.contains_allowed_attrs( result._info, allowed_attrs, @@ -1128,19 +1129,20 @@ class TestUpdateInstance(object): @test def test_update_name(self): new_name = 'new-name' - result = dbaas.instances.edit(instance_info.id, name=new_name) + result = dbaas.instances.update(instance_info.id, name=new_name) assert_equal(202, dbaas.last_http_code) result = dbaas.instances.get(instance_info.id) assert_equal(200, dbaas.last_http_code) assert_equal(new_name, result.name) # Restore instance name because other tests depend on it - dbaas.instances.edit(instance_info.id, name=instance_info.name) + dbaas.instances.update(instance_info.id, name=instance_info.name) assert_equal(202, dbaas.last_http_code) @test def test_update_name_to_invalid_instance(self): # test assigning to an instance that does not exist invalid_id = "invalid-inst-id" - assert_raises(exceptions.NotFound, instance_info.dbaas.instances.edit, + assert_raises(exceptions.NotFound, + instance_info.dbaas.instances.update, invalid_id, name='name') assert_equal(404, instance_info.dbaas.last_http_code) diff --git a/trove/tests/api/replication.py b/trove/tests/api/replication.py index 2f5157fc63..96da65af3f 100644 --- a/trove/tests/api/replication.py +++ b/trove/tests/api/replication.py @@ -386,8 +386,8 @@ class DetachReplica(object): if CONFIG.fake_mode: raise SkipTest("Detach replica not supported in fake mode") - instance_info.dbaas.instances.edit(slave_instance.id, - detach_replica_source=True) + instance_info.dbaas.instances.update(slave_instance.id, + detach_replica_source=True) assert_equal(202, instance_info.dbaas.last_http_code) poll_until(slave_is_running(False)) diff --git a/trove/tests/scenario/runners/replication_runners.py b/trove/tests/scenario/runners/replication_runners.py index 2d862ebdd2..239c3fecfe 100644 --- a/trove/tests/scenario/runners/replication_runners.py +++ b/trove/tests/scenario/runners/replication_runners.py @@ -354,7 +354,7 @@ class ReplicationRunner(TestRunner): def assert_detach_replica( self, replica_id, expected_states, expected_http_code): client = self.auth_client - client.instances.edit(replica_id, detach_replica_source=True) + client.instances.update(replica_id, detach_replica_source=True) self.assert_client_code(client, expected_http_code) self.assert_instance_action(replica_id, expected_states) diff --git a/trove/tests/unittests/instance/test_instance_controller.py b/trove/tests/unittests/instance/test_instance_controller.py index 8be487f123..a4def8faf5 100644 --- a/trove/tests/unittests/instance/test_instance_controller.py +++ b/trove/tests/unittests/instance/test_instance_controller.py @@ -14,6 +14,8 @@ # under the License. # import copy +from unittest import mock +from unittest.mock import MagicMock from unittest.mock import Mock import uuid @@ -23,6 +25,7 @@ from testtools.matchers import Is from testtools.testcase import skip from trove.common import apischema +from trove.common import exception from trove.instance.service import InstanceController from trove.tests.unittests import trove_testtools @@ -359,12 +362,61 @@ class TestInstanceController(trove_testtools.TestCase): self.assertEqual(1, instance.detach_configuration.call_count) - def test_modify_instance_with_access(self): - instance = self._setup_modify_instance_mocks() - args = {} - args['access'] = {'is_public': True} + def test_update_api_invalid_field(self): + body = { + 'instance': { + 'invalid': 'invalid' + } + } + schema = self.controller.get_schema('update', body) + validator = jsonschema.Draft4Validator(schema) + self.assertFalse(validator.is_valid(body)) - self.controller._modify_instance(self.context, self.req, - instance, **args) + @mock.patch('trove.instance.models.Instance.load') + def test_update_name(self, load_mock): + body = { + 'instance': { + 'name': 'new_name' + } + } + ins_mock = MagicMock() + load_mock.return_value = ins_mock - instance.update_access.assert_called_once_with({'is_public': True}) + self.controller.update(MagicMock(), 'fake_id', body, 'fake_tenant_id') + + ins_mock.update_db.assert_called_once_with(name='new_name') + + def test_update_multiple_operations(self): + body = { + 'instance': { + 'name': 'new_name', + 'replica_of': None, + 'configuration': 'fake_config_id' + } + } + + self.assertRaises( + exception.BadRequest, + self.controller.update, + MagicMock(), 'fake_id', body, 'fake_tenant_id' + ) + + @mock.patch('trove.instance.models.Instance.load') + def test_update_name_and_access(self, load_mock): + body = { + 'instance': { + 'name': 'new_name', + 'access': { + 'is_public': True, + 'allowed_cidrs': [] + } + } + } + ins_mock = MagicMock() + load_mock.return_value = ins_mock + + self.controller.update(MagicMock(), 'fake_id', body, 'fake_tenant_id') + + ins_mock.update_db.assert_called_once_with(name='new_name') + ins_mock.update_access.assert_called_once_with( + body['instance']['access'])