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
