Merge branch 'named-callproc'

7 files changed, 187 insertions, 31 deletions

diff --git a/NEWS b/NEWS
index 67883d7..6ffa66a 100644
--- a/NEWS
+++ b/NEWS
@@ -24,6 +24,7 @@ New features:
   adapter is deprecated (:tickets:`#317, #343, #387`).
 - Added `~psycopg2.extensions.quote_ident()` function (:ticket:`#359`).
 - Added `~connection.get_dsn_parameters()` connection method (:ticket:`#364`).
+- `~cursor.callproc()` now accepts a dictionary of parameters (:ticket:`#381`).
 
 Other changes:
 
diff --git a/doc/src/cursor.rst b/doc/src/cursor.rst
index 45e6278..aee6b46 100644
--- a/doc/src/cursor.rst
+++ b/doc/src/cursor.rst
@@ -201,13 +201,19 @@ The ``cursor`` class
             
         Call a stored database procedure with the given name. The sequence of
         parameters must contain one entry for each argument that the procedure
-        expects. The result of the call is returned as modified copy of the
-        input sequence. Input parameters are left untouched, output and
-        input/output parameters replaced with possibly new values.
-        
-        The procedure may also provide a result set as output. This must then
-        be made available through the standard |fetch*|_ methods.
+        expects. Overloaded procedures are supported. Named parameters can be
+        used by supplying the parameters as a dictionary.
+
+        This function is, at present, not DBAPI-compliant. The return value is
+        supposed to consist of the sequence of parameters with modified output
+        and input/output parameters. In future versions, the DBAPI-compliant
+        return value may be implemented, but for now the function returns None.
+
+        The procedure may provide a result set as output. This is then made
+        available through the standard |fetch*|_ methods.
 
+        .. versionchanged:: 2.7
+          added support for named arguments.
 
     .. method:: mogrify(operation [, parameters])
 
diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c
index 6b66217..baa5b8f 100644
--- a/psycopg/cursor_type.c
+++ b/psycopg/cursor_type.c
@@ -1025,10 +1025,17 @@ psyco_curs_callproc(cursorObject *self, PyObject *args)
     PyObject *operation = NULL;
     PyObject *res = NULL;
 
-    if (!PyArg_ParseTuple(args, "s#|O",
-          &procname, &procname_len, &parameters
-       ))
-    { goto exit; }
+    int using_dict;
+    PyObject *pname = NULL;
+    PyObject *pnames = NULL;
+    PyObject *pvals = NULL;
+    char *cpname = NULL;
+    char **scpnames = NULL;
+
+    if (!PyArg_ParseTuple(args, "s#|O", &procname, &procname_len,
+                &parameters)) {
+        goto exit;
+    }
 
     EXC_IF_CURS_CLOSED(self);
     EXC_IF_ASYNC_IN_PROGRESS(self, callproc);
@@ -1036,7 +1043,7 @@ psyco_curs_callproc(cursorObject *self, PyObject *args)
 
     if (self->name != NULL) {
         psyco_set_error(ProgrammingError, self,
-                         "can't call .callproc() on named cursors");
+                "can't call .callproc() on named cursors");
         goto exit;
     }
 
@@ -1044,31 +1051,108 @@ psyco_curs_callproc(cursorObject *self, PyObject *args)
         if (-1 == (nparameters = PyObject_Length(parameters))) { goto exit; }
     }
 
-    /* allocate some memory, build the SQL and create a PyString from it */
-    sl = procname_len + 17 + nparameters*3 - (nparameters ? 1 : 0);
-    sql = (char*)PyMem_Malloc(sl);
-    if (sql == NULL) {
-        PyErr_NoMemory();
-        goto exit;
+    using_dict = nparameters > 0 && PyDict_Check(parameters);
+
+    /* a Dict is complicated; the parameter names go into the query */
+    if (using_dict) {
+        if (!(pnames = PyDict_Keys(parameters))) { goto exit; }
+        if (!(pvals = PyDict_Values(parameters))) { goto exit; }
+
+        sl = procname_len + 17 + nparameters * 5 - (nparameters ? 1 : 0);
+
+        if (!(scpnames = PyMem_New(char *, nparameters))) {
+            PyErr_NoMemory();
+            goto exit;
+        }
+
+        memset(scpnames, 0, sizeof(char *) * nparameters);
+
+        /* each parameter has to be processed; it's a few steps. */
+        for (i = 0; i < nparameters; i++) {
+            /* all errors are RuntimeErrors as they should never occur */
+
+            if (!(pname = PyList_GetItem(pnames, i))) { goto exit; }
+            Py_INCREF(pname);   /* was borrowed */
+
+            /* this also makes a check for keys being strings */
+            if (!(pname = psycopg_ensure_bytes(pname))) { goto exit; }
+            if (!(cpname = Bytes_AsString(pname))) { goto exit; }
+
+            if (!(scpnames[i] = psycopg_escape_identifier(
+                    self->conn, cpname, 0))) {
+                Py_CLEAR(pname);
+                goto exit;
+            }
+
+            Py_CLEAR(pname);
+
+            sl += strlen(scpnames[i]);
+        }
+
+        if (!(sql = (char*)PyMem_Malloc(sl))) {
+            PyErr_NoMemory();
+            goto exit;
+        }
+
+        sprintf(sql, "SELECT * FROM %s(", procname);
+        for (i = 0; i < nparameters; i++) {
+            strcat(sql, scpnames[i]);
+            strcat(sql, ":=%s,");
+        }
+        sql[sl-2] = ')';
+        sql[sl-1] = '\0';
     }
 
-    sprintf(sql, "SELECT * FROM %s(", procname);
-    for(i=0; i<nparameters; i++) {
-        strcat(sql, "%s,");
+    /* a list (or None, or empty data structure) is a little bit simpler */
+    else {
+        Py_INCREF(parameters);
+        pvals = parameters;
+
+        sl = procname_len + 17 + nparameters * 3 - (nparameters ? 1 : 0);
+
+        sql = (char*)PyMem_Malloc(sl);
+        if (sql == NULL) {
+            PyErr_NoMemory();
+            goto exit;
+        }
+
+        sprintf(sql, "SELECT * FROM %s(", procname);
+        for (i = 0; i < nparameters; i++) {
+            strcat(sql, "%s,");
+        }
+        sql[sl-2] = ')';
+        sql[sl-1] = '\0';
     }
-    sql[sl-2] = ')';
-    sql[sl-1] = '\0';
 
-    if (!(operation = Bytes_FromString(sql))) { goto exit; }
+    if (!(operation = Bytes_FromString(sql))) {
+        goto exit;
+    }
 
-    if (0 <= _psyco_curs_execute(self, operation, parameters,
-            self->conn->async, 0)) {
-        Py_INCREF(parameters);
-        res = parameters;
+    if (0 <= _psyco_curs_execute(
+            self, operation, pvals, self->conn->async, 0)) {
+        /* The dict case is outside DBAPI scope anyway, so simply return None */
+        if (using_dict) {
+            res = Py_None;
+        }
+        else {
+            res = pvals;
+        }
+        Py_INCREF(res);
     }
 
 exit:
+    if (scpnames != NULL) {
+        for (i = 0; i < nparameters; i++) {
+            if (scpnames[i] != NULL) {
+                PQfreemem(scpnames[i]);
+            }
+        }
+    }
+    PyMem_Del(scpnames);
+    Py_XDECREF(pname);
+    Py_XDECREF(pnames);
     Py_XDECREF(operation);
+    Py_XDECREF(pvals);
     PyMem_Free((void*)sql);
     return res;
 }
diff --git a/psycopg/psycopgmodule.c b/psycopg/psycopgmodule.c
index 012df6b..bf7d908 100644
--- a/psycopg/psycopgmodule.c
+++ b/psycopg/psycopgmodule.c
@@ -62,7 +62,6 @@
 HIDDEN PyObject *pyDateTimeModuleP = NULL;
 
 HIDDEN PyObject *psycoEncodings = NULL;
-
 #ifdef PSYCOPG_DEBUG
 HIDDEN int psycopg_debug_enabled = 0;
 #endif
@@ -191,9 +190,8 @@ psyco_quote_ident(PyObject *self, PyObject *args, PyObject *kwargs)
 
     str = Bytes_AS_STRING(ident);
 
-    quoted = PQescapeIdentifier(conn->pgconn, str, strlen(str));
+    quoted = psycopg_escape_identifier(conn, str, strlen(str));
     if (!quoted) {
-        PyErr_NoMemory();
         goto exit;
     }
     result = conn_text_from_chars(conn, quoted);
diff --git a/psycopg/utils.c b/psycopg/utils.c
index c518cc5..bc6f7be 100644
--- a/psycopg/utils.c
+++ b/psycopg/utils.c
@@ -123,6 +123,7 @@ exit:
     return rv;
 }
 
+
 /* Duplicate a string.
  *
  * Allocate a new buffer on the Python heap containing the new string.
diff --git a/tests/test_cursor.py b/tests/test_cursor.py
index 4aae6b2..fc924c4 100755
--- a/tests/test_cursor.py
+++ b/tests/test_cursor.py
@@ -498,6 +498,48 @@ class CursorTests(ConnectingTestCase):
         cur = self.conn.cursor()
         self.assertRaises(TypeError, cur.callproc, 'lower', 42)
 
+    # It would be inappropriate to test callproc's named parameters in the
+    # DBAPI2.0 test section because they are a psycopg2 extension.
+    @skip_before_postgres(9, 0)
+    def test_callproc_dict(self):
+        # This parameter name tests for injection and quote escaping
+        paramname = '''
+            Robert'); drop table "students" --
+        '''.strip()
+        escaped_paramname = '"%s"' % paramname.replace('"', '""')
+        procname = 'pg_temp.randall'
+
+        cur = self.conn.cursor()
+
+        # Set up the temporary function
+        cur.execute('''
+            CREATE FUNCTION %s(%s INT)
+            RETURNS INT AS
+                'SELECT $1 * $1'
+            LANGUAGE SQL
+        ''' % (procname, escaped_paramname));
+
+        # Make sure callproc works right
+        cur.callproc(procname, { paramname: 2 })
+        self.assertEquals(cur.fetchone()[0], 4)
+
+        # Make sure callproc fails right
+        failing_cases = [
+            ({ paramname: 2, 'foo': 'bar' }, psycopg2.ProgrammingError),
+            ({ paramname: '2' },             psycopg2.ProgrammingError),
+            ({ paramname: 'two' },           psycopg2.ProgrammingError),
+            ({ u'bj\xc3rn': 2 },             psycopg2.ProgrammingError),
+            ({ 3: 2 },                       TypeError),
+            ({ self: 2 },                    TypeError),
+        ]
+        for parameter_sequence, exception in failing_cases:
+            self.assertRaises(exception, cur.callproc, procname, parameter_sequence)
+            self.conn.rollback()
+
+    def test_callproc_badparam(self):
+        cur = self.conn.cursor()
+        self.assertRaises(TypeError, cur.callproc, 'lower', 42)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)
diff --git a/tests/test_psycopg2_dbapi20.py b/tests/test_psycopg2_dbapi20.py
index 80473b7..c780d50 100755
--- a/tests/test_psycopg2_dbapi20.py
+++ b/tests/test_psycopg2_dbapi20.py
@@ -36,7 +36,31 @@ class Psycopg2Tests(dbapi20.DatabaseAPI20Test):
     connect_args = ()
     connect_kw_args = {'dsn': dsn}
 
-    lower_func = 'lower'    # For stored procedure test
+    lower_func = 'lower' # For stored procedure test
+
+    def test_callproc(self):
+        # Until DBAPI 2.0 compliance, callproc should return None or it's just
+        # misleading. Therefore, we will skip the return value test for
+        # callproc and only perform the fetch test.
+        #
+        # For what it's worth, the DBAPI2.0 test_callproc doesn't actually
+        # test for DBAPI2.0 compliance! It doesn't check for modified OUT and
+        # IN/OUT parameters in the return values!
+        con = self._connect()
+        try:
+            cur = con.cursor()
+            if self.lower_func and hasattr(cur,'callproc'):
+                cur.callproc(self.lower_func,('FOO',))
+                r = cur.fetchall()
+                self.assertEqual(len(r),1,'callproc produced no result set')
+                self.assertEqual(len(r[0]),1,
+                    'callproc produced invalid result set'
+                    )
+                self.assertEqual(r[0][0],'foo',
+                    'callproc produced invalid results'
+                    )
+        finally:
+            con.close()
 
     def test_setoutputsize(self):
         # psycopg2's setoutputsize() is a no-op