From 1c1295cacb98b9b92df398bfcc66f93ffb52cf0d Mon Sep 17 00:00:00 2001 From: Ferdinand Kuhl Date: Wed, 14 Dec 2016 19:34:35 +0100 Subject: [PATCH 1/3] TASK: Just some very early notes, without claiming of completeness --- Classes/Connection/ConnectionInterface.php | 6 ++++++ Classes/Connection/Elasticsearch/Connection.php | 2 ++ Classes/Domain/Index/IndexerInterface.php | 7 +++++-- Classes/Domain/Model/SearchRequest.php | 1 + Classes/Domain/Search/SearchService.php | 15 --------------- Classes/Domain/Service/DataHandler.php | 2 ++ Tests/Functional/AbstractFunctionalTestCase.php | 1 + Tests/Functional/Hooks/DataHandlerTest.php | 7 ++++--- Tests/Functional/Indexing/IndexTcaTableTest.php | 9 +++++---- 9 files changed, 26 insertions(+), 24 deletions(-) diff --git a/Classes/Connection/ConnectionInterface.php b/Classes/Connection/ConnectionInterface.php index 65acdeb..a47bf11 100644 --- a/Classes/Connection/ConnectionInterface.php +++ b/Classes/Connection/ConnectionInterface.php @@ -28,6 +28,8 @@ interface ConnectionInterface /** * Will add a new document. * + * TODO: Should be addDocument + * * @param string $documentType * @param array $document * @@ -48,6 +50,8 @@ interface ConnectionInterface /** * Will update an existing document. * + * TODO: updateDocument (what about batches? consistency) + * * @param string $documentType * @param array $document * @@ -58,6 +62,8 @@ interface ConnectionInterface /** * Will remove an existing document. * + * TODO: deleteDocument (what about batches? consistency) + * * @param string $documentType * @param int $identifier * diff --git a/Classes/Connection/Elasticsearch/Connection.php b/Classes/Connection/Elasticsearch/Connection.php index e473241..8fb0a64 100644 --- a/Classes/Connection/Elasticsearch/Connection.php +++ b/Classes/Connection/Elasticsearch/Connection.php @@ -27,6 +27,8 @@ use TYPO3\CMS\Core\SingletonInterface as Singleton; * The current connection to elasticsearch. * * Wrapper for Elastica\Client. + * + * TODO: Catch inner exception and throw general ones (at least in Connection-Namespace (not elastic specific)) */ class Connection implements Singleton { diff --git a/Classes/Domain/Index/IndexerInterface.php b/Classes/Domain/Index/IndexerInterface.php index 88972dd..27c0d3c 100644 --- a/Classes/Domain/Index/IndexerInterface.php +++ b/Classes/Domain/Index/IndexerInterface.php @@ -26,16 +26,19 @@ namespace Leonmrni\SearchCore\Domain\Index; interface IndexerInterface { /** - * Index the index. + * Fetches all documents from the indexerService and pushes it to the connection. * * @return void */ public function index(); /** - * Index a single record. + * Fetches a single document from the indexerService and pushes it to the connection. + * + * @param string $identifier identifier, the indexer needs to identify a single document * * @return void + * TODO: is record the correct name? (minor) */ public function indexRecord($identifier); } diff --git a/Classes/Domain/Model/SearchRequest.php b/Classes/Domain/Model/SearchRequest.php index ba16349..900a11a 100644 --- a/Classes/Domain/Model/SearchRequest.php +++ b/Classes/Domain/Model/SearchRequest.php @@ -55,6 +55,7 @@ class SearchRequest implements SearchRequestInterface */ public function getSearchTerm() { + //TODO: This seems to be connection specific return '"' . $this->query . '"'; } } diff --git a/Classes/Domain/Search/SearchService.php b/Classes/Domain/Search/SearchService.php index 7867aa1..afbaf7e 100644 --- a/Classes/Domain/Search/SearchService.php +++ b/Classes/Domain/Search/SearchService.php @@ -35,21 +35,6 @@ class SearchService */ protected $connection; - /** - * @var \TYPO3\CMS\Core\Log\Logger - */ - protected $logger; - - /** - * Inject log manager to get concrete logger from it. - * - * @param \TYPO3\CMS\Core\Log\LogManager $logManager - */ - public function injectLogger(\TYPO3\CMS\Core\Log\LogManager $logManager) - { - $this->logger = $logManager->getLogger(__CLASS__); - } - /** * @param ConnectionInterface $connection */ diff --git a/Classes/Domain/Service/DataHandler.php b/Classes/Domain/Service/DataHandler.php index 6446979..300f7d6 100644 --- a/Classes/Domain/Service/DataHandler.php +++ b/Classes/Domain/Service/DataHandler.php @@ -27,6 +27,8 @@ use TYPO3\CMS\Core\SingletonInterface as Singleton; * * This is the place to add mappings of further parts to adjust the data before * sending ot to connection. + * + * TODO: Probably a candidate for deletion */ class DataHandler implements Singleton { diff --git a/Tests/Functional/AbstractFunctionalTestCase.php b/Tests/Functional/AbstractFunctionalTestCase.php index fd90e8e..2029813 100644 --- a/Tests/Functional/AbstractFunctionalTestCase.php +++ b/Tests/Functional/AbstractFunctionalTestCase.php @@ -26,6 +26,7 @@ use TYPO3\CMS\Core\Tests\FunctionalTestCase as CoreTestCase; * All functional tests should extend this base class. * * It will take care of leaving a clean environment for next test. + * TODO: this is in reality an "elastica" abstract case - not search_core ;) */ abstract class AbstractFunctionalTestCase extends CoreTestCase { diff --git a/Tests/Functional/Hooks/DataHandlerTest.php b/Tests/Functional/Hooks/DataHandlerTest.php index a1a17b7..60c0115 100644 --- a/Tests/Functional/Hooks/DataHandlerTest.php +++ b/Tests/Functional/Hooks/DataHandlerTest.php @@ -76,7 +76,7 @@ class DataHandlerTest extends AbstractFunctionalTestCase $hook->processDatamap_afterDatabaseOperations('update', 'tt_content', 6, [], $dataHandler); $response = $this->client->request('typo3content/_search?q=*:*'); - $this->assertTrue($response->isOK()); + $this->assertTrue($response->isOK(), 'Elastica did not answer with ok code.'); $this->assertSame($response->getData()['hits']['total'], 1, 'Not exactly 1 document was indexed.'); } @@ -91,7 +91,7 @@ class DataHandlerTest extends AbstractFunctionalTestCase $hook->processCmdmap_deleteAction('tt_content', 6, [], false, $dataHandler); $response = $this->client->request('typo3content/_search?q=*:*'); - $this->assertTrue($response->isOK()); + $this->assertTrue($response->isOK(), 'Elastica did not answer with ok code.'); $this->assertSame($response->getData()['hits']['total'], 0, 'Not exactly 0 document was indexed.'); } @@ -99,10 +99,11 @@ class DataHandlerTest extends AbstractFunctionalTestCase * @test * @expectedException \Elastica\Exception\ResponseException */ - public function someUnkownOperationDoesNotBreakSomething() + public function someUnknownOperationDoesNotBreakSomething() { $dataHandler = new CoreDataHandler(); $hook = \TYPO3\CMS\Core\Utility\GeneralUtility::makeInstance(Hook::class); + //TODO: this test is senseless, checking an exception not correct, this operation should not do anything! $hook->processDatamap_afterDatabaseOperations('something', 'tt_content', 6, [], $dataHandler); // Should trigger Exception diff --git a/Tests/Functional/Indexing/IndexTcaTableTest.php b/Tests/Functional/Indexing/IndexTcaTableTest.php index 88ba1e4..6588c78 100644 --- a/Tests/Functional/Indexing/IndexTcaTableTest.php +++ b/Tests/Functional/Indexing/IndexTcaTableTest.php @@ -49,7 +49,7 @@ class IndexTcaTableTest extends AbstractFunctionalTestCase $response = $this->client->request('typo3content/_search?q=*:*'); - $this->assertTrue($response->isOK()); + $this->assertTrue($response->isOK(), 'Elastica did not answer with ok code.'); $this->assertSame($response->getData()['hits']['total'], 1, 'Not exactly 1 document was indexed.'); $this->assertArraySubset( ['_source' => ['header' => 'indexed content element']], @@ -72,9 +72,10 @@ class IndexTcaTableTest extends AbstractFunctionalTestCase } /** + * TODO: this does not test the indexer, it tests the backend * @test */ - public function canHandleExisingIndex() + public function canHandleExistingIndex() { $indexer = \TYPO3\CMS\Core\Utility\GeneralUtility::makeInstance(ObjectManager::class) ->get(IndexerFactory::class) @@ -88,7 +89,7 @@ class IndexTcaTableTest extends AbstractFunctionalTestCase $response = $this->client->request('typo3content/_search?q=*:*'); - $this->assertTrue($response->isOK()); + $this->assertTrue($response->isOK(), 'Elastica did not answer with ok code.'); $this->assertSame($response->getData()['hits']['total'], 1, 'Not exactly 1 document was indexed.'); } @@ -108,7 +109,7 @@ class IndexTcaTableTest extends AbstractFunctionalTestCase $response = $this->client->request('typo3content/_search?q=*:*'); - $this->assertTrue($response->isOK()); + $this->assertTrue($response->isOK(), 'Elastica did not answer with ok code.'); $this->assertSame($response->getData()['hits']['total'], 2, 'Not exactly 2 document was indexed.'); $this->assertArraySubset( ['_source' => ['header' => 'Also indexable record']], From 98affa8f690ff6575c71aa487e35143e7a5bc82d Mon Sep 17 00:00:00 2001 From: Daniel Siepmann Date: Thu, 15 Dec 2016 09:17:58 +0100 Subject: [PATCH 2/3] TASK: Resolve first review results * Also remove coverage output on stdout, as it won't help anyone. --- Classes/Command/IndexCommandController.php | 2 +- Classes/Connection/ConnectionInterface.php | 12 +++++------- Classes/Connection/Elasticsearch.php | 15 +++++---------- Classes/Domain/Index/IndexerInterface.php | 5 ++--- Classes/Domain/Index/TcaIndexer.php | 6 +++--- Classes/Domain/Model/SearchRequest.php | 3 +-- Classes/Domain/Service/DataHandler.php | 6 +++--- Tests/Functional/FunctionalTests.xml | 1 - Tests/Functional/Indexing/IndexTcaTableTest.php | 8 ++++---- Tests/Unit/UnitTests.xml | 1 - 10 files changed, 24 insertions(+), 35 deletions(-) diff --git a/Classes/Command/IndexCommandController.php b/Classes/Command/IndexCommandController.php index 8f61e6f..c92eb65 100644 --- a/Classes/Command/IndexCommandController.php +++ b/Classes/Command/IndexCommandController.php @@ -50,6 +50,6 @@ class IndexCommandController extends CommandController { // TODO: Allow to index multiple tables at once? // TODO: Also allow to index everything? - $this->indexerFactory->getIndexer($table)->index(); + $this->indexerFactory->getIndexer($table)->indexAllDocuments(); } } diff --git a/Classes/Connection/ConnectionInterface.php b/Classes/Connection/ConnectionInterface.php index a47bf11..fdd09e3 100644 --- a/Classes/Connection/ConnectionInterface.php +++ b/Classes/Connection/ConnectionInterface.php @@ -28,14 +28,12 @@ interface ConnectionInterface /** * Will add a new document. * - * TODO: Should be addDocument - * * @param string $documentType * @param array $document * * @return void */ - public function add($documentType, array $document); + public function addDocument($documentType, array $document); /** * Add the given documents. @@ -50,26 +48,26 @@ interface ConnectionInterface /** * Will update an existing document. * - * TODO: updateDocument (what about batches? consistency) + * NOTE: Batch updating is not yet supported. * * @param string $documentType * @param array $document * * @return void */ - public function update($documentType, array $document); + public function updateDocument($documentType, array $document); /** * Will remove an existing document. * - * TODO: deleteDocument (what about batches? consistency) + * NOTE: Batch deleting is not yet supported. * * @param string $documentType * @param int $identifier * * @return void */ - public function delete($documentType, $identifier); + public function deleteDocument($documentType, $identifier); /** * Search by given request and return result. diff --git a/Classes/Connection/Elasticsearch.php b/Classes/Connection/Elasticsearch.php index 49d169b..42c4359 100644 --- a/Classes/Connection/Elasticsearch.php +++ b/Classes/Connection/Elasticsearch.php @@ -80,7 +80,7 @@ class Elasticsearch implements Singleton, ConnectionInterface $this->documentFactory = $documentFactory; } - public function add($documentType, array $document) + public function addDocument($documentType, array $document) { $this->withType( $documentType, @@ -90,7 +90,7 @@ class Elasticsearch implements Singleton, ConnectionInterface ); } - public function delete($documentType, $identifier) + public function deleteDocument($documentType, $identifier) { $this->withType( $documentType, @@ -100,7 +100,7 @@ class Elasticsearch implements Singleton, ConnectionInterface ); } - public function update($documentType, array $document) + public function updateDocument($documentType, array $document) { $this->withType( $documentType, @@ -110,12 +110,6 @@ class Elasticsearch implements Singleton, ConnectionInterface ); } - /** - * Add the given documents to elasticsearch. - * - * @param string $documentType - * @param array $documents - */ public function addDocuments($documentType, array $documents) { $this->withType( @@ -141,6 +135,7 @@ class Elasticsearch implements Singleton, ConnectionInterface /** * @param SearchRequestInterface $searchRequest + * * @return \Elastica\ResultSet */ public function search(SearchRequestInterface $searchRequest) @@ -152,7 +147,7 @@ class Elasticsearch implements Singleton, ConnectionInterface // TODO: Return wrapped result to implement our interface. // Also update php doc to reflect the change. - return $search->search($searchRequest->getSearchTerm()); + return $search->search('"' . $searchRequest->getSearchTerm() . '"'); } /** diff --git a/Classes/Domain/Index/IndexerInterface.php b/Classes/Domain/Index/IndexerInterface.php index 27c0d3c..d70b410 100644 --- a/Classes/Domain/Index/IndexerInterface.php +++ b/Classes/Domain/Index/IndexerInterface.php @@ -30,7 +30,7 @@ interface IndexerInterface * * @return void */ - public function index(); + public function indexAllDocuments(); /** * Fetches a single document from the indexerService and pushes it to the connection. @@ -38,7 +38,6 @@ interface IndexerInterface * @param string $identifier identifier, the indexer needs to identify a single document * * @return void - * TODO: is record the correct name? (minor) */ - public function indexRecord($identifier); + public function indexDocument($identifier); } diff --git a/Classes/Domain/Index/TcaIndexer.php b/Classes/Domain/Index/TcaIndexer.php index a55d4c5..229f9e8 100644 --- a/Classes/Domain/Index/TcaIndexer.php +++ b/Classes/Domain/Index/TcaIndexer.php @@ -65,7 +65,7 @@ class TcaIndexer implements IndexerInterface $this->connection = $connection; } - public function index() + public function indexAllDocuments() { $this->logger->info('Start indexing'); foreach ($this->getRecordGenerator() as $records) { @@ -79,10 +79,10 @@ class TcaIndexer implements IndexerInterface $this->logger->info('Finish indexing'); } - public function indexRecord($identifier) + public function indexDocument($identifier) { $this->logger->info('Start indexing single record.', [$identifier]); - $this->connection->add($this->tcaTableService->getTableName(), $this->getRecord($identifier)); + $this->connection->addDocument($this->tcaTableService->getTableName(), $this->getRecord($identifier)); $this->logger->info('Finish indexing'); } diff --git a/Classes/Domain/Model/SearchRequest.php b/Classes/Domain/Model/SearchRequest.php index 900a11a..1de2f71 100644 --- a/Classes/Domain/Model/SearchRequest.php +++ b/Classes/Domain/Model/SearchRequest.php @@ -55,7 +55,6 @@ class SearchRequest implements SearchRequestInterface */ public function getSearchTerm() { - //TODO: This seems to be connection specific - return '"' . $this->query . '"'; + return $this->query; } } diff --git a/Classes/Domain/Service/DataHandler.php b/Classes/Domain/Service/DataHandler.php index 300f7d6..1b611c9 100644 --- a/Classes/Domain/Service/DataHandler.php +++ b/Classes/Domain/Service/DataHandler.php @@ -66,7 +66,7 @@ class DataHandler implements Singleton public function add($table, array $record) { $this->logger->debug('Record received for add.', [$table, $record]); - $this->indexerFactory->getIndexer($table)->indexRecord($record['uid']); + $this->indexerFactory->getIndexer($table)->indexDocument($record['uid']); } /** @@ -75,7 +75,7 @@ class DataHandler implements Singleton public function update($table, array $record) { $this->logger->debug('Record received for update.', [$table, $record]); - $this->indexerFactory->getIndexer($table)->indexRecord($record['uid']); + $this->indexerFactory->getIndexer($table)->indexDocument($record['uid']); } /** @@ -85,6 +85,6 @@ class DataHandler implements Singleton public function delete($table, $identifier) { $this->logger->debug('Record received for delete.', [$table, $identifier]); - $this->connection->delete($table, $identifier); + $this->connection->deleteDocument($table, $identifier); } } diff --git a/Tests/Functional/FunctionalTests.xml b/Tests/Functional/FunctionalTests.xml index 44a3f8f..d42ef21 100644 --- a/Tests/Functional/FunctionalTests.xml +++ b/Tests/Functional/FunctionalTests.xml @@ -29,6 +29,5 @@ - diff --git a/Tests/Functional/Indexing/IndexTcaTableTest.php b/Tests/Functional/Indexing/IndexTcaTableTest.php index 6588c78..808dca4 100644 --- a/Tests/Functional/Indexing/IndexTcaTableTest.php +++ b/Tests/Functional/Indexing/IndexTcaTableTest.php @@ -44,7 +44,7 @@ class IndexTcaTableTest extends AbstractFunctionalTestCase \TYPO3\CMS\Core\Utility\GeneralUtility::makeInstance(ObjectManager::class) ->get(IndexerFactory::class) ->getIndexer('tt_content') - ->index() + ->indexAllDocuments() ; $response = $this->client->request('typo3content/_search?q=*:*'); @@ -82,10 +82,10 @@ class IndexTcaTableTest extends AbstractFunctionalTestCase ->getIndexer('tt_content') ; - $indexer->index(); + $indexer->indexAllDocuments(); // Index 2nd time, index already exists in elasticsearch. - $indexer->index(); + $indexer->indexAllDocuments(); $response = $this->client->request('typo3content/_search?q=*:*'); @@ -104,7 +104,7 @@ class IndexTcaTableTest extends AbstractFunctionalTestCase \TYPO3\CMS\Core\Utility\GeneralUtility::makeInstance(ObjectManager::class) ->get(IndexerFactory::class) ->getIndexer('tt_content') - ->index() + ->indexAllDocuments() ; $response = $this->client->request('typo3content/_search?q=*:*'); diff --git a/Tests/Unit/UnitTests.xml b/Tests/Unit/UnitTests.xml index 67719fc..ac98de1 100644 --- a/Tests/Unit/UnitTests.xml +++ b/Tests/Unit/UnitTests.xml @@ -29,6 +29,5 @@ - From 7cabcea0d5f72518e701a58f6abb24053f36a51a Mon Sep 17 00:00:00 2001 From: Daniel Siepmann Date: Thu, 15 Dec 2016 09:24:43 +0100 Subject: [PATCH 3/3] TASK: Explain why this todo is not resolved yet --- Classes/Domain/Service/DataHandler.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Classes/Domain/Service/DataHandler.php b/Classes/Domain/Service/DataHandler.php index 1b611c9..cf24616 100644 --- a/Classes/Domain/Service/DataHandler.php +++ b/Classes/Domain/Service/DataHandler.php @@ -28,7 +28,8 @@ use TYPO3\CMS\Core\SingletonInterface as Singleton; * This is the place to add mappings of further parts to adjust the data before * sending ot to connection. * - * TODO: Probably a candidate for deletion + * TODO: Probably a candidate for deletion. Currently this class makes use of + * extbase DI. We have to resolve this somehow. */ class DataHandler implements Singleton {