Merge "Avoid using stale data for revision visibility and actor data"
[lhc/web/wiklou.git] / includes / Revision / RevisionStore.php
index fc1f6df..632bd31 100644 (file)
@@ -132,6 +132,9 @@ class RevisionStore
        /** @var int An appropriate combination of SCHEMA_COMPAT_XXX flags. */
        private $mcrMigrationStage;
 
+       /** @var SlotRoleRegistry */
+       private $slotRoleRegistry;
+
        /**
         * @todo $blobStore should be allowed to be any BlobStore!
         *
@@ -146,11 +149,11 @@ class RevisionStore
         * @param CommentStore $commentStore
         * @param NameTableStore $contentModelStore
         * @param NameTableStore $slotRoleStore
+        * @param SlotRoleRegistry $slotRoleRegistry
         * @param int $mcrMigrationStage An appropriate combination of SCHEMA_COMPAT_XXX flags
         * @param ActorMigration $actorMigration
         * @param bool|string $wikiId
         *
-        * @throws MWException if $mcrMigrationStage or $wikiId is invalid.
         */
        public function __construct(
                ILoadBalancer $loadBalancer,
@@ -159,6 +162,7 @@ class RevisionStore
                CommentStore $commentStore,
                NameTableStore $contentModelStore,
                NameTableStore $slotRoleStore,
+               SlotRoleRegistry $slotRoleRegistry,
                $mcrMigrationStage,
                ActorMigration $actorMigration,
                $wikiId = false
@@ -199,6 +203,7 @@ class RevisionStore
                $this->commentStore = $commentStore;
                $this->contentModelStore = $contentModelStore;
                $this->slotRoleStore = $slotRoleStore;
+               $this->slotRoleRegistry = $slotRoleRegistry;
                $this->mcrMigrationStage = $mcrMigrationStage;
                $this->actorMigration = $actorMigration;
                $this->wikiId = $wikiId;
@@ -923,7 +928,7 @@ class RevisionStore
                $format = $content->getDefaultFormat();
                $model = $content->getModel();
 
-               $this->checkContent( $content, $title );
+               $this->checkContent( $content, $title, $slot->getRole() );
 
                return $this->blobStore->storeBlob(
                        $content->serialize( $format ),
@@ -982,11 +987,12 @@ class RevisionStore
         *
         * @param Content $content
         * @param Title $title
+        * @param string $role
         *
         * @throws MWException
         * @throws MWUnknownContentModelException
         */
-       private function checkContent( Content $content, Title $title ) {
+       private function checkContent( Content $content, Title $title, $role ) {
                // Note: may return null for revisions that have not yet been inserted
 
                $model = $content->getModel();
@@ -1005,7 +1011,8 @@ class RevisionStore
 
                        $this->assertCrossWikiContentLoadingIsSafe();
 
-                       $defaultModel = ContentHandler::getDefaultModelFor( $title );
+                       $roleHandler = $this->slotRoleRegistry->getRoleHandler( $role );
+                       $defaultModel = $roleHandler->getDefaultModel( $title );
                        $defaultHandler = ContentHandler::getForModelID( $defaultModel );
                        $defaultFormat = $defaultHandler->getDefaultFormat();
 
@@ -1350,9 +1357,8 @@ class RevisionStore
                        $mainSlotRow->model_name = function ( SlotRecord $slot ) use ( $title ) {
                                $this->assertCrossWikiContentLoadingIsSafe();
 
-                               // TODO: MCR: consider slot role in getDefaultModelFor()! Use LinkTarget!
-                               // TODO: MCR: deprecate $title->getModel().
-                               return ContentHandler::getDefaultModelFor( $title );
+                               return $this->slotRoleRegistry->getRoleHandler( $slot->getRole() )
+                                       ->getDefaultModel( $title );
                        };
                }
 
@@ -1754,10 +1760,16 @@ class RevisionStore
         * @param object $row
         * @param int $queryFlags
         * @param Title|null $title
-        *
+        * @param bool $fromCache if true, the returned RevisionRecord will ensure that no stale
+        *   data is returned from getters, by querying the database as needed
         * @return RevisionRecord
         */
-       public function newRevisionFromRow( $row, $queryFlags = 0, Title $title = null ) {
+       public function newRevisionFromRow(
+               $row,
+               $queryFlags = 0,
+               Title $title = null,
+               $fromCache = false
+       ) {
                Assert::parameterType( 'object', $row, '$row' );
 
                if ( !$title ) {
@@ -1791,7 +1803,23 @@ class RevisionStore
 
                $slots = $this->newRevisionSlots( $row->rev_id, $row, $queryFlags, $title );
 
-               return new RevisionStoreRecord( $title, $user, $comment, $row, $slots, $this->wikiId );
+               // If this is a cached row, instantiate a cache-aware revision class to avoid stale data.
+               if ( $fromCache ) {
+                       $rev = new RevisionStoreCacheRecord(
+                               function ( $revId ) use ( $queryFlags ) {
+                                       $db = $this->getDBConnectionRefForQueryFlags( $queryFlags );
+                                       return $this->fetchRevisionRowFromConds(
+                                               $db,
+                                               [ 'rev_id' => intval( $revId ) ]
+                                       );
+                               },
+                               $title, $user, $comment, $row, $slots, $this->wikiId
+                       );
+               } else {
+                       $rev = new RevisionStoreRecord(
+                               $title, $user, $comment, $row, $slots, $this->wikiId );
+               }
+               return $rev;
        }
 
        /**
@@ -2302,7 +2330,7 @@ class RevisionStore
                                'page_is_redirect',
                                'page_len',
                        ] );
-                       $ret['joins']['page'] = [ 'INNER JOIN', [ 'page_id = rev_page' ] ];
+                       $ret['joins']['page'] = [ 'JOIN', [ 'page_id = rev_page' ] ];
                }
 
                if ( in_array( 'user', $options, true ) ) {
@@ -2331,7 +2359,7 @@ class RevisionStore
                                'old_text',
                                'old_flags'
                        ] );
-                       $ret['joins']['text'] = [ 'INNER JOIN', [ 'rev_text_id=old_id' ] ];
+                       $ret['joins']['text'] = [ 'JOIN', [ 'rev_text_id=old_id' ] ];
                }
 
                return $ret;
@@ -2407,7 +2435,7 @@ class RevisionStore
                                        'content_address',
                                        'content_model',
                                ] );
-                               $ret['joins']['content'] = [ 'INNER JOIN', [ 'slot_content_id = content_id' ] ];
+                               $ret['joins']['content'] = [ 'JOIN', [ 'slot_content_id = content_id' ] ];
 
                                if ( in_array( 'model', $options, true ) ) {
                                        // Use left join to attach model name, so we still find the revision row even
@@ -2517,45 +2545,78 @@ class RevisionStore
        }
 
        /**
-        * Get previous revision for this title
+        * Get the revision before $rev in the page's history, if any.
+        * Will return null for the first revision but also for deleted or unsaved revisions.
         *
         * MCR migration note: this replaces Revision::getPrevious
         *
+        * @see Title::getPreviousRevisionID
+        * @see PageArchive::getPreviousRevision
+        *
         * @param RevisionRecord $rev
         * @param Title|null $title if known (optional)
         *
         * @return RevisionRecord|null
         */
        public function getPreviousRevision( RevisionRecord $rev, Title $title = null ) {
+               if ( !$rev->getId() || !$rev->getPageId() ) {
+                       // revision is unsaved or otherwise incomplete
+                       return null;
+               }
+
+               if ( $rev instanceof RevisionArchiveRecord ) {
+                       // revision is deleted, so it's not part of the page history
+                       return null;
+               }
+
                if ( $title === null ) {
+                       // this would fail for deleted revisions
                        $title = $this->getTitle( $rev->getPageId(), $rev->getId() );
                }
+
                $prev = $title->getPreviousRevisionID( $rev->getId() );
-               if ( $prev ) {
-                       return $this->getRevisionByTitle( $title, $prev );
+               if ( !$prev ) {
+                       return null;
                }
-               return null;
+
+               return $this->getRevisionByTitle( $title, $prev );
        }
 
        /**
-        * Get next revision for this title
+        * Get the revision after $rev in the page's history, if any.
+        * Will return null for the latest revision but also for deleted or unsaved revisions.
         *
         * MCR migration note: this replaces Revision::getNext
         *
+        * @see Title::getNextRevisionID
+        *
         * @param RevisionRecord $rev
         * @param Title|null $title if known (optional)
         *
         * @return RevisionRecord|null
         */
        public function getNextRevision( RevisionRecord $rev, Title $title = null ) {
+               if ( !$rev->getId() || !$rev->getPageId() ) {
+                       // revision is unsaved or otherwise incomplete
+                       return null;
+               }
+
+               if ( $rev instanceof RevisionArchiveRecord ) {
+                       // revision is deleted, so it's not part of the page history
+                       return null;
+               }
+
                if ( $title === null ) {
+                       // this would fail for deleted revisions
                        $title = $this->getTitle( $rev->getPageId(), $rev->getId() );
                }
+
                $next = $title->getNextRevisionID( $rev->getId() );
-               if ( $next ) {
-                       return $this->getRevisionByTitle( $title, $next );
+               if ( !$next ) {
+                       return null;
                }
-               return null;
+
+               return $this->getRevisionByTitle( $title, $next );
        }
 
        /**
@@ -2734,27 +2795,29 @@ class RevisionStore
                        return false;
                }
 
+               // Load the row from cache if possible.  If not possible, populate the cache.
+               // As a minor optimization, remember if this was a cache hit or miss.
+               // We can sometimes avoid a database query later if this is a cache miss.
+               $fromCache = true;
                $row = $this->cache->getWithSetCallback(
                        // Page/rev IDs passed in from DB to reflect history merges
                        $this->getRevisionRowCacheKey( $db, $pageId, $revId ),
                        WANObjectCache::TTL_WEEK,
-                       function ( $curValue, &$ttl, array &$setOpts ) use ( $db, $pageId, $revId ) {
+                       function ( $curValue, &$ttl, array &$setOpts ) use (
+                               $db, $pageId, $revId, &$fromCache
+                       ) {
                                $setOpts += Database::getCacheSetOptions( $db );
-
-                               $conds = [
-                                       'rev_page' => intval( $pageId ),
-                                       'page_id' => intval( $pageId ),
-                                       'rev_id' => intval( $revId ),
-                               ];
-
-                               $row = $this->fetchRevisionRowFromConds( $db, $conds );
-                               return $row ?: false; // don't cache negatives
+                               $row = $this->fetchRevisionRowFromConds( $db, [ 'rev_id' => intval( $revId ) ] );
+                               if ( $row ) {
+                                       $fromCache = false;
+                               }
+                               return $row; // don't cache negatives
                        }
                );
 
-               // Reflect revision deletion and user renames
+               // Reflect revision deletion and user renames.
                if ( $row ) {
-                       return $this->newRevisionFromRow( $row, 0, $title );
+                       return $this->newRevisionFromRow( $row, 0, $title, $fromCache );
                } else {
                        return false;
                }