Hi Ralf:

In message <20241021162755.4s6uag2wukr6va22@runtux.com>,
Ralf Schlatterbeck writes:
>The latest version is pushed to branch permission-performance.

I am just starting to look at these changes. What were you seeing
that prompted:

   changeset:   8131:8e9181dfc9fa
   Fix searchable checks in roundup/security.py

You moved the test:

        if self.check:
            return 0

from the last test in the searchable() method to before the property
checks. From the original method description, this seems correct.

Then you added a test:

      if self.properties is None:
          return 1

that used to be handled by fall through to the return 1 at the end of
the method. I think "not self._properties_dict[property]" would be
False for every property if self.properties is None.

Is this just an optimization to not hit the final return 1 in the
method?  If so is that final "return 1" ever hit? Maybe add a
log.warning to see if it is called and under what circumstances?

Also, I think a search permissions like:

  addPermission("Search", "user", properties=("username", "id"),
                check=somecheck)

would have returned 1 with the original code when called as:

   permission.searchable("Search", "user", "username")

But would return 0 with this changeset.

Is this correct? If so we need a section in upgrading.txt because this
is an incompatible change.

Also adding a test of this case to the test suite is needed. Maybe a
new testSearch in tests/test_security.py? It could be thrown in with
testTransitiveSearchPermissions, but that is already a large test.

Any idea why Search permissions can not have a check function? The
docstring for it indicates checks are not allowed. I don't see this
documented anywhere in the permissions section of
doc/references.txt. (Granted that section is kind of light on
details/examples.) No mention in customizing.txt either.

I don't have a use case for a Search permission with a check
command. However IIRC a View permission will use/run a check command
and a Search permission is called as a fallback in search scenarios
only if a View permission did not allow access.

I am sure there is more to follow.