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 <huanxuan.ao@easystack.cn>
Closes-Bug:#1597296

4 files changed, 91 insertions, 34 deletions

diff --git a/openstackclient/identity/common.py b/openstackclient/identity/common.py
index 379f4114..1e40f396 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 b2b6d0f1..26ec654d 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 318fa83d..7efd2a60 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 a1f85adc..65d8ebd7 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))