[CONPY-386] named_tuple: rename a column the row type cannot carry instead of refusing the result set

A member name is kept when collections.namedtuple accepts it: a valid identifier, not a keyword, no leading underscore, not already used. Any other name: a duplicate, "COUNT(*)", "def", "_foo", one that is not valid UTF-8 - becomes column_<index>, with _1, _2, ... appended should that name itself be taken. (One rule for the C and the pure-Python implementation)
cursor.description keeps the names as the server sent them.

Author: rusher

include/mariadb_python.h

@@ -387,7 +387,8 @@ extern PyObject *Mariadb_NotSupportedError;
 extern PyObject *Mariadb_Warning;
 
 extern PyObject *decimal_module,
-                *decimal_type;
+                *decimal_type,
+                *keyword_iskeyword;
 
 /* Object types */
 extern PyTypeObject MrdbPool_Type;

mariadb/mariadb.c

@@ -46,6 +46,7 @@ extern int cursor_datetime_init(void);
 
 PyObject *decimal_module= NULL,
          *decimal_type= NULL,
+         *keyword_iskeyword= NULL,
          *socket_module= NULL,
          *indicator_module= NULL;
 extern uint16_t max_pool_size;
@@ -151,6 +152,21 @@ PyMODINIT_FUNC PyInit__mariadb(void)
         goto error;
     }
 
+    {
+        PyObject *keyword_module;
+
+        if (!(keyword_module= PyImport_ImportModule("keyword")))
+        {
+            goto error;
+        }
+        keyword_iskeyword= PyObject_GetAttrString(keyword_module, "iskeyword");
+        Py_DECREF(keyword_module);
+        if (!keyword_iskeyword)
+        {
+            goto error;
+        }
+    }
+
     Py_SET_TYPE(&MrdbCursor_Type, &PyType_Type);
     if (PyType_Ready(&MrdbCursor_Type) == -1)
     {

mariadb/mariadb_cursor.c

@@ -579,6 +579,94 @@ static void MrdbCursor_finalize(MrdbCursor *self)
 }
 /* }}} */
 
+static PyObject *ma_named_tuple_names(MrdbCursor *self)
+{
+    PyObject *names, *seen= NULL;
+    unsigned int i;
+
+    if (!(names= PyList_New(self->field_count)) ||
+        !(seen= PySet_New(NULL)))
+    {
+        goto error;
+    }
+
+    for (i=0; i < self->field_count; i++)
+    {
+        PyObject *name, *res;
+        int keep;
+
+        if (!(name= PyUnicode_DecodeUTF8(self->fields[i].name,
+                                         self->fields[i].name_length, NULL)))
+        {
+            if (!PyErr_ExceptionMatches(PyExc_UnicodeDecodeError))
+            {
+                goto error;
+            }
+            PyErr_Clear();
+            keep= 0;
+        }
+        else if ((keep= PyUnicode_IsIdentifier(name)) > 0)
+        {
+            keep= PyUnicode_AsUTF8(name)[0] != '_';
+        }
+        if (keep > 0)
+        {
+            if (!(res= PyObject_CallFunctionObjArgs(keyword_iskeyword, name,
+                                                    NULL)))
+            {
+                Py_DECREF(name);
+                goto error;
+            }
+            keep= PyObject_Not(res);
+            Py_DECREF(res);
+        }
+        if (keep > 0)
+        {
+            /* already taken by an earlier column? */
+            if ((keep= PySet_Contains(seen, name)) >= 0)
+            {
+                keep= !keep;
+            }
+        }
+        if (keep < 0)
+        {
+            Py_XDECREF(name);
+            goto error;
+        }
+        if (!keep)
+        {
+            unsigned int counter= 0;
+            int taken;
+
+            Py_XDECREF(name);
+            name= PyUnicode_FromFormat("column_%u", i);
+            while (name && (taken= PySet_Contains(seen, name)) > 0)
+            {
+                Py_DECREF(name);
+                name= PyUnicode_FromFormat("column_%u_%u", i, ++counter);
+            }
+            if (!name || taken < 0)
+            {
+                Py_XDECREF(name);
+                goto error;
+            }
+        }
+        if (PySet_Add(seen, name) < 0)
+        {
+            Py_DECREF(name);
+            goto error;
+        }
+        PyList_SET_ITEM(names, i, name);
+    }
+    Py_DECREF(seen);
+    return names;
+
+error:
+    Py_XDECREF(seen);
+    Py_XDECREF(names);
+    return NULL;
+}
+
 static int Mrdb_GetFieldInfo(MrdbCursor *self)
 {
     self->row_number= 0;
@@ -613,61 +701,41 @@ static int Mrdb_GetFieldInfo(MrdbCursor *self)
             unsigned int i;
             PyStructSequence_Desc sequence_desc;
             PyObject *field_names;
-            PyObject *seen;
+            PyObject *names;
             char *p;
             size_t names_size= 0;
 
-            /* All column names are known now: a struct sequence can't carry
-               the same member twice, the second one would silently shadow
-               the first. */
-            if (!(seen= PySet_New(NULL)))
+            /* All column names are known now; the member names are derived
+               from them (see ma_named_tuple_names) and copied into one
+               buffer owned by the row type. */
+            if (!(names= ma_named_tuple_names(self)))
             {
                 return 1;
             }
             for (i=0; i < self->field_count; i++)
             {
-                PyObject *name;
-                int found;
+                Py_ssize_t len;
 
-                if (!(name= PyUnicode_FromString(self->fields[i].name)))
-                {
-                    Py_DECREF(seen);
-                    return 1;
-                }
-                found= PySet_Contains(seen, name);
-                if (found == 0)
-                {
-                    found= PySet_Add(seen, name) < 0 ? -1 : 0;
-                }
-                Py_DECREF(name);
-                if (found)
+                if (!PyUnicode_AsUTF8AndSize(PyList_GET_ITEM(names, i), &len))
                 {
-                    Py_DECREF(seen);
-                    if (found > 0)
-                    {
-                        mariadb_throw_exception(NULL, Mariadb_ProgrammingError,
-                            0, "Duplicate column name '%s' in result set: a "
-                            "named_tuple cursor requires unique column names",
-                            self->fields[i].name);
-                    }
+                    Py_DECREF(names);
                     return 1;
                 }
-            }
-            Py_DECREF(seen);
-
-            for (i=0; i < self->field_count; i++)
-            {
-                names_size+= strlen(self->fields[i].name) + 1;
+                names_size+= (size_t)len + 1;
             }
 
             if (!(field_names= PyBytes_FromStringAndSize(NULL,
                             (Py_ssize_t)names_size)))
+            {
+                Py_DECREF(names);
                 return 1;
+            }
 
             if (!(self->sequence_fields= (PyStructSequence_Field *)
                         PyMem_RawCalloc(self->field_count + 1,
                             sizeof(PyStructSequence_Field))))
             {
+                Py_DECREF(names);
                 Py_DECREF(field_names);
                 return 1;
             }
@@ -680,12 +748,15 @@ static int Mrdb_GetFieldInfo(MrdbCursor *self)
             p= PyBytes_AS_STRING(field_names);
             for (i=0; i < self->field_count; i++)
             {
-                size_t len= strlen(self->fields[i].name) + 1;
+                Py_ssize_t len;
+                const char *name= PyUnicode_AsUTF8AndSize(
+                        PyList_GET_ITEM(names, i), &len);
 
-                memcpy(p, self->fields[i].name, len);
+                memcpy(p, name, (size_t)len + 1);
                 self->sequence_fields[i].name= p;
-                p+= len;
+                p+= len + 1;
             }
+            Py_DECREF(names);
             self->sequence_type= PyStructSequence_NewType(&sequence_desc);
             if (!self->sequence_type)
             {

testing/test/integration/test_cursor_invalid_field_name.py

@@ -1,7 +1,16 @@
 #!/usr/bin/env python -O
 # -*- coding: utf-8 -*-
 
-"""Regression tests for CONPY-380"""
+"""Regression tests for CONPY-380: a column name that is not valid UTF-8.
+
+Building the row type of a named_tuple cursor used to fail on such a name and,
+when the error was caught, a fetch walked a result set whose row buffer was
+never allocated and segfaulted. The name is now treated like any other name a
+named tuple cannot carry: the member is called after its position,
+"column_<index>",
+and the statement succeeds. The undecodable name is only reported when it is
+actually needed as text, i.e. from cursor.description.
+"""
 
 import struct
 import unittest
@@ -21,7 +30,8 @@
 
 # query -> (columns, rows)
 _RESULTS = {
-    BAD_QUERY: ([(BAD_NAME, MYSQL_TYPE_LONG)], [(1,)]),
+    BAD_QUERY: ([(BAD_NAME, MYSQL_TYPE_LONG), ("fine", MYSQL_TYPE_LONG)],
+                [(1, 2)]),
     GOOD_QUERY: ([("answer", MYSQL_TYPE_LONG),
                   ("status", MYSQL_TYPE_VAR_STRING)],
                  [(42, "ok")]),
@@ -67,51 +77,33 @@ def tearDown(self):
         self.server.__exit__(None, None, None)
         self.assertIsNone(self.server.error)
 
-    def _undecodable_column(self, cursor: mariadb.Cursor) -> None:
-        with self.assertRaises(Exception) as ctx:
-            cursor.execute(BAD_QUERY)
-        # a real error, not "returned a result with an exception set"
-        self.assertNotIsInstance(ctx.exception, SystemError)
-
-    def test_fetch_after_failure_reports_no_result_set(self):
-        """Catching the error and fetching anyway used to segfault."""
+    def test_undecodable_name_is_renamed(self):
+        for binary in (False, True):
+            with self.subTest(binary=binary):
+                cursor = self.connection.cursor(named_tuple=True,
+                                                binary=binary)
+                cursor.execute(BAD_QUERY)
+                row = cursor.fetchone()
+                self.assertEqual((row.column_0, row.fine), (1, 2))
+                self.assertIn("column_0=1", repr(row))
+                cursor.close()
+
+    def test_description_reports_the_undecodable_name(self):
+        """The name is only decoded when it is needed as text."""
         cursor = self.connection.cursor(named_tuple=True)
-        try:
-            cursor.execute(BAD_QUERY)
-        except Exception:
-            pass
-
-        with self.assertRaises(Exception) as ctx:
-            cursor.fetchall()
-        self.assertIn("result set", str(ctx.exception))
+        cursor.execute(BAD_QUERY)
+        cursor.fetchall()
+        with self.assertRaises((UnicodeDecodeError, mariadb.DataError)):
+            cursor.description
         cursor.close()
 
-    def test_cursor_usable_after_failure(self):
-        """The cursor must recover for the next statement."""
+    def test_cursor_and_connection_stay_usable(self):
         cursor = self.connection.cursor(named_tuple=True)
-        self._undecodable_column(cursor)
-
+        cursor.execute(BAD_QUERY)
+        cursor.fetchall()
         cursor.execute(GOOD_QUERY)
         row = cursor.fetchall()[0]
-        self.assertEqual(row.answer, 42)
-        self.assertEqual(row.status, "ok")
-        cursor.close()
-
-    def test_cursor_usable_after_failure_binary(self):
-        """Same on the prepared statement path."""
-        cursor = self.connection.cursor(named_tuple=True, binary=True)
-        self._undecodable_column(cursor)
-
-        cursor.execute(GOOD_QUERY)
-        row = cursor.fetchall()[0]
-        self.assertEqual(row.answer, 42)
-        self.assertEqual(row.status, "ok")
-        cursor.close()
-
-    def test_connection_usable_after_failure(self):
-        """The pending result set must be drained, not left on the wire."""
-        cursor = self.connection.cursor(named_tuple=True)
-        self._undecodable_column(cursor)
+        self.assertEqual((row.answer, row.status), (42, "ok"))
         cursor.close()
 
         other = self.connection.cursor()