D16040.id38591.diff
No OneTemporary
Actions

Size

16 KB

Referenced Files

None

Subscribers

None

D16040.id38591.diff
View Options

	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,7 +162,9 @@

	$user = $user_table->loadFromArray($info);

	+ $cache_raw = $this->filterRawCacheData($user, $types_map, $cache_raw);
	$user->attachRawCacheData($cache_raw);
	+ $user->setAllowInlineCacheGeneration(true);

	switch ($session_type) {
	case PhabricatorAuthSession::TYPE_WEB:
	@@ -760,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;
	}
	}

	@@ -808,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 @@
	+<?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);
	+ }
	+
	+}
	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,20 +473,48 @@

	$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');
	foreach ($user_map as $user_phid => $user) {
	$raw_rows = idx($cache_data, $user_phid, array());
	- if (!$raw_rows) {
	- continue;
	- }
	$raw_data = ipull($raw_rows, 'cacheData', 'cacheKey');

	foreach ($keys as $key) {
	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;
	@@ -65,6 +63,7 @@

	private $settingCacheKeys = array();
	private $settingCache = array();
	+ private $allowCacheGeneration;

	protected function readField($field) {
	switch ($field) {
	@@ -195,7 +194,6 @@
	'isApproved' => 'uint32',
	'accountSecret' => 'bytes64',
	'isEnrolledInMultiFactor' => 'bool',
	- 'profileImageCache' => 'text255?',
	'availabilityCache' => 'text255?',
	'availabilityCacheTTL' => 'uint32?',
	),
	@@ -217,7 +215,6 @@
	),
	),
	self::CONFIG_NO_MUTATE => array(
	- 'profileImageCache' => true,
	'availabilityCache' => true,
	'availabilityCacheTTL' => true,
	),
	@@ -786,13 +783,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() {
	@@ -1003,72 +996,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 )---------------------------------------- */


	@@ -1486,6 +1413,10 @@
	return $this;
	}

	+ public function setAllowInlineCacheGeneration($allow_cache_generation) {
	+ $this->allowCacheGeneration = $allow_cache_generation;
	+ return $this;
	+ }

	/**
	* @task cache
	@@ -1506,6 +1437,12 @@
	return $usable_value;
	}

	+ // By default, we throw if a cache isn't available. This is consistent
	+ // with the standard `needX()` + `attachX()` + `getX()` interaction.
	+ if (!$this->allowCacheGeneration) {
	+ throw new PhabricatorDataNotAttachedException($this);
	+ }
	+
	$usable_value = $type->getDefaultValue();

	$user_phid = $this->getPHID();
	@@ -1514,6 +1451,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);
	}

File Metadata

Mime Type: text/plain
Expires: Fri, Dec 27, 3:03 PM (8 h, 8 m)
Storage Engine: blob
Storage Format: Encrypted (AES-256-CBC)
Storage Handle: 6932234
Default Alt Text: D16040.id38591.diff (16 KB)

D16040.id38591.diffNo OneTemporaryActions

D16040.id38591.diffView Options

File Metadata

Event Timeline

D16040.id38591.diff
No OneTemporary
Actions

D16040.id38591.diff
View Options