-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Meilisearch::FilterBuilder class to convert Ruby hashes to filter strings #617
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm generally in favor of this addition. The interface is a bit too much for my liking, but I suppose users don't have to use every single feature here. I think with this merged we could have an ActiveRecord-like interface on filter.
Suggestion: Allow using ranges as values (year: 2014..2024
becomes year 2014 TO 2024
). Ruby devs are probably familiar with passing ranges in this context from ActiveRecord.
From the level of code in this PR, I assumed you were an experienced developer, so a lot of my comments don't go into a lot of depth. If some of my feedback is confusing please ask for clarification.
I had a lot of feedback, and I probably will have more. Don't take this as an offense to your code or your code style, it's normal for there to be a lot of back-and-forth for a change of this size.
Thank you for contributing to Meilisearch!
def build_not_expression(hash, logical_op) = "NOT (#{build(hash[logical_op] || hash[logical_op.to_s])})" | ||
|
||
def get_conditions_array(hash, logical_op) = Array(hash[logical_op] || hash[logical_op.to_s]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see in the code, there cannot be a case when logical_op
is not the same type as the key in the hash, since it comes from this expression:
logical_op = hash.keys.find { |k| LOGICAL_OPERATORS.include?(k.to_sym) }
It will always be the correct type, and there is no need to convert it to a String.
Let me know if I misunderstood something.
def logical_operator?(hash) = hash.keys.any? { |k| LOGICAL_OPERATORS.include?(k.to_sym) } | ||
|
||
def build_logical_expression(hash) | ||
logical_op = hash.keys.find { |k| LOGICAL_OPERATORS.include?(k.to_sym) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but you could find
and any
are not very different. Instead of having logical_operator?
you could simply return if logical_op.nil?
in build_logical_expression
.
process_attribute_conditions(attribute, conditions) | ||
elsif conditions.nil? | ||
"#{attribute} IS NULL" | ||
elsif conditions.is_a?(Array) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a nitpick:
I would not consider this behavior to be consistent, since get_conditions_array
works with array-like structs due to the call to Array()
.
It would not make sense to call Array()
here but it might make sense to check if conditions responds to to_ary
.
elsif hash.size == 1 | ||
build_single_attribute_expression(hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elsif hash.size == 1 | |
build_single_attribute_expression(hash) |
I don't think this actually needs to exist. build_single_attribute_expression
can probably be inlined inside of build_multi_atttribute_expression
.
end | ||
end | ||
|
||
def format_string_value(string) = needs_quoting?(string) ? "'#{string.gsub("'", "\\'")}'" : string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def format_string_value(string) = needs_quoting?(string) ? "'#{string.gsub("'", "\\'")}'" : string | |
def format_string_value(string) = needs_quoting?(string) ? "'#{string.gsub("'", "\\\\'")}'" : string |
This does not have the output that you would expect. The replacement string is correctly \'
, but that is a special sequence in gsub, which substitutes in the value of the $'
global variable, which is a regexp global variable that is string after the last match.
Try this in IRB:
irb(main):001> s = "puts 'hello world'"
=> "puts 'hello world'"
irb(main):002> s.gsub("'", "\\'")
=> "puts hello world'hello world"
irb(main):003> s.gsub("'", "\\\\'")
=> "puts \\'hello world\\'"
Documentation:
Thanks for your feedback @ellnix, I have a lot to do 🥲, but I'm happy with that. I'm not a very experienced Ruby developer, but I'm happy to learn. Will look into the comments and address them/ask for clarification shortly. |
Pull Request
Related issue
Fixes #616
What does this PR do?
Meilisearch::FilterBuilder
, which is takes Ruby hashes and converts them into Meilisearch filter strings.Examples
Example 1
Ruby object:
Meilisearch filter expressions:
Example 2
Ruby object:
Meilisearch filter expressions:
Example 3
Ruby object:
Meilisearch filter expressions:
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!