From d778ab5126cc8dcfaf018cc27862ca0fc60d5ea4 Mon Sep 17 00:00:00 2001 From: Mathieu Darse Date: Fri, 10 Jul 2015 17:33:10 +0200 Subject: [PATCH] Refactor query context No more private collection map, uses new features from LimitedStructure. From now on, Context tries to return Field objects instead of strings. New context methods: - getUnrestrictedFields() - getPrivateFields() - localizeField(Field) (signature changed) - localizeFieldName(string) QueryContext::localizeField() now takes a Field object, use localizeFieldName() if you want to pass a string. Field::getIndexFieldName() renamed to Field::getIndexField(). Raw index fields are now obtained with Field::getIndexField(true). --- .../Elastic/ElasticSearchEngine.php | 4 +- .../Elastic/Search/QueryContext.php | 69 ++++++++++++------- .../Elastic/Search/QueryHelper.php | 6 +- .../SearchEngine/Elastic/Structure/Field.php | 7 +- .../SearchEngine/AST/QuotedTextNodeTest.php | 18 ++--- .../Phrasea/SearchEngine/AST/TextNodeTest.php | 16 ++--- .../SearchEngine/Search/QueryContextTest.php | 4 +- 7 files changed, 70 insertions(+), 54 deletions(-) diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/ElasticSearchEngine.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/ElasticSearchEngine.php index 3d55882cf5..93617d4710 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/ElasticSearchEngine.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/ElasticSearchEngine.php @@ -309,7 +309,6 @@ class ElasticSearchEngine implements SearchEngineInterface { return new QueryContext( new LimitedStructure($this->structure, $options), - [], $this->locales, $this->app['locale'] ); @@ -401,8 +400,7 @@ class ElasticSearchEngine implements SearchEngineInterface // 2015-05-26 (mdarse) Removed databox filtering. // It was already done by the ACL filter in the query scope, so no // document that shouldn't be displayed can go this far. - $field_name = $field->getIndexFieldName(); - $aggs[$name]['terms']['field'] = sprintf('%s.raw', $field_name); + $aggs[$name]['terms']['field'] = $field->getIndexField(true); } return $aggs; diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Search/QueryContext.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Search/QueryContext.php index bef69453c7..c05f17b1b2 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Search/QueryContext.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Search/QueryContext.php @@ -19,13 +19,10 @@ class QueryContext private $queryLocale; /** @var array */ private $fields; - /** @var array */ - private $privateCollectionMap; - public function __construct(Structure $structure, array $privateCollectionMap, array $locales, $queryLocale, array $fields = null) + public function __construct(Structure $structure, array $locales, $queryLocale, array $fields = null) { $this->structure = $structure; - $this->privateCollectionMap = $privateCollectionMap; $this->locales = $locales; $this->queryLocale = $queryLocale; $this->fields = $fields; @@ -41,7 +38,7 @@ class QueryContext } } - return new static($this->structure, $this->privateCollectionMap, $this->locales, $this->queryLocale, $fields); + return new static($this->structure, $this->locales, $this->queryLocale, $fields); } public function getRawFields() @@ -51,11 +48,8 @@ class QueryContext } $fields = array(); - foreach ($this->fields as $name) { - $field = $this->normalizeField($name); - if ($field) { - $fields[] = sprintf('%s.raw', $field); - } + foreach ($this->getUnrestrictedFields() as $name => $field) { + $fields[] = $field->getIndexField(true); } return $fields; @@ -64,39 +58,62 @@ class QueryContext public function getLocalizedFields() { if ($this->fields === null) { - return $this->localizeField('caption_all'); + return $this->localizeFieldName('caption_all'); } $fields = array(); - foreach ($this->fields as $field) { - $normalized = $this->normalizeField($field); - foreach ($this->localizeField($normalized) as $fields[]); + foreach ($this->getUnrestrictedFields() as $_ => $field) { + foreach ($this->localizeField($field) as $fields[]); } return $fields; } - public function getAllowedPrivateFields() + public function getUnrestrictedFields() { - $allowed_field_names = array_keys($this->privateCollectionMap); - - return array_map(array($this->structure, 'get'), $allowed_field_names); + // TODO Restore search optimization by using "caption_all" field + // (only when $this->fields is null) + return array_intersect_key( + $this->structure->getUnrestrictedFields(), + array_flip($this->fields) + ); } + public function getPrivateFields() + { + $private_fields = $this->structure->getPrivateFields(); + if ($this->fields === null) { + return $private_fields; + } else { + return array_intersect_key($private_fields, array_flip($this->fields)); + } + } + + /** + * @deprecated Use getPrivateFields() instead + */ + public function getAllowedPrivateFields() + { + return $this->getPrivateFields(); + } + + /** + * @deprecated Use getDependantCollections() on a Field from a LimitedStructure + */ public function getAllowedCollectionsOnPrivateField(Field $field) { - $name = $field->getName(); - if (!isset($this->privateCollectionMap[$name])) { - throw new \OutOfRangeException('Given field is not an allowed private field.'); - } - - return $this->privateCollectionMap[$name]; + return $field->getDependantCollections(); } /** * @todo Maybe we should put this logic in Field class? */ - public function localizeField($field) + public function localizeField(Field $field) + { + return $this->localizeFieldName($field->getIndexField()); + } + + private function localizeFieldName($field) { $fields = array(); foreach ($this->locales as $locale) { @@ -122,7 +139,7 @@ class QueryContext return null; } // TODO Field label dereferencing (we only want names) - return $field->getIndexFieldName(); + return $field->getIndexField(); } public function getFields() diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Search/QueryHelper.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Search/QueryHelper.php index ba940bd20d..ab677f6ade 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Search/QueryHelper.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Search/QueryHelper.php @@ -12,15 +12,15 @@ class QueryHelper // (instead of a clause for each field, with his collection set) $fields_map = []; $collections_map = []; - foreach ($context->getAllowedPrivateFields() as $field) { - $collections = $context->getAllowedCollectionsOnPrivateField($field); + foreach ($context->getPrivateFields() as $field) { + $collections = $field->getDependantCollections(); $hash = self::hashCollections($collections); $collections_map[$hash] = $collections; if (!isset($fields_map[$hash])) { $fields_map[$hash] = []; } // Merge fields with others having the same collections - $fields = $context->localizeField($field->getIndexFieldName()); + $fields = $context->localizeField($field); foreach ($fields as $fields_map[$hash][]); } diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Field.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Field.php index bda4700bf5..3c4ee4c65e 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Field.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Field.php @@ -95,12 +95,13 @@ class Field return $this->name; } - public function getIndexFieldName() + public function getIndexField($raw = false) { return sprintf( - '%scaption.%s', + '%scaption.%s%s', $this->is_private ? 'private_' : '', - $this->name + $this->name, + $raw ? '.raw' : '' ); } diff --git a/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/QuotedTextNodeTest.php b/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/QuotedTextNodeTest.php index 7d62972848..cd005212cc 100644 --- a/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/QuotedTextNodeTest.php +++ b/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/QuotedTextNodeTest.php @@ -25,7 +25,7 @@ class QuotedTextNodeTest extends \PHPUnit_Framework_TestCase { $query_context = $this->prophesize(QueryContext::class); $query_context->getLocalizedFields()->willReturn(['foo.fr', 'foo.en']); - $query_context->getAllowedPrivateFields()->willReturn([]); + $query_context->getPrivateFields()->willReturn([]); $node = new QuotedTextNode('bar'); $query = $node->buildQuery($query_context->reveal()); @@ -44,20 +44,20 @@ class QuotedTextNodeTest extends \PHPUnit_Framework_TestCase public function testQueryBuildWithPrivateFields() { $public_field = new Field('foo', Mapping::TYPE_STRING, ['private' => false]); - $private_field = new Field('bar', Mapping::TYPE_STRING, ['private' => true]); + $private_field = new Field('bar', Mapping::TYPE_STRING, [ + 'private' => true, + 'used_by_collections' => [1, 2, 3] + ]); $query_context = $this->prophesize(QueryContext::class); + $query_context + ->getPrivateFields() + ->willReturn([$private_field]); $query_context ->getLocalizedFields() ->willReturn(['foo.fr', 'foo.en']); $query_context - ->getAllowedPrivateFields() - ->willReturn([$private_field]); - $query_context - ->getAllowedCollectionsOnPrivateField($private_field) - ->willReturn([1, 2, 3]); - $query_context - ->localizeField('private_caption.bar') + ->localizeField($private_field) ->willReturn(['private_caption.bar.fr', 'private_caption.bar.en']); $node = new QuotedTextNode('baz'); diff --git a/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/TextNodeTest.php b/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/TextNodeTest.php index d79118f896..1f935c6d54 100644 --- a/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/TextNodeTest.php +++ b/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/TextNodeTest.php @@ -44,7 +44,7 @@ class TextNodeTest extends \PHPUnit_Framework_TestCase { $query_context = $this->prophesize(QueryContext::class); $query_context->getLocalizedFields()->willReturn(['foo.fr', 'foo.en']); - $query_context->getAllowedPrivateFields()->willReturn([]); + $query_context->getPrivateFields()->willReturn([]); $node = new TextNode('bar', new Context('baz')); $query = $node->buildQuery($query_context->reveal()); @@ -63,20 +63,20 @@ class TextNodeTest extends \PHPUnit_Framework_TestCase public function testQueryBuildWithPrivateFields() { $public_field = new Field('foo', Mapping::TYPE_STRING, ['private' => false]); - $private_field = new Field('bar', Mapping::TYPE_STRING, ['private' => true]); + $private_field = new Field('bar', Mapping::TYPE_STRING, [ + 'private' => true, + 'used_by_collections' => [1, 2, 3] + ]); $query_context = $this->prophesize(QueryContext::class); $query_context ->getLocalizedFields() ->willReturn(['foo.fr', 'foo.en']); $query_context - ->getAllowedPrivateFields() + ->getPrivateFields() ->willReturn([$private_field]); $query_context - ->getAllowedCollectionsOnPrivateField($private_field) - ->willReturn([1, 2, 3]); - $query_context - ->localizeField('private_caption.bar') + ->localizeField($private_field) ->willReturn(['private_caption.bar.fr', 'private_caption.bar.en']); $node = new TextNode('baz'); @@ -118,7 +118,7 @@ class TextNodeTest extends \PHPUnit_Framework_TestCase ->getLocalizedFields() ->willReturn(['foo.fr', 'foo.en']); $query_context - ->getAllowedPrivateFields() + ->getPrivateFields() ->willReturn([]); $query_context ->getFields() diff --git a/tests/Alchemy/Tests/Phrasea/SearchEngine/Search/QueryContextTest.php b/tests/Alchemy/Tests/Phrasea/SearchEngine/Search/QueryContextTest.php index 77178cedef..6824b37a9f 100644 --- a/tests/Alchemy/Tests/Phrasea/SearchEngine/Search/QueryContextTest.php +++ b/tests/Alchemy/Tests/Phrasea/SearchEngine/Search/QueryContextTest.php @@ -17,7 +17,7 @@ class QueryContextTest extends \PHPUnit_Framework_TestCase { $structure = $this->prophesize(Structure::class)->reveal(); $available_locales = ['ab', 'cd', 'ef']; - $context = new QueryContext($structure, [], $available_locales, 'fr'); + $context = new QueryContext($structure, $available_locales, 'fr'); $narrowed = $context->narrowToFields(['some_field']); $this->assertEquals(['some_field'], $narrowed->getFields()); } @@ -30,7 +30,7 @@ class QueryContextTest extends \PHPUnit_Framework_TestCase $structure->get('foo')->willReturn($public_field); $structure->get('bar')->willReturn($restricted_field); - $context = new QueryContext($structure->reveal(), [], [], 'fr'); + $context = new QueryContext($structure->reveal(), [], 'fr'); $this->assertEquals('caption.foo', $context->normalizeField('foo')); $this->assertEquals('private_caption.bar', $context->normalizeField('bar')); }