Merge pull request #1158 from nextcloud/cache_avatars

Cache avatars

core/Controller/AvatarController.php

@@ -27,6 +27,7 @@
  */
 namespace OC\Core\Controller;
 
+use OC\AppFramework\Utility\TimeFactory;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataDisplayResponse;
@@ -73,6 +74,9 @@ class AvatarController extends Controller {
 	/** @var string */
 	protected $userId;
 
+	/** @var TimeFactory */
+	protected $timeFactory;
+
 	/**
 	 * @param string $appName
 	 * @param IRequest $request
@@ -83,6 +87,7 @@ class AvatarController extends Controller {
 	 * @param IRootFolder $rootFolder
 	 * @param ILogger $logger
 	 * @param string $userId
+	 * @param TimeFactory $timeFactory
 	 */
 	public function __construct($appName,
 								IRequest $request,
@@ -92,7 +97,8 @@ class AvatarController extends Controller {
 								IUserManager $userManager,
 								IRootFolder $rootFolder,
 								ILogger $logger,
-								$userId) {
+								$userId,
+								TimeFactory $timeFactory) {
 		parent::__construct($appName, $request);
 
 		$this->avatarManager = $avatarManager;
@@ -102,6 +108,7 @@ class AvatarController extends Controller {
 		$this->rootFolder = $rootFolder;
 		$this->logger = $logger;
 		$this->userId = $userId;
+		$this->timeFactory = $timeFactory;
 	}
 
 	/**
@@ -126,6 +133,21 @@ class AvatarController extends Controller {
 				Http::STATUS_OK,
 				['Content-Type' => $avatar->getMimeType()]);
 			$resp->setETag($avatar->getEtag());
+
+			// Let cache this!
+			$resp->addHeader('Pragma', 'public');
+			// Cache for 15 minutes
+			$resp->cacheFor(900);
+			// Set last modified
+			$lastModified = new \DateTime();
+			$lastModified->setTimestamp($avatar->getMTime());
+			$resp->setLastModified($lastModified);
+
+			$expires = new \DateTime();
+			$expires->setTimestamp($this->timeFactory->getTime());
+			$expires->add(new \DateInterval('PT15M'));
+			$resp->addHeader('Expires', $expires->format(\DateTime::RFC2822));
+
 		} catch (NotFoundException $e) {
 			$user = $this->userManager->get($userId);
 			$resp = new JSONResponse([
@@ -133,18 +155,20 @@ class AvatarController extends Controller {
 					'displayname' => $user->getDisplayName(),
 				],
 			]);
+			// Don't cache this
+			$resp->cacheFor(0);
+			$resp->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
 		} catch (\Exception $e) {
 			$resp = new JSONResponse([
 				'data' => [
 					'displayname' => '',
 				],
 			]);
+			// Don't cache this
+			$resp->cacheFor(0);
+			$resp->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
 		}
 
-		$resp->addHeader('Pragma', 'public');
-		$resp->cacheFor(0);
-		$resp->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
-
 		return $resp;
 	}
 

core/js/config.php

@@ -149,8 +149,8 @@ $array = array(
 	"firstDay" => json_encode($l->l('firstday', null)) ,
 	"oc_config" => json_encode(
 		array(
-			'session_lifetime'	=> min(\OCP\Config::getSystemValue('session_lifetime', OC::$server->getIniWrapper()->getNumeric('session.gc_maxlifetime')), OC::$server->getIniWrapper()->getNumeric('session.gc_maxlifetime')),
-			'session_keepalive'	=> \OCP\Config::getSystemValue('session_keepalive', true),
+			'session_lifetime'	=> min($config->getSystemValue('session_lifetime', OC::$server->getIniWrapper()->getNumeric('session.gc_maxlifetime')), OC::$server->getIniWrapper()->getNumeric('session.gc_maxlifetime')),
+			'session_keepalive'	=> $config->getSystemValue('session_keepalive', true),
 			'version'			=> implode('.', \OCP\Util::getVersion()),
 			'versionstring'		=> OC_Util::getVersionString(),
 			'enable_avatars'	=> \OC::$server->getConfig()->getSystemValue('enable_avatars', true) === true,
@@ -187,9 +187,17 @@ $array = array(
 			'longFooter' => $defaults->getLongFooter(),
 			'folder' => OC_Util::getTheme(),
 		)
-	)
+	),
 );
 
+if (OC_User::getUser() !== null && OC_User::getUser() !== false) {
+	$array['oc_userconfig'] = json_encode([
+		'avatar' => [
+			'version' => (int)$config->getUserValue(OC_User::getUser(), 'avatar', 'version', 0),
+		]
+	]);
+}
+
 // Allow hooks to modify the output values
 OC_Hook::emit('\OCP\Config', 'js', array('array' => &$array));
 

core/js/jquery.avatar.js

@@ -74,10 +74,25 @@
 		user = String(user).replace(/\//g,'');
 
 		var $div = this;
+		var url;
 
-		var url = OC.generateUrl(
-			'/avatar/{user}/{size}',
-			{user: user, size: Math.ceil(size * window.devicePixelRatio)});
+		// If this is our own avatar we have to use the version attribute
+		if (user === OC.getCurrentUser().uid) {
+			url = OC.generateUrl(
+				'/avatar/{user}/{size}?v={version}',
+				{
+					user: user,
+					size: Math.ceil(size * window.devicePixelRatio),
+					version: oc_userconfig.avatar.version
+				});
+		} else {
+			url = OC.generateUrl(
+				'/avatar/{user}/{size}',
+				{
+					user: user,
+					size: Math.ceil(size * window.devicePixelRatio)
+				});
+		}
 
 		// If the displayname is not defined we use the old code path
 		if (typeof(displayname) === 'undefined') {
@@ -122,7 +137,7 @@
 				$div.show();
 				$div.text('');
 				$div.append(img);
-			}
+			};
 
 			img.width = size;
 			img.height = size;

core/templates/layout.user.php

@@ -66,8 +66,8 @@
 					<div class="avatardiv<?php if ($_['userAvatarSet']) { print_unescaped(' avatardiv-shown'); } else { print_unescaped('" style="display: none'); } ?>">
 						<?php if ($_['userAvatarSet']): ?>
 							<img alt="" width="32" height="32"
-							src="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 32]));?>"
-							srcset="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 64]));?> 2x, <?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 128]));?> 4x"
+							src="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 32, 'v' => $_['userAvatarVersion']]));?>"
+							srcset="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 64, 'v' => $_['userAvatarVersion']]));?> 2x, <?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 128, 'v' => $_['userAvatarVersion']]));?> 4x"
 							>
 						<?php endif; ?>
 					</div>

lib/private/Avatar.php

@@ -34,6 +34,7 @@ use OCP\Files\File;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
 use OCP\IAvatar;
+use OCP\IConfig;
 use OCP\IImage;
 use OCP\IL10N;
 use OC_Image;
@@ -52,6 +53,8 @@ class Avatar implements IAvatar {
 	private $user;
 	/** @var ILogger  */
 	private $logger;
+	/** @var IConfig */
+	private $config;
 
 	/**
 	 * constructor
@@ -60,12 +63,18 @@ class Avatar implements IAvatar {
 	 * @param IL10N $l
 	 * @param User $user
 	 * @param ILogger $logger
+	 * @param IConfig $config
 	 */
-	public function __construct (Folder $folder, IL10N $l, $user, ILogger $logger) {
+	public function __construct(Folder $folder,
+								IL10N $l,
+								$user,
+								ILogger $logger,
+								IConfig $config) {
 		$this->folder = $folder;
 		$this->l = $l;
 		$this->user = $user;
 		$this->logger = $logger;
+		$this->config = $config;
 	}
 
 	/**
@@ -137,6 +146,9 @@ class Avatar implements IAvatar {
 		$regex = '/^avatar\.([0-9]+\.)?(jpg|png)$/';
 		$avatars = $this->folder->getDirectoryListing();
 
+		$this->config->setUserValue($this->user->getUID(), 'avatar', 'version',
+			(int)$this->config->getUserValue($this->user->getUID(), 'avatar', 'version', 0) + 1);
+
 		foreach ($avatars as $avatar) {
 			if (preg_match($regex, $avatar->getName())) {
 				$avatar->delete();

lib/private/AvatarManager.php

@@ -30,6 +30,7 @@ namespace OC;
 use OCP\Files\Folder;
 use OCP\Files\NotFoundException;
 use OCP\IAvatarManager;
+use OCP\IConfig;
 use OCP\ILogger;
 use OCP\IUserManager;
 use OCP\Files\IRootFolder;
@@ -52,6 +53,9 @@ class AvatarManager implements IAvatarManager {
 	/** @var ILogger  */
 	private $logger;
 
+	/** @var IConfig */
+	private $config;
+
 	/**
 	 * AvatarManager constructor.
 	 *
@@ -59,16 +63,19 @@ class AvatarManager implements IAvatarManager {
 	 * @param IRootFolder $rootFolder
 	 * @param IL10N $l
 	 * @param ILogger $logger
+	 * @param IConfig $config
 	 */
 	public function __construct(
 			IUserManager $userManager,
 			IRootFolder $rootFolder,
 			IL10N $l,
-			ILogger $logger) {
+			ILogger $logger,
+			IConfig $config) {
 		$this->userManager = $userManager;
 		$this->rootFolder = $rootFolder;
 		$this->l = $l;
 		$this->logger = $logger;
+		$this->config = $config;
 	}
 
 	/**
@@ -94,6 +101,6 @@ class AvatarManager implements IAvatarManager {
 		/** @var Folder $folder */
 		$folder = $this->rootFolder->get($dir);
 
-		return new Avatar($folder, $this->l, $user, $this->logger);
+		return new Avatar($folder, $this->l, $user, $this->logger, $this->config);
 	}
 }

lib/private/Server.php

@@ -361,7 +361,8 @@ class Server extends ServerContainer implements IServerContainer {
 				$c->getUserManager(),
 				$c->getRootFolder(),
 				$c->getL10N('lib'),
-				$c->getLogger()
+				$c->getLogger(),
+				$c->getConfig()
 			);
 		});
 		$this->registerService('Logger', function (Server $c) {

lib/private/TemplateLayout.php

@@ -112,6 +112,7 @@ class TemplateLayout extends \OC_Template {
 				$this->assign('userAvatarSet', false);
 			} else {
 				$this->assign('userAvatarSet', \OC::$server->getAvatarManager()->getAvatar(\OC_User::getUser())->exists());
+				$this->assign('userAvatarVersion', \OC::$server->getConfig()->getUserValue(\OC_User::getUser(), 'avatar', 'version', 0));
 			}
 
 		} else if ($renderAs == 'error') {

lib/public/AppFramework/Http/Response.php

@@ -92,8 +92,7 @@ class Response {
 	public function cacheFor($cacheSeconds) {
 
 		if($cacheSeconds > 0) {
-			$this->addHeader('Cache-Control', 'max-age=' . $cacheSeconds .
-				', must-revalidate');
+			$this->addHeader('Cache-Control', 'max-age=' . $cacheSeconds . ', must-revalidate');
 		} else {
 			$this->addHeader('Cache-Control', 'no-cache, no-store, must-revalidate');
 		}

settings/js/personal.js

@@ -100,6 +100,9 @@ function updateAvatar (hidedefault) {
 	var $headerdiv = $('#header .avatardiv');
 	var $displaydiv = $('#displayavatar .avatardiv');
 
+	//Bump avatar avatarversion
+	oc_userconfig.avatar.version = -(Math.floor(Math.random() * 1000));
+
 	if (hidedefault) {
 		$headerdiv.hide();
 		$('#header .avatardiv').removeClass('avatardiv-shown');
@@ -112,7 +115,10 @@ function updateAvatar (hidedefault) {
 	$displaydiv.avatar(OC.currentUser, 145, true);
 	$.get(OC.generateUrl(
 		'/avatar/{user}/{size}',
-		{user: OC.currentUser, size: 1}
+		{
+			user: OC.currentUser,
+			size: 1
+		}
 	), function (result) {
 		if (typeof(result) === 'string') {
 			// Show the delete button when the avatar is custom
@@ -358,7 +364,10 @@ $(document).ready(function () {
 	// does the user have a custom avatar? if he does show #removeavatar
 	$.get(OC.generateUrl(
 		'/avatar/{user}/{size}',
-		{user: OC.currentUser, size: 1}
+		{
+			user: OC.currentUser,
+			size: 1
+		}
 	), function (result) {
 		if (typeof(result) === 'string') {
 			// Show the delete button when the avatar is custom

tests/Core/Controller/AvatarControllerTest.php

@@ -31,6 +31,7 @@ function is_uploaded_file($filename) {
 
 namespace Tests\Core\Controller;
 
+use OC\AppFramework\Utility\TimeFactory;
 use OC\Core\Controller\AvatarController;
 use OCP\AppFramework\Http;
 use OCP\Files\Cache\ICache;
@@ -74,6 +75,8 @@ class AvatarControllerTest extends \Test\TestCase {
 	private $logger;
 	/** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */
 	private $request;
+	/** @var TimeFactory|\PHPUnit_Framework_MockObject_MockObject */
+	private $timeFactory;
 	
 	protected function setUp() {
 		parent::setUp();
@@ -87,6 +90,7 @@ class AvatarControllerTest extends \Test\TestCase {
 		$this->request = $this->getMockBuilder('OCP\IRequest')->getMock();
 		$this->rootFolder = $this->getMockBuilder('OCP\Files\IRootFolder')->getMock();
 		$this->logger = $this->getMockBuilder('OCP\ILogger')->getMock();
+		$this->timeFactory = $this->getMockBuilder('OC\AppFramework\Utility\TimeFactory')->getMock();
 
 		$this->avatarMock = $this->getMockBuilder('OCP\IAvatar')->getMock();
 		$this->userMock = $this->getMockBuilder('OCP\IUser')->getMock();
@@ -100,7 +104,8 @@ class AvatarControllerTest extends \Test\TestCase {
 			$this->userManager,
 			$this->rootFolder,
 			$this->logger,
-			'userid'
+			'userid',
+			$this->timeFactory
 		);
 
 		// Configure userMock

tests/lib/AvatarTest.php

@@ -20,15 +20,26 @@ class AvatarTest extends \Test\TestCase {
 	/** @var \OC\User\User | \PHPUnit_Framework_MockObject_MockObject $user */
 	private $user;
 
+	/** @var \OCP\IConfig|\PHPUnit_Framework_MockObject_MockObject */
+	private $config;
+
 	public function setUp() {
 		parent::setUp();
 
-		$this->folder = $this->getMock('\OCP\Files\Folder');
+		$this->folder = $this->getMockBuilder('OCP\Files\Folder')->getMock();
 		/** @var \OCP\IL10N | \PHPUnit_Framework_MockObject_MockObject $l */
-		$l = $this->getMock('\OCP\IL10N');
+		$l = $this->getMockBuilder('OCP\IL10N')->getMock();
 		$l->method('t')->will($this->returnArgument(0));
-		$this->user = $this->getMockBuilder('\OC\User\User')->disableOriginalConstructor()->getMock();
-		$this->avatar = new \OC\Avatar($this->folder, $l, $this->user, $this->getMock('\OCP\ILogger'));
+		$this->user = $this->getMockBuilder('OC\User\User')->disableOriginalConstructor()->getMock();
+		$this->config = $this->getMockBuilder('OCP\IConfig')->getMock();
+
+		$this->avatar = new \OC\Avatar(
+			$this->folder,
+			$l,
+			$this->user,
+			$this->getMockBuilder('\OCP\ILogger')->getMock(),
+			$this->config
+		);
 	}
 
 	public function testGetNoAvatar() {
@@ -44,7 +55,7 @@ class AvatarTest extends \Test\TestCase {
 
 		$expected = new \OC_Image(\OC::$SERVERROOT . '/tests/data/testavatar.png');
 
-		$file = $this->getMock('\OCP\Files\File');
+		$file = $this->getMockBuilder('OCP\Files\File')->getMock();
 		$file->method('getContent')->willReturn($expected->data());
 		$this->folder->method('get')->with('avatar.128.jpg')->willReturn($file);
 
@@ -59,7 +70,7 @@ class AvatarTest extends \Test\TestCase {
 
 		$expected = new \OC_Image(\OC::$SERVERROOT . '/tests/data/testavatar.png');
 
-		$file = $this->getMock('\OCP\Files\File');
+		$file = $this->getMockBuilder('OCP\Files\File')->getMock();
 		$file->method('getContent')->willReturn($expected->data());
 		$this->folder->method('get')->with('avatar.jpg')->willReturn($file);
 
@@ -77,7 +88,7 @@ class AvatarTest extends \Test\TestCase {
 		$expected2 = new \OC_Image(\OC::$SERVERROOT . '/tests/data/testavatar.png');
 		$expected2->resize(32);
 
-		$file = $this->getMock('\OCP\Files\File');
+		$file = $this->getMockBuilder('OCP\Files\File')->getMock();
 		$file->method('getContent')->willReturn($expected->data());
 
 		$this->folder->method('get')
@@ -91,7 +102,7 @@ class AvatarTest extends \Test\TestCase {
 				}
 			));
 
-		$newFile = $this->getMock('\OCP\Files\File');
+		$newFile = $this->getMockBuilder('OCP\Files\File')->getMock();
 		$newFile->expects($this->once())
 			->method('putContent')
 			->with($expected2->data());
@@ -129,22 +140,22 @@ class AvatarTest extends \Test\TestCase {
 	}
 
 	public function testSetAvatar() {
-		$avatarFileJPG = $this->getMock('\OCP\Files\File');
+		$avatarFileJPG = $this->getMockBuilder('OCP\Files\File')->getMock();
 		$avatarFileJPG->method('getName')
 			->willReturn('avatar.jpg');
 		$avatarFileJPG->expects($this->once())->method('delete');
 
-		$avatarFilePNG = $this->getMock('\OCP\Files\File');
+		$avatarFilePNG = $this->getMockBuilder('OCP\Files\File')->getMock();
 		$avatarFilePNG->method('getName')
 			->willReturn('avatar.png');
 		$avatarFilePNG->expects($this->once())->method('delete');
 
-		$resizedAvatarFile = $this->getMock('\OCP\Files\File');
+		$resizedAvatarFile = $this->getMockBuilder('OCP\Files\File')->getMock();
 		$resizedAvatarFile->method('getName')
 			->willReturn('avatar.32.jpg');
 		$resizedAvatarFile->expects($this->once())->method('delete');
 
-		$nonAvatarFile = $this->getMock('\OCP\Files\File');
+		$nonAvatarFile = $this->getMockBuilder('OCP\Files\File')->getMock();
 		$nonAvatarFile->method('getName')
 			->willReturn('avatarX');
 		$nonAvatarFile->expects($this->never())->method('delete');
@@ -152,7 +163,7 @@ class AvatarTest extends \Test\TestCase {
 		$this->folder->method('getDirectoryListing')
 			->willReturn([$avatarFileJPG, $avatarFilePNG, $resizedAvatarFile, $nonAvatarFile]);
 
-		$newFile = $this->getMock('\OCP\Files\File');
+		$newFile = $this->getMockBuilder('OCP\Files\File')->getMock();
 		$this->folder->expects($this->once())
 			->method('newFile')
 			->with('avatar.png')
@@ -163,6 +174,11 @@ class AvatarTest extends \Test\TestCase {
 			->method('putContent')
 			->with($image->data());
 
+		$this->config->expects($this->once())
+			->method('setUserValue');
+		$this->config->expects($this->once())
+			->method('getUserValue');
+
 		// One on remove and once on setting the new avatar
 		$this->user->expects($this->exactly(2))->method('triggerChange');