Skip to content

Commit b3cbc65

Browse files
skurfuerstskurfuerst
committed
Merge pull request #91 from daniellienert/querybuilder-refactoring
[TASK] Querybuilder refactoring
2 parents c88d809 + 8212db0 commit b3cbc65

4 files changed

Lines changed: 230 additions & 92 deletions

File tree

‎Classes/Flowpack/ElasticSearch/ContentRepositoryAdaptor/Eel/ElasticSearchQueryBuilder.php‎

Lines changed: 69 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -68,13 +68,6 @@ class ElasticSearchQueryBuilder implements QueryBuilderInterface, ProtectedConte
6868
*/
6969
protected $unsupportedFieldsInCountRequest = array('fields', 'sort', 'from', 'size', 'highlight', 'aggs', 'aggregations');
7070

71-
/**
72-
* Amount of total items in response without limit
73-
*
74-
* @var integer
75-
*/
76-
protected $totalItems;
77-
7871
/**
7972
* This (internal) array stores, for the last search request, a mapping from Node Identifiers
8073
* to the full ElasticSearch Hit which was returned.
@@ -85,12 +78,6 @@ class ElasticSearchQueryBuilder implements QueryBuilderInterface, ProtectedConte
8578
*/
8679
protected $elasticSearchHitsIndexedByNodeFromLastRequest;
8780

88-
/**
89-
* This (internal) array stores all aggregation results for the last search request
90-
*
91-
* @var array
92-
*/
93-
protected $elasticSearchAggregationsFromLastRequest;
9481

9582
/**
9683
* The ElasticSearch request, as it is being built up.
@@ -148,6 +135,13 @@ class ElasticSearchQueryBuilder implements QueryBuilderInterface, ProtectedConte
148135
'fields' => array('__path')
149136
);
150137

138+
139+
/**
140+
* @var array
141+
*/
142+
protected $result = array();
143+
144+
151145
/**
152146
* HIGH-LEVEL API
153147
*/
@@ -342,7 +336,7 @@ public function lessThanOrEqual($propertyName, $value) {
342336
*/
343337
public function queryFilter($filterType, $filterOptions, $clauseType = 'must') {
344338
if (!in_array($clauseType, array('must', 'should', 'must_not'))) {
345-
throw new QueryBuildingException('The given clause type "' . $clauseType . '" is not supported. Must be one of "mmust", "should", "must_not".', 1383716082);
339+
throw new QueryBuildingException('The given clause type "' . $clauseType . '" is not supported. Must be one of "must", "should", "must_not".', 1383716082);
346340
}
347341
return $this->appendAtPath('query.filtered.filter.bool.' . $clauseType, array($filterType => $filterOptions));
348342
}
@@ -518,7 +512,9 @@ public function log($message = NULL) {
518512
* @return integer
519513
*/
520514
public function getTotalItems() {
521-
return $this->totalItems;
515+
if (array_key_exists('total', $this->result)) {
516+
return (int) $this->result['total'];
517+
}
522518
}
523519

524520
/**
@@ -551,7 +547,7 @@ public function getFullElasticSearchHitForNode(NodeInterface $node) {
551547
/**
552548
* Execute the query and return the list of nodes as result.
553549
*
554-
* This method is rather internal; just to be called from the ElasticSearchQueryResult. For the public API, please use execute()
550+
* This method is rather internal; just to be called from the ElasticSearchQueryResult. For the public API, please use execute()
555551
*
556552
* @return array<\TYPO3\TYPO3CR\Domain\Model\NodeInterface>
557553
*/
@@ -560,65 +556,20 @@ public function fetch() {
560556
$response = $this->elasticSearchClient->getIndex()->request('GET', '/_search', array(), json_encode($this->request));
561557
$timeAfterwards = microtime(TRUE);
562558

563-
$treatedContent = $response->getTreatedContent();
564-
$hits = $treatedContent['hits'];
565-
566-
if ($this->logThisQuery === TRUE) {
567-
$this->logger->log('Query Log (' . $this->logMessage . '): ' . json_encode($this->request) . ' -- execution time: ' . (($timeAfterwards - $timeBefore) * 1000) . ' ms -- Limit: ' . $this->limit . ' -- Number of results returned: ' . count($hits['hits']) . ' -- Total Results: ' . $hits['total'], LOG_DEBUG);
568-
}
569-
570-
$this->totalItems = $hits['total'];
571-
572-
if ($hits['total'] === 0) {
573-
return array();
574-
}
575-
576-
$nodes = array();
577-
$elasticSearchHitPerNode = array();
559+
$this->result = $response->getTreatedContent();
578560

579-
/**
580-
* TODO: This code below is not fully correct yet:
581-
*
582-
* We always fetch $limit * (numerOfWorkspaces) records; so that we find a node:
583-
* - *once* if it is only in live workspace and matches the query
584-
* - *once* if it is only in user workspace and matches the query
585-
* - *twice* if it is in both workspaces and matches the query *both times*. In this case we filter the duplicate record.
586-
* - *once* if it is in the live workspace and has been DELETED in the user workspace (STILL WRONG)
587-
* - *once* if it is in the live workspace and has been MODIFIED to NOT MATCH THE QUERY ANYMORE in user workspace (STILL WRONG)
588-
*
589-
* If we want to fix this cleanly, we'd need to do an *additional query* in order to filter all nodes from a non-user workspace
590-
* which *do exist in the user workspace but do NOT match the current query*. This has to be done somehow "recursively"; and later
591-
* we might be able to use https://github.com/elasticsearch/elasticsearch/issues/3300 as soon as it is merged.
592-
*/
593-
foreach ($hits['hits'] as $hit) {
594-
// with ElasticSearch 1.0 fields always returns an array,
595-
// see https://github.com/Flowpack/Flowpack.ElasticSearch.ContentRepositoryAdaptor/issues/17
596-
if (is_array($hit['fields']['__path'])) {
597-
$nodePath = current($hit['fields']['__path']);
598-
} else {
599-
$nodePath = $hit['fields']['__path'];
600-
}
601-
$node = $this->contextNode->getNode($nodePath);
602-
if ($node instanceof NodeInterface) {
603-
$nodes[$node->getIdentifier()] = $node;
604-
$elasticSearchHitPerNode[$node->getIdentifier()] = $hit;
605-
if ($this->limit > 0 && count($nodes) >= $this->limit) {
606-
break;
607-
}
608-
}
561+
$this->result['nodes'] = array();
562+
if (array_key_exists('hits', $this->result) && is_array($this->result['hits']) && count($this->result['hits']) > 0) {
563+
$this->result['nodes'] = $this->convertHitsToNodes($this->result['hits']);
609564
}
610565

611566
if ($this->logThisQuery === TRUE) {
612-
$this->logger->log('Query Log (' . $this->logMessage . ') Number of returned results: ' . count($nodes), LOG_DEBUG);
567+
$this->logger->log(sprintf('Query Log (%s): %s -- execution time: %s ms -- Limit: %s -- Number of results returned: %s -- Total Results: %s',
568+
$this->logMessage, json_encode($this->request), (($timeAfterwards - $timeBefore) * 1000), $this->limit, count($this->result['hits']['hits']), $this->result['hits']['total'])
569+
, LOG_DEBUG);
613570
}
614571

615-
$this->elasticSearchHitsIndexedByNodeFromLastRequest = $elasticSearchHitPerNode;
616-
617-
if(array_key_exists("aggregations", $treatedContent)) {
618-
$this->elasticSearchAggregationsFromLastRequest = $treatedContent['aggregations'];
619-
}
620-
621-
return array_values($nodes);
572+
return $this->result;
622573
}
623574

624575
/**
@@ -633,12 +584,6 @@ public function execute() {
633584
return $result;
634585
}
635586

636-
/**
637-
* @return array
638-
*/
639-
public function getElasticSearchAggregationsFromLastRequest() {
640-
return $this->elasticSearchAggregationsFromLastRequest;
641-
}
642587

643588
/**
644589
* Return the total number of hits for the query.
@@ -751,4 +696,53 @@ public function query(NodeInterface $contextNode) {
751696
public function allowsCallOfMethod($methodName) {
752697
return TRUE;
753698
}
699+
700+
/**
701+
* @param array $hits
702+
* @return array Array of Node objects
703+
*/
704+
protected function convertHitsToNodes(array $hits) {
705+
$nodes = array();
706+
$elasticSearchHitPerNode = array();
707+
708+
/**
709+
* TODO: This code below is not fully correct yet:
710+
*
711+
* We always fetch $limit * (numerOfWorkspaces) records; so that we find a node:
712+
* - *once* if it is only in live workspace and matches the query
713+
* - *once* if it is only in user workspace and matches the query
714+
* - *twice* if it is in both workspaces and matches the query *both times*. In this case we filter the duplicate record.
715+
* - *once* if it is in the live workspace and has been DELETED in the user workspace (STILL WRONG)
716+
* - *once* if it is in the live workspace and has been MODIFIED to NOT MATCH THE QUERY ANYMORE in user workspace (STILL WRONG)
717+
*
718+
* If we want to fix this cleanly, we'd need to do an *additional query* in order to filter all nodes from a non-user workspace
719+
* which *do exist in the user workspace but do NOT match the current query*. This has to be done somehow "recursively"; and later
720+
* we might be able to use https://github.com/elasticsearch/elasticsearch/issues/3300 as soon as it is merged.
721+
*/
722+
foreach ($hits['hits'] as $hit) {
723+
// with ElasticSearch 1.0 fields always returns an array,
724+
// see https://github.com/Flowpack/Flowpack.ElasticSearch.ContentRepositoryAdaptor/issues/17
725+
if (is_array($hit['fields']['__path'])) {
726+
$nodePath = current($hit['fields']['__path']);
727+
} else {
728+
$nodePath = $hit['fields']['__path'];
729+
}
730+
$node = $this->contextNode->getNode($nodePath);
731+
if ($node instanceof NodeInterface) {
732+
$nodes[$node->getIdentifier()] = $node;
733+
$elasticSearchHitPerNode[$node->getIdentifier()] = $hit;
734+
if ($this->limit > 0 && count($nodes) >= $this->limit) {
735+
break;
736+
}
737+
}
738+
}
739+
740+
if ($this->logThisQuery === TRUE) {
741+
$this->logger->log('Query Log (' . $this->logMessage . ') Number of returned results: ' . count($nodes), LOG_DEBUG);
742+
}
743+
744+
$this->elasticSearchHitsIndexedByNodeFromLastRequest = $elasticSearchHitPerNode;
745+
746+
return array_values($nodes);
747+
}
754748
}

‎Classes/Flowpack/ElasticSearch/ContentRepositoryAdaptor/Eel/ElasticSearchQueryResult.php‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,12 @@ class ElasticSearchQueryResult implements QueryResultInterface, ProtectedContext
2525
/**
2626
* @var array
2727
*/
28-
protected $results = NULL;
28+
protected $result = NULL;
29+
30+
/**
31+
* @var array
32+
*/
33+
protected $nodes = NULL;
2934

3035
/**
3136
* @var integer
@@ -40,9 +45,10 @@ public function __construct(ElasticSearchQuery $elasticSearchQuery) {
4045
* Initialize the results by really executing the query
4146
*/
4247
protected function initialize() {
43-
if ($this->results === NULL) {
48+
if ($this->result === NULL) {
4449
$queryBuilder = $this->elasticSearchQuery->getQueryBuilder();
45-
$this->results = $queryBuilder->fetch();
50+
$this->result = $queryBuilder->fetch();
51+
$this->nodes = $this->result['nodes'];
4652
$this->count = $queryBuilder->getTotalItems();
4753
}
4854
}
@@ -59,80 +65,80 @@ public function getQuery() {
5965
*/
6066
public function current() {
6167
$this->initialize();
62-
return current($this->results);
68+
return current($this->nodes);
6369
}
6470

6571
/**
6672
* {@inheritdoc}
6773
*/
6874
public function next() {
6975
$this->initialize();
70-
return next($this->results);
76+
return next($this->nodes);
7177
}
7278

7379
/**
7480
* {@inheritdoc}
7581
*/
7682
public function key() {
7783
$this->initialize();
78-
return key($this->results);
84+
return key($this->nodes);
7985
}
8086

8187
/**
8288
* {@inheritdoc}
8389
*/
8490
public function valid() {
8591
$this->initialize();
86-
return current($this->results) !== FALSE;
92+
return current($this->nodes) !== FALSE;
8793
}
8894

8995
/**
9096
* {@inheritdoc}
9197
*/
9298
public function rewind() {
9399
$this->initialize();
94-
reset($this->results);
100+
reset($this->nodes);
95101
}
96102

97103
/**
98104
* {@inheritdoc}
99105
*/
100106
public function offsetExists($offset) {
101107
$this->initialize();
102-
return isset($this->results[$offset]);
108+
return isset($this->nodes[$offset]);
103109
}
104110

105111
/**
106112
* {@inheritdoc}
107113
*/
108114
public function offsetGet($offset) {
109115
$this->initialize();
110-
return $this->results[$offset];
116+
return $this->nodes[$offset];
111117
}
112118

113119
/**
114120
* {@inheritdoc}
115121
*/
116122
public function offsetSet($offset, $value) {
117123
$this->initialize();
118-
$this->results[$offset] = $value;
124+
$this->nodes[$offset] = $value;
119125
}
120126

121127
/**
122128
* {@inheritdoc}
123129
*/
124130
public function offsetUnset($offset) {
125131
$this->initialize();
126-
unset($this->results[$offset]);
132+
unset($this->nodes[$offset]);
127133
}
128134

129135
/**
130136
* {@inheritdoc}
131137
*/
132138
public function getFirst() {
133139
$this->initialize();
134-
if (count($this->results) > 0) {
135-
return array_slice($this->results, 0, 1);
140+
if (count($this->nodes) > 0) {
141+
return array_slice($this->nodes, 0, 1);
136142
}
137143
}
138144

@@ -141,7 +147,7 @@ public function getFirst() {
141147
*/
142148
public function toArray() {
143149
$this->initialize();
144-
return $this->results;
150+
return $this->nodes;
145151
}
146152

147153
/**
@@ -161,15 +167,15 @@ public function count() {
161167
*/
162168
public function getAccessibleCount() {
163169
$this->initialize();
164-
return count($this->results);
170+
return count($this->nodes);
165171
}
166172

167173
/**
168174
* @return array
169175
*/
170176
public function getAggregations() {
171177
$this->initialize();
172-
return $this->elasticSearchQuery->getQueryBuilder()->getElasticSearchAggregationsFromLastRequest();
178+
return $this->result['aggregations'];
173179
}
174180

175181
/**

‎Configuration/Testing/Settings.yaml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
TYPO3:
2+
TYPO3CR:
3+
Search:
4+
elasticSearch:
5+
indexName: typo3cr_testing
6+
log:
7+
backendOptions:
8+
fileBackend:
9+
logFileURL: '%FLOW_PATH_DATA%Logs/ElasticSearch_Testing.log'

0 commit comments

Comments
 (0)