Merge pull request #20018 from owncloud/cache_group_backend

The group database backend should cache groups

3 files changed, 213 insertions, 87 deletions

diff --git a/lib/private/group/database.php b/lib/private/group/database.php
index 63aadeb7ade..b769e69b4ba 100644
--- a/lib/private/group/database.php
+++ b/lib/private/group/database.php
@@ -50,6 +50,30 @@
  */
 class OC_Group_Database extends OC_Group_Backend {
 
+	/** @var string[] */
+	private $groupCache = [];
+
+	/** @var \OCP\IDBConnection */
+	private $dbConn;
+
+	/**
+	 * OC_Group_Database constructor.
+	 *
+	 * @param \OCP\IDBConnection|null $dbConn
+	 */
+	public function __construct(\OCP\IDBConnection $dbConn = null) {
+		$this->dbConn = $dbConn;
+	}
+
+	/**
+	 * FIXME: This function should not be required!
+	 */
+	private function fixDI() {
+		if ($this->dbConn === null) {
+			$this->dbConn = \OC::$server->getDatabaseConnection();
+		}
+	}
+
 	/**
 	 * Try to create a new group
 	 * @param string $gid The name of the group to create
@@ -59,21 +83,46 @@ class OC_Group_Database extends OC_Group_Backend {
 	 * be returned.
 	 */
 	public function createGroup( $gid ) {
-		// Check for existence
-		$stmt = OC_DB::prepare( "SELECT `gid` FROM `*PREFIX*groups` WHERE `gid` = ?" );
-		$result = $stmt->execute( array( $gid ));
+		$this->fixDI();
 
-		if( $result->fetchRow() ) {
-			// Can not add an existing group
+		// Check cache first
+		if (isset($this->groupCache[$gid])) {
 			return false;
+		} else {
+			// Check for existence in DB
+			$qb = $this->dbConn->getQueryBuilder();
+			$cursor = $qb->select('gid')
+				->from('groups')
+				->where($qb->expr()->eq('gid', $qb->createNamedParameter($gid)))
+				->execute();
+
+			$result = $cursor->fetch();
+			$cursor->closeCursor();
+
+			if($result) {
+				// Can not add an existing group
+
+				// Add to cache
+				$this->groupCache[$gid] = $gid;
+
+				return false;
+			}
 		}
-		else{
-			// Add group and exit
-			$stmt = OC_DB::prepare( "INSERT INTO `*PREFIX*groups` ( `gid` ) VALUES( ? )" );
-			$result = $stmt->execute( array( $gid ));
 
-			return $result ? true : false;
+		// Add group and exit
+		$qb = $this->dbConn->getQueryBuilder();
+		$result = $qb->insert('groups')
+			->setValue('gid', $qb->createNamedParameter($gid))
+			->execute();
+
+		if (!$result) {
+			return false;
 		}
+
+		// Add to cache
+		$this->groupCache[$gid] = $gid;
+
+		return true;
 	}
 
 	/**
@@ -84,17 +133,28 @@ class OC_Group_Database extends OC_Group_Backend {
 	 * Deletes a group and removes it from the group_user-table
 	 */
 	public function deleteGroup( $gid ) {
+		$this->fixDI();
+
 		// Delete the group
-		$stmt = OC_DB::prepare( "DELETE FROM `*PREFIX*groups` WHERE `gid` = ?" );
-		$stmt->execute( array( $gid ));
+		$qb = $this->dbConn->getQueryBuilder();
+		$qb->delete('groups')
+			->where($qb->expr()->eq('gid', $qb->createNamedParameter($gid)))
+			->execute();
 
 		// Delete the group-user relation
-		$stmt = OC_DB::prepare( "DELETE FROM `*PREFIX*group_user` WHERE `gid` = ?" );
-		$stmt->execute( array( $gid ));
+		$qb = $this->dbConn->getQueryBuilder();
+		$qb->delete('group_user')
+			->where($qb->expr()->eq('gid', $qb->createNamedParameter($gid)))
+			->execute();
 
 		// Delete the group-groupadmin relation
-		$stmt = OC_DB::prepare( "DELETE FROM `*PREFIX*group_admin` WHERE `gid` = ?" );
-		$stmt->execute( array( $gid ));
+		$qb = $this->dbConn->getQueryBuilder();
+		$qb->delete('group_admin')
+			->where($qb->expr()->eq('gid', $qb->createNamedParameter($gid)))
+			->execute();
+
+		// Delete from cache
+		unset($this->groupCache[$gid]);
 
 		return true;
 	}
@@ -108,11 +168,20 @@ class OC_Group_Database extends OC_Group_Backend {
 	 * Checks whether the user is member of a group or not.
 	 */
 	public function inGroup( $uid, $gid ) {
+		$this->fixDI();
+
 		// check
-		$stmt = OC_DB::prepare( "SELECT `uid` FROM `*PREFIX*group_user` WHERE `gid` = ? AND `uid` = ?" );
-		$result = $stmt->execute( array( $gid, $uid ));
+		$qb = $this->dbConn->getQueryBuilder();
+		$cursor = $qb->select('uid')
+			->from('group_user')
+			->where($qb->expr()->eq('gid', $qb->createNamedParameter($gid)))
+			->andWhere($qb->expr()->eq('uid', $qb->createNamedParameter($uid)))
+			->execute();
 
-		return $result->fetchRow() ? true : false;
+		$result = $cursor->fetch();
+		$cursor->closeCursor();
+
+		return $result ? true : false;
 	}
 
 	/**
@@ -124,10 +193,15 @@ class OC_Group_Database extends OC_Group_Backend {
 	 * Adds a user to a group.
 	 */
 	public function addToGroup( $uid, $gid ) {
+		$this->fixDI();
+
 		// No duplicate entries!
 		if( !$this->inGroup( $uid, $gid )) {
-			$stmt = OC_DB::prepare( "INSERT INTO `*PREFIX*group_user` ( `uid`, `gid` ) VALUES( ?, ? )" );
-			$stmt->execute( array( $uid, $gid ));
+			$qb = $this->dbConn->getQueryBuilder();
+			$qb->insert('group_user')
+				->setValue('uid', $qb->createNamedParameter($uid))
+				->setValue('gid', $qb->createNamedParameter($gid))
+				->execute();
 			return true;
 		}else{
 			return false;
@@ -143,8 +217,13 @@ class OC_Group_Database extends OC_Group_Backend {
 	 * removes the user from a group.
 	 */
 	public function removeFromGroup( $uid, $gid ) {
-		$stmt = OC_DB::prepare( "DELETE FROM `*PREFIX*group_user` WHERE `uid` = ? AND `gid` = ?" );
-		$stmt->execute( array( $uid, $gid ));
+		$this->fixDI();
+
+		$qb = $this->dbConn->getQueryBuilder();
+		$qb->delete('group_user')
+			->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid)))
+			->andWhere($qb->expr()->eq('gid', $qb->createNamedParameter($gid)))
+			->execute();
 
 		return true;
 	}
@@ -158,14 +237,21 @@ class OC_Group_Database extends OC_Group_Backend {
 	 * if the user exists at all.
 	 */
 	public function getUserGroups( $uid ) {
+		$this->fixDI();
+
 		// No magic!
-		$stmt = OC_DB::prepare( "SELECT `gid` FROM `*PREFIX*group_user` WHERE `uid` = ?" );
-		$result = $stmt->execute( array( $uid ));
+		$qb = $this->dbConn->getQueryBuilder();
+		$cursor = $qb->select('gid')
+			->from('group_user')
+			->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid)))
+			->execute();
 
-		$groups = array();
-		while( $row = $result->fetchRow()) {
+		$groups = [];
+		while( $row = $cursor->fetch()) {
 			$groups[] = $row["gid"];
+			$this->groupCache[$row['gid']] = $row['gid'];
 		}
+		$cursor->closeCursor();
 
 		return $groups;
 	}
@@ -202,9 +288,23 @@ class OC_Group_Database extends OC_Group_Backend {
 	 * @return bool
 	 */
 	public function groupExists($gid) {
-		$query = OC_DB::prepare('SELECT `gid` FROM `*PREFIX*groups` WHERE `gid` = ?');
-		$result = $query->execute(array($gid))->fetchOne();
+		$this->fixDI();
+
+		// Check cache first
+		if (isset($this->groupCache[$gid])) {
+			return true;
+		}
+
+		$qb = $this->dbConn->getQueryBuilder();
+		$cursor = $qb->select('gid')
+			->from('groups')
+			->where($qb->expr()->eq('gid', $qb->createNamedParameter($gid)))
+			->execute();
+		$result = $cursor->fetch();
+		$cursor->closeCursor();
+
 		if ($result !== false) {
+			$this->groupCache[$gid] = $gid;
 			return true;
 		}
 		return false;
diff --git a/tests/lib/group/backend.php b/tests/lib/group/backend.php
index ce41a6c6359..238b83de5d7 100644
--- a/tests/lib/group/backend.php
+++ b/tests/lib/group/backend.php
@@ -1,24 +1,28 @@
 <?php
 /**
-* ownCloud
-*
-* @author Robin Appelman
-* @copyright 2012 Robin Appelman icewind@owncloud.com
-*
-* This library is free software; you can redistribute it and/or
-* modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE
-* License as published by the Free Software Foundation; either
-* version 3 of the License, or any later version.
-*
-* This library 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 along with this library.  If not, see <http://www.gnu.org/licenses/>.
-*
-*/
+ * @author Arthur Schiwon <blizzz@owncloud.com>
+ * @author Felix Moeller <mail@felixmoeller.de>
+ * @author Joas Schilling <nickvergessen@owncloud.com>
+ * @author Robin Appelman <icewind@owncloud.com>
+ * @author Scrutinizer Auto-Fixer <auto-fixer@scrutinizer-ci.com>
+ * @author Thomas Müller <thomas.mueller@tmit.eu>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @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/>
+ *
+ */
 
 /**
  * Class Test_Group_Backend
@@ -34,10 +38,11 @@ abstract class Test_Group_Backend extends \Test\TestCase {
 	/**
 	 * get a new unique group name
 	 * test cases can override this in order to clean up created groups
+	 *
 	 * @return string
 	 */
 	public function getGroupName($name = null) {
-		if(is_null($name)) {
+		if (is_null($name)) {
 			return $this->getUniqueID('test_');
 		} else {
 			return $name;
@@ -47,6 +52,7 @@ abstract class Test_Group_Backend extends \Test\TestCase {
 	/**
 	 * get a new unique user name
 	 * test cases can override this in order to clean up created user
+	 *
 	 * @return string
 	 */
 	public function getUserName() {
@@ -55,36 +61,36 @@ abstract class Test_Group_Backend extends \Test\TestCase {
 
 	public function testAddRemove() {
 		//get the number of groups we start with, in case there are exising groups
-		$startCount=count($this->backend->getGroups());
+		$startCount = count($this->backend->getGroups());
 
-		$name1=$this->getGroupName();
-		$name2=$this->getGroupName();
+		$name1 = $this->getGroupName();
+		$name2 = $this->getGroupName();
 		$this->backend->createGroup($name1);
-		$count=count($this->backend->getGroups())-$startCount;
+		$count = count($this->backend->getGroups()) - $startCount;
 		$this->assertEquals(1, $count);
-		$this->assertTrue((array_search($name1, $this->backend->getGroups())!==false));
-		$this->assertFalse((array_search($name2, $this->backend->getGroups())!==false));
+		$this->assertTrue((array_search($name1, $this->backend->getGroups()) !== false));
+		$this->assertFalse((array_search($name2, $this->backend->getGroups()) !== false));
 		$this->backend->createGroup($name2);
-		$count=count($this->backend->getGroups())-$startCount;
+		$count = count($this->backend->getGroups()) - $startCount;
 		$this->assertEquals(2, $count);
-		$this->assertTrue((array_search($name1, $this->backend->getGroups())!==false));
-		$this->assertTrue((array_search($name2, $this->backend->getGroups())!==false));
+		$this->assertTrue((array_search($name1, $this->backend->getGroups()) !== false));
+		$this->assertTrue((array_search($name2, $this->backend->getGroups()) !== false));
 
 		$this->backend->deleteGroup($name2);
-		$count=count($this->backend->getGroups())-$startCount;
+		$count = count($this->backend->getGroups()) - $startCount;
 		$this->assertEquals(1, $count);
-		$this->assertTrue((array_search($name1, $this->backend->getGroups())!==false));
-		$this->assertFalse((array_search($name2, $this->backend->getGroups())!==false));
+		$this->assertTrue((array_search($name1, $this->backend->getGroups()) !== false));
+		$this->assertFalse((array_search($name2, $this->backend->getGroups()) !== false));
 	}
 
 	public function testUser() {
-		$group1=$this->getGroupName();
-		$group2=$this->getGroupName();
+		$group1 = $this->getGroupName();
+		$group2 = $this->getGroupName();
 		$this->backend->createGroup($group1);
 		$this->backend->createGroup($group2);
 
-		$user1=$this->getUserName();
-		$user2=$this->getUserName();
+		$user1 = $this->getUserName();
+		$user2 = $this->getUserName();
 
 		$this->assertFalse($this->backend->inGroup($user1, $group1));
 		$this->assertFalse($this->backend->inGroup($user2, $group1));
@@ -143,4 +149,11 @@ abstract class Test_Group_Backend extends \Test\TestCase {
 		$result = $this->backend->countUsersInGroup($group, 'bar');
 		$this->assertSame(2, $result);
 	}
+
+	public function testAddDouble() {
+		$group = $this->getGroupName();
+
+		$this->backend->createGroup($group);
+		$this->backend->createGroup($group);
+	}
 }
diff --git a/tests/lib/group/database.php b/tests/lib/group/database.php
index 3997ff8bba5..b0be5774c7d 100644
--- a/tests/lib/group/database.php
+++ b/tests/lib/group/database.php
@@ -1,24 +1,27 @@
 <?php
 /**
-* ownCloud
-*
-* @author Robin Appelman
-* @copyright 2012 Robin Appelman icewind@owncloud.com
-*
-* This library is free software; you can redistribute it and/or
-* modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE
-* License as published by the Free Software Foundation; either
-* version 3 of the License, or any later version.
-*
-* This library 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 along with this library.  If not, see <http://www.gnu.org/licenses/>.
-*
-*/
+ * @author Arthur Schiwon <blizzz@owncloud.com>
+ * @author Joas Schilling <nickvergessen@owncloud.com>
+ * @author Robin Appelman <icewind@owncloud.com>
+ * @author Scrutinizer Auto-Fixer <auto-fixer@scrutinizer-ci.com>
+ * @author Thomas Müller <thomas.mueller@tmit.eu>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @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/>
+ *
+ */
 
 /**
  * Class Test_Group_Database
@@ -26,11 +29,12 @@
  * @group DB
  */
 class Test_Group_Database extends Test_Group_Backend {
-	private $groups=array();
+	private $groups = array();
 
 	/**
 	 * get a new unique group name
 	 * test cases can override this in order to clean up created groups
+	 *
 	 * @return string
 	 */
 	public function getGroupName($name = null) {
@@ -41,13 +45,22 @@ class Test_Group_Database extends Test_Group_Backend {
 
 	protected function setUp() {
 		parent::setUp();
-		$this->backend=new OC_Group_Database();
+		$this->backend = new OC_Group_Database();
 	}
 
 	protected function tearDown() {
-		foreach($this->groups as $group) {
+		foreach ($this->groups as $group) {
 			$this->backend->deleteGroup($group);
 		}
 		parent::tearDown();
 	}
+
+	public function testAddDoubleNoCache() {
+		$group = $this->getGroupName();
+
+		$this->backend->createGroup($group);
+
+		$backend = new OC_Group_Database();
+		$this->assertFalse($backend->createGroup($group));
+	}
 }