From e9539efe2ca9a56e40e53ce41d89b52fdc8f0099 Mon Sep 17 00:00:00 2001 From: "C. Scott Ananian" Date: Sun, 20 Jul 2025 14:39:39 -1000 Subject: [PATCH] Use JsonCodec to serialize SelserContext This cleans up a FIXME left over from I9e6b924d62ccc3312f5c70989477da1e2f21c86b. SimpleParsoidOutputStashTest was temporary changed from a unit test to an integration test, since the serialization/deserialization mechanism for Content relies on ContentHandlerFactory in a way which is difficult to unit test. This will be restored in I0cc1fc1b7403674467d85618b38a3b5a4718b66e once native JSON serialization for Content is landed. Follows-Up: I9e6b924d62ccc3312f5c70989477da1e2f21c86b Change-Id: If985e99f9ca9596d0fe40f0a5ef2cdb72286627d (cherry picked from commit 2ebf7e12df28f9861bb204ff4134871089a1c771) --- includes/ServiceWiring.php | 2 +- includes/edit/SelserContext.php | 70 ++++++++--- includes/edit/SimpleParsoidOutputStash.php | 78 +++--------- includes/json/JsonCodec.php | 35 ++++++ .../Rest/Handler/HTMLHandlerTestTrait.php | 4 +- .../Helper/HtmlOutputRendererHelperTest.php | 13 +- ...pleParsoidOutputStashSerializationTest.php | 115 +++++++++--------- .../edit/SimpleParsoidOutputStashTest.php | 27 ++-- 8 files changed, 180 insertions(+), 164 deletions(-) rename tests/phpunit/{unit => integration}/includes/edit/SimpleParsoidOutputStashTest.php (54%) diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 1e9e7927af1..af33de1f2c7 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -1660,7 +1660,7 @@ return [ : $services->getMainObjectStash(); return new SimpleParsoidOutputStash( - $services->getContentHandlerFactory(), + $services->getJsonCodec(), $backend, $config['StashDuration'] ); diff --git a/includes/edit/SelserContext.php b/includes/edit/SelserContext.php index 5933e5f0d7e..dc4c426f4a4 100644 --- a/includes/edit/SelserContext.php +++ b/includes/edit/SelserContext.php @@ -3,7 +3,10 @@ namespace MediaWiki\Edit; use MediaWiki\Content\Content; +use MediaWiki\MediaWikiServices; use UnexpectedValueException; +use Wikimedia\JsonCodec\JsonCodecable; +use Wikimedia\JsonCodec\JsonCodecableTrait; use Wikimedia\Parsoid\Core\PageBundle; use Wikimedia\Parsoid\Core\SelserData; @@ -15,29 +18,20 @@ use Wikimedia\Parsoid\Core\SelserData; * * @since 1.40 */ -class SelserContext { - private PageBundle $pageBundle; +class SelserContext implements JsonCodecable { + use JsonCodecableTrait; - private int $revId; - - private ?Content $content; - - /** - * @param PageBundle $pageBundle - * @param int $revId - * @param Content|null $content - */ - public function __construct( PageBundle $pageBundle, int $revId, ?Content $content = null ) { + public function __construct( + private PageBundle $pageBundle, + private int $revId, + private ?Content $content = null + ) { if ( !$revId && !$content ) { throw new UnexpectedValueException( 'If $revId is 0, $content must be given. ' . 'If we can\'t load the content from a revision, we have to stash it.' ); } - - $this->pageBundle = $pageBundle; - $this->revId = $revId; - $this->content = $content; } /** @@ -61,4 +55,48 @@ class SelserContext { return $this->content; } + public function toJsonArray(): array { + return [ + 'revId' => $this->revId, + 'pb' => $this->pageBundle, + // After I544625136088164561b9169a63aed7450cce82f5 this can be: + // 'c' => $this->content, + 'content' => $this->content ? [ + 'model' => $this->content->getModel(), + 'data' => $this->content->serialize(), + ] : null, + ]; + } + + public static function jsonClassHintFor( string $keyName ): ?string { + if ( $keyName === 'pb' ) { + return PageBundle::class; + } + return null; + } + + public static function newFromJsonArray( array $json ): self { + $revId = (int)$json['revId']; + $pb = $json['pb']; + if ( is_array( $pb ) ) { + // Backward compatibility with old serialization format + $pb = new PageBundle( + $pb['html'], + $pb['parsoid'] ?? null, + $pb['mw'] ?? null, + $pb['version'] ?? null, + $pb['headers'] ?? null, + $pb['contentmodel'] ?? null + ); + } + $content = $json['c'] ?? $json['content'] ?? null; + if ( is_array( $content ) ) { + // Backward compatibility with old serialization format + $contentHandler = MediaWikiServices::getInstance() + ->getContentHandlerFactory() + ->getContentHandler( $content['model'] ); + $content = $contentHandler->unserializeContent( $content['data'] ); + } + return new self( $pb, $revId, $content ); + } } diff --git a/includes/edit/SimpleParsoidOutputStash.php b/includes/edit/SimpleParsoidOutputStash.php index a19db49e7b4..0c74fe95303 100644 --- a/includes/edit/SimpleParsoidOutputStash.php +++ b/includes/edit/SimpleParsoidOutputStash.php @@ -2,7 +2,7 @@ namespace MediaWiki\Edit; -use MediaWiki\Content\IContentHandlerFactory; +use MediaWiki\Json\JsonCodec; use MediaWiki\Parser\Parsoid\PageBundleJsonTrait; use Wikimedia\ObjectCache\BagOStuff; @@ -13,24 +13,13 @@ use Wikimedia\ObjectCache\BagOStuff; class SimpleParsoidOutputStash implements ParsoidOutputStash { use PageBundleJsonTrait; - /** @var BagOStuff */ - private $bagOfStuff; - - /** @var int */ - private $duration; - - /** @var IContentHandlerFactory */ - private $contentHandlerFactory; - - /** - * @param IContentHandlerFactory $contentHandlerFactory - * @param BagOStuff $bagOfStuff storage backend - * @param int $duration cache duration in seconds - */ - public function __construct( IContentHandlerFactory $contentHandlerFactory, BagOStuff $bagOfStuff, int $duration ) { - $this->bagOfStuff = $bagOfStuff; - $this->duration = $duration; - $this->contentHandlerFactory = $contentHandlerFactory; + public function __construct( + private JsonCodec $jsonCodec, + /** Storage backend */ + private BagOStuff $bagOfStuff, + /** Cache duration in seconds */ + private int $duration, + ) { } private function makeCacheKey( ParsoidRenderID $renderId ): string { @@ -47,8 +36,9 @@ class SimpleParsoidOutputStash implements ParsoidOutputStash { * @return bool */ public function set( ParsoidRenderID $renderId, SelserContext $selserContext ): bool { - $jsonic = $this->selserContextToJsonArray( $selserContext ); - + $jsonic = $this->jsonCodec->toJsonArray( + $selserContext, SelserContext::class + ); $key = $this->makeCacheKey( $renderId ); return $this->bagOfStuff->set( $key, $jsonic, $this->duration ); } @@ -71,50 +61,12 @@ class SimpleParsoidOutputStash implements ParsoidOutputStash { // Only needed for a couple of days after this code has been deployed. return null; } - - $selserContext = $this->newSelserContextFromJson( $jsonic ); - return $selserContext ?: null; - } - - private function newSelserContextFromJson( array $json ): ?SelserContext { - if ( !isset( $json['pb'] ) ) { + if ( !isset( $jsonic['pb'] ) ) { return null; } - - $pb = $this->newPageBundleFromJson( $json['pb'] ); - - if ( !$pb ) { - return null; - } - - $revId = (int)$json['revId']; - - if ( isset( $json['content'] ) ) { - $contentHandler = $this->contentHandlerFactory->getContentHandler( $json['content']['model'] ); - $content = $contentHandler->unserializeContent( $json['content']['data'] ); - } else { - $content = null; - } - - return new SelserContext( $pb, $revId, $content ); - } - - private function selserContextToJsonArray( SelserContext $selserContext ): array { - $json = [ - 'revId' => $selserContext->getRevisionID(), - ]; - - $json['pb'] = $this->jsonSerializePageBundle( $selserContext->getPageBundle() ); - - $content = $selserContext->getContent(); - if ( $content ) { - $json['content'] = [ - 'model' => $content->getModel(), - 'data' => $content->serialize() - ]; - } - - return $json; + return $this->jsonCodec->newFromJsonArray( + $jsonic, SelserContext::class + ); } } diff --git a/includes/json/JsonCodec.php b/includes/json/JsonCodec.php index 0ec82526cfe..c38906e920e 100644 --- a/includes/json/JsonCodec.php +++ b/includes/json/JsonCodec.php @@ -30,6 +30,7 @@ use stdClass; use Wikimedia\Assert\Assert; use Wikimedia\JsonCodec\JsonClassCodec; use Wikimedia\JsonCodec\JsonCodecable; +use Wikimedia\Parsoid\Core\PageBundle; /** * Helper class to serialize/deserialize things to/from JSON. @@ -55,6 +56,40 @@ class JsonCodec */ public function __construct( ?ContainerInterface $services = null ) { parent::__construct( $services ); + // Forward-compatibility with MW 1.44: ensure Parsoid PageBundle is + // codecable + $this->addCodecFor( PageBundle::class, new class implements JsonClassCodec { + /** @inheritDoc */ + public function toJsonArray( $obj ): array { + '@phan-var PageBundle $obj'; /** @var PageBundle $obj */ + return [ + 'html' => $obj->html, + 'parsoid' => $obj->parsoid, + 'mw' => $obj->mw, + 'version' => $obj->version, + 'headers' => $obj->headers, + 'contentmodel' => $obj->contentmodel, + ]; + } + + /** @inheritDoc */ + public function newFromJsonArray( string $className, array $json ) { + // @phan-suppress-next-line PhanTypeMismatchReturn + return new PageBundle( + $json['html'] ?? '', + $json['parsoid'] ?? null, + $json['mw'] ?? null, + $json['version'] ?? null, + $json['headers'] ?? null, + $json['contentmodel'] ?? null + ); + } + + /** @inheritDoc */ + public function jsonClassHintFor( string $className, string $keyName ) { + return null; + } + } ); } /** diff --git a/tests/phpunit/integration/includes/Rest/Handler/HTMLHandlerTestTrait.php b/tests/phpunit/integration/includes/Rest/Handler/HTMLHandlerTestTrait.php index ed6ba434fb6..a6eea19ab33 100644 --- a/tests/phpunit/integration/includes/Rest/Handler/HTMLHandlerTestTrait.php +++ b/tests/phpunit/integration/includes/Rest/Handler/HTMLHandlerTestTrait.php @@ -27,8 +27,8 @@ trait HTMLHandlerTestTrait { private function getParsoidOutputStash(): ParsoidOutputStash { if ( !$this->parsoidOutputStash ) { - $chFactory = $this->getServiceContainer()->getContentHandlerFactory(); - $this->parsoidOutputStash = new SimpleParsoidOutputStash( $chFactory, new HashBagOStuff(), 120 ); + $jsonCodec = $this->getServiceContainer()->getJsonCodec(); + $this->parsoidOutputStash = new SimpleParsoidOutputStash( $jsonCodec, new HashBagOStuff(), 120 ); } return $this->parsoidOutputStash; } diff --git a/tests/phpunit/integration/includes/Rest/Handler/Helper/HtmlOutputRendererHelperTest.php b/tests/phpunit/integration/includes/Rest/Handler/Helper/HtmlOutputRendererHelperTest.php index 7a0154471ff..b552eded2a1 100644 --- a/tests/phpunit/integration/includes/Rest/Handler/Helper/HtmlOutputRendererHelperTest.php +++ b/tests/phpunit/integration/includes/Rest/Handler/Helper/HtmlOutputRendererHelperTest.php @@ -4,7 +4,6 @@ namespace MediaWiki\Tests\Rest\Handler\Helper; use Exception; use MediaWiki\Content\CssContent; -use MediaWiki\Content\IContentHandlerFactory; use MediaWiki\Content\WikitextContent; use MediaWiki\Deferred\DeferredUpdates; use MediaWiki\Edit\ParsoidRenderID; @@ -232,9 +231,9 @@ class HtmlOutputRendererHelperTest extends MediaWikiIntegrationTestCase { $revision = null, bool $lenientRevHandling = false ): HtmlOutputRendererHelper { - $chFactory = $this->getServiceContainer()->getContentHandlerFactory(); + $jsonCodec = $this->getServiceContainer()->getJsonCodec(); $cache = $options['cache'] ?? new EmptyBagOStuff(); - $stash = new SimpleParsoidOutputStash( $chFactory, $cache, 1 ); + $stash = new SimpleParsoidOutputStash( $jsonCodec, $cache, 1 ); $services = $this->getServiceContainer(); @@ -424,8 +423,8 @@ class HtmlOutputRendererHelperTest extends MediaWikiIntegrationTestCase { $eTag = $helper->getETag(); $parsoidStashKey = ParsoidRenderID::newFromETag( $eTag ); - $chFactory = $this->createNoOpMock( IContentHandlerFactory::class ); - $stash = new SimpleParsoidOutputStash( $chFactory, $cache, 1 ); + $jsonCodec = $this->getServiceContainer()->getJsonCodec(); + $stash = new SimpleParsoidOutputStash( $jsonCodec, $cache, 1 ); $this->assertNotNull( $stash->get( $parsoidStashKey ) ); } @@ -445,8 +444,8 @@ class HtmlOutputRendererHelperTest extends MediaWikiIntegrationTestCase { $eTag = $helper->getETag(); $parsoidStashKey = ParsoidRenderID::newFromETag( $eTag ); - $chFactory = $this->getServiceContainer()->getContentHandlerFactory(); - $stash = new SimpleParsoidOutputStash( $chFactory, $cache, 1 ); + $jsonCodec = $this->getServiceContainer()->getJsonCodec(); + $stash = new SimpleParsoidOutputStash( $jsonCodec, $cache, 1 ); $selserContext = $stash->get( $parsoidStashKey ); $this->assertNotNull( $selserContext ); diff --git a/tests/phpunit/integration/includes/edit/SimpleParsoidOutputStashSerializationTest.php b/tests/phpunit/integration/includes/edit/SimpleParsoidOutputStashSerializationTest.php index cc735c1339d..64c4747a45e 100644 --- a/tests/phpunit/integration/includes/edit/SimpleParsoidOutputStashSerializationTest.php +++ b/tests/phpunit/integration/includes/edit/SimpleParsoidOutputStashSerializationTest.php @@ -7,11 +7,10 @@ use MediaWiki\Content\ContentHandler; use MediaWiki\Content\IContentHandlerFactory; use MediaWiki\Content\WikitextContent; use MediaWiki\Edit\SelserContext; -use MediaWiki\Edit\SimpleParsoidOutputStash; +use MediaWiki\Json\JsonCodec; use MediaWikiIntegrationTestCase; -use Wikimedia\ObjectCache\HashBagOStuff; +use Psr\Container\ContainerInterface; use Wikimedia\Parsoid\Core\PageBundle; -use Wikimedia\TestingAccessWrapper; use Wikimedia\Tests\SerializationTestTrait; /** @@ -21,26 +20,14 @@ use Wikimedia\Tests\SerializationTestTrait; class SimpleParsoidOutputStashSerializationTest extends MediaWikiIntegrationTestCase { use SerializationTestTrait; - /** - * Overrides SerializationTestTrait::getClassToTest - * @return string - */ public static function getClassToTest(): string { return SelserContext::class; } - /** - * Overrides SerializationTestTrait::getSerializedDataPath - * @return string - */ public static function getSerializedDataPath(): string { return __DIR__ . '/../../../data/SelserContext'; } - /** - * Overrides SerializationTestTrait::getTestInstancesAndAssertions - * @return array - */ public static function getTestInstancesAndAssertions(): array { return [ 'basic' => [ @@ -99,62 +86,80 @@ class SimpleParsoidOutputStashSerializationTest extends MediaWikiIntegrationTest ]; } - /** - * Overrides SerializationTestTrait::getSupportedSerializationFormats - * @return array - */ public static function getSupportedSerializationFormats(): array { - $stash = new SimpleParsoidOutputStash( - new class implements IContentHandlerFactory { - public function getContentHandler( string $modelID ): ContentHandler { - return new class( CONTENT_MODEL_WIKITEXT, [ CONTENT_FORMAT_WIKITEXT ] ) extends ContentHandler { - public function serializeContent( Content $content, $format = null ) { - return $content->getText(); - } + $mockServices = new class implements ContainerInterface { + private $contents = []; - public function unserializeContent( $blob, $format = null ) { - return new WikitextContent( $blob ); - } + public function get( $id ) { + return $this->contents[$id] ?? null; + } - public function makeEmptyContent() { - throw new \Error( "unimplemented" ); - } - }; - } + public function has( $id ): bool { + return isset( $this->contents[$id] ); + } - public function getContentModels(): array { - return [ CONTENT_MODEL_WIKITEXT ]; - } + public function set( $id, $value ) { + $this->contents[$id] = $value; + } + }; + $chFactory = new class implements IContentHandlerFactory { + public function getContentHandler( string $modelID ): ContentHandler { + return new class( CONTENT_MODEL_WIKITEXT, [ CONTENT_FORMAT_WIKITEXT ] ) extends ContentHandler { + public function serializeContent( Content $content, $format = null ) { + return $content->getText(); + } - public function getAllContentFormats(): array { - return [ CONTENT_FORMAT_WIKITEXT ]; - } + public function unserializeContent( $blob, $format = null ) { + return new WikitextContent( $blob ); + } - public function isDefinedModel( string $modelId ): bool { - return $modelId === CONTENT_MODEL_WIKITEXT; - } - }, - new HashBagOStuff(), - 10000 + public function makeEmptyContent() { + throw new \Error( "unimplemented" ); + } + }; + } + + public function getContentModels(): array { + return [ CONTENT_MODEL_WIKITEXT ]; + } + + public function getAllContentFormats(): array { + return [ CONTENT_FORMAT_WIKITEXT ]; + } + + public function isDefinedModel( string $modelId ): bool { + return $modelId === CONTENT_MODEL_WIKITEXT; + } + }; + $mockServices->set( + 'ContentHandlerFactory', $chFactory ); - $wrapper = TestingAccessWrapper::newFromObject( $stash ); + $jsonCodec = new JsonCodec( $mockServices ); return [ [ 'ext' => 'serialized', - 'serializer' => static function ( $obj ) use ( $wrapper ) { - return serialize( $wrapper->selserContextToJsonArray( $obj ) ); + 'serializer' => static function ( $obj ) use ( $jsonCodec ) { + return serialize( + $jsonCodec->toJsonArray( $obj, SelserContext::class ) + ); }, - 'deserializer' => static function ( $data ) use ( $wrapper ) { - return $wrapper->newSelserContextFromJson( unserialize( $data ) ); + 'deserializer' => static function ( $data ) use ( $jsonCodec ) { + return $jsonCodec->newFromJsonArray( + unserialize( $data ), SelserContext::class + ); }, ], [ 'ext' => 'json', - 'serializer' => static function ( $obj ) use ( $wrapper ) { - return json_encode( $wrapper->selserContextToJsonArray( $obj ) ); + 'serializer' => static function ( $obj ) use ( $jsonCodec ) { + return json_encode( + $jsonCodec->toJsonArray( $obj, SelserContext::class ) + ); }, - 'deserializer' => static function ( $data ) use ( $wrapper ) { - return $wrapper->newSelserContextFromJson( json_decode( $data, true ) ); + 'deserializer' => static function ( $data ) use ( $jsonCodec ) { + return $jsonCodec->newFromJsonArray( + json_decode( $data, true ), SelserContext::class + ); }, ], ]; diff --git a/tests/phpunit/unit/includes/edit/SimpleParsoidOutputStashTest.php b/tests/phpunit/integration/includes/edit/SimpleParsoidOutputStashTest.php similarity index 54% rename from tests/phpunit/unit/includes/edit/SimpleParsoidOutputStashTest.php rename to tests/phpunit/integration/includes/edit/SimpleParsoidOutputStashTest.php index 4af403db2c9..33fd2bd337e 100644 --- a/tests/phpunit/unit/includes/edit/SimpleParsoidOutputStashTest.php +++ b/tests/phpunit/integration/includes/edit/SimpleParsoidOutputStashTest.php @@ -2,12 +2,11 @@ namespace MediaWiki\Tests\Unit\Edit; -use MediaWiki\Content\TextContentHandler; use MediaWiki\Content\WikitextContent; use MediaWiki\Edit\ParsoidRenderID; use MediaWiki\Edit\SelserContext; use MediaWiki\Edit\SimpleParsoidOutputStash; -use MediaWiki\Tests\Unit\DummyServicesTrait; +use MediaWiki\Json\JsonCodec; use Wikimedia\ObjectCache\HashBagOStuff; use Wikimedia\Parsoid\Core\PageBundle; @@ -15,12 +14,11 @@ use Wikimedia\Parsoid\Core\PageBundle; * @covers \MediaWiki\Edit\SimpleParsoidOutputStash * @covers \MediaWiki\Edit\SelserContext */ -class SimpleParsoidOutputStashTest extends \MediaWikiUnitTestCase { - use DummyServicesTrait; +class SimpleParsoidOutputStashTest extends \MediaWikiIntegrationTestCase { public function testSetAndGetWithNoContent() { - $chFactory = $this->getDummyContentHandlerFactory(); - $stash = new SimpleParsoidOutputStash( $chFactory, new HashBagOStuff(), 12 ); + $codec = new JsonCodec( $this->getServiceContainer() ); + $stash = new SimpleParsoidOutputStash( $codec, new HashBagOStuff(), 12 ); $key = new ParsoidRenderID( 7, 'acme' ); $pageBundle = new PageBundle( '

Hello World

' ); @@ -31,23 +29,13 @@ class SimpleParsoidOutputStashTest extends \MediaWikiUnitTestCase { } public function testSetAndGetWithContent() { - $contentHandler = $this->createNoOpMock( TextContentHandler::class, [ 'unserializeContent' ] ); - $contentHandler->method( 'unserializeContent' )->willReturnCallback( static function ( $data ) { - return new WikitextContent( $data ); - } ); - - $chFactory = $this->getDummyContentHandlerFactory( - [ CONTENT_MODEL_WIKITEXT => $contentHandler ] - ); - - $stash = new SimpleParsoidOutputStash( $chFactory, new HashBagOStuff(), 12 ); + $codec = new JsonCodec( $this->getServiceContainer() ); + $stash = new SimpleParsoidOutputStash( $codec, new HashBagOStuff(), 12 ); $key = new ParsoidRenderID( 7, 'acme' ); $pageBundle = new PageBundle( '

Hello World

' ); - $content = $this->createNoOpMock( WikitextContent::class, [ 'getModel', 'serialize' ] ); - $content->method( 'getModel' )->willReturn( CONTENT_MODEL_WIKITEXT ); - $content->method( 'serialize' )->willReturn( 'Hello World' ); + $content = new WikitextContent( 'Hello World' ); $selserContext = new SelserContext( $pageBundle, 7, $content ); @@ -58,5 +46,4 @@ class SimpleParsoidOutputStashTest extends \MediaWikiUnitTestCase { $this->assertEquals( 'Hello World', $actual->getContent()->getText() ); $this->assertEquals( 7, $actual->getRevisionID() ); } - }