Merge lp:~canonical-isd-hackers/canonical-identity-provider/sql-limit-for-openid-transactions into lp:canonical-identity-provider/release

[Łukasz Czyżykowski]

Database access improvements during OpenID transaction.

Overview
========
This branch limits number of SQL queries executed while processing OpenID transaction.

Improvement
===========
During an OpenID request, with all possible SReg fields and Teams extension, before this branch, there are 28 SQL queries executed. With this branch, that number is down to 12. The raw speed improvement is hard to quantify, as it requires database configured in very similar way as the production one.

=== modified file 'identityprovider/auth.py'
--- identityprovider/auth.py	2011-07-27 13:00:22 +0000
+++ identityprovider/auth.py	2011-09-06 14:43:24 +0000
@@ -11,7 +11,7 @@
 
 from identityprovider.models import Account, AccountPassword
 from identityprovider.models.api import APIUser
-from identityprovider.models.const import EmailStatus, AccountStatus
+from identityprovider.models.const import EmailStatus
 from identityprovider.utils import (
     validate_launchpad_password, get_object_or_none)
 
 
=== modified file 'identityprovider/forms.py'
--- identityprovider/forms.py	2011-08-25 13:26:06 +0000
+++ identityprovider/forms.py	2011-09-06 14:43:24 +0000
@@ -418,7 +418,7 @@
         """Get the list of teams actually approved by the user for the request.
         """
         if self.request_method == 'POST':
-            return [t for t in self.data if t in self.request.POST]
+            return [t for t in self.memberships if t in self.request.POST]
         else:
             return []
 
@@ -428,16 +428,13 @@
         self.teams_request = teams_request
         self.rpconfig = rpconfig
         self.approved_data = approved_data
-        self.data = self._get_teams_for_user(request.user,
-            rpconfig and rpconfig.can_query_any_team)
-
-        super(TeamsRequestForm, self).__init__(self.data)
-        self._init_fields(self.data)
-        all_teams = self.teams_request.allRequestedTeams()
-        membership = TeamMembership.get_team_memberships_for_user(
-            all_teams, self.request.user,
-            self.rpconfig and self.rpconfig.can_query_any_team)
-        self.should_display = len(membership) > 0
+        self.memberships = self._get_teams_for_user(
+            request.user, rpconfig and rpconfig.can_query_any_team)
+
+        super(TeamsRequestForm, self).__init__(self.memberships)
+
+        self._init_fields(self.memberships)
+        self.should_display = len(self.memberships) > 0
 
     def _get_teams_for_user(self, user, include_private=False):
         """Get the list of teams to ask about in the form based on the user's
 
=== modified file 'identityprovider/models/account.py'
--- identityprovider/models/account.py	2011-07-27 13:00:22 +0000
+++ identityprovider/models/account.py	2011-09-06 14:43:24 +0000
@@ -130,20 +130,23 @@
         return sites.order_by('-date_last_used')
 
     def _get_preferredemail(self):
-        try:
-            return self.emailaddress_set.filter(
-                status=EmailStatus.PREFERRED)[0]
-        except IndexError:
-            # Try to determine a suitable address, and mark it as preferred
+        if not hasattr(self, '_preferredemail'):
             try:
-                email = self.emailaddress_set.filter(
-                    status=EmailStatus.VALIDATED).order_by('date_created')[0]
-                email.status = EmailStatus.PREFERRED
-                email.save()
-                return email
+                self._preferredemail = self.emailaddress_set.filter(
+                    status=EmailStatus.PREFERRED)[0]
             except IndexError:
-                # Now we really don't have anything
-                return None
+                # Try to determine a suitable address, and mark it as preferred
+                try:
+                    emails = self.emailaddress_set.filter(
+                        status=EmailStatus.VALIDATED)
+                    email = emails.order_by('date_created')[0]
+                    email.status = EmailStatus.PREFERRED
+                    email.save()
+                    self._preferredemail = email
+                except IndexError:
+                    # Now we really don't have anything
+                    self._preferredemail = None
+        return self._preferredemail
 
     def _set_preferredemail(self, email):
         current = self.preferredemail
@@ -152,6 +155,7 @@
             current.save()
         email.status = EmailStatus.PREFERRED
         email.save()
+        self._preferredemail = email
 
     preferredemail = property(_get_preferredemail, _set_preferredemail)
 
@@ -166,9 +170,11 @@
                 lp_account = open_ids[0].lp_account
                 # Importing it here to prevent cyclic import issue
                 from .person import Person
-                persons = Person.objects.filter(lp_account=lp_account)
-                if len(persons) > 0:
-                    self._person = persons[0]
+                try:
+                    self._person = Person.objects.select_related(
+                        'personlocation').get(lp_account=lp_account)
+                except Person.DoesNotExist:
+                    pass
         return getattr(self, '_person', None)
 
     @property
 
=== modified file 'identityprovider/models/openidmodels.py'
--- identityprovider/models/openidmodels.py	2011-07-27 13:00:22 +0000
+++ identityprovider/models/openidmodels.py	2011-09-06 14:43:24 +0000
@@ -2,14 +2,19 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import base64
+import hashlib
 import time
+
 from datetime import datetime
 from openid.association import Association
+
 import openid.store
 import openid.store.nonce
+
 from openid.store.interface import OpenIDStore
 
 from django.conf import settings
+from django.core.cache import cache
 from django.db import models
 from django.utils import simplejson as json
 from django.utils.translation import ugettext_lazy as _
@@ -167,6 +172,16 @@
     def __unicode__(self):
         return self.displayname
 
+    @classmethod
+    def cache_key(cls, trust_root):
+        # Need to calculate digest of the url to make it work with long trust_root
+        # values when caching in memcached
+        return 'rpconfig-' + hashlib.md5(trust_root.rstrip('/')).hexdigest()
+
+    def save(self, *args, **kwargs):
+        cache.delete(self.cache_key(self.trust_root))
+        return super(OpenIDRPConfig, self).save(*args, **kwargs)
+
     @property
     def logo_url(self):
         if self.logo:
@@ -186,19 +201,30 @@
             return None
         if openid_identifier is None:
             openid_identifier = account.openid_identity_url
+        if approved_data:
+            approved_data = json.dumps(approved_data)
         try:
             summary = OpenIDRPSummary.objects.get(
                 account=account,
                 trust_root=trust_root,
                 openid_identifier=openid_identifier)
-            summary.increment()
+            update_bits = dict(
+                total_logins=models.F('total_logins') + 1,
+                date_last_used=datetime.utcnow(),
+            )
+            if approved_data:
+                update_bits['approved_data'] = approved_data
+            # Using update is not causing Django to select the record once
+            # again from the database, it's one less SELECT.
+            OpenIDRPSummary.objects.filter(pk=summary.pk).update(**update_bits)
         except OpenIDRPSummary.DoesNotExist:
-            summary = OpenIDRPSummary.objects.create(
+            summary = OpenIDRPSummary(
                 account=account,
                 trust_root=trust_root,
                 openid_identifier=openid_identifier)
-        if approved_data:
-            summary.set_approved_data(approved_data)
+            if approved_data:
+                summary.approved_data = approved_data
+            summary.save()
         return summary
 
 
@@ -220,11 +246,6 @@
         db_table = u'openidrpsummary'
         unique_together = ('account', 'trust_root', 'openid_identifier')
 
-    def increment(self):
-        self.total_logins += 1
-        self.date_last_used = datetime.utcnow()
-        self.save()
-
     def get_approved_data(self):
         try:
             data = json.loads(self.approved_data)
@@ -233,8 +254,9 @@
         return data
 
     def set_approved_data(self, approved_data):
-        self.approved_data = json.dumps(approved_data)
-        self.save()
+        if not isinstance(approved_data, basestring):
+            approved_data = json.dumps(approved_data)
+        self.approved_data = approved_data
 
 
 class DjangoOpenIDStore(OpenIDStore):
 
=== modified file 'identityprovider/models/team.py'
--- identityprovider/models/team.py	2011-07-01 19:47:04 +0000
+++ identityprovider/models/team.py	2011-09-06 14:43:24 +0000
@@ -50,21 +50,21 @@
 
     @staticmethod
     def get_team_memberships_for_user(team_names, user, include_private=False):
-        memberships = []
-        for team_name in team_names:
-            try:
-                team = Person.objects.get(name=team_name)
-            except Person.DoesNotExist:
-                team = None
-            if team is None or not team.is_team():
-                continue
-            # Control access to private teams
-            if (team.visibility != PERSON_VISIBILITY_PUBLIC and
-                    not include_private):
-                continue
-            if user.person_in_team(team):
-                memberships.append(team_name)
-        return memberships
+        teams = Person.objects.filter(
+            name__in=team_names,      # All team names provided
+            teamowner__isnull=False,  # Only teams
+        )
+        if not include_private:
+            teams = teams.filter(
+                visibility=PERSON_VISIBILITY_PUBLIC)
+        # Only teams in which the user is a member, which can be either through
+        # direct membership or by being a team owner.
+        teams = teams.filter(
+            models.Q(team_participations__person=user.person) |
+            models.Q(teamowner__id=models.F('id')),
+        )
+        # By default it's a query set result, not a true list
+        return list(teams.values_list('name', flat=True))
 
 
 class TeamParticipation(models.Model):
 
=== modified file 'identityprovider/tests/test_models_openidmodels.py'
--- identityprovider/tests/test_models_openidmodels.py	2011-07-27 13:00:22 +0000
+++ identityprovider/tests/test_models_openidmodels.py	2011-09-06 14:43:24 +0000
@@ -253,6 +253,7 @@
         summary = OpenIDRPSummary.objects.create(account=self.account,
             trust_root=self.trust_root, openid_identifier='oid')
         summary.set_approved_data(self.approved_data)
+        summary.save()
         return summary
 
     def test_record_when_readonly(self):
@@ -281,6 +282,7 @@
         summary1 = self.create_summary()
         summary2 = OpenIDRPSummary.objects.record(self.account,
             self.trust_root, openid_identifier='oid')
+        summary2 = OpenIDRPSummary.objects.get(pk=summary2.pk)
         self.assertEqual(summary2.total_logins, 2)
         self.assertEqual(summary1, summary2)
 
@@ -294,23 +296,17 @@
         self.assertNotEqual(summary1, summary2)
 
     def test_record_not_existing(self):
-        summary1 = OpenIDRPSummary.objects.record(self.account, self.trust_root,
-            openid_identifier='oid')
+        summary1 = OpenIDRPSummary.objects.record(
+            self.account, self.trust_root, openid_identifier='oid')
         self.assertEqual(summary1.total_logins, 1)
         summary2 = OpenIDRPSummary.objects.get(account=self.account,
             trust_root=self.trust_root, openid_identifier='oid')
         self.assertEqual(summary1, summary2)
 
-    def test_increment(self):
-        summary = self.create_summary()
-        self.assertEqual(summary.total_logins, 1)
-
-        summary.increment()
-        self.assertEqual(summary.total_logins, 2)
-
     def test_approved_data(self):
         summary = self.create_summary()
         summary.set_approved_data(self.approved_data)
+        summary.save()
         summary2 = OpenIDRPSummary.objects.get(account=self.account,
             trust_root=self.trust_root, openid_identifier='oid')
         self.assertEqual(summary2.get_approved_data(), self.approved_data)
 
=== modified file 'identityprovider/tests/test_views_server.py'
--- identityprovider/tests/test_views_server.py	2011-07-27 13:00:22 +0000
+++ identityprovider/tests/test_views_server.py	2011-09-06 14:43:24 +0000
@@ -7,6 +7,7 @@
 
 from django.conf import settings
 from django.contrib.auth.models import AnonymousUser
+from django.core.cache import cache
 from django.http import HttpRequest
 from django.test import TestCase
 from mock import Mock
@@ -67,6 +68,11 @@
         self.META = {}
 
 
+def delete_rpconfig_cache_entry(trust_root):
+        cache.delete(OpenIDRPConfig.cache_key(trust_root))
+
+
+
 class HandleOpenIDErrorTestCase(SSOBaseTestCase):
 
     # tests for the _handle_openid_error method
@@ -418,6 +424,8 @@
         # create multiple matching OpenIDRPSummary objects
         account = Account.objects.get_by_email(self.login_email)
         trust_root = 'http://localhost/'
+        delete_rpconfig_cache_entry(trust_root)
+
         OpenIDRPSummary.objects.create(account=account, trust_root=trust_root,
             openid_identifier='http://localhost/~openid1')
         OpenIDRPSummary.objects.create(account=account, trust_root=trust_root,
@@ -427,6 +435,7 @@
         self.assertEqual(r.status_code, 200)
         self.assertTemplateUsed(r, 'decide.html')
 
+
     def test_decide_team_membership_with_auto_authorize(self):
         # make sure rpconfig is set to auto authorize
         self.client.login(username='[e-mail]',
@@ -537,10 +546,14 @@
                           password='test')
 
         # create a trusted rpconfig
-        rpconfig = OpenIDRPConfig(trust_root='http://untrusted/',
+        trust_root = 'http://untrusted/'
+        rpconfig = OpenIDRPConfig(trust_root=trust_root,
             can_query_any_team=True,
             description="Some description",
         )
+
+        delete_rpconfig_cache_entry("http://localhost/")
+
         rpconfig.save()
         param_overrides = {
             'openid.sreg.required': 'nickname,email,fullname',
@@ -1048,6 +1061,8 @@
         session[token] = signed.dumps(DummyORequest())
         session.save()
 
+        delete_rpconfig_cache_entry("http://localhost/")
+
         r = self.client.get('/%s/+decide' % token)
         self.assertContains(r, '<em>not</em>')
 
 
=== modified file 'identityprovider/tests/test_views_ui.py'
--- identityprovider/tests/test_views_ui.py	2011-08-25 13:25:51 +0000
+++ identityprovider/tests/test_views_ui.py	2011-09-06 14:43:24 +0000
@@ -522,10 +522,13 @@
         # make sure the account is deactivated and preferred email is removed
         _preferred_email = account.preferredemail
         account.status = AccountStatus.DEACTIVATED
+        # Blow out the cached value
         account.save()
         for email_obj in account.emailaddress_set.all():
             email_obj.status = EmailStatus.NEW
             email_obj.save()
+
+        account = Account.objects.get_by_email('[e-mail]')
         self.assertEqual(account.preferredemail, None)
 
         r = self.client.post('/+forgot_password',
@@ -539,6 +542,7 @@
         r = self.client.post(self._token_url('+resetpassword'), data)
         self.assertRedirects(r, '/')
 
+        account = Account.objects.get_by_email('[e-mail]')
         self.assertEqual(account.preferredemail.email, '[e-mail]')
 
         for email_obj in account.emailaddress_set.all():
 
=== modified file 'identityprovider/views/server.py'
--- identityprovider/views/server.py	2011-07-27 19:20:16 +0000
+++ identityprovider/views/server.py	2011-09-06 14:43:24 +0000
@@ -504,7 +504,7 @@
         team_names = team_names.split(',')
         teams_form = TeamsRequestForm(request,
             TeamsRequest.fromOpenIDRequest(orequest),
-            utils.get_rpconfig(orequest.trust_root))
+            rpconfig)
         approved_data['teams'] = {
             'requested': team_names,
             'approved': teams_form.teams_approved_by_user}
 
=== modified file 'identityprovider/views/utils.py'
--- identityprovider/views/utils.py	2011-07-13 14:30:29 +0000
+++ identityprovider/views/utils.py	2011-09-06 14:43:24 +0000
@@ -1,7 +1,9 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
+import hashlib
 
 from django.conf import settings
+from django.core.cache import cache
 
 from identityprovider.models import OpenIDRPConfig
 
@@ -24,12 +26,19 @@
 
 
 def get_rpconfig(trust_root):
-    alternatives = [trust_root]
-
-    if trust_root.endswith('/'):
-        alternatives.append(trust_root[:-1])
-    else:
-        alternatives.append(trust_root + '/')
+    cache_key = OpenIDRPConfig.cache_key(trust_root)
+
+    default = object()
+    rpconfig = cache.get(cache_key, default)
+    if rpconfig is not default:
+        return rpconfig
+
+    alternatives = [
+        trust_root,
+        trust_root[:-1] if trust_root.endswith('/') else (trust_root + '/'),
+    ]
 
     rpconfig = OpenIDRPConfig.objects.filter(trust_root__in=alternatives)
-    return rpconfig and rpconfig[0] or None
+    rpconfig = rpconfig and rpconfig[0] or None
+    cache.set(cache_key, rpconfig, 60 * 60 * 24)
+    return rpconfig