From 90e7c3592e96f42160375382d2dfd8bda1923a99 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sat, 13 Oct 2018 16:14:20 +0000 Subject: [PATCH] rdbms: re-add DB domain sanity checks to LoadBalancer Also clean up empty schema handling in DatabaseDomain This reverts commit f23ac02f4fcf156767df66a5df2fa407310fe1d2. Bug: T193565 Bug: T234022 Change-Id: I95fde5c069f180ca888a023fade25ec81b846d44 --- .../libs/rdbms/database/DatabaseDomain.php | 53 +++++++++++- .../libs/rdbms/loadbalancer/LoadBalancer.php | 8 +- .../rdbms/database/DatabaseDomainTest.php | 83 +++++++++++++++++++ 3 files changed, 140 insertions(+), 4 deletions(-) diff --git a/includes/libs/rdbms/database/DatabaseDomain.php b/includes/libs/rdbms/database/DatabaseDomain.php index ef6600b4fc..b9c06343eb 100644 --- a/includes/libs/rdbms/database/DatabaseDomain.php +++ b/includes/libs/rdbms/database/DatabaseDomain.php @@ -84,6 +84,10 @@ class DatabaseDomain { $database = null; } + if ( $schema === '' ) { + $schema = null; + } + return new self( $database, $schema, $prefix ); } @@ -96,10 +100,10 @@ class DatabaseDomain { /** * @param DatabaseDomain|string $other - * @return bool + * @return bool Whether the domain instances are the same by value */ public function equals( $other ) { - if ( $other instanceof DatabaseDomain ) { + if ( $other instanceof self ) { return ( $this->database === $other->database && $this->schema === $other->schema && @@ -110,6 +114,44 @@ class DatabaseDomain { return ( $this->getId() === $other ); } + /** + * Check whether the domain $other meets the specifications of this domain + * + * If this instance has a null database specifier, then $other can have any database + * specified, including the null, and likewise if the schema specifier is null. This + * is not transitive like equals() since a domain that explicitly wants a certain + * database or schema cannot be satisfied by one of another (nor null). If the prefix + * is empty and the DB and schema are both null, then the entire domain is considered + * unspecified, and any prefix of $other is considered compatible. + * + * @param DatabaseDomain|string $other + * @return bool + * @since 1.32 + */ + public function isCompatible( $other ) { + if ( $this->isUnspecified() ) { + return true; // even the prefix doesn't matter + } + + $other = ( $other instanceof self ) ? $other : self::newFromId( $other ); + + return ( + ( $this->database === $other->database || $this->database === null ) && + ( $this->schema === $other->schema || $this->schema === null ) && + $this->prefix === $other->prefix + ); + } + + /** + * @return bool + * @since 1.32 + */ + public function isUnspecified() { + return ( + $this->database === null && $this->schema === null && $this->prefix === '' + ); + } + /** * @return string|null Database name */ @@ -150,7 +192,12 @@ class DatabaseDomain { if ( $this->schema !== null ) { $parts[] = $this->schema; } - if ( $this->prefix != '' ) { + if ( $this->prefix != '' || $this->schema !== null ) { + // If there is a schema, then we need the prefix to disambiguate. + // For engines like Postgres that use schemas, this awkwardness is hopefully + // avoided since it is easy to have one DB per server (to avoid having many users) + // and use schema/prefix to have wiki farms. For example, a domain schemes could be + // wiki--, e.g. "wiki-fitness-es"/"wiki-sports-fr"/"wiki-news-en". $parts[] = $this->prefix; } diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index d5e65cdddf..672c3bd8b7 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -28,6 +28,7 @@ use BagOStuff; use EmptyBagOStuff; use WANObjectCache; use ArrayUtils; +use UnexpectedValueException; use InvalidArgumentException; use RuntimeException; use Exception; @@ -998,8 +999,13 @@ class LoadBalancer implements ILoadBalancer { } } - // Increment reference count if ( $conn instanceof IDatabase ) { + // Final sanity check to make sure the right domain is selected + if ( !$domainInstance->isCompatible( $conn->getDomainID() ) ) { + throw new UnexpectedValueException( + "Got connection to '{$conn->getDomainID()}', but expected '$domain'." ); + } + // Increment reference count $refCount = $conn->getLBInfo( 'foreignPoolRefCount' ); $conn->setLBInfo( 'foreignPoolRefCount', $refCount + 1 ); } diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseDomainTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseDomainTest.php index b2e7155400..b36fe11f85 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseDomainTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseDomainTest.php @@ -130,4 +130,87 @@ class DatabaseDomainTest extends PHPUnit\Framework\TestCase { $this->assertSame( null, $domain->getSchema() ); $this->assertSame( '', $domain->getTablePrefix() ); } + + public static function provideIsCompatible() { + return [ + 'Basic' => + [ 'foo', 'foo', null, '', true ], + 'db+prefix' => + [ 'foo-bar', 'foo', null, 'bar', true ], + 'db+schema+prefix' => + [ 'foo-bar-baz', 'foo', 'bar', 'baz', true ], + 'db+dontcare_schema+prefix' => + [ 'foo-bar-baz', 'foo', null, 'baz', false ], + '?h -> -' => + [ 'foo?hbar-baz-baa', 'foo-bar', 'baz', 'baa', true ], + '?? -> ?' => + [ 'foo??bar-baz-baa', 'foo?bar', 'baz', 'baa', true ], + 'Nothing' => + [ '', null, null, '', true ], + 'dontcaredb+dontcaredbschema+prefix' => + [ 'mywiki-mediawiki-prefix', null, null, 'prefix', false ], + 'dontcaredb+schema+prefix' => + [ 'mywiki-schema-prefix', null, 'schema', 'prefix', false ], + 'db+dontcareschema+prefix' => + [ 'mywiki-schema-prefix', 'mywiki', null, 'prefix', false ], + 'postgres-db-jobqueue' => + [ 'postgres-mediawiki-', 'postgres', null, '', false ] + ]; + } + + /** + * @dataProvider provideIsCompatible + * @covers Wikimedia\Rdbms\DatabaseDomain::isCompatible + */ + public function testIsCompatible( $id, $db, $schema, $prefix, $transitive ) { + $compareIdObj = DatabaseDomain::newFromId( $id ); + $this->assertInstanceOf( DatabaseDomain::class, $compareIdObj ); + + $fromId = new DatabaseDomain( $db, $schema, $prefix ); + + $this->assertTrue( $fromId->isCompatible( $id ), 'constructed equals string' ); + $this->assertTrue( $fromId->isCompatible( $compareIdObj ), 'fromId equals string' ); + + $this->assertEquals( $transitive, $compareIdObj->isCompatible( $fromId ), + 'test transitivity of nulls components' ); + } + + public static function provideIsCompatible2() { + return [ + 'db+schema+prefix' => + [ 'mywiki-schema-prefix', 'thatwiki', 'schema', 'prefix' ], + 'dontcaredb+dontcaredbschema+prefix' => + [ 'thatwiki-mediawiki-otherprefix', null, null, 'prefix' ], + 'dontcaredb+schema+prefix' => + [ 'mywiki-otherschema-prefix', null, 'schema', 'prefix' ], + 'db+dontcareschema+prefix' => + [ 'notmywiki-schema-prefix', 'mywiki', null, 'prefix' ], + ]; + } + + /** + * @dataProvider provideIsCompatible2 + * @covers Wikimedia\Rdbms\DatabaseDomain::isCompatible + */ + public function testIsCompatible2( $id, $db, $schema, $prefix ) { + $compareIdObj = DatabaseDomain::newFromId( $id ); + $this->assertInstanceOf( DatabaseDomain::class, $compareIdObj ); + + $fromId = new DatabaseDomain( $db, $schema, $prefix ); + + $this->assertFalse( $fromId->isCompatible( $id ), 'constructed equals string' ); + $this->assertFalse( $fromId->isCompatible( $compareIdObj ), 'fromId equals string' ); + } + + /** + * @covers Wikimedia\Rdbms\DatabaseDomain::isUnspecified + */ + public function testIsUnspecified() { + $domain = new DatabaseDomain( null, null, '' ); + $this->assertTrue( $domain->isUnspecified() ); + $domain = new DatabaseDomain( 'mywiki', null, '' ); + $this->assertFalse( $domain->isUnspecified() ); + $domain = new DatabaseDomain( 'mywiki', null, '' ); + $this->assertFalse( $domain->isUnspecified() ); + } } -- 2.20.1