[ARVADOS] updated: 1.3.0-2532-ge77609d17

Git user git at public.arvados.org
Mon May 4 21:32:48 UTC 2020


Summary of changes:
 services/api/app/models/arvados_model.rb           |  8 +-
 services/api/app/models/group.rb                   | 17 +++++
 services/api/app/models/user.rb                    |  8 +-
 .../db/migrate/20200501150153_permission_table.rb  | 85 +++++++++++++++++-----
 services/api/lib/refresh_permission_view.rb        |  1 +
 .../arvados/v1/groups_controller_test.rb           | 14 +++-
 services/api/test/test_helper.rb                   |  5 ++
 7 files changed, 109 insertions(+), 29 deletions(-)

       via  e77609d17994548cd5abed7a0d25517c10c73327 (commit)
       via  64fc7012a9e0c28e441415759d780e1a78b26e67 (commit)
       via  96cc61430cdaa88983365ebaf87cbca4e396272d (commit)
       via  12e896874595d4b78ec61f931772c391e8c7973d (commit)
       via  5f915857cbb3620587f321514a065a73fd6ecc46 (commit)
      from  dbd795d39343d2c91db1602603002b2a604f1947 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit e77609d17994548cd5abed7a0d25517c10c73327
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Mon May 4 17:27:15 2020 -0400

    16007: Remember to set trash_at in trashed_groups.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index f64a5b259..551201773 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -42,10 +42,12 @@ class Group < ArvadosModel
     if is_trashed_changed? or owner_uuid_changed?
       if is_trashed == true
         ActiveRecord::Base.connection.exec_query %{
-insert into trashed_groups (group_uuid) select * from project_subtree($1);
+insert into trashed_groups (group_uuid, trash_at)
+  select target_uuid as group_uuid, $2 as trash_at from project_subtree($1);
 },
                                                  'Group.trash_subtree',
-                                                 [[nil, self.uuid]]
+                                                 [[nil, self.uuid],
+                                                  [nil, self.trash_at]]
       elsif is_trashed == false && TrashedGroup.find_by_group_uuid(self.owner_uuid).nil?
         ActiveRecord::Base.connection.exec_query %{
 delete from trashed_groups where group_uuid in (select * from project_subtree_notrash($1));

commit 64fc7012a9e0c28e441415759d780e1a78b26e67
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Mon May 4 17:13:24 2020 -0400

    16007: Update trashed_groups incrementally
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 531a2bbb9..f64a5b259 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -39,7 +39,7 @@ class Group < ArvadosModel
   end
 
   def maybe_invalidate_permissions_cache
-    if is_trashed_changed?
+    if is_trashed_changed? or owner_uuid_changed?
       if is_trashed == true
         ActiveRecord::Base.connection.exec_query %{
 insert into trashed_groups (group_uuid) select * from project_subtree($1);
diff --git a/services/api/lib/refresh_permission_view.rb b/services/api/lib/refresh_permission_view.rb
index 919079667..5ee8a7b44 100644
--- a/services/api/lib/refresh_permission_view.rb
+++ b/services/api/lib/refresh_permission_view.rb
@@ -10,8 +10,6 @@ def do_refresh_permission_view
     ActiveRecord::Base.connection.execute("LOCK TABLE permission_refresh_lock")
     ActiveRecord::Base.connection.execute("DELETE FROM #{PERMISSION_VIEW}")
     ActiveRecord::Base.connection.execute("INSERT INTO #{PERMISSION_VIEW} select * from compute_permission_table()")
-    ActiveRecord::Base.connection.execute("DELETE FROM #{TRASHED_GROUPS}")
-    ActiveRecord::Base.connection.execute("INSERT INTO #{TRASHED_GROUPS} select * from compute_trashed()")
   end
 end
 
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 5747a85cf..a5bd142b9 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -60,6 +60,11 @@ class ActiveSupport::TestCase
   include ArvadosTestSupport
   include CurrentApiClient
 
+  setup do
+    ActiveRecord::Base.connection.execute("DELETE FROM #{TRASHED_GROUPS}")
+    ActiveRecord::Base.connection.execute("INSERT INTO #{TRASHED_GROUPS} select * from compute_trashed()")
+  end
+
   teardown do
     Thread.current[:api_client_ip_address] = nil
     Thread.current[:api_client_authorization] = nil

commit 96cc61430cdaa88983365ebaf87cbca4e396272d
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Mon May 4 16:59:06 2020 -0400

    16007: Check trash_at timestamp
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 12f729813..83b9a6e10 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -298,14 +298,14 @@ class ArvadosModel < ApplicationRecord
           # Only include records where the owner is not trashed
           #sql_conds = "#{sql_table}.owner_uuid NOT IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+
           #            "WHERE trashed = 1) #{exclude_trashed_records}"
-          sql_conds = "#{sql_table}.owner_uuid NOT IN (SELECT * FROM trashed_groups) #{exclude_trashed_records}"
+          sql_conds = "#{sql_table}.owner_uuid NOT IN (SELECT group_uuid FROM trashed_groups where trash_at < clock_timestamp()) #{exclude_trashed_records}"
         end
       end
     else
       trashed_check = ""
       if !include_trash then
         #trashed_check = "AND trashed = 0"
-        trashed_check = "AND target_uuid NOT IN (SELECT * FROM trashed_groups)"
+        trashed_check = "AND target_uuid NOT IN (SELECT group_uuid FROM trashed_groups where trash_at < clock_timestamp())"
       end
 
       # Note: it is possible to combine the direct_check and
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index dbe2d4ed5..531a2bbb9 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -42,13 +42,13 @@ class Group < ArvadosModel
     if is_trashed_changed?
       if is_trashed == true
         ActiveRecord::Base.connection.exec_query %{
-insert into trashed_groups (uuid) select * from project_subtree($1);
+insert into trashed_groups (group_uuid) select * from project_subtree($1);
 },
                                                  'Group.trash_subtree',
                                                  [[nil, self.uuid]]
-      elsif is_trashed == false && TrashedGroup.find_by_uuid(self.owner_uuid).nil?
+      elsif is_trashed == false && TrashedGroup.find_by_group_uuid(self.owner_uuid).nil?
         ActiveRecord::Base.connection.exec_query %{
-delete from trashed_groups where uuid in (select * from project_subtree_notrash($1));
+delete from trashed_groups where group_uuid in (select * from project_subtree_notrash($1));
 },
                               'Group.untrash_subtree',
                               [[nil, self.uuid]]
diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb
index e1d0ab1b3..9bd11c2c1 100644
--- a/services/api/db/migrate/20200501150153_permission_table.rb
+++ b/services/api/db/migrate/20200501150153_permission_table.rb
@@ -97,16 +97,18 @@ $$;
 }
 
     create_table :trashed_groups, :id => false do |t|
-      t.string :uuid
+      t.string :group_uuid
+      t.datetime :trash_at
     end
+    add_index :trashed_groups, :group_uuid
 
         ActiveRecord::Base.connection.execute %{
 create or replace function compute_trashed ()
-returns table (uuid varchar(27))
+returns table (uuid varchar(27), trash_at timestamp)
 STABLE
 language SQL
 as $$
-select ps.target_uuid from groups,
+select ps.target_uuid as group_uuid, groups.trash_at from groups,
   lateral project_subtree(groups.uuid) ps
   where groups.is_trashed = true
 $$;
@@ -118,5 +120,10 @@ $$;
   def down
     drop_table :materialized_permissions
     drop_table :trashed_groups
+
+    ActiveRecord::Base.connection.execute "DROP function compute_permission_table ();"
+    ActiveRecord::Base.connection.execute "DROP function project_subtree (varchar(27));"
+    ActiveRecord::Base.connection.execute "DROP function project_subtree_notrash (varchar(27));"
+    ActiveRecord::Base.connection.execute "DROP function compute_trashed ();"
   end
 end

commit 12e896874595d4b78ec61f931772c391e8c7973d
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Mon May 4 16:34:24 2020 -0400

    16007: Remove 'trashed' from permission computation
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index dd447ca51..a5fc12d0a 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -155,12 +155,12 @@ class User < ArvadosModel
   def self.all_group_permissions
     all_perms = {}
     ActiveRecord::Base.connection.
-      exec_query("SELECT user_uuid, target_owner_uuid, perm_level, trashed
+      exec_query("SELECT user_uuid, target_owner_uuid, perm_level
                   FROM #{PERMISSION_VIEW}
                   WHERE target_owner_uuid IS NOT NULL",
                   # "name" arg is a query label that appears in logs:
                   "all_group_permissions",
-                  ).rows.each do |user_uuid, group_uuid, max_p_val, trashed|
+                  ).rows.each do |user_uuid, group_uuid, max_p_val|
       all_perms[user_uuid] ||= {}
       all_perms[user_uuid][group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
     end
@@ -173,7 +173,7 @@ class User < ArvadosModel
   def group_permissions
     group_perms = {self.uuid => {:read => true, :write => true, :manage => true}}
     ActiveRecord::Base.connection.
-      exec_query("SELECT target_owner_uuid, perm_level, trashed
+      exec_query("SELECT target_owner_uuid, perm_level
                   FROM #{PERMISSION_VIEW}
                   WHERE user_uuid = $1
                   AND target_owner_uuid IS NOT NULL",
@@ -181,7 +181,7 @@ class User < ArvadosModel
                   "group_permissions for #{uuid}",
                   # "binds" arg is an array of [col_id, value] for '$1' vars:
                   [[nil, uuid]],
-                ).rows.each do |group_uuid, max_p_val, trashed|
+                ).rows.each do |group_uuid, max_p_val|
       group_perms[group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
     end
     group_perms
diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb
index 87c8a4cb8..e1d0ab1b3 100644
--- a/services/api/db/migrate/20200501150153_permission_table.rb
+++ b/services/api/db/migrate/20200501150153_permission_table.rb
@@ -5,7 +5,6 @@ class PermissionTable < ActiveRecord::Migration[5.0]
       t.string :target_uuid
       t.integer :perm_level
       t.string :target_owner_uuid
-      t.integer :trashed
     end
 
     ActiveRecord::Base.connection.execute %{
@@ -13,20 +12,17 @@ create or replace function compute_permission_table ()
 returns table(user_uuid character varying (27),
               target_uuid character varying (27),
               perm_level smallint,
-              target_owner_uuid character varying(27),
-              trashed smallint)
+              target_owner_uuid character varying(27))
 VOLATILE
 language SQL
 as $$
  WITH RECURSIVE perm_value(name, val) AS (
          VALUES ('can_read'::text,(1)::smallint), ('can_login'::text,1), ('can_write'::text,2), ('can_manage'::text,3)
-        ), perm_edges(tail_uuid, head_uuid, val, follow, trashed) AS (
+        ), perm_edges(tail_uuid, head_uuid, val, follow) AS (
          SELECT links.tail_uuid,
             links.head_uuid,
             pv.val,
-            ((pv.val = 3) OR (groups.uuid IS NOT NULL)) AS follow,
-            (0)::smallint AS trashed,
-            (0)::smallint AS followtrash
+            ((pv.val = 3) OR (groups.uuid IS NOT NULL)) AS follow
            FROM ((public.links
              LEFT JOIN perm_value pv ON ((pv.name = (links.name)::text)))
              LEFT JOIN public.groups ON (((pv.val < 3) AND ((groups.uuid)::text = (links.head_uuid)::text))))
@@ -35,26 +31,19 @@ as $$
          SELECT groups.owner_uuid,
             groups.uuid,
             3,
-            true AS bool,
-                CASE
-                    WHEN ((groups.trash_at IS NOT NULL) AND (groups.trash_at < clock_timestamp())) THEN 1
-                    ELSE 0
-                END AS "case",
-            1
+            true AS bool
            FROM public.groups
-        ), perm(val, follow, user_uuid, target_uuid, trashed) AS (
+        ), perm(val, follow, user_uuid, target_uuid) AS (
          SELECT (3)::smallint AS val,
             true AS follow,
             (users.uuid)::character varying(32) AS user_uuid,
-            (users.uuid)::character varying(32) AS target_uuid,
-            (0)::smallint AS trashed
+            (users.uuid)::character varying(32) AS target_uuid
            FROM public.users
         UNION
          SELECT (LEAST((perm_1.val)::integer, edges.val))::smallint AS val,
             edges.follow,
             perm_1.user_uuid,
-            (edges.head_uuid)::character varying(32) AS target_uuid,
-            ((GREATEST((perm_1.trashed)::integer, edges.trashed) * edges.followtrash))::smallint AS trashed
+            (edges.head_uuid)::character varying(32) AS target_uuid
            FROM (perm perm_1
              JOIN perm_edges edges ON ((perm_1.follow AND ((edges.tail_uuid)::text = (perm_1.target_uuid)::text))))
         )
@@ -64,8 +53,7 @@ as $$
     CASE perm.follow
        WHEN true THEN perm.target_uuid
        ELSE NULL::character varying
-    END AS target_owner_uuid,
-    max(perm.trashed) AS trashed
+    END AS target_owner_uuid
    FROM perm
   GROUP BY perm.user_uuid, perm.target_uuid,
         CASE perm.follow

commit 5f915857cbb3620587f321514a065a73fd6ecc46
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Mon May 4 16:22:59 2020 -0400

    16007: Separating 'trashed' from 'permissions' WIP
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 816dbf475..12f729813 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -296,14 +296,16 @@ class ArvadosModel < ApplicationRecord
       if !include_trash
         if sql_table != "api_client_authorizations"
           # Only include records where the owner is not trashed
-          sql_conds = "#{sql_table}.owner_uuid NOT IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+
-                      "WHERE trashed = 1) #{exclude_trashed_records}"
+          #sql_conds = "#{sql_table}.owner_uuid NOT IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+
+          #            "WHERE trashed = 1) #{exclude_trashed_records}"
+          sql_conds = "#{sql_table}.owner_uuid NOT IN (SELECT * FROM trashed_groups) #{exclude_trashed_records}"
         end
       end
     else
       trashed_check = ""
       if !include_trash then
-        trashed_check = "AND trashed = 0"
+        #trashed_check = "AND trashed = 0"
+        trashed_check = "AND target_uuid NOT IN (SELECT * FROM trashed_groups)"
       end
 
       # Note: it is possible to combine the direct_check and
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 1f2b0d8b7..dbe2d4ed5 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -39,6 +39,21 @@ class Group < ArvadosModel
   end
 
   def maybe_invalidate_permissions_cache
+    if is_trashed_changed?
+      if is_trashed == true
+        ActiveRecord::Base.connection.exec_query %{
+insert into trashed_groups (uuid) select * from project_subtree($1);
+},
+                                                 'Group.trash_subtree',
+                                                 [[nil, self.uuid]]
+      elsif is_trashed == false && TrashedGroup.find_by_uuid(self.owner_uuid).nil?
+        ActiveRecord::Base.connection.exec_query %{
+delete from trashed_groups where uuid in (select * from project_subtree_notrash($1));
+},
+                              'Group.untrash_subtree',
+                              [[nil, self.uuid]]
+      end
+    end
     if uuid_changed? or owner_uuid_changed? or is_trashed_changed?
       # This can change users' permissions on other groups as well as
       # this one.
diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb
index 00ec5b19b..87c8a4cb8 100644
--- a/services/api/db/migrate/20200501150153_permission_table.rb
+++ b/services/api/db/migrate/20200501150153_permission_table.rb
@@ -73,6 +73,55 @@ as $$
             ELSE NULL::character varying
         END
 $$;
+}
+
+    ActiveRecord::Base.connection.execute %{
+create or replace function project_subtree (starting_uuid varchar(27))
+returns table (target_uuid varchar(27))
+STABLE
+language SQL
+as $$
+WITH RECURSIVE
+	project_subtree(uuid) as (
+	values (starting_uuid)
+	union
+	select groups.uuid from groups join project_subtree on (groups.owner_uuid = project_subtree.uuid)
+	)
+	select uuid from project_subtree;
+$$;
+}
+
+    ActiveRecord::Base.connection.execute %{
+create or replace function project_subtree_notrash (starting_uuid varchar(27))
+returns table (target_uuid varchar(27))
+STABLE
+language SQL
+as $$
+WITH RECURSIVE
+	project_subtree(uuid) as (
+	values (starting_uuid)
+	union
+	select groups.uuid from groups join project_subtree on (groups.owner_uuid = project_subtree.uuid)
+        where groups.is_trashed=false
+	)
+	select uuid from project_subtree;
+$$;
+}
+
+    create_table :trashed_groups, :id => false do |t|
+      t.string :uuid
+    end
+
+        ActiveRecord::Base.connection.execute %{
+create or replace function compute_trashed ()
+returns table (uuid varchar(27))
+STABLE
+language SQL
+as $$
+select ps.target_uuid from groups,
+  lateral project_subtree(groups.uuid) ps
+  where groups.is_trashed = true
+$$;
 }
 
     ActiveRecord::Base.connection.execute "DROP MATERIALIZED VIEW IF EXISTS materialized_permission_view;"
@@ -80,5 +129,6 @@ $$;
   end
   def down
     drop_table :materialized_permissions
+    drop_table :trashed_groups
   end
 end
diff --git a/services/api/lib/refresh_permission_view.rb b/services/api/lib/refresh_permission_view.rb
index 2a6eb3f65..919079667 100644
--- a/services/api/lib/refresh_permission_view.rb
+++ b/services/api/lib/refresh_permission_view.rb
@@ -3,12 +3,15 @@
 # SPDX-License-Identifier: AGPL-3.0
 
 PERMISSION_VIEW = "materialized_permissions"
+TRASHED_GROUPS = "trashed_groups"
 
 def do_refresh_permission_view
   ActiveRecord::Base.transaction do
     ActiveRecord::Base.connection.execute("LOCK TABLE permission_refresh_lock")
     ActiveRecord::Base.connection.execute("DELETE FROM #{PERMISSION_VIEW}")
     ActiveRecord::Base.connection.execute("INSERT INTO #{PERMISSION_VIEW} select * from compute_permission_table()")
+    ActiveRecord::Base.connection.execute("DELETE FROM #{TRASHED_GROUPS}")
+    ActiveRecord::Base.connection.execute("INSERT INTO #{TRASHED_GROUPS} select * from compute_trashed()")
   end
 end
 
diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index 30ab89c7e..2b5e8d5a9 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -505,9 +505,19 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
 
   ### trashed project tests ###
 
-  [:active, :admin].each do |auth|
+  #
+  # The structure is
+  #
+  # trashed_project         (zzzzz-j7d0g-trashedproject1)
+  #   trashed_subproject    (zzzzz-j7d0g-trashedproject2)
+  #   trashed_subproject3   (zzzzz-j7d0g-trashedproject3)
+  #   zzzzz-xvhdp-cr5trashedcontr
+
+  [:active,
+   :admin].each do |auth|
     # project: to query,    to untrash,    is visible, parent contents listing success
-    [[:trashed_project,     [],                 false, true],
+    [
+     [:trashed_project,     [],                 false, true],
      [:trashed_project,     [:trashed_project], true,  true],
      [:trashed_subproject,  [],                 false, false],
      [:trashed_subproject,  [:trashed_project], true,  true],

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list