diff --git a/resources/sql/autopatches/20160604.user.02.removeimagecache.sql b/resources/sql/autopatches/20160604.user.02.removeimagecache.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20160604.user.02.removeimagecache.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_user.user + DROP COLUMN profileImageCache; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3650,6 +3650,7 @@ '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 @@ 'PhabricatorUserPreferencesTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorUserProfile' => 'PhabricatorUserDAO', 'PhabricatorUserProfileEditor' => 'PhabricatorApplicationTransactionEditor', + 'PhabricatorUserProfileImageCacheType' => 'PhabricatorUserCacheType', 'PhabricatorUserRealNameField' => 'PhabricatorUserCustomField', 'PhabricatorUserRolesField' => 'PhabricatorUserCustomField', 'PhabricatorUserSchemaSpec' => 'PhabricatorConfigSchemaSpec', diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -113,7 +113,7 @@ $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 @@ $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 @@ $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 @@ $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; } } diff --git a/src/applications/people/cache/PhabricatorUserCacheType.php b/src/applications/people/cache/PhabricatorUserCacheType.php --- a/src/applications/people/cache/PhabricatorUserCacheType.php +++ b/src/applications/people/cache/PhabricatorUserCacheType.php @@ -18,6 +18,14 @@ 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); } diff --git a/src/applications/people/cache/PhabricatorUserProfileImageCacheType.php b/src/applications/people/cache/PhabricatorUserProfileImageCacheType.php new file mode 100644 --- /dev/null +++ b/src/applications/people/cache/PhabricatorUserProfileImageCacheType.php @@ -0,0 +1,82 @@ +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); + } + +} diff --git a/src/applications/people/extension/PhabricatorPeopleMainMenuBarExtension.php b/src/applications/people/extension/PhabricatorPeopleMainMenuBarExtension.php --- a/src/applications/people/extension/PhabricatorPeopleMainMenuBarExtension.php +++ b/src/applications/people/extension/PhabricatorPeopleMainMenuBarExtension.php @@ -7,15 +7,7 @@ public function buildMainMenus() { $viewer = $this->getViewer(); - - // TODO: This should get cached. - - $profile = id(new PhabricatorPeopleQuery()) - ->setViewer($viewer) - ->needProfileImage(true) - ->withPHIDs(array($viewer->getPHID())) - ->executeOne(); - $image = $profile->getProfileImageURI(); + $image = $viewer->getProfileImageURI(); $bar_item = id(new PHUIListItemView()) ->setName($viewer->getUsername()) diff --git a/src/applications/people/query/PhabricatorPeopleQuery.php b/src/applications/people/query/PhabricatorPeopleQuery.php --- a/src/applications/people/query/PhabricatorPeopleQuery.php +++ b/src/applications/people/query/PhabricatorPeopleQuery.php @@ -106,7 +106,14 @@ } 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 @@ } } - 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 @@ $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 @@ $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'); diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -30,7 +30,6 @@ protected $passwordSalt; protected $passwordHash; protected $profileImagePHID; - protected $profileImageCache; protected $availabilityCache; protected $availabilityCacheTTL; @@ -46,7 +45,6 @@ protected $accountSecret; - private $profileImage = self::ATTACHABLE; private $profile = null; private $availability = self::ATTACHABLE; private $preferences = null; @@ -196,7 +194,6 @@ 'isApproved' => 'uint32', 'accountSecret' => 'bytes64', 'isEnrolledInMultiFactor' => 'bool', - 'profileImageCache' => 'text255?', 'availabilityCache' => 'text255?', 'availabilityCacheTTL' => 'uint32?', ), @@ -218,7 +215,6 @@ ), ), self::CONFIG_NO_MUTATE => array( - 'profileImageCache' => true, 'availabilityCache' => true, 'availabilityCacheTTL' => true, ), @@ -791,13 +787,9 @@ 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 @@ } -/* -( 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 @@ 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( diff --git a/src/applications/people/storage/PhabricatorUserCache.php b/src/applications/people/storage/PhabricatorUserCache.php --- a/src/applications/people/storage/PhabricatorUserCache.php +++ b/src/applications/people/storage/PhabricatorUserCache.php @@ -86,7 +86,9 @@ $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); }