Draft: Add filter tests for discussion

[sabard]

The purpose of this Draft PR is to facilitate discussion around implementing filters and to provide a concrete set of test cases that filtering must support in order to be incorporated into graphene-sqlalchemy.

So far, I have added test cases for the following:

• simple filter on field
• custom filter
• relationship filters (requires improvement)
• logic operators

I still need to implement test cases for the following:

• hybrid property

Please feel free to discuss general topics related to implementing filters here as well as specific improvements needed to testing and filter syntax.

[sabard]

@erikwrede not sure if I'm understanding your point about MultipleOfType correctly. Currently, the syntax would support this kind of query:

query {
  x (filters: {
    y: { name: {in: ["A", "B"]} }
  }) {
    ...
  }
}

SQLAlchemy: sa.query(x).join(x.y).filter(y.name.in_("A", "B"))

Is this kind of query you mentioned in slack? Where does MultipleOfType come in to support this?

query {
  x (filters: {
    and: [
      y: { name: {in: ["A", "B"]} },
      z: { id: 123 },
    ]
  }) {
    ...
  }
}

SQLAlchemy: sa.query(x).join(x.y).filter(y.name.in_("A", "B")).join(x.z).filter(z.id=123)

Let me know if I have anything wrong here.

[erikwrede]

Hey, sorry for not following up with an example. Think of my comment like a friendsWith filter on User that has a list of friends. Currently, you could only filter for "A is friends with one of B or C." What's missing is "A is friends with ALL of B and C" or "A's friends are exactly B and C"
Let me know if that clears things up.

EDIT:
Following up with an SQL example. Basically, I'm talking about the SQL-Equivalent to Relational Division in Relational Algebra. This example explains it better than I could: https://www.inf.usi.ch/faculty/soule/teaching/2016-fall/db/division.pdf

Example: Student is enrolled exactly in Courses SQL and Graphene

sa.query(Student).join(student_course).filter(student_course.course.name.in_(["SQL","Graphene"])).group_by(Student.id).having(sa.func.count(student_course.course.name.distinct()) == len(["SQL",Graphene]))

Filtering it this way is certainly more complex than regular relationship filters like the ones you mentioned above. However, we included these kinds of filters in the document (Section 1:n relationships, RelationshipFilter).
I think this is a great point to discuss with @PaulSchweizer and @palisadoes. If we decide that this is too complex for our initial filtering release, we should definitely design the filters so we can add this feature later.

Here is where the intermediate RelationshipFilter comes into action. We could use this in every non- x:1 relationship to aggregate filters over that particular list of instances of the same type. For the above example, we would add a CourseRelationshipFilter. If we decide to go with the simple approach, the CourseEnrolmentRelationshipFilter would only support a contains filter, supporting a simple CourseEnrolmentFilter. If we add the relational division, the contains filter could be supplemented by a containsAllOf or a containsExactly filter.

This is why it's absolutely necessary to have these intermediate relationship filters. This would be the only way to proxy additional logic on relationships or add new functionality while preserving backward compatibility.

I've typed this down in an example below:

type StudentFilter
name: StringFilter
courseEnrolments: CourseEnrolmentRelationshipFilter
  
type CourseEnrolmentRelationshipFilter
contains: CourseEnrolmentFilter
containsAllOf: [CourseEnrolmentFilter]
containsExactly: [CourseEnrolmentFilter] 

query {
  students(filter: {courseEnrolments: 
                     {contains: {course: 
                         {name:{in: ["SQL", "Graphene"]}}
                     }}
               }) {
            name
  }
}

The above example would semantically mean "Student who is enrolled either in the SQL or the Graphene Course."

The following example would be equal to my example from above (Student is enrolled exactly in Courses SQL and Graphene):

query {
  students(filter: {courseEnrolments: 
                     {containsExactly: 
                         [{course: 
                            {name:{eq: "SQL"}}
                            }, {course: 
                            {name:{eq: "Graphene"}}
                            }]
                       }
               }) {
            name
  }
}

[sabard]

Thanks for the detailed examples here. This makes a lot more sense to me and I'll update the tests to reflect the desired behavior of containsAllOf and containsExactly. Happy to aim for this support in the first release. Are there any other default RelationshipFilter behaviors we want to support? Maybe a better question is how can we make it easy for users to augment custom filters on top of this in the case they'd like to implement something like containsOneOf or containsNoneOf?

[palisadoes]

Sqlalchemy has .startswith and .endswith that we should consider as filtering options. I discovered an issue with VARBINARY filtering with .startswith and .endswith that should have been fixed. You may want to take a look just in case as it may have a material impact: sqlalchemy/sqlalchemy#8253 (comment)

…

[sabard]

Just added a couple subtests for a RelationshipFilter for 1:n and n:m relationshpis with contains, containsAllOf, and containsExactly. Let me know what you think. One question: Is it correct to provide roughly the same filter type for 1:n and n:m relationships? I assume something in the implementation will need to detect the exact relationship type and make sure the correct SQLAlchemy calls are made.

As for startsWith and endsWith, maybe we add examples of doing this as a RelationshipFilter extension for now? Would it be correct to assume that anything from this RelationshipProperty list could potentially be fair game for providing as a default option on RelationshipFilter?

I'm going to go ahead and start implementing basic filtering on top of this PR rebased off #355, but will keep updating this as needed.

[sabard]

Closing in favor of #357