Smalyshev has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/333995 )
Change subject: [WIP] Refactor search fields: use ContentHandler instead of hooks ...................................................................... [WIP] Refactor search fields: use ContentHandler instead of hooks Change-Id: If9e945a7730a2d9797b8b57ce08dec6b882f588b --- M repo/Wikibase.php M repo/includes/Content/EntityHandler.php M repo/includes/Content/ItemHandler.php D repo/includes/Hooks/CirrusSearchHookHandlers.php M repo/includes/Search/Elastic/Fields/LabelCountField.php M repo/includes/Search/Elastic/Fields/SearchIndexField.php M repo/includes/Search/Elastic/Fields/SiteLinkCountField.php M repo/includes/Search/Elastic/Fields/StatementCountField.php A repo/includes/Search/Elastic/Fields/WikibaseNumericField.php D repo/tests/phpunit/includes/Hooks/CirrusSearchHookHandlersTest.php 10 files changed, 77 insertions(+), 326 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase refs/changes/95/333995/1 diff --git a/repo/Wikibase.php b/repo/Wikibase.php index 732986d..8a4be58 100644 --- a/repo/Wikibase.php +++ b/repo/Wikibase.php @@ -402,10 +402,6 @@ $wgHooks['BeforeDisplayNoArticleText'][] = 'Wikibase\ViewEntityAction::onBeforeDisplayNoArticleText'; $wgHooks['InfoAction'][] = '\Wikibase\RepoHooks::onInfoAction'; - // CirrusSearch hooks - $wgHooks['CirrusSearchMappingConfig'][] = 'Wikibase\Repo\Hooks\CirrusSearchHookHandlers::onCirrusSearchMappingConfig'; - $wgHooks['CirrusSearchBuildDocumentParse'][] = 'Wikibase\Repo\Hooks\CirrusSearchHookHandlers::onCirrusSearchBuildDocumentParse'; - // update hooks $wgHooks['LoadExtensionSchemaUpdates'][] = '\Wikibase\Repo\Store\Sql\ChangesSubscriptionSchemaUpdater::onSchemaUpdate'; diff --git a/repo/includes/Content/EntityHandler.php b/repo/includes/Content/EntityHandler.php index e4c7269..2ed296b 100644 --- a/repo/includes/Content/EntityHandler.php +++ b/repo/includes/Content/EntityHandler.php @@ -13,8 +13,10 @@ use MWContentSerializationException; use MWException; use ParserOptions; +use ParserOutput; use RequestContext; use Revision; +use SearchEngine; use Title; use User; use Wikibase\Content\DeferredDecodingEntityHolder; @@ -27,6 +29,7 @@ use Wikibase\EntityContent; use Wikibase\Lib\Store\EntityContentDataCodec; use Wikibase\Repo\Diff\EntityContentDiffView; +use Wikibase\Repo\Search\Elastic\Fields\WikibaseFieldDefinitions; use Wikibase\Repo\Store\EntityPerPage; use Wikibase\Repo\Validators\EntityConstraintProvider; use Wikibase\Repo\Validators\EntityValidator; @@ -34,6 +37,7 @@ use Wikibase\Repo\WikibaseRepo; use Wikibase\TermIndex; use Wikibase\Updates\DataUpdateAdapter; +use WikiPage; /** * Base handler class for Entity content classes. @@ -52,6 +56,7 @@ * to parser output. */ const PARSER_VERSION = 3; + protected $fieldDefinitions; /** * @var EntityPerPage @@ -129,6 +134,8 @@ $this->errorLocalizer = $errorLocalizer; $this->entityIdParser = $entityIdParser; $this->legacyExportFormatDetector = $legacyExportFormatDetector; + // FIXME: convert to DI + $this->fieldDefinitions = new WikibaseFieldDefinitions(); } /** @@ -331,7 +338,7 @@ * @param string $blob * @param string|null $format * - * @return string|void + * @return string */ public function exportTransform( $blob, $format = null ) { if ( !$this->legacyExportFormatDetector ) { @@ -758,4 +765,36 @@ return false; } + public function getFieldsForSearchIndex( SearchEngine $engine ) { + $fields = []; + + foreach ( $this->fieldDefinitions->getFields() as $name => $field ) { + $fields[$name] = $field->getMapping( $engine, $name ); + } + + return $fields; + } + + /** + * @param WikiPage $page + * @param ParserOutput $output + * @param SearchEngine $engine + * @return array + */ + public function getDataForSearchIndex( WikiPage $page, ParserOutput $output, + SearchEngine $engine ) { + $fieldsData = parent::getDataForSearchIndex( $page, $output, $engine ); + + $content = $page->getContent(); + if ( ( $content instanceof EntityContent ) && !$content->isRedirect() ) { + $entity = $content->getEntity(); + $fields = $this->fieldDefinitions->getFields(); + + foreach ( $fields as $fieldName => $field ) { + $fieldsData[$fieldName] = $field->getFieldData( $entity ); + } + } + + return $fieldsData; + } } diff --git a/repo/includes/Content/ItemHandler.php b/repo/includes/Content/ItemHandler.php index bcb22ad..253af4e 100644 --- a/repo/includes/Content/ItemHandler.php +++ b/repo/includes/Content/ItemHandler.php @@ -5,7 +5,10 @@ use DataUpdate; use IContextSource; use Page; +use SearchEngine; +use SearchIndexField; use Title; +use Wikibase\DataModel\Entity\EntityDocument; use Wikibase\DataModel\Entity\EntityId; use Wikibase\DataModel\Entity\Item; use Wikibase\DataModel\Entity\ItemId; @@ -17,6 +20,7 @@ use Wikibase\Lib\Store\EntityContentDataCodec; use Wikibase\Lib\Store\LanguageFallbackLabelDescriptionLookupFactory; use Wikibase\Lib\Store\SiteLinkStore; +use Wikibase\Repo\Search\Elastic\Fields\WikibaseFieldDefinitions; use Wikibase\Repo\Store\EntityPerPage; use Wikibase\Repo\Validators\EntityConstraintProvider; use Wikibase\Repo\Validators\ValidatorErrorLocalizer; @@ -195,7 +199,7 @@ /** * @see EntityHandler::makeEmptyEntity() * - * @return EntityContent + * @return EntityDocument */ public function makeEmptyEntity() { return new Item(); @@ -211,5 +215,4 @@ public function makeEntityId( $id ) { return new ItemId( $id ); } - } diff --git a/repo/includes/Hooks/CirrusSearchHookHandlers.php b/repo/includes/Hooks/CirrusSearchHookHandlers.php deleted file mode 100644 index a7910cf..0000000 --- a/repo/includes/Hooks/CirrusSearchHookHandlers.php +++ /dev/null @@ -1,113 +0,0 @@ -<?php - -namespace Wikibase\Repo\Hooks; - -use CirrusSearch\Connection; -use CirrusSearch\Maintenance\MappingConfigBuilder; -use Content; -use Elastica\Document; -use ParserOutput; -use Title; -use UnexpectedValueException; -use Wikibase\EntityContent; -use Wikibase\Repo\Search\Elastic\Fields\WikibaseFieldDefinitions; - -/** - * @license GPL-2.0+ - * @author Katie Filbert < aude.w...@gmail.com > - */ -class CirrusSearchHookHandlers { - - /** - * @var WikibaseFieldDefinitions - */ - private $fieldDefinitions; - - /** - * @param Document $document - * @param Title $title - * @param Content $content - * @param ParserOutput $parserOutput - * @param Connection $connection - * - * @return bool - */ - public static function onCirrusSearchBuildDocumentParse( - Document $document, - Title $title, - Content $content, - ParserOutput $parserOutput, - Connection $connection - ) { - $hookHandler = self::newFromGlobalState(); - $hookHandler->indexExtraFields( $document, $content ); - - return true; - } - - /** - * @param array &$config - * @param MappingConfigBuilder $mappingConfigBuilder - * - * @return bool - */ - public static function onCirrusSearchMappingConfig( - array &$config, - MappingConfigBuilder $mappingConfigBuilder - ) { - $handler = self::newFromGlobalState(); - $handler->addExtraFieldsToMappingConfig( $config ); - - return true; - } - - /** - * @return self - */ - public static function newFromGlobalState() { - return new self( new WikibaseFieldDefinitions() ); - } - - /** - * @param WikibaseFieldDefinitions $fieldDefinitions - */ - public function __construct( WikibaseFieldDefinitions $fieldDefinitions ) { - $this->fieldDefinitions = $fieldDefinitions; - } - - /** - * @param Document $document - * @param Content $content - */ - public function indexExtraFields( Document $document, Content $content ) { - if ( !$content instanceof EntityContent || $content->isRedirect() === true ) { - return; - } - - $fields = $this->fieldDefinitions->getFields(); - $entity = $content->getEntity(); - - foreach ( $fields as $fieldName => $field ) { - $data = $field->getFieldData( $entity ); - $document->set( $fieldName, $data ); - } - } - - /** - * @param array &$config - * - * @throws UnexpectedValueException - */ - public function addExtraFieldsToMappingConfig( array &$config ) { - $fields = $this->fieldDefinitions->getFields(); - - foreach ( $fields as $fieldName => $field ) { - if ( array_key_exists( $fieldName, $config['page']['properties'] ) ) { - throw new UnexpectedValueException( "$fieldName is already set in the mapping." ); - } - - $config['page']['properties'][$fieldName] = $field->getMapping(); - } - } - -} diff --git a/repo/includes/Search/Elastic/Fields/LabelCountField.php b/repo/includes/Search/Elastic/Fields/LabelCountField.php index 3ad28b3..157df1f 100644 --- a/repo/includes/Search/Elastic/Fields/LabelCountField.php +++ b/repo/includes/Search/Elastic/Fields/LabelCountField.php @@ -9,19 +9,7 @@ * @license GPL-2.0+ * @author Katie Filbert < aude.w...@gmail.com > */ -class LabelCountField implements SearchIndexField { - - /** - * @see SearchIndexField::getMapping - * - * @return array - */ - public function getMapping() { - return array( - 'type' => 'integer' - ); - } - +class LabelCountField extends WikibaseNumericField { /** * @see SearchIndexField::getFieldData * diff --git a/repo/includes/Search/Elastic/Fields/SearchIndexField.php b/repo/includes/Search/Elastic/Fields/SearchIndexField.php index f9dcaa1..fa3d9ab 100644 --- a/repo/includes/Search/Elastic/Fields/SearchIndexField.php +++ b/repo/includes/Search/Elastic/Fields/SearchIndexField.php @@ -2,6 +2,7 @@ namespace Wikibase\Repo\Search\Elastic\Fields; +use SearchEngine; use Wikibase\DataModel\Entity\EntityDocument; /** @@ -18,14 +19,12 @@ interface SearchIndexField { /** - * @return array The field mapping defines attributes of the field, - * such as the field type (e.g. "string", "long", "nested") - * and other attributes like "not_analyzed". - * - * For detailed documentation about mapping of fields, see: - * https://www.elastic.co/guide/en/elasticsearch/guide/current/mapping-intro.html + * Produce specific field mapping + * @param SearchEngine $engine + * @param string $name + * @return \SearchIndexField */ - public function getMapping(); + public function getMapping( SearchEngine $engine, $name ); /** * @param EntityDocument $entity diff --git a/repo/includes/Search/Elastic/Fields/SiteLinkCountField.php b/repo/includes/Search/Elastic/Fields/SiteLinkCountField.php index 5cc6a4b..acd14d7 100644 --- a/repo/includes/Search/Elastic/Fields/SiteLinkCountField.php +++ b/repo/includes/Search/Elastic/Fields/SiteLinkCountField.php @@ -9,19 +9,7 @@ * @license GPL-2.0+ * @author Katie Filbert < aude.w...@gmail.com > */ -class SiteLinkCountField implements SearchIndexField { - - /** - * @see SearchIndexField::getMapping - * - * @return array - */ - public function getMapping() { - return array( - 'type' => 'integer' - ); - } - +class SiteLinkCountField extends WikibaseNumericField { /** * @see SearchIndexField::getFieldData * diff --git a/repo/includes/Search/Elastic/Fields/StatementCountField.php b/repo/includes/Search/Elastic/Fields/StatementCountField.php index 0ee5d01..308bc3e 100644 --- a/repo/includes/Search/Elastic/Fields/StatementCountField.php +++ b/repo/includes/Search/Elastic/Fields/StatementCountField.php @@ -9,18 +9,7 @@ * @license GPL-2.0+ * @author Katie Filbert < aude.w...@gmail.com > */ -class StatementCountField implements SearchIndexField { - - /** - * @see SearchIndexField::getMapping - * - * @return array - */ - public function getMapping() { - return array( - 'type' => 'integer' - ); - } +class StatementCountField extends WikibaseNumericField { /** * @see SearchIndexField::getFieldData diff --git a/repo/includes/Search/Elastic/Fields/WikibaseNumericField.php b/repo/includes/Search/Elastic/Fields/WikibaseNumericField.php new file mode 100644 index 0000000..e79b842 --- /dev/null +++ b/repo/includes/Search/Elastic/Fields/WikibaseNumericField.php @@ -0,0 +1,23 @@ +<?php +namespace Wikibase\Repo\Search\Elastic\Fields; + +use SearchEngine; + +/** + * Generic numeric field + * @package Wikibase\Repo\Search\Elastic\Fields + */ +abstract class WikibaseNumericField implements SearchIndexField { + + /** + * @param SearchEngine $engine + * @param string $name + * @return \SearchIndexField + */ + public function getMapping( SearchEngine $engine, $name ) { + return $engine->makeSearchFieldMapping( + $name, + \SearchIndexField::INDEX_TYPE_INTEGER + ); + } +} diff --git a/repo/tests/phpunit/includes/Hooks/CirrusSearchHookHandlersTest.php b/repo/tests/phpunit/includes/Hooks/CirrusSearchHookHandlersTest.php deleted file mode 100644 index ded9389..0000000 --- a/repo/tests/phpunit/includes/Hooks/CirrusSearchHookHandlersTest.php +++ /dev/null @@ -1,161 +0,0 @@ -<?php - -namespace Wikibase\Repo\Tests\Hooks; - -use CirrusSearch\Connection; -use CirrusSearch\Maintenance\MappingConfigBuilder; -use CirrusSearch; -use Elastica\Document; -use ParserOutput; -use PHPUnit_Framework_TestCase; -use Title; -use UnexpectedValueException; -use Wikibase\DataModel\Entity\Item; -use Wikibase\DataModel\Entity\PropertyId; -use Wikibase\DataModel\Snak\PropertyNoValueSnak; -use Wikibase\Repo\Search\Elastic\Fields\WikibaseFieldDefinitions; -use Wikibase\Repo\Hooks\CirrusSearchHookHandlers; -use Wikibase\Repo\WikibaseRepo; - -/** - * @covers Wikibase\Repo\Hooks\CirrusSearchHookHandlers - * - * @group WikibaseElastic - * @group Wikibase - * - * @license GPL-2.0+ - * @author Katie Filbert < aude.w...@gmail.com > - */ -class CirrusSearchHookHandlersTest extends PHPUnit_Framework_TestCase { - - protected function setUp() { - parent::setUp(); - - if ( !class_exists( CirrusSearch::class ) ) { - $this->markTestSkipped( 'CirrusSearch is not available' ); - } - } - - public function testOnCirrusSearchBuildDocumentParse() { - $connection = $this->getMockBuilder( Connection::class ) - ->disableOriginalConstructor() - ->getMock(); - - $document = new Document(); - - CirrusSearchHookHandlers::onCirrusSearchBuildDocumentParse( - $document, - Title::newFromText( 'Q1' ), - $this->getContent(), - new ParserOutput(), - $connection - ); - - $this->assertSame( 1, $document->get( 'label_count' ), 'label_count' ); - $this->assertSame( 1, $document->get( 'sitelink_count' ), 'sitelink_count' ); - $this->assertSame( 1, $document->get( 'statement_count' ), 'statement_count' ); - } - - public function testOnCirrusSearchMappingConfig() { - $mappingConfigBuilder = $this->getMockBuilder( MappingConfigBuilder::class ) - ->disableOriginalConstructor() - ->getMock(); - - $config = array( - 'page' => array( - 'properties' => array() - ) - ); - - CirrusSearchHookHandlers::onCirrusSearchMappingConfig( $config, $mappingConfigBuilder ); - - $this->assertSame( - array( 'label_count', 'sitelink_count', 'statement_count' ), - array_keys( $config['page']['properties'] ) - ); - } - - public function testIndexExtraFields() { - $fieldDefinitions = $this->newFieldDefinitions(); - - $document = new Document(); - $content = $this->getContent(); - - $hookHandlers = new CirrusSearchHookHandlers( $fieldDefinitions ); - $hookHandlers->indexExtraFields( $document, $content ); - - $this->assertSame( 1, $document->get( 'label_count' ), 'label_count' ); - $this->assertSame( 1, $document->get( 'sitelink_count' ), 'sitelink_count' ); - $this->assertSame( 1, $document->get( 'statement_count' ), 'statement_count' ); - } - - public function testAddExtraFieldsToMappingConfig() { - $fieldDefinitions = $this->newFieldDefinitions(); - - $config = array( - 'page' => array( - 'properties' => array() - ) - ); - - $hookHandlers = new CirrusSearchHookHandlers( $fieldDefinitions ); - $hookHandlers->addExtraFieldsToMappingConfig( $config ); - - $expected = array( - 'page' => array( - 'properties' => array( - 'label_count' => array( - 'type' => 'integer' - ), - 'sitelink_count' => array( - 'type' => 'integer' - ), - 'statement_count' => array( - 'type' => 'integer' - ) - ) - ) - ); - - $this->assertSame( $expected, $config ); - } - - public function testAddExtraFields_throwsExceptionIfFieldNameAlreadySet() { - $fieldDefinitions = $this->newFieldDefinitions(); - - $config = array( - 'page' => array( - 'properties' => array( - 'sitelink_count' => array( - 'type' => 'long' - ) - ) - ) - ); - - $this->setExpectedException( UnexpectedValueException::class ); - - $hookHandlers = new CirrusSearchHookHandlers( $fieldDefinitions ); - $hookHandlers->addExtraFieldsToMappingConfig( $config ); - } - - private function newFieldDefinitions() { - // when we add multilingual fields, then WikibaseFieldDefinitions - // will take WikibaseContentLanguages as an argument. - return new WikibaseFieldDefinitions(); - } - - private function getContent() { - $item = new Item(); - $item->getFingerprint()->setLabel( 'en', 'Kitten' ); - $item->getSiteLinkList()->addNewSiteLink( 'enwiki', 'Kitten' ); - $item->getStatements()->addNewStatement( - new PropertyNoValueSnak( new PropertyId( 'P1' ) ) - ); - - $entityContentFactory = WikibaseRepo::getDefaultInstance()->getEntityContentFactory(); - - return $entityContentFactory->newFromEntity( $item ); - } - -} -- To view, visit https://gerrit.wikimedia.org/r/333995 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If9e945a7730a2d9797b8b57ce08dec6b882f588b Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Smalyshev <smalys...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits