From e57d8d6521f25cf6523c1a880ad22727674f3ae0 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 8 Jan 2021 11:54:07 +0100 Subject: [PATCH 1/4] respect DB limits limit per statement and query Signed-off-by: Arthur Schiwon --- .../user_ldap/lib/Mapping/AbstractMapping.php | 51 ++++++++++++++++--- .../tests/Mapping/AbstractMappingTest.php | 19 +++++++ 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/apps/user_ldap/lib/Mapping/AbstractMapping.php b/apps/user_ldap/lib/Mapping/AbstractMapping.php index 0daa4f1504..d46010efc3 100644 --- a/apps/user_ldap/lib/Mapping/AbstractMapping.php +++ b/apps/user_ldap/lib/Mapping/AbstractMapping.php @@ -27,6 +27,7 @@ namespace OCA\User_LDAP\Mapping; use OC\DB\QueryBuilder\QueryBuilder; +use OCP\DB\QueryBuilder\IQueryBuilder; /** * Class AbstractMapping @@ -188,19 +189,57 @@ abstract class AbstractMapping { return $this->cache[$fdn]; } - public function getListOfIdsByDn(array $fdns): array { + protected function prepareListOfIdsQuery(array $dnList): IQueryBuilder { $qb = $this->dbc->getQueryBuilder(); $qb->select('owncloud_name', 'ldap_dn') ->from($this->getTableName(false)) - ->where($qb->expr()->in('ldap_dn', $qb->createNamedParameter($fdns, QueryBuilder::PARAM_STR_ARRAY))); - $stmt = $qb->execute(); + ->where($qb->expr()->in('ldap_dn', $qb->createNamedParameter($dnList, QueryBuilder::PARAM_STR_ARRAY))); + return $qb; + } - $results = $stmt->fetchAll(\Doctrine\DBAL\FetchMode::ASSOCIATIVE); - foreach ($results as $key => $entry) { - unset($results[$key]); + protected function collectResultsFromListOfIdsQuery(IQueryBuilder $qb, array &$results): void { + $stmt = $qb->execute(); + while ($entry = $stmt->fetch(\Doctrine\DBAL\FetchMode::ASSOCIATIVE)) { $results[$entry['ldap_dn']] = $entry['owncloud_name']; $this->cache[$entry['ldap_dn']] = $entry['owncloud_name']; } + $stmt->closeCursor(); + } + + public function getListOfIdsByDn(array $fdns): array { + $totalDBParamLimit = 65000; + $sliceSize = 1000; + $maxSlices = $totalDBParamLimit / $sliceSize; + $results = []; + + $slice = 1; + $fdnsSlice = count($fdns) > $sliceSize ? array_slice($fdns, 0, $sliceSize) : $fdns; + $qb = $this->prepareListOfIdsQuery($fdnsSlice); + + while (isset($fdnsSlice[999])) { + // Oracle does not allow more than 1000 values in the IN list, + // but allows slicing + $slice++; + $fdnsSlice = array_slice($fdns, $sliceSize * ($slice - 1), $sliceSize); + + if (!isset($qb)) { + $qb = $this->prepareListOfIdsQuery($fdnsSlice); + continue; + } + + if (!empty($fdnsSlice)) { + $qb->orWhere($qb->expr()->in('ldap_dn', $qb->createNamedParameter($fdnsSlice, QueryBuilder::PARAM_STR_ARRAY))); + } + + if ($slice % $maxSlices === 0) { + $this->collectResultsFromListOfIdsQuery($qb, $results); + unset($qb); + } + } + + if (isset($qb)) { + $this->collectResultsFromListOfIdsQuery($qb, $results); + } return $results; } diff --git a/apps/user_ldap/tests/Mapping/AbstractMappingTest.php b/apps/user_ldap/tests/Mapping/AbstractMappingTest.php index 079c2e21b1..35259345f2 100644 --- a/apps/user_ldap/tests/Mapping/AbstractMappingTest.php +++ b/apps/user_ldap/tests/Mapping/AbstractMappingTest.php @@ -281,4 +281,23 @@ abstract class AbstractMappingTest extends \Test\TestCase { $results = $mapper->getList(1, 1); $this->assertSame(1, count($results)); } + + public function testGetListOfIdsByDn() { + /** @var AbstractMapping $mapper */ + list($mapper,) = $this->initTest(); + + $listOfDNs = []; + for ($i = 0; $i < 66640; $i++) { + // Postgres has a limit of 65535 values in a single IN list + $name = 'as_' . $i; + $dn = 'uid=' . $name . ',dc=example,dc=org'; + $listOfDNs[] = $dn; + if ($i % 20 === 0) { + $mapper->map($dn, $name, 'fake-uuid-' . $i); + } + } + + $result = $mapper->getListOfIdsByDn($listOfDNs); + $this->assertSame(66640 / 20, count($result)); + } } From 83b0b84c1fbdbe9843a6d5509ddfdcec29f3ed06 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 8 Jan 2021 12:46:10 +0100 Subject: [PATCH 2/4] DB: warn on parameter number constraints Signed-off-by: Arthur Schiwon --- lib/private/DB/QueryBuilder/QueryBuilder.php | 30 +++++++ .../lib/DB/QueryBuilder/QueryBuilderTest.php | 88 +++++++++++++++++-- 2 files changed, 112 insertions(+), 6 deletions(-) diff --git a/lib/private/DB/QueryBuilder/QueryBuilder.php b/lib/private/DB/QueryBuilder/QueryBuilder.php index a2941950d5..9ab05a60a1 100644 --- a/lib/private/DB/QueryBuilder/QueryBuilder.php +++ b/lib/private/DB/QueryBuilder/QueryBuilder.php @@ -214,6 +214,36 @@ class QueryBuilder implements IQueryBuilder { } return $this->queryBuilder->execute(); + $numberOfParameters = 0; + $hasTooLargeArrayParameter = false; + foreach ($this->getParameters() as $parameter) { + if (is_array($parameter)) { + $count = count($parameter); + $numberOfParameters += $count; + $hasTooLargeArrayParameter = $hasTooLargeArrayParameter || ($count > 1000); + } + } + + if ($hasTooLargeArrayParameter) { + $exception = new QueryException('More than 1000 expressions in a list are not allowed on Oracle.'); + $this->logger->logException($exception, [ + 'message' => 'More than 1000 expressions in a list are not allowed on Oracle.', + 'query' => $this->getSQL(), + 'level' => ILogger::ERROR, + 'app' => 'core', + ]); + } + + if ($numberOfParameters > 65535) { + $exception = new QueryException('The number of parameters must not exceed 65535. Restriction by PostgreSQL.'); + $this->logger->logException($exception, [ + 'message' => 'The number of parameters must not exceed 65535. Restriction by PostgreSQL.', + 'query' => $this->getSQL(), + 'level' => ILogger::ERROR, + 'app' => 'core', + ]); + } + } /** diff --git a/tests/lib/DB/QueryBuilder/QueryBuilderTest.php b/tests/lib/DB/QueryBuilder/QueryBuilderTest.php index a9542e2d61..e787b20a84 100644 --- a/tests/lib/DB/QueryBuilder/QueryBuilderTest.php +++ b/tests/lib/DB/QueryBuilder/QueryBuilderTest.php @@ -22,6 +22,8 @@ namespace Test\DB\QueryBuilder; use Doctrine\DBAL\Query\Expression\CompositeExpression; +use Doctrine\DBAL\Query\QueryException; +use Doctrine\DBAL\Result; use OC\DB\QueryBuilder\Literal; use OC\DB\QueryBuilder\Parameter; use OC\DB\QueryBuilder\QueryBuilder; @@ -1223,6 +1225,10 @@ class QueryBuilderTest extends \Test\TestCase { ->expects($this->once()) ->method('execute') ->willReturn(3); + $queryBuilder + ->expects($this->any()) + ->method('getParameters') + ->willReturn([]); $this->logger ->expects($this->never()) ->method('debug'); @@ -1239,14 +1245,14 @@ class QueryBuilderTest extends \Test\TestCase { public function testExecuteWithLoggerAndNamedArray() { $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); $queryBuilder - ->expects($this->at(0)) + ->expects($this->any()) ->method('getParameters') ->willReturn([ 'foo' => 'bar', 'key' => 'value', ]); $queryBuilder - ->expects($this->at(1)) + ->expects($this->any()) ->method('getSQL') ->willReturn('SELECT * FROM FOO WHERE BAR = ?'); $queryBuilder @@ -1277,11 +1283,11 @@ class QueryBuilderTest extends \Test\TestCase { public function testExecuteWithLoggerAndUnnamedArray() { $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); $queryBuilder - ->expects($this->at(0)) + ->expects($this->any()) ->method('getParameters') ->willReturn(['Bar']); $queryBuilder - ->expects($this->at(1)) + ->expects($this->any()) ->method('getSQL') ->willReturn('SELECT * FROM FOO WHERE BAR = ?'); $queryBuilder @@ -1312,11 +1318,11 @@ class QueryBuilderTest extends \Test\TestCase { public function testExecuteWithLoggerAndNoParams() { $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); $queryBuilder - ->expects($this->at(0)) + ->expects($this->any()) ->method('getParameters') ->willReturn([]); $queryBuilder - ->expects($this->at(1)) + ->expects($this->any()) ->method('getSQL') ->willReturn('SELECT * FROM FOO WHERE BAR = ?'); $queryBuilder @@ -1342,4 +1348,74 @@ class QueryBuilderTest extends \Test\TestCase { $this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]); $this->assertEquals(3, $this->queryBuilder->execute()); } + + public function testExecuteWithParameterTooLarge() { + $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); + $p = array_fill(0, 1001, 'foo'); + $queryBuilder + ->expects($this->any()) + ->method('getParameters') + ->willReturn([$p]); + $queryBuilder + ->expects($this->any()) + ->method('getSQL') + ->willReturn('SELECT * FROM FOO WHERE BAR IN (?)'); + $queryBuilder + ->expects($this->once()) + ->method('execute') + ->willReturn($this->createMock(Result::class)); + $this->logger + ->expects($this->once()) + ->method('logException') + ->willReturnCallback(function ($e, $parameters) { + $this->assertInstanceOf(QueryException::class, $e); + $this->assertSame( + 'More than 1000 expressions in a list are not allowed on Oracle.', + $parameters['message'] + ); + }); + $this->config + ->expects($this->once()) + ->method('getValue') + ->with('log_query', false) + ->willReturn(false); + + $this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]); + $this->queryBuilder->execute(); + } + + public function testExecuteWithParametersTooMany() { + $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); + $p = array_fill(0, 999, 'foo'); + $queryBuilder + ->expects($this->any()) + ->method('getParameters') + ->willReturn(array_fill(0, 66, $p)); + $queryBuilder + ->expects($this->any()) + ->method('getSQL') + ->willReturn('SELECT * FROM FOO WHERE BAR IN (?) OR BAR IN (?)'); + $queryBuilder + ->expects($this->once()) + ->method('execute') + ->willReturn($this->createMock(Result::class)); + $this->logger + ->expects($this->once()) + ->method('logException') + ->willReturnCallback(function ($e, $parameters) { + $this->assertInstanceOf(QueryException::class, $e); + $this->assertSame( + 'The number of parameters must not exceed 65535. Restriction by PostgreSQL.', + $parameters['message'] + ); + }); + $this->config + ->expects($this->once()) + ->method('getValue') + ->with('log_query', false) + ->willReturn(false); + + $this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]); + $this->queryBuilder->execute(); + } } From b4d83c6f02135e129ecbee2d1fd171d862f9d038 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 13 Jan 2021 11:54:29 +0100 Subject: [PATCH 3/4] silence psalm false positive Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Mapping/AbstractMapping.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/user_ldap/lib/Mapping/AbstractMapping.php b/apps/user_ldap/lib/Mapping/AbstractMapping.php index d46010efc3..6a395b2c84 100644 --- a/apps/user_ldap/lib/Mapping/AbstractMapping.php +++ b/apps/user_ldap/lib/Mapping/AbstractMapping.php @@ -222,6 +222,8 @@ abstract class AbstractMapping { $slice++; $fdnsSlice = array_slice($fdns, $sliceSize * ($slice - 1), $sliceSize); + /** @see https://github.com/vimeo/psalm/issues/4995 */ + /** @psalm-suppress TypeDoesNotContainType */ if (!isset($qb)) { $qb = $this->prepareListOfIdsQuery($fdnsSlice); continue; From b78ed6ea3f353d527a7908fbc4bf0ed051c5f1d4 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 14 Jan 2021 13:16:02 +0100 Subject: [PATCH 4/4] CI satisfaction Signed-off-by: Arthur Schiwon --- lib/private/DB/QueryBuilder/QueryBuilder.php | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/private/DB/QueryBuilder/QueryBuilder.php b/lib/private/DB/QueryBuilder/QueryBuilder.php index 9ab05a60a1..3b109b8c67 100644 --- a/lib/private/DB/QueryBuilder/QueryBuilder.php +++ b/lib/private/DB/QueryBuilder/QueryBuilder.php @@ -243,7 +243,6 @@ class QueryBuilder implements IQueryBuilder { 'app' => 'core', ]); } - } /**