Convert PageEditStash to PageIdentity/UserIdentity

One tricky piece left when we get the Request from user
to check whether it was posted, needs a more thorrow
investigation.

Depends-On: Iab96a35be8f50fdbc66194bd8956d98b5b6b0032
Change-Id: I7164b914299441bd0f82e764252c8b5d30b45fbe

includes/ServiceWiring.php

@@ -943,6 +943,8 @@ return [
 			$services->getDBLoadBalancer(),
 			LoggerFactory::getInstance( 'StashEdit' ),
 			$services->getStatsdDataFactory(),
+			$services->getUserEditTracker(),
+			$services->getUserFactory(),
 			$services->getHookContainer(),
 			defined( 'MEDIAWIKI_JOB_RUNNER' ) || $config->get( 'CommandLineMode' )
 				? PageEditStash::INITIATOR_JOB_OR_CLI

includes/Storage/PageEditStash.php

@@ -28,11 +28,14 @@ use Content;
 use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface;
 use MediaWiki\HookContainer\HookContainer;
 use MediaWiki\HookContainer\HookRunner;
+use MediaWiki\Page\PageIdentity;
 use MediaWiki\Storage\Hook\ParserOutputStashForEditHook;
+use MediaWiki\User\UserEditTracker;
+use MediaWiki\User\UserFactory;
+use MediaWiki\User\UserIdentity;
 use ParserOutput;
 use Psr\Log\LoggerInterface;
 use stdClass;
-use Title;
 use User;
 use Wikimedia\Rdbms\ILoadBalancer;
 use Wikimedia\ScopedCallback;
@@ -54,6 +57,10 @@ class PageEditStash {
 	private $stats;
 	/** @var ParserOutputStashForEditHook */
 	private $hookRunner;
+	/** @var UserEditTracker */
+	private $userEditTracker;
+	/** @var UserFactory */
+	private $userFactory;
 	/** @var int */
 	private $initiator;
 
@@ -77,6 +84,8 @@ class PageEditStash {
 	 * @param ILoadBalancer $lb
 	 * @param LoggerInterface $logger
 	 * @param StatsdDataFactoryInterface $stats
+	 * @param UserEditTracker $userEditTracker
+	 * @param UserFactory $userFactory
 	 * @param HookContainer $hookContainer
 	 * @param int $initiator Class INITIATOR__* constant
 	 */
@@ -85,6 +94,8 @@ class PageEditStash {
 		ILoadBalancer $lb,
 		LoggerInterface $logger,
 		StatsdDataFactoryInterface $stats,
+		UserEditTracker $userEditTracker,
+		UserFactory $userFactory,
 		HookContainer $hookContainer,
 		$initiator
 	) {
@@ -92,6 +103,8 @@ class PageEditStash {
 		$this->lb = $lb;
 		$this->logger = $logger;
 		$this->stats = $stats;
+		$this->userEditTracker = $userEditTracker;
+		$this->userFactory = $userFactory;
 		$this->hookRunner = new HookRunner( $hookContainer );
 		$this->initiator = $initiator;
 	}
@@ -99,15 +112,14 @@ class PageEditStash {
 	/**
 	 * @param WikiPage $page
 	 * @param Content $content Edit content
-	 * @param User $user
+	 * @param UserIdentity $user
 	 * @param string $summary Edit summary
 	 * @return string Class ERROR_* constant
 	 */
-	public function parseAndCache( WikiPage $page, Content $content, User $user, string $summary ) {
+	public function parseAndCache( WikiPage $page, Content $content, UserIdentity $user, string $summary ) {
 		$logger = $this->logger;
 
-		$title = $page->getTitle();
-		$key = $this->getStashKey( $title, $this->getContentHash( $content ), $user );
+		$key = $this->getStashKey( $page, $this->getContentHash( $content ), $user );
 		$fname = __METHOD__;
 
 		// Use the master DB to allow for fast blocking locks on the "save path" where this
@@ -139,15 +151,16 @@ class PageEditStash {
 			$alreadyCached = false;
 		}
 
-		$context = [ 'cachekey' => $key, 'title' => $title->getPrefixedText() ];
+		$logContext = [ 'cachekey' => $key, 'title' => (string)$page ];
 
 		if ( $editInfo && $editInfo->output ) {
 			// Let extensions add ParserOutput metadata or warm other caches
+			$legacyUser = $this->userFactory->newFromUserIdentity( $user );
 			$this->hookRunner->onParserOutputStashForEdit(
-				$page, $content, $editInfo->output, $summary, $user );
+				$page, $content, $editInfo->output, $summary, $legacyUser );
 
 			if ( $alreadyCached ) {
-				$logger->debug( "Parser output for key '{cachekey}' already cached.", $context );
+				$logger->debug( "Parser output for key '{cachekey}' already cached.", $logContext );
 
 				return self::ERROR_NONE;
 			}
@@ -161,20 +174,20 @@ class PageEditStash {
 			);
 
 			if ( $code === true ) {
-				$logger->debug( "Cached parser output for key '{cachekey}'.", $context );
+				$logger->debug( "Cached parser output for key '{cachekey}'.", $logContext );
 
 				return self::ERROR_NONE;
 			} elseif ( $code === 'uncacheable' ) {
 				$logger->info(
 					"Uncacheable parser output for key '{cachekey}' [{code}].",
-					$context + [ 'code' => $code ]
+					$logContext + [ 'code' => $code ]
 				);
 
 				return self::ERROR_UNCACHEABLE;
 			} else {
 				$logger->error(
 					"Failed to cache parser output for key '{cachekey}'.",
-					$context + [ 'code' => $code ]
+					$logContext + [ 'code' => $code ]
 				);
 
 				return self::ERROR_CACHE;
@@ -199,12 +212,12 @@ class PageEditStash {
 	 *   - timestamp: the timestamp of the parse
 	 *   - edits: author edit count if they are logged in or NULL otherwise
 	 *
-	 * @param Title $title
+	 * @param PageIdentity $page
 	 * @param Content $content
-	 * @param User $user User to get parser options from
+	 * @param User $user to get parser options from
 	 * @return stdClass|bool Returns edit stash object or false on cache miss
 	 */
-	public function checkCache( Title $title, Content $content, User $user ) {
+	public function checkCache( PageIdentity $page, Content $content, User $user ) {
 		if (
 			// The context is not an HTTP POST request
 			!$user->getRequest()->wasPosted() ||
@@ -219,10 +232,10 @@ class PageEditStash {
 
 		$logger = $this->logger;
 
-		$key = $this->getStashKey( $title, $this->getContentHash( $content ), $user );
-		$context = [
+		$key = $this->getStashKey( $page, $this->getContentHash( $content ), $user );
+		$logContext = [
 			'key' => $key,
-			'title' => $title->getPrefixedText(),
+			'title' => (string)$page,
 			'user' => $user->getName()
 		];
 
@@ -230,43 +243,43 @@ class PageEditStash {
 		if ( !is_object( $editInfo ) || !$editInfo->output ) {
 			$this->incrStatsByContent( 'cache_misses.no_stash', $content );
 			if ( $this->recentStashEntryCount( $user ) > 0 ) {
-				$logger->info( "Empty cache for key '{key}' but not for user.", $context );
+				$logger->info( "Empty cache for key '{key}' but not for user.", $logContext );
 			} else {
-				$logger->debug( "Empty cache for key '{key}'.", $context );
+				$logger->debug( "Empty cache for key '{key}'.", $logContext );
 			}
 
 			return false;
 		}
 
 		$age = time() - (int)wfTimestamp( TS_UNIX, $editInfo->output->getCacheTime() );
-		$context['age'] = $age;
+		$logContext['age'] = $age;
 
 		$isCacheUsable = true;
 		if ( $age <= self::PRESUME_FRESH_TTL_SEC ) {
 			// Assume nothing changed in this time
 			$this->incrStatsByContent( 'cache_hits.presumed_fresh', $content );
-			$logger->debug( "Timestamp-based cache hit for key '{key}'.", $context );
+			$logger->debug( "Timestamp-based cache hit for key '{key}'.", $logContext );
 		} elseif ( $user->isAnon() ) {
 			$lastEdit = $this->lastEditTime( $user );
 			$cacheTime = $editInfo->output->getCacheTime();
 			if ( $lastEdit < $cacheTime ) {
 				// Logged-out user made no local upload/template edits in the meantime
 				$this->incrStatsByContent( 'cache_hits.presumed_fresh', $content );
-				$logger->debug( "Edit check based cache hit for key '{key}'.", $context );
+				$logger->debug( "Edit check based cache hit for key '{key}'.", $logContext );
 			} else {
 				$isCacheUsable = false;
 				$this->incrStatsByContent( 'cache_misses.proven_stale', $content );
-				$logger->info( "Stale cache for key '{key}' due to outside edits.", $context );
+				$logger->info( "Stale cache for key '{key}' due to outside edits.", $logContext );
 			}
 		} else {
-			if ( $editInfo->edits === $user->getEditCount() ) {
+			if ( $editInfo->edits === $this->userEditTracker->getUserEditCount( $user ) ) {
 				// Logged-in user made no local upload/template edits in the meantime
 				$this->incrStatsByContent( 'cache_hits.presumed_fresh', $content );
-				$logger->debug( "Edit count based cache hit for key '{key}'.", $context );
+				$logger->debug( "Edit count based cache hit for key '{key}'.", $logContext );
 			} else {
 				$isCacheUsable = false;
 				$this->incrStatsByContent( 'cache_misses.proven_stale', $content );
-				$logger->info( "Stale cache for key '{key}'due to outside edits.", $context );
+				$logger->info( "Stale cache for key '{key}'due to outside edits.", $logContext );
 			}
 		}
 
@@ -279,7 +292,7 @@ class PageEditStash {
 			// but a second parse will be triggered in doEditUpdates() no matter what
 			$logger->info(
 				"Cache for key '{key}' has vary-revision; post-insertion parse inevitable.",
-				$context
+				$logContext
 			);
 		} else {
 			static $flagsMaybeReparse = [
@@ -296,7 +309,7 @@ class PageEditStash {
 				if ( $editInfo->output->getFlag( $flag ) ) {
 					$logger->debug(
 						"Cache for key '{key}' has $flag; post-insertion parse possible.",
-						$context
+						$logContext
 					);
 				}
 			}
@@ -367,10 +380,10 @@ class PageEditStash {
 	}
 
 	/**
-	 * @param User $user
+	 * @param UserIdentity $user
 	 * @return string|null TS_MW timestamp or null
 	 */
-	private function lastEditTime( User $user ) {
+	private function lastEditTime( UserIdentity $user ) {
 		$db = $this->lb->getConnectionRef( DB_REPLICA );
 
 		$actorQuery = ActorMigration::newMigration()->getWhere( $db, 'rc_user', $user, false );
@@ -407,19 +420,19 @@ class PageEditStash {
 	 *   - a) The $user was used for PST options
 	 *   - b) The parser output was made from the PST using cannonical matching options
 	 *
-	 * @param Title $title
+	 * @param PageIdentity $page
 	 * @param string $contentHash Result of getContentHash()
-	 * @param User $user User to get parser options from
+	 * @param UserIdentity $user User to get parser options from
 	 * @return string
 	 */
-	private function getStashKey( Title $title, $contentHash, User $user ) {
+	private function getStashKey( PageIdentity $page, $contentHash, UserIdentity $user ) {
 		return $this->cache->makeKey(
 			'stashedit-info-v1',
-			md5( $title->getPrefixedDBkey() ),
+			md5( "{$page->getNamespace()}\n{$page->getDBkey()}" ),
 			// Account for the edit model/text
 			$contentHash,
 			// Account for user name related variables like signatures
-			md5( $user->getId() . "\n" . $user->getName() )
+			md5( "{$user->getId()}\n{$user->getName()}" )
 		);
 	}
 
@@ -445,7 +458,7 @@ class PageEditStash {
 	 * @param Content $pstContent Pre-Save transformed content
 	 * @param ParserOutput $parserOutput
 	 * @param string $timestamp TS_MW
-	 * @param User $user
+	 * @param UserIdentity $user
 	 * @return string|bool True or an error code
 	 */
 	private function storeStashValue(
@@ -453,7 +466,7 @@ class PageEditStash {
 		Content $pstContent,
 		ParserOutput $parserOutput,
 		$timestamp,
-		User $user
+		UserIdentity $user
 	) {
 		// If an item is renewed, mind the cache TTL determined by config and parser functions.
 		// Put an upper limit on the TTL for sanity to avoid extreme template/file staleness.
@@ -473,7 +486,7 @@ class PageEditStash {
 			'pstContent' => $pstContent,
 			'output'     => $parserOutput,
 			'timestamp'  => $timestamp,
-			'edits'      => $user->getEditCount()
+			'edits'      => $user->isRegistered() ? $this->userEditTracker->getUserEditCount( $user ) : null,
 		];
 
 		$ok = $this->cache->set( $key, $stashInfo, $ttl, BagOStuff::WRITE_ALLOW_SEGMENTS );
@@ -486,10 +499,10 @@ class PageEditStash {
 	}
 
 	/**
-	 * @param User $user
+	 * @param UserIdentity $user
 	 * @param string $newKey
 	 */
-	private function pruneExcessStashedEntries( User $user, $newKey ) {
+	private function pruneExcessStashedEntries( UserIdentity $user, $newKey ) {
 		$key = $this->cache->makeKey( 'stash-edit-recent', sha1( $user->getName() ) );
 
 		$keyList = $this->cache->get( $key ) ?: [];
@@ -503,10 +516,10 @@ class PageEditStash {
 	}
 
 	/**
-	 * @param User $user
+	 * @param UserIdentity $user
 	 * @return int
 	 */
-	private function recentStashEntryCount( User $user ) {
+	private function recentStashEntryCount( UserIdentity $user ) {
 		$key = $this->cache->makeKey( 'stash-edit-recent', sha1( $user->getName() ) );
 
 		return count( $this->cache->get( $key ) ?: [] );

includes/api/ApiStashEdit.php

@@ -22,6 +22,7 @@ use MediaWiki\Content\IContentHandlerFactory;
 use MediaWiki\Revision\RevisionLookup;
 use MediaWiki\Revision\SlotRecord;
 use MediaWiki\Storage\PageEditStash;
+use MediaWiki\User\UserIdentity;
 
 /**
  * Prepare an edit in shared cache so that it can be reused on edit
@@ -204,13 +205,13 @@ class ApiStashEdit extends ApiBase {
 	/**
 	 * @param WikiPage $page
 	 * @param Content $content Edit content
-	 * @param User $user
+	 * @param UserIdentity $user
 	 * @param string $summary Edit summary
 	 * @return string ApiStashEdit::ERROR_* constant
 	 * @since 1.25
 	 * @deprecated Since 1.34
 	 */
-	public function parseAndStash( WikiPage $page, Content $content, User $user, $summary ) {
+	public function parseAndStash( WikiPage $page, Content $content, UserIdentity $user, $summary ) {
 		return $this->pageEditStash->parseAndCache( $page, $content, $user, $summary ?? '' );
 	}
 

tests/phpunit/includes/api/ApiStashEditTest.php

@@ -15,11 +15,19 @@ use Wikimedia\TestingAccessWrapper;
 class ApiStashEditTest extends ApiTestCase {
 	protected function setUp() : void {
 		parent::setUp();
+		// Hack to make user edit tracker survive service reset.
+		// We want it's cache to persist within tests run, otherwise
+		// incorrect in-process cache is being reset, and we get outdated
+		// edit counts.
+		$this->setService( 'UserEditTracker', $this->getServiceContainer()
+			->getUserEditTracker() );
 		$this->setService( 'PageEditStash', new PageEditStash(
 			new HashBagOStuff( [] ),
 			MediaWikiServices::getInstance()->getDBLoadBalancer(),
 			new NullLogger(),
 			new NullStatsdDataFactory(),
+			MediaWikiServices::getInstance()->getUserEditTracker(),
+			MediaWikiServices::getInstance()->getUserFactory(),
 			MediaWikiServices::getInstance()->getHookContainer(),
 			PageEditStash::INITIATOR_USER
 		) );