Merge "Search: Validate pagination query params for SimplePropertySearch"

Author: jenkins-bot

repo/domains/search/src/Application/UseCases/SimplePropertySearch/SimplePropertySearchValidator.php

@@ -10,7 +10,12 @@
  * @license GPL-2.0-or-later
  */
 class SimplePropertySearchValidator {
+
 	public const LANGUAGE_QUERY_PARAM = 'language';
+	public const LIMIT_QUERY_PARAM = 'limit';
+	public const OFFSET_QUERY_PARAM = 'offset';
+
+	private const MAX_LIMIT = 500;
 
 	private SearchLanguageValidator $languageValidator;
 
@@ -38,5 +43,20 @@ public function validate( SimplePropertySearchRequest $request ): void {
 					throw new LogicException( 'unknown validation error code ' . $validationError->getCode() );
 			}
 		}
+
+		$this->validateLimitAndOffset( $request );
+	}
+
+	private function validateLimitAndOffset( SimplePropertySearchRequest $request ): void {
+		$limit = $request->getLimit();
+		$offset = $request->getOffset();
+
+		if ( $limit < 0 || $limit > self::MAX_LIMIT ) {
+			throw UseCaseError::invalidQueryParameter( self::LIMIT_QUERY_PARAM );
+		}
+
+		if ( $offset < 0 ) {
+			throw UseCaseError::invalidQueryParameter( self::OFFSET_QUERY_PARAM );
+		}
 	}
 }

repo/domains/search/tests/mocha/api-testing/SimplePropertySearchTest.js

@@ -18,6 +18,13 @@ function newSearchRequest( language, searchTerm ) {
 		.withQueryParam( 'q', searchTerm );
 }
 
+function assertValidError( response, statusCode, responseBodyErrorCode, context ) {
+	expect( response ).to.have.status( statusCode );
+	assert.header( response, 'Content-Language', 'en' );
+	assert.strictEqual( response.body.code, responseBodyErrorCode );
+	assert.deepStrictEqual( response.body.context, context );
+}
+
 describe( 'Simple property search', () => {
 	let property1;
 	let property2;
@@ -181,11 +188,7 @@ describe( 'Simple property search', () => {
 				.assertInvalidRequest()
 				.makeRequest();
 
-			expect( response ).to.have.status( 400 );
-
-			assert.header( response, 'Content-Language', 'en' );
-			assert.strictEqual( response.body.code, 'invalid-query-parameter' );
-			assert.deepStrictEqual( response.body.context, { parameter: 'language' } );
+			assertValidError( response, 400, 'invalid-query-parameter', { parameter: 'language' } );
 		} );
 
 		it( 'User-Agent empty', async () => {
@@ -196,6 +199,29 @@ describe( 'Simple property search', () => {
 			expect( response ).to.have.status( 400 );
 			assert.strictEqual( response.body.code, 'missing-user-agent' );
 		} );
-	} );
 
+		Object.entries( {
+			'invalid limit parameter - exceeds max limit 500': {
+				parameter: 'limit',
+				value: 501
+			},
+			'invalid limit parameter - negative limit': {
+				parameter: 'limit',
+				value: -1
+			},
+			'invalid offset parameter - negative offset': {
+				parameter: 'offset',
+				value: -2
+			}
+		} ).forEach( ( [ title, { parameter, value } ] ) => {
+			it( title, async () => {
+
+				const response = await newSearchRequest( 'en', 'search term' )
+					.withQueryParam( parameter, value )
+					.makeRequest();
+
+				assertValidError( response, 400, 'invalid-query-parameter', { parameter } );
+			} );
+		} );
+	} );
 } );

repo/domains/search/tests/phpunit/Application/UseCases/SimplePropertySearch/SimplePropertySearchValidatorTest.php

@@ -2,6 +2,7 @@
 
 namespace Wikibase\Repo\Tests\Domains\Search\Application\UseCases\SimplePropertySearch;
 
+use Generator;
 use MediaWiki\MediaWikiServices;
 use PHPUnit\Framework\TestCase;
 use Wikibase\Repo\Domains\Search\Application\UseCases\SimplePropertySearch\SimplePropertySearchRequest;
@@ -24,18 +25,29 @@
  */
 class SimplePropertySearchValidatorTest extends TestCase {
 
+	private const DEFAULT_LIMIT = 10;
+	private const DEFAULT_OFFSET = 0;
+
 	/**
 	 * @doesNotPerformAssertions
 	 */
 	public function testValidate_passes(): void {
 		$this->newUseCaseValidator()
-			->validate( new SimplePropertySearchRequest( 'search term', 'en', 10, 0 ) );
+			->validate( new SimplePropertySearchRequest( 'search term', 'en', self::DEFAULT_LIMIT, self::DEFAULT_OFFSET ) );
+	}
+
+	/**
+	 * @doesNotPerformAssertions
+	 */
+	public function testValidateWithoutLimitAndOffsetParams_passe(): void {
+		$this->newUseCaseValidator()
+			->validate( new SimplePropertySearchRequest( 'search term', 'en', null, null ) );
 	}
 
 	public function testGivenInvalidLanguageCode_throws(): void {
 		try {
 			$this->newUseCaseValidator()
-				->validate( new SimplePropertySearchRequest( 'search term', 'xyz', 10, 0 ) );
+				->validate( new SimplePropertySearchRequest( 'search term', 'xyz', self::DEFAULT_LIMIT, self::DEFAULT_OFFSET ) );
 
 			$this->fail( 'Expected exception was not thrown' );
 		} catch ( UseCaseError $e ) {
@@ -48,6 +60,43 @@ public function testGivenInvalidLanguageCode_throws(): void {
 		}
 	}
 
+	/**
+	 * @dataProvider provideInvalidLimitAndOffset
+	 */
+	public function testGivenInvalidLimitAndOffset_throws(
+		UseCaseError $expectedError,
+		int $limit,
+		int $offset
+	): void {
+		try {
+			$this->newUseCaseValidator()
+				->validate( new SimplePropertySearchRequest( 'search term', 'en', $limit, $offset ) );
+			$this->fail( 'Expected exception was not thrown' );
+		} catch ( UseCaseError $e ) {
+			$this->assertEquals( $expectedError, $e );
+		}
+	}
+
+	public static function provideInvalidLimitAndOffset(): Generator {
+		yield 'invalid limit - negative limit' => [
+			UseCaseError::invalidQueryParameter( 'limit' ),
+			-1,
+			self::DEFAULT_OFFSET,
+		];
+
+		yield 'invalid limit - limit exceeds max (500)' => [
+			UseCaseError::invalidQueryParameter( 'limit' ),
+			501,
+			self::DEFAULT_OFFSET,
+		];
+
+		yield 'invalid offset - negative offset' => [
+			UseCaseError::invalidQueryParameter( 'offset' ),
+			self::DEFAULT_LIMIT,
+			-2,
+		];
+	}
+
 	private function newUseCaseValidator(): SimplePropertySearchValidator {
 		return new SimplePropertySearchValidator( $this->newSearchLanguageValidator() );
 	}