Merge pull request #33034 from nextcloud/td/noid/ldap_group_updater

cleanup LDAP's UpdateGroups

2 files changed, 85 insertions, 66 deletions

diff --git a/apps/user_ldap/lib/Jobs/UpdateGroups.php b/apps/user_ldap/lib/Jobs/UpdateGroups.php
index b42049eb3a8..86b4b2b59f4 100644
--- a/apps/user_ldap/lib/Jobs/UpdateGroups.php
+++ b/apps/user_ldap/lib/Jobs/UpdateGroups.php
@@ -28,11 +28,15 @@
  */
 namespace OCA\User_LDAP\Jobs;
 
-use OC\BackgroundJob\TimedJob;
+use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\BackgroundJob\TimedJob;
 use OCA\User_LDAP\Group_Proxy;
+use OCP\DB\Exception;
+use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Group\Events\UserAddedEvent;
 use OCP\Group\Events\UserRemovedEvent;
+use OCP\IConfig;
 use OCP\IDBConnection;
 use OCP\IGroupManager;
 use OCP\IUser;
@@ -40,20 +44,14 @@ use OCP\IUserManager;
 use Psr\Log\LoggerInterface;
 
 class UpdateGroups extends TimedJob {
-	private $groupsFromDB;
-
-	/** @var Group_Proxy */
-	private $groupBackend;
-	/** @var IEventDispatcher */
-	private $dispatcher;
-	/** @var IGroupManager */
-	private $groupManager;
-	/** @var IUserManager */
-	private $userManager;
-	/** @var LoggerInterface */
-	private $logger;
-	/** @var IDBConnection */
-	private $dbc;
+    /** @var ?array<string, array{owncloudusers: string, owncloudname: string}>  */
+	private ?array $groupsFromDB = null;
+	private Group_Proxy $groupBackend;
+	private IEventDispatcher $dispatcher;
+	private IGroupManager $groupManager;
+	private IUserManager $userManager;
+	private LoggerInterface $logger;
+	private IDBConnection $dbc;
 
 	public function __construct(
 		Group_Proxy $groupBackend,
@@ -61,9 +59,12 @@ class UpdateGroups extends TimedJob {
 		IGroupManager $groupManager,
 		IUserManager $userManager,
 		LoggerInterface $logger,
-		IDBConnection $dbc
+		IDBConnection $dbc,
+		IConfig $config,
+		ITimeFactory $timeFactory
 	) {
-		$this->interval = $this->getRefreshInterval();
+		parent::__construct($timeFactory);
+		$this->interval = (int)$config->getAppValue('user_ldap', 'bgjRefreshInterval', '3600');
 		$this->groupBackend = $groupBackend;
 		$this->dispatcher = $dispatcher;
 		$this->groupManager = $groupManager;
@@ -73,26 +74,23 @@ class UpdateGroups extends TimedJob {
 	}
 
 	/**
-	 * @return int
-	 */
-	private function getRefreshInterval() {
-		//defaults to every hour
-		return \OC::$server->getConfig()->getAppValue('user_ldap', 'bgjRefreshInterval', 3600);
-	}
-
-	/**
 	 * @param mixed $argument
+	 * @throws Exception
 	 */
-	public function run($argument) {
+	public function run($argument): void {
 		$this->updateGroups();
 	}
 
-	public function updateGroups() {
+	/**
+	 * @throws Exception
+	 */
+	public function updateGroups(): void {
 		$this->logger->debug(
 			'Run background job "updateGroups"',
 			['app' => 'user_ldap']
 		);
 
+		/** @var string[] $knownGroups */
 		$knownGroups = array_keys($this->getKnownGroups());
 		$actualGroups = $this->groupBackend->getGroups();
 
@@ -115,17 +113,18 @@ class UpdateGroups extends TimedJob {
 	}
 
 	/**
-	 * @return array
+	 * @return array<string, array{owncloudusers: string, owncloudname: string}>
+	 * @throws Exception
 	 */
-	private function getKnownGroups() {
+	private function getKnownGroups(): array {
 		if (is_array($this->groupsFromDB)) {
-			$this->groupsFromDB;
+			return $this->groupsFromDB;
 		}
 		$qb = $this->dbc->getQueryBuilder();
 		$qb->select(['owncloudname', 'owncloudusers'])
 			->from('ldap_group_members');
 
-		$qResult = $qb->execute();
+		$qResult = $qb->executeQuery();
 		$result = $qResult->fetchAll();
 		$qResult->closeCursor();
 
@@ -137,7 +136,11 @@ class UpdateGroups extends TimedJob {
 		return $this->groupsFromDB;
 	}
 
-	private function handleKnownGroups(array $groups) {
+	/**
+	 * @param string[] $groups
+	 * @throws Exception
+	 */
+	private function handleKnownGroups(array $groups): void {
 		$this->logger->debug(
 			'bgJ "updateGroups" – Dealing with known Groups.',
 			['app' => 'user_ldap']
@@ -147,11 +150,9 @@ class UpdateGroups extends TimedJob {
 			->set('owncloudusers', $qb->createParameter('members'))
 			->where($qb->expr()->eq('owncloudname', $qb->createParameter('groupId')));
 
-		if (!is_array($this->groupsFromDB)) {
-			$this->getKnownGroups();
-		}
+		$groupsFromDB = $this->getKnownGroups();
 		foreach ($groups as $group) {
-			$knownUsers = unserialize($this->groupsFromDB[$group]['owncloudusers']);
+			$knownUsers = unserialize($groupsFromDB[$group]['owncloudusers']);
 			$actualUsers = $this->groupBackend->usersInGroup($group);
 			$hasChanged = false;
 
@@ -191,7 +192,7 @@ class UpdateGroups extends TimedJob {
 					'members' => serialize($actualUsers),
 					'groupId' => $group
 				]);
-				$qb->execute();
+				$qb->executeStatement();
 			}
 		}
 		$this->logger->debug(
@@ -202,8 +203,9 @@ class UpdateGroups extends TimedJob {
 
 	/**
 	 * @param string[] $createdGroups
+	 * @throws Exception
 	 */
-	private function handleCreatedGroups($createdGroups) {
+	private function handleCreatedGroups(array $createdGroups): void {
 		$this->logger->debug(
 			'bgJ "updateGroups" – dealing with created Groups.',
 			['app' => 'user_ldap']
@@ -213,6 +215,7 @@ class UpdateGroups extends TimedJob {
 		$query->insert('ldap_group_members')
 			->setValue('owncloudname', $query->createParameter('owncloudname'))
 			->setValue('owncloudusers', $query->createParameter('owncloudusers'));
+
 		foreach ($createdGroups as $createdGroup) {
 			$this->logger->info(
 				'bgJ "updateGroups" – new group "' . $createdGroup . '" found.',
@@ -222,7 +225,7 @@ class UpdateGroups extends TimedJob {
 
 			$query->setParameter('owncloudname', $createdGroup)
 				->setParameter('owncloudusers', $users);
-			$query->execute();
+			$query->executeStatement();
 		}
 		$this->logger->debug(
 			'bgJ "updateGroups" – FINISHED dealing with created Groups.',
@@ -232,8 +235,9 @@ class UpdateGroups extends TimedJob {
 
 	/**
 	 * @param string[] $removedGroups
+	 * @throws Exception
 	 */
-	private function handleRemovedGroups($removedGroups) {
+	private function handleRemovedGroups(array $removedGroups): void {
 		$this->logger->debug(
 			'bgJ "updateGroups" – dealing with removed groups.',
 			['app' => 'user_ldap']
@@ -241,16 +245,20 @@ class UpdateGroups extends TimedJob {
 
 		$query = $this->dbc->getQueryBuilder();
 		$query->delete('ldap_group_members')
-			->where($query->expr()->eq('owncloudname', $query->createParameter('owncloudname')));
+			->where($query->expr()->in('owncloudname', $query->createParameter('owncloudnames')));
 
-		foreach ($removedGroups as $removedGroup) {
+		foreach (array_chunk($removedGroups, 1000) as $removedGroupsChunk) {
 			$this->logger->info(
-				'bgJ "updateGroups" – group "' . $removedGroup . '" was removed.',
-				['app' => 'user_ldap']
+				'bgJ "updateGroups" – groups {removedGroups} were removed.',
+				[
+					'app' => 'user_ldap',
+					'removedGroups' => $removedGroupsChunk
+				]
 			);
-			$query->setParameter('owncloudname', $removedGroup);
-			$query->execute();
+			$query->setParameter('owncloudnames', $removedGroupsChunk, IQueryBuilder::PARAM_STR_ARRAY);
+			$query->executeStatement();
 		}
+
 		$this->logger->debug(
 			'bgJ "updateGroups" – FINISHED dealing with removed groups.',
 			['app' => 'user_ldap']
diff --git a/apps/user_ldap/tests/Jobs/UpdateGroupsTest.php b/apps/user_ldap/tests/Jobs/UpdateGroupsTest.php
index 9c9c62b0d0e..22a02451e27 100644
--- a/apps/user_ldap/tests/Jobs/UpdateGroupsTest.php
+++ b/apps/user_ldap/tests/Jobs/UpdateGroupsTest.php
@@ -28,37 +28,44 @@ namespace OCA\user_ldap\tests\Jobs;
 
 use OCA\User_LDAP\Group_Proxy;
 use OCA\User_LDAP\Jobs\UpdateGroups;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\DB\IResult;
 use OCP\DB\QueryBuilder\IExpressionBuilder;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Group\Events\UserAddedEvent;
 use OCP\Group\Events\UserRemovedEvent;
+use OCP\IConfig;
 use OCP\IDBConnection;
 use OCP\IGroup;
 use OCP\IGroupManager;
 use OCP\IUser;
 use OCP\IUserManager;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
+use function serialize;
 
 class UpdateGroupsTest extends TestCase {
 
-	/** @var Group_Proxy|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var Group_Proxy|MockObject  */
 	protected $groupBackend;
-	/** @var IEventDispatcher|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IEventDispatcher|MockObject  */
 	protected $dispatcher;
-	/** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IGroupManager|MockObject  */
 	protected $groupManager;
-	/** @var IUserManager|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IUserManager|MockObject */
 	protected $userManager;
-	/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var LoggerInterface|MockObject */
 	protected $logger;
-	/** @var IDBConnection|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IDBConnection|MockObject  */
 	protected $dbc;
+	/** @var IConfig|MockObject */
+	protected $config;
+	/** @var ITimeFactory|MockObject */
+	protected $timeFactory;
 
-	/** @var UpdateGroups */
-	protected $updateGroupsJob;
+	protected UpdateGroups $updateGroupsJob;
 
 	public function setUp(): void {
 		$this->groupBackend = $this->createMock(Group_Proxy::class);
@@ -67,6 +74,8 @@ class UpdateGroupsTest extends TestCase {
 		$this->userManager = $this->createMock(IUserManager::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->dbc = $this->createMock(IDBConnection::class);
+		$this->config = $this->createMock(IConfig::class);
+		$this->timeFactory = $this->createMock(ITimeFactory::class);
 
 		$this->updateGroupsJob = new UpdateGroups(
 			$this->groupBackend,
@@ -74,18 +83,20 @@ class UpdateGroupsTest extends TestCase {
 			$this->groupManager,
 			$this->userManager,
 			$this->logger,
-			$this->dbc
+			$this->dbc,
+			$this->config,
+			$this->timeFactory
 		);
 	}
 
-	public function testHandleKnownGroups() {
+	public function testHandleKnownGroups(): void {
 		$knownGroups = [
-			'emptyGroup' => \serialize([]),
-			'stableGroup' => \serialize(['userA', 'userC', 'userE']),
-			'groupWithAdditions' => \serialize(['userA', 'userC', 'userE']),
-			'groupWithRemovals' => \serialize(['userA', 'userC', 'userDeleted', 'userE']),
-			'groupWithAdditionsAndRemovals' => \serialize(['userA', 'userC', 'userE']),
-			'vanishedGroup' => \serialize(['userB', 'userDeleted'])
+			'emptyGroup' => serialize([]),
+			'stableGroup' => serialize(['userA', 'userC', 'userE']),
+			'groupWithAdditions' => serialize(['userA', 'userC', 'userE']),
+			'groupWithRemovals' => serialize(['userA', 'userC', 'userDeleted', 'userE']),
+			'groupWithAdditionsAndRemovals' => serialize(['userA', 'userC', 'userE']),
+			'vanishedGroup' => serialize(['userB', 'userDeleted'])
 		];
 		$knownGroupsDB = [];
 		foreach ($knownGroups as $gid => $members) {
@@ -104,7 +115,7 @@ class UpdateGroupsTest extends TestCase {
 		];
 		$groups = array_intersect(array_keys($knownGroups), array_keys($actualGroups));
 
-		/** @var IQueryBuilder|\PHPUnit\Framework\MockObject\MockObject $updateQb */
+		/** @var IQueryBuilder|MockObject $updateQb */
 		$updateQb = $this->createMock(IQueryBuilder::class);
 		$updateQb->expects($this->once())
 			->method('update')
@@ -119,7 +130,7 @@ class UpdateGroupsTest extends TestCase {
 		$updateQb->expects($this->exactly(3))
 			->method('setParameters');
 		$updateQb->expects($this->exactly(3))
-			->method('execute');
+			->method('executeStatement');
 		$updateQb->expects($this->any())
 			->method('expr')
 			->willReturn($this->createMock(IExpressionBuilder::class));
@@ -137,7 +148,7 @@ class UpdateGroupsTest extends TestCase {
 			->method('from')
 			->willReturn($selectQb);
 		$selectQb->expects($this->once())
-			->method('execute')
+			->method('executeQuery')
 			->willReturn($stmt);
 
 		$this->dbc->expects($this->any())
@@ -147,7 +158,7 @@ class UpdateGroupsTest extends TestCase {
 		$this->groupBackend->expects($this->any())
 			->method('usersInGroup')
 			->willReturnCallback(function ($groupID) use ($actualGroups) {
-				return isset($actualGroups[$groupID]) ? $actualGroups[$groupID] : [];
+				return $actualGroups[$groupID] ?? [];
 			});
 
 		$this->groupManager->expects($this->any())