-
Notifications
You must be signed in to change notification settings - Fork 76
feat: support nested filtering in the query parser #366
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
Conversation
Also renames a few private functions to clarify the difference between "validity" of a filter or include (represents a real option as determined by a view) vs. "allowed" (is specified in the allow-list passed to the QueryParser plug).
775b7ea
to
9c5aa26
Compare
fn | ||
[_first | _rest] = old_val -> deep_merge(old_val, val) | ||
old_val -> Keyword.put([], old_val, val) | ||
end |
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 wonder if we need to do some defensive programming here.
fn
old_val when is_list(old_val) and is_tuple(hd(old_val)) ->
if is_list(val), do: deep_merge(old_val, val), else: val
old_val ->
if is_atom(old_val) and is_list(val), do: Keyword.put([], old_val, val), else: val
end
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 think I've addressed concerns over robustness with my latest commit. Please take a look and let me know what you think.
I've protected the first case by ensuring we have more keyword list to go before deep merging.
I've simplified the second case to always take the new value on conflict or when the remaining value is not a list.
29117b8
to
d9ee357
Compare
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.
Very nice addition!
filter = parse_filter(config, %{"author.username" => "jason", "author.id" => "123"}).filter | ||
assert filter[:author][:username] == "jason" | ||
assert filter[:author][:id] == "123" | ||
end |
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 thought about whether Config could come with a filter other than []
but it doesn't look like it
The project I am working on allows filtering of related resource fields (e.g.
user.name
meaning thename
field of theuser
relationship). I realized when updating to the latest version of this library that such filters are not supported; it was actually a Credo PR that resulted in a chore update that broke things because a call toString.to_atom/1
became a call toString.to_existing_atom/1
in a place where the string"user.name"
might be the input.This PR fixes that in a backwards compatible way. It supports filtering on related resource properties by creating deeper Keyword Lists similar to those output by the include parser (which already supports nested includes). I am not confident Keyword Lists would be the best way to go for this nested filter structure if we were coming at it from scratch and could choose a Map, but this is backwards compatible and quite functional.
This PR also renames a two private functions to clarify the difference between "validity" of a filter or include (represents a real option as determined by a view) vs. "allowed" (is specified in the allow-list passed to the QueryParser plug). That is,
check_filter_validity!/3
becomescheck_filter_allowed!/3
andcheck_include_validity!/3
becomescheck_include_allowed!/3
.If this feels like a reasonable direction, it isn't lost on me that
IncludeTree
may no longer be the best name for the module holding helpers that are now useful for both includes and filters, but I honestly wasn't sure if it was worth it to rename the module to something more generic for now.