20640: Fix keyset paging for computed permissions.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

diff --git a/sdk/python/arvados/util.py b/sdk/python/arvados/util.py
index bc87bb83570710c3c4f4e16dbeb2e2c41e9c16c4..35b9338819534955a7dfa05ddb9181c99c04c7d6 100644 (file)
--- a/sdk/python/arvados/util.py
+++ b/sdk/python/arvados/util.py
@@ -402,18 +402,32 @@ def keyset_list_all(
             # In cases where there's more than one record with the
             # same order key, the result could include records we
             # already saw in the last page.  Skip them.
-            if i["uuid"] in seen_prevpage:
+            seen_key = None
+            if "uuid" in i:
+                seen_key = i["uuid"]
+            elif "user_uuid" in i and "target_uuid" in i:
+                # If we are looking at computed_permissions, there is
+                # no uuid field. The primary key is a tuple.
+                seen_key = (i["user_uuid"], i["target_uuid"])
+            if seen_key in seen_prevpage:
                 continue
-            seen_thispage.add(i["uuid"])
+            if seen_key is not None:
+                seen_thispage.add(seen_key)
             yield i
 
         firstitem = items["items"][0]
         lastitem = items["items"][-1]
 
+        tiebreak_key = "uuid"
+        if "uuid" not in lastitem and "target_uuid" in lastitem:
+            # Special case for computed_permissions, where items do
+            # not have a uuid.
+            tiebreak_key = "target_uuid"
+
         if firstitem[order_key] == lastitem[order_key]:
             # Got a page where every item has the same order key.
-            # Switch to using uuid for paging.
-            nextpage = [[order_key, "=", lastitem[order_key]], ["uuid", ">" if ascending else "<", lastitem["uuid"]]]
+            # Switch to using tiebreak key for paging.
+            nextpage = [[order_key, "=", lastitem[order_key]], [tiebreak_key, ">" if ascending else "<", lastitem[tiebreak_key]]]
             prev_page_all_same_order_key = True
         else:
             # Start from the last order key seen, but skip the last
@@ -422,7 +436,9 @@ def keyset_list_all(
             # still likely we'll end up retrieving duplicate rows.
             # That's handled by tracking the "seen" rows for each page
             # so they can be skipped if they show up on the next page.
-            nextpage = [[order_key, ">=" if ascending else "<=", lastitem[order_key]], ["uuid", "!=", lastitem["uuid"]]]
+            nextpage = [[order_key, ">=" if ascending else "<=", lastitem[order_key]]]
+            if tiebreak_key == "uuid":
+                nextpage += [[tiebreak_key, "!=", lastitem[tiebreak_key]]]
             prev_page_all_same_order_key = False
 
 def ca_certs_path(fallback: T=httplib2.CA_CERTS) -> Union[str, T]:

diff --git a/sdk/python/tests/test_computed_permissions.py b/sdk/python/tests/test_computed_permissions.py
index 7f0eee2014a222b855bd5ae81a72fc398449cf71..73043e648e2d6cd444ac58e67fafca64e9ebedbb 100644 (file)
--- a/sdk/python/tests/test_computed_permissions.py
+++ b/sdk/python/tests/test_computed_permissions.py
@@ -3,6 +3,7 @@
 # SPDX-License-Identifier: Apache-2.0
 
 import arvados
+import arvados.util
 from . import run_test_server
 
 class ComputedPermissionTest(run_test_server.TestCaseWithServers):
@@ -16,3 +17,13 @@ class ComputedPermissionTest(run_test_server.TestCaseWithServers):
         assert len(resp['items']) > 0
         for item in resp['items']:
             assert item['user_uuid'] == active_user_uuid
+
+    def test_keyset_list_all(self):
+        run_test_server.authorize_with('admin')
+        api_client = arvados.api('v1')
+        seen = {}
+        for item in arvados.util.keyset_list_all(api_client.computed_permissions().list, order_key='user_uuid'):
+            import sys
+            print(f"{item['user_uuid']} {item['target_uuid']}", file=sys.stderr)
+            assert (item['user_uuid'], item['target_uuid']) not in seen
+            seen[(item['user_uuid'], item['target_uuid'])] = True

diff --git a/services/api/app/models/computed_permission.rb b/services/api/app/models/computed_permission.rb
index af89eadf1feb25a3ff838b5cf44d4965205fb8c3..c89860c48e76aaba886c14b9b4778093c60bcc19 100644 (file)
--- a/services/api/app/models/computed_permission.rb
+++ b/services/api/app/models/computed_permission.rb
@@ -55,4 +55,8 @@ class ComputedPermission < ApplicationRecord
   def self.serialized_attributes
     {}
   end
+
+  def self.unique_columns
+    []
+  end
 end

diff --git a/services/api/lib/load_param.rb b/services/api/lib/load_param.rb
index 9a360c538b6e432e36ad72cfe00d98493af925b6..29f3e35df54c14a93596a2276d07fbe21394108d 100644 (file)
--- a/services/api/lib/load_param.rb
+++ b/services/api/lib/load_param.rb
@@ -93,14 +93,14 @@ module LoadParam
         # The attr can have its table unspecified if it happens to be for the current "model_class" (the first case)
         # or it can be fully specified with the database tablename (the second case) (e.g. "collections.name").
         # NB that the security check for the second case table_name will not work if the model
-        # has used set_table_name to use an alternate table name from the Rails standard.
+        # has used table_name= to use an alternate table name from the Rails standard.
         # I could not find a perfect way to handle this well, but ActiveRecord::Base.send(:descendants)
         # would be a place to start if this ever becomes necessary.
         if (attr.match(/^[a-z][_a-z0-9]+$/) &&
             model_class.columns.collect(&:name).index(attr) &&
             ['asc','desc'].index(direction.downcase))
           if fill_table_names
-            @orders << "#{table_name}.#{attr} #{direction.downcase}"
+            @orders << "#{model_class.table_name}.#{attr} #{direction.downcase}"
           else
             @orders << "#{attr} #{direction.downcase}"
           end