From 623d34972dca25003d3f4107ec2715ff42bca086 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 13 Nov 2015 10:32:45 +0100 Subject: [PATCH 1/5] Oracle can not return statements but only values So evaluate the condition directly and return 1 or 0 --- apps/files/lib/activity.php | 9 ++++++--- apps/files/tests/activitytest.php | 8 ++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/apps/files/lib/activity.php b/apps/files/lib/activity.php index f3bbff48640..d473120b31f 100644 --- a/apps/files/lib/activity.php +++ b/apps/files/lib/activity.php @@ -379,6 +379,7 @@ class Activity implements IExtension { */ $parameters = $fileQueryList = []; $parameters[] = self::APP_FILES; + $parameters[] = self::APP_FILES; $fileQueryList[] = '(`type` <> ? AND `type` <> ?)'; $parameters[] = self::TYPE_SHARE_CREATED; @@ -393,10 +394,12 @@ class Activity implements IExtension { $parameters[] = $favorite . '/%'; } - $parameters[] = self::APP_FILES; - return [ - ' CASE WHEN `app` = ? THEN (' . implode(' OR ', $fileQueryList) . ') ELSE `app` <> ? END ', + ' CASE ' + . 'WHEN `app` <> ? THEN 1 ' + . 'WHEN `app` = ? AND (' . implode(' OR ', $fileQueryList) . ') THEN 1 ' + . 'ELSE 0 ' + . 'END = 1 ', $parameters, ]; } diff --git a/apps/files/tests/activitytest.php b/apps/files/tests/activitytest.php index cdb1d21bcd8..485c559d488 100644 --- a/apps/files/tests/activitytest.php +++ b/apps/files/tests/activitytest.php @@ -290,16 +290,16 @@ class ActivityTest extends TestCase { 'items' => [], 'folders' => [], ], - ' CASE WHEN `app` = ? THEN ((`type` <> ? AND `type` <> ?)) ELSE `app` <> ? END ', - ['files', Activity::TYPE_SHARE_CREATED, Activity::TYPE_SHARE_CHANGED, 'files'] + ' CASE WHEN `app` <> ? THEN 1 WHEN `app` = ? AND ((`type` <> ? AND `type` <> ?)) THEN 1 ELSE 0 END = 1 ', + ['files', 'files', Activity::TYPE_SHARE_CREATED, Activity::TYPE_SHARE_CHANGED] ], [ [ 'items' => ['file.txt', 'folder'], 'folders' => ['folder'], ], - ' CASE WHEN `app` = ? THEN ((`type` <> ? AND `type` <> ?) OR `file` = ? OR `file` = ? OR `file` LIKE ?) ELSE `app` <> ? END ', - ['files', Activity::TYPE_SHARE_CREATED, Activity::TYPE_SHARE_CHANGED, 'file.txt', 'folder', 'folder/%', 'files'] + ' CASE WHEN `app` <> ? THEN 1 WHEN `app` = ? AND ((`type` <> ? AND `type` <> ?) OR `file` = ? OR `file` = ? OR `file` LIKE ?) THEN 1 ELSE 0 END = 1 ', + ['files', 'files', Activity::TYPE_SHARE_CREATED, Activity::TYPE_SHARE_CHANGED, 'file.txt', 'folder', 'folder/%'] ], ]; } From 2cdec74e8a1dd68f757ab89d64aeafac9e56d0ed Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 13 Nov 2015 10:33:33 +0100 Subject: [PATCH 2/5] Correctly escape the paths so we only display favorites instead of wildcards --- apps/files/lib/activity.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files/lib/activity.php b/apps/files/lib/activity.php index d473120b31f..c171b3bfabf 100644 --- a/apps/files/lib/activity.php +++ b/apps/files/lib/activity.php @@ -391,7 +391,7 @@ class Activity implements IExtension { } foreach ($favorites['folders'] as $favorite) { $fileQueryList[] = '`file` LIKE ?'; - $parameters[] = $favorite . '/%'; + $parameters[] = \OC::$server->getDatabaseConnection()->escapeLikeParameter($favorite) . '/%'; } return [ From 6e0596432c174b73d11b5bfba04f53108d788d9a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 18 Nov 2015 10:32:29 +0100 Subject: [PATCH 3/5] Add a unit test that executes the query --- apps/files/tests/activitytest.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/apps/files/tests/activitytest.php b/apps/files/tests/activitytest.php index 485c559d488..f6a21a7db62 100644 --- a/apps/files/tests/activitytest.php +++ b/apps/files/tests/activitytest.php @@ -333,6 +333,21 @@ class ActivityTest extends TestCase { $result = $this->activityExtension->getQueryForFilter('all'); $this->assertEquals([$query, $parameters], $result); + + $this->executeQueryForFilter($result); + } + + public function executeQueryForFilter(array $result) { + list($resultQuery, $resultParameters) = $result; + $resultQuery = str_replace('`file`', '`user`', $resultQuery); + $resultQuery = str_replace('`type`', '`key`', $resultQuery); + + $connection = \OC::$server->getDatabaseConnection(); + // Test the query on the privatedata table, because the activity table + // does not exist in core + $result = $connection->executeQuery('SELECT * FROM `*PREFIX*privatedata` WHERE ' . $resultQuery, $resultParameters); + $rows = $result->fetchAll(); + $result->closeCursor(); } protected function mockUserSession($user) { From e9094b8a41e81fbd4f86aae5a4cadcab0674d93c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 18 Nov 2015 10:34:52 +0100 Subject: [PATCH 4/5] Only require the interface --- apps/files/lib/activity.php | 8 ++++---- apps/files/tests/activitytest.php | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/apps/files/lib/activity.php b/apps/files/lib/activity.php index c171b3bfabf..132f4169dee 100644 --- a/apps/files/lib/activity.php +++ b/apps/files/lib/activity.php @@ -22,7 +22,7 @@ namespace OCA\Files; -use OC\L10N\Factory; +use OCP\L10N\IFactory; use OCP\Activity\IExtension; use OCP\Activity\IManager; use OCP\IConfig; @@ -43,7 +43,7 @@ class Activity implements IExtension { /** @var IL10N */ protected $l; - /** @var Factory */ + /** @var IFactory */ protected $languageFactory; /** @var IURLGenerator */ @@ -59,13 +59,13 @@ class Activity implements IExtension { protected $helper; /** - * @param Factory $languageFactory + * @param IFactory $languageFactory * @param IURLGenerator $URLGenerator * @param IManager $activityManager * @param ActivityHelper $helper * @param IConfig $config */ - public function __construct(Factory $languageFactory, IURLGenerator $URLGenerator, IManager $activityManager, ActivityHelper $helper, IConfig $config) { + public function __construct(IFactory $languageFactory, IURLGenerator $URLGenerator, IManager $activityManager, ActivityHelper $helper, IConfig $config) { $this->languageFactory = $languageFactory; $this->URLGenerator = $URLGenerator; $this->l = $this->getL10N(); diff --git a/apps/files/tests/activitytest.php b/apps/files/tests/activitytest.php index f6a21a7db62..6a3424e727a 100644 --- a/apps/files/tests/activitytest.php +++ b/apps/files/tests/activitytest.php @@ -30,19 +30,19 @@ class ActivityTest extends TestCase { /** @var \OC\ActivityManager */ private $activityManager; - /** @var \PHPUnit_Framework_MockObject_MockObject */ + /** @var \OCP\IRequest|\PHPUnit_Framework_MockObject_MockObject */ protected $request; - /** @var \PHPUnit_Framework_MockObject_MockObject */ + /** @var \OCP\IUserSession|\PHPUnit_Framework_MockObject_MockObject */ protected $session; - /** @var \PHPUnit_Framework_MockObject_MockObject */ + /** @var \OCP\IConfig|\PHPUnit_Framework_MockObject_MockObject */ protected $config; - /** @var \PHPUnit_Framework_MockObject_MockObject */ + /** @var \OCA\Files\ActivityHelper|\PHPUnit_Framework_MockObject_MockObject */ protected $activityHelper; - /** @var \PHPUnit_Framework_MockObject_MockObject */ + /** @var \OCP\L10N\IFactory|\PHPUnit_Framework_MockObject_MockObject */ protected $l10nFactory; /** @var \OCA\Files\Activity */ @@ -70,7 +70,7 @@ class ActivityTest extends TestCase { $this->config ); - $this->l10nFactory = $this->getMockBuilder('OC\L10N\Factory') + $this->l10nFactory = $this->getMockBuilder('OCP\L10N\IFactory') ->disableOriginalConstructor() ->getMock(); $deL10n = $this->getMockBuilder('OC_L10N') From 23046ca5b7484b69ffe89b947d84f0b5cf2222ca Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 30 Nov 2015 09:42:11 +0100 Subject: [PATCH 5/5] Inject the database connection --- apps/files/appinfo/app.php | 1 + apps/files/lib/activity.php | 10 ++++++++-- apps/files/tests/activitytest.php | 7 +++++++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/apps/files/appinfo/app.php b/apps/files/appinfo/app.php index c752b5e7d72..61ff6d748f9 100644 --- a/apps/files/appinfo/app.php +++ b/apps/files/appinfo/app.php @@ -65,6 +65,7 @@ $templateManager->registerTemplate('application/vnd.oasis.opendocument.spreadshe new \OCA\Files\ActivityHelper( \OC::$server->getTagManager() ), + \OC::$server->getDatabaseConnection(), \OC::$server->getConfig() ); }); diff --git a/apps/files/lib/activity.php b/apps/files/lib/activity.php index 132f4169dee..23e3f26e62d 100644 --- a/apps/files/lib/activity.php +++ b/apps/files/lib/activity.php @@ -22,6 +22,7 @@ namespace OCA\Files; +use OCP\IDBConnection; use OCP\L10N\IFactory; use OCP\Activity\IExtension; use OCP\Activity\IManager; @@ -52,6 +53,9 @@ class Activity implements IExtension { /** @var \OCP\Activity\IManager */ protected $activityManager; + /** @var \OCP\IDBConnection */ + protected $connection; + /** @var \OCP\IConfig */ protected $config; @@ -63,14 +67,16 @@ class Activity implements IExtension { * @param IURLGenerator $URLGenerator * @param IManager $activityManager * @param ActivityHelper $helper + * @param IDBConnection $connection * @param IConfig $config */ - public function __construct(IFactory $languageFactory, IURLGenerator $URLGenerator, IManager $activityManager, ActivityHelper $helper, IConfig $config) { + public function __construct(IFactory $languageFactory, IURLGenerator $URLGenerator, IManager $activityManager, ActivityHelper $helper, IDBConnection $connection, IConfig $config) { $this->languageFactory = $languageFactory; $this->URLGenerator = $URLGenerator; $this->l = $this->getL10N(); $this->activityManager = $activityManager; $this->helper = $helper; + $this->connection = $connection; $this->config = $config; } @@ -391,7 +397,7 @@ class Activity implements IExtension { } foreach ($favorites['folders'] as $favorite) { $fileQueryList[] = '`file` LIKE ?'; - $parameters[] = \OC::$server->getDatabaseConnection()->escapeLikeParameter($favorite) . '/%'; + $parameters[] = $this->connection->escapeLikeParameter($favorite) . '/%'; } return [ diff --git a/apps/files/tests/activitytest.php b/apps/files/tests/activitytest.php index 6a3424e727a..59c020c9042 100644 --- a/apps/files/tests/activitytest.php +++ b/apps/files/tests/activitytest.php @@ -25,6 +25,12 @@ namespace OCA\Files\Tests; use OCA\Files\Activity; use Test\TestCase; +/** + * Class ActivityTest + * + * @group DB + * @package OCA\Files\Tests + */ class ActivityTest extends TestCase { /** @var \OC\ActivityManager */ @@ -95,6 +101,7 @@ class ActivityTest extends TestCase { $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), $this->activityManager, $this->activityHelper, + \OC::$server->getDatabaseConnection(), $this->config );