Merge lp:~cjwatson/canonical-identity-provider/api-accounts-bulk into lp:canonical-identity-provider/release

[Colin Watson]

Add an /api/v2/accounts-bulk endpoint.

This fetches information about a set of accounts in bulk by OpenID
identifier, useful for other services that need to render pages related to
multiple accounts.

This is part of https://docs.google.com/document/d/1K7Dxm5RuKgioKVSyhc5uUefsM4UH91pRUfuYrUe5hCg.

Getting the bulk database queries right (which I think is important since the set of accounts being requested could quite reasonably be 50-100 or so, and we don't want to scale the query count linearly in all their email addresses) was really quite complicated, and I'd welcome feedback from Django experts on how I went about it. My general approach was to add methods that fetch objects by a set of accounts to each of the appropriate managers, and to convert some of the individual properties into bulk prefetching methods; but there were quite significant complications around Account.preferredemail and Account.is_verified.

[Simon Davy (bloodearnest)]

LGTM, left a few minor comments.

Also, I wonder if it's worth adding assertNumQueries checks to some of the bulk query tests?

https://docs.djangoproject.com/en/dev/topics/testing/tools/#django.test.TransactionTestCase.assertNumQueries

[Natalia Bidart (nataliabidart)]

review: Needs Information

[Natalia Bidart (nataliabidart)]

Thanks for the changes! I think the code is ready for landing, except for the following:

* a few nitpicks added as inline comments
* a few tests missing, for all the changes/addings to src/identityprovider/models/account.py, src/identityprovider/models/person.py and src/identityprovider/signals.py (the changes to signals can just be tested in test_models_account.py by deleting emails and asserting over the preferred email result).

Thanks!

[Natalia Bidart (nataliabidart)]

Thank you! Looks great.

review: Approve

=== modified file 'src/api/v20/auth.py'
--- src/api/v20/auth.py	2016-03-18 18:40:31 +0000
+++ src/api/v20/auth.py	2018-01-23 17:52:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 import json
 import logging
@@ -72,6 +72,15 @@
         return result
 
 
+class ApiAccountsBulkAuthentication(SSOOAuthAuthentication):
+
+    def challenge(self):
+        """Return a json body 401 response."""
+        response = errors.INVALID_CREDENTIALS()
+        response['WWW-Authenticate'] = 'OAuth realm="%s"' % self.realm
+        return response
+
+
 class ApiAccountRegistrationAuthentication(ApiOAuthAuthentication):
 
     def _has_ownership(self, request):

=== modified file 'src/api/v20/authorization.py'
--- src/api/v20/authorization.py	2016-03-15 18:38:16 +0000
+++ src/api/v20/authorization.py	2018-01-23 17:52:15 +0000
@@ -1,11 +1,11 @@
-# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 import functools
 
 from api.v20.utils import errors
 
 
-def is_authorized(request):
+def is_authorized(request, edit=None):
     """Checks if logged in user has permissions based on request method.
 
     This helper method's main use is for keeping backwards compatibility
@@ -13,21 +13,26 @@
 
     New API views should check permissions with @authorization_required.
     """
-    if request.method == 'GET':
+    if edit is None:
+        edit = request.method != 'GET'
+    if edit:
+        perm = 'identityprovider.api_edit_account_details'
+    else:
         perm = 'identityprovider.api_view_account_details'
-    else:
-        perm = 'identityprovider.api_edit_account_details'
     account = request.user
     return account.user.has_perm(perm)
 
 
-def authorization_required(func):
+def authorization_required(edit=None):
     """View decorator that checks logged in user has specific permissions."""
 
-    @functools.wraps(func)
-    def wrapped(self, request, *args, **kwargs):
-        if not is_authorized(request):
-            return errors.FORBIDDEN()
-        return func(self, request, *args, **kwargs)
-
-    return wrapped
+    def wrapper(func):
+        @functools.wraps(func)
+        def wrapped(self, request, *args, **kwargs):
+            if not is_authorized(request, edit=edit):
+                return errors.FORBIDDEN()
+            return func(self, request, *args, **kwargs)
+
+        return wrapped
+
+    return wrapper

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2016-12-15 21:46:04 +0000
+++ src/api/v20/handlers.py	2018-01-23 17:52:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import json
@@ -27,6 +27,7 @@
 from api.v20.utils import (
     errors,
     get_account_data,
+    get_accounts_data,
     get_email_data,
     get_minimal_account_data,
     get_token_data,
@@ -148,7 +149,7 @@
         expand = (request.GET.get('expand', '').lower() == 'true')
         return get_account_data(account, expand=expand)
 
-    @authorization_required
+    @authorization_required(edit=True)
     @require_mime('json')
     @add_piston_http_patch_support
     @validate(AccountStatusForm, 'data')
@@ -164,6 +165,36 @@
         return get_account_data(account)
 
 
+class AccountsBulkHandler(BaseHandler):
+    allowed_methods = ('POST',)
+
+    # no throttle limit since this endpoint is used by privileged accounts only
+    @authorization_required(edit=False)
+    @require_mime('json')
+    def create(self, request):
+        """Get information about a set of accounts.
+
+        We use POST for this to avoid needing to worry about practical
+        limits on the length of the query string.
+
+        """
+        data = request.data
+
+        try:
+            openids = data['openids']
+        except KeyError:
+            missing = {'openids': [FIELD_REQUIRED]}
+            return errors.INVALID_DATA(new_style=True, **missing)
+        expand = data.get('expand', False)
+
+        accounts = Account.objects.prefetch_related(
+            'emailaddress_set',
+            'token_set',
+        ).filter(
+            openid_identifier__in=openids, status__exact=AccountStatus.ACTIVE)
+        return get_accounts_data(accounts, expand=expand)
+
+
 class PasswordResetTokenHandler(BaseHandler):
     allowed_methods = ('POST',)
 
@@ -596,7 +627,7 @@
             result = errors.RESOURCE_NOT_FOUND()
         return result
 
-    @authorization_required
+    @authorization_required(edit=True)
     @require_mime('json')
     @add_piston_http_patch_support
     @validate(AccountStatusForm, 'data')

=== modified file 'src/api/v20/tests/test_handlers.py'
--- src/api/v20/tests/test_handlers.py	2016-12-07 16:28:36 +0000
+++ src/api/v20/tests/test_handlers.py	2018-01-23 17:52:15 +0000
@@ -48,6 +48,7 @@
 from identityprovider.tests.test_auth import AuthLogTestCaseMixin
 from identityprovider.tests.utils import SSOBaseTestCase, TimelineActionMixin
 from identityprovider.utils import redirection_url_for_token
+from testing.helpers import assert_no_extra_queries_after
 
 
 OVERRIDES = dict(
@@ -328,6 +329,99 @@
         self.assert_account_data_expanded(' ', False)
 
 
+class AccountsBulkHandlerTestCase(BaseTestCase):
+
+    url = reverse('api-accounts-bulk')
+
+    def setUp(self):
+        super(AccountsBulkHandlerTestCase, self).setUp()
+        account = self.factory.make_account(
+            email='[e-mail]', permissions=['api_view_account_details'])
+        self.super_token = account.create_oauth_token('super-token')
+
+    def test_any_server_error_is_json(self):
+        self.assert_any_server_error_is_json(self.do_post, token=self.token)
+
+    def test_post_unauthenticated_401(self):
+        self.do_post(status_code=401)
+        self.do_post({'openids': []}, status_code=401)
+
+    def test_post_authenticated_with_django_401(self):
+        assert self.client.login(
+            username='[e-mail]', password=DEFAULT_USER_PASSWORD)
+        self.do_post(status_code=401)
+        self.do_post({'openids': []}, status_code=401)
+
+    def test_post_authenticated_with_oauth_unprivileged(self):
+        json_body = self.do_post(
+            {'openids': []}, token=self.token, status_code=403)
+        self.assertEqual(json_body['code'], 'FORBIDDEN')
+        self.assertEqual(json_body['extra'], {})
+
+    def test_post_authenticated_no_match(self):
+        json_body = self.do_post({'openids': []}, token=self.super_token)
+        self.assertEqual(json_body, self.to_json({}))
+
+    def test_post_authenticated_match(self):
+        accounts = [self.factory.make_account() for _ in range(4)]
+        json_body = self.do_post(
+            {
+                'openids': [
+                    account.openid_identifier for account in accounts[:3]
+                ],
+            },
+            token=self.super_token)
+
+        expected = handlers.get_accounts_data(accounts[:3])
+        self.assertEqual(json_body, self.to_json(expected))
+
+    def test_post_authenticated_match_expanded(self):
+        self.maxDiff = None
+        accounts = [self.factory.make_account() for _ in range(4)]
+        json_body = self.do_post(
+            {
+                'openids': [
+                    account.openid_identifier for account in accounts[:3]
+                ],
+                'expand': True,
+            },
+            token=self.super_token)
+
+        expected = handlers.get_accounts_data(accounts[:3], expand=True)
+        self.assertEqual(json_body, self.to_json(expected))
+
+    def test_constant_query_count(self):
+        accounts = []
+
+        def add_accounts():
+            accounts.extend([
+                self.factory.make_account(email_validated=(i % 2) == 0)
+                for i in range(4)
+            ])
+            for i, account in enumerate(accounts[-4:]):
+                if (i % 2) == 0:
+                    self.factory.make_person(account=account)
+                for _ in range(10):
+                    self.factory.make_email_for_account(account=account)
+                for _ in range(10):
+                    self.factory.make_oauth_token(account=account)
+
+        def do_request():
+            return self.do_post(
+                {
+                    'openids': [
+                        account.openid_identifier for account in accounts
+                    ],
+                    'expand': True,
+                },
+                token=self.super_token)
+
+        add_accounts()
+        json_body = assert_no_extra_queries_after(add_accounts, do_request)
+        expected = handlers.get_accounts_data(accounts, expand=True)
+        self.assertEqual(json_body, self.to_json(expected))
+
+
 class AnonymousAccountRegistrationHandlerTestCase(BaseTestCase):
 
     url = reverse('api-registration')

=== modified file 'src/api/v20/tests/test_utils.py'
--- src/api/v20/tests/test_utils.py	2016-08-05 08:44:31 +0000
+++ src/api/v20/tests/test_utils.py	2018-01-23 17:52:15 +0000
@@ -1,13 +1,17 @@
 import json
 from datetime import timedelta
+from functools import partial
+from operator import attrgetter
 
 from django.core.urlresolvers import reverse
 
 from api.v20.utils import (
     EnsureJSONResponseOnAPIErrorMiddleware,
     get_account_data,
+    get_accounts_data,
     get_email_data,
     get_minimal_account_data,
+    get_minimal_accounts_data,
     get_token_data,
 )
 from identityprovider.models.const import EmailStatus
@@ -41,6 +45,64 @@
         self.assertEqual(json.loads(response.content), expected_data)
 
 
+class PluralTestCase(SSOBaseTestCase):
+
+    def assert_equivalent_plural(self, plural_func, singular_func, objects,
+                                 get_key):
+        """Assert that a plural function matches its singular sibling.
+
+        `plural_func` applied to all objects at once produces the same
+        results as `singular_func` applied to each object in turn.
+        """
+        plural_results = plural_func(objects)
+        singular_results = {
+            get_key(obj): singular_func(obj) for obj in objects
+        }
+        self.assertEqual(plural_results, singular_results)
+
+
+class GetAccountsDataTestCase(PluralTestCase):
+
+    def make_accounts(self, count):
+        return [
+            self.factory.make_account(email_validated=(i % 2) == 0)
+            for i in range(count)
+        ]
+
+    def test_get_minimal_accounts_data(self):
+        accounts = self.make_accounts(4)
+        self.factory.make_person(account=accounts[0])
+
+        self.assert_equivalent_plural(
+            get_minimal_accounts_data, get_minimal_account_data,
+            accounts[:3], attrgetter('openid_identifier'))
+
+    def test_get_accounts_data(self):
+        accounts = self.make_accounts(4)
+        self.factory.make_person(account=accounts[0])
+        for _ in range(15):
+            self.factory.make_email_for_account(account=accounts[1])
+        for _ in range(15):
+            self.factory.make_oauth_token(account=accounts[2])
+
+        self.assert_equivalent_plural(
+            get_accounts_data, partial(get_account_data, include_tokens=False),
+            accounts[:3], attrgetter('openid_identifier'))
+        self.assert_equivalent_plural(
+            partial(get_accounts_data, limit=5),
+            partial(get_account_data, limit=5, include_tokens=False),
+            accounts[:3], attrgetter('openid_identifier'))
+        self.assert_equivalent_plural(
+            partial(get_accounts_data, expand=True),
+            partial(get_account_data, expand=True, include_tokens=False),
+            accounts[:3], attrgetter('openid_identifier'))
+        self.assert_equivalent_plural(
+            partial(get_accounts_data, limit=5, expand=True),
+            partial(
+                get_account_data, limit=5, expand=True, include_tokens=False),
+            accounts[:3], attrgetter('openid_identifier'))
+
+
 class GetAccountDataTestCase(SSOBaseTestCase):
 
     openid = '1234567'

=== modified file 'src/api/v20/urls.py'
--- src/api/v20/urls.py	2016-11-01 10:53:35 +0000
+++ src/api/v20/urls.py	2018-01-23 17:52:15 +0000
@@ -1,10 +1,11 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from django.conf.urls import patterns, url
 
 from api.v20.auth import (
     ApiAccountsAuthentication,
+    ApiAccountsBulkAuthentication,
     ApiAccountRegistrationAuthentication,
     ApiEmailsAuthentication,
     ApiTokensAuthentication,
@@ -13,6 +14,7 @@
 from api.v20.handlers import (
     AccountLoginHandler,
     AccountRegistrationHandler,
+    AccountsBulkHandler,
     AccountsHandler,
     EmailsHandler,
     MacaroonDischargeHandler,
@@ -26,6 +28,9 @@
 
 v2accounts = ApiResource(
     handler=AccountsHandler, authentication=ApiAccountsAuthentication())
+v2accounts_bulk = ApiResource(
+    handler=AccountsBulkHandler,
+    authentication=ApiAccountsBulkAuthentication())
 v2emails = ApiResource(
     handler=EmailsHandler, authentication=ApiEmailsAuthentication())
 v2login = ApiResource(handler=AccountLoginHandler)
@@ -44,6 +49,7 @@
     '',
     url(r'^accounts$', v2registration, name='api-registration'),
     url(r'^accounts/(?P<openid>\w+)$', v2accounts, name='api-account'),
+    url(r'^accounts-bulk$', v2accounts_bulk, name='api-accounts-bulk'),
     url(r'^emails/(?P<email>.+)$', v2emails, name='api-email'),
     url(r'^requests/validate$', v2requests, name='api-requests'),
     url(r'^tokens/discharge$', v2macaroon_discharge,

=== modified file 'src/api/v20/utils.py'
--- src/api/v20/utils.py	2016-09-14 16:47:06 +0000
+++ src/api/v20/utils.py	2018-01-23 17:52:15 +0000
@@ -1,12 +1,18 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
+
 import json
+from operator import attrgetter
 
 from django.core.urlresolvers import reverse
 from django.http import HttpResponse
 from django.utils.translation import ugettext_lazy as _
 
-from identityprovider.models.const import AccountStatus
+from identityprovider.models.account import Account
+from identityprovider.models.const import (
+    AccountStatus,
+    TokenScope,
+)
 
 
 class EnsureJSONResponseOnAPIErrorMiddleware(object):
@@ -180,40 +186,83 @@
     return unicode(AccountStatus._verbose[status])
 
 
+def get_minimal_accounts_data(accounts):
+    """Return the minimal non-private data for a sequence of accounts."""
+    Account.objects.prefetch_person(accounts)
+    # Given that we've prefetched related EmailAddress rows already, this is
+    # equivalent to account.is_verified for each account: an account is
+    # verified if it has at least one PREFERRED or VALIDATED email address.
+    is_verified = {
+        account: (
+            account.preferredemail is not None and
+            account.preferredemail.is_verified)
+        for account in accounts
+    }
+    return {
+        account.openid_identifier: {
+            'href': reverse('api-account', args=(account.openid_identifier,)),
+            'openid': account.openid_identifier,
+            'verified': is_verified[account],
+            'username': account.person_name,
+        }
+        for account in accounts
+    }
+
+
 def get_minimal_account_data(account):
     """Return the minimal non-private data for the account."""
-    href = reverse('api-account', args=(account.openid_identifier,))
-    data = dict(
-        href=href,
-        openid=account.openid_identifier,
-        verified=account.is_verified,
-        username=account.person_name,
-    )
+    return get_minimal_accounts_data([account])[account.openid_identifier]
+
+
+def get_accounts_data(accounts, limit=10, expand=False, include_tokens=False):
+    """Get the relevant data from a sequence of accounts to be serialized.
+
+    Emails and tokens will be limited to the latest 10 results for each
+    account, where latest for email means creation date, and for tokens
+    means updated date.
+
+    """
+    data = get_minimal_accounts_data(accounts)
+
+    for account in accounts:
+        # Don't use helpers such as Account.oauth_tokens or directly slice
+        # QuerySets here, since that would invalidate previous
+        # prefetch_related calls.  Iterating over the whole
+        # EmailAddress/Token sets shouldn't be too bad in this case.
+        emails = sorted(
+            account.emailaddress_set.all(),
+            key=attrgetter('date_created'), reverse=True)[:limit]
+        data[account.openid_identifier].update({
+            'email': (
+                None if account.preferredemail is None
+                else account.preferredemail.email),
+            'displayname': account.displayname,
+            'status': _get_account_status_text(account.status),
+            'emails': [get_email_data(e, expand=expand) for e in emails],
+        })
+        if include_tokens:
+            tokens = sorted(
+                (token for token in account.token_set.all()
+                 if token.scope == TokenScope.VERSION_2),
+                key=attrgetter('date_updated'), reverse=True)[:limit]
+            data[account.openid_identifier]['tokens'] = [
+                get_token_data(t, expand=expand) for t in tokens]
+
     return data
 
 
-def get_account_data(account, limit=10, expand=False):
+# When fetching a single account, include_tokens defaults to True for
+# historical reasons, although the tokens should now be mostly unused.
+def get_account_data(account, limit=10, expand=False, include_tokens=True):
     """Get the relevant data from an account to be serialized.
 
     Emails and tokens will be limited to the latest 10 results, where latest
-    for email means creation date, and for tokens mean updated date.
+    for email means creation date, and for tokens means updated date.
 
     """
-    data = get_minimal_account_data(account)
-    email = None
-    if account.preferredemail is not None:
-        email = account.preferredemail.email
-
-    emails = account.emails(limit=limit)
-    tokens = account.oauth_tokens(limit=limit)
-    data.update(
-        email=email,
-        displayname=account.displayname,
-        status=_get_account_status_text(account.status),
-        emails=[get_email_data(e, expand) for e in emails],
-        tokens=[get_token_data(t, expand) for t in tokens],
-    )
-    return data
+    return get_accounts_data(
+        [account], limit=limit, expand=expand,
+        include_tokens=include_tokens)[account.openid_identifier]
 
 
 def get_email_data(email, expand=True):

=== modified file 'src/identityprovider/models/account.py'
--- src/identityprovider/models/account.py	2017-07-20 14:22:30 +0000
+++ src/identityprovider/models/account.py	2018-01-23 17:52:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2016 Canonical Ltd.  This software is licensed under
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under
 # the GNU Affero General Public License version 3 (see the file
 # LICENSE).
 
@@ -6,6 +6,7 @@
 from __future__ import unicode_literals
 
 import logging
+from operator import attrgetter
 
 from django.conf import settings
 from django.contrib.auth import get_backends
@@ -147,6 +148,13 @@
         return get_object_or_none(self, openid_identifier=openid_identifier,
                                   status=AccountStatus.ACTIVE)
 
+    def prefetch_person(self, accounts):
+        """Pre-fetch person for a set of accounts."""
+        from identityprovider.models.person import Person
+        for account, person in Person.objects.get_by_account(
+                accounts).items():
+            account.__dict__['person'] = person
+
 
 class DisplaynameField(models.TextField):
     def __init__(self, null=False, **kwargs):
@@ -317,18 +325,26 @@
         if email is None or not email.is_verified:
             try:
                 email = None
-                account_emails = self.emailaddress_set.filter(
-                    status=EmailStatus.PREFERRED)
-                if account_emails.count() > 0:
-                    email = account_emails[0]
+                # We assume that accounts only ever have a smallish number
+                # of email addresses; with that assumption, doing this
+                # rather than filtering on separate statuses is more
+                # prefetch-friendly and involves fewer DB round-trips.
+                all_emails = sorted(
+                    self.emailaddress_set.all(),
+                    key=attrgetter('date_created'))
+                emails = [
+                    e for e in all_emails if e.status == EmailStatus.PREFERRED]
+                if emails:
+                    email = emails[0]
 
                 if not email:
                     # Try to determine a suitable address, and mark it
                     # as preferred.
-                    emails = self.emailaddress_set.filter(
-                        status=EmailStatus.VALIDATED)
-                    if emails.count() > 0:
-                        email = emails.order_by('date_created')[0]
+                    emails = [
+                        e for e in all_emails
+                        if e.status == EmailStatus.VALIDATED]
+                    if emails:
+                        email = emails[0]
                         email.status = EmailStatus.PREFERRED
                         email.save()
                         logger.info(
@@ -339,10 +355,10 @@
                 if not email:
                     # we have no validated email, so use the first NEW email
                     # but don't save it
-                    emails = self.emailaddress_set.filter(
-                        status=EmailStatus.NEW)
-                    if emails.count() > 0:
-                        email = emails.order_by('date_created')[0]
+                    emails = [
+                        e for e in all_emails if e.status == EmailStatus.NEW]
+                    if emails:
+                        email = emails[0]
 
                 self._preferredemail = email
             except:

=== modified file 'src/identityprovider/models/person.py'
--- src/identityprovider/models/person.py	2016-04-28 13:01:03 +0000
+++ src/identityprovider/models/person.py	2018-01-23 17:52:15 +0000
@@ -1,9 +1,12 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from __future__ import unicode_literals
 
-from django.db import models
+from django.db import (
+    connection,
+    models,
+)
 
 from identityprovider.const import PERSON_VISIBILITY_PUBLIC
 from identityprovider.models.account import Account, LPOpenIdIdentifier
@@ -16,6 +19,44 @@
 )
 
 
+class PersonManager(models.Manager):
+    """A custom manager for Person models."""
+
+    def get_by_account(self, accounts):
+        """Return Person instances for a set of accounts.
+
+        The return value is a dict mapping Account instances to Person
+        instances.
+
+        """
+        if not accounts:
+            return {}
+        accounts_by_id = {account.id: account for account in accounts}
+        # This is rather cumbersome because we don't have Django model
+        # relationships for the tables synced over from Launchpad.
+        cursor = connection.cursor()
+        cursor.execute("""
+            SELECT account.id, lp_person.id
+            FROM account, lp_openididentifier, lp_person
+            WHERE
+                lp_openididentifier.account = lp_person.account
+                AND lp_openididentifier.identifier = account.openid_identifier
+                AND account.id IN %s
+            """, (tuple(accounts_by_id),))
+        rows = cursor.fetchall()
+        persons = self.filter(
+            id__in={person_id for _, person_id in rows}).all()
+        persons_by_id = {person.id: person for person in persons}
+        result = {
+            accounts_by_id[account_id]: persons_by_id[person_id]
+            for account_id, person_id in rows
+        }
+        for account in accounts:
+            if account not in result:
+                result[account] = None
+        return result
+
+
 class Person(models.Model):
     displayname = models.TextField(null=True, blank=True)
     teamowner = models.IntegerField(db_column=b'teamowner',
@@ -65,6 +106,8 @@
     lp_account = models.IntegerField(null=True, db_column=b'account',
                                      unique=True)
 
+    objects = PersonManager()
+
     class Meta:
         app_label = 'identityprovider'
         db_table = u'lp_person'

=== modified file 'src/identityprovider/signals.py'
--- src/identityprovider/signals.py	2016-05-30 21:41:39 +0000
+++ src/identityprovider/signals.py	2018-01-23 17:52:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 import logging
 import traceback
@@ -16,6 +16,7 @@
 from identityprovider.const import SESSION_TOKEN_KEY, SESSION_TOKEN_NAME
 from identityprovider.models import Account, AccountPassword, AuthLog, Token
 from identityprovider.models.const import AuthLogType, TokenScope
+from identityprovider.models.emailaddress import EmailAddress
 from webservices.utils import http_request_with_timeout
 
 
@@ -180,3 +181,17 @@
         )
 
 pre_delete.connect(track_isdtest_account_deletion, sender=Account)
+
+
+# account.preferredemail may be prefetched for API performance, but deleting
+# EmailAddress rows may invalidate the prefetched value.
+def invalidate_preferredemail(sender, instance, using, **kwargs):
+    account_cache_name = instance._meta.get_field('account').get_cache_name()
+    account = getattr(instance, account_cache_name, None)
+    if account is not None:
+        try:
+            del account._preferredemail
+        except AttributeError:
+            pass
+
+pre_delete.connect(invalidate_preferredemail, sender=EmailAddress)

=== modified file 'src/identityprovider/tests/test_models_account.py'
--- src/identityprovider/tests/test_models_account.py	2016-12-13 14:30:16 +0000
+++ src/identityprovider/tests/test_models_account.py	2018-01-23 17:52:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 
@@ -252,6 +252,15 @@
             else:
                 self.assertIsNone(result)
 
+    def test_prefetch_person(self):
+        accounts = [self.factory.make_account() for _ in range(3)]
+        persons = [
+            self.factory.make_person(account=account)
+            for account in accounts[:2]] + [None]
+        Account.objects.prefetch_person(accounts)
+        with self.assertNumQueries(0):
+            self.assertEqual([account.person for account in accounts], persons)
+
     def test_verified_list_only_verified_accounts(self):
         accounts = Account.objects.verified()
         for account in accounts:

=== modified file 'src/identityprovider/tests/test_models_person.py'
--- src/identityprovider/tests/test_models_person.py	2016-04-28 13:01:03 +0000
+++ src/identityprovider/tests/test_models_person.py	2018-01-23 17:52:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 from random import randint
 
@@ -18,6 +18,45 @@
     AccountStatus,
 )
 from identityprovider.tests.utils import SSOBaseTestCase
+from testing.helpers import assert_no_extra_queries_after
+
+
+class PersonManagerTestCase(SSOBaseTestCase):
+
+    def test_get_by_account_no_accounts(self):
+        with self.assertNumQueries(0):
+            self.assertEqual(Person.objects.get_by_account([]), {})
+
+    def test_get_by_account(self):
+        accounts = [self.factory.make_account() for _ in range(10)]
+        persons = [
+            self.factory.make_person(account=account)
+            for account in accounts[:5]]
+        self.factory.make_person()
+
+        with self.assertNumQueries(2):
+            self.assertEqual(
+                Person.objects.get_by_account(accounts[:3] + accounts[5:8]),
+                {
+                    accounts[0]: persons[0],
+                    accounts[1]: persons[1],
+                    accounts[2]: persons[2],
+                    accounts[5]: None,
+                    accounts[6]: None,
+                    accounts[7]: None,
+                })
+
+    def test_get_by_account_constant_query_count(self):
+        accounts = []
+
+        def add_accounts():
+            accounts.append(self.factory.make_account())
+            self.factory.make_person(account=accounts[-1])
+            accounts.append(self.factory.make_account())
+
+        add_accounts()
+        assert_no_extra_queries_after(
+            add_accounts, Person.objects.get_by_account, accounts)
 
 
 class PersonTestCase(SSOBaseTestCase):

=== modified file 'src/identityprovider/tests/test_signals.py'
--- src/identityprovider/tests/test_signals.py	2015-11-05 16:43:46 +0000
+++ src/identityprovider/tests/test_signals.py	2018-01-23 17:52:15 +0000
@@ -1,28 +1,37 @@
 # -*- coding: utf-8 -*-
 
-# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2013-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from django.conf import settings
 from django.contrib.auth.signals import user_logged_in
 from django.core.urlresolvers import reverse
-from django.db.models.signals import post_save
+from django.db.models.signals import (
+    post_save,
+    pre_delete,
+)
 from django.utils.timezone import now
 
 from mock import Mock, patch
 
 from identityprovider.const import SESSION_TOKEN_KEY, SESSION_TOKEN_NAME
-from identityprovider.models.const import AuthLogType, TokenScope
+from identityprovider.models.const import (
+    AuthLogType,
+    EmailStatus,
+    TokenScope,
+)
+from identityprovider.models.emailaddress import EmailAddress
+from identityprovider.readonly import ReadOnlyManager
 from identityprovider.signals import (
     invalidate_account_oauth_tokens,
-    login_failed,
+    invalidate_preferredemail,
     log_login_failed,
     log_login_succeeded,
+    login_failed,
+    login_succeeded,
     set_session_oauth_token,
     track_failed_login,
-    login_succeeded
 )
-from identityprovider.readonly import ReadOnlyManager
 from identityprovider.tests import DEFAULT_USER_PASSWORD
 from identityprovider.tests.test_auth import AuthLogTestCaseMixin
 from identityprovider.tests.utils import SSOBaseTestCase
@@ -304,3 +313,53 @@
             log_login_succeeded(
                 sender, self.account, request, email='[e-mail]',
                 authlogtype=AuthLogType.OPENID_EXISTING)
+
+
+class InvalidatePreferredEmailTestCase(SSOBaseTestCase):
+
+    def test_signal_connected(self):
+        registered_functions = [r[1]() for r in pre_delete.receivers]
+        self.assertIn(invalidate_preferredemail, registered_functions)
+
+    def test_invalidates_on_email_address_deletion(self):
+        account = self.factory.make_account()
+        email = self.factory.make_email_for_account(
+            account, status=EmailStatus.PREFERRED)
+        self.assertEqual(account.preferredemail, email)
+        # Avoid spurious pass if the property caching doesn't work.
+        with self.assertNumQueries(0):
+            self.assertEqual(account.preferredemail, email)
+
+        account.emailaddress_set.all().delete()
+        with self.assertNumQueries(1):
+            self.assertIsNone(account.preferredemail)
+
+    def test_leaves_unrelated_account_untouched(self):
+        accounts = [self.factory.make_account() for _ in range(2)]
+        emails = [
+            self.factory.make_email_for_account(
+                account=account, status=EmailStatus.PREFERRED)
+            for account in accounts
+        ]
+        for account, email in zip(accounts, emails):
+            self.assertEqual(account.preferredemail, email)
+            with self.assertNumQueries(0):
+                self.assertEqual(account.preferredemail, email)
+
+        accounts[0].emailaddress_set.all().delete()
+        with self.assertNumQueries(1):
+            self.assertIsNone(accounts[0].preferredemail)
+        with self.assertNumQueries(0):
+            self.assertEqual(accounts[1].preferredemail, email)
+
+    def test_ignores_email_without_account(self):
+        person = self.factory.make_person()
+        email = EmailAddress.objects.create(
+            email=self.factory.make_email_address(),
+            lp_person=person.id,
+            status=EmailStatus.PREFERRED,
+            account=None)
+
+        # There's no account to modify in this case; just make sure it
+        # doesn't raise an exception.
+        EmailAddress.objects.filter(email=email.email).delete()

=== added file 'src/testing/helpers.py'
--- src/testing/helpers.py	1970-01-01 00:00:00 +0000
+++ src/testing/helpers.py	2018-01-23 17:52:15 +0000
@@ -0,0 +1,66 @@
+# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+from operator import itemgetter
+
+from django.db import connection
+from django.test.utils import CaptureQueriesContext
+
+
+def format_queries(qs):
+    return "\n\n".join(map(str, map(itemgetter('sql'), qs)))
+
+
+def assert_no_extra_queries_after(modify_data, f, *args, **kwargs):
+    """Assert function performs equal query numbers after some change.
+
+    Many views perform terribly because they perform extra queries
+    for every item in some list. For example when looping over a
+    list of users to present them, if a value is accessed through
+    a foreign key then it may perform an extra query. If care
+    is not taken then the view may perform terribly when
+    operating on a large list. This is often worked around via
+    .prefetch_related() or .select_related().
+
+    This function helps in testing that this doesn't happen. The arguments
+    to this function are a callable to modify data, and a function
+    to call before and after (*args and **kwargs are passed
+    through to the latter function). Both calls to the function
+    should issue the same number of queries.
+
+    For instance
+
+    self.factory.make_user()
+    assert_no_extra_queries_after(
+        self.factory.make_user,
+        self.client.get, '/users'):
+
+    Will first request the view and record how many queries were
+    done. It will then run the modify function, creating another user
+    in this case, and then request the view again. If the second
+    request doesn't issue the same number of queries then an
+    AssertionError will be raised.
+
+    """
+
+    first = CaptureQueriesContext(connection)
+    with first:
+        f(*args, **kwargs)
+    modify_data()
+    second = CaptureQueriesContext(connection)
+    with second:
+        result = f(*args, **kwargs)
+
+    if len(first) != len(second):
+        message = ("Different number of queries for the "
+                   "second execution. {} != {}".format(
+                       len(first), len(second)))
+        message += "\nFirst queries:\n"
+        message += format_queries(first.captured_queries)
+        message += "\n\n\nSecond queries:\n"
+        message += format_queries(second.captured_queries)
+        raise AssertionError(message)
+
+    return result