Skip to content

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

Merged
merged 4 commits into from
May 18, 2025

Conversation

mattpolzin
Copy link
Member

@mattpolzin mattpolzin commented May 14, 2025

The project I am working on allows filtering of related resource fields (e.g. user.name meaning the name field of the user 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 to String.to_atom/1 became a call to String.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 becomes check_filter_allowed!/3 and check_include_validity!/3 becomes check_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.

@mattpolzin mattpolzin requested a review from a team as a code owner May 14, 2025 22:48
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).
@mattpolzin mattpolzin force-pushed the nested-include-atoms branch from 775b7ea to 9c5aa26 Compare May 14, 2025 22:49
fn
[_first | _rest] = old_val -> deep_merge(old_val, val)
old_val -> Keyword.put([], old_val, val)
end
Copy link
Contributor

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

Copy link
Member Author

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.

@mattpolzin mattpolzin requested a review from snewcomer May 15, 2025 15:49
@mattpolzin mattpolzin force-pushed the nested-include-atoms branch from 29117b8 to d9ee357 Compare May 15, 2025 15:57
Copy link
Contributor

@snewcomer snewcomer left a 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
Copy link
Contributor

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

@snewcomer snewcomer merged commit 63d297c into beam-community:main May 18, 2025
8 checks passed
@mattpolzin mattpolzin deleted the nested-include-atoms branch May 18, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants