FileCustomPropertiesBackend::loadChildrenProperties() - Code Metrics - Inspection of "fixed exception, if $node is type SimpleCollection" - owncloud/core - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — master (#29292)

by Victor

created 2017-10-19 12:54 UTC

loadChildrenProperties() B

↳ Parent: FileCustomPropertiesBackend

Complexity

Conditions	6
Paths	8

Size

Total Lines	41
Code Lines	24

Duplication

Lines	0
Ratio	0 %

Importance

Changes

Metric	Value
cc	6
eloc	24
nc	8
nop	2
dl	0
loc	41
rs	8.439
c	0
b	0
f	0

<?php
/**
 * @author Thomas Müller <[email protected]>
 * @author Vincent Petry <[email protected]>
 * @author Viktar Dubiniuk <[email protected]>
 *
 * @copyright Copyright (c) 2017, 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\DAV;

use Doctrine\DBAL\Connection;
use OCA\DAV\Connector\Sabre\Directory;
use OCA\DAV\Connector\Sabre\Node;
use Sabre\DAV\INode;

/**
 * Class FileCustomPropertiesBackend
 *
 * Provides ability to store/retrieve custom file properties via DAV
 * into oc_properties DB table using fileId as a reference to the file
 *
 * @package OCA\DAV\DAV
 */
class FileCustomPropertiesBackend extends AbstractCustomPropertiesBackend {

	const SELECT_BY_ID_STMT = 'SELECT * FROM `*PREFIX*properties` WHERE `fileid` = ?';
	const INSERT_BY_ID_STMT = 'INSERT INTO `*PREFIX*properties`'
	. ' (`fileid`,`propertyname`,`propertyvalue`) VALUES(?,?,?)';
	const UPDATE_BY_ID_AND_NAME_STMT = 'UPDATE `*PREFIX*properties`'
	. ' SET `propertyvalue` = ? WHERE `fileid` = ? AND `propertyname` = ?';
	const DELETE_BY_ID_STMT = 'DELETE FROM `*PREFIX*properties` WHERE `fileid` = ?';
	const DELETE_BY_ID_AND_NAME_STMT = 'DELETE FROM `*PREFIX*properties`'
	. ' WHERE `fileid` = ? AND `propertyname` = ?';

	/**
	 * This method is called after a node is deleted.
	 *
	 * @param string $path path of node for which to delete properties
	 */
	public function delete($path) {
		$node = $this->getNodeForPath($path);
		if (is_null($node)) {
			return;
		}

		$fileId = $node->getId();
		$statement = $this->connection->prepare(self::DELETE_BY_ID_STMT);
		$statement->execute([$fileId]);
		$this->offsetUnset($fileId);
		$statement->closeCursor();
	}

	/**
	 * This method is called after a successful MOVE
	 *
	 * @param string $source
	 * @param string $destination
	 *
	 * @return void
	 */
	public function move($source, $destination) {
		// Part of interface. We don't care about move because it doesn't affect fileId
	}

	/**
	 * @inheritdoc
	 */
	protected function getProperties($path, INode $node, array $requestedProperties) {
		$fileId = $node->getId();
interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}
		if (is_null($this->offsetGet($fileId))) {
			// TODO: chunking if more than 1000 properties
			$sql = self::SELECT_BY_ID_STMT;
			$whereValues = [$fileId];
			$whereTypes = [null];

			if (!empty($requestedProperties)) {
				// request only a subset
				$sql .= ' AND `propertyname` in (?)';
				$whereValues[] = $requestedProperties;
				$whereTypes[] = Connection::PARAM_STR_ARRAY;
			}

			$props = $this->fetchProperties($sql, $whereValues, $whereTypes);
			$this->offsetSet($fileId, $props);
		}
		return $this->offsetGet($fileId);
	}

	/**
	 * @inheritdoc
	 */
	protected function updateProperties($path, INode $node, $changedProperties) {
		$existingProperties = $this->getProperties($path, $node, []);
		$fileId = $node->getId();
interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}
		$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();
		foreach ($changedProperties as $propertyName => $propertyValue) {
			$propertyExists = array_key_exists($propertyName, $existingProperties);
			// If it was null, we need to delete the property
			if (is_null($propertyValue)) {
				if ($propertyExists) {
					$this->connection->executeUpdate($deleteStatement,
						[
							$fileId,
							$propertyName
						]
					);
				}
			} else {
				if (!$propertyExists) {
					$this->connection->executeUpdate($insertStatement,
						[
							$fileId,
							$propertyName,
							$propertyValue
						]
					);
				} else {
					$this->connection->executeUpdate($updateStatement,
						[
							$propertyValue,
							$fileId,
							$propertyName
						]
					);
				}
			}
		}

		$this->connection->commit();
		$this->offsetUnset($fileId);

		return true;
	}

	/**
	 * Bulk load properties for directory children
	 *
	 * @param INode $node
	 * @param array $requestedProperties requested properties
	 *
	 * @return void
	 */
	protected function loadChildrenProperties(INode $node, $requestedProperties) {
		// note: pre-fetching only supported for depth <= 1
		if (!($node instanceof Directory)){
			return;
		}

		$fileId = $node->getId();
		if (!is_null($this->offsetGet($fileId))) {
			// we already loaded them at some point
			return;
		}

		$childNodes = $node->getChildren();
		$childrenIds = [];
		// pre-fill cache
		foreach ($childNodes as $childNode) {
			$childId = $childNode->getId();
interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}
			if ($childId) {
				$childrenIds[] = $childId;
				$this->offsetSet($childId, []);
			}
		}

		// TODO: use query builder
		$sql = 'SELECT * FROM `*PREFIX*properties` WHERE `fileid` IN (?)';
		$sql .= ' AND `propertyname` in (?) ORDER BY `propertyname`';

		$result = $this->connection->executeQuery(
			$sql,
			[$childrenIds, $requestedProperties],
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
			[Connection::PARAM_STR_ARRAY, Connection::PARAM_STR_ARRAY]
		);

		$props = [];
		while ($row = $result->fetch()) {
			$props[$row['propertyname']] = $row['propertyvalue'];
			$this->offsetSet($row['fileid'], $props);
		}

		$result->closeCursor();
	}

	/**
	 * @param string $path
	 * @return INode|null
	 */
	protected function getNodeForPath($path){
		$node = parent::getNodeForPath($path);
		if (!$node instanceof Node) {
			return null;
		}
		return $node;
	}

}


Pull Request — master (#29292)

loadChildrenProperties() B

Complexity

Size

Duplication

Importance

Available Fixes

Available Fixes

Available Fixes

1		<?php
2		/**
3		* @author Thomas Müller <[email protected]>
4		* @author Vincent Petry <[email protected]>
5		* @author Viktar Dubiniuk <[email protected]>
6		*
7		* @copyright Copyright (c) 2017, ownCloud GmbH
8		* @license AGPL-3.0
9		*
10		* This code is free software: you can redistribute it and/or modify
11		* it under the terms of the GNU Affero General Public License, version 3,
12		* as published by the Free Software Foundation.
13		*
14		* This program is distributed in the hope that it will be useful,
15		* but WITHOUT ANY WARRANTY; without even the implied warranty of
16		* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
17		* GNU Affero General Public License for more details.
18		*
19		* You should have received a copy of the GNU Affero General Public License, version 3,
20		* along with this program. If not, see <http://www.gnu.org/licenses/>
21		*
22		*/
23
24		namespace OCA\DAV\DAV;
25
26		use Doctrine\DBAL\Connection;
27		use OCA\DAV\Connector\Sabre\Directory;
28		use OCA\DAV\Connector\Sabre\Node;
29		use Sabre\DAV\INode;
30
31		/**
32		* Class FileCustomPropertiesBackend
33		*
34		* Provides ability to store/retrieve custom file properties via DAV
35		* into oc_properties DB table using fileId as a reference to the file
36		*
37		* @package OCA\DAV\DAV
38		*/
39		class FileCustomPropertiesBackend extends AbstractCustomPropertiesBackend {
40
41		const SELECT_BY_ID_STMT = 'SELECT * FROM `PREFIXproperties` WHERE `fileid` = ?';
42		const INSERT_BY_ID_STMT = 'INSERT INTO `PREFIXproperties`'
43		. ' (`fileid`,`propertyname`,`propertyvalue`) VALUES(?,?,?)';
44		const UPDATE_BY_ID_AND_NAME_STMT = 'UPDATE `PREFIXproperties`'
45		. ' SET `propertyvalue` = ? WHERE `fileid` = ? AND `propertyname` = ?';
46		const DELETE_BY_ID_STMT = 'DELETE FROM `PREFIXproperties` WHERE `fileid` = ?';
47		const DELETE_BY_ID_AND_NAME_STMT = 'DELETE FROM `PREFIXproperties`'
48		. ' WHERE `fileid` = ? AND `propertyname` = ?';
49
50		/**
51		* This method is called after a node is deleted.
52		*
53		* @param string $path path of node for which to delete properties
54		*/
55	View Code Duplication	public function delete($path) {
56		$node = $this->getNodeForPath($path);
57		if (is_null($node)) {
58		return;
59		}
60
61		$fileId = $node->getId();
62		$statement = $this->connection->prepare(self::DELETE_BY_ID_STMT);
63		$statement->execute([$fileId]);
64		$this->offsetUnset($fileId);
65		$statement->closeCursor();
66		}
67
68		/**
69		* This method is called after a successful MOVE
70		*
71		* @param string $source
72		* @param string $destination
73		*
74		* @return void
75		*/
76		public function move($source, $destination) {
77		// Part of interface. We don't care about move because it doesn't affect fileId
78		}
79
80		/**
81		* @inheritdoc
82		*/
83	View Code Duplication	protected function getProperties($path, INode $node, array $requestedProperties) {
84		$fileId = $node->getId();
		0 ignored issues – show Bug introduced 2017-10-10 20:42 UTC by Report Bug Copy Issue Report It seems like you code against a concrete implementation and not the interface `Sabre\DAV\INode` as the method `getId()` does only exist in the following implementations of said interface: `OCA\Comments\Dav\EntityCollection`, `OCA\DAV\Connector\Sabre\Directory`, `OCA\DAV\Connector\Sabre\File`, `OCA\DAV\Connector\Sabre\Node`, `OCA\DAV\Files\FilesHome`. Let’s take a look at an example: interface User { /** @return string / public function getPassword(); } class MyUser implements User { public function getPassword() { // return something } public function getDisplayName() { // return some name. } } class AuthSystem { public function authenticate(User $user) { $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName())); // do something. } } In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different implementation of User which does not have a getDisplayName() method, the code will break. Available Fixes Change the type-hint for the parameter: class AuthSystem { public function authenticate(MyUser $user) { / ... / } } Add an additional type-check: class AuthSystem { public function authenticate(User $user) { if ($user instanceof MyUser) { $this->logger->info(/* ... /); } // or alternatively if ( ! $user instanceof MyUser) { throw new \LogicException( '$user must be an instance of MyUser, ' .'other instances are not supported.' ); } } } Note:* PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case. Add the method to the interface: interface User { /** @return string / public function getPassword(); /* @return string */ public function getDisplayName(); } Loading history...
85		if (is_null($this->offsetGet($fileId))) {
86		// TODO: chunking if more than 1000 properties
87		$sql = self::SELECT_BY_ID_STMT;
88		$whereValues = [$fileId];
89		$whereTypes = [null];
90
91		if (!empty($requestedProperties)) {
92		// request only a subset
93		$sql .= ' AND `propertyname` in (?)';
94		$whereValues[] = $requestedProperties;
95		$whereTypes[] = Connection::PARAM_STR_ARRAY;
96		}
97
98		$props = $this->fetchProperties($sql, $whereValues, $whereTypes);
99		$this->offsetSet($fileId, $props);
100		}
101		return $this->offsetGet($fileId);
102		}
103
104		/**
105		* @inheritdoc
106		*/
107	View Code Duplication	protected function updateProperties($path, INode $node, $changedProperties) {
108		$existingProperties = $this->getProperties($path, $node, []);
109		$fileId = $node->getId();
		0 ignored issues – show Bug introduced 2017-10-10 20:42 UTC by Report Bug Copy Issue Report It seems like you code against a concrete implementation and not the interface `Sabre\DAV\INode` as the method `getId()` does only exist in the following implementations of said interface: `OCA\Comments\Dav\EntityCollection`, `OCA\DAV\Connector\Sabre\Directory`, `OCA\DAV\Connector\Sabre\File`, `OCA\DAV\Connector\Sabre\Node`, `OCA\DAV\Files\FilesHome`. Let’s take a look at an example: interface User { /** @return string / public function getPassword(); } class MyUser implements User { public function getPassword() { // return something } public function getDisplayName() { // return some name. } } class AuthSystem { public function authenticate(User $user) { $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName())); // do something. } } In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different implementation of User which does not have a getDisplayName() method, the code will break. Available Fixes Change the type-hint for the parameter: class AuthSystem { public function authenticate(MyUser $user) { / ... / } } Add an additional type-check: class AuthSystem { public function authenticate(User $user) { if ($user instanceof MyUser) { $this->logger->info(/* ... /); } // or alternatively if ( ! $user instanceof MyUser) { throw new \LogicException( '$user must be an instance of MyUser, ' .'other instances are not supported.' ); } } } Note:* PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case. Add the method to the interface: interface User { /** @return string / public function getPassword(); /* @return string */ public function getDisplayName(); } Loading history...
110		$deleteStatement = self::DELETE_BY_ID_AND_NAME_STMT;
111		$insertStatement = self::INSERT_BY_ID_STMT;
112		$updateStatement = self::UPDATE_BY_ID_AND_NAME_STMT;
113
114		// TODO: use "insert or update" strategy ?
115		$this->connection->beginTransaction();
116		foreach ($changedProperties as $propertyName => $propertyValue) {
117		$propertyExists = array_key_exists($propertyName, $existingProperties);
118		// If it was null, we need to delete the property
119		if (is_null($propertyValue)) {
120		if ($propertyExists) {
121		$this->connection->executeUpdate($deleteStatement,
122		[
123		$fileId,
124		$propertyName
125		]
126		);
127		}
128		} else {
129		if (!$propertyExists) {
130		$this->connection->executeUpdate($insertStatement,
131		[
132		$fileId,
133		$propertyName,
134		$propertyValue
135		]
136		);
137		} else {
138		$this->connection->executeUpdate($updateStatement,
139		[
140		$propertyValue,
141		$fileId,
142		$propertyName
143		]
144		);
145		}
146		}
147		}
148
149		$this->connection->commit();
150		$this->offsetUnset($fileId);
151
152		return true;
153		}
154
155		/**
156		* Bulk load properties for directory children
157		*
158		* @param INode $node
159		* @param array $requestedProperties requested properties
160		*
161		* @return void
162		*/
163		protected function loadChildrenProperties(INode $node, $requestedProperties) {
164		// note: pre-fetching only supported for depth <= 1
165		if (!($node instanceof Directory)){
166		return;
167		}
168
169		$fileId = $node->getId();
170		if (!is_null($this->offsetGet($fileId))) {
171		// we already loaded them at some point
172		return;
173		}
174
175		$childNodes = $node->getChildren();
176		$childrenIds = [];
177		// pre-fill cache
178		foreach ($childNodes as $childNode) {
179		$childId = $childNode->getId();
		0 ignored issues – show Bug introduced 2017-04-17 10:13 UTC by Report Bug Copy Issue Report It seems like you code against a concrete implementation and not the interface `Sabre\DAV\INode` as the method `getId()` does only exist in the following implementations of said interface: `OCA\Comments\Dav\EntityCollection`, `OCA\DAV\Connector\Sabre\Directory`, `OCA\DAV\Connector\Sabre\File`, `OCA\DAV\Connector\Sabre\Node`, `OCA\DAV\Files\FilesHome`. Let’s take a look at an example: interface User { /** @return string / public function getPassword(); } class MyUser implements User { public function getPassword() { // return something } public function getDisplayName() { // return some name. } } class AuthSystem { public function authenticate(User $user) { $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName())); // do something. } } In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different implementation of User which does not have a getDisplayName() method, the code will break. Available Fixes Change the type-hint for the parameter: class AuthSystem { public function authenticate(MyUser $user) { / ... / } } Add an additional type-check: class AuthSystem { public function authenticate(User $user) { if ($user instanceof MyUser) { $this->logger->info(/* ... /); } // or alternatively if ( ! $user instanceof MyUser) { throw new \LogicException( '$user must be an instance of MyUser, ' .'other instances are not supported.' ); } } } Note:* PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case. Add the method to the interface: interface User { /** @return string / public function getPassword(); /* @return string */ public function getDisplayName(); } Loading history...
180		if ($childId) {
181		$childrenIds[] = $childId;
182		$this->offsetSet($childId, []);
183		}
184		}
185
186		// TODO: use query builder
187		$sql = 'SELECT * FROM `PREFIXproperties` WHERE `fileid` IN (?)';
188		$sql .= ' AND `propertyname` in (?) ORDER BY `propertyname`';
189
190		$result = $this->connection->executeQuery(
191		$sql,
192		[$childrenIds, $requestedProperties],
		0 ignored issues – show Documentation introduced 2017-04-17 10:13 UTC by Report Bug Copy Issue Report `array($childrenIds, $requestedProperties)` is of type `array<integer,array,{"0":"array","1":"array"}>`, but the function expects a `array<integer,string>`. It seems like the type of the argument is not accepted by the function/method which you are calling. In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug. We suggest to add an explicit type cast like in the following example: function acceptsInteger($int) { } $x = '123'; // string "123" // Instead of acceptsInteger($x); // we recommend to use acceptsInteger((integer) $x); Loading history...
193		[Connection::PARAM_STR_ARRAY, Connection::PARAM_STR_ARRAY]
194		);
195
196		$props = [];
197		while ($row = $result->fetch()) {
198		$props[$row['propertyname']] = $row['propertyvalue'];
199		$this->offsetSet($row['fileid'], $props);
200		}
201
202		$result->closeCursor();
203		}
204
205		/**
206		* @param string $path
207		* @return INode\|null
208		*/
209		protected function getNodeForPath($path){
210		$node = parent::getNodeForPath($path);
211		if (!$node instanceof Node) {
212		return null;
213		}
214		return $node;
215		}
216
217		}
218

owncloud / core

Pull Request — master (#29292)

loadChildrenProperties() B

Complexity

Size

Duplication

Importance

Available Fixes

Available Fixes

Available Fixes

Duplication Side-by-Side

Filter issues like