From b303185126aa16213f668c500f9a732f13ef130b Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 26 Aug 2025 10:35:07 +0200 Subject: [PATCH] refactor(CustomPropertiesBackend): Modernize class - Use query builder - Add chunking - Add type hinting where we can - Use match expression Signed-off-by: Carl Schwan Signed-off-by: Carl Schwan --- apps/dav/lib/DAV/CustomPropertiesBackend.php | 140 ++++++++---------- .../unit/DAV/CustomPropertiesBackendTest.php | 6 +- build/psalm-baseline.xml | 9 -- 3 files changed, 68 insertions(+), 87 deletions(-) diff --git a/apps/dav/lib/DAV/CustomPropertiesBackend.php b/apps/dav/lib/DAV/CustomPropertiesBackend.php index be7345f25df..dc207d17880 100644 --- a/apps/dav/lib/DAV/CustomPropertiesBackend.php +++ b/apps/dav/lib/DAV/CustomPropertiesBackend.php @@ -22,6 +22,7 @@ use OCA\DAV\Db\PropertyMapper; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; use OCP\IUser; +use Override; use Sabre\CalDAV\Schedule\Inbox; use Sabre\DAV\Exception as DavException; use Sabre\DAV\PropertyStorage\Backend\BackendInterface; @@ -98,6 +99,7 @@ class CustomPropertiesBackend implements BackendInterface { /** * Map of custom XML elements to parse when trying to deserialize an instance of * \Sabre\DAV\Xml\Property\Complex to find a more specialized PROPERTY_TYPE_* + * @var array */ private const COMPLEX_XML_ELEMENT_MAP = [ '{urn:ietf:params:xml:ns:caldav}schedule-default-calendar-URL' => Href::class, @@ -105,6 +107,7 @@ class CustomPropertiesBackend implements BackendInterface { /** * Map of well-known property names to default values + * @var array */ private const PROPERTY_DEFAULT_VALUES = [ '{http://owncloud.org/ns}calendar-enabled' => '1', @@ -118,17 +121,15 @@ class CustomPropertiesBackend implements BackendInterface { private XmlService $xmlService; /** - * @param Tree $tree node tree - * @param IDBConnection $connection database connection * @param IUser $user owner of the tree and properties */ public function __construct( - private Server $server, - private Tree $tree, - private IDBConnection $connection, - private IUser $user, - private PropertyMapper $propertyMapper, - private DefaultCalendarValidator $defaultCalendarValidator, + private readonly Server $server, + private readonly Tree $tree, + private readonly IDBConnection $connection, + private readonly IUser $user, + private readonly PropertyMapper $propertyMapper, + private readonly DefaultCalendarValidator $defaultCalendarValidator, ) { $this->xmlService = new XmlService(); $this->xmlService->elementMap = array_merge( @@ -142,9 +143,9 @@ class CustomPropertiesBackend implements BackendInterface { * * @param string $path * @param PropFind $propFind - * @return void */ - public function propFind($path, PropFind $propFind) { + #[Override] + public function propFind($path, PropFind $propFind): void { $requestedProps = $propFind->get404Properties(); $requestedProps = array_filter( @@ -257,12 +258,10 @@ class CustomPropertiesBackend implements BackendInterface { * Updates properties for a path * * @param string $path - * @param PropPatch $propPatch - * - * @return void */ - public function propPatch($path, PropPatch $propPatch) { - $propPatch->handleRemaining(function ($changedProps) use ($path) { + #[Override] + public function propPatch($path, PropPatch $propPatch): void { + $propPatch->handleRemaining(function (array $changedProps) use ($path) { return $this->updateProperties($path, $changedProps); }); } @@ -272,13 +271,13 @@ class CustomPropertiesBackend implements BackendInterface { * * @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([$this->user->getUID(), $this->formatPath($path)]); - $statement->closeCursor(); - + #[Override] + public function delete($path): void { + $qb = $this->connection->getQueryBuilder(); + $qb->delete('properties') + ->where($qb->expr()->eq('userid', $qb->createNamedParameter($this->user->getUID()))) + ->andWhere($qb->expr()->eq('propertypath', $qb->createNamedParameter($this->formatPath($path)))); + $qb->executeStatement(); unset($this->userCache[$path]); } @@ -287,16 +286,15 @@ class CustomPropertiesBackend implements BackendInterface { * * @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([$this->formatPath($destination), $this->user->getUID(), $this->formatPath($source)]); - $statement->closeCursor(); + #[Override] + public function move($source, $destination): void { + $qb = $this->connection->getQueryBuilder(); + $qb->update('properties') + ->set('propertypath', $qb->createNamedParameter($this->formatPath($destination))) + ->where($qb->expr()->eq('userid', $qb->createNamedParameter($this->user->getUID()))) + ->andWhere($qb->expr()->eq('propertypath', $qb->createNamedParameter($this->formatPath($source)))); + $qb->executeStatement(); } /** @@ -325,10 +323,10 @@ class CustomPropertiesBackend implements BackendInterface { } /** - * @param string $path * @param string[] $requestedProperties * - * @return array + * @return array + * @throws \OCP\DB\Exception */ private function getPublishedProperties(string $path, array $requestedProperties): array { $allowedProps = array_intersect(self::PUBLISHED_READ_ONLY_PROPERTIES, $requestedProperties); @@ -356,7 +354,7 @@ class CustomPropertiesBackend implements BackendInterface { } /** - * prefetch all user properties in a directory + * Prefetch all user properties in a directory */ private function cacheDirectory(string $path, Directory $node): void { $prefix = ltrim($path . '/', '/'); @@ -449,45 +447,44 @@ class CustomPropertiesBackend implements BackendInterface { /** * Returns a list of properties for the given path and current user * - * @param string $path * @param array $requestedProperties requested properties or empty array for "all" - * @return array + * @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 getUserProperties(string $path, array $requestedProperties) { + private function getUserProperties(string $path, array $requestedProperties): array { if (isset($this->userCache[$path])) { return $this->userCache[$path]; } - // TODO: chunking if more than 1000 properties - $sql = 'SELECT * FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ?'; + $props = []; - $whereValues = [$this->user->getUID(), $this->formatPath($path)]; - $whereTypes = [null, null]; + $qb = $this->connection->getQueryBuilder(); + $qb->select('*') + ->from('properties') + ->where($qb->expr()->eq('userid', $qb->createNamedParameter($this->user->getUID(), IQueryBuilder::PARAM_STR))) + ->andWhere($qb->expr()->eq('propertypath', $qb->createNamedParameter($this->formatPath($path), IQueryBuilder::PARAM_STR))); if (!empty($requestedProperties)) { // request only a subset - $sql .= ' AND `propertyname` in (?)'; - $whereValues[] = $requestedProperties; - $whereTypes[] = IQueryBuilder::PARAM_STR_ARRAY; - } - - $result = $this->connection->executeQuery( - $sql, - $whereValues, - $whereTypes - ); - - $props = []; - while ($row = $result->fetch()) { - $props[$row['propertyname']] = $this->decodeValueFromDatabase($row['propertyvalue'], $row['valuetype']); + $qb->andWhere($qb->expr()->in('propertyname', $qb->createParameter('requestedProperties'))); + $chunks = array_chunk($requestedProperties, 1000); + foreach ($chunks as $chunk) { + $qb->setParameter('requestedProperties', $chunk, IQueryBuilder::PARAM_STR_ARRAY); + $result = $qb->executeQuery(); + while ($row = $result->fetch()) { + $props[$row['propertyname']] = $this->decodeValueFromDatabase($row['propertyvalue'], $row['valuetype']); + } + } + } else { + $result = $qb->executeQuery(); + while ($row = $result->fetch()) { + $props[$row['propertyname']] = $this->decodeValueFromDatabase($row['propertyvalue'], $row['valuetype']); + } } - $result->closeCursor(); - $this->userCache[$path] = $props; return $props; } @@ -501,6 +498,7 @@ class CustomPropertiesBackend implements BackendInterface { } /** + * @param array $properties * @throws Exception */ private function updateProperties(string $path, array $properties): bool { @@ -558,10 +556,7 @@ class CustomPropertiesBackend implements BackendInterface { } /** - * long paths are hashed to ensure they fit in the database - * - * @param string $path - * @return string + * Long paths are hashed to ensure they fit in the database */ private function formatPath(string $path): string { if (strlen($path) > 250) { @@ -616,20 +611,15 @@ class CustomPropertiesBackend implements BackendInterface { /** * @return mixed|Complex|string */ - private function decodeValueFromDatabase(string $value, int $valueType) { - switch ($valueType) { - case self::PROPERTY_TYPE_XML: - return new Complex($value); - case self::PROPERTY_TYPE_HREF: - return new Href($value); - case self::PROPERTY_TYPE_OBJECT: - // some databases can not handel null characters, these are custom encoded during serialization - // this custom encoding needs to be first reversed before unserializing - return unserialize(str_replace('\x00', chr(0), $value)); - case self::PROPERTY_TYPE_STRING: - default: - return $value; - } + private function decodeValueFromDatabase(string $value, int $valueType): mixed { + return match ($valueType) { + self::PROPERTY_TYPE_XML => new Complex($value), + self::PROPERTY_TYPE_HREF => new Href($value), + // some databases can not handel null characters, these are custom encoded during serialization + // this custom encoding needs to be first reversed before unserializing + self::PROPERTY_TYPE_OBJECT => unserialize(str_replace('\x00', chr(0), $value)), + default => $value, + }; } private function encodeDefaultCalendarUrl(Href $value): Href { diff --git a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php index 517969fc9a3..08fdf288f9c 100644 --- a/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php +++ b/apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php @@ -67,7 +67,7 @@ class CustomPropertiesBackendTest extends TestCase { protected function tearDown(): void { $query = $this->dbConnection->getQueryBuilder(); $query->delete('properties'); - $query->execute(); + $query->executeStatement(); parent::tearDown(); } @@ -102,7 +102,7 @@ class CustomPropertiesBackendTest extends TestCase { 'propertyvalue' => $query->createNamedParameter($value), 'valuetype' => $query->createNamedParameter($type, IQueryBuilder::PARAM_INT) ]); - $query->execute(); + $query->executeStatement(); } protected function getProps(string $user, string $path): array { @@ -112,7 +112,7 @@ class CustomPropertiesBackendTest extends TestCase { ->where($query->expr()->eq('userid', $query->createNamedParameter($user))) ->andWhere($query->expr()->eq('propertypath', $query->createNamedParameter($this->formatPath($path)))); - $result = $query->execute(); + $result = $query->executeQuery(); $data = []; while ($row = $result->fetch()) { $value = $row['propertyvalue']; diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 44176b2bfd0..3154b50682e 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -792,15 +792,6 @@ DTSTAMP]]> - - - - - - - - -