erichard / elasticsearch-query-builder

Build query for an ElasticSearch client using a fluent interface
MIT License
39 stars 20 forks source link

Allow full Elastic API using a params argument #17

Closed igoooor closed 1 year ago

igoooor commented 1 year ago

I can only image the effort that it is to be able to implement every single options that each query DQL offers (and to maintain it). That's why I thought about giving the possibility to the developers to add any options they would need, that may not be available yet. What do you think?

erichard commented 1 year ago

I see the idea but I don't quite like the implementation with a trait for this.

I suggest the use of a new optional parameter in all constructors array $params = [] so we could create a query like this

new QueryStringQuery(
    query: '(foo bar) OR (bar foo)', 
    params: [
        'unimplemented_param': value  
    ]
] 
igoooor commented 1 year ago

I saw your different traits so I simply copy pasted one ^^ But if you prefer the constructor approach, sure. Would you like me to update this PR?

erichard commented 1 year ago

Traits are for shared params across all queries

Yes please update the PR

igoooor commented 1 year ago

Please let me know if this way is better and if you would like me to change anything. I did my best to follow your patterns

igoooor commented 1 year ago

Oh I realise I forgot the setter for params. I will add it

igoooor commented 1 year ago

setter is now added. While I was at it, I also added \Erichard\ElasticQueryBuilder\Query\QueryStringQuery::$fuzziness I hope that's ok for you

igoooor commented 1 year ago

I would also like to add the following into \Erichard\ElasticQueryBuilder\QueryBuilder:

class QueryBuilder
{
...
    private ?array $fields = null;

    public function setFields(?array $fields): self
    {
        $this->fields = $fields;

        return $this;
    }
...
    public function build(): array
    {
...
        if (null !== $this->fields) {
            $query['body']['fields'] = $this->fields;
        }
...
   }
...
}

This would allows https://www.elastic.co/guide/en/elasticsearch/reference/current/search-fields.html#search-fields-param Tell me if it's ok to be in the same PR, or if you prefer another one. And possibly, the same but for highlight https://www.elastic.co/guide/en/elasticsearch/reference/current/highlighting.html

igoooor commented 1 year ago

I pushed what I mentioned above, and I also added the same params logic to the QueryBuilder, so that you have all the flexibility you may need until 100% of ES features are implemented. Please let me know if I should change anything

igoooor commented 1 year ago

I believe I implemented your changes requests

igoooor commented 1 year ago

Let me know if I can support you further in merging that PR, I assume you must be quite busy

erichard commented 1 year ago

Merged ! Thanks :pray: