From ce398cf7bdfa1c4edeeee9b5259a415ef23031ac Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 31 Jan 2020 15:06:26 +0100 Subject: [PATCH 1/8] merge the two almost identical custom property backends Signed-off-by: Robin Appelman --- .../Sabre/CustomPropertiesBackend.php | 351 ------------------ .../dav/lib/Connector/Sabre/ServerFactory.php | 2 +- apps/dav/lib/DAV/CustomPropertiesBackend.php | 118 +++++- .../Sabre/CustomPropertiesBackendTest.php | 4 +- .../unit/DAV/CustomPropertiesBackendTest.php | 36 +- 5 files changed, 135 insertions(+), 376 deletions(-) delete mode 100644 apps/dav/lib/Connector/Sabre/CustomPropertiesBackend.php diff --git a/apps/dav/lib/Connector/Sabre/CustomPropertiesBackend.php b/apps/dav/lib/Connector/Sabre/CustomPropertiesBackend.php deleted file mode 100644 index daa71dac07..0000000000 --- a/apps/dav/lib/Connector/Sabre/CustomPropertiesBackend.php +++ /dev/null @@ -1,351 +0,0 @@ - - * @author Lukas Reschke - * @author Roeland Jago Douma - * @author Thomas Müller - * @author Vincent Petry - * - * @license AGPL-3.0 - * - * This code is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License, version 3, - * as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License, version 3, - * along with this program. If not, see - * - */ - -namespace OCA\DAV\Connector\Sabre; - -use OCP\IDBConnection; -use OCP\IUser; -use Sabre\DAV\Exception\NotFound; -use Sabre\DAV\Exception\ServiceUnavailable; -use Sabre\DAV\PropertyStorage\Backend\BackendInterface; -use Sabre\DAV\PropFind; -use Sabre\DAV\PropPatch; -use Sabre\DAV\Tree; - -class CustomPropertiesBackend implements BackendInterface { - - /** - * Ignored properties - * - * @var array - */ - private $ignoredProperties = array( - '{DAV:}getcontentlength', - '{DAV:}getcontenttype', - '{DAV:}getetag', - '{DAV:}quota-used-bytes', - '{DAV:}quota-available-bytes', - '{http://owncloud.org/ns}permissions', - '{http://owncloud.org/ns}downloadURL', - '{http://owncloud.org/ns}dDC', - '{http://owncloud.org/ns}size', - '{http://nextcloud.org/ns}is-encrypted', - ); - - /** - * @var Tree - */ - private $tree; - - /** - * @var IDBConnection - */ - private $connection; - - /** - * @var IUser - */ - private $user; - - /** - * Properties cache - * - * @var array - */ - private $cache = []; - - /** - * @param Tree $tree node tree - * @param IDBConnection $connection database connection - * @param IUser $user owner of the tree and properties - */ - public function __construct( - Tree $tree, - IDBConnection $connection, - IUser $user) { - $this->tree = $tree; - $this->connection = $connection; - $this->user = $user->getUID(); - } - - /** - * Fetches properties for a path. - * - * @param string $path - * @param PropFind $propFind - * @return void - */ - public function propFind($path, PropFind $propFind) { - try { - $node = $this->tree->getNodeForPath($path); - if (!($node instanceof Node)) { - return; - } - } catch (ServiceUnavailable $e) { - // might happen for unavailable mount points, skip - return; - } catch (NotFound $e) { - // in some rare (buggy) cases the node might not be found, - // we catch the exception to prevent breaking the whole list with a 404 - // (soft fail) - \OC::$server->getLogger()->warning( - 'Could not get node for path: \"' . $path . '\" : ' . $e->getMessage(), - array('app' => 'files') - ); - return; - } - - $requestedProps = $propFind->get404Properties(); - - // these might appear - $requestedProps = array_diff( - $requestedProps, - $this->ignoredProperties - ); - - if (empty($requestedProps)) { - return; - } - - $props = $this->getProperties($node, $requestedProps); - foreach ($props as $propName => $propValue) { - $propFind->set($propName, $propValue); - } - } - - /** - * Updates properties for a path - * - * @param string $path - * @param PropPatch $propPatch - * - * @return void - */ - public function propPatch($path, PropPatch $propPatch) { - $node = $this->tree->getNodeForPath($path); - if (!($node instanceof Node)) { - return; - } - - $propPatch->handleRemaining(function($changedProps) use ($node) { - return $this->updateProperties($node, $changedProps); - }); - } - - /** - * This method is called after a node is deleted. - * - * @param string $path path of node for which to delete properties - */ - public function delete($path) { - $statement = $this->connection->prepare( - 'DELETE FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ?' - ); - $statement->execute(array($this->user, '/' . $path)); - $statement->closeCursor(); - - unset($this->cache[$path]); - } - - /** - * This method is called after a successful MOVE - * - * @param string $source - * @param string $destination - * - * @return void - */ - public function move($source, $destination) { - $statement = $this->connection->prepare( - 'UPDATE `*PREFIX*properties` SET `propertypath` = ?' . - ' WHERE `userid` = ? AND `propertypath` = ?' - ); - $statement->execute(array('/' . $destination, $this->user, '/' . $source)); - $statement->closeCursor(); - } - - /** - * Returns a list of properties for this nodes.; - * @param Node $node - * @param array $requestedProperties requested properties or empty array for "all" - * @return array - * @note The properties list is a list of propertynames the client - * requested, encoded as xmlnamespace#tagName, for example: - * http://www.example.org/namespace#author If the array is empty, all - * properties should be returned - */ - private function getProperties(Node $node, array $requestedProperties) { - $path = $node->getPath(); - if (isset($this->cache[$path])) { - return $this->cache[$path]; - } - - // TODO: chunking if more than 1000 properties - $sql = 'SELECT * FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ?'; - - $whereValues = array($this->user, $path); - $whereTypes = array(null, null); - - if (!empty($requestedProperties)) { - // request only a subset - $sql .= ' AND `propertyname` in (?)'; - $whereValues[] = $requestedProperties; - $whereTypes[] = \Doctrine\DBAL\Connection::PARAM_STR_ARRAY; - } - - $result = $this->connection->executeQuery( - $sql, - $whereValues, - $whereTypes - ); - - $props = []; - while ($row = $result->fetch()) { - $props[$row['propertyname']] = $row['propertyvalue']; - } - - $result->closeCursor(); - - $this->cache[$path] = $props; - return $props; - } - - /** - * Update properties - * - * @param Node $node node for which to update properties - * @param array $properties array of properties to update - * - * @return bool - */ - private function updateProperties($node, $properties) { - $path = $node->getPath(); - - $deleteStatement = 'DELETE FROM `*PREFIX*properties`' . - ' WHERE `userid` = ? AND `propertypath` = ? AND `propertyname` = ?'; - - $insertStatement = 'INSERT INTO `*PREFIX*properties`' . - ' (`userid`,`propertypath`,`propertyname`,`propertyvalue`) VALUES(?,?,?,?)'; - - $updateStatement = 'UPDATE `*PREFIX*properties` SET `propertyvalue` = ?' . - ' WHERE `userid` = ? AND `propertypath` = ? AND `propertyname` = ?'; - - // TODO: use "insert or update" strategy ? - $existing = $this->getProperties($node, array()); - $this->connection->beginTransaction(); - foreach ($properties as $propertyName => $propertyValue) { - // If it was null, we need to delete the property - if (is_null($propertyValue)) { - if (array_key_exists($propertyName, $existing)) { - $this->connection->executeUpdate($deleteStatement, - array( - $this->user, - $path, - $propertyName - ) - ); - } - } else { - if (!array_key_exists($propertyName, $existing)) { - $this->connection->executeUpdate($insertStatement, - array( - $this->user, - $path, - $propertyName, - $propertyValue - ) - ); - } else { - $this->connection->executeUpdate($updateStatement, - array( - $propertyValue, - $this->user, - $path, - $propertyName - ) - ); - } - } - } - - $this->connection->commit(); - unset($this->cache[$path]); - - return true; - } - - /** - * Bulk load properties for directory children - * - * @param Directory $node - * @param array $requestedProperties requested properties - * - * @return void - */ - private function loadChildrenProperties(Directory $node, $requestedProperties) { - $path = $node->getPath(); - if (isset($this->cache[$path])) { - // we already loaded them at some point - return; - } - - $childNodes = $node->getChildren(); - // pre-fill cache - foreach ($childNodes as $childNode) { - $this->cache[$childNode->getPath()] = []; - } - - $sql = 'SELECT * FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` LIKE ?'; - $sql .= ' AND `propertyname` in (?) ORDER BY `propertypath`, `propertyname`'; - - $result = $this->connection->executeQuery( - $sql, - array($this->user, $this->connection->escapeLikeParameter(rtrim($path, '/')) . '/%', $requestedProperties), - array(null, null, \Doctrine\DBAL\Connection::PARAM_STR_ARRAY) - ); - - $oldPath = null; - $props = []; - while ($row = $result->fetch()) { - $path = $row['propertypath']; - if ($oldPath !== $path) { - // save previously gathered props - $this->cache[$oldPath] = $props; - $oldPath = $path; - // prepare props for next path - $props = []; - } - $props[$row['propertyname']] = $row['propertyvalue']; - } - if (!is_null($oldPath)) { - // save props from last run - $this->cache[$oldPath] = $props; - } - - $result->closeCursor(); - } - -} diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index 595e3b734b..9efd9ba7b5 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -195,7 +195,7 @@ class ServerFactory { // custom properties plugin must be the last one $server->addPlugin( new \Sabre\DAV\PropertyStorage\Plugin( - new \OCA\DAV\Connector\Sabre\CustomPropertiesBackend( + new \OCA\DAV\DAV\CustomPropertiesBackend( $objectTree, $this->databaseConnection, $this->userSession->getUser() diff --git a/apps/dav/lib/DAV/CustomPropertiesBackend.php b/apps/dav/lib/DAV/CustomPropertiesBackend.php index 35b3daa8a4..9f2fc571c6 100644 --- a/apps/dav/lib/DAV/CustomPropertiesBackend.php +++ b/apps/dav/lib/DAV/CustomPropertiesBackend.php @@ -3,9 +3,11 @@ * @copyright Copyright (c) 2016, ownCloud, Inc. * @copyright Copyright (c) 2017, Georg Ehrke * - * @author Georg Ehrke - * @author Robin Appelman + * @author Aaron Wood + * @author Lukas Reschke + * @author Roeland Jago Douma * @author Thomas Müller + * @author Vincent Petry * * @license AGPL-3.0 * @@ -25,8 +27,12 @@ namespace OCA\DAV\DAV; +use OCA\DAV\Connector\Sabre\Directory; +use OCA\DAV\Connector\Sabre\Node; use OCP\IDBConnection; use OCP\IUser; +use Sabre\DAV\Exception\NotFound; +use Sabre\DAV\Exception\ServiceUnavailable; use Sabre\DAV\PropertyStorage\Backend\BackendInterface; use Sabre\DAV\PropFind; use Sabre\DAV\PropPatch; @@ -63,7 +69,7 @@ class CustomPropertiesBackend implements BackendInterface { private $connection; /** - * @var string + * @var IUser */ private $user; @@ -85,7 +91,7 @@ class CustomPropertiesBackend implements BackendInterface { IUser $user) { $this->tree = $tree; $this->connection = $connection; - $this->user = $user->getUID(); + $this->user = $user; } /** @@ -96,6 +102,24 @@ class CustomPropertiesBackend implements BackendInterface { * @return void */ public function propFind($path, PropFind $propFind) { + try { + $node = $this->tree->getNodeForPath($path); + if (!($node instanceof Node)) { + return; + } + } catch (ServiceUnavailable $e) { + // might happen for unavailable mount points, skip + return; + } catch (NotFound $e) { + // in some rare (buggy) cases the node might not be found, + // we catch the exception to prevent breaking the whole list with a 404 + // (soft fail) + \OC::$server->getLogger()->warning( + 'Could not get node for path: \"' . $path . '\" : ' . $e->getMessage(), + array('app' => 'files') + ); + return; + } $requestedProps = $propFind->get404Properties(); @@ -129,7 +153,7 @@ class CustomPropertiesBackend implements BackendInterface { return; } - $props = $this->getProperties($path, $requestedProps); + $props = $this->getProperties($node, $requestedProps); foreach ($props as $propName => $propValue) { $propFind->set($propName, $propValue); } @@ -144,8 +168,13 @@ class CustomPropertiesBackend implements BackendInterface { * @return void */ public function propPatch($path, PropPatch $propPatch) { - $propPatch->handleRemaining(function($changedProps) use ($path) { - return $this->updateProperties($path, $changedProps); + $node = $this->tree->getNodeForPath($path); + if (!($node instanceof Node)) { + return; + } + + $propPatch->handleRemaining(function($changedProps) use ($node) { + return $this->updateProperties($node, $changedProps); }); } @@ -158,7 +187,7 @@ class CustomPropertiesBackend implements BackendInterface { $statement = $this->connection->prepare( 'DELETE FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ?' ); - $statement->execute(array($this->user, $path)); + $statement->execute(array($this->user->getUID(), $path)); $statement->closeCursor(); unset($this->cache[$path]); @@ -177,13 +206,13 @@ class CustomPropertiesBackend implements BackendInterface { 'UPDATE `*PREFIX*properties` SET `propertypath` = ?' . ' WHERE `userid` = ? AND `propertypath` = ?' ); - $statement->execute(array($destination, $this->user, $source)); + $statement->execute(array($destination, $this->user->getUID(), $source)); $statement->closeCursor(); } /** * Returns a list of properties for this nodes.; - * @param string $path + * @param Node $node * @param array $requestedProperties requested properties or empty array for "all" * @return array * @note The properties list is a list of propertynames the client @@ -191,7 +220,8 @@ class CustomPropertiesBackend implements BackendInterface { * http://www.example.org/namespace#author If the array is empty, all * properties should be returned */ - private function getProperties($path, array $requestedProperties) { + private function getProperties(Node $node, array $requestedProperties) { + $path = $node->getPath(); if (isset($this->cache[$path])) { return $this->cache[$path]; } @@ -199,7 +229,7 @@ class CustomPropertiesBackend implements BackendInterface { // TODO: chunking if more than 1000 properties $sql = 'SELECT * FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ?'; - $whereValues = array($this->user, $path); + $whereValues = array($this->user->getUID(), $path); $whereTypes = array(null, null); if (!empty($requestedProperties)) { @@ -229,12 +259,13 @@ class CustomPropertiesBackend implements BackendInterface { /** * Update properties * - * @param string $path node for which to update properties + * @param Node $node node for which to update properties * @param array $properties array of properties to update * * @return bool */ - private function updateProperties($path, $properties) { + private function updateProperties($node, $properties) { + $path = $node->getPath(); $deleteStatement = 'DELETE FROM `*PREFIX*properties`' . ' WHERE `userid` = ? AND `propertypath` = ? AND `propertyname` = ?'; @@ -246,7 +277,7 @@ class CustomPropertiesBackend implements BackendInterface { ' WHERE `userid` = ? AND `propertypath` = ? AND `propertyname` = ?'; // TODO: use "insert or update" strategy ? - $existing = $this->getProperties($path, array()); + $existing = $this->getProperties($node, array()); $this->connection->beginTransaction(); foreach ($properties as $propertyName => $propertyValue) { // If it was null, we need to delete the property @@ -254,7 +285,7 @@ class CustomPropertiesBackend implements BackendInterface { if (array_key_exists($propertyName, $existing)) { $this->connection->executeUpdate($deleteStatement, array( - $this->user, + $this->user->getUID(), $path, $propertyName ) @@ -264,7 +295,7 @@ class CustomPropertiesBackend implements BackendInterface { if (!array_key_exists($propertyName, $existing)) { $this->connection->executeUpdate($insertStatement, array( - $this->user, + $this->user->getUID(), $path, $propertyName, $propertyValue @@ -274,7 +305,7 @@ class CustomPropertiesBackend implements BackendInterface { $this->connection->executeUpdate($updateStatement, array( $propertyValue, - $this->user, + $this->user->getUID(), $path, $propertyName ) @@ -289,4 +320,55 @@ class CustomPropertiesBackend implements BackendInterface { return true; } + /** + * Bulk load properties for directory children + * + * @param Directory $node + * @param array $requestedProperties requested properties + * + * @return void + */ + private function loadChildrenProperties(Directory $node, $requestedProperties) { + $path = $node->getPath(); + if (isset($this->cache[$path])) { + // we already loaded them at some point + return; + } + + $childNodes = $node->getChildren(); + // pre-fill cache + foreach ($childNodes as $childNode) { + $this->cache[$childNode->getPath()] = []; + } + + $sql = 'SELECT * FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` LIKE ?'; + $sql .= ' AND `propertyname` in (?) ORDER BY `propertypath`, `propertyname`'; + + $result = $this->connection->executeQuery( + $sql, + array($this->user->getUID(), $this->connection->escapeLikeParameter(rtrim($path, '/')) . '/%', $requestedProperties), + array(null, null, \Doctrine\DBAL\Connection::PARAM_STR_ARRAY) + ); + + $oldPath = null; + $props = []; + while ($row = $result->fetch()) { + $path = $row['propertypath']; + if ($oldPath !== $path) { + // save previously gathered props + $this->cache[$oldPath] = $props; + $oldPath = $path; + // prepare props for next path + $props = []; + } + $props[$row['propertyname']] = $row['propertyvalue']; + } + if (!is_null($oldPath)) { + // save props from last run + $this->cache[$oldPath] = $props; + } + + $result->closeCursor(); + } + } diff --git a/apps/dav/tests/unit/Connector/Sabre/CustomPropertiesBackendTest.php b/apps/dav/tests/unit/Connector/Sabre/CustomPropertiesBackendTest.php index 495efefa79..855f727650 100644 --- a/apps/dav/tests/unit/Connector/Sabre/CustomPropertiesBackendTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/CustomPropertiesBackendTest.php @@ -58,7 +58,7 @@ class CustomPropertiesBackendTest extends \Test\TestCase { private $tree; /** - * @var \OCA\DAV\Connector\Sabre\CustomPropertiesBackend + * @var \OCA\DAV\DAV\CustomPropertiesBackend */ private $plugin; @@ -83,7 +83,7 @@ class CustomPropertiesBackendTest extends \Test\TestCase { ->method('getUID') ->will($this->returnValue($userId)); - $this->plugin = new \OCA\DAV\Connector\Sabre\CustomPropertiesBackend( + $this->plugin = new \OCA\DAV\DAV\CustomPropertiesBackend( $this->tree, \OC::$server->getDatabaseConnection(), $this->user diff --git a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php index 44e5c6464a..bbe3dcbd03 100644 --- a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php +++ b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php @@ -24,9 +24,11 @@ namespace OCA\DAV\Tests\DAV; +use OCA\DAV\Connector\Sabre\Node; use OCA\DAV\DAV\CustomPropertiesBackend; use OCP\IDBConnection; use OCP\IUser; +use Sabre\DAV\Exception\NotFound; use Sabre\DAV\PropFind; use Sabre\DAV\PropPatch; use Sabre\DAV\Tree; @@ -46,19 +48,42 @@ class CustomPropertiesBackendTest extends TestCase { /** @var CustomPropertiesBackend | \PHPUnit_Framework_MockObject_MockObject */ private $backend; + /** @var (Node | \PHPUnit_Framework_MockObject_MockObject)[] */ + private $nodes = []; + protected function setUp(): void { parent::setUp(); $this->tree = $this->createMock(Tree::class); $this->dbConnection = $this->createMock(IDBConnection::class); $this->user = $this->createMock(IUser::class); - $this->user->expects($this->once()) - ->method('getUID') + $this->user->method('getUID') ->with() ->will($this->returnValue('dummy_user_42')); $this->backend = new CustomPropertiesBackend($this->tree, $this->dbConnection, $this->user); + + $this->tree->method('getNodeForPath') + ->willReturnCallback(function ($path) { + if (isset($this->nodes[$path])) { + return $this->nodes[$path]; + } else { + throw new NotFound(); + } + }); + } + + /** + * @param string $path + * @return Node|\PHPUnit\Framework\MockObject\MockObject + */ + private function addNode($path) { + $node = $this->createMock(Node::class); + $node->method('getPath') + ->willReturn($path); + $this->nodes[$path] = $node; + return $node; } public function testPropFindNoDbCalls() { @@ -76,6 +101,7 @@ class CustomPropertiesBackendTest extends TestCase { $this->dbConnection->expects($this->never()) ->method($this->anything()); + $this->addNode('foo_bar_path_1337_0'); $this->backend->propFind('foo_bar_path_1337_0', $propFind); } @@ -88,7 +114,7 @@ class CustomPropertiesBackendTest extends TestCase { '{DAV:}getcontentlength', '{DAV:}getcontenttype', '{DAV:}getetag', - '{abc}def' + '{abc}def', ])); $propFind->expects($this->at(1)) @@ -101,7 +127,7 @@ class CustomPropertiesBackendTest extends TestCase { '{DAV:}displayname', '{urn:ietf:params:xml:ns:caldav}calendar-description', '{urn:ietf:params:xml:ns:caldav}calendar-timezone', - '{abc}def' + '{abc}def', ])); $statement = $this->createMock('\Doctrine\DBAL\Driver\Statement'); @@ -116,6 +142,7 @@ class CustomPropertiesBackendTest extends TestCase { [null, null, 102]) ->will($this->returnValue($statement)); + $this->addNode('calendars/foo/bar_path_1337_0'); $this->backend->propFind('calendars/foo/bar_path_1337_0', $propFind); } @@ -123,6 +150,7 @@ class CustomPropertiesBackendTest extends TestCase { * @dataProvider propPatchProvider */ public function testPropPatch($path, $propPatch) { + $this->addNode($path); $propPatch->expects($this->once()) ->method('handleRemaining'); From e178718e74097b072ad84173912800b951de4c57 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 31 Jan 2020 15:47:13 +0100 Subject: [PATCH 2/8] test custom properties backend against real database test behaviour not implementation Signed-off-by: Robin Appelman --- .../unit/DAV/CustomPropertiesBackendTest.php | 139 +++++++++++------- 1 file changed, 87 insertions(+), 52 deletions(-) diff --git a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php index bbe3dcbd03..45be59151e 100644 --- a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php +++ b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php @@ -34,12 +34,15 @@ use Sabre\DAV\PropPatch; use Sabre\DAV\Tree; use Test\TestCase; +/** + * @group DB + */ class CustomPropertiesBackendTest extends TestCase { /** @var Tree | \PHPUnit_Framework_MockObject_MockObject */ private $tree; - /** @var IDBConnection | \PHPUnit_Framework_MockObject_MockObject */ + /** @var IDBConnection */ private $dbConnection; /** @var IUser | \PHPUnit_Framework_MockObject_MockObject */ @@ -55,14 +58,17 @@ class CustomPropertiesBackendTest extends TestCase { parent::setUp(); $this->tree = $this->createMock(Tree::class); - $this->dbConnection = $this->createMock(IDBConnection::class); $this->user = $this->createMock(IUser::class); $this->user->method('getUID') ->with() ->will($this->returnValue('dummy_user_42')); + $this->dbConnection = \OC::$server->getDatabaseConnection(); - $this->backend = new CustomPropertiesBackend($this->tree, - $this->dbConnection, $this->user); + $this->backend = new CustomPropertiesBackend( + $this->tree, + $this->dbConnection, + $this->user + ); $this->tree->method('getNodeForPath') ->willReturnCallback(function ($path) { @@ -86,7 +92,49 @@ class CustomPropertiesBackendTest extends TestCase { return $node; } + protected function tearDown(): void { + $query = $this->dbConnection->getQueryBuilder(); + $query->delete('properties'); + $query->execute(); + + parent::tearDown(); + } + + protected function insertProps(string $user, string $path, array $props) { + foreach ($props as $name => $value) { + $this->insertProp($user, $path, $name, $value); + } + } + + protected function insertProp(string $user, string $path, string $name, string $value) { + $query = $this->dbConnection->getQueryBuilder(); + $query->insert('properties') + ->values([ + 'userid' => $query->createNamedParameter($user), + 'propertypath' => $query->createNamedParameter($path), + 'propertyname' => $query->createNamedParameter($name), + 'propertyvalue' => $query->createNamedParameter($value), + ]); + $query->execute(); + } + + protected function getProps(string $user, string $path) { + $query = $this->dbConnection->getQueryBuilder(); + $query->select('propertyname', 'propertyvalue') + ->from('properties') + ->where($query->expr()->eq('userid', $query->createNamedParameter($user))) + ->where($query->expr()->eq('propertypath', $query->createNamedParameter($path))); + return $query->execute()->fetchAll(\PDO::FETCH_KEY_PAIR); + } + public function testPropFindNoDbCalls() { + $db = $this->createMock(IDBConnection::class); + $backend = new CustomPropertiesBackend( + $this->tree, + $db, + $this->user + ); + $propFind = $this->createMock(PropFind::class); $propFind->expects($this->at(0)) ->method('get404Properties') @@ -98,17 +146,16 @@ class CustomPropertiesBackendTest extends TestCase { '{http://owncloud.org/ns}size', ])); - $this->dbConnection->expects($this->never()) + $db->expects($this->never()) ->method($this->anything()); $this->addNode('foo_bar_path_1337_0'); - $this->backend->propFind('foo_bar_path_1337_0', $propFind); + $backend->propFind('foo_bar_path_1337_0', $propFind); } public function testPropFindCalendarCall() { $propFind = $this->createMock(PropFind::class); - $propFind->expects($this->at(0)) - ->method('get404Properties') + $propFind->method('get404Properties') ->with() ->will($this->returnValue([ '{DAV:}getcontentlength', @@ -117,8 +164,7 @@ class CustomPropertiesBackendTest extends TestCase { '{abc}def', ])); - $propFind->expects($this->at(1)) - ->method('getRequestedProperties') + $propFind->method('getRequestedProperties') ->with() ->will($this->returnValue([ '{DAV:}getcontentlength', @@ -130,71 +176,60 @@ class CustomPropertiesBackendTest extends TestCase { '{abc}def', ])); - $statement = $this->createMock('\Doctrine\DBAL\Driver\Statement'); - $this->dbConnection->expects($this->once()) - ->method('executeQuery') - ->with('SELECT * FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ? AND `propertyname` in (?)', - ['dummy_user_42', 'calendars/foo/bar_path_1337_0', [ - 3 => '{abc}def', - 4 => '{DAV:}displayname', - 5 => '{urn:ietf:params:xml:ns:caldav}calendar-description', - 6 => '{urn:ietf:params:xml:ns:caldav}calendar-timezone']], - [null, null, 102]) - ->will($this->returnValue($statement)); + $props = [ + '{abc}def' => 'a', + '{DAV:}displayname' => 'b', + '{urn:ietf:params:xml:ns:caldav}calendar-description' => 'c', + '{urn:ietf:params:xml:ns:caldav}calendar-timezone' => 'd', + ]; + + $this->insertProps('dummy_user_42', 'calendars/foo/bar_path_1337_0', $props); + + $setProps = []; + $propFind->method('set') + ->willReturnCallback(function ($name, $value, $status) use (&$setProps) { + $setProps[$name] = $value; + }); $this->addNode('calendars/foo/bar_path_1337_0'); + $this->backend->propFind('calendars/foo/bar_path_1337_0', $propFind); + $this->assertEquals($props, $setProps); } /** * @dataProvider propPatchProvider */ - public function testPropPatch($path, $propPatch) { + public function testPropPatch(string $path, array $existing, array $props, array $result) { + $this->insertProps($this->user->getUID(), $path, $existing); $this->addNode($path); - $propPatch->expects($this->once()) - ->method('handleRemaining'); + $propPatch = new PropPatch($props); $this->backend->propPatch($path, $propPatch); + $propPatch->commit(); + + $storedProps = $this->getProps($this->user->getUID(), $path); + $this->assertEquals($result, $storedProps); } public function propPatchProvider() { - $propPatchMock = $this->createMock(PropPatch::class); return [ - ['foo_bar_path_1337', $propPatchMock], + ['foo_bar_path_1337', [], ['{DAV:}displayname' => 'anything'], ['{DAV:}displayname' => 'anything']], + ['foo_bar_path_1337', ['{DAV:}displayname' => 'foo'], ['{DAV:}displayname' => 'anything'], ['{DAV:}displayname' => 'anything']], + ['foo_bar_path_1337', ['{DAV:}displayname' => 'foo'], ['{DAV:}displayname' => null], []], ]; } public function testDelete() { - $statement = $this->createMock('\Doctrine\DBAL\Driver\Statement'); - $statement->expects($this->at(0)) - ->method('execute') - ->with(['dummy_user_42', 'foo_bar_path_1337']); - $statement->expects($this->at(1)) - ->method('closeCursor') - ->with(); - - $this->dbConnection->expects($this->at(0)) - ->method('prepare') - ->with('DELETE FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ?') - ->will($this->returnValue($statement)); - + $this->insertProps('dummy_user_42', 'foo_bar_path_1337', ['foo' => 'bar']); $this->backend->delete('foo_bar_path_1337'); + $this->assertEquals([], $this->getProps('dummy_user_42', 'foo_bar_path_1337')); } public function testMove() { - $statement = $this->createMock('\Doctrine\DBAL\Driver\Statement'); - $statement->expects($this->at(0)) - ->method('execute') - ->with(['bar_foo_path_7331', 'dummy_user_42', 'foo_bar_path_1337']); - $statement->expects($this->at(1)) - ->method('closeCursor') - ->with(); - - $this->dbConnection->expects($this->at(0)) - ->method('prepare') - ->with('UPDATE `*PREFIX*properties` SET `propertypath` = ? WHERE `userid` = ? AND `propertypath` = ?') - ->will($this->returnValue($statement)); - + $this->insertProps('dummy_user_42', 'foo_bar_path_1337', ['foo' => 'bar']); $this->backend->move('foo_bar_path_1337', 'bar_foo_path_7331'); + $this->assertEquals([], $this->getProps('dummy_user_42', 'foo_bar_path_1337')); + $this->assertEquals(['foo' => 'bar'], $this->getProps('dummy_user_42', 'bar_foo_path_7331')); } } From 95cd44c41e5202738721f8e67eb88d488257612d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 31 Jan 2020 15:56:52 +0100 Subject: [PATCH 3/8] remove unused code Signed-off-by: Robin Appelman --- apps/dav/lib/DAV/CustomPropertiesBackend.php | 53 -------------------- 1 file changed, 53 deletions(-) diff --git a/apps/dav/lib/DAV/CustomPropertiesBackend.php b/apps/dav/lib/DAV/CustomPropertiesBackend.php index 9f2fc571c6..c6357869a0 100644 --- a/apps/dav/lib/DAV/CustomPropertiesBackend.php +++ b/apps/dav/lib/DAV/CustomPropertiesBackend.php @@ -27,7 +27,6 @@ namespace OCA\DAV\DAV; -use OCA\DAV\Connector\Sabre\Directory; use OCA\DAV\Connector\Sabre\Node; use OCP\IDBConnection; use OCP\IUser; @@ -319,56 +318,4 @@ class CustomPropertiesBackend implements BackendInterface { return true; } - - /** - * Bulk load properties for directory children - * - * @param Directory $node - * @param array $requestedProperties requested properties - * - * @return void - */ - private function loadChildrenProperties(Directory $node, $requestedProperties) { - $path = $node->getPath(); - if (isset($this->cache[$path])) { - // we already loaded them at some point - return; - } - - $childNodes = $node->getChildren(); - // pre-fill cache - foreach ($childNodes as $childNode) { - $this->cache[$childNode->getPath()] = []; - } - - $sql = 'SELECT * FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` LIKE ?'; - $sql .= ' AND `propertyname` in (?) ORDER BY `propertypath`, `propertyname`'; - - $result = $this->connection->executeQuery( - $sql, - array($this->user->getUID(), $this->connection->escapeLikeParameter(rtrim($path, '/')) . '/%', $requestedProperties), - array(null, null, \Doctrine\DBAL\Connection::PARAM_STR_ARRAY) - ); - - $oldPath = null; - $props = []; - while ($row = $result->fetch()) { - $path = $row['propertypath']; - if ($oldPath !== $path) { - // save previously gathered props - $this->cache[$oldPath] = $props; - $oldPath = $path; - // prepare props for next path - $props = []; - } - $props[$row['propertyname']] = $row['propertyvalue']; - } - if (!is_null($oldPath)) { - // save props from last run - $this->cache[$oldPath] = $props; - } - - $result->closeCursor(); - } - } From 7819a904d7fe2db2bf48a1e041db6aa84976e4bd Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 31 Jan 2020 16:10:19 +0100 Subject: [PATCH 4/8] handle long property paths to hasing paths >250 chars Signed-off-by: Robin Appelman --- apps/dav/lib/DAV/CustomPropertiesBackend.php | 57 ++++++++++++------- .../unit/DAV/CustomPropertiesBackendTest.php | 52 +++++++++++++---- 2 files changed, 77 insertions(+), 32 deletions(-) diff --git a/apps/dav/lib/DAV/CustomPropertiesBackend.php b/apps/dav/lib/DAV/CustomPropertiesBackend.php index c6357869a0..5a0128e52c 100644 --- a/apps/dav/lib/DAV/CustomPropertiesBackend.php +++ b/apps/dav/lib/DAV/CustomPropertiesBackend.php @@ -44,7 +44,7 @@ class CustomPropertiesBackend implements BackendInterface { * * @var array */ - private $ignoredProperties = array( + private $ignoredProperties = [ '{DAV:}getcontentlength', '{DAV:}getcontenttype', '{DAV:}getetag', @@ -55,7 +55,7 @@ class CustomPropertiesBackend implements BackendInterface { '{http://owncloud.org/ns}dDC', '{http://owncloud.org/ns}size', '{http://nextcloud.org/ns}is-encrypted', - ); + ]; /** * @var Tree @@ -115,7 +115,7 @@ class CustomPropertiesBackend implements BackendInterface { // (soft fail) \OC::$server->getLogger()->warning( 'Could not get node for path: \"' . $path . '\" : ' . $e->getMessage(), - array('app' => 'files') + ['app' => 'files'] ); return; } @@ -172,7 +172,7 @@ class CustomPropertiesBackend implements BackendInterface { return; } - $propPatch->handleRemaining(function($changedProps) use ($node) { + $propPatch->handleRemaining(function ($changedProps) use ($node) { return $this->updateProperties($node, $changedProps); }); } @@ -186,7 +186,7 @@ class CustomPropertiesBackend implements BackendInterface { $statement = $this->connection->prepare( 'DELETE FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ?' ); - $statement->execute(array($this->user->getUID(), $path)); + $statement->execute([$this->user->getUID(), $this->formatPath($path)]); $statement->closeCursor(); unset($this->cache[$path]); @@ -205,12 +205,13 @@ class CustomPropertiesBackend implements BackendInterface { 'UPDATE `*PREFIX*properties` SET `propertypath` = ?' . ' WHERE `userid` = ? AND `propertypath` = ?' ); - $statement->execute(array($destination, $this->user->getUID(), $source)); + $statement->execute([$this->formatPath($destination), $this->user->getUID(), $this->formatPath($source)]); $statement->closeCursor(); } /** * Returns a list of properties for this nodes.; + * * @param Node $node * @param array $requestedProperties requested properties or empty array for "all" * @return array @@ -228,8 +229,8 @@ class CustomPropertiesBackend implements BackendInterface { // TODO: chunking if more than 1000 properties $sql = 'SELECT * FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ?'; - $whereValues = array($this->user->getUID(), $path); - $whereTypes = array(null, null); + $whereValues = [$this->user->getUID(), $this->formatPath($path)]; + $whereTypes = [null, null]; if (!empty($requestedProperties)) { // request only a subset @@ -276,38 +277,38 @@ class CustomPropertiesBackend implements BackendInterface { ' WHERE `userid` = ? AND `propertypath` = ? AND `propertyname` = ?'; // TODO: use "insert or update" strategy ? - $existing = $this->getProperties($node, array()); + $existing = $this->getProperties($node, []); $this->connection->beginTransaction(); foreach ($properties as $propertyName => $propertyValue) { // If it was null, we need to delete the property if (is_null($propertyValue)) { if (array_key_exists($propertyName, $existing)) { $this->connection->executeUpdate($deleteStatement, - array( + [ $this->user->getUID(), - $path, - $propertyName - ) + $this->formatPath($path), + $propertyName, + ] ); } } else { if (!array_key_exists($propertyName, $existing)) { $this->connection->executeUpdate($insertStatement, - array( + [ $this->user->getUID(), - $path, + $this->formatPath($path), $propertyName, - $propertyValue - ) + $propertyValue, + ] ); } else { $this->connection->executeUpdate($updateStatement, - array( + [ $propertyValue, $this->user->getUID(), - $path, - $propertyName - ) + $this->formatPath($path), + $propertyName, + ] ); } } @@ -318,4 +319,18 @@ class CustomPropertiesBackend implements BackendInterface { return true; } + + /** + * long paths are hashed to ensure they fit in the database + * + * @param string $path + * @return string + */ + private function formatPath(string $path): string { + if (strlen($path) > 250) { + return sha1($path); + } else { + return $path; + } + } } diff --git a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php index 45be59151e..3250361ed1 100644 --- a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php +++ b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php @@ -100,6 +100,14 @@ class CustomPropertiesBackendTest extends TestCase { parent::tearDown(); } + private function formatPath(string $path): string { + if (strlen($path) > 250) { + return sha1($path); + } else { + return $path; + } + } + protected function insertProps(string $user, string $path, array $props) { foreach ($props as $name => $value) { $this->insertProp($user, $path, $name, $value); @@ -111,7 +119,7 @@ class CustomPropertiesBackendTest extends TestCase { $query->insert('properties') ->values([ 'userid' => $query->createNamedParameter($user), - 'propertypath' => $query->createNamedParameter($path), + 'propertypath' => $query->createNamedParameter($this->formatPath($path)), 'propertyname' => $query->createNamedParameter($name), 'propertyvalue' => $query->createNamedParameter($value), ]); @@ -123,7 +131,7 @@ class CustomPropertiesBackendTest extends TestCase { $query->select('propertyname', 'propertyvalue') ->from('properties') ->where($query->expr()->eq('userid', $query->createNamedParameter($user))) - ->where($query->expr()->eq('propertypath', $query->createNamedParameter($path))); + ->where($query->expr()->eq('propertypath', $query->createNamedParameter($this->formatPath($path)))); return $query->execute()->fetchAll(\PDO::FETCH_KEY_PAIR); } @@ -213,23 +221,45 @@ class CustomPropertiesBackendTest extends TestCase { } public function propPatchProvider() { + $longPath = str_repeat('long_path', 100); return [ ['foo_bar_path_1337', [], ['{DAV:}displayname' => 'anything'], ['{DAV:}displayname' => 'anything']], ['foo_bar_path_1337', ['{DAV:}displayname' => 'foo'], ['{DAV:}displayname' => 'anything'], ['{DAV:}displayname' => 'anything']], ['foo_bar_path_1337', ['{DAV:}displayname' => 'foo'], ['{DAV:}displayname' => null], []], + [$longPath, [], ['{DAV:}displayname' => 'anything'], ['{DAV:}displayname' => 'anything']], ]; } - public function testDelete() { - $this->insertProps('dummy_user_42', 'foo_bar_path_1337', ['foo' => 'bar']); - $this->backend->delete('foo_bar_path_1337'); - $this->assertEquals([], $this->getProps('dummy_user_42', 'foo_bar_path_1337')); + /** + * @dataProvider deleteProvider + */ + public function testDelete(string $path) { + $this->insertProps('dummy_user_42', $path, ['foo' => 'bar']); + $this->backend->delete($path); + $this->assertEquals([], $this->getProps('dummy_user_42', $path)); } - public function testMove() { - $this->insertProps('dummy_user_42', 'foo_bar_path_1337', ['foo' => 'bar']); - $this->backend->move('foo_bar_path_1337', 'bar_foo_path_7331'); - $this->assertEquals([], $this->getProps('dummy_user_42', 'foo_bar_path_1337')); - $this->assertEquals(['foo' => 'bar'], $this->getProps('dummy_user_42', 'bar_foo_path_7331')); + public function deleteProvider() { + return [ + ['foo_bar_path_1337'], + [str_repeat('long_path', 100)] + ]; + } + + /** + * @dataProvider moveProvider + */ + public function testMove(string $source, string $target) { + $this->insertProps('dummy_user_42', $source, ['foo' => 'bar']); + $this->backend->move($source, $target); + $this->assertEquals([], $this->getProps('dummy_user_42', $source)); + $this->assertEquals(['foo' => 'bar'], $this->getProps('dummy_user_42', $target)); + } + + public function moveProvider() { + return [ + ['foo_bar_path_1337', 'foo_bar_path_7333'], + [str_repeat('long_path1', 100), str_repeat('long_path2', 100)] + ]; } } From 451c8761a710c62bc19b75ceba9de670b95aca9e Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 3 Feb 2020 13:23:40 +0100 Subject: [PATCH 5/8] use INode instead of Node for custom properties Signed-off-by: Robin Appelman --- apps/dav/lib/DAV/CustomPropertiesBackend.php | 11 ++++++----- .../unit/DAV/CustomPropertiesBackendTest.php | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/apps/dav/lib/DAV/CustomPropertiesBackend.php b/apps/dav/lib/DAV/CustomPropertiesBackend.php index 5a0128e52c..5e0842200b 100644 --- a/apps/dav/lib/DAV/CustomPropertiesBackend.php +++ b/apps/dav/lib/DAV/CustomPropertiesBackend.php @@ -32,6 +32,7 @@ use OCP\IDBConnection; use OCP\IUser; use Sabre\DAV\Exception\NotFound; use Sabre\DAV\Exception\ServiceUnavailable; +use Sabre\DAV\INode; use Sabre\DAV\PropertyStorage\Backend\BackendInterface; use Sabre\DAV\PropFind; use Sabre\DAV\PropPatch; @@ -103,7 +104,7 @@ class CustomPropertiesBackend implements BackendInterface { public function propFind($path, PropFind $propFind) { try { $node = $this->tree->getNodeForPath($path); - if (!($node instanceof Node)) { + if (!($node instanceof INode)) { return; } } catch (ServiceUnavailable $e) { @@ -168,7 +169,7 @@ class CustomPropertiesBackend implements BackendInterface { */ public function propPatch($path, PropPatch $propPatch) { $node = $this->tree->getNodeForPath($path); - if (!($node instanceof Node)) { + if (!($node instanceof INode)) { return; } @@ -220,7 +221,7 @@ class CustomPropertiesBackend implements BackendInterface { * http://www.example.org/namespace#author If the array is empty, all * properties should be returned */ - private function getProperties(Node $node, array $requestedProperties) { + private function getProperties(INode $node, array $requestedProperties) { $path = $node->getPath(); if (isset($this->cache[$path])) { return $this->cache[$path]; @@ -259,12 +260,12 @@ class CustomPropertiesBackend implements BackendInterface { /** * Update properties * - * @param Node $node node for which to update properties + * @param INode $node node for which to update properties * @param array $properties array of properties to update * * @return bool */ - private function updateProperties($node, $properties) { + private function updateProperties(INode $node, array $properties) { $path = $node->getPath(); $deleteStatement = 'DELETE FROM `*PREFIX*properties`' . diff --git a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php index 3250361ed1..9f0fbdc3a0 100644 --- a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php +++ b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php @@ -29,6 +29,7 @@ use OCA\DAV\DAV\CustomPropertiesBackend; use OCP\IDBConnection; use OCP\IUser; use Sabre\DAV\Exception\NotFound; +use Sabre\DAV\INode; use Sabre\DAV\PropFind; use Sabre\DAV\PropPatch; use Sabre\DAV\Tree; @@ -82,9 +83,21 @@ class CustomPropertiesBackendTest extends TestCase { /** * @param string $path - * @return Node|\PHPUnit\Framework\MockObject\MockObject + * @return INode|\PHPUnit\Framework\MockObject\MockObject */ private function addNode($path) { + $node = $this->createMock(INode::class); + $node->method('getPath') + ->willReturn($path); + $this->nodes[$path] = $node; + return $node; + } + + /** + * @param string $path + * @return Node|\PHPUnit\Framework\MockObject\MockObject + */ + private function addCalendar($path) { $node = $this->createMock(Node::class); $node->method('getPath') ->willReturn($path); From 15a21ee19a7a0db41a7e20c15eb1943a6f5f37b8 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 3 Feb 2020 13:37:30 +0100 Subject: [PATCH 6/8] remove the detour trough node and work with path directly Signed-off-by: Robin Appelman --- apps/dav/lib/DAV/CustomPropertiesBackend.php | 42 ++++--------------- .../unit/DAV/CustomPropertiesBackendTest.php | 39 ----------------- 2 files changed, 8 insertions(+), 73 deletions(-) diff --git a/apps/dav/lib/DAV/CustomPropertiesBackend.php b/apps/dav/lib/DAV/CustomPropertiesBackend.php index 5e0842200b..8b0408a461 100644 --- a/apps/dav/lib/DAV/CustomPropertiesBackend.php +++ b/apps/dav/lib/DAV/CustomPropertiesBackend.php @@ -102,25 +102,6 @@ class CustomPropertiesBackend implements BackendInterface { * @return void */ public function propFind($path, PropFind $propFind) { - try { - $node = $this->tree->getNodeForPath($path); - if (!($node instanceof INode)) { - return; - } - } catch (ServiceUnavailable $e) { - // might happen for unavailable mount points, skip - return; - } catch (NotFound $e) { - // in some rare (buggy) cases the node might not be found, - // we catch the exception to prevent breaking the whole list with a 404 - // (soft fail) - \OC::$server->getLogger()->warning( - 'Could not get node for path: \"' . $path . '\" : ' . $e->getMessage(), - ['app' => 'files'] - ); - return; - } - $requestedProps = $propFind->get404Properties(); // these might appear @@ -153,7 +134,7 @@ class CustomPropertiesBackend implements BackendInterface { return; } - $props = $this->getProperties($node, $requestedProps); + $props = $this->getProperties($path, $requestedProps); foreach ($props as $propName => $propValue) { $propFind->set($propName, $propValue); } @@ -168,13 +149,8 @@ class CustomPropertiesBackend implements BackendInterface { * @return void */ public function propPatch($path, PropPatch $propPatch) { - $node = $this->tree->getNodeForPath($path); - if (!($node instanceof INode)) { - return; - } - - $propPatch->handleRemaining(function ($changedProps) use ($node) { - return $this->updateProperties($node, $changedProps); + $propPatch->handleRemaining(function ($changedProps) use ($path) { + return $this->updateProperties($path, $changedProps); }); } @@ -213,7 +189,7 @@ class CustomPropertiesBackend implements BackendInterface { /** * Returns a list of properties for this nodes.; * - * @param Node $node + * @param string $path * @param array $requestedProperties requested properties or empty array for "all" * @return array * @note The properties list is a list of propertynames the client @@ -221,8 +197,7 @@ class CustomPropertiesBackend implements BackendInterface { * http://www.example.org/namespace#author If the array is empty, all * properties should be returned */ - private function getProperties(INode $node, array $requestedProperties) { - $path = $node->getPath(); + private function getProperties(string $path, array $requestedProperties) { if (isset($this->cache[$path])) { return $this->cache[$path]; } @@ -260,13 +235,12 @@ class CustomPropertiesBackend implements BackendInterface { /** * Update properties * - * @param INode $node node for which to update properties + * @param string $path path for which to update properties * @param array $properties array of properties to update * * @return bool */ - private function updateProperties(INode $node, array $properties) { - $path = $node->getPath(); + private function updateProperties(string $path, array $properties) { $deleteStatement = 'DELETE FROM `*PREFIX*properties`' . ' WHERE `userid` = ? AND `propertypath` = ? AND `propertyname` = ?'; @@ -278,7 +252,7 @@ class CustomPropertiesBackend implements BackendInterface { ' WHERE `userid` = ? AND `propertypath` = ? AND `propertyname` = ?'; // TODO: use "insert or update" strategy ? - $existing = $this->getProperties($node, []); + $existing = $this->getProperties($path, []); $this->connection->beginTransaction(); foreach ($properties as $propertyName => $propertyValue) { // If it was null, we need to delete the property diff --git a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php index 9f0fbdc3a0..45aab9af1f 100644 --- a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php +++ b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php @@ -52,9 +52,6 @@ class CustomPropertiesBackendTest extends TestCase { /** @var CustomPropertiesBackend | \PHPUnit_Framework_MockObject_MockObject */ private $backend; - /** @var (Node | \PHPUnit_Framework_MockObject_MockObject)[] */ - private $nodes = []; - protected function setUp(): void { parent::setUp(); @@ -71,38 +68,6 @@ class CustomPropertiesBackendTest extends TestCase { $this->user ); - $this->tree->method('getNodeForPath') - ->willReturnCallback(function ($path) { - if (isset($this->nodes[$path])) { - return $this->nodes[$path]; - } else { - throw new NotFound(); - } - }); - } - - /** - * @param string $path - * @return INode|\PHPUnit\Framework\MockObject\MockObject - */ - private function addNode($path) { - $node = $this->createMock(INode::class); - $node->method('getPath') - ->willReturn($path); - $this->nodes[$path] = $node; - return $node; - } - - /** - * @param string $path - * @return Node|\PHPUnit\Framework\MockObject\MockObject - */ - private function addCalendar($path) { - $node = $this->createMock(Node::class); - $node->method('getPath') - ->willReturn($path); - $this->nodes[$path] = $node; - return $node; } protected function tearDown(): void { @@ -170,7 +135,6 @@ class CustomPropertiesBackendTest extends TestCase { $db->expects($this->never()) ->method($this->anything()); - $this->addNode('foo_bar_path_1337_0'); $backend->propFind('foo_bar_path_1337_0', $propFind); } @@ -212,8 +176,6 @@ class CustomPropertiesBackendTest extends TestCase { $setProps[$name] = $value; }); - $this->addNode('calendars/foo/bar_path_1337_0'); - $this->backend->propFind('calendars/foo/bar_path_1337_0', $propFind); $this->assertEquals($props, $setProps); } @@ -223,7 +185,6 @@ class CustomPropertiesBackendTest extends TestCase { */ public function testPropPatch(string $path, array $existing, array $props, array $result) { $this->insertProps($this->user->getUID(), $path, $existing); - $this->addNode($path); $propPatch = new PropPatch($props); $this->backend->propPatch($path, $propPatch); From 861c897e93633d4561917128e30b4adc6fe84a79 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 3 Feb 2020 13:41:55 +0100 Subject: [PATCH 7/8] fix tests Signed-off-by: Robin Appelman --- .../Sabre/CustomPropertiesBackendTest.php | 59 +------------------ 1 file changed, 2 insertions(+), 57 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/CustomPropertiesBackendTest.php b/apps/dav/tests/unit/Connector/Sabre/CustomPropertiesBackendTest.php index 855f727650..a78d64f9c9 100644 --- a/apps/dav/tests/unit/Connector/Sabre/CustomPropertiesBackendTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/CustomPropertiesBackendTest.php @@ -144,16 +144,6 @@ class CustomPropertiesBackendTest extends \Test\TestCase { * Test that propFind on a missing file soft fails */ public function testPropFindMissingFileSoftFail() { - $this->tree->expects($this->at(0)) - ->method('getNodeForPath') - ->with('/dummypath') - ->will($this->throwException(new \Sabre\DAV\Exception\NotFound())); - - $this->tree->expects($this->at(1)) - ->method('getNodeForPath') - ->with('/dummypath') - ->will($this->throwException(new \Sabre\DAV\Exception\ServiceUnavailable())); - $propFind = new \Sabre\DAV\PropFind( '/dummypath', array( @@ -174,20 +164,14 @@ class CustomPropertiesBackendTest extends \Test\TestCase { $propFind ); - // no exception, soft fail - $this->addToAssertionCount(1); + // assert that the above didn't throw exceptions + $this->assertTrue(true); } /** * Test setting/getting properties */ public function testSetGetPropertiesForFile() { - $node = $this->createTestNode(File::class); - $this->tree->expects($this->any()) - ->method('getNodeForPath') - ->with('/dummypath') - ->will($this->returnValue($node)); - $this->applyDefaultProps(); $propFind = new \Sabre\DAV\PropFind( @@ -214,39 +198,6 @@ class CustomPropertiesBackendTest extends \Test\TestCase { * Test getting properties from directory */ public function testGetPropertiesForDirectory() { - $rootNode = $this->createTestNode(Directory::class); - - $nodeSub = $this->getMockBuilder(File::class) - ->disableOriginalConstructor() - ->getMock(); - $nodeSub->expects($this->any()) - ->method('getId') - ->will($this->returnValue(456)); - - $nodeSub->expects($this->any()) - ->method('getPath') - ->will($this->returnValue('/dummypath/test.txt')); - - $this->tree->expects($this->at(0)) - ->method('getNodeForPath') - ->with('/dummypath') - ->will($this->returnValue($rootNode)); - - $this->tree->expects($this->at(1)) - ->method('getNodeForPath') - ->with('/dummypath/test.txt') - ->will($this->returnValue($nodeSub)); - - $this->tree->expects($this->at(2)) - ->method('getNodeForPath') - ->with('/dummypath') - ->will($this->returnValue($rootNode)); - - $this->tree->expects($this->at(3)) - ->method('getNodeForPath') - ->with('/dummypath/test.txt') - ->will($this->returnValue($nodeSub)); - $this->applyDefaultProps('/dummypath'); $this->applyDefaultProps('/dummypath/test.txt'); @@ -294,12 +245,6 @@ class CustomPropertiesBackendTest extends \Test\TestCase { * Test delete property */ public function testDeleteProperty() { - $node = $this->createTestNode(File::class); - $this->tree->expects($this->any()) - ->method('getNodeForPath') - ->with('/dummypath') - ->will($this->returnValue($node)); - $this->applyDefaultProps(); $propPatch = new \Sabre\DAV\PropPatch(array( From e44c276aab230fbc907f05b5981025614a090c2c Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 4 Feb 2020 13:11:51 +0100 Subject: [PATCH 8/8] rebuild autoloader Signed-off-by: Robin Appelman --- apps/dav/composer/composer/autoload_classmap.php | 1 - apps/dav/composer/composer/autoload_static.php | 1 - 2 files changed, 2 deletions(-) diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index ecf51164e8..ab0c7ee63b 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -124,7 +124,6 @@ return array( 'OCA\\DAV\\Connector\\Sabre\\ChecksumList' => $baseDir . '/../lib/Connector/Sabre/ChecksumList.php', 'OCA\\DAV\\Connector\\Sabre\\CommentPropertiesPlugin' => $baseDir . '/../lib/Connector/Sabre/CommentPropertiesPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\CopyEtagHeaderPlugin' => $baseDir . '/../lib/Connector/Sabre/CopyEtagHeaderPlugin.php', - 'OCA\\DAV\\Connector\\Sabre\\CustomPropertiesBackend' => $baseDir . '/../lib/Connector/Sabre/CustomPropertiesBackend.php', 'OCA\\DAV\\Connector\\Sabre\\DavAclPlugin' => $baseDir . '/../lib/Connector/Sabre/DavAclPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\Directory' => $baseDir . '/../lib/Connector/Sabre/Directory.php', 'OCA\\DAV\\Connector\\Sabre\\DummyGetResponsePlugin' => $baseDir . '/../lib/Connector/Sabre/DummyGetResponsePlugin.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 4df92c174e..b142b93d79 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -139,7 +139,6 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Connector\\Sabre\\ChecksumList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ChecksumList.php', 'OCA\\DAV\\Connector\\Sabre\\CommentPropertiesPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/CommentPropertiesPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\CopyEtagHeaderPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/CopyEtagHeaderPlugin.php', - 'OCA\\DAV\\Connector\\Sabre\\CustomPropertiesBackend' => __DIR__ . '/..' . '/../lib/Connector/Sabre/CustomPropertiesBackend.php', 'OCA\\DAV\\Connector\\Sabre\\DavAclPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/DavAclPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\Directory' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Directory.php', 'OCA\\DAV\\Connector\\Sabre\\DummyGetResponsePlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/DummyGetResponsePlugin.php',