Repository: ncclient/ncclient

Issue:
Need some improvements in class EditConfig
Hi @einarnn,
Lately, I've been digging into the code, and found some improvements in class `EditConfig` .
1). the args in the method `request()` don't check their validity before use.
2). the parameter `test_option` is different between [RFC 4741](https://tools.ietf.org/html/rfc4741#section-7.2) and [RFC 6241](https://tools.ietf.org/html/rfc6241#section-7.2), a new value 'test-only',  which is only supported by `:validate:1.1` capability, is added in  [RFC 6241](https://tools.ietf.org/html/rfc6241#section-7.2)

The source code:
https://github.com/ncclient/ncclient/blob/3255200588c27efbd12e30a5d0ab4f7ad003dd3f/ncclient/operations/edit.py#L29-L65

The parameter `test_option` in  [RFC 4741](https://tools.ietf.org/html/rfc4741#section-7.2):
```
test-option:

         The test-option element may be specified only if the device
         advertises the :validate capability (Section 8.6).

         The test-option element has one of the following values:

         test-then-set: Perform a validation test before attempting to
            set.  If validation errors occur, do not perform the
            <edit-config> operation.  This is the default test-option.

         set: Perform a set without a validation test first.
```
The parameter `test_option` in [RFC 6241](https://tools.ietf.org/html/rfc6241#section-7.2):
```
test-option:  The <test-option> element MAY be specified only if
         the device advertises the :validate:1.1 capability
         (Section 8.6).

         The <test-option> element has one of the following values:

         test-then-set:  Perform a validation test before attempting to
            set.  If validation errors occur, do not perform the
            <edit-config> operation.  This is the default test-option.

         set:  Perform a set without a validation test first.

         test-only:  Perform only the validation test, without
            attempting to set.
```


Discussion:

Patch:
diff --git a/ncclient/operations/edit.py b/ncclient/operations/edit.py
index 05d7e05..8cfda3f 100644
--- a/ncclient/operations/edit.py
+++ b/ncclient/operations/edit.py
@@ -39,7 +39,7 @@ class EditConfig(RPC):
 
         *default_operation* if specified must be one of { `"merge"`, `"replace"`, or `"none"` }
 
-        *test_option* if specified must be one of { `"test_then_set"`, `"set"` }
+        *test_option* if specified must be one of { `"test-then-set"`, `"set"`, `"test-only"` }
 
         *error_option* if specified must be one of { `"stop-on-error"`, `"continue-on-error"`, `"rollback-on-error"` }
 
@@ -47,13 +47,17 @@ class EditConfig(RPC):
         """
         node = new_ele("edit-config")
         node.append(util.datastore_or_url("target", target, self._assert))
-        if default_operation is not None:
-        # TODO: check if it is a valid default-operation
+        if (default_operation is not None
+                and util.validate_args('default_operation', default_operation, ["merge", "replace", "none"]) is True):
             sub_ele(node, "default-operation").text = default_operation
-        if test_option is not None:
+        if (test_option is not None
+                and util.validate_args('test_option', test_option, ["test-then-set", "set", "test-only"]) is True):
             self._assert(':validate')
+            if test_option == 'test-only':
+                self._assert(':validate:1.1')
             sub_ele(node, "test-option").text = test_option
-        if error_option is not None:
+        if (error_option is not None
+                and util.validate_args('error_option', error_option, ["stop-on-error", "continue-on-error", "rollback-on-error"]) is True):
             if error_option == "rollback-on-error":
                 self._assert(":rollback-on-error")
             sub_ele(node, "error-option").text = error_option
diff --git a/ncclient/operations/util.py b/ncclient/operations/util.py
index b760fd1..b1bd733 100755
--- a/ncclient/operations/util.py
+++ b/ncclient/operations/util.py
@@ -71,3 +71,9 @@ def build_filter(spec, capcheck=None):
     if type == "xpath" and capcheck is not None:
         capcheck(":xpath")
     return rep
+
+def validate_args(arg_name, value, args_list):
+    # this is a common method, which used to check whether a value is in args_list
+    if value not in args_list:
+        raise OperationError('Invalid value "%s" in "%s" element' % (value, arg_name))
+    return True


Test patch:
diff --git a/test/unit/operations/test_edit.py b/test/unit/operations/test_edit.py
index 548e400..6e13691 100644
--- a/test/unit/operations/test_edit.py
+++ b/test/unit/operations/test_edit.py
@@ -52,12 +52,16 @@ class TestEdit(unittest.TestCase):
                 host-name foo-bar;
             }
             """
-        obj.request(copy.deepcopy(root), format="text", target="running", error_option="rollback-on-error",
-                    default_operation="default", test_option="test")
+        obj.request(copy.deepcopy(root),
+                    format="text",
+                    target="running",
+                    error_option="rollback-on-error",
+                    default_operation="merge",
+                    test_option="test-then-set")
         node = new_ele("edit-config")
         node.append(util.datastore_or_url("target", "running"))
-        sub_ele(node, "default-operation").text = "default"
-        sub_ele(node, "test-option").text = "test"
+        sub_ele(node, "default-operation").text = "merge"
+        sub_ele(node, "test-option").text = "test-then-set"
         sub_ele(node, "error-option").text = "rollback-on-error"
         config_text = sub_ele(node, "config-text")
         sub_ele(config_text, "configuration-text").text = root
@@ -66,6 +70,63 @@ class TestEdit(unittest.TestCase):
         call = ElementTree.tostring(call)
         self.assertEqual(call, xml)
 
+    def test_edit_config_invalid_arguments_exception(self):
+        session = ncclient.transport.SSHSession(self.device_handler)
+        session._server_capabilities = [":rollback-on-error", ":validate", ":validate:1.1"]
+        obj = EditConfig(
+            session,
+            self.device_handler,
+            raise_mode=RaiseMode.ALL)
+        root = """
+            system {
+                host-name foo-bar;
+            }
+            """
+        #invalid argument "default_operation"
+        self.assertRaises(OperationError, obj.request,
+                          copy.deepcopy(root),
+                          format="xml",
+                          target="running",
+                          error_option="stop-on-error",
+                          default_operation="create",
+                          test_option="test-then-set")
+        #invalid argument "error_option"
+        self.assertRaises(OperationError, obj.request,
+                          copy.deepcopy(root),
+                          format="xml",
+                          target="running",
+                          error_option="commit-on-error",
+                          default_operation="merge",
+                          test_option="test-then-set")
+        #invalid argument "test_option"
+        self.assertRaises(OperationError, obj.request,
+                          copy.deepcopy(root),
+                          format="xml",
+                          target="running",
+                          error_option="stop-on-error",
+                          default_operation="merge",
+                          test_option="test")
+
+    def test_edit_config_validate_capability_exception(self):
+        session = ncclient.transport.SSHSession(self.device_handler)
+        session._server_capabilities = [":validate"]
+        obj = EditConfig(
+            session,
+            self.device_handler,
+            raise_mode=RaiseMode.ALL)
+        root = """
+            system {
+                host-name foo-bar;
+            }
+            """
+        self.assertRaises(MissingCapabilityError, obj.request,
+                          copy.deepcopy(root),
+                          format="xml",
+                          target="running",
+                          error_option="stop-on-error",
+                          default_operation="merge",
+                          test_option="test-only")
+
     @patch('ncclient.operations.RPC._request')
     def test_delete_config(self, mock_request):
         session = ncclient.transport.SSHSession(self.device_handler)