ongr-io / ElasticsearchDSL

Query DSL library for Elasticsearch
MIT License
461 stars 200 forks source link

Field Extra Parameters are wrong on some query #280

Open homersimpsons opened 5 years ago

homersimpsons commented 5 years ago

For some query like the RangeQuery or the TermQuery an extra parameter like _name (used for Named Queries) won't be at the top level.

Example:

$range = new RangeQuery('field', ['gt' => 0]);
$range->addParameter('_name', 'myname');
$range->toArray();
/*
Result:
[
    'range' => [
        'field' => [
            'gt' => 0,
            '_name' => 'myname'
        ]
    ]
]
Instead of expected:
[
    'range' => [
        'field' => [
            'gt' => 0
        ],
        '_name' => 'myname'
    ]
]
*/

This is due to the use of parameters as query parameters and not extra ones https://github.com/ongr-io/ElasticsearchDSL/blob/197eeddacccf2195bd7a9e0572ef24bb50a700c8/src/Query/TermLevel/RangeQuery.php#L72

It's the same for TermQuery https://github.com/ongr-io/ElasticsearchDSL/blob/197eeddacccf2195bd7a9e0572ef24bb50a700c8/src/Query/TermLevel/TermQuery.php#L70 I think here the processArray() should be called after...

This works with a Terms query:

$terms = new TermsQuery('field', ['values']);
$terms->addParameter('_name', 'myname');
$terms->toArray();
/*
Result:
[
    'terms' => [
        'field' => [
            'values'
        ]
        '_name' => 'myname'
    ]
]
*/
saimaz commented 5 years ago

An excellent catch @homersimpsons. Will write a test for those cases and will double check if repositioning processArray() fix the issue.

homersimpsons commented 5 years ago

@saimaz Thank you.

Moving the processArray() won't give the expected result, for example if one use 'boost' => 2.0 he expect it to be at the FieldLevel (i.e. 'field' => ['boost' => 2.0, ...]) so this processArray() should stay here...

I guess the best is to get 2 array of parameters:

Then in the process of building the array this will be generated

[
    $this->field => $this->fieldParameters,
    ...$this->queryParameters // (via array_merge / processArray())
]

So the resulting Query would be

[
    $query->getType() => [
        'field' => [FieldParameters],
        ...QueryParameters
    ]
]