tonyair
/
silverstripe-fulltextsearch
mirror of https://github.com/silverstripe/silverstripe-fulltextsearch
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Merge pull request #117 from tractorcow/pulls/fix-shared-variants 

BUG fix issues with search variants applying to more than one class
This commit is contained in:
Daniel Hensby 2016-04-26 02:20:41 +01:00
parent afe7af18d2 e5fbdf9d42
commit da22629d47
12 changed files with 437 additions and 133 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
.travis.yml
View File

@ -1,36 +1,35 @@
# See https://github.com/silverstripe-labs/silverstripe-travis-support for setup details
sudo: false
language: php
sudo: false
php:
- 5.3
- 5.4
- 5.5
- 5.6
- 7.0
env:
- DB=MYSQL CORE_RELEASE=3.2
matrix:
include:
- php: 5.3
env: DB=PGSQL CORE_RELEASE=3.1
- php: 5.6
env: DB=MYSQL CORE_RELEASE=3
env: DB=MYSQL CORE_RELEASE=3.2
- php: 5.6
env: DB=MYSQL CORE_RELEASE=3.1
env: DB=MYSQL CORE_RELEASE=3.3 SUBSITES=1
- php: 5.6
env: DB=PGSQL CORE_RELEASE=3.2
allow_failures:
- php: 7.0
env: DB=MYSQL CORE_RELEASE=3.3 QUEUEDJOBS=1
before_script:
- composer self-update || true
- git clone git://github.com/silverstripe-labs/silverstripe-travis-support.git ~/travis-support
- php ~/travis-support/travis_setup.php --source `pwd` --target ~/builds/ss
- "if [ \"$SUBSITES\" = \"\" -a \"$QUEUEDJOBS\" = \"\" ]; then php ~/travis-support/travis_setup.php --source `pwd` --target ~/builds/ss; fi"
- "if [ \"$SUBSITES\" = \"1\" ]; then php ~/travis-support/travis_setup.php --source `pwd` --target ~/builds/ss --require silverstripe/subsites; fi"
- "if [ \"$QUEUEDJOBS\" = \"1\" ]; then php ~/travis-support/travis_setup.php --source `pwd` --target ~/builds/ss --require silverstripe/queuedjobs; fi"
- cd ~/builds/ss
- composer install
script:
- vendor/bin/phpunit fulltextsearch/tests
- vendor/bin/phpunit fulltextsearch/tests/

17
code/search/SearchIndex.php
View File

@ -465,7 +465,7 @@ abstract class SearchIndex extends ViewableData
$method = $step['method'];
$object = $object->$method();
} elseif ($step['call'] == 'variant') {
$variants = SearchVariant::variants($field['base'], true);
$variants = SearchVariant::variants();
$variant = $variants[$step['variant']];
$method = $step['method'];
$object = $variant->$method($object);
@ -476,6 +476,7 @@ abstract class SearchIndex extends ViewableData
}
}
} catch (Exception $e) {
static::warn($e);
$object = null;
}
@ -483,6 +484,20 @@ abstract class SearchIndex extends ViewableData
return $object;
}
/**
* Log non-fatal errors
*
* @param Exception $e
* @throws Exception
*/
public static function warn($e) {
// Noisy errors during testing
if(class_exists('SapphireTest', false) && SapphireTest::is_running_test()) {
throw $e;
}
SS_Log::log($e, SS_Log::WARN);
}
/**
* Given a class, object id, set of stateful ids and a list of changed fields (in a special format),
* return what statefulids need updating in this index

55
code/search/SearchVariant.php
View File

@ -123,7 +123,7 @@ abstract class SearchVariant
* @param bool $includeSubclasses - (Optional) If false, only variants that apply strictly to the passed class or its super-classes
* will be checked. If true (the default), variants that apply to any sub-class of the passed class with also be checked
*
* @return An object with one method, call()
* @return SearchVariant_Caller An object with one method, call()
*/
public static function with($class = null, $includeSubclasses = true)
{
@ -197,6 +197,59 @@ abstract class SearchVariant
return $allstates ? new CombinationsArrayIterator($allstates) : array(array());
}
/**
* Add new filter field to index safely.
*
* This method will respect existing filters with the same field name that
* correspond to multiple classes
*
* @param SearchIndex $index
* @param string $name Field name
* @param array $field Field spec
*/
protected function addFilterField($index, $name, $field) {
// If field already exists, make sure to merge origin / base fields
if(isset($index->filterFields[$name])) {
$field['base'] = $this->mergeClasses(
$index->filterFields[$name]['base'],
$field['base']
);
$field['origin'] = $this->mergeClasses(
$index->filterFields[$name]['origin'],
$field['origin']
);
}
$index->filterFields[$name] = $field;
}
/**
* Merge sets of (or individual) class names together for a search index field.
*
* If there is only one unique class name, then just return it as a string instead of array.
*
* @param array|string $left Left class(es)
* @param array|string $right Right class(es)
* @return array|string List of classes, or single class
*/
protected function mergeClasses($left, $right) {
// Merge together and remove dupes
if(!is_array($left)) {
$left = array($left);
}
if(!is_array($right)) {
$right = array($right);
}
$merged = array_values(array_unique(array_merge($left, $right)));
// If there is only one item, return it as a single string
if(count($merged) === 1) {
return reset($merged);
}
return $merged;
}
}
/**

10
code/search/SearchVariantSiteTreeSubsitesPolyhome.php
View File

@ -40,19 +40,19 @@ class SearchVariantSiteTreeSubsitesPolyhome extends SearchVariant
}
}
public function alterDefinition($base, $index)
public function alterDefinition($class, $index)
{
$self = get_class($this);
$index->filterFields['_subsite'] = array(
$this->addFilterField($index, '_subsite', array(
'name' => '_subsite',
'field' => '_subsite',
'fullfield' => '_subsite',
'base' => $base,
'origin' => $base,
'base' => ClassInfo::baseDataClass($class),
'origin' => $class,
'type' => 'Int',
'lookup_chain' => array(array('call' => 'variant', 'variant' => $self, 'method' => 'currentState'))
);
));
}
public function alterQuery($query, $index)

36
code/search/SearchVariantSubsites.php
View File

@ -44,29 +44,29 @@ class SearchVariantSubsites extends SearchVariant
Permission::flush_permission_cache();
}
public function alterDefinition($base, $index)
public function alterDefinition($class, $index)
{
$self = get_class($this);
$index->filterFields['_subsite'] = array(
// Add field to root
$this->addFilterField($index, '_subsite', array(
'name' => '_subsite',
'field' => '_subsite',
'fullfield' => '_subsite',
'base' => $base,
'origin' => $base,
'base' => ClassInfo::baseDataClass($class),
'origin' => $class,
'type' => 'Int',
'lookup_chain' => array(array('call' => 'variant', 'variant' => $self, 'method' => 'currentState'))
);
));
}
public function alterQuery($query, $index)
{
$subsite = Subsite::currentSubsiteID();
$query->filter('_subsite', array($subsite, SearchQuery::$missing));
}
public static $subsites = null;
/**
* We need _really_ complicated logic to find just the changed subsites (because we use versions there's no explicit
* deletes, just new versions with different members) so just always use all of them
@ -74,25 +74,27 @@ class SearchVariantSubsites extends SearchVariant
public function extractManipulationWriteState(&$writes)
{
$self = get_class($this);
$query = new SQLQuery('"ID"', '"Subsite"');
$subsites = array_merge(array('0'), $query->execute()->column());
foreach ($writes as $key => $write) {
if (!$this->appliesTo($write['class'], true)) {
$applies = $this->appliesTo($write['class'], true);
if (!$applies) {
continue;
}
if (self::$subsites === null) {
$query = new SQLQuery('"ID"', '"Subsite"');
self::$subsites = array_merge(array('0'), $query->execute()->column());
}
$next = array();
foreach ($write['statefulids'] as $i => $statefulid) {
foreach (self::$subsites as $subsiteID) {
$next[] = array('id' => $statefulid['id'], 'state' => array_merge($statefulid['state'], array($self => (string)$subsiteID)));
foreach ($subsites as $subsiteID) {
$next[] = array(
'id' => $statefulid['id'],
'state' => array_merge(
$statefulid['state'],
array($self => (string)$subsiteID)
)
);
}
}
$writes[$key]['statefulids'] = $next;
}
}

10
code/search/SearchVariantVersioned.php
View File

@ -25,19 +25,19 @@ class SearchVariantVersioned extends SearchVariant
Versioned::reading_stage($state);
}
public function alterDefinition($base, $index)
public function alterDefinition($class, $index)
{
$self = get_class($this);
$index->filterFields['_versionedstage'] = array(
$this->addFilterField($index, '_versionedstage', array(
'name' => '_versionedstage',
'field' => '_versionedstage',
'fullfield' => '_versionedstage',
'base' => $base,
'origin' => $base,
'base' => ClassInfo::baseDataClass($class),
'origin' => $class,
'type' => 'String',
'lookup_chain' => array(array('call' => 'variant', 'variant' => $self, 'method' => 'currentState'))
);
));
}
public function alterQuery($query, $index)

31
code/solr/SolrIndex.php
View File

@ -451,10 +451,31 @@ abstract class SolrIndex extends SearchIndex
return implode("\n\t", $xml);
}
/**
* Determine if the given object is one of the given type
*
* @param string $class
* @param array|string $base Class or list of base classes
* @return bool
*/
protected function classIs($class, $base) {
if(is_array($base)) {
foreach($base as $nextBase) {
if($this->classIs($class, $nextBase)) {
return true;
}
}
return false;
}
// Check single origin
return $class === $base || is_subclass_of($class, $base);
}
protected function _addField($doc, $object, $field)
{
$class = get_class($object);
if ($class != $field['origin'] && !is_subclass_of($class, $field['origin'])) {
if(!$this->classIs($class, $field['origin'])) {
return;
}
@ -516,7 +537,7 @@ abstract class SolrIndex extends SearchIndex
// Add the user-specified fields
foreach ($this->getFieldsIterator() as $name => $field) {
if ($field['base'] == $base) {
if ($field['base'] === $base || (is_array($field['base']) && in_array($base, $field['base']))) {
$this->_addField($doc, $object, $field);
}
}
@ -524,7 +545,7 @@ abstract class SolrIndex extends SearchIndex
try {
$this->getService()->addDocument($doc);
} catch (Exception $e) {
SS_Log::log($e, SS_Log::WARN);
static::warn($e);
return false;
}
@ -564,7 +585,7 @@ abstract class SolrIndex extends SearchIndex
try {
$this->getService()->deleteById($documentID);
} catch (Exception $e) {
SS_Log::log($e, SS_Log::WARN);
static::warn($e);
return false;
}
}
@ -608,7 +629,7 @@ abstract class SolrIndex extends SearchIndex
try {
$this->getService()->commit(false, false, false);
} catch (Exception $e) {
SS_Log::log($e, SS_Log::WARN);
static::warn($e);
return false;
}
}

145
tests/SolrIndexSubsitesTest.php Normal file
View File

@ -0,0 +1,145 @@
<?php
if (class_exists('Phockito')) {
Phockito::include_hamcrest(false);
}
/**
* Subsite specific solr testing
*/
class SolrIndexSubsitesTest extends SapphireTest {
public static $fixture_file = 'SolrIndexSubsitesTest.yml';
/**
* @var SolrIndexSubsitesTest_Index
*/
private static $index = null;
protected $server = null;
public function setUp()
{
// Prevent parent::setUp() crashing on db build
if (!class_exists('Subsite')) {
$this->skipTest = true;
}
parent::setUp();
$this->server = $_SERVER;
if (!class_exists('Phockito')) {
$this->skipTest = true;
$this->markTestSkipped("These tests need the Phockito module installed to run");
return;
}
// Check versioned available
if (!class_exists('Subsite')) {
$this->skipTest = true;
$this->markTestSkipped('The subsite module is not installed');
return;
}
if (self::$index === null) {
self::$index = singleton('SolrIndexSubsitesTest_Index');
}
SearchUpdater::bind_manipulation_capture();
Config::inst()->update('Injector', 'SearchUpdateProcessor', array(
'class' => 'SearchUpdateImmediateProcessor'
));
FullTextSearch::force_index_list(self::$index);
SearchUpdater::clear_dirty_indexes();
}
public function tearDown()
{
if($this->server) {
$_SERVER = $this->server;
$this->server = null;
}
parent::tearDown();
}
protected function getServiceMock()
{
return Phockito::mock('Solr4Service');
}
/**
* @param DataObject $object Item being added
* @param int $subsiteID
* @param string $stage
* @return string
*/
protected function getExpectedDocumentId($object, $subsiteID, $stage = null)
{
$id = $object->ID;
$class = ClassInfo::baseDataClass($object);
$variants = array();
// Check subsite
if(class_exists('Subsite') && $object->hasOne('Subsite')) {
$variants[] = '"SearchVariantSubsites":"' . $subsiteID. '"';
}
// Check versioned
if($stage) {
$variants[] = '"SearchVariantVersioned":"' . $stage . '"';
}
return $id.'-'.$class.'-{'.implode(',',$variants).'}';
}
public function testPublishing()
{
// Setup mocks
$serviceMock = $this->getServiceMock();
self::$index->setService($serviceMock);
$subsite1 = $this->objFromFixture('Subsite', 'subsite1');
// Add records to first subsite
Versioned::reading_stage('Stage');
$_SERVER['HTTP_HOST'] = 'www.subsite1.com';
Phockito::reset($serviceMock);
$file = new File();
$file->Title = 'My File';
$file->SubsiteID = $subsite1->ID;
$file->write();
$page = new Page();
$page->Title = 'My Page';
$page->SubsiteID = $subsite1->ID;
$page->write();
SearchUpdater::flush_dirty_indexes();
$doc1 = new SolrDocumentMatcher(array(
'_documentid' => $this->getExpectedDocumentId($page, $subsite1->ID, 'Stage'),
'ClassName' => 'Page',
'SiteTree_Title' => 'My Page',
'_versionedstage' => 'Stage',
'_subsite' => $subsite1->ID
));
$doc2 = new SolrDocumentMatcher(array(
'_documentid' => $this->getExpectedDocumentId($file, $subsite1->ID),
'ClassName' => 'File',
'File_Title' => 'My File',
'_subsite' => $subsite1->ID
));
Phockito::verify($serviceMock)->addDocument($doc1);
Phockito::verify($serviceMock)->addDocument($doc2);
}
}
class SolrIndexSubsitesTest_Index extends SolrIndex
{
public function init()
{
$this->addClass('File');
$this->addClass('SiteTree');
$this->addAllFulltextFields();
}
}

16
tests/SolrIndexSubsitesTest.yml Normal file
View File

@ -0,0 +1,16 @@
Subsite:
main:
Title: Template
subsite1:
Title: 'Subsite1 Template'
subsite2:
Title: 'Subsite2 Template'
SubsiteDomain:
subsite1:
SubsiteID: =>Subsite.subsite1
Domain: www.subsite1.com
Protocol: automatic
subsite2:
SubsiteID: =>Subsite.subsite2
Domain: www.subsite2.com
Protocol: automatic

91
tests/SolrIndexVersionedTest.php
View File

@ -11,7 +11,8 @@ class SolrIndexVersionedTest extends SapphireTest
protected static $index = null;
protected $extraDataObjects = array(
'SearchVariantVersionedTest_Item'
'SearchVariantVersionedTest_Item',
'SolrIndexVersionedTest_Object',
);
public function setUp()
@ -20,13 +21,15 @@ class SolrIndexVersionedTest extends SapphireTest
if (!class_exists('Phockito')) {
$this->skipTest = true;
return $this->markTestSkipped("These tests need the Phockito module installed to run");
$this->markTestSkipped("These tests need the Phockito module installed to run");
return;
}
// Check versioned available
if (!class_exists('Versioned')) {
$this->skipTest = true;
return $this->markTestSkipped('The versioned decorator is not installed');
$this->markTestSkipped('The versioned decorator is not installed');
return;
}
if (self::$index === null) {
@ -57,11 +60,21 @@ class SolrIndexVersionedTest extends SapphireTest
return Phockito::mock('Solr3Service');
}
protected function getExpectedDocumentId($id, $stage)
/**
* @param DataObject $object Item being added
* @param string $stage
* @return string
*/
protected function getExpectedDocumentId($object, $stage)
{
$id = $object->ID;
$class = ClassInfo::baseDataClass($object);
// Prevent subsites from breaking tests
$subsites = class_exists('Subsite') ? '"SearchVariantSubsites":"0",' : '';
return $id.'-SiteTree-{'.$subsites.'"SearchVariantVersioned":"'.$stage.'"}';
$subsites = '';
if(class_exists('Subsite') && $object->hasOne('Subsite')) {
$subsites = '"SearchVariantSubsites":"0",';
}
return $id.'-'.$class.'-{'.$subsites.'"SearchVariantVersioned":"'.$stage.'"}';
}
public function testPublishing()
@ -74,32 +87,54 @@ class SolrIndexVersionedTest extends SapphireTest
// Check that write updates Stage
Versioned::reading_stage('Stage');
Phockito::reset($serviceMock);
$item = new SearchVariantVersionedTest_Item(array('Title' => 'Foo'));
$item = new SearchVariantVersionedTest_Item(array('TestText' => 'Foo'));
$item->write();
$object = new SolrIndexVersionedTest_Object(array('TestText' => 'Bar'));
$object->write();
SearchUpdater::flush_dirty_indexes();
$doc = new SolrDocumentMatcher(array(
'_documentid' => $this->getExpectedDocumentId($item->ID, 'Stage'),
'ClassName' => 'SearchVariantVersionedTest_Item'
$doc1 = new SolrDocumentMatcher(array(
'_documentid' => $this->getExpectedDocumentId($item, 'Stage'),
'ClassName' => 'SearchVariantVersionedTest_Item',
'SearchVariantVersionedTest_Item_TestText' => 'Foo',
'_versionedstage' => 'Stage'
));
Phockito::verify($serviceMock)->addDocument($doc);
$doc2 = new SolrDocumentMatcher(array(
'_documentid' => $this->getExpectedDocumentId($object, 'Stage'),
'ClassName' => 'SolrIndexVersionedTest_Object',
'SolrIndexVersionedTest_Object_TestText' => 'Bar',
'_versionedstage' => 'Stage'
));
Phockito::verify($serviceMock)->addDocument($doc1);
Phockito::verify($serviceMock)->addDocument($doc2);
// Check that write updates Live
Versioned::reading_stage('Stage');
Phockito::reset($serviceMock);
$item = new SearchVariantVersionedTest_Item(array('Title' => 'Bar'));
$item = new SearchVariantVersionedTest_Item(array('TestText' => 'Foo'));
$item->write();
$item->publish('Stage', 'Live');
$object = new SolrIndexVersionedTest_Object(array('TestText' => 'Bar'));
$object->write();
$object->publish('Stage', 'Live');
SearchUpdater::flush_dirty_indexes();
$doc = new SolrDocumentMatcher(array(
'_documentid' => $this->getExpectedDocumentId($item->ID, 'Live'),
'ClassName' => 'SearchVariantVersionedTest_Item'
'_documentid' => $this->getExpectedDocumentId($item, 'Live'),
'ClassName' => 'SearchVariantVersionedTest_Item',
'SearchVariantVersionedTest_Item_TestText' => 'Foo',
'_versionedstage' => 'Live'
));
$doc2 = new SolrDocumentMatcher(array(
'_documentid' => $this->getExpectedDocumentId($object, 'Live'),
'ClassName' => 'SolrIndexVersionedTest_Object',
'SolrIndexVersionedTest_Object_TestText' => 'Bar',
'_versionedstage' => 'Live'
));
Phockito::verify($serviceMock)->addDocument($doc);
Phockito::verify($serviceMock)->addDocument($doc2);
}
public function testDelete()
{
// Setup mocks
$serviceMock = $this->getServiceMock();
self::$index->setService($serviceMock);
@ -107,11 +142,11 @@ class SolrIndexVersionedTest extends SapphireTest
// Delete the live record (not the stage)
Versioned::reading_stage('Stage');
Phockito::reset($serviceMock);
$item = new SearchVariantVersionedTest_Item(array('Title' => 'Too'));
$item = new SearchVariantVersionedTest_Item(array('TestText' => 'Too'));
$item->write();
$item->publish('Stage', 'Live');
Versioned::reading_stage('Live');
$id = $item->ID;
$id = clone $item;
$item->delete();
SearchUpdater::flush_dirty_indexes();
Phockito::verify($serviceMock, 1)
@ -122,10 +157,10 @@ class SolrIndexVersionedTest extends SapphireTest
// Delete the stage record
Versioned::reading_stage('Stage');
Phockito::reset($serviceMock);
$item = new SearchVariantVersionedTest_Item(array('Title' => 'Too'));
$item = new SearchVariantVersionedTest_Item(array('TestText' => 'Too'));
$item->write();
$item->publish('Stage', 'Live');
$id = $item->ID;
$id = clone $item;
$item->delete();
SearchUpdater::flush_dirty_indexes();
Phockito::verify($serviceMock, 1)
@ -141,7 +176,9 @@ class SolrVersionedTest_Index extends SolrIndex
public function init()
{
$this->addClass('SearchVariantVersionedTest_Item');
$this->addClass('SolrIndexVersionedTest_Object');
$this->addFilterField('TestText');
$this->addFulltextField('Content');
}
}
@ -178,3 +215,19 @@ class SolrDocumentMatcher extends Hamcrest_BaseMatcher
return true;
}
}
/**
* Non-sitetree versioned dataobject
*/
class SolrIndexVersionedTest_Object extends DataObject implements TestOnly {
private static $extensions = array(
'Versioned'
);
private static $db = array(
'Title' => 'Varchar',
'Content' => 'Text',
'TestText' => 'Varchar',
);
}

10
tests/SolrReindexTest.php
View File

@ -416,19 +416,19 @@ class SolrReindexTest_Variant extends SearchVariant implements TestOnly
}
}
public function alterDefinition($base, $index)
public function alterDefinition($class, $index)
{
$self = get_class($this);
$index->filterFields['_testvariant'] = array(
$this->addFilterField($index, '_testvariant', array(
'name' => '_testvariant',
'field' => '_testvariant',
'fullfield' => '_testvariant',
'base' => $base,
'origin' => $base,
'base' => ClassInfo::baseDataClass($class),
'origin' => $class,
'type' => 'Int',
'lookup_chain' => array(array('call' => 'variant', 'variant' => $self, 'method' => 'currentState'))
);
));
}
public function alterQuery($query, $index)