Repository: djrobstep/sqlakeyset (python)

Issue:
Using the SQLAlchemy 2 native UUID as the column type breaks cursor pagination
Using the sa.UUID as the column type breaks the pagination:

```
sqlalchemy.exc.StatementError: (builtins.AttributeError) 'str' object has no attribute 'hex'
```

The issue happens in the `pair_for_comparison` function when it applies the preprocessing: 
```
value = compval.type.bind_processor(dialect)(value)
```
resulting in the UUID being converted into a str prematurely.  
It must pass the uuid.UUID value directly when using the sa.UUID as the column type.

Commenting out the line `value = compval.type.bind_processor(dialect)(value)` resolves the issue.

Pull request:
Fix double bind processing in certain cases
Resolves #104.

When order clauses had only a single column OR the database backend did not support native tuples, bind param processing was being applied twice - once by us and once by sqlalchemy.

Patch:
diff --git a/sqlakeyset/columns.py b/sqlakeyset/columns.py
index 5491e45..b0ab2b8 100644
--- a/sqlakeyset/columns.py
+++ b/sqlakeyset/columns.py
@@ -111,7 +111,7 @@ class OC:
             raise ValueError  # pragma: no cover
         return OC(new_uo)
 
-    def pair_for_comparison(self, value, dialect):
+    def pair_for_comparison(self, value, dialect, apply_bind_processor: bool = False):
         """Return a pair of SQL expressions representing comparable values for
         this ordering column and a specified value.
 
@@ -122,12 +122,13 @@ class OC:
             condition for the value of this OC being past `value` in the paging
             order."""
         compval = self.comparable_value
-        # If this OC is a column with a custom type, apply the custom
-        # preprocessing to the comparsion value:
-        try:
-            value = compval.type.bind_processor(dialect)(value)  # type: ignore
-        except (TypeError, AttributeError):
-            pass
+        if apply_bind_processor:
+            # If this OC is a column with a custom type, apply the custom
+            # preprocessing to the comparison value:
+            try:
+                value = compval.type.bind_processor(dialect)(value)  # type: ignore
+            except (TypeError, AttributeError):
+                pass
         if self.is_ascending:
             return compval, value
         else:
diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py
index e82c738..f36e36f 100644
--- a/sqlakeyset/paging.py
+++ b/sqlakeyset/paging.py
@@ -52,22 +52,15 @@ PER_PAGE_DEFAULT = 10
 SUPPORTS_NATIVE_TUPLE_COMPARISON = ("postgresql", "mysql", "sqlite")
 
 
-def compare_tuples(
-    lesser: Sequence, greater: Sequence, dialect: Optional[Dialect] = None
-) -> ColumnElement[bool]:
+def compare_tuples(lesser: Sequence, greater: Sequence) -> ColumnElement[bool]:
     """Given two sequences of equal length (whose entries can be SQL clauses or
-    simple values), create an SQL clause defining the lexicographic tuple
-    comparison ``lesser < greater``.
-
-    If ``dialect`` is provided and is an sqlalchemy SQL dialect supporting
-    native tuple comparison, the SQL emitted is a native tuple comparison.
-    Otherwise it is built manually using OR and AND."""
+    simple values), create a simple SQL clause equivalent to the lexicographic
+    tuple comparison ``lesser < greater``."""
     if len(lesser) != len(greater):
         raise ValueError("Tuples must have same length to be compared!")
-    if len(lesser) == 1:
-        return lesser[0] < greater[0]
-    if dialect is not None and dialect.name.lower() in SUPPORTS_NATIVE_TUPLE_COMPARISON:
-        return tuple_(*lesser) < tuple_(*greater)
+
+    # This monster comprehension is just the long way of writing the
+    # lexicographic comparison lesser < greater
     return or_(
         *[
             and_(
@@ -98,11 +91,21 @@ def where_condition_for_page(
             "Page marker has different column count to query's order clause"
         )
 
+    native_tuple = (
+        len(place) > 1 and dialect.name.lower() in SUPPORTS_NATIVE_TUPLE_COMPARISON
+    )
+
     zipped = zip(ordering_columns, place)
-    swapped = [c.pair_for_comparison(value, dialect) for c, value in zipped]
+    swapped = [
+        c.pair_for_comparison(value, dialect, apply_bind_processor=native_tuple)
+        for c, value in zipped
+    ]
     row, place_row = zip(*swapped)
 
-    return compare_tuples(greater=row, lesser=place_row, dialect=dialect)
+    if native_tuple:
+        return tuple_(*place_row) < tuple_(*row)
+    else:
+        return compare_tuples(lesser=place_row, greater=row)
 
 
 class _PagingQuery(NamedTuple):


Test patch:
diff --git a/sqlakeyset/py.typed b/sqlakeyset/py.typed
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/conftest.py b/tests/conftest.py
index 8136337..9da2320 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2,6 +2,7 @@ import enum
 from datetime import timedelta
 from functools import partial
 from random import randrange
+import uuid
 
 import arrow
 import pytest
@@ -43,6 +44,23 @@ else:
     select = _select
     S = partial(_S, future=True)
 
+if SQLA_VERSION >= version.parse("2.0"):
+    from sqlalchemy.types import Uuid
+else:
+    # On older sqlalchemy we don't run the uuid tests anyway, so this is good enough
+    class Uuid(TypeDecorator):
+        impl = String
+        cache_ok = True
+
+        def process_result_value(self, value, dialect):
+            return uuid.UUID(hex=value) if value is not None else None
+
+        def process_bind_param(self, value, dialect):
+            return value.hex if value is not None else None
+
+        def __init__(self):
+            super().__init__(32)
+
 
 class Base(declarative_base()):
     __abstract__ = True
@@ -51,7 +69,7 @@ class Base(declarative_base()):
         return {k: getattr(self, k) for k, v in self.__mapper__.columns.items()}
 
     def __eq__(self, other):
-        return type(self) == type(other) and self._as_dict() == other._as_dict()
+        return type(self) is type(other) and self._as_dict() == other._as_dict()
 
     def __repr__(self):
         try:
@@ -75,30 +93,57 @@ def randtime():
 
 
 # Custom type to guard against double processing: see issue #47
-class MyInteger(float):
-    pass
+class MyInteger:
+    val: int
+
+    def __init__(self, val: int):
+        self.val = val
 
+    def __eq__(self, other):
+        if isinstance(other, MyInteger):
+            return self.val == other.val
+        return other == self  # for SQL
 
-custom_bookmark_type(MyInteger, "mi")
+    def __hash__(self):
+        return hash(self.val)
 
+    def __str__(self):
+        return str(self.val)
+
+    def __repr__(self):
+        return f"MyInteger({self.val})"
 
-class DoubleResultProcessing(Exception):
+
+custom_bookmark_type(
+    MyInteger,
+    "mi",
+    serializer=lambda mi: str(mi.val),
+    deserializer=lambda s: MyInteger(int(s)),
+)
+
+
+class DoubleProcessing(Exception):
     pass
 
 
-class GuardDoubleResultProcessing(TypeDecorator):
+class GuardDoubleProcessing(TypeDecorator):
     impl = Integer
     cache_ok = True
 
     def process_result_value(self, value, dialect):
         if isinstance(value, MyInteger):
-            raise DoubleResultProcessing(
-                "Result processor was called on an already processed value!"
+            raise DoubleProcessing(
+                f"Result processor was called on an already processed value: {value!r}"
             )
+        assert isinstance(value, int)
         return MyInteger(value)
 
     def process_bind_param(self, value, dialect):
-        return float(value)
+        if isinstance(value, int):
+            raise DoubleProcessing(
+                f"Bind processor was called on an already processed value: {value!r} is not a MyInteger"
+            )
+        return value.val
 
 
 class Colour(enum.Enum):
@@ -120,7 +165,7 @@ class Light(Base):
     id = Column(Integer, primary_key=True)
     intensity = Column(Integer, nullable=False)
     colour = Column(Enum(Colour), nullable=False)
-    myint = Column(GuardDoubleResultProcessing, nullable=False)
+    myint = Column(GuardDoubleProcessing, nullable=False)
 
 
 class Book(Base):
@@ -222,6 +267,11 @@ class Mammal(Vertebrate):
     }
 
 
+class Ticket(Base):
+    __tablename__ = "ticket"
+    id = Column(Uuid, primary_key=True)
+
+
 def _dburl(request):
     count = 10
     data = []
@@ -255,11 +305,14 @@ def _dburl(request):
             data.append(b)
 
     data += [
-        Light(colour=Colour(i % 3), intensity=(i * 13) % 53, myint=i) for i in range(99)
+        Light(colour=Colour(i % 3), intensity=(i * 13) % 53, myint=MyInteger(i))
+        for i in range(99)
     ]
 
     widgets = [dict(name=f"widget {i}") for i in range(99)]
 
+    data += [Ticket(id=uuid.uuid4()) for _ in range(100)]
+
     with temporary_database(request.param, host="localhost") as dburl:
         with S(dburl) as s:
             Base.metadata.create_all(s.connection())
diff --git a/tests/test_paging.py b/tests/test_paging.py
index 1c1d30c..89485b0 100644
--- a/tests/test_paging.py
+++ b/tests/test_paging.py
@@ -462,6 +462,10 @@ def test_core_result_processor(dburl):
 
 def test_core_order_by_result_processor(dburl):
     with S(dburl, echo=ECHO) as s:
+        # Check both 1-col and multicol ordering clauses (see #104):
+        selectable = select(Light.id).order_by(Light.myint)
+        check_paging_core(selectable=selectable, s=s)
+
         selectable = select(Light.id).order_by(Light.myint, Light.id)
         check_paging_core(selectable=selectable, s=s)
 
diff --git a/tests/test_paging_20_style.py b/tests/test_paging_20_style.py
index 9c16350..b86d93b 100644
--- a/tests/test_paging_20_style.py
+++ b/tests/test_paging_20_style.py
@@ -8,6 +8,7 @@ from conftest import (
     SQLA_VERSION,
     Author,
     Book,
+    Ticket,
     S,
 )
 from test_paging import check_paging_core
@@ -18,6 +19,12 @@ if SQLA_VERSION < version.parse("1.4.0b1"):
     )
 
 
+def test_uuid(dburl):
+    with S(dburl, echo=ECHO) as s:
+        q = select(Ticket).order_by(Ticket.id)
+        check_paging_core(q, s)
+
+
 def test_new_orm_query1(dburl):
     spec = [desc(Book.b), Book.d, Book.id]