From 2b3357565e302552d32e512395c019ff15b8f9e9 Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Fri, 16 Jun 2017 09:39:37 +1200 Subject: [PATCH 1/3] Index now supports multiple relations with the same name. --- code/search/SearchIndex.php | 57 +++++++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 5 deletions(-) diff --git a/code/search/SearchIndex.php b/code/search/SearchIndex.php index 7e4ab9a..eba8096 100644 --- a/code/search/SearchIndex.php +++ b/code/search/SearchIndex.php @@ -37,6 +37,27 @@ abstract class SearchIndex extends ViewableData */ private static $hide_ancestor; + /** + * Used to separate class name and relation name in the sources array + * this string must not be present in class name + * @var string + * @config + */ + private static $class_delimiter = '_|_'; + + /** + * This is used to clean the source name from suffix + * suffixes are needed to support multiple relations with the same name on different page types + * @param string $source + * @return string + */ + private function getSourceName($source) + { + $source = explode(self::config()->get('class_delimiter'), $source); + + return $source[0]; + } + public function __construct() { parent::__construct(); @@ -77,19 +98,35 @@ abstract class SearchIndex extends ViewableData foreach ($lookups as $lookup) { $next = array(); - foreach ($sources as $source => $options) { - $class = null; + foreach ($sources as $source => $baseOptions) { + $source = $this->getSourceName($source); - foreach (SearchIntrospection::hierarchy($source, $options['include_children']) as $dataclass) { + foreach (SearchIntrospection::hierarchy($source, $baseOptions['include_children']) as $dataclass) { + $class = null; + $options = $baseOptions; $singleton = singleton($dataclass); if ($hasOne = $singleton->has_one($lookup)) { + // we only want to include base class for relation, omit classes that inherited the relation + $relationList = Config::inst()->get($dataclass, 'has_one', Config::UNINHERITED); + $relationList = (!is_null($relationList)) ? $relationList : array(); + if (!array_key_exists($lookup, $relationList)) { + continue; + } + $class = $hasOne; $options['lookup_chain'][] = array( 'call' => 'method', 'method' => $lookup, 'through' => 'has_one', 'class' => $dataclass, 'otherclass' => $class, 'foreignkey' => "{$lookup}ID" ); } elseif ($hasMany = $singleton->has_many($lookup)) { + // we only want to include base class for relation, omit classes that inherited the relation + $relationList = Config::inst()->get($dataclass, 'has_many', Config::UNINHERITED); + $relationList = (!is_null($relationList)) ? $relationList : array(); + if (!array_key_exists($lookup, $relationList)) { + continue; + } + $class = $hasMany; $options['multi_valued'] = true; $options['lookup_chain'][] = array( @@ -97,6 +134,13 @@ abstract class SearchIndex extends ViewableData 'through' => 'has_many', 'class' => $dataclass, 'otherclass' => $class, 'foreignkey' => $singleton->getRemoteJoinField($lookup, 'has_many') ); } elseif ($manyMany = $singleton->many_many($lookup)) { + // we only want to include base class for relation, omit classes that inherited the relation + $relationList = Config::inst()->get($dataclass, 'many_many', Config::UNINHERITED); + $relationList = (!is_null($relationList)) ? $relationList : array(); + if (!array_key_exists($lookup, $relationList)) { + continue; + } + $class = $manyMany[1]; $options['multi_valued'] = true; $options['lookup_chain'][] = array( @@ -109,8 +153,10 @@ abstract class SearchIndex extends ViewableData if (!isset($options['origin'])) { $options['origin'] = $dataclass; } - $next[$class] = $options; - continue 2; + + // we add suffix here to prevent the relation to be overwritten by other instances + // all sources lookups must clean the source name before reading it via getSourceName() + $next[$class . self::config()->get('class_delimiter') . $dataclass] = $options; } } } @@ -123,6 +169,7 @@ abstract class SearchIndex extends ViewableData } foreach ($sources as $class => $options) { + $class = $this->getSourceName($class); $dataclasses = SearchIntrospection::hierarchy($class, $options['include_children']); while (count($dataclasses)) { From b7f4c1e5f8ac469c46355927a9855464e37793fc Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Mon, 19 Jun 2017 10:50:11 +1200 Subject: [PATCH 2/3] Test for ambiguous relationships See https://github.com/silverstripe/silverstripe-fulltextsearch/pull/145 --- tests/SearchUpdaterTest.php | 20 +++++++++++-- tests/SolrIndexTest.php | 57 +++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/tests/SearchUpdaterTest.php b/tests/SearchUpdaterTest.php index d2211c0..3b42f11 100644 --- a/tests/SearchUpdaterTest.php +++ b/tests/SearchUpdaterTest.php @@ -21,6 +21,20 @@ class SearchUpdaterTest_Container extends DataObject ); } +/** + * Used to test ambiguous relationships. + */ +class SearchUpdaterTest_OtherContainer extends DataObject +{ + private static $has_many = array( + 'HasManyObjects' => 'SearchUpdaterTest_HasMany' + ); + + private static $many_many = array( + 'ManyManyObjects' => 'SearchUpdaterTest_ManyMany' + ); +} + class SearchUpdaterTest_HasOne extends DataObject { private static $db = array( @@ -41,7 +55,8 @@ class SearchUpdaterTest_HasMany extends DataObject ); private static $has_one = array( - 'HasManyContainer' => 'SearchUpdaterTest_Container' + 'HasManyContainer' => 'SearchUpdaterTest_Container', + 'HasManyOtherContainer' => 'SearchUpdaterTest_OtherContainer' ); } @@ -53,7 +68,8 @@ class SearchUpdaterTest_ManyMany extends DataObject ); private static $belongs_many_many = array( - 'ManyManyContainer' => 'SearchUpdaterTest_Container' + 'ManyManyContainer' => 'SearchUpdaterTest_Container', + 'ManyManyOtherContainer' => 'SearchUpdaterTest_OtherContainer' ); } diff --git a/tests/SolrIndexTest.php b/tests/SolrIndexTest.php index c710113..cc7e745 100644 --- a/tests/SolrIndexTest.php +++ b/tests/SolrIndexTest.php @@ -53,6 +53,44 @@ class SolrIndexTest extends SapphireTest $this->assertEquals('SearchUpdaterTest_ManyMany', $data['class']); } + public function testFieldDataAmbiguousHasMany() + { + $index = new SolrIndexTest_AmbiguousRelationIndex(); + $data = $index->fieldData('HasManyObjects.Field1'); + + $this->assertArrayHasKey('SearchUpdaterTest_Container_HasManyObjects_Field1', $data); + $this->assertArrayHasKey('SearchUpdaterTest_OtherContainer_HasManyObjects_Field1', $data); + + $dataContainer = $data['SearchUpdaterTest_Container_HasManyObjects_Field1']; + $this->assertEquals($dataContainer['origin'], 'SearchUpdaterTest_Container'); + $this->assertEquals($dataContainer['base'], 'SearchUpdaterTest_Container'); + $this->assertEquals($dataContainer['class'], 'SearchUpdaterTest_HasMany'); + + $dataOtherContainer = $data['SearchUpdaterTest_OtherContainer_HasManyObjects_Field1']; + $this->assertEquals($dataOtherContainer['origin'], 'SearchUpdaterTest_OtherContainer'); + $this->assertEquals($dataOtherContainer['base'], 'SearchUpdaterTest_OtherContainer'); + $this->assertEquals($dataOtherContainer['class'], 'SearchUpdaterTest_HasMany'); + } + + public function testFieldDataAmbiguousManyMany() + { + $index = new SolrIndexTest_AmbiguousRelationIndex(); + $data = $index->fieldData('ManyManyObjects.Field1'); + + $this->assertArrayHasKey('SearchUpdaterTest_Container_ManyManyObjects_Field1', $data); + $this->assertArrayHasKey('SearchUpdaterTest_OtherContainer_ManyManyObjects_Field1', $data); + + $dataContainer = $data['SearchUpdaterTest_Container_ManyManyObjects_Field1']; + $this->assertEquals($dataContainer['origin'], 'SearchUpdaterTest_Container'); + $this->assertEquals($dataContainer['base'], 'SearchUpdaterTest_Container'); + $this->assertEquals($dataContainer['class'], 'SearchUpdaterTest_ManyMany'); + + $dataOtherContainer = $data['SearchUpdaterTest_OtherContainer_ManyManyObjects_Field1']; + $this->assertEquals($dataOtherContainer['origin'], 'SearchUpdaterTest_OtherContainer'); + $this->assertEquals($dataOtherContainer['base'], 'SearchUpdaterTest_OtherContainer'); + $this->assertEquals($dataOtherContainer['class'], 'SearchUpdaterTest_ManyMany'); + } + /** * Test boosting on SearchQuery */ @@ -359,3 +397,22 @@ class SolrIndexTest_BoostedIndex extends SolrIndex $this->addBoostedField('Field2', null, array(), 2.1); } } + +class SolrIndexTest_AmbiguousRelationIndex extends SolrIndex +{ + protected function getStoredDefault() + { + // Override isDev defaulting to stored + return 'false'; + } + + public function init() + { + $this->addClass('SearchUpdaterTest_Container'); + $this->addClass('SearchUpdaterTest_OtherContainer'); + + // These relationships exist on both classes + $this->addFilterField('HasManyObjects.Field1'); + $this->addFilterField('ManyManyObjects.Field1'); + } +} From 16fc54e1019f6b7f6e0e49ba05695f76f0e8fb83 Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Fri, 16 Jun 2017 09:39:37 +1200 Subject: [PATCH 3/3] Index now supports multiple relations with the same name. --- code/search/SearchIndex.php | 7 +++++-- tests/SearchUpdaterTest.php | 14 +++++++++++-- tests/SolrIndexTest.php | 41 +++++++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 4 deletions(-) diff --git a/code/search/SearchIndex.php b/code/search/SearchIndex.php index eba8096..30054e7 100644 --- a/code/search/SearchIndex.php +++ b/code/search/SearchIndex.php @@ -51,7 +51,7 @@ abstract class SearchIndex extends ViewableData * @param string $source * @return string */ - private function getSourceName($source) + protected function getSourceName($source) { $source = explode(self::config()->get('class_delimiter'), $source); @@ -78,6 +78,9 @@ abstract class SearchIndex extends ViewableData /** * Examines the classes this index is built on to try and find defined fields in the class hierarchy for those classes. * Looks for db and viewable-data fields, although can't nessecarily find type for viewable-data fields. + * If multiple classes have a relation with the same name all of these will be included in the search index + * Note that only classes that have the relations uninherited (defined in them) will be listed + * this is because inherited relations do not need to be processed by index explicitly */ public function fieldData($field, $forceType = null, $extraOptions = array()) { @@ -604,7 +607,7 @@ abstract class SearchIndex extends ViewableData $ids = $sql->execute()->column(); } - + if (empty($ids)) { break; } diff --git a/tests/SearchUpdaterTest.php b/tests/SearchUpdaterTest.php index 3b42f11..9b662d4 100644 --- a/tests/SearchUpdaterTest.php +++ b/tests/SearchUpdaterTest.php @@ -35,6 +35,16 @@ class SearchUpdaterTest_OtherContainer extends DataObject ); } +/** + * Used to test inherited ambiguous relationships. + */ +class SearchUpdaterTest_ExtendedContainer extends SearchUpdaterTest_OtherContainer +{ + private static $db = array( + 'SomeField' => 'Varchar', + ); +} + class SearchUpdaterTest_HasOne extends DataObject { private static $db = array( @@ -56,7 +66,7 @@ class SearchUpdaterTest_HasMany extends DataObject private static $has_one = array( 'HasManyContainer' => 'SearchUpdaterTest_Container', - 'HasManyOtherContainer' => 'SearchUpdaterTest_OtherContainer' + 'HasManyOtherContainer' => 'SearchUpdaterTest_OtherContainer', ); } @@ -69,7 +79,7 @@ class SearchUpdaterTest_ManyMany extends DataObject private static $belongs_many_many = array( 'ManyManyContainer' => 'SearchUpdaterTest_Container', - 'ManyManyOtherContainer' => 'SearchUpdaterTest_OtherContainer' + 'ManyManyOtherContainer' => 'SearchUpdaterTest_OtherContainer', ); } diff --git a/tests/SolrIndexTest.php b/tests/SolrIndexTest.php index cc7e745..b01ce17 100644 --- a/tests/SolrIndexTest.php +++ b/tests/SolrIndexTest.php @@ -91,6 +91,26 @@ class SolrIndexTest extends SapphireTest $this->assertEquals($dataOtherContainer['class'], 'SearchUpdaterTest_ManyMany'); } + public function testFieldDataAmbiguousManyManyInherited() + { + $index = new SolrIndexTest_AmbiguousRelationInheritedIndex(); + $data = $index->fieldData('ManyManyObjects.Field1'); + + $this->assertArrayHasKey('SearchUpdaterTest_Container_ManyManyObjects_Field1', $data); + $this->assertArrayHasKey('SearchUpdaterTest_OtherContainer_ManyManyObjects_Field1', $data); + $this->assertArrayNotHasKey('SearchUpdaterTest_ExtendedContainer_ManyManyObjects_Field1', $data); + + $dataContainer = $data['SearchUpdaterTest_Container_ManyManyObjects_Field1']; + $this->assertEquals($dataContainer['origin'], 'SearchUpdaterTest_Container'); + $this->assertEquals($dataContainer['base'], 'SearchUpdaterTest_Container'); + $this->assertEquals($dataContainer['class'], 'SearchUpdaterTest_ManyMany'); + + $dataOtherContainer = $data['SearchUpdaterTest_OtherContainer_ManyManyObjects_Field1']; + $this->assertEquals($dataOtherContainer['origin'], 'SearchUpdaterTest_OtherContainer'); + $this->assertEquals($dataOtherContainer['base'], 'SearchUpdaterTest_OtherContainer'); + $this->assertEquals($dataOtherContainer['class'], 'SearchUpdaterTest_ManyMany'); + } + /** * Test boosting on SearchQuery */ @@ -416,3 +436,24 @@ class SolrIndexTest_AmbiguousRelationIndex extends SolrIndex $this->addFilterField('ManyManyObjects.Field1'); } } + +class SolrIndexTest_AmbiguousRelationInheritedIndex extends SolrIndex +{ + protected function getStoredDefault() + { + // Override isDev defaulting to stored + return 'false'; + } + + public function init() + { + $this->addClass('SearchUpdaterTest_Container'); + // this one has not the relation defined in it's class but is rather inherited from parent + // note that even if we do not include it's parent class the fields will be properly added + $this->addClass('SearchUpdaterTest_ExtendedContainer'); + + // These relationships exist on both classes + $this->addFilterField('HasManyObjects.Field1'); + $this->addFilterField('ManyManyObjects.Field1'); + } +}