Skip to content
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

Add the ability to filter queries before construction of the CTE #66

Merged
merged 5 commits into from
Apr 25, 2024

Conversation

rhomboss
Copy link
Contributor

  • Update query.py
    -Add pre_filter and pre_exclude methods to TreeQuerySet.
    -Make query methods call _setup_query to deal with sibling order and pre_filter persistence issues.

  • Update compiler.py
    -Replace the get_sibling_order_params with get_rank_table_params to support early tree filtering.
    -Change sibling_order and pre_filter from class variables to variables that have to be initiated by _setup_query so that they don't persist between user queries.
    -Handle pre_filter params by passing them to the django backend.

  • Update test_queries.py
    -Added tests for the pre_filter and pre_exclude methods.

* Update query.py

Add `pre_filter` and `pre_exclude` methods to TreeQuerySet.

Make query methods call `_setup_query` to deal with sibling order and pre_filter persistence issues.


* Update compiler.py

Replace the `get_sibling_order_params` with `get_rank_table_params` to support early tree filtering.

Change `sibling_order` and `pre_filter` from class variables to variables that have to be initiated by `_setup_query` so that they don't persist between user queries.

Handle pre_filter params by passing them to the django backend.


* Update test_queries.py

Added tests for the `pre_filter` and `pre_exclude` methods.
@rhomboss
Copy link
Contributor Author

This addresses issue #50 and also addresses a possible bug where sibling ordering was persisting between queries even when no explicit ordering was given because it was being stored as a class variable.

It should be noted that filtering prior to the construction of the CTE means that filtered status is inherited i.e. if a parent is excluded so too will all of its descendants. This contrasts with a normal filter which when applied after the CTE is built will retain the descendants of an excluded parent if those descendants meet the filtering criteria.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I like the functionality and it makes a lot of sense to me.

I wonder if something like tree_filter wouldn't be a better name for this? Also, I'd prefer using Q objects here.

.tree_filter(~Q(name="2-2")) would replace .pre_exclude(name="2-2"), and .tree_filter(Q(name__in=[...])) would replace .pre_filter(name__in=[...]). Or do you see a reason why that would be worse? I think using Q objects directly would simplify the filtering code by a relevant amount.

@rhomboss
Copy link
Contributor Author

rhomboss commented Apr 15, 2024

If you think tree_filter is a better name I can change it. When I was writing this over the weekend I really only thought of the filter as something that applied to the tree earlier than the normal filter.

I had completely forgotten about Q objects. Using them exclusively won't really simplify the code any since all that really happens is the arguments of pre_filter gets passed directly into django's normal queryset .filter. That being said I did forget to add support for Q objects which is a pretty easy fix. I just have to make sure that pre_filter passes both *args and **kwargs instead of just **kwargs.

Change `pre_filter` to `tree_filter` and add support for `Q` objects
@rhomboss
Copy link
Contributor Author

I had an idea for how to simplify this feature by making the Django ORM generate the entire __rank_table by itself. I'm still investigating but it looks promising and should simplify any future API extensions that involve the __rank_table so don't commit any of my changes just yet!

* Update compiler.py & query.py

Generate the rank_table entirely with django's backend by providing a queryset instance variable that can be modified by the API
@rhomboss
Copy link
Contributor Author

I've simplified the code by adding the instance variable called rank_table_query. This is a default Django queryset that can be used to generate the CTE's __rank_table sql. Now, the tree_filter and tree_exclude methods simply apply Django's normal queryset methods to the rank_table_query. And, in the future if there is need to allow users to further modify the __rank_table beyond basic filtering you only have to expose the appropriate Django queryset function with the API.

@rhomboss rhomboss requested a review from matthiask April 22, 2024 17:07
Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reminder! I'm not working this week and have been a bit stressed out last week, so I have forgotten about this.

Using the query is an excellent idea!

tests/testapp/test_queries.py Outdated Show resolved Hide resolved
tree_queries/compiler.py Show resolved Hide resolved
Add django version checking to back compat code
Remove Python 2 support and use SimpleNamespace
@matthiask
Copy link
Member

Thank you!

@matthiask matthiask merged commit 4790faf into feincms:main Apr 25, 2024
15 checks passed
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