Repository: ncclient/ncclient

Issue:
TypeError: __init__() missing 1 required positional argument: 'device_handler'
Why an optional parameter was added in between (and not in the end)

https://github.com/ncclient/ncclient/pull/452/files#diff-9f0edad4d5a881f4165c86a0b3a9116fe67d9b7cdda2d9a888fb98bcb09311feR147

also as device handler is an optional parameter with None value, if a user doesnt pass, this line is bound to through NoneType exception

https://github.com/ncclient/ncclient/pull/452/files#diff-9f0edad4d5a881f4165c86a0b3a9116fe67d9b7cdda2d9a888fb98bcb09311feR175

```
  File "/content/tmp/pyez/tests/unit/test_console.py", line 262, in test_load_console
    op = cu.load(xml, format="xml")
  File "/content/tmp/pyez/lib/jnpr/junos/utils/config.py", line 568, in load
    return try_load(rpc_contents, rpc_xattrs, ignore_warning=ignore_warning)
  File "/content/tmp/pyez/lib/jnpr/junos/utils/config.py", line 480, in try_load
    rpc_contents, ignore_warning=ignore_warning, **rpc_xattrs
File "/content/tmp/pyez/lib/jnpr/junos/rpcmeta.py", line 288, in load_config
    return self._junos.execute(rpc, ignore_warning=ignore_warning)
  File "/content/tmp/pyez/lib/jnpr/junos/decorators.py", line 76, in wrapper
    return function(*args, **kwargs)
  File "/content/tmp/pyez/lib/jnpr/junos/decorators.py", line 31, in wrapper
    return function(*args, **kwargs)
  File "/content/tmp/pyez/lib/jnpr/junos/device.py", line 816, in execute
    filter_xml=kvargs.get("filter_xml"),
  File "/content/tmp/pyez/lib/jnpr/junos/decorators.py", line 117, in wrapper
    rsp = function(self, *args, **kwargs)
  File "/content/tmp/pyez/lib/jnpr/junos/console.py", line 279, in _rpc_reply
    reply = self._tty.nc.rpc(rpc_cmd)
  File "/content/tmp/pyez/lib/jnpr/junos/transport/tty_netconf.py", line 123, in rpc
    reply = RPCReply(rsp, huge_tree=self._tty._huge_tree)
TypeError: __init__() missing 1 required positional argument: 'device_handler'
```

Discussion:

Patch:
diff --git a/ncclient/devices/default.py b/ncclient/devices/default.py
index f9107b4..bac3195 100644
--- a/ncclient/devices/default.py
+++ b/ncclient/devices/default.py
@@ -242,16 +242,16 @@ class DefaultDeviceHandler(object):
     def handle_connection_exceptions(self, sshsession):
         return False
 
-    def handle_reply_parsing_error(self, root, reply):
+    def reply_parsing_error_transform(self, reply_cls):
         """
-        Hook for working around bugs in replies from devices (the root emelent can be "fixed")
+        Hook for working around bugs in replies from devices (the root element can be "fixed")
 
-        :param root: the rpc reply root element
-        :param reply: the RPCReply object that is parsing 'root'
+        :param reply_cls: the RPCReply class that is parsing the reply 'root' xml element
 
-        :return:
+        :return: transform function for the 'root' xml element of the RPC reply in case the normal parsing fails
         """
-        pass
+        # No transformation by default
+        return None
 
     def transform_reply(self):
         return False
diff --git a/ncclient/devices/junos.py b/ncclient/devices/junos.py
index 01e7501..d751560 100644
--- a/ncclient/devices/junos.py
+++ b/ncclient/devices/junos.py
@@ -39,7 +39,7 @@ class JunosDeviceHandler(DefaultDeviceHandler):
 
     def __init__(self, device_params):
         super(JunosDeviceHandler, self).__init__(device_params)
-        self.__reply_parsing_error_handler_by_cls = {
+        self.__reply_parsing_error_transform_by_cls = {
             GetSchemaReply: fix_get_schema_reply
         }
 
@@ -95,13 +95,9 @@ class JunosDeviceHandler(DefaultDeviceHandler):
         c.exec_command("xml-mode netconf need-trailer")
         return True
 
-    def handle_reply_parsing_error(self, root, reply):
-        reply_class = type(reply)
-
-        # Apply transform if found
-        transform_handler = self.__reply_parsing_error_handler_by_cls.get(reply_class)
-        if transform_handler is not None:
-            transform_handler(root)
+    def reply_parsing_error_transform(self, reply_cls):
+        # return transform function if found, else None
+        return self.__reply_parsing_error_transform_by_cls.get(reply_cls)
 
     def transform_reply(self):
         reply = '''<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
diff --git a/ncclient/operations/rpc.py b/ncclient/operations/rpc.py
index 2a6f32d..75f0839 100644
--- a/ncclient/operations/rpc.py
+++ b/ncclient/operations/rpc.py
@@ -144,9 +144,9 @@ class RPCReply(object):
     ERROR_CLS = RPCError
     "Subclasses can specify a different error class, but it should be a subclass of `RPCError`."
 
-    def __init__(self, raw, device_handler, huge_tree=False):
+    def __init__(self, raw, huge_tree=False, parsing_error_transform=None):
         self._raw = raw
-        self._device_handler = device_handler
+        self._parsing_error_transform = parsing_error_transform
         self._parsed = False
         self._root = None
         self._errors = []
@@ -171,8 +171,13 @@ class RPCReply(object):
         try:
             self._parsing_hook(root)
         except Exception as e:
+            if self._parsing_error_transform is None:
+                # re-raise as we have no workaround
+                exc_type, exc_value, exc_traceback = sys.exc_info()
+                six.reraise(exc_type, exc_value, exc_traceback)
+
             # Apply device specific workaround and try again
-            self._device_handler.handle_reply_parsing_error(root, self)
+            self._parsing_error_transform(root)
             self._parsing_hook(root)
 
         self._parsed = True
@@ -181,6 +186,9 @@ class RPCReply(object):
         "No-op by default. Gets passed the *root* element for the reply."
         pass
 
+    def set_parsing_error_transform(self, transform_function):
+        self._parsing_error_transform = transform_function
+
     @property
     def xml(self):
         "*rpc-reply* element as returned."
@@ -387,7 +395,14 @@ class RPC(object):
 
     def deliver_reply(self, raw):
         # internal use
-        self._reply = self.REPLY_CLS(raw, self._device_handler, huge_tree=self._huge_tree)
+        self._reply = self.REPLY_CLS(raw, huge_tree=self._huge_tree)
+
+        # Set the reply_parsing_error transform outside the constructor, to keep compatibility for
+        # third party reply classes outside of ncclient
+        self._reply.set_parsing_error_transform(
+            self._device_handler.reply_parsing_error_transform(self.REPLY_CLS)
+        )
+
         self._event.set()
 
     def deliver_error(self, err):


Test patch:
diff --git a/test/unit/operations/test_rpc.py b/test/unit/operations/test_rpc.py
index 3267531..249e876 100644
--- a/test/unit/operations/test_rpc.py
+++ b/test/unit/operations/test_rpc.py
@@ -121,7 +121,7 @@ xml7 = """<rpc-error xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
 class TestRPC(unittest.TestCase):
 
     def test_rpc_reply(self):
-        obj = RPCReply(xml4, self._mock_device_handler())
+        obj = RPCReply(xml4)
         obj.parse()
         self.assertTrue(obj.ok)
         self.assertFalse(obj.error)
@@ -129,11 +129,11 @@ class TestRPC(unittest.TestCase):
         self.assertTrue(obj._parsed)
 
     def test_rpc_reply_huge_text_node_exception(self):
-        obj = RPCReply(xml5_huge, self._mock_device_handler())
+        obj = RPCReply(xml5_huge)
         self.assertRaises(etree.XMLSyntaxError, obj.parse)
 
     def test_rpc_reply_huge_text_node_workaround(self):
-        obj = RPCReply(xml5_huge, self._mock_device_handler(), huge_tree=True)
+        obj = RPCReply(xml5_huge, huge_tree=True)
         obj.parse()
         self.assertTrue(obj.ok)
         self.assertFalse(obj.error)
@@ -145,7 +145,7 @@ class TestRPC(unittest.TestCase):
     def test_rpc_send(self, mock_thread, mock_send):
         device_handler, session = self._mock_device_handler_and_session()
         obj = RPC(session, device_handler, raise_mode=RaiseMode.ALL, timeout=0)
-        reply = RPCReply(xml1, device_handler)
+        reply = RPCReply(xml1)
         obj._reply = reply
         node = new_ele("commit")
         sub_ele(node, "confirmed")
@@ -171,7 +171,7 @@ class TestRPC(unittest.TestCase):
     def test_generic_rpc_send(self, mock_thread, mock_send):
         device_handler, session = self._mock_device_handler_and_session()
         obj = GenericRPC(session, device_handler, raise_mode=RaiseMode.ALL, timeout=0)
-        reply = RPCReply(xml1, device_handler)
+        reply = RPCReply(xml1)
         obj._reply = reply
         rpc_command = 'edit-config'
         filters = ('subtree', '<top xmlns="urn:mod1"/>')
@@ -206,7 +206,7 @@ class TestRPC(unittest.TestCase):
             raise_mode=RaiseMode.ALL,
             timeout=0,
             async_mode=True)
-        reply = RPCReply(xml1, device_handler)
+        reply = RPCReply(xml1)
         obj._reply = reply
         node = new_ele("commit")
         result = obj._request(node)
@@ -217,7 +217,7 @@ class TestRPC(unittest.TestCase):
     def test_rpc_timeout_error(self, mock_thread, mock_send):
         device_handler, session = self._mock_device_handler_and_session()
         obj = RPC(session, device_handler, raise_mode=RaiseMode.ALL, timeout=0)
-        reply = RPCReply(xml1, device_handler)
+        reply = RPCReply(xml1)
         obj.deliver_reply(reply)
         node = new_ele("commit")
         sub_ele(node, "confirmed")
@@ -229,7 +229,7 @@ class TestRPC(unittest.TestCase):
     def test_rpc_rpcerror(self, mock_thread, mock_send):
         device_handler, session = self._mock_device_handler_and_session()
         obj = RPC(session, device_handler, raise_mode=RaiseMode.ALL, timeout=0)
-        reply = RPCReply(xml1, device_handler)
+        reply = RPCReply(xml1)
         obj._reply = reply
         node = new_ele("commit")
         sub_ele(node, "confirmed")
@@ -315,9 +315,6 @@ class TestRPC(unittest.TestCase):
         obj.huge_tree = False
         self.assertFalse(obj.huge_tree)
 
-    def _mock_device_handler(self):
-        return manager.make_device_handler({'name': 'default'})
-
     def _mock_device_handler_and_session(self):
         device_handler = manager.make_device_handler({'name': 'junos'})
         capabilities = Capabilities(device_handler.get_capabilities())