From c45b1d7b230e900d0416a4953607e5d4e1dc9cfd Mon Sep 17 00:00:00 2001 From: sunyajing Date: Fri, 24 Jun 2016 12:40:29 +0800 Subject: [PATCH] Fix error for find_service() in identity if there are more than one services be found with one name, a NoUniqueMatch exception should be raised but we can see a NotFound Exception raised instead. It is because in "find_service()", we use "find_resource()" first, if "find_resource()" return a exception, we just think it is a NotFound Exception and continue to find by type but ignore a NoUniqueMatch exception of "find_resource()". This patch refactor the "find_service()" method to solve this problem. Change-Id: Id4619092c57f276ae0698c89df0d5503b7423a4e Co-Authored-By: Huanxuan Ao Closes-Bug:#1597296 --- openstackclient/identity/common.py | 41 ++++++++++------ .../tests/identity/v2_0/test_endpoint.py | 3 -- .../tests/identity/v2_0/test_service.py | 47 ++++++++++++++----- .../tests/identity/v3/test_service.py | 34 ++++++++++++-- .../notes/bug-1597296-9735f33eacf5552e.yaml | 5 ++ 5 files changed, 96 insertions(+), 34 deletions(-) create mode 100644 releasenotes/notes/bug-1597296-9735f33eacf5552e.yaml diff --git a/openstackclient/identity/common.py b/openstackclient/identity/common.py index 379f4114e4..1e40f39687 100644 --- a/openstackclient/identity/common.py +++ b/openstackclient/identity/common.py @@ -30,21 +30,32 @@ def find_service(identity_client, name_type_or_id): """Find a service by id, name or type.""" try: - # search for the usual ID or name - return utils.find_resource(identity_client.services, name_type_or_id) - except exceptions.CommandError: - try: - # search for service type - return identity_client.services.find(type=name_type_or_id) - # FIXME(dtroyer): This exception should eventually come from - # common client exceptions - except identity_exc.NotFound: - msg = _("No service with a type, name or ID of '%s' exists.") - raise exceptions.CommandError(msg % name_type_or_id) - except identity_exc.NoUniqueMatch: - msg = _("Multiple service matches found for '%s', " - "use an ID to be more specific.") - raise exceptions.CommandError(msg % name_type_or_id) + # search for service id + return identity_client.services.get(name_type_or_id) + except identity_exc.NotFound: + # ignore NotFound exception, raise others + pass + + try: + # search for service name + return identity_client.services.find(name=name_type_or_id) + except identity_exc.NotFound: + pass + except identity_exc.NoUniqueMatch: + msg = _("Multiple service matches found for '%s', " + "use an ID to be more specific.") + raise exceptions.CommandError(msg % name_type_or_id) + + try: + # search for service type + return identity_client.services.find(type=name_type_or_id) + except identity_exc.NotFound: + msg = _("No service with a type, name or ID of '%s' exists.") + raise exceptions.CommandError(msg % name_type_or_id) + except identity_exc.NoUniqueMatch: + msg = _("Multiple service matches found for '%s', " + "use an ID to be more specific.") + raise exceptions.CommandError(msg % name_type_or_id) def _get_token_resource(client, resource, parsed_name): diff --git a/openstackclient/tests/identity/v2_0/test_endpoint.py b/openstackclient/tests/identity/v2_0/test_endpoint.py index b2b6d0f180..26ec654d8b 100644 --- a/openstackclient/tests/identity/v2_0/test_endpoint.py +++ b/openstackclient/tests/identity/v2_0/test_endpoint.py @@ -103,9 +103,6 @@ class TestEndpointDelete(TestEndpoint): super(TestEndpointDelete, self).setUp() self.endpoints_mock.get.return_value = self.fake_endpoint - - self.services_mock.get.return_value = self.fake_service - self.endpoints_mock.delete.return_value = None # Get the command object to test diff --git a/openstackclient/tests/identity/v2_0/test_service.py b/openstackclient/tests/identity/v2_0/test_service.py index 318fa83d70..7efd2a6047 100644 --- a/openstackclient/tests/identity/v2_0/test_service.py +++ b/openstackclient/tests/identity/v2_0/test_service.py @@ -13,6 +13,9 @@ # under the License. # +from keystoneclient import exceptions as identity_exc +from osc_lib import exceptions + from openstackclient.identity.v2_0 import service from openstackclient.tests.identity.v2_0 import fakes as identity_fakes @@ -170,7 +173,8 @@ class TestServiceDelete(TestService): def setUp(self): super(TestServiceDelete, self).setUp() - self.services_mock.get.return_value = self.fake_service + self.services_mock.get.side_effect = identity_exc.NotFound(None) + self.services_mock.find.return_value = self.fake_service self.services_mock.delete.return_value = None # Get the command object to test @@ -253,20 +257,23 @@ class TestServiceList(TestService): class TestServiceShow(TestService): + fake_service_s = identity_fakes.FakeService.create_one_service() + def setUp(self): super(TestServiceShow, self).setUp() - self.services_mock.get.return_value = self.fake_service + self.services_mock.get.side_effect = identity_exc.NotFound(None) + self.services_mock.find.return_value = self.fake_service_s # Get the command object to test self.cmd = service.ShowService(self.app, None) def test_service_show(self): arglist = [ - self.fake_service.name, + self.fake_service_s.name, ] verifylist = [ - ('service', self.fake_service.name), + ('service', self.fake_service_s.name), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -275,17 +282,35 @@ class TestServiceShow(TestService): # data to be shown. columns, data = self.cmd.take_action(parsed_args) - # ServiceManager.get(id) - self.services_mock.get.assert_called_with( - self.fake_service.name, + # ServiceManager.find(id) + self.services_mock.find.assert_called_with( + name=self.fake_service_s.name, ) collist = ('description', 'id', 'name', 'type') self.assertEqual(collist, columns) datalist = ( - self.fake_service.description, - self.fake_service.id, - self.fake_service.name, - self.fake_service.type, + self.fake_service_s.description, + self.fake_service_s.id, + self.fake_service_s.name, + self.fake_service_s.type, ) self.assertEqual(datalist, data) + + def test_service_show_nounique(self): + self.services_mock.find.side_effect = identity_exc.NoUniqueMatch(None) + arglist = [ + 'nounique_service', + ] + verifylist = [ + ('service', 'nounique_service'), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + try: + self.cmd.take_action(parsed_args) + self.fail('CommandError should be raised.') + except exceptions.CommandError as e: + self.assertEqual( + "Multiple service matches found for 'nounique_service'," + " use an ID to be more specific.", str(e)) diff --git a/openstackclient/tests/identity/v3/test_service.py b/openstackclient/tests/identity/v3/test_service.py index a1f85adc7b..65d8ebd7de 100644 --- a/openstackclient/tests/identity/v3/test_service.py +++ b/openstackclient/tests/identity/v3/test_service.py @@ -15,6 +15,9 @@ import copy +from keystoneclient import exceptions as identity_exc +from osc_lib import exceptions + from openstackclient.identity.v3 import service from openstackclient.tests import fakes from openstackclient.tests.identity.v3 import fakes as identity_fakes @@ -185,7 +188,8 @@ class TestServiceDelete(TestService): def setUp(self): super(TestServiceDelete, self).setUp() - self.services_mock.get.return_value = fakes.FakeResource( + self.services_mock.get.side_effect = identity_exc.NotFound(None) + self.services_mock.find.return_value = fakes.FakeResource( None, copy.deepcopy(identity_fakes.SERVICE), loaded=True, @@ -282,7 +286,8 @@ class TestServiceSet(TestService): def setUp(self): super(TestServiceSet, self).setUp() - self.services_mock.get.return_value = fakes.FakeResource( + self.services_mock.get.side_effect = identity_exc.NotFound(None) + self.services_mock.find.return_value = fakes.FakeResource( None, copy.deepcopy(identity_fakes.SERVICE), loaded=True, @@ -460,7 +465,8 @@ class TestServiceShow(TestService): def setUp(self): super(TestServiceShow, self).setUp() - self.services_mock.get.return_value = fakes.FakeResource( + self.services_mock.get.side_effect = identity_exc.NotFound(None) + self.services_mock.find.return_value = fakes.FakeResource( None, copy.deepcopy(identity_fakes.SERVICE), loaded=True, @@ -484,8 +490,8 @@ class TestServiceShow(TestService): columns, data = self.cmd.take_action(parsed_args) # ServiceManager.get(id) - self.services_mock.get.assert_called_with( - identity_fakes.service_name, + self.services_mock.find.assert_called_with( + name=identity_fakes.service_name ) collist = ('description', 'enabled', 'id', 'name', 'type') @@ -498,3 +504,21 @@ class TestServiceShow(TestService): identity_fakes.service_type, ) self.assertEqual(datalist, data) + + def test_service_show_nounique(self): + self.services_mock.find.side_effect = identity_exc.NoUniqueMatch(None) + arglist = [ + 'nounique_service', + ] + verifylist = [ + ('service', 'nounique_service'), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + try: + self.cmd.take_action(parsed_args) + self.fail('CommandError should be raised.') + except exceptions.CommandError as e: + self.assertEqual( + "Multiple service matches found for 'nounique_service'," + " use an ID to be more specific.", str(e)) diff --git a/releasenotes/notes/bug-1597296-9735f33eacf5552e.yaml b/releasenotes/notes/bug-1597296-9735f33eacf5552e.yaml new file mode 100644 index 0000000000..677f8d47ce --- /dev/null +++ b/releasenotes/notes/bug-1597296-9735f33eacf5552e.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - Fixed service name lookup in Identity commands to properly handle + multiple matches. + [Bug `1597296 `_]