From 8e5d09bd769cfda6810db615cb6ee88d5d9f0746 Mon Sep 17 00:00:00 2001 From: Mathieu Darse Date: Tue, 10 Nov 2015 16:26:34 +0100 Subject: [PATCH] Test value compatibility before creating ES query --- .../Elastic/AST/KeyValue/EqualExpression.php | 2 +- .../Elastic/AST/KeyValue/FieldKey.php | 3 +- .../Elastic/AST/KeyValue/MatchExpression.php | 5 ++ .../Elastic/AST/KeyValue/MetadataKey.php | 3 +- .../Elastic/AST/QuotedTextNode.php | 6 +- .../SearchEngine/Elastic/AST/RawNode.php | 6 +- .../SearchEngine/Elastic/AST/TextNode.php | 4 +- .../SearchEngine/Elastic/Structure/Field.php | 38 +-------- .../SearchEngine/Elastic/Structure/Tag.php | 7 +- .../SearchEngine/Elastic/Structure/Typed.php | 13 +++ .../Elastic/Structure/ValueChecker.php | 48 +++++++++++ .../SearchEngine/AST/EqualExpressionTest.php | 21 ++++- .../SearchEngine/AST/MatchExpressionTest.php | 1 + .../Phrasea/SearchEngine/AST/RawNodeTest.php | 3 + .../Structure/ValueCheckerTest.php | 80 +++++++++++++++++++ 15 files changed, 183 insertions(+), 57 deletions(-) create mode 100644 lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Typed.php create mode 100644 lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/ValueChecker.php create mode 100644 tests/Alchemy/Tests/Phrasea/SearchEngine/Structure/ValueCheckerTest.php diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/EqualExpression.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/EqualExpression.php index 6559bf5af4..827147a1e0 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/EqualExpression.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/EqualExpression.php @@ -24,7 +24,7 @@ class EqualExpression extends Node public function buildQuery(QueryContext $context) { if (!$this->key->isValueCompatible($this->value, $context)) { - return null; + throw new QueryException(sprintf('Value "%s" for metadata tag "%s" is not valid.', $this->value, $this->key)); } $query = [ diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/FieldKey.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/FieldKey.php index e238b41cb1..89b6376e31 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/FieldKey.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/FieldKey.php @@ -6,6 +6,7 @@ use Alchemy\Phrasea\SearchEngine\Elastic\Exception\QueryException; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryContext; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryHelper; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryPostProcessor; +use Alchemy\Phrasea\SearchEngine\Elastic\Structure\ValueChecker; use Assert\Assertion; class FieldKey implements Key, QueryPostProcessor @@ -26,7 +27,7 @@ class FieldKey implements Key, QueryPostProcessor public function isValueCompatible($value, QueryContext $context) { - return $this->getField($context)->isValueCompatible($value); + return ValueChecker::isValueCompatible($this->getField($context), $value); } public function postProcessQuery($query, QueryContext $context) diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/MatchExpression.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/MatchExpression.php index 9142e62a2f..89043ca310 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/MatchExpression.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/MatchExpression.php @@ -3,6 +3,7 @@ namespace Alchemy\Phrasea\SearchEngine\Elastic\AST\KeyValue; use Alchemy\Phrasea\SearchEngine\Elastic\AST\Node; +use Alchemy\Phrasea\SearchEngine\Elastic\Exception\QueryException; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryContext; use Assert\Assertion; @@ -20,6 +21,10 @@ class MatchExpression extends Node public function buildQuery(QueryContext $context) { + if (!$this->key->isValueCompatible($this->value, $context)) { + throw new QueryException(sprintf('Value "%s" for metadata tag "%s" is not valid.', $this->value, $this->key)); + } + return [ 'match' => [ $this->key->getIndexField($context) => $this->value diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/MetadataKey.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/MetadataKey.php index 81309dcae7..292b6e7237 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/MetadataKey.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/KeyValue/MetadataKey.php @@ -4,6 +4,7 @@ namespace Alchemy\Phrasea\SearchEngine\Elastic\AST\KeyValue; use Alchemy\Phrasea\SearchEngine\Elastic\Exception\QueryException; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryContext; +use Alchemy\Phrasea\SearchEngine\Elastic\Structure\ValueChecker; use Assert\Assertion; class MetadataKey implements Key @@ -24,7 +25,7 @@ class MetadataKey implements Key public function isValueCompatible($value, QueryContext $context) { - return $this->getTag($context)->isValueCompatible($value); + return ValueChecker::isValueCompatible($this->getTag($context), $value); } private function getTag(QueryContext $context) diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/QuotedTextNode.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/QuotedTextNode.php index fb7ac29409..8d2d5fd9ad 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/QuotedTextNode.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/QuotedTextNode.php @@ -4,7 +4,7 @@ namespace Alchemy\Phrasea\SearchEngine\Elastic\AST; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryContext; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryHelper; -use Alchemy\Phrasea\SearchEngine\Elastic\Structure\Field as StructureField; +use Alchemy\Phrasea\SearchEngine\Elastic\Structure\ValueChecker; class QuotedTextNode extends Node { @@ -38,11 +38,11 @@ class QuotedTextNode extends Node }; $unrestricted_fields = $context->getUnrestrictedFields(); - $unrestricted_fields = StructureField::filterByValueCompatibility($unrestricted_fields, $this->text); + $unrestricted_fields = ValueChecker::filterByValueCompatibility($unrestricted_fields, $this->text); $query = $query_builder($unrestricted_fields); $private_fields = $context->getPrivateFields(); - $private_fields = StructureField::filterByValueCompatibility($private_fields, $this->text); + $private_fields = ValueChecker::filterByValueCompatibility($private_fields, $this->text); foreach (QueryHelper::wrapPrivateFieldQueries($private_fields, $query_builder) as $private_field_query) { $query = QueryHelper::applyBooleanClause($query, 'should', $private_field_query); } diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/RawNode.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/RawNode.php index ac387f0371..784fadcbfa 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/RawNode.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/RawNode.php @@ -4,7 +4,7 @@ namespace Alchemy\Phrasea\SearchEngine\Elastic\AST; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryContext; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryHelper; -use Alchemy\Phrasea\SearchEngine\Elastic\Structure\Field as StructureField; +use Alchemy\Phrasea\SearchEngine\Elastic\Structure\ValueChecker; class RawNode extends Node { @@ -56,11 +56,11 @@ class RawNode extends Node }; $unrestricted_fields = $context->getUnrestrictedFields(); - $unrestricted_fields = StructureField::filterByValueCompatibility($unrestricted_fields, $this->text); + $unrestricted_fields = ValueChecker::filterByValueCompatibility($unrestricted_fields, $this->text); $query = $query_builder($unrestricted_fields); $private_fields = $context->getPrivateFields(); - $private_fields = StructureField::filterByValueCompatibility($private_fields, $this->text); + $private_fields = ValueChecker::filterByValueCompatibility($private_fields, $this->text); foreach (QueryHelper::wrapPrivateFieldQueries($private_fields, $query_builder) as $private_field_query) { $query = QueryHelper::applyBooleanClause($query, 'should', $private_field_query); } diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/TextNode.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/TextNode.php index 8727b6e227..49b8d49da9 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/TextNode.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/AST/TextNode.php @@ -4,7 +4,7 @@ namespace Alchemy\Phrasea\SearchEngine\Elastic\AST; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryContext; use Alchemy\Phrasea\SearchEngine\Elastic\Search\QueryHelper; -use Alchemy\Phrasea\SearchEngine\Elastic\Structure\Field as StructureField; +use Alchemy\Phrasea\SearchEngine\Elastic\Structure\ValueChecker; use Alchemy\Phrasea\SearchEngine\Elastic\Thesaurus\Term; class TextNode extends AbstractTermNode implements ContextAbleInterface @@ -41,7 +41,7 @@ class TextNode extends AbstractTermNode implements ContextAbleInterface $query_builder = function (array $fields) use ($context) { // Full text $index_fields = []; - foreach (StructureField::filterByValueCompatibility($fields, $this->text) as $field) { + foreach (ValueChecker::filterByValueCompatibility($fields, $this->text) as $field) { foreach ($context->localizeField($field) as $f) { $index_fields[] = $f; } diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Field.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Field.php index 39060fd81f..06d8730c52 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Field.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Field.php @@ -4,7 +4,6 @@ namespace Alchemy\Phrasea\SearchEngine\Elastic\Structure; use Alchemy\Phrasea\SearchEngine\Elastic\Exception\MergeException; use Alchemy\Phrasea\SearchEngine\Elastic\Mapping; -use Alchemy\Phrasea\SearchEngine\Elastic\RecordHelper; use Alchemy\Phrasea\SearchEngine\Elastic\Thesaurus\Concept; use Alchemy\Phrasea\SearchEngine\Elastic\Thesaurus\Helper as ThesaurusHelper; use Assert\Assertion; @@ -13,7 +12,7 @@ use databox_field; /** * @todo Field labels */ -class Field +class Field implements Typed { private $name; private $type; @@ -119,41 +118,6 @@ class Field ); } - public function isValueCompatible($value) - { - return count(self::filterByValueCompatibility([$this], $value)) > 0; - } - - public static function filterByValueCompatibility(array $fields, $value) - { - $is_numeric = is_numeric($value); - $is_valid_date = RecordHelper::validateDate($value); - $filtered = []; - foreach ($fields as $field) { - switch ($field->type) { - case Mapping::TYPE_FLOAT: - case Mapping::TYPE_DOUBLE: - case Mapping::TYPE_INTEGER: - case Mapping::TYPE_LONG: - case Mapping::TYPE_SHORT: - case Mapping::TYPE_BYTE: - if ($is_numeric) { - $filtered[] = $field; - } - break; - case Mapping::TYPE_DATE: - if ($is_valid_date) { - $filtered[] = $field; - } - break; - case Mapping::TYPE_STRING: - default: - $filtered[] = $field; - } - } - return $filtered; - } - public function getConceptPathIndexField() { return sprintf('concept_path.%s', $this->name); diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Tag.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Tag.php index 91f0d6a1e0..0a4895832e 100644 --- a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Tag.php +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Tag.php @@ -5,7 +5,7 @@ namespace Alchemy\Phrasea\SearchEngine\Elastic\Structure; use Alchemy\Phrasea\SearchEngine\Elastic\Mapping; use Assert\Assertion; -class Tag +class Tag implements Typed { private $name; private $type; @@ -36,11 +36,6 @@ class Tag return $this->analyzable; } - public function isValueCompatible() - { - return true; - } - public function getIndexField($raw = false) { return sprintf( diff --git a/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Typed.php b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Typed.php new file mode 100644 index 0000000000..bd50d70d07 --- /dev/null +++ b/lib/Alchemy/Phrasea/SearchEngine/Elastic/Structure/Typed.php @@ -0,0 +1,13 @@ + 0; + } + + public static function filterByValueCompatibility(array $list, $value) + { + Assertion::allIsInstanceOf($list, Typed::class); + $is_numeric = is_numeric($value); + $is_valid_date = RecordHelper::validateDate($value); + $filtered = []; + foreach ($list as $item) { + switch ($item->getType()) { + case Mapping::TYPE_FLOAT: + case Mapping::TYPE_DOUBLE: + case Mapping::TYPE_INTEGER: + case Mapping::TYPE_LONG: + case Mapping::TYPE_SHORT: + case Mapping::TYPE_BYTE: + if ($is_numeric) { + $filtered[] = $item; + } + break; + case Mapping::TYPE_DATE: + if ($is_valid_date) { + $filtered[] = $item; + } + break; + case Mapping::TYPE_STRING: + default: + $filtered[] = $item; + } + } + return $filtered; + } +} diff --git a/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/EqualExpressionTest.php b/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/EqualExpressionTest.php index 677f2a5880..6670197fe8 100644 --- a/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/EqualExpressionTest.php +++ b/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/EqualExpressionTest.php @@ -12,7 +12,7 @@ use Alchemy\Phrasea\SearchEngine\Elastic\Structure\Field as StructureField; * @group searchengine * @group ast */ -class FieldEqualsExpressionTest extends \PHPUnit_Framework_TestCase +class EqualExpressionTest extends \PHPUnit_Framework_TestCase { public function testSerialization() { @@ -33,6 +33,7 @@ class FieldEqualsExpressionTest extends \PHPUnit_Framework_TestCase $key = $this->prophesize(Key::class); $key->isValueCompatible($value, $query_context)->willReturn($compatible_value); $key->getIndexField($query_context, true)->willReturn($index_field); + $key->__toString()->willReturn('foo'); // TODO Test keys implementing QueryPostProcessor $node = new EqualExpression($key->reveal(), 'bar'); @@ -56,8 +57,22 @@ class FieldEqualsExpressionTest extends \PHPUnit_Framework_TestCase ['foo.raw', 'bar', true, false, '{ "term": { "foo.raw": "bar" } }'], - ['foo.raw', 'bar', false, true, 'null'], - ['foo.raw', 'bar', false, false, 'null'], ]; } + + /** + * @expectedException Alchemy\Phrasea\SearchEngine\Elastic\Exception\QueryException + * @expectedExceptionMessageRegExp #"foo"#u + */ + public function testQueryBuildWithIncompatibleValue() + { + $query_context = $this->prophesize(QueryContext::class)->reveal(); + $key = $this->prophesize(Key::class); + $key->isValueCompatible('bar', $query_context)->willReturn(false); + $key->getIndexField($query_context, true)->willReturn('foo.raw'); + $key->__toString()->willReturn('foo'); + + $node = new EqualExpression($key->reveal(), 'bar'); + $node->buildQuery($query_context); + } } diff --git a/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/MatchExpressionTest.php b/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/MatchExpressionTest.php index 273a2a1b27..bcb93de7c4 100644 --- a/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/MatchExpressionTest.php +++ b/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/MatchExpressionTest.php @@ -28,6 +28,7 @@ class MatchExpressionTest extends \PHPUnit_Framework_TestCase { $query_context = $this->prophesize(QueryContext::class)->reveal(); $key = $this->prophesize(Key::class); + $key->isValueCompatible('bar', $query_context)->willReturn(true); $key->getIndexField($query_context)->willReturn('foo'); $node = new MatchExpression($key->reveal(), 'bar'); $query = $node->buildQuery($query_context); diff --git a/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/RawNodeTest.php b/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/RawNodeTest.php index e734e2b01b..4c02032741 100644 --- a/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/RawNodeTest.php +++ b/tests/Alchemy/Tests/Phrasea/SearchEngine/AST/RawNodeTest.php @@ -24,6 +24,7 @@ class RawNodeTest extends \PHPUnit_Framework_TestCase public function testQueryBuildOnSingleField() { $field = $this->prophesize(Field::class); + $field->getType()->willReturn(Mapping::TYPE_STRING); $field->getIndexField(true)->willReturn('foo.raw'); $query_context = $this->prophesize(QueryContext::class); @@ -45,8 +46,10 @@ class RawNodeTest extends \PHPUnit_Framework_TestCase public function testQueryBuildOnMultipleFields() { $field_a = $this->prophesize(Field::class); + $field_a->getType()->willReturn(Mapping::TYPE_STRING); $field_a->getIndexField(true)->willReturn('foo.raw'); $field_b = $this->prophesize(Field::class); + $field_b->getType()->willReturn(Mapping::TYPE_STRING); $field_b->getIndexField(true)->willReturn('bar.raw'); $query_context = $this->prophesize(QueryContext::class); diff --git a/tests/Alchemy/Tests/Phrasea/SearchEngine/Structure/ValueCheckerTest.php b/tests/Alchemy/Tests/Phrasea/SearchEngine/Structure/ValueCheckerTest.php new file mode 100644 index 0000000000..f6ef923329 --- /dev/null +++ b/tests/Alchemy/Tests/Phrasea/SearchEngine/Structure/ValueCheckerTest.php @@ -0,0 +1,80 @@ +assertEquals($compatible, ValueChecker::isValueCompatible($subject, $value)); + } + + public function escapeRawProvider() + { + $values = [ + [Mapping::TYPE_FLOAT , 42 , true ], + [Mapping::TYPE_FLOAT , '42' , true ], + [Mapping::TYPE_FLOAT , '42foo' , false], + [Mapping::TYPE_FLOAT , 'foo' , false], + [Mapping::TYPE_DOUBLE , 42 , true ], + [Mapping::TYPE_DOUBLE , '42' , true ], + [Mapping::TYPE_DOUBLE , '42foo' , false], + [Mapping::TYPE_DOUBLE , 'foo' , false], + [Mapping::TYPE_INTEGER, 42 , true ], + [Mapping::TYPE_INTEGER, '42' , true ], + [Mapping::TYPE_INTEGER, '42foo' , false], + [Mapping::TYPE_INTEGER, 'foo' , false], + [Mapping::TYPE_LONG , 42 , true ], + [Mapping::TYPE_LONG , '42' , true ], + [Mapping::TYPE_LONG , '42foo' , false], + [Mapping::TYPE_LONG , 'foo' , false], + [Mapping::TYPE_SHORT , 42 , true ], + [Mapping::TYPE_SHORT , '42' , true ], + [Mapping::TYPE_SHORT , '42foo' , false], + [Mapping::TYPE_SHORT , 'foo' , false], + [Mapping::TYPE_BYTE , 42 , true ], + [Mapping::TYPE_BYTE , '42' , true ], + [Mapping::TYPE_BYTE , '42foo' , false], + [Mapping::TYPE_BYTE , 'foo' , false], + + [Mapping::TYPE_STRING , 'foo' , true ], + [Mapping::TYPE_STRING , '42' , true ], + [Mapping::TYPE_STRING , 42 , true ], + + [Mapping::TYPE_BOOLEAN, true , true ], + [Mapping::TYPE_BOOLEAN, false , true ], + [Mapping::TYPE_BOOLEAN, 'yes' , true ], + [Mapping::TYPE_BOOLEAN, 'no' , true ], + [Mapping::TYPE_BOOLEAN, 'foo' , true ], + [Mapping::TYPE_BOOLEAN, 42 , true ], + + [Mapping::TYPE_DATE , '2015/01/01' , true ], + [Mapping::TYPE_DATE , '2015/01/01 00:00:00', false], + [Mapping::TYPE_DATE , 'foo' , false], + ]; + + foreach ($values as &$value) { + $value[0] = $this->createTypedMock($value[0]); + } + + return $values; + } + + private function createTypedMock($type) + { + $typed = $this->prophesize(Typed::class); + $typed->getType()->willReturn($type)->shouldBeCalled(); + return $typed->reveal(); + } +}