diff sat_pubsub/pgsql_storage.py @ 451:0e6e176cb572

pgsql: fix items order: Due to a misinterpretation, items where returned in wrong order when RSM was used. Pubsub vanilla `max_items` is reversing the items order to get last items, but when this attribute is not used (RSM being used or not), the items must be in chronological order. This patch fixes it, so RSM returns oldest items by defaut, and empty `<before/>` must be used to get last page (and thus most recent items). Items are always finally ordered chronologically inside the returned page (default by `update` time, but this can be modified when a specific order is requested). Reference field is also fixed to use `updated` instead of `item_id` when item are ordered by `update`.
author Goffi <goffi@goffi.org>
date Sun, 06 Jun 2021 19:42:57 +0200
parents 96059231d58f
children 7f1394bb96db
line wrap: on
line diff
--- a/sat_pubsub/pgsql_storage.py	Thu Apr 22 18:27:03 2021 +0200
+++ b/sat_pubsub/pgsql_storage.py	Sun Jun 06 19:42:57 2021 +0200
@@ -1,5 +1,4 @@
 #!/usr/bin/env python3
-#-*- coding: utf-8 -*-
 
 # Copyright (c) 2012-2021 Jérôme Poisson
 # Copyright (c) 2013-2016 Adrien Cossa
@@ -882,7 +881,7 @@
 
     nodeType = 'leaf'
 
-    def getOrderBy(self, ext_data, direction='DESC'):
+    def getOrderBy(self, ext_data, direction='ASC'):
         """Return ORDER BY clause corresponding to Order By key in ext_data
 
         @param ext_data (dict): extra data as used in getItems
@@ -1139,7 +1138,7 @@
 
         query.extend(query_filters)
 
-        return self.getOrderBy(ext_data)
+        return self.getOrderBy(ext_data, direction="ASC")
 
     def _getItems(self, cursor, authorized_groups, unrestricted, maxItems, ext_data, ids_only):
         self._checkNodeExists(cursor)
@@ -1156,6 +1155,10 @@
             query = ["SELECT data::text,items.access_model,item_id,created,updated"]
 
         query_order = self._appendSourcesAndFilters(query, args, authorized_groups, unrestricted, ext_data)
+        if query_order.startswith("ORDER BY updated"):
+            ref_field = "updated"
+        else:
+            ref_field = "item_id"
 
         if 'rsm' in ext_data:
             rsm = ext_data['rsm']
@@ -1178,29 +1181,48 @@
             elif rsm.before is not None:
                 if rsm.before != '':
                     query.append(
-                        "AND item_id>(SELECT item_id FROM items WHERE node_id=%s AND "
-                        "item=%s LIMIT 1)"
+                        f"AND {ref_field}<(SELECT {ref_field} FROM items WHERE "
+                        "node_id=%s AND item=%s LIMIT 1)"
                     )
                     args.extend((self.nodeDbId, rsm.before))
-                if maxItems is not None:
-                    # if we have maxItems (i.e. a limit), we need to reverse order
-                    # in a first query to get the right items
-                    query.insert(0,"SELECT * from (")
-                    query.append(self.getOrderBy(ext_data, direction='ASC'))
-                    query.append("LIMIT %s) as x")
+                # we need to reverse order in a first query to get the right
+                # items
+                query.insert(0,"SELECT * from (")
+                query.append(self.getOrderBy(ext_data, direction='DESC'))
+                query.append("LIMIT %s) as x")
+                if maxItems is None:
+                    # we default to 10 max when we request last page
+                    log.msg(
+                        "WARNING: getting last page with RSM without max, "
+                        "we default to max=10"
+                    )
+                    args.append(10)
+                else:
                     args.append(maxItems)
             elif rsm.after:
                 query.append(
-                    "AND item_id<(SELECT item_id FROM items WHERE node_id=%s AND item=%s "
-                    "LIMIT 1)"
+                    f"AND {ref_field}>(SELECT {ref_field} FROM items WHERE node_id=%s "
+                    "AND item=%s LIMIT 1)"
                 )
                 args.extend((self.nodeDbId, rsm.after))
+        else:
+            rsm = False
 
         query.append(query_order)
 
         if maxItems is not None:
-            query.append("LIMIT %s")
-            args.append(maxItems)
+            if rsm == False:
+                # if we have legacy maxItems (i.e. not RSM max), we need to
+                # reverse order (maxItems get last items, cf.
+                # https://xmpp.org/extensions/xep-0060.html#subscriber-retrieve-requestrecent)
+                # in a first query to get the right items
+                query.insert(0,"SELECT * from (")
+                query.insert(len(query)-1, self.getOrderBy(ext_data, direction='DESC'))
+                query.insert(len(query)-1, "LIMIT %s) as x")
+                args.append(maxItems)
+            else:
+                query.append("LIMIT %s")
+                args.append(maxItems)
 
         cursor.execute(' '.join(query), args)