Change Request -- high risk, high reward

[Toshio Kuratomi]

ricky and I have identified a piece of code in fas2's new safasprovider
that's querying the database a lot.  It's causing anything that checks
identity to issue a query to the database to lookup the visit cookie.
This is causing things that lookup information on the identity multiple
times to take a lot of time.  One of the functions that does this is
filter_private(), the function that removes excess information according
to privacy settings and who is looking for the data.  Our test was the
/user/list method which is currently running over 5 minutes for an
otherwise non-database loop.  Changing this caused the loop to run for 8
seconds.

That's the benefit.  The risk is that this is a change to the
safasprovider, ie the portion of fas2 that authenticates the user.  So
if there's a reason this shouldn't be cached, we could potentially be
breaking a lot of things.

Ricky and I have both looked at the code in
fas/safasprovider.py::SaFasIdentity and think that it's safe to cache
this.  The TG-1.0.8 saprovider on which safasprovider is based does not
cache this but I've looked at the code and it seems like their provider
only uses the variable in question a maximum of two times during a
request.  The CSRF protection that we've enabled needs to use this
variable more often.

Here's the code:

--- a/fas/safasprovider.py
+++ b/fas/safasprovider.py
@@ -65,6 +65,7 @@ class SaFasIdentity(object):

     def __init__(self, visit_key=None, user=None, using_ssl=False):
         self.visit_key = visit_key
+        self._visit_link = None
         if user:
             self._user = user
             if visit_key is not None:
@@ -201,9 +202,13 @@ class SaFasIdentity(object):
     ### TG: Same as TG-1.0.8
     def _get_visit_link(self):
         '''Get the visit link to this identity.'''
+        if self._visit_link:
+            return self.visit_link
         if self.visit_key is None:
-            return None
-        return
visit_class.query.filter_by(visit_key=self.visit_key).first()
+            self._visit_link = None
+        else:
+            self._visit_link =
visit_class.query.filter_by(visit_key=self.visi
t_key).first()
+        return self._visit_link
     visit_link = property(_get_visit_link)

If we were outside of freeze, I would apply this as it is causing issues
for some of the things that talk to fas (like zodbot and developer
instances of pkgdb).

-Toshio

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/fedora-infrastructure-list/attachments/20090314/aeca5522/attachment.sig>