From 53e37804ec67779a3582ce3ce1ad964dcf311bda Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Sun, 25 Oct 2015 09:45:51 +0100 Subject: The group database backend should cache groups This avoids duplicated queries like first checking the group_users db and then just doing a select on the group db. Those enries are linked (and should be using foreign keys!) This commit makes sure we cache those entries. If a user is part of N groups this saves N queries on webdav access --- lib/private/group/database.php | 51 ++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/lib/private/group/database.php b/lib/private/group/database.php index ad6174808bb..f1aa72d2c5f 100644 --- a/lib/private/group/database.php +++ b/lib/private/group/database.php @@ -50,6 +50,9 @@ */ class OC_Group_Database extends OC_Group_Backend { + /** @var string[] */ + private $groupCache = []; + /** * Try to create a new group * @param string $gid The name of the group to create @@ -59,21 +62,36 @@ 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 )); - - 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 + $stmt = OC_DB::prepare( "SELECT `gid` FROM `*PREFIX*groups` WHERE `gid` = ?" ); + $result = $stmt->execute( [$gid] ); + + if( $result->fetchRow() ) { + // 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 + $stmt = OC_DB::prepare( "INSERT INTO `*PREFIX*groups` ( `gid` ) VALUES( ? )" ); + $result = $stmt->execute( [$gid] ); + + if (!$result) { + return false; } + + // Add to cache + $this->groupCache[$gid] = $gid; + + return true; } /** @@ -96,6 +114,9 @@ class OC_Group_Database extends OC_Group_Backend { $stmt = OC_DB::prepare( "DELETE FROM `*PREFIX*group_admin` WHERE `gid` = ?" ); $stmt->execute( array( $gid )); + // Delete from cache + unset($this->groupCache[$gid]); + return true; } @@ -162,9 +183,10 @@ class OC_Group_Database extends OC_Group_Backend { $stmt = OC_DB::prepare( "SELECT `gid` FROM `*PREFIX*group_user` WHERE `uid` = ?" ); $result = $stmt->execute( array( $uid )); - $groups = array(); + $groups = []; while( $row = $result->fetchRow()) { $groups[] = $row["gid"]; + $this->groupCache[$row['gid']] = $row['gid']; } return $groups; @@ -202,6 +224,11 @@ class OC_Group_Database extends OC_Group_Backend { * @return bool */ public function groupExists($gid) { + // Check cache first + if (isset($this->groupCache[$gid])) { + return true; + } + $query = OC_DB::prepare('SELECT `gid` FROM `*PREFIX*groups` WHERE `gid` = ?'); $result = $query->execute(array($gid))->fetchOne(); if ($result !== false) { -- cgit v1.2.3 From 3c8f4784e9ee7ea8e7fe8eae3ab91031404619c3 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 30 Dec 2015 14:16:51 +0100 Subject: Inject DBConnection * Use query builder * Minor unit tests additions --- lib/private/group/database.php | 113 ++++++++++++++++++++++++++++++++--------- tests/lib/group/backend.php | 85 ++++++++++++++++++------------- tests/lib/group/database.php | 57 +++++++++++++-------- 3 files changed, 173 insertions(+), 82 deletions(-) diff --git a/lib/private/group/database.php b/lib/private/group/database.php index f1aa72d2c5f..23d63234a4f 100644 --- a/lib/private/group/database.php +++ b/lib/private/group/database.php @@ -53,6 +53,27 @@ 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 @@ -62,15 +83,20 @@ class OC_Group_Database extends OC_Group_Backend { * be returned. */ public function createGroup( $gid ) { + $this->fixDI(); + // Check cache first if (isset($this->groupCache[$gid])) { return false; } else { // Check for existence in DB - $stmt = OC_DB::prepare( "SELECT `gid` FROM `*PREFIX*groups` WHERE `gid` = ?" ); - $result = $stmt->execute( [$gid] ); + $qb = $this->dbConn->getQueryBuilder(); + $result = $qb->select('gid') + ->from('groups') + ->where($qb->expr()->eq('gid', $qb->createNamedParameter($gid))) + ->execute(); - if( $result->fetchRow() ) { + if( $result->fetch() ) { // Can not add an existing group // Add to cache @@ -81,8 +107,10 @@ class OC_Group_Database extends OC_Group_Backend { } // Add group and exit - $stmt = OC_DB::prepare( "INSERT INTO `*PREFIX*groups` ( `gid` ) VALUES( ? )" ); - $result = $stmt->execute( [$gid] ); + $qb = $this->dbConn->getQueryBuilder(); + $result = $qb->insert('groups') + ->setValue('gid', $qb->createNamedParameter($gid)) + ->execute(); if (!$result) { return false; @@ -102,17 +130,25 @@ 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]); @@ -129,11 +165,17 @@ class OC_Group_Database extends OC_Group_Backend { * Checks whether the user is member of a group or not. */ public function inGroup( $uid, $gid ) { - // check - $stmt = OC_DB::prepare( "SELECT `uid` FROM `*PREFIX*group_user` WHERE `gid` = ? AND `uid` = ?" ); - $result = $stmt->execute( array( $gid, $uid )); + $this->fixDI(); - return $result->fetchRow() ? true : false; + // check + $qb = $this->dbConn->getQueryBuilder(); + $result = $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->fetch() ? true : false; } /** @@ -145,10 +187,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; @@ -164,8 +211,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; } @@ -179,12 +231,17 @@ 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(); + $result = $qb->select('gid') + ->from('group_user') + ->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid))) + ->execute(); $groups = []; - while( $row = $result->fetchRow()) { + while( $row = $result->fetch()) { $groups[] = $row["gid"]; $this->groupCache[$row['gid']] = $row['gid']; } @@ -224,13 +281,21 @@ class OC_Group_Database extends OC_Group_Backend { * @return bool */ public function groupExists($gid) { + $this->fixDI(); + // Check cache first if (isset($this->groupCache[$gid])) { return true; } - $query = OC_DB::prepare('SELECT `gid` FROM `*PREFIX*groups` WHERE `gid` = ?'); - $result = $query->execute(array($gid))->fetchOne(); + $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) { return true; } 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 @@ . -* -*/ + * @author Arthur Schiwon + * @author Felix Moeller + * @author Joas Schilling + * @author Robin Appelman + * @author Scrutinizer Auto-Fixer + * @author Thomas Müller + * + * @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 + * + */ /** * 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 @@ . -* -*/ + * @author Arthur Schiwon + * @author Joas Schilling + * @author Robin Appelman + * @author Scrutinizer Auto-Fixer + * @author Thomas Müller + * + * @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 + * + */ /** * 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)); + } } -- cgit v1.2.3 From 9e322828f259c8cebb9953a5a315c2eacfa26ac2 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 30 Dec 2015 16:31:52 +0100 Subject: Cache if a group exists --- lib/private/group/database.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/group/database.php b/lib/private/group/database.php index 23d63234a4f..b85cb72d859 100644 --- a/lib/private/group/database.php +++ b/lib/private/group/database.php @@ -297,6 +297,7 @@ class OC_Group_Database extends OC_Group_Backend { $cursor->closeCursor(); if ($result !== false) { + $this->groupCache[$gid] = $gid; return true; } return false; -- cgit v1.2.3 From 9f4b296685d0dc14b586aa936eb713113b9c3ef5 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 5 Jan 2016 16:04:40 +0100 Subject: Properly close cursors --- lib/private/group/database.php | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/private/group/database.php b/lib/private/group/database.php index b85cb72d859..1aa821db087 100644 --- a/lib/private/group/database.php +++ b/lib/private/group/database.php @@ -91,12 +91,15 @@ class OC_Group_Database extends OC_Group_Backend { } else { // Check for existence in DB $qb = $this->dbConn->getQueryBuilder(); - $result = $qb->select('gid') + $cursor = $qb->select('gid') ->from('groups') ->where($qb->expr()->eq('gid', $qb->createNamedParameter($gid))) ->execute(); - if( $result->fetch() ) { + $result = $cursor->fetch(); + $cursor->closeCursor(); + + if($result) { // Can not add an existing group // Add to cache @@ -169,13 +172,16 @@ class OC_Group_Database extends OC_Group_Backend { // check $qb = $this->dbConn->getQueryBuilder(); - $result = $qb->select('uid') + $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->fetch() ? true : false; + $result = $cursor->fetch(); + $cursor->closeCursor(); + + return $result ? true : false; } /** @@ -235,16 +241,17 @@ class OC_Group_Database extends OC_Group_Backend { // No magic! $qb = $this->dbConn->getQueryBuilder(); - $result = $qb->select('gid') + $cursor = $qb->select('gid') ->from('group_user') ->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid))) ->execute(); $groups = []; - while( $row = $result->fetch()) { + while( $row = $cursor->fetch()) { $groups[] = $row["gid"]; $this->groupCache[$row['gid']] = $row['gid']; } + $cursor->closeCursor(); return $groups; } -- cgit v1.2.3