Fix: Inconsistent pagination (#777)

* Fix: Inconsistent pagination
 - include item_hash as a tie-breaker

* Fix: posts tests messages with pagination

Author: aliel

src/aleph/db/accessors/files.py

@@ -170,12 +170,18 @@ def get_address_files_for_api(
     if pagination:
         select_stmt = select_stmt.limit(pagination).offset((page - 1) * pagination)
 
-    order_by = (
-        MessageFilePinDb.created.desc()
-        if sort_order == SortOrder.DESCENDING
-        else MessageFilePinDb.created.asc()
-    )
-    select_stmt = select_stmt.order_by(order_by)
+    if sort_order == SortOrder.DESCENDING:
+        order_by_columns = (
+            MessageFilePinDb.created.desc(),
+            MessageFilePinDb.item_hash.asc(),
+        )
+    else:  # ASCENDING
+        order_by_columns = (
+            MessageFilePinDb.item_hash.asc(),
+            MessageFilePinDb.item_hash.asc(),
+        )
+
+    select_stmt = select_stmt.order_by(*order_by_columns)
 
     return session.execute(select_stmt).all()
 

src/aleph/db/accessors/messages.py

@@ -144,30 +144,37 @@ def make_matching_messages_query(
             select_stmt = select_stmt.where(
                 select_earliest_confirmation.c.height < end_block
             )
+
+        sort_by = SortBy.TX_TIME
         if sort_by == SortBy.TX_TIME:
             order_by_columns = (
                 (
                     nullsfirst(
                         select_earliest_confirmation.c.earliest_confirmation.desc()
                     ),
                     MessageDb.time.desc(),
+                    MessageDb.item_hash.asc(),
                 )
                 if sort_order == SortOrder.DESCENDING
                 else (
                     nullslast(
                         select_earliest_confirmation.c.earliest_confirmation.asc()
                     ),
                     MessageDb.time.asc(),
+                    MessageDb.item_hash.asc(),
                 )
             )
     else:
-        order_by_columns = (
-            (
-                MessageDb.time.desc()
-                if sort_order == SortOrder.DESCENDING
-                else MessageDb.time.asc()
-            ),
-        )
+        if sort_order == SortOrder.DESCENDING:
+            order_by_columns = (
+                MessageDb.time.desc(),
+                MessageDb.item_hash.asc(),
+            )
+        else:  # ASCENDING
+            order_by_columns = (
+                MessageDb.time.asc(),
+                MessageDb.item_hash.asc(),
+            )
 
     select_stmt = select_stmt.order_by(*order_by_columns).offset(
         (page - 1) * pagination
@@ -653,10 +660,20 @@ def make_matching_hashes_query(
     if status:
         select_stmt = select_stmt.where(MessageStatusDb.status == status)
 
-    if sort_order == SortOrder.ASCENDING:
-        select_stmt = select_stmt.order_by(MessageStatusDb.reception_time.asc())
-    else:
-        select_stmt = select_stmt.order_by(MessageStatusDb.reception_time.desc())
+    order_by_columns: Tuple = ()
+
+    if sort_order == SortOrder.DESCENDING:
+        order_by_columns = (
+            MessageStatusDb.reception_time.desc(),
+            MessageStatusDb.item_hash.asc(),
+        )
+    else:  # ASCENDING
+        order_by_columns = (
+            MessageStatusDb.reception_time.asc(),
+            MessageStatusDb.item_hash.asc(),
+        )
+
+    select_stmt = select_stmt.order_by(*order_by_columns)
 
     select_stmt = select_stmt.offset((page - 1) * pagination)
 

src/aleph/db/accessors/posts.py

@@ -256,29 +256,33 @@ def filter_post_select_stmt(
                 isouter=True,
             )
 
-            order_by_columns = (
-                (
+            if sort_order == SortOrder.DESCENDING:
+                order_by_columns = (
                     nullsfirst(
                         select_earliest_confirmation.c.earliest_confirmation.desc()
                     ),
                     select_merged_post_subquery.c.created.desc(),
+                    select_merged_post_subquery.c.item_hash.asc(),
                 )
-                if sort_order == SortOrder.DESCENDING
-                else (
+            else:  # ASCENDING
+                order_by_columns = (
                     nullslast(
                         select_earliest_confirmation.c.earliest_confirmation.asc()
                     ),
                     select_merged_post_subquery.c.created.asc(),
+                    select_merged_post_subquery.c.item_hash.asc(),
                 )
-            )
         else:
-            order_by_columns = (
-                (
-                    last_updated_column.desc()
-                    if sort_order == SortOrder.DESCENDING
-                    else last_updated_column.asc()
-                ),
-            )
+            if sort_order == SortOrder.DESCENDING:
+                order_by_columns = (
+                    last_updated_column.desc(),
+                    select_merged_post_subquery.c.original_item_hash.asc(),
+                )
+            else:  # ASCENDING
+                order_by_columns = (
+                    last_updated_column.asc(),
+                    select_merged_post_subquery.c.original_item_hash.asc(),
+                )
         select_stmt = select_stmt.order_by(*order_by_columns)
 
     # If pagination == 0, return all matching results

src/aleph/web/controllers/accounts.py

@@ -155,7 +155,7 @@ async def get_account_files(request: web.Request) -> web.Response:
             address=address,
             total_size=total_size,
             files=[
-                GetAccountFilesResponseItem.model_validate(file_pin)
+                GetAccountFilesResponseItem.model_validate(dict(file_pin))
                 for file_pin in file_pins
             ],
             pagination_page=query_params.page,

tests/api/test_list_messages.py

@@ -395,6 +395,20 @@ async def fetch_messages_with_pagination_expect_success(
 
 @pytest.mark.asyncio()
 async def test_pagination(fixture_messages, ccn_api_client):
+    """
+    forgotten_messages = list(
+        filter(lambda msg: msg["type"] == "FORGET", fixture_messages)
+    )
+    forgotten_hashes = list(
+        itertools.chain.from_iterable(
+            [msg["content"]["hashes"] for msg in forgotten_messages]
+        )
+    )
+
+    messages_without_forgotten = list(
+        filter(lambda msg: msg["item_hash"] not in forgotten_hashes, fixture_messages)
+    )
+    """
     sorted_messages_by_time = sorted(fixture_messages, key=lambda msg: msg["time"])
 
     # More messages than available
@@ -427,7 +441,7 @@ async def test_pagination(fixture_messages, ccn_api_client):
     )
     assert_messages_equal(messages, sorted_messages_by_time[-1:])
 
-    # Some messages, reverse sort order
+    # Some messages, ascending sort order
     messages = await fetch_messages_with_pagination_expect_success(
         ccn_api_client, page=1, pagination=3, sort_order=1
     )