DB: warn on parameter number constraints

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
nextcloud · Jan 8, 2021 · 36ed317 · 36ed317
1 parent 13bda55
commit 36ed317
Show file tree

Hide file tree

Showing 2 changed files with 111 additions and 6 deletions.
diff --git a/lib/private/DB/QueryBuilder/QueryBuilder.php b/lib/private/DB/QueryBuilder/QueryBuilder.php
@@ -246,6 +246,36 @@ public function 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',
+			]);
+		}
+
 		return $this->queryBuilder->execute();
 	}
 

diff --git a/tests/lib/DB/QueryBuilder/QueryBuilderTest.php b/tests/lib/DB/QueryBuilder/QueryBuilderTest.php
@@ -22,6 +22,7 @@
 namespace Test\DB\QueryBuilder;
 
 use Doctrine\DBAL\Query\Expression\CompositeExpression;
+use Doctrine\DBAL\Query\QueryException;
 use OC\DB\QueryBuilder\Literal;
 use OC\DB\QueryBuilder\Parameter;
 use OC\DB\QueryBuilder\QueryBuilder;
@@ -1261,6 +1262,10 @@ public function testExecuteWithoutLogger() {
 			->expects($this->once())
 			->method('execute')
 			->willReturn(3);
+		$queryBuilder
+			->expects($this->any())
+			->method('getParameters')
+			->willReturn([]);
 		$this->logger
 			->expects($this->never())
 			->method('debug');
@@ -1277,14 +1282,14 @@ public function testExecuteWithoutLogger() {
 	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
@@ -1315,11 +1320,11 @@ public function testExecuteWithLoggerAndNamedArray() {
 	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
@@ -1350,11 +1355,11 @@ public function testExecuteWithLoggerAndUnnamedArray() {
 	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
@@ -1380,4 +1385,74 @@ public function testExecuteWithLoggerAndNoParams() {
 		$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->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->assertEquals([], $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->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->assertEquals([], $this->queryBuilder->execute());
+	}
 }