rdbms: re-add DB domain sanity checks to LoadBalancer

Also clean up empty schema handling in DatabaseDomain

This reverts commit f23ac02f4fcf156767df66a5df2fa407310fe1d2.

Bug: T193565
Change-Id: I95fde5c069f180ca888a023fade25ec81b846d44

diff --git a/includes/libs/rdbms/database/DatabaseDomain.php b/includes/libs/rdbms/database/DatabaseDomain.php
index 6c9c54e..7559b06 100644 (file)
--- 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-<project>-<language>, 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 d84ba65..1f3fe4c 100644 (file)
--- 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;
@@ -952,6 +953,16 @@ class LoadBalancer implements ILoadBalancer {
                        }
                }
 
+               // Final sanity check to make sure the right domain is selected
+               if (
+                       $conn instanceof IDatabase &&
+                       !$this->localDomain->isCompatible( $conn->getDomainID() )
+               ) {
+                       throw new UnexpectedValueException(
+                               "Got connection to '{$conn->getDomainID()}', " .
+                               "but expected local domain ('{$this->localDomain}')." );
+               }
+
                return $conn;
        }
 
@@ -1038,8 +1049,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 b2e7155..b36fe11 100644 (file)
--- 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() );
+       }
 }