Convert user profile images into a standard cache

Summary:
Ref T4103. Ref T10078. This moves profile image caches to new usercache infrastructure.

These dirty automatically based on configuration and User properties, so add some stuff to make that happen.

This reduces the number of queries issued on every page by 1.

Test Plan: Browsed around, changed profile image, viewed as self, viewed as another user, verified no more query to pull this information on every page

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4103, T10078

Differential Revision: https://secure.phabricator.com/D16040

resources/sql/autopatches/20160604.user.02.removeimagecache.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_user.user
+  DROP COLUMN profileImageCache;

src/__phutil_library_map__.php

@@ -3650,6 +3650,7 @@ phutil_register_library_map(array(
     'PhabricatorUserPreferencesTransactionQuery' => 'applications/settings/query/PhabricatorUserPreferencesTransactionQuery.php',
     'PhabricatorUserProfile' => 'applications/people/storage/PhabricatorUserProfile.php',
     'PhabricatorUserProfileEditor' => 'applications/people/editor/PhabricatorUserProfileEditor.php',
+    'PhabricatorUserProfileImageCacheType' => 'applications/people/cache/PhabricatorUserProfileImageCacheType.php',
     'PhabricatorUserRealNameField' => 'applications/people/customfield/PhabricatorUserRealNameField.php',
     'PhabricatorUserRolesField' => 'applications/people/customfield/PhabricatorUserRolesField.php',
     'PhabricatorUserSchemaSpec' => 'applications/people/storage/PhabricatorUserSchemaSpec.php',
@@ -8465,6 +8466,7 @@ phutil_register_library_map(array(
     'PhabricatorUserPreferencesTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PhabricatorUserProfile' => 'PhabricatorUserDAO',
     'PhabricatorUserProfileEditor' => 'PhabricatorApplicationTransactionEditor',
+    'PhabricatorUserProfileImageCacheType' => 'PhabricatorUserCacheType',
     'PhabricatorUserRealNameField' => 'PhabricatorUserCustomField',
     'PhabricatorUserRolesField' => 'PhabricatorUserCustomField',
     'PhabricatorUserSchemaSpec' => 'PhabricatorConfigSchemaSpec',

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -113,7 +113,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     $session_key = PhabricatorHash::digest($session_token);
 
     $cache_parts = $this->getUserCacheQueryParts($conn_r);
-    list($cache_selects, $cache_joins, $cache_map) = $cache_parts;
+    list($cache_selects, $cache_joins, $cache_map, $types_map) = $cache_parts;
 
     $info = queryfx_one(
       $conn_r,
@@ -162,6 +162,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
 
     $user = $user_table->loadFromArray($info);
 
+    $cache_raw = $this->filterRawCacheData($user, $types_map, $cache_raw);
     $user->attachRawCacheData($cache_raw);
     $user->setAllowInlineCacheGeneration(true);
 
@@ -761,11 +762,13 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     $cache_map = array();
 
     $keys = array();
+    $types_map = array();
 
     $cache_types = PhabricatorUserCacheType::getAllCacheTypes();
     foreach ($cache_types as $cache_type) {
       foreach ($cache_type->getAutoloadKeys() as $autoload_key) {
         $keys[] = $autoload_key;
+        $types_map[$autoload_key] = $cache_type;
       }
     }
 
@@ -809,7 +812,24 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       $cache_joins = '';
     }
 
-    return array($cache_selects, $cache_joins, $cache_map);
+    return array($cache_selects, $cache_joins, $cache_map, $types_map);
+  }
+
+  private function filterRawCacheData(
+    PhabricatorUser $user,
+    array $types_map,
+    array $cache_raw) {
+
+    foreach ($cache_raw as $cache_key => $cache_data) {
+      $type = $types_map[$cache_key];
+      if ($type->shouldValidateRawCacheData()) {
+        if (!$type->isRawCacheDataValid($user, $cache_key, $cache_data)) {
+          unset($cache_raw[$cache_key]);
+        }
+      }
+    }
+
+    return $cache_raw;
   }
 
 }

src/applications/people/cache/PhabricatorUserCacheType.php

@@ -18,6 +18,14 @@ abstract class PhabricatorUserCacheType extends Phobject {
     return array();
   }
 
+  public function shouldValidateRawCacheData() {
+    return false;
+  }
+
+  public function isRawCacheDataValid(PhabricatorUser $user, $key, $data) {
+    throw new PhutilMethodNotImplementedException();
+  }
+
   public function getValueFromStorage($value) {
     return phutil_json_decode($value);
   }

src/applications/people/cache/PhabricatorUserProfileImageCacheType.php

@@ -0,0 +1,82 @@
+<?php
+
+final class PhabricatorUserProfileImageCacheType
+  extends PhabricatorUserCacheType {
+
+  const CACHETYPE = 'user.profile';
+
+  const KEY_URI = 'user.profile.image.uri.v1';
+
+  public function getAutoloadKeys() {
+    return array(
+      self::KEY_URI,
+    );
+  }
+
+  public function canManageKey($key) {
+    return ($key === self::KEY_URI);
+  }
+
+  public function newValueForUsers($key, array $users) {
+    $viewer = $this->getViewer();
+
+    $file_phids = mpull($users, 'getProfileImagePHID');
+    $file_phids = array_filter($file_phids);
+
+    if ($file_phids) {
+      $files = id(new PhabricatorFileQuery())
+        ->setViewer($viewer)
+        ->withPHIDs($file_phids)
+        ->execute();
+      $files = mpull($files, null, 'getPHID');
+    } else {
+      $files = array();
+    }
+
+    $results = array();
+    foreach ($users as $user) {
+      $image_phid = $user->getProfileImagePHID();
+      if (isset($files[$image_phid])) {
+        $image_uri = $files[$image_phid]->getBestURI();
+      } else {
+        $image_uri = PhabricatorUser::getDefaultProfileImageURI();
+      }
+
+      $user_phid = $user->getPHID();
+      $version = $this->getCacheVersion($user);
+      $results[$user_phid] = "{$version},{$image_uri}";
+    }
+
+    return $results;
+  }
+
+  public function getValueFromStorage($value) {
+    $parts = explode(',', $value, 2);
+    return end($parts);
+  }
+
+  public function getValueForStorage($value) {
+    return $value;
+  }
+
+  public function shouldValidateRawCacheData() {
+    return true;
+  }
+
+  public function isRawCacheDataValid(PhabricatorUser $user, $key, $data) {
+    $parts = explode(',', $data, 2);
+    $version = reset($parts);
+    return ($version === $this->getCacheVersion($user));
+  }
+
+  private function getCacheVersion(PhabricatorUser $user) {
+    $parts = array(
+      PhabricatorEnv::getCDNURI('/'),
+      PhabricatorEnv::getEnvConfig('cluster.instance'),
+      $user->getProfileImagePHID(),
+    );
+    $parts = serialize($parts);
+    return PhabricatorHash::digestForIndex($parts);
+  }
+
+}

src/applications/people/extension/PhabricatorPeopleMainMenuBarExtension.php

@@ -7,15 +7,7 @@ final class PhabricatorPeopleMainMenuBarExtension
 
   public function buildMainMenus() {
     $viewer = $this->getViewer();
-
+    $image = $viewer->getProfileImageURI();
-    // TODO: This should get cached.
-
-    $profile = id(new PhabricatorPeopleQuery())
-      ->setViewer($viewer)
-      ->needProfileImage(true)
-      ->withPHIDs(array($viewer->getPHID()))
-      ->executeOne();
-    $image = $profile->getProfileImageURI();
 
     $bar_item = id(new PHUIListItemView())
       ->setName($viewer->getUsername())

src/applications/people/query/PhabricatorPeopleQuery.php

@@ -106,7 +106,14 @@ final class PhabricatorPeopleQuery
   }
 
   public function needProfileImage($need) {
-    $this->needProfileImage = $need;
+    $cache_key = PhabricatorUserProfileImageCacheType::KEY_URI;
+
+    if ($need) {
+      $this->cacheKeys[$cache_key] = true;
+    } else {
+      unset($this->cacheKeys[$cache_key]);
+    }
+
     return $this;
   }
 
@@ -182,59 +189,6 @@ final class PhabricatorPeopleQuery
       }
     }
 
-    if ($this->needProfileImage) {
-      $rebuild = array();
-      foreach ($users as $user) {
-        $image_uri = $user->getProfileImageCache();
-        if ($image_uri) {
-          // This user has a valid cache, so we don't need to fetch any
-          // data or rebuild anything.
-
-          $user->attachProfileImageURI($image_uri);
-          continue;
-        }
-
-        // This user's cache is invalid or missing, so we're going to rebuild
-        // it.
-        $rebuild[] = $user;
-      }
-
-      if ($rebuild) {
-        $file_phids = mpull($rebuild, 'getProfileImagePHID');
-        $file_phids = array_filter($file_phids);
-
-        if ($file_phids) {
-          // NOTE: We're using the omnipotent user here because older profile
-          // images do not have the 'profile' flag, so they may not be visible
-          // to the executing viewer. At some point, we could migrate to add
-          // this flag and then use the real viewer, or just use the real
-          // viewer after enough time has passed to limit the impact of old
-          // data. The consequence of missing here is that we cache a default
-          // image when a real image exists.
-          $files = id(new PhabricatorFileQuery())
-            ->setParentQuery($this)
-            ->setViewer(PhabricatorUser::getOmnipotentUser())
-            ->withPHIDs($file_phids)
-            ->execute();
-          $files = mpull($files, null, 'getPHID');
-        } else {
-          $files = array();
-        }
-
-        foreach ($rebuild as $user) {
-          $image_phid = $user->getProfileImagePHID();
-          if (isset($files[$image_phid])) {
-            $image_uri = $files[$image_phid]->getBestURI();
-          } else {
-            $image_uri = PhabricatorUser::getDefaultProfileImageURI();
-          }
-
-          $user->writeProfileImageCache($image_uri);
-          $user->attachProfileImageURI($image_uri);
-        }
-      }
-    }
-
     if ($this->needAvailability) {
       $rebuild = array();
       foreach ($users as $user) {
@@ -509,6 +463,8 @@ final class PhabricatorPeopleQuery
       $hashes[] = PhabricatorHash::digestForIndex($key);
     }
 
+    $types = PhabricatorUserCacheType::getAllCacheTypes();
+
     // First, pull any available caches. If we wanted to be particularly clever
     // we could do this with JOINs in the main query.
 
@@ -517,12 +473,43 @@ final class PhabricatorPeopleQuery
 
     $cache_data = queryfx_all(
       $cache_conn,
-      'SELECT cacheKey, userPHID, cacheData FROM %T
+      'SELECT cacheKey, userPHID, cacheData, cacheType FROM %T
         WHERE cacheIndex IN (%Ls) AND userPHID IN (%Ls)',
       $cache_table->getTableName(),
       $hashes,
       array_keys($user_map));
 
+    $skip_validation = array();
+
+    // After we read caches from the database, discard any which have data that
+    // invalid or out of date. This allows cache types to implement TTLs or
+    // versions instead of or in addition to explicit cache clears.
+    foreach ($cache_data as $row_key => $row) {
+      $cache_type = $row['cacheType'];
+
+      if (isset($skip_validation[$cache_type])) {
+        continue;
+      }
+
+      if (empty($types[$cache_type])) {
+        unset($cache_data[$row_key]);
+        continue;
+      }
+
+      $type = $types[$cache_type];
+      if (!$type->shouldValidateRawCacheData()) {
+        $skip_validation[$cache_type] = true;
+        continue;
+      }
+
+      $user = $user_map[$row['userPHID']];
+      $raw_data = $row['cacheData'];
+      if (!$type->isRawCacheDataValid($user, $row['cacheKey'], $raw_data)) {
+        unset($cache_data[$row_key]);
+        continue;
+      }
+    }
+
     $need = array();
 
     $cache_data = igroup($cache_data, 'userPHID');

src/applications/people/storage/PhabricatorUser.php

@@ -30,7 +30,6 @@ final class PhabricatorUser
   protected $passwordSalt;
   protected $passwordHash;
   protected $profileImagePHID;
-  protected $profileImageCache;
   protected $availabilityCache;
   protected $availabilityCacheTTL;
 
@@ -46,7 +45,6 @@ final class PhabricatorUser
 
   protected $accountSecret;
 
-  private $profileImage = self::ATTACHABLE;
   private $profile = null;
   private $availability = self::ATTACHABLE;
   private $preferences = null;
@@ -196,7 +194,6 @@ final class PhabricatorUser
         'isApproved' => 'uint32',
         'accountSecret' => 'bytes64',
         'isEnrolledInMultiFactor' => 'bool',
-        'profileImageCache' => 'text255?',
         'availabilityCache' => 'text255?',
         'availabilityCacheTTL' => 'uint32?',
       ),
@@ -218,7 +215,6 @@ final class PhabricatorUser
         ),
       ),
       self::CONFIG_NO_MUTATE => array(
-        'profileImageCache' => true,
         'availabilityCache' => true,
         'availabilityCacheTTL' => true,
       ),
@@ -791,13 +787,9 @@ final class PhabricatorUser
     return celerity_get_resource_uri('/rsrc/image/avatar.png');
   }
 
-  public function attachProfileImageURI($uri) {
-    $this->profileImage = $uri;
-    return $this;
-  }
-
   public function getProfileImageURI() {
-    return $this->assertAttached($this->profileImage);
+    $uri_key = PhabricatorUserProfileImageCacheType::KEY_URI;
+    return $this->requireCacheData($uri_key);
   }
 
   public function getFullName() {
@@ -1008,72 +1000,6 @@ final class PhabricatorUser
   }
 
 
-/* -(  Profile Image Cache  )------------------------------------------------ */
-
-
-  /**
-   * Get this user's cached profile image URI.
-   *
-   * @return string|null Cached URI, if a URI is cached.
-   * @task image-cache
-   */
-  public function getProfileImageCache() {
-    $version = $this->getProfileImageVersion();
-
-    $parts = explode(',', $this->profileImageCache, 2);
-    if (count($parts) !== 2) {
-      return null;
-    }
-
-    if ($parts[0] !== $version) {
-      return null;
-    }
-
-    return $parts[1];
-  }
-
-
-  /**
-   * Generate a new cache value for this user's profile image.
-   *
-   * @return string New cache value.
-   * @task image-cache
-   */
-  public function writeProfileImageCache($uri) {
-    $version = $this->getProfileImageVersion();
-    $cache = "{$version},{$uri}";
-
-    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-    queryfx(
-      $this->establishConnection('w'),
-      'UPDATE %T SET profileImageCache = %s WHERE id = %d',
-      $this->getTableName(),
-      $cache,
-      $this->getID());
-    unset($unguarded);
-  }
-
-
-  /**
-   * Get a version identifier for a user's profile image.
-   *
-   * This version will change if the image changes, or if any of the
-   * environment configuration which goes into generating a URI changes.
-   *
-   * @return string Cache version.
-   * @task image-cache
-   */
-  private function getProfileImageVersion() {
-    $parts = array(
-      PhabricatorEnv::getCDNURI('/'),
-      PhabricatorEnv::getEnvConfig('cluster.instance'),
-      $this->getProfileImagePHID(),
-    );
-    $parts = serialize($parts);
-    return PhabricatorHash::digestForIndex($parts);
-  }
-
-
 /* -(  Multi-Factor Authentication  )---------------------------------------- */
 
 
@@ -1529,6 +1455,7 @@ final class PhabricatorUser
       if (array_key_exists($user_phid, $map)) {
         $usable_value = $map[$user_phid];
         $raw_value = $type->getValueForStorage($usable_value);
+        $usable_value = $type->getValueFromStorage($raw_value);
 
         $this->rawCacheData[$key] = $raw_value;
         PhabricatorUserCache::writeCache(

src/applications/people/storage/PhabricatorUserCache.php

@@ -86,7 +86,9 @@ final class PhabricatorUserCache extends PhabricatorUserDAO {
         $conn_w,
         'INSERT INTO %T (userPHID, cacheIndex, cacheKey, cacheData, cacheType)
           VALUES %Q
-          ON DUPLICATE KEY UPDATE cacheData = VALUES(cacheData)',
+          ON DUPLICATE KEY UPDATE
+            cacheData = VALUES(cacheData),
+            cacheType = VALUES(cacheType)',
         $table->getTableName(),
         $chunk);
     }