Merge pull request #21240 from owncloud/avatar_speedup

Avatar speedup

5 files changed, 116 insertions, 45 deletions

diff --git a/core/avatar/avatarcontroller.php b/core/avatar/avatarcontroller.php
index 6c0321e6b5e..e8139aa50ae 100644
--- a/core/avatar/avatarcontroller.php
+++ b/core/avatar/avatarcontroller.php
@@ -30,6 +30,7 @@ use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\DataDisplayResponse;
+use OCP\Files\NotFoundException;
 use OCP\IAvatarManager;
 use OCP\ILogger;
 use OCP\IL10N;
@@ -112,20 +113,23 @@ class AvatarController extends Controller {
 			$size = 64;
 		}
 
-		$avatar = $this->avatarManager->getAvatar($userId);
-		$image = $avatar->get($size);
-
-		if ($image instanceof \OCP\IImage) {
-			$resp = new DataDisplayResponse($image->data(),
+		try {
+			$avatar = $this->avatarManager->getAvatar($userId)->getFile($size);
+			$resp = new DataDisplayResponse($avatar->getContent(),
 				Http::STATUS_OK,
-				['Content-Type' => $image->mimeType()]);
-			$resp->setETag(crc32($image->data()));
-		} else {
+				['Content-Type' => $avatar->getMimeType()]);
+			$resp->setETag($avatar->getEtag());
+		} catch (NotFoundException $e) {
 			$user = $this->userManager->get($userId);
-			$userName = $user ? $user->getDisplayName() : '';
 			$resp = new DataResponse([
 				'data' => [
-					'displayname' => $userName,
+					'displayname' => $user->getDisplayName(),
+				],
+			]);
+		} catch (\Exception $e) {
+			$resp = new DataResponse([
+				'data' => [
+					'displayname' => '',
 				],
 			]);
 		}
diff --git a/lib/private/avatar.php b/lib/private/avatar.php
index 37a813f3ff8..c87facd25da 100644
--- a/lib/private/avatar.php
+++ b/lib/private/avatar.php
@@ -31,6 +31,7 @@ namespace OC;
 
 use OCP\Files\Folder;
 use OCP\Files\File;
+use OCP\Files\NotFoundException;
 use OCP\IL10N;
 use OC_Image;
 
@@ -62,28 +63,14 @@ class Avatar implements \OCP\IAvatar {
 	 * @return boolean|\OCP\IImage containing the avatar or false if there's no image
 	*/
 	public function get ($size = 64) {
-		if ($this->folder->nodeExists('avatar.jpg')) {
-			$ext = 'jpg';
-		} elseif ($this->folder->nodeExists('avatar.png')) {
-			$ext = 'png';
-		} else {
+		try {
+			$file = $this->getFile($size);
+		} catch (NotFoundException $e) {
 			return false;
 		}
 
 		$avatar = new OC_Image();
-		if ($this->folder->nodeExists('avatar.' . $size . '.' . $ext)) {
-			/** @var File $node */
-			$node = $this->folder->get('avatar.' . $size . '.' . $ext);
-			$avatar->loadFromData($node->getContent());
-		} else {
-			/** @var File $node */
-			$node = $this->folder->get('avatar.' . $ext);
-			$avatar->loadFromData($node->getContent());
-			if ($size > 0) {
-				$avatar->resize($size);
-			}
-			$this->folder->newFile('avatar.' . $size . '.' . $ext)->putContent($avatar->data());
-		}
+		$avatar->loadFromData($file->getContent());
 		return $avatar;
 	}
 
@@ -144,4 +131,50 @@ class Avatar implements \OCP\IAvatar {
 			$this->folder->get('avatar.png')->delete();
 		} catch (\OCP\Files\NotFoundException $e) {}
 	}
+
+	/**
+	 * Get the File of an avatar of size $size.
+	 *
+	 * @param int $size
+	 * @return File
+	 * @throws NotFoundException
+	 */
+	public function getFile($size) {
+		$ext = $this->getExtention();
+
+		$path = 'avatar.' . $size . '.' . $ext;
+
+		try {
+			$file = $this->folder->get($path);
+		} catch (NotFoundException $e) {
+			if ($size <= 0) {
+				throw new NotFoundException;
+			}
+
+			$avatar = new OC_Image();
+			/** @var File $file */
+			$file = $this->folder->get('avatar.' . $ext);
+			$avatar->loadFromData($file->getContent());
+			$avatar->resize($size);
+			$file = $this->folder->newFile($path);
+			$file->putContent($avatar->data());
+		}
+
+		return $file;
+	}
+
+	/**
+	 * Get the extention of the avatar. If there is no avatar throw Exception
+	 *
+	 * @return string
+	 * @throws NotFoundException
+	 */
+	private function getExtention() {
+		if ($this->folder->nodeExists('avatar.jpg')) {
+			return 'jpg';
+		} elseif ($this->folder->nodeExists('avatar.png')) {
+			return 'png';
+		}
+		throw new NotFoundException;
+	}
 }
diff --git a/lib/public/iavatar.php b/lib/public/iavatar.php
index fc29212a599..3d92d00b83d 100644
--- a/lib/public/iavatar.php
+++ b/lib/public/iavatar.php
@@ -24,6 +24,8 @@
  */
 
 namespace OCP;
+use OCP\Files\File;
+use OCP\Files\NotFoundException;
 
 /**
  * This class provides avatar functionality
@@ -64,4 +66,13 @@ interface IAvatar {
 	 * @since 6.0.0
 	 */
 	public function remove();
+
+	/**
+	 * Get the file of the avatar
+	 * @param int $size
+	 * @return File
+	 * @throws NotFoundException
+	 * @since 9.0.0
+	 */
+	public function getFile($size);
 }
diff --git a/tests/core/avatar/avatarcontrollertest.php b/tests/core/avatar/avatarcontrollertest.php
index a113add72b9..7f69ba0aadb 100644
--- a/tests/core/avatar/avatarcontrollertest.php
+++ b/tests/core/avatar/avatarcontrollertest.php
@@ -26,8 +26,10 @@ use OCP\AppFramework\IAppContainer;
 use OCP\AppFramework\Http;
 use OCP\Files\Folder;
 use OCP\Files\File;
+use OCP\Files\NotFoundException;
 use OCP\IUser;
 use OCP\IAvatar;
+use Punic\Exception;
 use Test\Traits\UserTrait;
 
 /**
@@ -56,6 +58,8 @@ class AvatarControllerTest extends \Test\TestCase {
 	private $avatarMock;
 	/** @var IUser */
 	private $userMock;
+	/** @var File */
+	private $avatarFile;
 	
 	protected function setUp() {
 		parent::setUp();
@@ -88,6 +92,10 @@ class AvatarControllerTest extends \Test\TestCase {
 			->willReturnMap([['userId', $this->userMock]]);
 		$this->container['UserSession']->method('getUser')->willReturn($this->userMock);
 
+		$this->avatarFile = $this->getMock('OCP\Files\File');
+		$this->avatarFile->method('getContnet')->willReturn('image data');
+		$this->avatarFile->method('getMimeType')->willReturn('image type');
+		$this->avatarFile->method('getEtag')->willReturn('my etag');
 	}
 
 	public function tearDown() {
@@ -100,6 +108,7 @@ class AvatarControllerTest extends \Test\TestCase {
 	 */
 	public function testGetAvatarNoAvatar() {
 		$this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock);
+		$this->avatarMock->method('getFile')->will($this->throwException(new NotFoundException()));
 		$response = $this->avatarController->getAvatar('userId', 32);
 
 		//Comment out until JS is fixed
@@ -112,12 +121,8 @@ class AvatarControllerTest extends \Test\TestCase {
 	 * Fetch the user's avatar
 	 */
 	public function testGetAvatar() {
-		$image = $this->getMock('OCP\IImage');
-		$image->method('data')->willReturn('image data');
-		$image->method('mimeType')->willReturn('image type');
-
-		$this->avatarMock->method('get')->willReturn($image);
-		$this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock);
+		$this->avatarMock->method('getFile')->willReturn($this->avatarFile);
+		$this->container['AvatarManager']->method('getAvatar')->with('userId')->willReturn($this->avatarMock);
 
 		$response = $this->avatarController->getAvatar('userId', 32);
 
@@ -125,17 +130,19 @@ class AvatarControllerTest extends \Test\TestCase {
 		$this->assertArrayHasKey('Content-Type', $response->getHeaders());
 		$this->assertEquals('image type', $response->getHeaders()['Content-Type']);
 
-		$this->assertEquals(crc32('image data'), $response->getEtag());
+		$this->assertEquals('my etag', $response->getEtag());
 	}
 
 	/**
 	 * Fetch the avatar of a non-existing user
 	 */
 	public function testGetAvatarNoUser() {
-		$this->avatarMock->method('get')->willReturn(null);
-		$this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock);
+		$this->container['AvatarManager']
+			->method('getAvatar')
+			->with('userDoesNotExist')
+			->will($this->throwException(new \Exception('user does not exist')));
 
-		$response = $this->avatarController->getAvatar('userDoesnotexist', 32);
+		$response = $this->avatarController->getAvatar('userDoesNotExist', 32);
 
 		//Comment out until JS is fixed
 		//$this->assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus());
@@ -148,8 +155,9 @@ class AvatarControllerTest extends \Test\TestCase {
 	 */
 	public function testGetAvatarSize() {
 		$this->avatarMock->expects($this->once())
-						 ->method('get')
-						 ->with($this->equalTo(32));
+			->method('getFile')
+			->with($this->equalTo(32))
+			->willReturn($this->avatarFile);
 
 		$this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock);
 
@@ -161,8 +169,9 @@ class AvatarControllerTest extends \Test\TestCase {
 	 */
 	public function testGetAvatarSizeMin() {
 		$this->avatarMock->expects($this->once())
-						 ->method('get')
-						 ->with($this->equalTo(64));
+			->method('getFile')
+			->with($this->equalTo(64))
+			->willReturn($this->avatarFile);
 
 		$this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock);
 
@@ -174,8 +183,9 @@ class AvatarControllerTest extends \Test\TestCase {
 	 */
 	public function testGetAvatarSizeMax() {
 		$this->avatarMock->expects($this->once())
-						 ->method('get')
-						 ->with($this->equalTo(2048));
+			->method('getFile')
+			->with($this->equalTo(2048))
+			->willReturn($this->avatarFile);
 
 		$this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock);
 
diff --git a/tests/lib/avatartest.php b/tests/lib/avatartest.php
index 49e8be98c83..3d77a282a7d 100644
--- a/tests/lib/avatartest.php
+++ b/tests/lib/avatartest.php
@@ -60,12 +60,25 @@ class AvatarTest extends \Test\TestCase {
 
 		$file = $this->getMock('\OCP\Files\File');
 		$file->method('getContent')->willReturn($expected->data());
-		$this->folder->method('get')->with('avatar.png')->willReturn($file);
+
+		$this->folder->method('get')
+			->will($this->returnCallback(
+				function($path) use ($file) {
+					if ($path === 'avatar.png') {
+						return $file;
+					} else {
+						throw new \OCP\Files\NotFoundException;
+					}
+				}
+			));
 
 		$newFile = $this->getMock('\OCP\Files\File');
 		$newFile->expects($this->once())
 			->method('putContent')
 			->with($expected2->data());
+		$newFile->expects($this->once())
+			->method('getContent')
+			->willReturn($expected2->data());
 		$this->folder->expects($this->once())
 			->method('newFile')
 			->with('avatar.32.png')