[trac983] allowed python representation of JSON (as well as bare string)

in load()

src/lib/python/isc/acl/dns_requestloader_inc.cc

@@ -65,7 +65,7 @@ this restriction may be removed.\n\
 const char* const RequestLoader_load_doc = "\
 load(description) -> RequestACL\n\
 \n\
-Load a DNS ACL.\n\
+Load a DNS (Request) ACL.\n\
 \n\
 This parses an ACL list, creates internal data for each rule\n\
 and returns a RequestACl object that contains all given rules.\n\
@@ -77,7 +77,9 @@ Exceptions:\n\
               exception.\n\
 \n\
 Parameters:\n\
-  description String representation of the JSON list of ACL.\n\
+  description String or Python representation of the JSON list of\n\
+              ACL. The Python representation is ones accepted by the\n\
+              standard json module.\n\
 \n\
 Return Value(s): The newly created RequestACL object\n\
 ";

src/lib/python/isc/acl/dns_requestloader_python.cc

@@ -82,13 +82,58 @@ RequestLoader_destroy(PyObject* po_self) {
     Py_TYPE(self)->tp_free(self);
 }
 
+// This helper function essentially does:
+//   import json
+//   return json.dumps
+// Getting access to the json module this way and call one of its functions
+// via PyObject_CallObject() may exceed the reasonably acceptable level for
+// straightforward bindings.  But the alternative would be to write a Python
+// frontend for the entire module only for this conversion, which would also
+// be too much.  So, right now, we implement everything within the binding
+// implementation.  If future extensions require more such non trivial
+// wrappers, we should consider the frontend approach more seriously.
+PyObject*
+getJSONDumpsObj() {
+    PyObject* json_dump_obj = NULL;
+    PyObject* json_module = PyImport_AddModule("json");
+    if (json_module != NULL) {
+        PyObject* json_dict = PyModule_GetDict(json_module);
+        if (json_dict != NULL) {
+            json_dump_obj = PyDict_GetItemString(json_dict, "dumps");
+        }
+    }
+    return (json_dump_obj);
+}
+
 PyObject*
 RequestLoader_load(PyObject* po_self, PyObject* args) {
     s_RequestLoader* const self = static_cast<s_RequestLoader*>(po_self);
-    const char* acl_config;
 
-    if (PyArg_ParseTuple(args, "s", &acl_config)) {
-        try {
+    try {
+        PyObjectContainer c1, c2; // placeholder for temporary py objects
+        const char* acl_config;
+
+        // First, try string
+        int py_result = PyArg_ParseTuple(args, "s", &acl_config);
+        if (!py_result) {
+            PyErr_Clear();  // need to clear the error from ParseTuple
+
+            // If that fails, confirm the argument is a single Python object,
+            // and pass the argument to json.dumps() without conversion.
+            // Note that we should pass 'args', not 'json_obj' to
+            // PyObject_CallObject(), since this function expects a form of
+            // tuple as its argument parameter, just like ParseTuple.
+            PyObject* json_obj;
+            if (PyArg_ParseTuple(args, "O", &json_obj)) {
+                PyObject* json_dumps_obj = getJSONDumpsObj();
+                if (json_dumps_obj != NULL) {
+                    c1.reset(PyObject_CallObject(json_dumps_obj, args));
+                    c2.reset(Py_BuildValue("(O)", c1.get()));
+                    py_result = PyArg_ParseTuple(c2.get(), "s", &acl_config);
+                }
+            }
+        }
+        if (py_result) {
             shared_ptr<RequestACL> acl(
                 self->cppobj->load(Element::fromJSON(acl_config)));
             s_RequestACL* py_acl = static_cast<s_RequestACL*>(
@@ -97,15 +142,24 @@ RequestLoader_load(PyObject* po_self, PyObject* args) {
                 py_acl->cppobj = acl;
             }
             return (py_acl);
-        } catch (const exception& ex) {
-            PyErr_SetString(getACLException("LoaderError"), ex.what());
-            return (NULL);
-        } catch (...) {
-            PyErr_SetString(PyExc_SystemError, "Unexpected C++ exception");
-            return (NULL);
         }
+    } catch (const PyCPPWrapperException&) {
+        // If the wrapper utility throws, it's most likely because an invalid
+        // type of argument is passed (and the call to json.dumps() failed
+        // above), rather than a rare case of system errors such as memory
+        // allocation failure.  So we fall through to the end of this function
+        // and raise a TypeError.
+        ;
+    } catch (const exception& ex) {
+        PyErr_SetString(getACLException("LoaderError"), ex.what());
+        return (NULL);
+    } catch (...) {
+        PyErr_SetString(PyExc_SystemError, "Unexpected C++ exception");
+        return (NULL);
     }
 
+    PyErr_SetString(PyExc_TypeError, "RequestLoader.load() "
+                    "expects str or python representation of JSON");
     return (NULL);
 }
 

src/lib/python/isc/acl/tests/dns_test.py

@@ -32,6 +32,13 @@ def get_acl(prefix):
     return REQUEST_LOADER.load('[{"action": "ACCEPT", "from": "' + \
                                    prefix + '"}]')
 
+def get_acl_json(prefix):
+    '''Same as get_acl, but this function passes a Python representation of
+    JSON to the loader, not a string.'''
+    json = [{"action": "ACCEPT"}]
+    json[0]["from"] = prefix
+    return REQUEST_LOADER.load(json)
+
 def get_context(address):
     '''This is a simple shortcut wrapper for creating a RequestContext
     object with a given IP address.  Port number doesn't matter in the test
@@ -100,11 +107,14 @@ class RequestACLTest(unittest.TestCase):
     def test_request_loader(self):
         # these shouldn't raise an exception
         REQUEST_LOADER.load('[{"action": "DROP"}]')
+        REQUEST_LOADER.load([{"action": "DROP"}])
         REQUEST_LOADER.load('[{"action": "DROP", "from": "192.0.2.1"}]')
+        REQUEST_LOADER.load([{"action": "DROP", "from": "192.0.2.1"}])
 
-        # Invalid types
-        self.assertRaises(TypeError, REQUEST_LOADER.load, 1)
-        self.assertRaises(TypeError, REQUEST_LOADER.load, [])
+        # Invalid types (note that arguments like '1' or '[]' is of valid
+        # 'type' (but syntax error at a higher level)).  So we need to use
+        # something that is not really JSON nor string.
+        self.assertRaises(TypeError, REQUEST_LOADER.load, b'')
 
         # Incorrect number of arguments
         self.assertRaises(TypeError, REQUEST_LOADER.load,
@@ -113,66 +123,119 @@ class RequestACLTest(unittest.TestCase):
     def test_bad_acl_syntax(self):
         # the following are derived from loader_test.cc
         self.assertRaises(LoaderError, REQUEST_LOADER.load, '{}');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, {});
         self.assertRaises(LoaderError, REQUEST_LOADER.load, '42');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, 42);
         self.assertRaises(LoaderError, REQUEST_LOADER.load, 'true');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, True);
         self.assertRaises(LoaderError, REQUEST_LOADER.load, 'null');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, None);
         self.assertRaises(LoaderError, REQUEST_LOADER.load, '"hello"');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, "hello");
         self.assertRaises(LoaderError, REQUEST_LOADER.load, '[42]');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, [42]);
         self.assertRaises(LoaderError, REQUEST_LOADER.load, '["hello"]');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, ["hello"]);
         self.assertRaises(LoaderError, REQUEST_LOADER.load, '[[]]');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, [[]]);
         self.assertRaises(LoaderError, REQUEST_LOADER.load, '[true]');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, [True]);
         self.assertRaises(LoaderError, REQUEST_LOADER.load, '[null]');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, [None]);
         self.assertRaises(LoaderError, REQUEST_LOADER.load, '[{}]');
+        self.assertRaises(LoaderError, REQUEST_LOADER.load, [{}]);
 
         # the following are derived from dns_test.cc
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
                           '[{"action": "ACCEPT", "bad": "192.0.2.1"}]')
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          [{"action": "ACCEPT", "bad": "192.0.2.1"}])
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
                           '[{"action": "ACCEPT", "from": 4}]')
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          [{"action": "ACCEPT", "from": 4}])
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
                           '[{"action": "ACCEPT", "from": []}]')
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          [{"action": "ACCEPT", "from": []}])
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
                           '[{"action": "ACCEPT", "from": "bad"}]')
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          [{"action": "ACCEPT", "from": "bad"}])
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
                           '[{"action": "ACCEPT", "from": null}]')
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          [{"action": "ACCEPT", "from": None}])
 
     def test_bad_acl_ipsyntax(self):
         # this test is derived from ip_check_unittest.cc
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
                           '[{"action": "DROP", "from": "192.0.2.43/-1"}]')
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
-                          '[{"action": "DROP", "from": "192.0.2.43//1"')
+                          [{"action": "DROP", "from": "192.0.2.43/-1"}])
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          '[{"action": "DROP", "from": "192.0.2.43//1"}]')
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          [{"action": "DROP", "from": "192.0.2.43//1"}])
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          '[{"action": "DROP", "from": "192.0.2.43/1/"}]')
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          [{"action": "DROP", "from": "192.0.2.43/1/"}])
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          '[{"action": "DROP", "from": "/192.0.2.43/1"}]')
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
-                          '[{"action": "DROP", "from": "192.0.2.43/1/"')
+                          [{"action": "DROP", "from": "/192.0.2.43/1"}])
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
-                          '[{"action": "DROP", "from": "/192.0.2.43/1"')
+                          '[{"action": "DROP", "from": "2001:db8::/xxxx"}]')
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
-                          '[{"action": "DROP", "from": "2001:db8::/xxxx"')
+                          [{"action": "DROP", "from": "2001:db8::/xxxx"}])
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
-                          '[{"action": "DROP", "from": "2001:db8::/32/s"')
+                          '[{"action": "DROP", "from": "2001:db8::/32/s"}]')
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
-                          '[{"action": "DROP", "from": "1/"')
+                          [{"action": "DROP", "from": "2001:db8::/32/s"}])
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
-                          '[{"action": "DROP", "from": "/1"')
+                          '[{"action": "DROP", "from": "1/"}]')
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
-                          '[{"action": "DROP", "from": "192.0.2.0/33"')
+                          [{"action": "DROP", "from": "1/"}])
         self.assertRaises(LoaderError, REQUEST_LOADER.load,
-                          '[{"action": "DROP", "from": "::1/129"')
+                          '[{"action": "DROP", "from": "/1"}]')
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          [{"action": "DROP", "from": "/1"}])
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          '[{"action": "DROP", "from": "192.0.2.0/33"}]')
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          [{"action": "DROP", "from": "192.0.2.0/33"}])
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          '[{"action": "DROP", "from": "::1/129"}]')
+        self.assertRaises(LoaderError, REQUEST_LOADER.load,
+                          [{"action": "DROP", "from": "::1/129"}])
 
     def test_execute(self):
         # tests derived from dns_test.cc.  We don't directly expose checks
         # in the python wrapper, so we test it via execute().
         self.assertEqual(ACCEPT, get_acl('192.0.2.1').execute(CONTEXT4))
+        self.assertEqual(ACCEPT, get_acl_json('192.0.2.1').execute(CONTEXT4))
         self.assertEqual(REJECT, get_acl('192.0.2.53').execute(CONTEXT4))
+        self.assertEqual(REJECT, get_acl_json('192.0.2.53').execute(CONTEXT4))
         self.assertEqual(ACCEPT, get_acl('192.0.2.0/24').execute(CONTEXT4))
+        self.assertEqual(ACCEPT, get_acl_json('192.0.2.0/24').execute(CONTEXT4))
         self.assertEqual(REJECT, get_acl('192.0.1.0/24').execute(CONTEXT4))
+        self.assertEqual(REJECT, get_acl_json('192.0.1.0/24').execute(CONTEXT4))
         self.assertEqual(REJECT, get_acl('192.0.1.0/24').execute(CONTEXT4))
+        self.assertEqual(REJECT, get_acl_json('192.0.1.0/24').execute(CONTEXT4))
 
         self.assertEqual(ACCEPT, get_acl('2001:db8::1').execute(CONTEXT6))
+        self.assertEqual(ACCEPT, get_acl_json('2001:db8::1').execute(CONTEXT6))
         self.assertEqual(REJECT, get_acl('2001:db8::53').execute(CONTEXT6))
+        self.assertEqual(REJECT, get_acl_json('2001:db8::53').execute(CONTEXT6))
         self.assertEqual(ACCEPT, get_acl('2001:db8::/64').execute(CONTEXT6))
+        self.assertEqual(ACCEPT,
+                         get_acl_json('2001:db8::/64').execute(CONTEXT6))
         self.assertEqual(REJECT, get_acl('2001:db8:1::/64').execute(CONTEXT6))
+        self.assertEqual(REJECT,
+                         get_acl_json('2001:db8:1::/64').execute(CONTEXT6))
         self.assertEqual(REJECT, get_acl('32.1.13.184').execute(CONTEXT6))
+        self.assertEqual(REJECT, get_acl_json('32.1.13.184').execute(CONTEXT6))
 
         # A bit more complicated example, derived from resolver_config_unittest
         acl = REQUEST_LOADER.load('[ {"action": "ACCEPT", ' +
@@ -187,6 +250,16 @@ class RequestACLTest(unittest.TestCase):
         self.assertEqual(DROP, acl.execute(get_context('2001:db8::1')))
         self.assertEqual(REJECT, acl.execute(get_context('2001:db8::2')))
 
+        # same test using the JSON representation
+        acl = REQUEST_LOADER.load([{"action": "ACCEPT", "from": "192.0.2.1"},
+                                   {"action": "REJECT",
+                                    "from": "192.0.2.0/24"},
+                                   {"action": "DROP", "from": "2001:db8::1"}])
+        self.assertEqual(ACCEPT, acl.execute(CONTEXT4))
+        self.assertEqual(REJECT, acl.execute(get_context('192.0.2.2')))
+        self.assertEqual(DROP, acl.execute(get_context('2001:db8::1')))
+        self.assertEqual(REJECT, acl.execute(get_context('2001:db8::2')))
+
     def test_bad_execute(self):
         acl = get_acl('192.0.2.1')
         # missing parameter