Avoid proxy functions in row functions

This streamlines a bit for non-C implementations, however
also adds and tests behavioral contracts that mappings should
not allow integer or slice access and should behave like a
Python mapping in that it raises KeyError for an integer
and TypeError for a slice.   Py3/Py2/C/noC :)

References: #5340
Change-Id: Id3cef452dc8a526b8371c90c5ca2bbb240b25c26

4 files changed, 84 insertions, 32 deletions

diff --git a/lib/sqlalchemy/cextension/resultproxy.c b/lib/sqlalchemy/cextension/resultproxy.c
index 8511d4223..ff6cadac0 100644
--- a/lib/sqlalchemy/cextension/resultproxy.c
+++ b/lib/sqlalchemy/cextension/resultproxy.c
@@ -54,7 +54,7 @@ static PyObject *sqlalchemy_engine_result = NULL;
 
 
 //static int KEY_INTEGER_ONLY = 0;
-//static int KEY_OBJECTS_ONLY = 1;
+static int KEY_OBJECTS_ONLY = 1;
 static int KEY_OBJECTS_BUT_WARN = 2;
 //static int KEY_OBJECTS_NO_WARN = 3;
 
@@ -345,14 +345,24 @@ BaseRow_getitem_by_object(BaseRow *self, PyObject *key, int asmapping)
     long index;
     int key_fallback = 0;
 
-    // if record is non null, it's a borrowed reference
+    // we want to raise TypeError for slice access on a mapping.
+    // Py3 will do this with PyDict_GetItemWithError, Py2 will do it
+    // with PyObject_GetItem.  However in the Python2 case the object
+    // protocol gets in the way for reasons not entirely clear, so
+    // detect slice we have a key error and raise directly.
+
     record = PyDict_GetItem((PyObject *)self->keymap, key);
 
     if (record == NULL) {
+        if (PySlice_Check(key)) {
+            PyErr_Format(PyExc_TypeError, "can't use slices for mapping access");
+            return NULL;
+        }
         record = PyObject_CallMethod(self->parent, "_key_fallback",
                                      "OO", key, Py_None);
         if (record == NULL)
             return NULL;
+
         key_fallback = 1;  // boolean to indicate record is a new reference
     }
 
@@ -408,22 +418,47 @@ BaseRow_subscript_impl(BaseRow *self, PyObject *key, int asmapping)
 
 #if PY_MAJOR_VERSION < 3
     if (PyInt_CheckExact(key)) {
+        if (self->key_style == KEY_OBJECTS_ONLY) {
+            // TODO: being very lazy with error catching here
+            PyErr_Format(PyExc_KeyError, "%s", PyString_AsString(PyObject_Repr(key)));
+            return NULL;
+        }
         index = PyInt_AS_LONG(key);
+
+        // support negative indexes.   We can also call PySequence_GetItem,
+        // but here we can stay with the simpler tuple protocol
+        // rather than the seqeunce protocol which has to check for
+        // __getitem__ methods etc.
         if (index < 0)
-            index += BaseRow_length(self);
+            index += (long)BaseRow_length(self);
         return BaseRow_getitem(self, index);
     } else
 #endif
 
     if (PyLong_CheckExact(key)) {
+        if (self->key_style == KEY_OBJECTS_ONLY) {
+#if PY_MAJOR_VERSION < 3
+            // TODO: being very lazy with error catching here
+            PyErr_Format(PyExc_KeyError, "%s", PyString_AsString(PyObject_Repr(key)));
+#else
+            PyErr_Format(PyExc_KeyError, "%R", key);
+#endif
+            return NULL;
+        }
         index = PyLong_AsLong(key);
-        if ((index == -1) && PyErr_Occurred())
+        if ((index == -1) && PyErr_Occurred() != NULL)
             /* -1 can be either the actual value, or an error flag. */
             return NULL;
+
+        // support negative indexes.   We can also call PySequence_GetItem,
+        // but here we can stay with the simpler tuple protocol
+        // rather than the seqeunce protocol which has to check for
+        // __getitem__ methods etc.
         if (index < 0)
             index += (long)BaseRow_length(self);
         return BaseRow_getitem(self, index);
-    } else if (PySlice_Check(key)) {
+
+    } else if (PySlice_Check(key) && self->key_style != KEY_OBJECTS_ONLY) {
         values = PyObject_GetItem(self->row, key);
         if (values == NULL)
             return NULL;
@@ -669,6 +704,12 @@ static PyMethodDef BaseRow_methods[] = {
     {NULL}  /* Sentinel */
 };
 
+// currently, the sq_item hook is not used by Python except for slices,
+// because we also implement subscript_mapping which seems to intercept
+// integers.  Ideally, when there
+// is a complete separation of "row" from "mapping", we can make
+// two separate types here so that one has only sq_item and the other
+// has only mp_subscript.
 static PySequenceMethods BaseRow_as_sequence = {
     (lenfunc)BaseRow_length,   /* sq_length */
     0,                              /* sq_concat */
diff --git a/lib/sqlalchemy/dialects/mssql/base.py b/lib/sqlalchemy/dialects/mssql/base.py
index f8ed7697a..05c34c171 100644
--- a/lib/sqlalchemy/dialects/mssql/base.py
+++ b/lib/sqlalchemy/dialects/mssql/base.py
@@ -2860,7 +2860,7 @@ class MSDialect(default.DefaultDialect):
         constraint_name = None
         for row in c.mappings():
             if "PRIMARY" in row[TC.c.constraint_type.name]:
-                pkeys.append(row[0])
+                pkeys.append(row["COLUMN_NAME"])
                 if constraint_name is None:
                     constraint_name = row[C.c.constraint_name.name]
         return {"constrained_columns": pkeys, "name": constraint_name}
diff --git a/lib/sqlalchemy/engine/result.py b/lib/sqlalchemy/engine/result.py
index 109ab41fe..ce844eb40 100644
--- a/lib/sqlalchemy/engine/result.py
+++ b/lib/sqlalchemy/engine/result.py
@@ -40,7 +40,7 @@ else:
             operator.methodcaller("_get_by_key_impl_mapping", index)
             for index in indexes
         ]
-        return lambda rec: tuple(getter(rec) for getter in getters)
+        return lambda rec: tuple([getter(rec) for getter in getters])
 
 
 class ResultMetaData(object):
@@ -775,15 +775,19 @@ class Result(InPlaceGenerative):
             uniques, strategy = self._unique_strategy
 
             def filterrows(make_row, rows, strategy, uniques):
+                if strategy:
+                    made_rows = (
+                        (made_row, strategy(made_row))
+                        for made_row in [make_row(row) for row in rows]
+                    )
+                else:
+                    made_rows = (
+                        (made_row, made_row)
+                        for made_row in [make_row(row) for row in rows]
+                    )
                 return [
                     made_row
-                    for made_row, sig_row in [
-                        (
-                            made_row,
-                            strategy(made_row) if strategy else made_row,
-                        )
-                        for made_row in [make_row(row) for row in rows]
-                    ]
+                    for made_row, sig_row in made_rows
                     if sig_row not in uniques and not uniques.add(sig_row)
                 ]
 
diff --git a/lib/sqlalchemy/engine/row.py b/lib/sqlalchemy/engine/row.py
index d279776ce..70f45c82c 100644
--- a/lib/sqlalchemy/engine/row.py
+++ b/lib/sqlalchemy/engine/row.py
@@ -103,16 +103,19 @@ except ImportError:
         def __getitem__(self, key):
             return self._data[key]
 
-        def _subscript_impl(self, key, ismapping):
+        def _get_by_key_impl(self, key):
+            if self._key_style == KEY_INTEGER_ONLY:
+                return self._data[key]
+
+            # the following is all LegacyRow support.   none of this
+            # should be called if not LegacyRow
+            # assert isinstance(self, LegacyRow)
+
             try:
                 rec = self._keymap[key]
             except KeyError as ke:
                 rec = self._parent._key_fallback(key, ke)
             except TypeError:
-                # the non-C version detects a slice using TypeError.
-                # this is pretty inefficient for the slice use case
-                # but is more efficient for the integer use case since we
-                # don't have to check it up front.
                 if isinstance(key, slice):
                     return tuple(self._data[key])
                 else:
@@ -124,7 +127,6 @@ except ImportError:
 
             elif (
                 self._key_style == KEY_OBJECTS_BUT_WARN
-                and not ismapping
                 and mdindex != key
                 and not isinstance(key, int)
             ):
@@ -132,14 +134,22 @@ except ImportError:
 
             return self._data[mdindex]
 
-        def _get_by_key_impl(self, key):
-            return self._subscript_impl(key, False)
-
         def _get_by_key_impl_mapping(self, key):
-            # the C code has two different methods so that we can distinguish
-            # between tuple-like keys (integers, slices) and mapping-like keys
-            # (strings, objects)
-            return self._subscript_impl(key, True)
+            try:
+                rec = self._keymap[key]
+            except KeyError as ke:
+                rec = self._parent._key_fallback(key, ke)
+
+            mdindex = rec[MD_INDEX]
+            if mdindex is None:
+                self._parent._raise_for_ambiguous_column_name(rec)
+            elif (
+                self._key_style == KEY_OBJECTS_ONLY
+                and int in key.__class__.__mro__
+            ):
+                raise KeyError(key)
+
+            return self._data[mdindex]
 
         def __getattr__(self, name):
             try:
@@ -348,9 +358,7 @@ class LegacyRow(Row):
         return self._parent._contains(key, self)
 
     if not _baserow_usecext:
-
-        def __getitem__(self, key):
-            return self._get_by_key_impl(key)
+        __getitem__ = BaseRow._get_by_key_impl
 
     @util.deprecated(
         "1.4",
@@ -510,8 +518,7 @@ class RowMapping(BaseRow, collections_abc.Mapping):
 
     if not _baserow_usecext:
 
-        def __getitem__(self, key):
-            return self._get_by_key_impl(key)
+        __getitem__ = BaseRow._get_by_key_impl_mapping
 
         def _values_impl(self):
             return list(self._data)