Merge "FilterLookup: Stop using DBAccessObjectUtils::getDBOptions()"

2024-09-23 10:18:22 +00:00 · 2024-03-25 18:20:52 +00:00 · 2024-03-25 18:20:52 +00:00 · 430b7f81ad
parent bea46722c7 fe0fed1d8f
commit 430b7f81ad
5 changed files with 96 additions and 137 deletions
--- a/extension.json
+++ b/extension.json
@ -559,7 +559,7 @@
 			"description": "Whether to include IP in the abuse_filter_log"
 		},
 		"AbuseFilterActorTableSchemaMigrationStage": {
-			"value": 3,
+			"value": 769,
 			"description": "Stage of the migration of af_user/af_user_text to af_actor and afh_user/afh_user_text to afh_actor. Should be one of the following (combinations of) constants: SCHEMA_COMPAT_OLD, SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, SCHEMA_COMPAT_NEW. See also \"Schema compatibility flags\" in includes/Defines.php in core. Note: When changing the default here, change it also in unit tests."
 		},
 		"AbuseFilterEnableBlockedExternalDomain": {
--- a/includes/FilterLookup.php
+++ b/includes/FilterLookup.php
@ -2,7 +2,6 @@

 namespace MediaWiki\Extension\AbuseFilter;

-use DBAccessObjectUtils;
 use IDBAccessObject;
 use MediaWiki\Extension\AbuseFilter\Filter\ClosestFilterVersionNotFoundException;
 use MediaWiki\Extension\AbuseFilter\Filter\ExistingFilter;
@ -12,12 +11,12 @@ use MediaWiki\Extension\AbuseFilter\Filter\Flags;
 use MediaWiki\Extension\AbuseFilter\Filter\HistoryFilter;
 use MediaWiki\Extension\AbuseFilter\Filter\LastEditInfo;
 use MediaWiki\Extension\AbuseFilter\Filter\Specs;
-use MediaWiki\User\ActorMigrationBase;
 use RuntimeException;
 use stdClass;
 use WANObjectCache;
 use Wikimedia\Rdbms\ILoadBalancer;
 use Wikimedia\Rdbms\IReadableDatabase;
+use Wikimedia\Rdbms\SelectQueryBuilder;

 /**
 * This class provides read access to the filters stored in the database.
@ -69,9 +68,6 @@ class FilterLookup implements IDBAccessObject {
 	/** @var CentralDBManager */
 	private $centralDBManager;

-	/** @var ActorMigrationBase */
-	private $actorMigration;
-
 	/**
 	 * @var bool Flag used in PHPUnit tests to "hide" local filters when testing global ones, so that we can use the
 	 * local database pretending it's not local.
@ -82,18 +78,15 @@ class FilterLookup implements IDBAccessObject {
 	 * @param ILoadBalancer $loadBalancer
 	 * @param WANObjectCache $cache
 	 * @param CentralDBManager $centralDBManager
-	 * @param ActorMigrationBase $actorMigration
 	 */
 	public function __construct(
 		ILoadBalancer $loadBalancer,
 		WANObjectCache $cache,
-		CentralDBManager $centralDBManager,
-		ActorMigrationBase $actorMigration
+		CentralDBManager $centralDBManager
 	) {
 		$this->loadBalancer = $loadBalancer;
 		$this->wanCache = $cache;
 		$this->centralDBManager = $centralDBManager;
-		$this->actorMigration = $actorMigration;
 	}

 	/**
@ -109,18 +102,14 @@ class FilterLookup implements IDBAccessObject {
 	): ExistingFilter {
 		$cacheKey = $this->getCacheKey( $filterID, $global );
 		if ( $flags !== IDBAccessObject::READ_NORMAL || !isset( $this->cache[$cacheKey] ) ) {
-			[ $dbIndex, $dbOptions ] = DBAccessObjectUtils::getDBOptions( $flags );
-			$dbr = $this->getDBConnection( $dbIndex, $global );
-			$query = $this->getAbuseFilterQueryInfo();
+			$dbr = ( $flags & IDBAccessObject::READ_LATEST )
+				? $this->getDBConnection( DB_PRIMARY, $global )
+				: $this->getDBConnection( DB_REPLICA, $global );
+			$row = $this->getAbuseFilterQueryBuilder( $dbr )
+				->where( [ 'af_id' => $filterID ] )
+				->recency( $flags )
+				->caller( __METHOD__ )->fetchRow();

-			$row = $dbr->selectRow(
-				$query['tables'],
-				$query['fields'],
-				[ 'af_id' => $filterID ],
-				__METHOD__,
-				$dbOptions,
-				$query['joins']
-			);
 			if ( !$row ) {
 				throw new FilterNotFoundException( $filterID, $global );
 			}
@ -184,30 +173,20 @@ class FilterLookup implements IDBAccessObject {
 		if ( $this->localFiltersHiddenForTest && !$global ) {
 			return [];
 		}
+		$dbr = ( $flags & IDBAccessObject::READ_LATEST )
+			? $this->getDBConnection( DB_PRIMARY, $global )
+			: $this->getDBConnection( DB_REPLICA, $global );
+		$queryBuilder = $this->getAbuseFilterQueryBuilder( $dbr )
+			->where( [ 'af_enabled' => 1, 'af_deleted' => 0, 'af_group' => $group ] )
+			->recency( $flags );

-		[ $dbIndex, $dbOptions ] = DBAccessObjectUtils::getDBOptions( $flags );
-		$dbr = $this->getDBConnection( $dbIndex, $global );
-
-		$query = $this->getAbuseFilterQueryInfo();
-		$where = [
-			'af_enabled' => 1,
-			'af_deleted' => 0,
-			'af_group' => $group,
-		];
 		if ( $global ) {
-			$where['af_global'] = 1;
+			$queryBuilder->andWhere( [ 'af_global' => 1 ] );
 		}

 		// Note, excluding individually cached filter now wouldn't help much, so take it as
 		// an occasion to refresh the cache later
-		$rows = $dbr->select(
-			$query['tables'],
-			$query['fields'],
-			$where,
-			__METHOD__,
-			$dbOptions,
-			$query['joins']
-		);
+		$rows = $queryBuilder->caller( __METHOD__ )->fetchResultSet();

 		$fname = __METHOD__;
 		$ret = [];
@ -275,18 +254,13 @@ class FilterLookup implements IDBAccessObject {
 		int $flags = IDBAccessObject::READ_NORMAL
 	): HistoryFilter {
 		if ( $flags !== IDBAccessObject::READ_NORMAL || !isset( $this->historyCache[$version] ) ) {
-			[ $dbIndex, $dbOptions ] = DBAccessObjectUtils::getDBOptions( $flags );
-			$dbr = $this->loadBalancer->getConnection( $dbIndex );
-			$query = $this->getAbuseFilterHistoryQueryInfo();
-
-			$row = $dbr->selectRow(
-				$query['tables'],
-				$query['fields'],
-				[ 'afh_id' => $version ],
-				__METHOD__,
-				$dbOptions,
-				$query['joins']
-			);
+			$dbr = ( $flags & IDBAccessObject::READ_LATEST )
+				? $this->loadBalancer->getConnection( DB_PRIMARY )
+				: $this->loadBalancer->getConnection( DB_REPLICA );
+			$row = $this->getAbuseFilterHistoryQueryBuilder( $dbr )
+				->where( [ 'afh_id' => $version ] )
+				->recency( $flags )
+				->caller( __METHOD__ )->fetchRow();
 			if ( !$row ) {
 				throw new FilterVersionNotFoundException( $version );
 			}
@ -304,15 +278,10 @@ class FilterLookup implements IDBAccessObject {
 	public function getLastHistoryVersion( int $filterID ): HistoryFilter {
 		if ( !isset( $this->lastVersionCache[$filterID] ) ) {
 			$dbr = $this->loadBalancer->getConnection( DB_REPLICA );
-			$query = $this->getAbuseFilterHistoryQueryInfo();
-			$row = $dbr->selectRow(
-				$query['tables'],
-				$query['fields'],
-				[ 'afh_filter' => $filterID ],
-				__METHOD__,
-				[ 'ORDER BY' => 'afh_id DESC' ],
-				$query['joins']
-			);
+			$row = $this->getAbuseFilterHistoryQueryBuilder( $dbr )
+				->where( [ 'afh_filter' => $filterID ] )
+				->orderBy( 'afh_id', SelectQueryBuilder::SORT_DESC )
+				->caller( __METHOD__ )->fetchRow();
 			if ( !$row ) {
 				throw new FilterNotFoundException( $filterID, false );
 			}
@ -335,18 +304,11 @@ class FilterLookup implements IDBAccessObject {
 			$comparison = $direction === self::DIR_PREV ? '<' : '>';
 			$order = $direction === self::DIR_PREV ? 'DESC' : 'ASC';
 			$dbr = $this->loadBalancer->getConnection( DB_REPLICA );
-			$query = $this->getAbuseFilterHistoryQueryInfo();
-			$row = $dbr->selectRow(
-				$query['tables'],
-				$query['fields'],
-				[
-					'afh_filter' => $filterID,
-					"afh_id $comparison" . $dbr->addQuotes( $historyID ),
-				],
-				__METHOD__,
-				[ 'ORDER BY' => "afh_timestamp $order" ],
-				$query['joins']
-			);
+			$row = $this->getAbuseFilterHistoryQueryBuilder( $dbr )
+				->where( [ 'afh_filter' => $filterID ] )
+				->andWhere( $dbr->expr( 'afh_id', $comparison, $historyID ) )
+				->orderBy( 'afh_timestamp', $order )
+				->caller( __METHOD__ )->fetchRow();
 			if ( !$row ) {
 				throw new ClosestFilterVersionNotFoundException( $filterID, $historyID );
 			}
@ -491,52 +453,48 @@ class FilterLookup implements IDBAccessObject {
 		);
 	}

-	/**
-	 * @return array
-	 */
-	private function getAbuseFilterQueryInfo(): array {
-		$actorQuery = $this->actorMigration->getJoin( 'af_user' );
-		return [
-			'tables' => [ 'abuse_filter' ] + $actorQuery['tables'],
-			'fields' => [
-					'af_id',
-					'af_pattern',
-					'af_timestamp',
-					'af_enabled',
-					'af_comments',
-					'af_public_comments',
-					'af_hidden',
-					'af_hit_count',
-					'af_throttled',
-					'af_deleted',
-					'af_actions',
-					'af_global',
-					'af_group'
-				] + $actorQuery['fields'],
-			'joins' => $actorQuery['joins']
-		];
+	private function getAbuseFilterQueryBuilder( IReadableDatabase $dbr ): SelectQueryBuilder {
+		return $dbr->newSelectQueryBuilder()
+			->select( [
+				'af_id',
+				'af_pattern',
+				'af_timestamp',
+				'af_enabled',
+				'af_comments',
+				'af_public_comments',
+				'af_hidden',
+				'af_hit_count',
+				'af_throttled',
+				'af_deleted',
+				'af_actions',
+				'af_global',
+				'af_group',
+				'af_user' => 'actor_af_user.actor_user',
+				'af_user_text' => 'actor_af_user.actor_name',
+				'af_actor' => 'af_actor'
+			] )
+			->from( 'abuse_filter' )
+			->join( 'actor', 'actor_af_user', 'actor_af_user.actor_id = af_actor' );
 	}

-	/**
-	 * @return array
-	 */
-	private function getAbuseFilterHistoryQueryInfo(): array {
-		$actorQuery = $this->actorMigration->getJoin( 'afh_user' );
-		return [
-			'tables' => [ 'abuse_filter_history' ] + $actorQuery['tables'],
-			'fields' => [
-					'afh_id',
-					'afh_pattern',
-					'afh_timestamp',
-					'afh_filter',
-					'afh_comments',
-					'afh_public_comments',
-					'afh_flags',
-					'afh_actions',
-					'afh_group'
-				] + $actorQuery['fields'],
-			'joins' => $actorQuery['joins']
-		];
+	private function getAbuseFilterHistoryQueryBuilder( IReadableDatabase $dbr ): SelectQueryBuilder {
+		return $dbr->newSelectQueryBuilder()
+			->select( [
+				'afh_id',
+				'afh_pattern',
+				'afh_timestamp',
+				'afh_filter',
+				'afh_comments',
+				'afh_public_comments',
+				'afh_flags',
+				'afh_actions',
+				'afh_group',
+				'afh_user' => 'actor_afh_user.actor_user',
+				'afh_user_text' => 'actor_afh_user.actor_name',
+				'afh_actor' => 'afh_actor'
+			] )
+			->from( 'abuse_filter_history' )
+			->join( 'actor', 'actor_afh_user', 'actor_afh_user.actor_id = afh_actor' );
 	}

 	/**
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@ -131,8 +131,7 @@ return [
 		return new FilterLookup(
 			$services->getDBLoadBalancer(),
 			$services->getMainWANObjectCache(),
-			$services->get( CentralDBManager::SERVICE_NAME ),
-			$services->get( AbuseFilterActorMigration::SERVICE_NAME )
+			$services->get( CentralDBManager::SERVICE_NAME )
 		);
 	},
 	EmergencyCache::SERVICE_NAME => static function ( MediaWikiServices $services ): EmergencyCache {
--- a/tests/phpunit/integration/FilterStoreTest.php
+++ b/tests/phpunit/integration/FilterStoreTest.php
@ -47,9 +47,15 @@ class FilterStoreTest extends MediaWikiIntegrationTestCase {
 		// Use some black magic to bypass checks
 		/** @var FilterStore $filterStore */
 		$filterStore = TestingAccessWrapper::newFromObject( AbuseFilterServices::getFilterStore() );
+		$row = $filterStore->filterToDatabaseRow( $filter );
+		$row += AbuseFilterServices::getActorMigration()->getInsertValues(
+			$this->db,
+			'af_user',
+			$this->getTestUser()->getUserIdentity()
+		);
 		$this->db->insert(
 			'abuse_filter',
-			$filterStore->filterToDatabaseRow( $filter ),
+			$row,
 			__METHOD__
 		);
 	}
@ -124,10 +130,6 @@ class FilterStoreTest extends MediaWikiIntegrationTestCase {
 	}

 	public function testSaveFilter_invalid() {
-		$this->overrideConfigValue(
-			'AbuseFilterActorTableSchemaMigrationStage',
-			SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD
-		);
 		$row = [
 			'id' => null,
 			'rules' => '1==1',
@ -152,10 +154,6 @@ class FilterStoreTest extends MediaWikiIntegrationTestCase {
 	}

 	public function testSaveFilter_noChange() {
-		$this->overrideConfigValue(
-			'AbuseFilterActorTableSchemaMigrationStage',
-			SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD
-		);
 		$row = [
 			'id' => '1',
 			'rules' => '/**/',
--- a/tests/phpunit/unit/FilterLookupTest.php
+++ b/tests/phpunit/unit/FilterLookupTest.php
@ -5,7 +5,6 @@ namespace MediaWiki\Extension\AbuseFilter\Tests\Unit;
 use AbuseFilterRowsAndFiltersTestTrait;
 use Generator;
 use HashBagOStuff;
-use MediaWiki\Extension\AbuseFilter\AbuseFilterActorMigration;
 use MediaWiki\Extension\AbuseFilter\CentralDBManager;
 use MediaWiki\Extension\AbuseFilter\CentralDBNotAvailableException;
 use MediaWiki\Extension\AbuseFilter\Filter\ClosestFilterVersionNotFoundException;
@ -18,14 +17,13 @@ use MediaWiki\Extension\AbuseFilter\Filter\HistoryFilter;
 use MediaWiki\Extension\AbuseFilter\Filter\LastEditInfo;
 use MediaWiki\Extension\AbuseFilter\Filter\Specs;
 use MediaWiki\Extension\AbuseFilter\FilterLookup;
-use MediaWiki\User\ActorMigrationBase;
-use MediaWiki\User\ActorStoreFactory;
 use MediaWikiUnitTestCase;
 use stdClass;
 use WANObjectCache;
 use Wikimedia\Rdbms\DBConnRef;
 use Wikimedia\Rdbms\ILoadBalancer;
 use Wikimedia\Rdbms\LBFactory;
+use Wikimedia\Rdbms\SelectQueryBuilder;

 /**
 * @group Test
@ -65,11 +63,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 			$lb,
 			// Cannot use mocks because final methods aren't mocked and they would error out
 			$cache ?? new WANObjectCache( [ 'cache' => new HashBagOStuff() ] ),
-			$centralDBManager,
-			new AbuseFilterActorMigration(
-				SCHEMA_COMPAT_READ_OLD | SCHEMA_COMPAT_WRITE_BOTH,
-				$this->createMock( ActorStoreFactory::class )
-			)
+			$centralDBManager
 		);
 	}

@ -81,6 +75,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 	 */
 	private function getDBWithMockRows( array $filterRows, array $actionRows = [] ): DBConnRef {
 		$db = $this->createMock( DBConnRef::class );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		$db->method( 'selectRow' )->willReturnCallback( static function ( $table ) use ( $filterRows ) {
 			$tables = (array)$table;
 			return array_intersect( $tables, [ 'abuse_filter', 'abuse_filter_history' ] ) ? $filterRows[0] : false;
@ -207,6 +202,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 	public function testGetFilterVersion_notfound() {
 		$db = $this->createMock( DBConnRef::class );
 		$db->method( 'selectRow' )->willReturn( false );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		$filterLookup = $this->getLookup( $db );
 		$this->expectException( FilterVersionNotFoundException::class );
 		$filterLookup->getFilterVersion( 42 );
@ -229,6 +225,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 	public function testGetLastHistoryVersion_notfound() {
 		$db = $this->createMock( DBConnRef::class );
 		$db->method( 'selectRow' )->willReturn( false );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		$filterLookup = $this->getLookup( $db );
 		$this->expectException( FilterNotFoundException::class );
 		$filterLookup->getLastHistoryVersion( 42 );
@ -251,6 +248,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 	public function testGetClosestVersion_notfound() {
 		$db = $this->createMock( DBConnRef::class );
 		$db->method( 'selectRow' )->willReturn( false );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		$filterLookup = $this->getLookup( $db );
 		$this->expectException( ClosestFilterVersionNotFoundException::class );
 		$filterLookup->getClosestVersion( 42, 42, FilterLookup::DIR_PREV );
@ -263,6 +261,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 		$versionID = 1234;
 		$db = $this->createMock( DBConnRef::class );
 		$db->method( 'selectField' )->willReturn( $versionID );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		$filterLookup = $this->getLookup( $db );
 		$this->assertSame( $versionID, $filterLookup->getFirstFilterVersionID( 42 ) );
 	}
@ -273,6 +272,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 	public function testGetFirstFilterVersionID_notfound() {
 		$db = $this->createMock( DBConnRef::class );
 		$db->method( 'selectField' )->willReturn( false );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		$filterLookup = $this->getLookup( $db );
 		$this->expectException( FilterNotFoundException::class );
 		$filterLookup->getFirstFilterVersionID( 42 );
@ -285,6 +285,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 	public function testLocalCache() {
 		$row = $this->getRowsAndFilters()['no actions']['row'];
 		$db = $this->createMock( DBConnRef::class );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		$db->expects( $this->once() )->method( 'selectRow' )->willReturn( $row );
 		$filterLookup = $this->getLookup( $db );

@ -300,6 +301,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 	public function testClearLocalCache() {
 		$row = $this->getRowsAndFilters()['no actions']['row'];
 		$db = $this->createMock( DBConnRef::class );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		$db->expects( $this->exactly( 2 ) )->method( 'selectRow' )->willReturn( $row );
 		$filterLookup = $this->getLookup( $db );

@ -329,6 +331,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 	public function testGlobalCache( bool $isCentral ) {
 		$row = $this->getRowsAndFilters()['no actions']['row'];
 		$db = $this->createMock( DBConnRef::class );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		// Should be called twice: once for the filter, once for the actions
 		$db->expects( $this->exactly( 2 ) )->method( 'select' )->willReturnOnConsecutiveCalls( [ $row ], [] );
 		$filterLookup = $this->getLookup( $db, 'foobar', null, $isCentral );
@ -355,6 +358,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 	public function testFilterLookupClearNetworkCache() {
 		$row = $this->getRowsAndFilters()['no actions']['row'];
 		$db = $this->createMock( DBConnRef::class );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		// 4 calls: row, actions, row, actions
 		$db->expects( $this->exactly( 4 ) )
 			->method( 'select' )
@ -385,8 +389,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 			new FilterLookup(
 				$this->createMock( ILoadBalancer::class ),
 				$this->createMock( WANObjectCache::class ),
-				$this->createMock( CentralDBManager::class ),
-				$this->createMock( ActorMigrationBase::class )
+				$this->createMock( CentralDBManager::class )
 			)
 		);
 	}
@ -436,6 +439,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase {
 	 */
 	public function testGetFilter_notfound() {
 		$db = $this->createMock( DBConnRef::class );
+		$db->method( 'newSelectQueryBuilder' )->willReturnCallback( static fn () => new SelectQueryBuilder( $db ) );
 		$db->method( 'selectRow' )->willReturn( false );
 		$filterLookup = $this->getLookup( $db );