Query "exists" in Latest not supported

[Zyqsempai]

Hi there, i got this error "query exists in Latest not supported", from here https://github.com/ruflin/Elastica/blob/master/lib/Elastica/QueryBuilder/Facade.php#L55 ,but real cause lays here https://github.com/ruflin/Elastica/blob/master/lib/Elastica/QueryBuilder/Version/Version240.php#L15 few methods from Query are missing in $queries array.
We have few ways to solve this problem.
1)First and easiest is just to add missing methods to $queries array.
2)Second method is pretty easy too, we can add new Version, with missing methods.
But i got the feeling that we doing something completely wrong by storing methods names statically in array, same for aggregations and suggestions DSL.
Thats why i think that we need to get methods names dynamically from their classes.
What do you think, which method do you prefer, point me and i will make PR.

[ruflin]

We should probably take a more general look at this problem as the query builder is kind of unmaintained. As Elastica and ES are now in sync again related to versions I'm not sure if it makes sense to keep the "Version" feature in place but only support the version it is for and remove all other methods. I remember @Tobion also had some thoughts about this.

If we need a quick solution, we should probably go for option 1. But if we have the time, lets take a step back and have a general look at the query builder future. More thoughts on this would be appreciated, especially as I never really use the query builder myself and I'm interested to hear more in the use cases around it.

[Tobion]

I'd prefer to deprecate the QueryBuilder as it's intention to support many ES version within Elastica is not how Elastica is maintained at the moment.
Also a QueryBuilder is usually there to build a query with a fluent interface and with a shorter notation like in Doctrine. But the elastica queries already have a fluent interface themselves and there is nothing to shorten. So using the QueryBuilder doesn't bring any advantage.

@Zyqsempai could you please give us examples how you use the QueryBuilder just to be sure we're not forgetting a use-case.

[Zyqsempai]

@ruflin I have an opposite position, i always use QueryBuilder, one of the main reason, is what @Tobion mentioned above, it's really cool to have same interfaces for DB and ES queries, and our current QB really have many common things with Doctrine QB and it's really safe for team and project to have such a simplicity, i can be sure that some one new for ES have an intuitive way to edit and create new queries.
I think that many of Elastica users choosed Elastica because of QB. And it will be a big mistake to remove that functionality, but i agree that we need to refactor it, first of all as @ruflin mentioned we can remove Version layer QB mast be syncronized to Elastica current version there is no way to keep few versions of QB.
Also, since i always used QB and never native Elastica queries i admit that maybe i am wrong)))
Here is a simply example of QB query in my project, how it will looks like with native query?

$this->query = $qb->query()->bool()->addShould(
                                $qb->query()->function_score()->setBoost(100)->setQuery(
                                    $qb->query()->term()->setTerm('number.raw', $rawQuery)
                                )
                            )->addShould(
                                $qb->query()->function_score()->setBoost(20)->setQuery(
                                    $qb->query()->term()->setTerm('number.clean', $clearQuery)
                                )
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', $clearQuery.'*', '2.0')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery, '1.5')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery.'*')
                            );

[Zyqsempai]

There are no response from you guys, so i will make simple PR with actions from step (1), that will "relieve tension" for some time.

[ruflin]

Looking at your examples above I agree that we should keep the query builder, but clean it up. We must make sure it is also tested to if we change things they don't break.

@Zyqsempai Will merge PR #1269 and looking forward to more contributions related to QB ;-)

[Tobion]

The query builder example you gave

$this->query = $qb->query()->bool()->addShould(
                                $qb->query()->function_score()->setBoost(100)->setQuery(
                                    $qb->query()->term()->setTerm('number.raw', $rawQuery)
                                )
                            )->addShould(
                                $qb->query()->function_score()->setBoost(20)->setQuery(
                                    $qb->query()->term()->setTerm('number.clean', $clearQuery)
                                )
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', $clearQuery.'*', '2.0')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery, '1.5')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery.'*')
                            );

translates to without the query builder:

$this->query = (new BoolQuery())->addShould(
                                (new FunctionScore())->setBoost(100)->setQuery(
                                    (new Term())->setTerm('number.raw', $rawQuery)
                                )
                            )->addShould(
                                (new FunctionScore())->setBoost(20)->setQuery(
                                   (new Term())->setTerm('number.clean', $clearQuery)
                                )
                            )->addShould(
                                (new Wildcard())->setValue('number.clean', $clearQuery.'*', '2.0')
                            )->addShould(
                                (new Wildcard())->setValue('number.clean', '*'.$clearQuery, '1.5')
                            )->addShould(
                                (new Wildcard())->setValue('number.clean', '*'.$clearQuery.'*')
                            );

So instead of calling a method you just instantiate a class. It's even shorter. The query builder doesn't add any value.

[Zyqsempai]

@Tobion it all depends on point of view or maybe just style preference, i think that this is great that we have that kind of choise in our library, i described above the reasons why i prefer that style, and i don't see anything bad in query builder, and i will take care of it from now.

[Tobion]

Having multiple choices without any difference is bad. Bad to maintain, bad to document, bad for consistency, bad for learning. You said yourself you don't know the other style, which shows exactly the problems.

[Zyqsempai]

Sorry, but who and why decided that query builder is a bad practice, for me it's a very good example of incapsulation in right way and in right place, don't forget that Doctrine keep it in that way, and you don't say that Ocramius is wrong here, for me your style are very primitive and not intuitive for new users, but i don't try to convince you, so and you just try to think a little bit different, Elastica lived with those two ways many years without any problem, and we will keep it in that way.

[mrVrAlex]

I agree with @Tobion that it is necessary to further maintenance & support on which need additional efforts. But @Zyqsempai also right - it is great, provide additional choose for developers. For example, code above with "new" (imagine that this is a class method) can not mocking normally and therefore hard testing because of hardcoded dependencies. And querybuilder methods can mock with return mocks for each elastica queries.
So I think we can keep querybuilder and clean up it. For example delete Versions functional (because each elastica version support own elasticsearch version), make querybuilder more fluent (without Facade), etc.

[Tobion]

Why would you want to mock the query? You mock requests and responses, but never queries.

[ruflin]

Let's make sure we do not discuss in this thread about good or bad coding practices because that is something which each engineer has its own perspective on. There are quite a few users out there which use the query build which is great. They way I personally position the query builder is for people that know the json representation in ES of a query and want to quickly write it in php (kind of).

The main thing I worry about query builder is the maintenance part. How much overhead is it to keep maintaining it? How can we assure it keeps working / is tested? Cleaning up the query builder code seems to me a good step in the right direction.

[Tobion]

Closing as #1269 has been merged