From: John Mulligan <jmulligan@redhat.com>
Date: Fri, 19 Aug 2022 15:39:44 +0000 (-0400)
Subject: pybind/mgr: improve behavior of ErrorResponse.wrap method
X-Git-Tag: v17.2.6~497^2~3
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=30ac14d39d09bd2053daea3ed12cfc0ef74b2033;p=ceph.git

pybind/mgr: improve behavior of ErrorResponse.wrap method

The wrap classmethod is intended to turn any exception into something
that can produce a manager response. All exceptions inheriting from
ErrorResponseBase can do that, so instead of losing all that state,
have wrap return those exceptions inheriting from ErrorResponseBase
as-is.

Additionally, only change the sign of errno values that aren't already
negative.

Adds tests for these cases.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
(cherry picked from commit 61f00ddcb0cce8f3da47ab20faa9d344726e71cd)
---

diff --git a/src/pybind/mgr/object_format.py b/src/pybind/mgr/object_format.py
index d4c04fd9144..b53bc3eb060 100644
--- a/src/pybind/mgr/object_format.py
+++ b/src/pybind/mgr/object_format.py
@@ -426,10 +426,14 @@ class ErrorResponse(ErrorResponseBase):
     @classmethod
     def wrap(
         cls: Type[E], exc: Exception, return_value: Optional[int] = None
-    ) -> E:
+    ) -> ErrorResponseBase:
+        if isinstance(exc, ErrorResponseBase):
+            return exc
         if return_value is None:
             try:
-                return_value = -int(getattr(exc, "errno"))
+                return_value = int(getattr(exc, "errno"))
+                if return_value > 0:
+                    return_value = -return_value
             except (AttributeError, ValueError):
                 pass
         err = cls(str(exc), return_value=return_value)
diff --git a/src/pybind/mgr/tests/test_object_format.py b/src/pybind/mgr/tests/test_object_format.py
index f708333a068..d2fd20870e7 100644
--- a/src/pybind/mgr/tests/test_object_format.py
+++ b/src/pybind/mgr/tests/test_object_format.py
@@ -272,11 +272,13 @@ class DecoDemo:
 
     @CLICommand("empty one", perm="rw")
     @object_format.EmptyResponder()
-    def empty_one(self, name: str = "default") -> None:
+    def empty_one(self, name: str = "default", retval: Optional[int] = None) -> None:
         # in real code, this would be making some sort of state change
         # but we need to handle erors still
+        if retval is None:
+            retval = -5
         if name in ["pow"]:
-            raise object_format.ErrorResponse(name, return_value=-5)
+            raise object_format.ErrorResponse(name, return_value=retval)
         return
 
     @CLICommand("empty bad", perm="rw")
@@ -464,6 +466,17 @@ class DecoDemo:
                 "pow",
             ),
         ),
+        # Ensure setting return_value to zero even on an exception is honored
+        (
+            "empty one",
+            False,
+            {"name": "pow", "retval": 0},
+            (
+                0,
+                "",
+                "pow",
+            ),
+        ),
     ],
 )
 def test_cli_with_decorators(prefix, can_format, args, response):
@@ -504,6 +517,64 @@ def test_error_response():
     assert "blat" in r[2]
     assert r[0] == e3.mgr_return_value()
 
+    # A custom exception type with an errno property
+
+    class MyCoolException(Exception):
+        def __init__(self, err_msg: str, errno: int = 0) -> None:
+            super().__init__(errno, err_msg)
+            self.errno = errno
+            self.err_msg = err_msg
+
+        def __str__(self) -> str:
+            return self.err_msg
+
+    e4 = object_format.ErrorResponse.wrap(MyCoolException("beep", -17))
+    r = e4.format_response()
+    assert r[0] == -17
+    assert r[1] == ""
+    assert r[2] == "beep"
+    assert e4.mgr_return_value() == -17
+
+    e5 = object_format.ErrorResponse.wrap(MyCoolException("ok, fine", 0))
+    r = e5.format_response()
+    assert r[0] == 0
+    assert r[1] == ""
+    assert r[2] == "ok, fine"
+
+    e5 = object_format.ErrorResponse.wrap(MyCoolException("no can do", 8))
+    r = e5.format_response()
+    assert r[0] == -8
+    assert r[1] == ""
+    assert r[2] == "no can do"
+
+    # A custom exception type that inherits from ErrorResponseBase
+
+    class MyErrorResponse(object_format.ErrorResponseBase):
+        def __init__(self, err_msg: str, return_value: int):
+            super().__init__(self, err_msg)
+            self.msg = err_msg
+            self.return_value = return_value
+
+        def format_response(self):
+            return self.return_value, "", self.msg
+
+
+    e6 = object_format.ErrorResponse.wrap(MyErrorResponse("yeah, sure", 0))
+    r = e6.format_response()
+    assert r[0] == 0
+    assert r[1] == ""
+    assert r[2] == "yeah, sure"
+    assert isinstance(e5, object_format.ErrorResponseBase)
+    assert isinstance(e6, MyErrorResponse)
+
+    e7 = object_format.ErrorResponse.wrap(MyErrorResponse("no can do", -8))
+    r = e7.format_response()
+    assert r[0] == -8
+    assert r[1] == ""
+    assert r[2] == "no can do"
+    assert isinstance(e7, object_format.ErrorResponseBase)
+    assert isinstance(e7, MyErrorResponse)
+
 
 def test_empty_responder_return_check():
     dd = DecoDemo()