refactor(CustomPropertiesBackend): Modernize class

- Use query builder
- Add chunking
- Add type hinting where we can
- Use match expression

Signed-off-by: Carl Schwan <carl.schwan@nextclound.com>
Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>

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<string, class-string>
 	 */
 	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<string, string>
 	 */
 	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 readonly Server $server,
-		private Tree $tree,
+		private readonly Tree $tree,
-		private IDBConnection $connection,
+		private readonly IDBConnection $connection,
-		private IUser $user,
+		private readonly IUser $user,
-		private PropertyMapper $propertyMapper,
+		private readonly PropertyMapper $propertyMapper,
-		private DefaultCalendarValidator $defaultCalendarValidator,
+		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) {
+	#[Override]
-		$propPatch->handleRemaining(function ($changedProps) use ($path) {
+	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) {
+	#[Override]
-		$statement = $this->connection->prepare(
+	public function delete($path): void {
-			'DELETE FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ?'
+		$qb = $this->connection->getQueryBuilder();
-		);
+		$qb->delete('properties')
-		$statement->execute([$this->user->getUID(), $this->formatPath($path)]);
+			->where($qb->expr()->eq('userid', $qb->createNamedParameter($this->user->getUID())))
-		$statement->closeCursor();
+			->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) {
+	#[Override]
-		$statement = $this->connection->prepare(
+	public function move($source, $destination): void {
-			'UPDATE `*PREFIX*properties` SET `propertypath` = ?'
+		$qb = $this->connection->getQueryBuilder();
-			. ' WHERE `userid` = ? AND `propertypath` = ?'
+		$qb->update('properties')
-		);
+			->set('propertypath', $qb->createNamedParameter($this->formatPath($destination)))
-		$statement->execute([$this->formatPath($destination), $this->user->getUID(), $this->formatPath($source)]);
+			->where($qb->expr()->eq('userid', $qb->createNamedParameter($this->user->getUID())))
-		$statement->closeCursor();
+			->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<string, mixed|Complex|Href|string>
+	 * @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<string, mixed>
 	 * @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
+		$props = [];
-		$sql = 'SELECT * FROM `*PREFIX*properties` WHERE `userid` = ? AND `propertypath` = ?';
 
-		$whereValues = [$this->user->getUID(), $this->formatPath($path)];
+		$qb = $this->connection->getQueryBuilder();
-		$whereTypes = [null, null];
+		$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 (?)';
+			$qb->andWhere($qb->expr()->in('propertyname', $qb->createParameter('requestedProperties')));
-			$whereValues[] = $requestedProperties;
+			$chunks = array_chunk($requestedProperties, 1000);
-			$whereTypes[] = IQueryBuilder::PARAM_STR_ARRAY;
+			foreach ($chunks as $chunk) {
-		}
+				$qb->setParameter('requestedProperties', $chunk, IQueryBuilder::PARAM_STR_ARRAY);
-
+				$result = $qb->executeQuery();
-		$result = $this->connection->executeQuery(
+				while ($row = $result->fetch()) {
-			$sql,
+					$props[$row['propertyname']] = $this->decodeValueFromDatabase($row['propertyvalue'], $row['valuetype']);
-			$whereValues,
+				}
-			$whereTypes
+			}
-		);
+		} else {
-
+			$result = $qb->executeQuery();
-		$props = [];
+			while ($row = $result->fetch()) {
-		while ($row = $result->fetch()) {
+				$props[$row['propertyname']] = $this->decodeValueFromDatabase($row['propertyvalue'], $row['valuetype']);
-			$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<string, string> $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
+	 * 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) {
@@ -616,20 +611,15 @@ class CustomPropertiesBackend implements BackendInterface {
 	/**
 	 * @return mixed|Complex|string
 	 */
-	private function decodeValueFromDatabase(string $value, int $valueType) {
+	private function decodeValueFromDatabase(string $value, int $valueType): mixed {
-		switch ($valueType) {
+		return match ($valueType) {
-			case self::PROPERTY_TYPE_XML:
+			self::PROPERTY_TYPE_XML => new Complex($value),
-				return new Complex($value);
+			self::PROPERTY_TYPE_HREF => new Href($value),
-			case self::PROPERTY_TYPE_HREF:
+			// some databases can not handel null characters, these are custom encoded during serialization
-				return new Href($value);
+			// this custom encoding needs to be first reversed before unserializing
-			case self::PROPERTY_TYPE_OBJECT:
+			self::PROPERTY_TYPE_OBJECT => unserialize(str_replace('\x00', chr(0), $value)),
-				// some databases can not handel null characters, these are custom encoded during serialization
+			default => $value,
-				// 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 encodeDefaultCalendarUrl(Href $value): Href {

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'];

build/psalm-baseline.xml

@@ -792,15 +792,6 @@
       <code><![CDATA[$vEvent->DTSTAMP]]></code>
     </UndefinedPropertyAssignment>
   </file>
-  <file src="apps/dav/lib/DAV/CustomPropertiesBackend.php">
-    <DeprecatedMethod>
-      <code><![CDATA[closeCursor]]></code>
-      <code><![CDATA[closeCursor]]></code>
-    </DeprecatedMethod>
-    <InvalidArgument>
-      <code><![CDATA[$whereValues]]></code>
-    </InvalidArgument>
-  </file>
   <file src="apps/dav/lib/DAV/GroupPrincipalBackend.php">
     <InvalidNullableReturnType>
       <code><![CDATA[array]]></code>