Hi Ralf:

In message <20241017112611.hn2rkqcg5ez2kcbo@runtux.com>,
Ralf Schlatterbeck writes:
>On Thu, Oct 17, 2024 at 03:32:58AM +0000, John Rouillard wrote:
>> 
>> I did a quick instrumentation of hasPermission in my custom tracker.
>> I displayed an issue index page logged in as admin with Admin role.
>
>Cool, I'll try running this for my tracker.

Thanks.

>Concerning memoization: 
>In method 'batch' in roundup/cgi/templating.py we have:
>
>    def batch(self, permission='View'):
>        check = self._client.db.security.hasPermission
>        ...
>        # filter for visibility
>        allowed = [id for id in klass.filter(matches, filterspec, sort, group)
>                   if check(permission, userid, self.classname, itemid=id)]
>
>So we're calling the check method for *each issue found* (or whatever
>table we're currently searching).

Correct. In my followup it looks like this would correspond to the

   2 hasPermission: (View, 1, issue, None, 210) (1, 5) allow

logs lines. That would also seem to be the point where every item
(even those that willl never be displayed because they are larger than
the batch size) is included.

>I would argue that if we find any permission on the given 'klass' that
>does not have a check method, we do not need to do this: The check will
>always tell us that we can see at least one property of *each* item in
>the klass. So we do not need to filter and consequently do not need to
>loop over all issues found when we establish that fact before the loop.

Your logic sounds right. The itemid AFAIK is only used in a check method.

Would the following code (properly indented) do the trick:

    if check(permission, userid, self.classname):
      allowed = [id for id in klass.filter(matches, filterspec, sort, group)]
    else:
      allowed = [id for id in klass.filter(matches, filterspec, sort, group)
                 if check(permission, userid, self.classname, itemid=id)]

The check(permission, userid, self.classname) should fail if all
permissions for 'permission' have a check command or properties with a
props_only=true.

>When rendering the HTML, the check is performed again for each property.
>If the property should not be shown it is rendered as '[Hidden]'.
>
>So no need for memoization.

I did a fast memoization change (patch attached) for hasPermission if
the check command and item command are missing. That at least seems to
be used for the 'Web Access' check in templating.py
HTMLClass::is_X_ok() where X is view, edit, restore ... and in the
HTMLClass::list() method.

>If we find only permissions *with* a check method we *need* to check
>*each of them* in the loop. And we cannot memoize things because the
>check is different for each item found.

Correct. Also it looks like the item check is done only once in most
cases so memoization of that check wouldn't help.

>Now my second proposal comes into play: If we have check methods with
>SQL (or some other mechanism that ultimately compiles to SQL) we can
>*OR* all the SQL for each check method and put that with an AND into the
>[sql generated by the] filter call. Then the loop would again only
>return valid items and again we would not have to do it in python in the
>loop.

We still need to figure out a method to skip the python permission
checks only for those checks that are pushed down to the DB.

>Of course this applies only to View permissions. And only when
>retrieving a list of issues (or other classes), either in the batch
>method cited above or in the REST (and probably XMLRPC) api.

Agreed.

>Note that I think the performance hog lies only in searching, not in
>other permission checks like creation or editing.

By searching do you mean finding issues to display on an index page?

Also I have a spreadsheet like editing interface for issues where I
mix an index with edit checks. So it might not always be a View
permission.

>And it probably
>applies only to 'View' permissions (I don't think batch is ever called
>with an explicit permission other than 'View')

I'm not sure that batch is only called with view permissions. A quick
grep for 'batch(' doesn't return any calls to batch() in the code. I
assume it's triggered from templates. Is there any reason to assume
that a template can't generate a batch(..., permission="edit") call?

If View is the only permission used in a batch call, why does the
batch() method have a permission parameter?

Thoughts?