Background jobs can take 4k of characters only. We find a good batch size.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

5 files changed, 89 insertions, 18 deletions

diff --git a/apps/user_ldap/lib/Migration/UUIDFixGroup.php b/apps/user_ldap/lib/Migration/UUIDFixGroup.php
index ae84e668871..cbc38366984 100644
--- a/apps/user_ldap/lib/Migration/UUIDFixGroup.php
+++ b/apps/user_ldap/lib/Migration/UUIDFixGroup.php
@@ -24,13 +24,13 @@
 namespace OCA\User_LDAP\Migration;
 
 use OCA\User_LDAP\Helper;
-use OCA\User_LDAP\ILDAPWrapper;
+use OCA\User_LDAP\LDAP;
 use OCA\User_LDAP\Mapping\GroupMapping;
 use OCA\User_LDAP\User_Proxy;
 use OCP\IConfig;
 
 class UUIDFixGroup extends UUIDFix {
-	public function __construct(GroupMapping $mapper, ILDAPWrapper $ldap, IConfig $config, Helper $helper) {
+	public function __construct(GroupMapping $mapper, LDAP $ldap, IConfig $config, Helper $helper) {
 		$this->mapper = $mapper;
 		$this->proxy = new User_Proxy($helper->getServerConfigurationPrefixes(true), $ldap, $config);
 	}
diff --git a/apps/user_ldap/lib/Migration/UUIDFixInsert.php b/apps/user_ldap/lib/Migration/UUIDFixInsert.php
index a7678b4e71b..4a1104f2c6f 100644
--- a/apps/user_ldap/lib/Migration/UUIDFixInsert.php
+++ b/apps/user_ldap/lib/Migration/UUIDFixInsert.php
@@ -58,7 +58,7 @@ class UUIDFixInsert implements IRepairStep {
 	 * @since 9.1.0
 	 */
 	public function getName() {
-		return 'Insert UUIDFix background job for user and group batches of 500';
+		return 'Insert UUIDFix background job for user and group in batches';
 	}
 
 	/**
@@ -75,15 +75,26 @@ class UUIDFixInsert implements IRepairStep {
 			return;
 		}
 
-		$batchSize = 500;
 		foreach ([$this->userMapper, $this->groupMapper] as $mapper) {
 			$offset = 0;
+			$batchSize = 50;
 			$jobClass = $mapper instanceof UserMapping ? UUIDFixUser::class : UUIDFixGroup::class;
 			do {
+				$retry = false;
 				$records = $mapper->getList($offset, $batchSize);
-				$this->jobList->add($jobClass, ['records' => $records]);
-				$offset += $batchSize;
-			} while (count($records) === $batchSize);
+				if(count($records) === 0){
+					continue;
+				}
+				try {
+					$this->jobList->add($jobClass, ['records' => $records]);
+					$offset += $batchSize;
+				} catch (\InvalidArgumentException $e) {
+					if(strpos($e->getMessage(), 'Background job arguments can\'t exceed 4000') !== false) {
+						$batchSize = intval(floor(count($records) * 0.8));
+						$retry = true;
+					}
+				}
+			} while (count($records) === $batchSize || $retry);
 		}
 
 	}
diff --git a/apps/user_ldap/lib/Migration/UUIDFixUser.php b/apps/user_ldap/lib/Migration/UUIDFixUser.php
index 1e1c1ae23d0..ee1457dcccb 100644
--- a/apps/user_ldap/lib/Migration/UUIDFixUser.php
+++ b/apps/user_ldap/lib/Migration/UUIDFixUser.php
@@ -24,13 +24,13 @@
 namespace OCA\User_LDAP\Migration;
 
 use OCA\User_LDAP\Helper;
-use OCA\User_LDAP\ILDAPWrapper;
+use OCA\User_LDAP\LDAP;
 use OCA\User_LDAP\Mapping\UserMapping;
 use OCA\User_LDAP\Group_Proxy;
 use OCP\IConfig;
 
 class UUIDFixUser extends UUIDFix {
-	public function __construct(UserMapping $mapper, ILDAPWrapper $ldap, IConfig $config, Helper $helper) {
+	public function __construct(UserMapping $mapper, LDAP $ldap, IConfig $config, Helper $helper) {
 		$this->mapper = $mapper;
 		$this->proxy = new Group_Proxy($helper->getServerConfigurationPrefixes(true), $ldap, $config);
 	}
diff --git a/apps/user_ldap/tests/Migration/AbstractUUIDFixTest.php b/apps/user_ldap/tests/Migration/AbstractUUIDFixTest.php
index 2b0cb4f20fc..8921648da83 100644
--- a/apps/user_ldap/tests/Migration/AbstractUUIDFixTest.php
+++ b/apps/user_ldap/tests/Migration/AbstractUUIDFixTest.php
@@ -23,10 +23,10 @@
 
 namespace OCA\User_LDAP\Tests\Migration;
 
+use OCA\User_LDAP\LDAP;
 use Test\TestCase;
 use OCA\User_LDAP\Access;
 use OCA\User_LDAP\Helper;
-use OCA\User_LDAP\ILDAPWrapper;
 use OCA\User_LDAP\Migration\UUIDFixUser;
 use OCA\User_LDAP\Mapping\UserMapping;
 use OCA\User_LDAP\Mapping\GroupMapping;
@@ -40,7 +40,7 @@ abstract class AbstractUUIDFixTest extends TestCase {
 	/** @var  IConfig|\PHPUnit_Framework_MockObject_MockObject */
 	protected $config;
 
-	/** @var  ILDAPWrapper|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var  LDAP|\PHPUnit_Framework_MockObject_MockObject */
 	protected $ldap;
 
 	/** @var  UserMapping|GroupMapping|\PHPUnit_Framework_MockObject_MockObject */
@@ -61,7 +61,7 @@ abstract class AbstractUUIDFixTest extends TestCase {
 	public function setUp() {
 		parent::setUp();
 
-		$this->ldap = $this->createMock(ILDAPWrapper::class);
+		$this->ldap = $this->createMock(LDAP::class);
 		$this->config = $this->createMock(IConfig::class);
 		$this->access = $this->createMock(Access::class);
 
diff --git a/apps/user_ldap/tests/Migration/UUIDFixInsertTest.php b/apps/user_ldap/tests/Migration/UUIDFixInsertTest.php
index 287edd7fdc4..a5f7ea50175 100644
--- a/apps/user_ldap/tests/Migration/UUIDFixInsertTest.php
+++ b/apps/user_ldap/tests/Migration/UUIDFixInsertTest.php
@@ -63,7 +63,7 @@ class UUIDFixInsertTest extends TestCase {
 	}
 
 	public function testGetName() {
-		$this->assertSame('Insert UUIDFix background job for user and group batches of 500', $this->job->getName());
+		$this->assertSame('Insert UUIDFix background job for user and group in batches', $this->job->getName());
 	}
 
 	public function recordProvider() {
@@ -72,11 +72,11 @@ class UUIDFixInsertTest extends TestCase {
 			'name' => 'Something',
 			'uuid' => 'AB12-3456-CDEF7-8GH9'
 		];
-		array_fill(0, 500, $record);
+		array_fill(0, 50, $record);
 
 		$userBatches = [
-			0 => array_fill(0, 500, $record),
-			1 => array_fill(0, 500, $record),
+			0 => array_fill(0, 50, $record),
+			1 => array_fill(0, 50, $record),
 			2 => array_fill(0,  13, $record),
 		];
 
@@ -89,6 +89,29 @@ class UUIDFixInsertTest extends TestCase {
 		];
 	}
 
+	public function recordProviderTooLongAndNone() {
+		$record = [
+			'dn' => 'cn=somerecord,dc=somewhere',
+			'name' => 'Something',
+			'uuid' => 'AB12-3456-CDEF7-8GH9'
+		];
+		array_fill(0, 50, $record);
+
+		$userBatches = [
+			0 => array_fill(0, 50, $record),
+			1 => array_fill(0, 40, $record),
+			2 => array_fill(0, 32, $record),
+			3 => array_fill(0, 32, $record),
+			4 => array_fill(0, 23, $record),
+		];
+
+		$groupBatches = [0 => []];
+
+		return [
+			['userBatches' => $userBatches, 'groupBatches' => $groupBatches]
+		];
+	}
+
 	/**
 	 * @dataProvider recordProvider
 	 */
@@ -100,12 +123,12 @@ class UUIDFixInsertTest extends TestCase {
 
 		$this->userMapper->expects($this->exactly(3))
 			->method('getList')
-			->withConsecutive([0, 500], [500, 500], [1000, 500])
+			->withConsecutive([0, 50], [50, 50], [100, 50])
 			->willReturnOnConsecutiveCalls($userBatches[0], $userBatches[1], $userBatches[2]);
 
 		$this->groupMapper->expects($this->exactly(1))
 			->method('getList')
-			->with(0, 500)
+			->with(0, 50)
 			->willReturn($groupBatches[0]);
 
 		$this->jobList->expects($this->exactly(4))
@@ -116,6 +139,43 @@ class UUIDFixInsertTest extends TestCase {
 		$this->job->run($out);
 	}
 
+	/**
+	 * @dataProvider recordProviderTooLongAndNone
+	 */
+	public function testRunWithManyAndNone($userBatches, $groupBatches) {
+		$this->config->expects($this->once())
+			->method('getAppValue')
+			->with('user_ldap', 'installed_version', '1.2.1')
+			->willReturn('1.2.0');
+
+		$this->userMapper->expects($this->exactly(5))
+			->method('getList')
+			->withConsecutive([0, 50], [0, 40], [0, 32], [32, 32], [64, 32])
+			->willReturnOnConsecutiveCalls($userBatches[0], $userBatches[1], $userBatches[2],  $userBatches[3],  $userBatches[4]);
+
+		$this->groupMapper->expects($this->once())
+			->method('getList')
+			->with(0, 50)
+			->willReturn($groupBatches[0]);
+
+		$this->jobList->expects($this->at(0))
+			->method('add')
+			->willThrowException(new \InvalidArgumentException('Background job arguments can\'t exceed 4000 etc'));
+		$this->jobList->expects($this->at(1))
+			->method('add')
+			->willThrowException(new \InvalidArgumentException('Background job arguments can\'t exceed 4000 etc'));
+		$this->jobList->expects($this->at(2))
+			->method('add');
+		$this->jobList->expects($this->at(3))
+			->method('add');
+		$this->jobList->expects($this->at(4))
+			->method('add');
+
+		/** @var IOutput $out */
+		$out = $this->createMock(IOutput::class);
+		$this->job->run($out);
+	}
+
 	public function testDonNotRun() {
 		$this->config->expects($this->once())
 			->method('getAppValue')