diff --git a/apps/dav/appinfo/Migrations/Version20200427142541.php b/apps/dav/appinfo/Migrations/Version20200427142541.php new file mode 100644 index 000000000000..2b5162d018fd --- /dev/null +++ b/apps/dav/appinfo/Migrations/Version20200427142541.php @@ -0,0 +1,56 @@ + + * + * @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 + * + */ + +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 index eeceaedcd6fc..b95918f94fc2 100644 --- 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 index ab62e5a3f7d3..72410151140a 100644 --- 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,7 +176,8 @@ 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 @@ -187,24 +185,23 @@ protected function updateProperties($path, INode $node, $changedProperties) { ); } } 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 index e0706f84c203..f210fd50eab5 100644 --- 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,7 +118,8 @@ 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 @@ -129,18 +127,23 @@ protected function updateProperties($path, INode $node, $changedProperties) { ); } } 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 new file mode 100644 index 000000000000..71c05b41b2d4 --- /dev/null +++ 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 index 1219a905deaf..f76b26f3ed51 100644 --- 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 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 "" 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 "" + Then the response should contain a custom "very-custom-prop" property with namespace "x1='http://whatever.org/ns'" and value "" Examples: | dav_version | | old |