From 42ddb12fd936523583c3d0231bb6ab5c901d8084 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 17 Feb 2017 20:06:25 +0100 Subject: [PATCH] Background jobs can take 4k of characters only. We find a good batch size. Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Migration/UUIDFixGroup.php | 4 +- .../user_ldap/lib/Migration/UUIDFixInsert.php | 21 ++++-- apps/user_ldap/lib/Migration/UUIDFixUser.php | 4 +- .../tests/Migration/AbstractUUIDFixTest.php | 6 +- .../tests/Migration/UUIDFixInsertTest.php | 72 +++++++++++++++++-- 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') -- 2.39.5