16007: Update comments to discuss edge_id

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb
index 0031bd766cd3b6b60d90fe76bd5778560b7bbed5..4f9ea156dc8d8315004aed0e761c69ba8e431de6 100644 (file)
--- a/services/api/db/migrate/20200501150153_permission_table.rb
+++ b/services/api/db/migrate/20200501150153_permission_table.rb
@@ -18,7 +18,6 @@ class PermissionTable < ActiveRecord::Migration[5.0]
     # permissions system.  Updating trashed items follows a similar
     # (but less complicated) strategy to updating permissions, so it
     # may be helpful to look at that first.
-    #
 
     ActiveRecord::Base.connection.execute "DROP MATERIALIZED VIEW IF EXISTS materialized_permission_view;"
     drop_table :permission_refresh_lock
@@ -99,12 +98,13 @@ $$;
 }
 
     # Merge all permission relationships into a single view.  This
-    # consists of: groups (projects) owning things, users owning
-    # things, users owning themselves, and explicit permission links.
+    # consists of: groups owned by users and projects, users owned
+    # by other users, users have permission on themselves,
+    # and explicit permission links.
     #
     # A SQL view gets inlined into the query where it is used as a
     # subquery.  This enables the query planner to inject constraints,
-    # so we only look up edges we plan to traverse and avoid a brute
+    # so it only has to look up edges it plans to traverse and avoid a brute
     # force query of all edges.
     ActiveRecord::Base.connection.execute %{
 create view permission_graph_edges as
@@ -131,9 +131,9 @@ union all
       where links.link_class='permission'
 }
 
-    # Code fragment that is used below.  This is used to ensure that
-    # the permission edge passed into compute_permission_subgraph
-    # takes precedence over an existing edge in the "edges" view.
+    # This is used to ensure that the permission edge passed into
+    # compute_permission_subgraph takes replaces the existing edge in
+    # the "edges" view that is about to be removed.
     edge_perm = %{
 case (edges.edge_id = perm_edge_id)
                                when true then starting_perm
@@ -141,13 +141,13 @@ case (edges.edge_id = perm_edge_id)
                             end
 }
 
-    #
     # The primary function to compute permissions for a subgraph.
-    # This originally was organized somewhat more cleanly, but this
-    # ran into performance issues due to the query optimizer not
-    # working across function and "with" expression boundaries.  So I
-    # had to fall back on using string templates for the repeated
-    # code.  I'm sorry.
+    # Comments on how it works are inline.
+    #
+    # Due to performance issues due to the query optimizer not
+    # working across function and "with" expression boundaries, I
+    # had to fall back on using string templates for repeated code
+    # in order to inline it.
 
     ActiveRecord::Base.connection.execute %{
 create or replace function compute_permission_subgraph (perm_origin_uuid varchar(27),
@@ -172,6 +172,14 @@ as $$
                   starting_uuid One of 1, 2, 3 for can_read,
                   can_write, can_manage respectively, or 0 to revoke
                   permissions.
+
+   perm_edge_id: Identifies the permission edge that is being updated.
+                 Changes of ownership, this is starting_uuid.
+                 For links, this is the uuid of the link object.
+                 This is used to override the edge value in the database
+                 with starting_perm.  This is necessary when revoking
+                 permissions because the update happens before edge is
+                 actually removed.
 */
 with
   /* Starting from starting_uuid, determine the set of objects that

diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index affdf7e4011f450cd5b90cf849d3ae24c4f16906..469475ad9604232fbeafc76889c4665b6cd77b80 100644 (file)
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -59,6 +59,14 @@ CREATE FUNCTION public.compute_permission_subgraph(perm_origin_uuid character va
                   starting_uuid One of 1, 2, 3 for can_read,
                   can_write, can_manage respectively, or 0 to revoke
                   permissions.
+
+   perm_edge_id: Identifies the permission edge that is being updated.
+                 Changes of ownership, this is starting_uuid.
+                 For links, this is the uuid of the link object.
+                 This is used to override the edge value in the database
+                 with starting_perm.  This is necessary when revoking
+                 permissions because the update happens before edge is
+                 actually removed.
 */
 with
   /* Starting from starting_uuid, determine the set of objects that

diff --git a/services/api/lib/update_permissions.rb b/services/api/lib/update_permissions.rb
index d6e6537725a436b9d152e7470ba0b5b2a878ed4f..4c2e72d9553c3b6e110c9c3c6a0360afebe64390 100644 (file)
--- a/services/api/lib/update_permissions.rb
+++ b/services/api/lib/update_permissions.rb
@@ -50,6 +50,11 @@ def update_permissions perm_origin_uuid, starting_uuid, perm_level, edge_id=nil
   # how the permissions are computed.
 
   if edge_id.nil?
+    # For changes of ownership, edge_id is starting_uuid.  In turns
+    # out most invocations of update_permissions are for changes of
+    # ownership, so make this parameter optional to reduce
+    # clutter.
+    # For permission links, the uuid of the link object will be passed in for edge_id.
     edge_id = starting_uuid
   end