From 5d1e7c41bc6bfdddea1a2818c6df3e8b6e373a7a Mon Sep 17 00:00:00 2001 From: Daniel Siepmann Date: Tue, 6 Mar 2018 11:36:05 +0100 Subject: [PATCH] !!!|FEATURE: Pass facet configuration to search service Do not limit integrator in possibilities to configure. Therefore previously configure facets for a field need to be adjusted to contain full configuration for elasticsearch. See changelog. Resolves: #120 --- Classes/Connection/Elasticsearch/Facet.php | 23 +++--- Classes/Connection/FacetRequestInterface.php | 10 +-- Classes/Domain/Model/FacetRequest.php | 24 ++---- Classes/Domain/Search/QueryFactory.php | 6 +- Classes/Domain/Search/SearchService.php | 7 +- Documentation/source/changelog.rst | 8 ++ .../20180406-120-facet-configuration.rst | 40 ++++++++++ Documentation/source/index.rst | 1 + .../Connection/Elasticsearch/FacetTest.php | 75 +++++++++++++++++++ .../Connection/Elasticsearch/FilterTest.php | 33 -------- Tests/Functional/Fixtures/BasicSetup.ts | 6 -- Tests/Functional/Fixtures/Searching/Facet.ts | 17 +++++ Tests/Unit/Domain/Search/QueryFactoryTest.php | 4 +- 13 files changed, 167 insertions(+), 87 deletions(-) create mode 100644 Documentation/source/changelog.rst create mode 100644 Documentation/source/changelog/20180406-120-facet-configuration.rst create mode 100644 Tests/Functional/Connection/Elasticsearch/FacetTest.php create mode 100644 Tests/Functional/Fixtures/Searching/Facet.ts diff --git a/Classes/Connection/Elasticsearch/Facet.php b/Classes/Connection/Elasticsearch/Facet.php index e24a659..1142d88 100644 --- a/Classes/Connection/Elasticsearch/Facet.php +++ b/Classes/Connection/Elasticsearch/Facet.php @@ -45,25 +45,26 @@ class Facet implements FacetInterface */ protected $options = []; - public function __construct($name, array $aggregation, ConfigurationContainerInterface $configuration) + public function __construct(string $name, array $aggregation, ConfigurationContainerInterface $configuration) { $this->name = $name; $this->buckets = $aggregation['buckets']; - $this->field = $configuration->getIfExists('searching.facets.' . $this->name . '.field') ?: ''; + + $config = $configuration->getIfExists('searching.facets.' . $this->name) ?: []; + foreach ($config as $configEntry) { + if (isset($configEntry['field'])) { + $this->field = $configEntry['field']; + break; + } + } } - /** - * @return string - */ - public function getName() + public function getName() : string { return $this->name; } - /** - * @return string - */ - public function getField() + public function getField() : string { return $this->field; } @@ -73,7 +74,7 @@ class Facet implements FacetInterface * * @return array */ - public function getOptions() + public function getOptions() : array { $this->initOptions(); diff --git a/Classes/Connection/FacetRequestInterface.php b/Classes/Connection/FacetRequestInterface.php index 9352f96..cdc16b1 100644 --- a/Classes/Connection/FacetRequestInterface.php +++ b/Classes/Connection/FacetRequestInterface.php @@ -28,15 +28,11 @@ interface FacetRequestInterface /** * The identifier of the facet, used as key in arrays and to get the facet * from search request, etc. - * - * @return string */ - public function getIdentifier(); + public function getIdentifier() : string; /** - * The field to use for facet building. - * - * @return string + * The config to use for facet building. */ - public function getField(); + public function getConfig() : array; } diff --git a/Classes/Domain/Model/FacetRequest.php b/Classes/Domain/Model/FacetRequest.php index b1dbc4c..0d82ad3 100644 --- a/Classes/Domain/Model/FacetRequest.php +++ b/Classes/Domain/Model/FacetRequest.php @@ -30,37 +30,27 @@ class FacetRequest implements FacetRequestInterface protected $identifier = ''; /** - * @var string + * @var array */ - protected $field = ''; + protected $config = []; /** - * TODO: Add validation / exception? * As the facets come from configuration this might be a good idea to help * integrators find issues. - * - * @param string $identifier - * @param string $field */ - public function __construct($identifier, $field) + public function __construct(string $identifier, array $config) { $this->identifier = $identifier; - $this->field = $field; + $this->config = $config; } - /** - * @return string - */ - public function getIdentifier() + public function getIdentifier() : string { return $this->identifier; } - /** - * @return string - */ - public function getField() + public function getConfig() : array { - return $this->field; + return $this->config; } } diff --git a/Classes/Domain/Search/QueryFactory.php b/Classes/Domain/Search/QueryFactory.php index 980b763..8e122ab 100644 --- a/Classes/Domain/Search/QueryFactory.php +++ b/Classes/Domain/Search/QueryFactory.php @@ -239,11 +239,7 @@ class QueryFactory foreach ($searchRequest->getFacets() as $facet) { $query = ArrayUtility::arrayMergeRecursiveOverrule($query, [ 'aggs' => [ - $facet->getIdentifier() => [ - 'terms' => [ - 'field' => $facet->getField(), - ], - ], + $facet->getIdentifier() => $facet->getConfig(), ], ]); } diff --git a/Classes/Domain/Search/SearchService.php b/Classes/Domain/Search/SearchService.php index 120ab3b..40c99bd 100644 --- a/Classes/Domain/Search/SearchService.php +++ b/Classes/Domain/Search/SearchService.php @@ -103,15 +103,10 @@ class SearchService } foreach ($facetsConfig as $identifier => $facetConfig) { - if (!isset($facetConfig['field']) || trim($facetConfig['field']) === '') { - // TODO: Finish throw - throw new \Exception('message', 1499171142); - } - $searchRequest->addFacet($this->objectManager->get( FacetRequest::class, $identifier, - $facetConfig['field'] + $facetConfig )); } } diff --git a/Documentation/source/changelog.rst b/Documentation/source/changelog.rst new file mode 100644 index 0000000..0d7f020 --- /dev/null +++ b/Documentation/source/changelog.rst @@ -0,0 +1,8 @@ +Changelog +========= + +.. toctree:: + :maxdepth: 1 + :glob: + + changelog/* diff --git a/Documentation/source/changelog/20180406-120-facet-configuration.rst b/Documentation/source/changelog/20180406-120-facet-configuration.rst new file mode 100644 index 0000000..53021d9 --- /dev/null +++ b/Documentation/source/changelog/20180406-120-facet-configuration.rst @@ -0,0 +1,40 @@ +Breacking Change 120 "Pass facets configuration to elasticsearch" +================================================================= + +In order to allow arbitrary facet configuration, we do not process the facet configuration anymore. +Instead integrators are able to configure facets for search service "as is". We just pipe the +configuration through. + +Therefore the following, which worked before, does not work anymore: + +.. code-block:: typoscript + :linenos: + :emphasize-lines: 4 + + plugin.tx_searchcore.settings.search { + facets { + category { + field = categories + } + } + } + +Instead you have to provide the full configuration yourself: + +.. code-block:: typoscript + :linenos: + :emphasize-lines: 4,6 + + plugin.tx_searchcore.settings.search { + facets { + category { + terms { + field = categories + } + } + } + } + +You need to add line 4 and 6, the additional level ``terms`` for elasticsearch. + +See :issue:`120`. diff --git a/Documentation/source/index.rst b/Documentation/source/index.rst index 5427fc9..2edfae3 100644 --- a/Documentation/source/index.rst +++ b/Documentation/source/index.rst @@ -15,3 +15,4 @@ Table of Contents connections indexer development + changelog diff --git a/Tests/Functional/Connection/Elasticsearch/FacetTest.php b/Tests/Functional/Connection/Elasticsearch/FacetTest.php new file mode 100644 index 0000000..c89bc10 --- /dev/null +++ b/Tests/Functional/Connection/Elasticsearch/FacetTest.php @@ -0,0 +1,75 @@ + + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA. + */ + +use Codappix\SearchCore\Domain\Index\IndexerFactory; +use Codappix\SearchCore\Domain\Model\SearchRequest; +use Codappix\SearchCore\Domain\Search\SearchService; +use TYPO3\CMS\Extbase\Object\ObjectManager; + +class FacetTest extends AbstractFunctionalTestCase +{ + protected function getTypoScriptFilesForFrontendRootPage() + { + return array_merge(parent::getTypoScriptFilesForFrontendRootPage(), ['EXT:search_core/Tests/Functional/Fixtures/Searching/Facet.ts']); + } + + protected function getDataSets() + { + return array_merge( + parent::getDataSets(), + ['Tests/Functional/Fixtures/Searching/Filter.xml'] + ); + } + + /** + * @test + */ + public function itsPossibleToFetchFacetsForField() + { + \TYPO3\CMS\Core\Utility\GeneralUtility::makeInstance(ObjectManager::class) + ->get(IndexerFactory::class) + ->getIndexer('tt_content') + ->indexAllDocuments() + ; + + $searchService = \TYPO3\CMS\Core\Utility\GeneralUtility::makeInstance(ObjectManager::class) + ->get(SearchService::class); + + $searchRequest = new SearchRequest(); + $result = $searchService->search($searchRequest); + + $this->assertSame(1, count($result->getFacets()), 'Did not receive the single defined facet.'); + + $facet = current($result->getFacets()); + $this->assertSame('contentTypes', $facet->getName(), 'Name of facet was not as expected.'); + $this->assertSame('CType', $facet->getField(), 'Field of facet was not expected.'); + + $options = $facet->getOptions(); + $this->assertSame(2, count($options), 'Did not receive the expected number of possible options for facet.'); + $option = $options['HTML']; + $this->assertSame('HTML', $option->getName(), 'Option did not have expected Name.'); + $this->assertSame(1, $option->getCount(), 'Option did not have expected count.'); + $option = $options['Header']; + $this->assertSame('Header', $option->getName(), 'Option did not have expected Name.'); + $this->assertSame(1, $option->getCount(), 'Option did not have expected count.'); + } +} diff --git a/Tests/Functional/Connection/Elasticsearch/FilterTest.php b/Tests/Functional/Connection/Elasticsearch/FilterTest.php index e364a4a..8bbe0f0 100644 --- a/Tests/Functional/Connection/Elasticsearch/FilterTest.php +++ b/Tests/Functional/Connection/Elasticsearch/FilterTest.php @@ -58,37 +58,4 @@ class FilterTest extends AbstractFunctionalTestCase $this->assertSame(5, $result->getResults()[0]['uid'], 'Did not get the expected result entry.'); $this->assertSame(1, count($result), 'Did not receive the single filtered element.'); } - - /** - * @test - */ - public function itsPossibleToFetchFacetsForField() - { - \TYPO3\CMS\Core\Utility\GeneralUtility::makeInstance(ObjectManager::class) - ->get(IndexerFactory::class) - ->getIndexer('tt_content') - ->indexAllDocuments() - ; - - $searchService = \TYPO3\CMS\Core\Utility\GeneralUtility::makeInstance(ObjectManager::class) - ->get(SearchService::class); - - $searchRequest = new SearchRequest('Search Word'); - $result = $searchService->search($searchRequest); - - $this->assertSame(1, count($result->getFacets()), 'Did not receive the single defined facet.'); - - $facet = current($result->getFacets()); - $this->assertSame('contentTypes', $facet->getName(), 'Name of facet was not as expected.'); - $this->assertSame('CType', $facet->getField(), 'Field of facet was not expected.'); - - $options = $facet->getOptions(); - $this->assertSame(2, count($options), 'Did not receive the expected number of possible options for facet.'); - $option = $options['HTML']; - $this->assertSame('HTML', $option->getName(), 'Option did not have expected Name.'); - $this->assertSame(1, $option->getCount(), 'Option did not have expected count.'); - $option = $options['Header']; - $this->assertSame('Header', $option->getName(), 'Option did not have expected Name.'); - $this->assertSame(1, $option->getCount(), 'Option did not have expected count.'); - } } diff --git a/Tests/Functional/Fixtures/BasicSetup.ts b/Tests/Functional/Fixtures/BasicSetup.ts index b7b0c6a..4129362 100644 --- a/Tests/Functional/Fixtures/BasicSetup.ts +++ b/Tests/Functional/Fixtures/BasicSetup.ts @@ -37,12 +37,6 @@ plugin { } searching { - facets { - contentTypes { - field = CType - } - } - fields { query = _all } diff --git a/Tests/Functional/Fixtures/Searching/Facet.ts b/Tests/Functional/Fixtures/Searching/Facet.ts new file mode 100644 index 0000000..10eeae6 --- /dev/null +++ b/Tests/Functional/Fixtures/Searching/Facet.ts @@ -0,0 +1,17 @@ +plugin { + tx_searchcore { + settings { + searching { + facets { + contentTypes { + terms { + field = CType + } + } + } + } + } + } +} + +module.tx_searchcore < plugin.tx_searchcore diff --git a/Tests/Unit/Domain/Search/QueryFactoryTest.php b/Tests/Unit/Domain/Search/QueryFactoryTest.php index e0fc86d..881cd8b 100644 --- a/Tests/Unit/Domain/Search/QueryFactoryTest.php +++ b/Tests/Unit/Domain/Search/QueryFactoryTest.php @@ -118,8 +118,8 @@ class QueryFactoryTest extends AbstractUnitTestCase { $this->configureConfigurationMockWithDefault(); $searchRequest = new SearchRequest('SearchWord'); - $searchRequest->addFacet(new FacetRequest('Identifier', 'FieldName')); - $searchRequest->addFacet(new FacetRequest('Identifier 2', 'FieldName 2')); + $searchRequest->addFacet(new FacetRequest('Identifier', ['terms' => ['field' => 'FieldName']])); + $searchRequest->addFacet(new FacetRequest('Identifier 2', ['terms' => ['field' => 'FieldName 2']])); $query = $this->subject->create($searchRequest); $this->assertSame(