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

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

diff --git a/apps/user_ldap/lib/Migration/UUIDFixGroup.php b/apps/user_ldap/lib/Migration/UUIDFixGroup.php
index ae84e668871b098753146ace2ed7345ddf29afc1..cbc3836698472d50b03972f287548aab13c7e599 100644 (file)
--- 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 a7678b4e71bf7910892c4685b34ce48cb531594d..4a1104f2c6f9329023787142a995156d182c80df 100644 (file)
--- 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 1e1c1ae23d0c7114de4e20336b291fea3273ef8d..ee1457dcccbb9a2dc0668d814ea17faef7d08515 100644 (file)
--- 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 2b0cb4f20fc6de559d40e2f49d3a131cc867c431..8921648da83aca6f380b039a6aa6dd20701f00c7 100644 (file)
--- 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 287edd7fdc473434441d392b428bccc88010a8fc..a5f7ea50175cb26cfdbf7b9d94ff470fe2b67f98 100644 (file)
--- 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')