Complex webdav properties support

owncloud · Apr 27, 2020 · 18e24f4 · 18e24f4
1 parent 1cff7eb
commit 18e24f4
Show file tree

Hide file tree

Showing 6 changed files with 143 additions and 31 deletions.
diff --git a/apps/dav/appinfo/Migrations/Version20200427142541.php b/apps/dav/appinfo/Migrations/Version20200427142541.php
@@ -0,0 +1,56 @@
+<?php
+/**
+ * @author Viktar Dubiniuk <[email protected]>
+ *
+ * @copyright Copyright (c) 2020, ownCloud GmbH
+ * @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 <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCA\DAV\Migrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\DBAL\Types\Type;
+use OCA\DAV\DAV\AbstractCustomPropertiesBackend;
+use OCP\Migration\ISchemaMigration;
+
+/*
+ * Adds a new property type column to properties and dav_properties tables
+ */
+class Version20200427142541 implements ISchemaMigration {
+
+	/**
+	 * @param Schema $schema
+	 * @param array $options
+	 */
+	public function changeSchema(Schema $schema, array $options) {
+		$prefix = $options['tablePrefix'];
+		$tableNames = ['properties', 'dav_properties'];
+
+		foreach ($tableNames as $tableName) {
+			$table = $schema->getTable("{$prefix}{$tableName}");
+			if ($table->hasColumn('propertytype') === false) {
+				$table->addColumn(
+					'propertytype',
+					Type::SMALLINT,
+					[
+						'notnull' => true,
+						'default' => AbstractCustomPropertiesBackend::VT_STRING,
+					]
+				);
+			}
+		}
+	}
+}
diff --git a/apps/dav/lib/DAV/AbstractCustomPropertiesBackend.php b/apps/dav/lib/DAV/AbstractCustomPropertiesBackend.php
@@ -32,8 +32,23 @@
 use Sabre\DAV\PropFind;
 use Sabre\DAV\PropPatch;
 use Sabre\DAV\Tree;
+use Sabre\DAV\Xml\Property\Complex;
 
 abstract class AbstractCustomPropertiesBackend implements BackendInterface {
+	/**
+	 * Value is stored as string.
+	 */
+	const VT_STRING = 1;
+
+	/**
+	 * Value is stored as XML fragment.
+	 */
+	const VT_XML = 2;
+
+	/**
+	 * Value is stored as a property object.
+	 */
+	const VT_OBJECT = 3;
 
 	/**
 	 * Ignored properties
@@ -230,7 +245,7 @@ protected function fetchProperties($sql, $whereValues, $whereTypes) {
 
 		$props = [];
 		while ($row = $result->fetch()) {
-			$props[$row['propertyname']] = $row['propertyvalue'];
+			$props[$row['propertyname']] = $this->decodeValue($row['propertyvalue'], $row['propertytype']);
 		}
 
 		$result->closeCursor();
@@ -264,4 +279,39 @@ protected function getNodeForPath($path) {
 		}
 		return null;
 	}
+
+	/**
+	 * @param mixed|Complex $value
+	 * @return array
+	 */
+	protected function encodeValue($value) {
+		if (\is_scalar($value)) {
+			$valueType = self::VT_STRING;
+		} elseif ($value instanceof Complex) {
+			$valueType = self::VT_XML;
+			$value = $value->getXml();
+		} else {
+			$valueType = self::VT_OBJECT;
+			$value = \serialize($value);
+		}
+		return [
+			'value' => $value,
+			'type' => $valueType
+		];
+	}
+
+	/**
+	 * @param string $value
+	 * @param int $valueType
+	 * @return mixed|Complex
+	 */
+	protected function decodeValue($value, $valueType) {
+		if ($valueType === self::VT_STRING) {
+			return $value;
+		} elseif ($valueType === self::VT_XML) {
+			return new Complex($value);
+		} elseif ($valueType === self::VT_OBJECT) {
+			return \unserialize($value);
+		}
+	}
 }
diff --git a/apps/dav/lib/DAV/FileCustomPropertiesBackend.php b/apps/dav/lib/DAV/FileCustomPropertiesBackend.php
@@ -43,9 +43,9 @@
 class FileCustomPropertiesBackend extends AbstractCustomPropertiesBackend {
 	public const SELECT_BY_ID_STMT = 'SELECT * FROM `*PREFIX*properties` WHERE `fileid` = ?';
 	public const INSERT_BY_ID_STMT = 'INSERT INTO `*PREFIX*properties`'
-	. ' (`fileid`,`propertyname`,`propertyvalue`) VALUES(?,?,?)';
+	. ' (`fileid`,`propertyname`,`propertyvalue`, `propertytype`) VALUES(?,?,?,?)';
 	public const UPDATE_BY_ID_AND_NAME_STMT = 'UPDATE `*PREFIX*properties`'
-	. ' SET `propertyvalue` = ? WHERE `fileid` = ? AND `propertyname` = ?';
+	. ' SET `propertyvalue` = ?, `propertytype` = ? WHERE `fileid` = ? AND `propertyname` = ?';
 	public const DELETE_BY_ID_STMT = 'DELETE FROM `*PREFIX*properties` WHERE `fileid` = ?';
 	public const DELETE_BY_ID_AND_NAME_STMT = 'DELETE FROM `*PREFIX*properties`'
 	. ' WHERE `fileid` = ? AND `propertyname` = ?';
@@ -168,9 +168,6 @@ protected function updateProperties($path, INode $node, $changedProperties) {
 		$existingProperties = $this->getProperties($path, $node, []);
 		'@phan-var \OCA\DAV\Connector\Sabre\Node $node';
 		$fileId = $node->getId();
-		$deleteStatement = self::DELETE_BY_ID_AND_NAME_STMT;
-		$insertStatement = self::INSERT_BY_ID_STMT;
-		$updateStatement = self::UPDATE_BY_ID_AND_NAME_STMT;
 
 		// TODO: use "insert or update" strategy ?
 		$this->connection->beginTransaction();
@@ -179,32 +176,32 @@ protected function updateProperties($path, INode $node, $changedProperties) {
 			// If it was null, we need to delete the property
 			if ($propertyValue === null) {
 				if ($propertyExists) {
-					$this->connection->executeUpdate($deleteStatement,
+					$this->connection->executeUpdate(
+						self::DELETE_BY_ID_AND_NAME_STMT,
 						[
 							$fileId,
 							$propertyName
 						]
 					);
 				}
 			} else {
-				// FIXME: PHP 7.4 handles object serialization differently so we store 'Object' here
-				// to keep old (wrong) behavior and fix Oracle failure
-				// see https://github.com/owncloud/core/issues/32670
-				if ($propertyValue instanceof Complex) {
-					$propertyValue = 'Object';
-				}
+				$propertyData = $this->encodeValue($propertyValue);
 				if (!$propertyExists) {
-					$this->connection->executeUpdate($insertStatement,
+					$this->connection->executeUpdate(
+						self::INSERT_BY_ID_STMT,
 						[
 							$fileId,
 							$propertyName,
-							$propertyValue
+							$propertyData['value'],
+							$propertyData['type']
 						]
 					);
 				} else {
-					$this->connection->executeUpdate($updateStatement,
+					$this->connection->executeUpdate(
+						self::UPDATE_BY_ID_AND_NAME_STMT,
 						[
-							$propertyValue,
+							$propertyData['value'],
+							$propertyData['type'],
 							$fileId,
 							$propertyName
 						]
@@ -266,7 +263,7 @@ protected function loadChildrenProperties(INode $node, $requestedProperties) {
 			);
 			while ($row = $result->fetch()) {
 				$props = $this->offsetGet($row['fileid']) ?? [];
-				$props[$row['propertyname']] = $row['propertyvalue'];
+				$props[$row['propertyname']] = $this->decodeValue($row['propertyvalue'], $row['propertytype']);
 				$this->offsetSet($row['fileid'], $props);
 			}
 			$result->closeCursor();

diff --git a/apps/dav/lib/DAV/MiscCustomPropertiesBackend.php b/apps/dav/lib/DAV/MiscCustomPropertiesBackend.php
@@ -36,11 +36,11 @@
 class MiscCustomPropertiesBackend extends AbstractCustomPropertiesBackend {
 	const SELECT_BY_PATH_STMT = 'SELECT * FROM `*PREFIX*dav_properties` WHERE `propertypath` = ?';
 	const INSERT_BY_PATH_STMT = 'INSERT INTO `*PREFIX*dav_properties`'
-	. ' (`propertypath`, `propertyname`, `propertyvalue`) VALUES(?,?,?)';
+	. ' (`propertypath`, `propertyname`, `propertyvalue`, `propertytype`) VALUES(?,?,?,?)';
 	const UPDATE_BY_PATH_STMT = 'UPDATE `*PREFIX*dav_properties`'
 	. ' SET `propertypath` = ? WHERE `propertypath` = ?';
 	const UPDATE_BY_PATH_AND_NAME_STMT = 'UPDATE `*PREFIX*dav_properties` '
-	. 'SET `propertyvalue` = ? WHERE `propertypath` = ? AND `propertyname` = ?';
+	. 'SET `propertyvalue` = ?, `propertytype` = ? WHERE `propertypath` = ? AND `propertyname` = ?';
 	const DELETE_BY_PATH_STMT = 'DELETE FROM `*PREFIX*dav_properties` WHERE `propertypath` = ?';
 	const DELETE_BY_PATH_AND_NAME_STMT = 'DELETE FROM `*PREFIX*dav_properties`'
 	. ' WHERE `propertypath` = ? AND `propertyname` = ?';
@@ -110,9 +110,6 @@ protected function getProperties($path, INode $node, array $requestedProperties)
 	 */
 	protected function updateProperties($path, INode $node, $changedProperties) {
 		$existingProperties = $this->getProperties($path, $node, []);
-		$deleteStatement = self::DELETE_BY_PATH_AND_NAME_STMT;
-		$insertStatement = self::INSERT_BY_PATH_STMT;
-		$updateStatement = self::UPDATE_BY_PATH_AND_NAME_STMT;
 
 		// TODO: use "insert or update" strategy ?
 		$this->connection->beginTransaction();
@@ -121,26 +118,32 @@ protected function updateProperties($path, INode $node, $changedProperties) {
 			// If it was null, we need to delete the property
 			if ($propertyValue === null) {
 				if ($propertyExists) {
-					$this->connection->executeUpdate($deleteStatement,
+					$this->connection->executeUpdate(
+						self::DELETE_BY_PATH_AND_NAME_STMT,
 						[
 							$path,
 							$propertyName
 						]
 					);
 				}
 			} else {
+				$propertyData = $this->encodeValue($propertyValue);
 				if (!$propertyExists) {
-					$this->connection->executeUpdate($insertStatement,
+					$this->connection->executeUpdate(
+						self::INSERT_BY_PATH_STMT,
 						[
 							$path,
 							$propertyName,
-							$propertyValue
+							$propertyData['value'],
+							$propertyData['type']
 						]
 					);
 				} else {
-					$this->connection->executeUpdate($updateStatement,
+					$this->connection->executeUpdate(
+						self::UPDATE_BY_PATH_AND_NAME_STMT,
 						[
-							$propertyValue,
+							$propertyData['value'],
+							$propertyData['type'],
 							$path,
 							$propertyName
 						]

diff --git a/changelog/unreleased/37314 b/changelog/unreleased/37314
@@ -0,0 +1,8 @@
+Bugfix: Properly store complex Webdav properties
+
+Fixed: setting custom complex DAV property and reading it returned
+just an 'Object' string instead of the original property value.
+
+https://github.com/owncloud/core/pull/37314
+https://github.com/owncloud/core/issues/32670
+https://github.com/owncloud/core/issues/37027
diff --git a/tests/acceptance/features/apiWebdavProperties/setFileProperties.feature b/tests/acceptance/features/apiWebdavProperties/setFileProperties.feature
@@ -20,14 +20,12 @@ Feature: set file properties
       | old         |
       | new         |
 
-  @issue-32670
   Scenario Outline: Setting custom complex DAV property and reading it
     Given using <dav_version> DAV path
     And user "user0" has uploaded file "filesForUpload/textfile.txt" to "/testcustomprop.txt"
     And user "user0" has set property "very-custom-prop" with namespace "x1='http://whatever.org/ns'" of file "/testcustomprop.txt" to "<foo xmlns='http://bar'/>"
     When user "user0" gets a custom property "very-custom-prop" with namespace "x1='http://whatever.org/ns'" of file "/testcustomprop.txt"
-    Then the response should contain a custom "very-custom-prop" property with namespace "x1='http://whatever.org/ns'" and value "Object"
-    #Then the response should contain a custom "very-custom-prop" property with namespace "x1='http://whatever.org/ns'" and value "<foo xmlns='http://bar'/>"
+    Then the response should contain a custom "very-custom-prop" property with namespace "x1='http://whatever.org/ns'" and value "<foo xmlns='http://bar'/>"
     Examples:
       | dav_version |
       | old         |