Skip to content

Commit

Permalink
feature #271 [Elasticsearch] pass custom field options even if no con…
Browse files Browse the repository at this point in the history
…dition is set (jkabat)

This PR was merged into the 2.0-dev branch.

Discussion
----------

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #270 
| License       | MIT
| Doc PR        | 

As I don't know elasticasearch internals good enough I'm not sure about other possible use cases besides "inner_hits". This option has to be set for nested/joined field conditionally (when field is really used in the query) and I can only solve it dirty way after generating of query...

I was thinking that we actually need to distinguish field options from conditions options and proper method signature could be:

```php
public function registerField(string $fieldName, string $property, array $conditions = [], array $conditionOptions = [], array $options = [])
{
}
```

prepareProcessedValuesQuery/prepareQuery updated accordingly.

This also brings even more complexity to the code, so other way round could be to leave it solely on developer and use only one "options" argument for all cases but later it could bite us.

@dkarlovi WDYT?

Commits
-------

b5e3fa6 pass field/condition options in all prepareQuery statements
e68696c remove possible typo when options are set for field without conditions
aa20f13 test adding custom options for registered field with no conditions set
  • Loading branch information
sstok committed Aug 18, 2019
2 parents 89cc707 + aa20f13 commit 6db9c56
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 7 deletions.
14 changes: 8 additions & 6 deletions lib/Elasticsearch/QueryConditionGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,8 @@ private function processGroup(ValuesGroup $group, bool $injectParams = false): a
$nested,
$join,
$injectParams,
$conditions
$conditions,
$options
));
}
if ($valuesBag->hasExcludedSimpleValues()) {
Expand All @@ -262,7 +263,8 @@ private function processGroup(ValuesGroup $group, bool $injectParams = false): a
$nested,
$join,
$injectParams,
$conditions
$conditions,
$options
));
}

Expand All @@ -272,15 +274,15 @@ private function processGroup(ValuesGroup $group, bool $injectParams = false): a
foreach ($valuesBag->get(Range::class) as $range) {
$hints->context = QueryPreparationHints::CONTEXT_RANGE_VALUES;
$range = $this->convertRangeValues($range, $valueConverter, $injectParams);
$this->mergeQuery($bool, $includingType, $this->prepareQuery($propertyName, $range, $hints, $queryConverter, $nested, $join, $conditions));
$this->mergeQuery($bool, $includingType, $this->prepareQuery($propertyName, $range, $hints, $queryConverter, $nested, $join, $conditions, $options));
}
}
if ($valuesBag->has(ExcludedRange::class)) {
/** @var Range $range */
foreach ($valuesBag->get(ExcludedRange::class) as $range) {
$hints->context = QueryPreparationHints::CONTEXT_EXCLUDED_RANGE_VALUES;
$range = $this->convertRangeValues($range, $valueConverter, $injectParams);
$this->mergeQuery($bool, self::CONDITION_NOT, $this->prepareQuery($propertyName, $range, $hints, $queryConverter, $nested, $join, $conditions));
$this->mergeQuery($bool, self::CONDITION_NOT, $this->prepareQuery($propertyName, $range, $hints, $queryConverter, $nested, $join, $conditions, $options));
}
}

Expand All @@ -291,7 +293,7 @@ private function processGroup(ValuesGroup $group, bool $injectParams = false): a
$hints->context = QueryPreparationHints::CONTEXT_COMPARISON;
$compare = $this->convertCompareValue($compare, $valueConverter, $injectParams);
$localIncludingType = self::COMPARISON_UNEQUAL === $compare->getOperator() ? self::CONDITION_NOT : $includingType;
$this->mergeQuery($bool, $localIncludingType, $this->prepareQuery($propertyName, $compare, $hints, $queryConverter, $nested, $join, $conditions));
$this->mergeQuery($bool, $localIncludingType, $this->prepareQuery($propertyName, $compare, $hints, $queryConverter, $nested, $join, $conditions, $options));
}
}

Expand All @@ -302,7 +304,7 @@ private function processGroup(ValuesGroup $group, bool $injectParams = false): a
$patternMatch = $this->convertMatcherValue($patternMatch, $valueConverter, $injectParams);
$hints->context = QueryPreparationHints::CONTEXT_PATTERN_MATCH;
$localIncludingType = $patternMatch->isExclusive() ? self::CONDITION_NOT : $includingType;
$this->mergeQuery($bool, $localIncludingType, $this->prepareQuery($propertyName, $patternMatch, $hints, $queryConverter, $nested, $join, $conditions));
$this->mergeQuery($bool, $localIncludingType, $this->prepareQuery($propertyName, $patternMatch, $hints, $queryConverter, $nested, $join, $conditions, $options));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ protected function getFieldSet(bool $build = true): FieldSet
protected function configureConditionGenerator(QueryConditionGenerator $conditionGenerator)
{
// customer
$conditionGenerator->registerField('customer-comment', 'customers/customers/#note>comment', [], ['_id' => ['unmapped_type' => 'long']]);
$conditionGenerator->registerField('customer-comment', 'customers/customers/#note>comment');
$conditionGenerator->registerField(
'customer-comment-restricted',
'customers/customers/#note>comment',
Expand Down
41 changes: 41 additions & 0 deletions lib/Elasticsearch/Tests/QueryConditionGeneratorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,47 @@ public function it_merges_has_child_queries_from_conditional_order_queries()
self::assertMapping(['@date'], $generator->getMappings());
}

/** @test */
public function it_adds_additional_options_for_nested_queries()
{
$condition = $this->createCondition()
->field('name')
->addSimpleValue('Doctor')
->addSimpleValue('Foo')
->end()
->getSearchCondition();

$generator = new QueryConditionGenerator($condition);
$generator->registerField('name', 'author[].name', [], [
'inner_hits' => [],
]);

self::assertEquals([
'query' => [
'bool' => [
'must' => [
[
'nested' => [
'path' => 'author',
'query' => [
'terms' => [
'author.name' => [
'Doctor',
'Foo',
],
],
],
'inner_hits' => [],
],
],
],
],
],
], $generator->getQuery()->toArray());

self::assertMapping(['name'], $generator->getMappings());
}

/** @test */
public function it_merges_single_level_nested_queries()
{
Expand Down

0 comments on commit 6db9c56

Please sign in to comment.