Adds customizable default operator to concatenate conditions

[danieldias51]

Based on the issue #48.
This PR adds an option to define the default operator to join the conditions.

By default multiple conditions on where are joined with AND operator and this new option allows to define to operator (AND or OR) to join the multiple conditions

[mrkamel]

hi, thx for the fast PR. Very cool.

Two points from my side:

a) The grammar ist not yet fully correct
as it no longer allows to specify AND specifically anymore.

The following test should work, but it doesn't (maybe add it to the test suite as well)

  def test_specific_operator
    expected = create(:product, :title => "Title Avengers")
    rejected = create(:product, :title => "Title Inception")

    results = Product.search_with_or_operator("Title AND Avengers")

    assert_includes results, expected
    refute_includes results, rejected
  end

b) I'd probably prefer to add the option as a query option instead of a scope option.

Product.search("Avengers Movie", default_operator: :or)

What do you think?

[mrkamel]

the rule should be

  rule and_expression
    or_expression space? ('AND' / 'and') space? complex_expression <AndExpression> /
    or_expression space !('OR' / 'or') complex_expression <AndOrExpression> /
    or_expression
  end 

[danieldias51]

I was going to say I will need a little help with the grammar, thank for that.

I'm ok having the default_operator as query parameter, actually it's look better in terms of responsibilities.
I may want to re-use the same search_scope, but the query in one place I may want to use OR as default operator and in other place don't.

going to update the code to pass default_operator as query parameter

[danieldias51]

@mrkamel the tests locally are failing intermittently. Really odd, any idea why?

[danieldias51]

@mrkamel Can you confirm if the grammar is correct?
I've been debugging and for the regression test results = Product.search_multi_columns("Title Avengers") is calling the <AndOrExpression> and not the <AndExpression> as was previously

[mrkamel]

hm, i think the issue is: you can't use the reflection like this. The reflection object is build when the search_scope gets defined. Thus, it is global and using it like this would not be thread safe. The pipeline fails, because it is not reset to :and between the tests. Best way imo would be to always pass the query_options (the options hash you added) around besides the query_info object (from unsafe_search to QueryBuilder to GrammarParser.

[danieldias51]

Refactored to pass the options hash along the query. Also renamed the variable to query_options, seems more descriptive.
I wasn't sure were to validate the query_options, I did that on the GrammarParser
Let me know your thoughts

[mrkamel]

very nice, only some minor improvements are required before we can merge it.

[danieldias51]

Two questions:

1. the logic to validate the query_options is duplicated, do you want to extract it to a dedicated class or method?

2. On the hash_parser, I'm just supporting the default_operator in simple cases like this one:
   Product.search(title: "blabla", description: "some desc", default_operator: :or)
   If you have a complex query when you specify conditions, the default operator doesn't seem necessary.
   thoughts?

[mrkamel]

Two questions:

1. the logic to validate the query_options is duplicated, do you want to extract it to a dedicated class or method?

sure, extract it. Add a SearchCop::Helpers class/module with a .sanitize_default_operator!(default_operator) class method or similar. I'm open for suggestions.

2. On the hash_parser, I'm just supporting the default_operator in simple cases like this one:
   Product.search(title: "blabla", description: "some desc", default_operator: :or)
   If you have a complex query when you specify conditions, the default operator doesn't seem necessary.

You just need to adapt HashParser#parse and change the inject call

thoughts?

[danieldias51]

hey @mrkamel, can you take a look on the CI? The tests are not running

ERROR 2002 (HY000): Can't connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock' (2)
The command "mysql -e 'create database search_cop;'" failed and exited with 1 during .

[mrkamel]

pipeline is fixed in master

[danieldias51]

Already updated the branch and the tests are passing 🚀
What do you think?

[mrkamel]

Awesome, great job! I'm fine with it. I'll do some final manual tests and then merge it. Feel free to add something to the readme.

[danieldias51]

Ok, I will add some examples to the readme