Add Filters

[sabard]

Supersedes #356

[codecov-commenter]

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (1436807) 96.46% compared to head (e698d7c) 94.74%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
graphene_sqlalchemy/registry.py	80.88%	13 Missing ⚠️
graphene_sqlalchemy/filters.py	95.25%	11 Missing ⚠️
graphene_sqlalchemy/types.py	89.53%	9 Missing ⚠️
graphene_sqlalchemy/converter.py	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
- Coverage   96.46%   94.74%   -1.72%     
==========================================
  Files           9       10       +1     
  Lines         933     1333     +400     
==========================================
+ Hits          900     1263     +363     
- Misses         33       70      +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sabard]

Should this live on NumberFilter? Technically strings are ordered as well. Maybe we can call this OrderedFilter and numbers, strings, etc. can inherit?

[erikwrede]

Great point, added!

[sabard]

thoughts on using ne_filter to stay consistent with graphene-sqlalchemy-filter and sqlalchemy methods?

[forana]

I'm realizing that graphene-sqlalchemy-filters handled pagination, while this does not - any guidance for a recommended strategy for implementing pagination using these filters?

[polgfred]

I'm realizing that graphene-sqlalchemy-filters handled pagination, while this does not - any guidance for a recommended strategy for implementing pagination using these filters?

Why would pagination not work if we're just attaching clauses to the SQLAlchemy query?

[forana]

I'm realizing that graphene-sqlalchemy-filters handled pagination, while this does not - any guidance for a recommended strategy for implementing pagination using these filters?

Why would pagination not work if we're just attaching clauses to the SQLAlchemy query?

Not so much "this breaks pagination" as "this doesn't provide it, and I've realized I'll need to handle it" - I had been (incorrectly) thinking of this PR as a complete replacement for graphene-sqlalchemy-filters (and to be clear, I don't think it needs to be - just looking for recommendations/pointers before I roll my own pagination).

[PaulSchweizer]

Pagination via offset and limit has to be implemented manually at this point, it could look something like this (going off of my memory):

class PaginationConnectionField(graphene_sqlalchemy.SQLAlchemyConnectionField):
    def __init__(self,  *args, **kwargs):
        kwargs["page"] = graphene.Int()
        kwargs["itemsPerPage"] = graphene.Int()
        super().__init__(*args, **kwargs)

    @classmethod
    def get_query(cls, *args, **kwargs):
        query = SQLAlchemyConnectionField.get_query(*args, **kwargs)

        if (
            kwargs.get("page", None) is not None
            and kwargs.get("itemsPerPage", None) is not None
        ):
            query = query.offset(kwargs["page"] * kwargs["itemsPerPage"])
            query = query.limit(kwargs["itemsPerPage"])

        return query

We already briefly discussed implementing this (or a version of this) in the library, but there is no concrete plan yet.

[PaulSchweizer]

We looked into the filters a bit more in depth now and came across this issue/question regarding filtering across relationships:

The new implementation uses INNERJOINs for the filters, graphene_sqlalchemy_filters on the other hand uses EXISTS statements to filter across relationships.

The graphql syntax of the two filter systems is basically the same.

This lead to some initial confusion on our end since we were getting different results when translating our existing queries one to one into the new filter syntax.

I prepared an example to showcase the "issue", unfortunately I had no time yet to put it up somewhere but I can showcase it in a call.

In the example we have Teams, which have a number of Tasks wich in turn have a number of Todos. Users are then assigned to the Todos (Teams -> Tasks -> Todos -> User).

This is the filter, translated 1:1 from the graphene_sqlalchemy_filters syntax:

query {
  teams(
    filter: {
      tasks: {
        contains: {
          and: [
            { name: { eq: "Accounting" } }
            { todos: { contains: { assignee_id: { eq: 2 } } } }
          ]
        }
      }
    }
  )

I would expect to get:

All teams that have an 'Accounting' task that contains a Todo where a user with id 2 is assigned.

Instead we get:

All teams that EITHER have an 'Accounting' task OR a Task with a Todo where a user with id 2 is assigned.

The "correct" query to get the initially expected result with the "new filters" would look like this:

query {
  teams(
    filter: {
      and: [
        {
          tasks: {
            contains: { name: { eq: "Accounting" } }
          }
        },
         {
          tasks: {
            contains: { todos: { contains: { assignee_id: { eq: 2 } } } }
          }
        }
      ]
    }
  )

BUT this one also does not work because each occurence of "tasks" gets a unique alias, so we again end up with the wrong result. The resulting SQL statement:

SELECT DISTINCT 
teams.id AS teams_id, 
FROM teams 
INNER JOIN tasks AS tasks_1 ON teams.id = tasks_1.shot_id 
INNER JOIN tasks AS tasks_2 ON teams.id = tasks_2.shot_id INNER JOIN todos AS todos_1 ON tasks_2.id = todos_1.task_id 

Should look like this:

SELECT DISTINCT 
teams.id AS teams_id, 
FROM teams 
INNER JOIN tasks AS tasks_1 ON teams.id = tasks_1.shot_id 
INNER JOIN todos AS todos_1 ON tasks_1.id = todos_1.task_id 

The solution here would probably be to use the same alias on each "level" of relationship.

This however highlights the question: Which behavior do we want to support? INNERJOINS or EXISTS? We talked about it in my team and came to the conclusion that the grapqhl filter syntax would rather suggest the EXISTS behavior over the INNERJOIN behaviour. Let me know what you think, probably best to jump on a call and discuss it in person (I would be free this coming Monday, May 15th)

[palisadoes]

@sabard @erikwrede When do you think you'll be able to merge the filters into the main branch? Is it just the Enum work that is outstanding? I really need to migrate off the old graphene-sqlalchemy-filter.

[erikwrede]

@palisadoes just fixed the enum filter case. AFAIK that covers our base scope, so what's missing now is a refactor and cleanup of the code, as it still contains many special cases from previous prototypes that can be generalized to improve code quality and readability.

[sabard]

thanks for figuring out the issue with enums @erikwrede!

@palisadoes i've been pretty swamped this month unfortunately. hope to have more time to work on this in august

[palisadoes]

@erikwrede Sorry about missing the meeting. Are there any more outstanding PRs for the filtering?