From 28069bc181980cd564d457d27ceb9f25adb2fe6e Mon Sep 17 00:00:00 2001 From: aliel Date: Thu, 17 Apr 2025 11:59:33 +0200 Subject: [PATCH 1/2] Fix: Inconsistent pagination - include item_hash as a tie-breaker --- src/aleph/db/accessors/files.py | 14 +++++++++----- src/aleph/db/accessors/messages.py | 25 ++++++++++++++++++++----- src/aleph/db/accessors/posts.py | 14 ++++++++++---- src/aleph/web/controllers/accounts.py | 2 +- 4 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/aleph/db/accessors/files.py b/src/aleph/db/accessors/files.py index 7b0f036f5..5df4df955 100644 --- a/src/aleph/db/accessors/files.py +++ b/src/aleph/db/accessors/files.py @@ -170,12 +170,16 @@ 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() + order_by_columns = ( + MessageFilePinDb.created.desc(), + ( + MessageFilePinDb.item_hash.asc() + if sort_order == SortOrder.DESCENDING + else MessageFilePinDb.created.asc() + ), + MessageFilePinDb.item_hash.asc(), ) - select_stmt = select_stmt.order_by(order_by) + select_stmt = select_stmt.order_by(*order_by_columns) return session.execute(select_stmt).all() diff --git a/src/aleph/db/accessors/messages.py b/src/aleph/db/accessors/messages.py index edf4b201c..d3a85d18b 100644 --- a/src/aleph/db/accessors/messages.py +++ b/src/aleph/db/accessors/messages.py @@ -144,6 +144,8 @@ 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 = ( ( @@ -151,6 +153,7 @@ def make_matching_messages_query( select_earliest_confirmation.c.earliest_confirmation.desc() ), MessageDb.time.desc(), + MessageDb.item_hash.asc(), ) if sort_order == SortOrder.DESCENDING else ( @@ -158,15 +161,18 @@ def make_matching_messages_query( select_earliest_confirmation.c.earliest_confirmation.asc() ), MessageDb.time.asc(), + MessageDb.item_hash.asc(), ) ) else: order_by_columns = ( + MessageDb.time.desc(), ( - MessageDb.time.desc() + MessageDb.item_hash.asc() if sort_order == SortOrder.DESCENDING else MessageDb.time.asc() ), + MessageDb.item_hash.asc(), ) select_stmt = select_stmt.order_by(*order_by_columns).offset( @@ -653,10 +659,19 @@ 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 = () + + order_by_columns = ( + MessageStatusDb.reception_time.asc(), + ( + MessageStatusDb.item_hash.asc() + if sort_order == SortOrder.ASCENDING + else MessageStatusDb.reception_time.desc() + ), + MessageStatusDb.item_hash.asc(), + ) + + select_stmt = select_stmt.order_by(*order_by_columns) select_stmt = select_stmt.offset((page - 1) * pagination) diff --git a/src/aleph/db/accessors/posts.py b/src/aleph/db/accessors/posts.py index 655339add..e7f65aa14 100644 --- a/src/aleph/db/accessors/posts.py +++ b/src/aleph/db/accessors/posts.py @@ -262,6 +262,7 @@ def filter_post_select_stmt( 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 ( @@ -269,15 +270,20 @@ def filter_post_select_stmt( 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() - ), + last_updated_column.desc(), + select_merged_post_subquery.c.original_item_hash.asc(), + ) + if sort_order == SortOrder.DESCENDING + else ( + last_updated_column.asc(), + select_merged_post_subquery.c.original_item_hash.asc(), + ) ) select_stmt = select_stmt.order_by(*order_by_columns) diff --git a/src/aleph/web/controllers/accounts.py b/src/aleph/web/controllers/accounts.py index 487f6d0cf..5f7c129f2 100644 --- a/src/aleph/web/controllers/accounts.py +++ b/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, From 84a6f8b57f987285f4e76fe925c8234f133382a4 Mon Sep 17 00:00:00 2001 From: aliel Date: Thu, 17 Apr 2025 17:08:22 +0200 Subject: [PATCH 2/2] Fix: posts tests messages with pagination --- src/aleph/db/accessors/files.py | 20 +++++++++------- src/aleph/db/accessors/messages.py | 38 ++++++++++++++++-------------- src/aleph/db/accessors/posts.py | 18 +++++++------- tests/api/test_list_messages.py | 16 ++++++++++++- 4 files changed, 54 insertions(+), 38 deletions(-) diff --git a/src/aleph/db/accessors/files.py b/src/aleph/db/accessors/files.py index 5df4df955..da7f51156 100644 --- a/src/aleph/db/accessors/files.py +++ b/src/aleph/db/accessors/files.py @@ -170,15 +170,17 @@ def get_address_files_for_api( if pagination: select_stmt = select_stmt.limit(pagination).offset((page - 1) * pagination) - order_by_columns = ( - MessageFilePinDb.created.desc(), - ( - MessageFilePinDb.item_hash.asc() - if sort_order == SortOrder.DESCENDING - else MessageFilePinDb.created.asc() - ), - MessageFilePinDb.item_hash.asc(), - ) + 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() diff --git a/src/aleph/db/accessors/messages.py b/src/aleph/db/accessors/messages.py index d3a85d18b..56e0790ea 100644 --- a/src/aleph/db/accessors/messages.py +++ b/src/aleph/db/accessors/messages.py @@ -165,15 +165,16 @@ def make_matching_messages_query( ) ) else: - order_by_columns = ( - MessageDb.time.desc(), - ( - MessageDb.item_hash.asc() - if sort_order == SortOrder.DESCENDING - else MessageDb.time.asc() - ), - MessageDb.item_hash.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 @@ -661,15 +662,16 @@ def make_matching_hashes_query( order_by_columns: Tuple = () - order_by_columns = ( - MessageStatusDb.reception_time.asc(), - ( - MessageStatusDb.item_hash.asc() - if sort_order == SortOrder.ASCENDING - else MessageStatusDb.reception_time.desc() - ), - MessageStatusDb.item_hash.asc(), - ) + 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) diff --git a/src/aleph/db/accessors/posts.py b/src/aleph/db/accessors/posts.py index e7f65aa14..cc1b8f57f 100644 --- a/src/aleph/db/accessors/posts.py +++ b/src/aleph/db/accessors/posts.py @@ -256,35 +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 = ( - ( + if sort_order == SortOrder.DESCENDING: + order_by_columns = ( last_updated_column.desc(), select_merged_post_subquery.c.original_item_hash.asc(), ) - if sort_order == SortOrder.DESCENDING - else ( + 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 diff --git a/tests/api/test_list_messages.py b/tests/api/test_list_messages.py index 73bb6b282..0db1bc379 100644 --- a/tests/api/test_list_messages.py +++ b/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 )