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 filters #164

Closed
wants to merge 1 commit into from
Closed

Add filters #164

wants to merge 1 commit into from

Conversation

comtihon
Copy link

@comtihon comtihon commented Sep 7, 2018

Add filters support for connections.

    class Query(graphene.ObjectType):
        pets = FilterableConnectionField(PetConnection)

FilterableConnectionField will create InputFilter object with filters for every Connection's Node's property. The date type will be the same as in Node object. Currenly enums and comlex objects (not scalars) are not supported.

I've added this filter operations: eq, ne, like, lt, gt based on sqlalchemy.sql.operators.

Query:

query {
        pets(filter: {name: {eq: "Lassie"}}) {
            edges {
                node {
                    name
                }
            }
        }
    }

Multiple filters will always work like AND.
This wil generate select * from pets where name == "Lassie"

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.5%) to 90.376% when pulling 487697f on comtihon:master into 33d5b74 on graphql-python:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.5%) to 90.376% when pulling 487697f on comtihon:master into 33d5b74 on graphql-python:master.

@comtihon comtihon mentioned this pull request Sep 8, 2018
@ahokinson
Copy link

ahokinson commented Nov 1, 2018

This should really be merged. @syrusakbary, is there a reason why it hasn't been yet?

Edit: I can actually see a couple of problems with this commit. I will attempt to provide some suggestions with the new Github features.

@comtihon
Copy link
Author

comtihon commented Dec 9, 2018

@ahokinson @syrusakbary
Hi,
It there any problem with this review (besides coverage report)?
Thanks.

@ahokinson
Copy link

I think my criticisms are stylistic and personal preference. There's some typos and I personally wouldn't leave TODO comments in a contribution.

I also think that some of your function names as well the use of @staticmethod are confusing.

However, I don't dispute that this works. I borrowed and adapted it for my own needs and it helped me to understand how arguments are constructed.

Honestly, I'm really not sure what the stance graphql-python has on contributions. If you look across their projects, there are so many ignored questions and contributions in addition to tutorials and examples that don't even work correctly. Either way, I support adding filtering to this similar to how it is done in the graphene-django implementation.

Here is the relevant part of my implementation for comparison:

from graphene_sqlalchemy import SQLAlchemyConnectionField
from sqlalchemy import inspect
from sqlalchemy.orm.attributes import InstrumentedAttribute

from core.aggregations import create_group_by_argument, create_order_by_argument
from core.exceptions import ArgumentCreationException, RequiredFilterMissingException, RequiredFilterTypeException
from core.filters import create_filter_argument, filter_query
from utilities.types.string import camel_to_snake


class ConnectionField(SQLAlchemyConnectionField):
    def __init__(self, type_, *args, **kwargs):
        try:
            model = type_.Edge.node._type._meta.model
            kwargs.setdefault("filter", create_filter_argument(model))
        except Exception:
            raise ArgumentCreationException(type_.__name__)

        self.required = kwargs.pop("required") if "required" in kwargs else None
        for required in self.required:
            if not issubclass(type(required), InstrumentedAttribute):
                raise RequiredFilterTypeException(type(required))

        super(ConnectionField, self).__init__(type_, *args, **kwargs)

    @classmethod
    def get_query(cls, model, info, filter=None, group_by=None, order_by=None, **kwargs):
        query = super(ConnectionField, cls).get_query(model, info, **kwargs)
        columns = inspect(model).columns.values()

        if filter:
            for k, v in filter.items():
                query = filter_query(query, model, k, v)

        return query

    @classmethod
    def resolve_connection(cls, connection_type, model, info, args, resolved):
        filters = args.get("filter", {})
        required_filters = [rf.key for rf in getattr(info.schema._query, camel_to_snake(info.field_name)).required]

        missing_filters = set(required_filters) - set(filters.keys())
        if missing_filters:
            raise RequiredFilterMissingException(missing_filters)

        return super(ConnectionField, cls).resolve_connection(
            connection_type, model, info, args, resolved)
from collections import OrderedDict

from graphene import Argument, Field, InputObjectType
from graphene_sqlalchemy.converter import convert_sqlalchemy_type
from sqlalchemy import inspect

argument_cache = {}
field_cache = {}


class FilterArgument:
    pass


class FilterField:
    pass


def filter_query(query, model, field, value):
    [(operator, value)] = value.items()
    if operator is "equal":
        query = query.filter(getattr(model, field) == value)
    elif operator is "notEqual":
        query = query.filter(getattr(model, field) != value)
    elif operator is "lessThan":
        query = query.filter(getattr(model, field) < value)
    elif operator is "greaterThan":
        query = query.filter(getattr(model, field) > value)
    elif operator is "like":
        query = query.filter(getattr(model, field).like(value))
    return query


def create_filter_argument(cls):
    name = "{}Filter".format(cls.__name__)
    if name in argument_cache:
        return Argument(argument_cache[name])

    fields = OrderedDict((column.name, field)
                         for column, field in [(column, create_filter_field(column))
                                               for column in inspect(cls).columns.values()] if field)

    argument_class = type(name, (FilterArgument, InputObjectType), {})
    argument_class._meta.fields.update(fields)

    argument_cache[name] = argument_class

    return Argument(argument_class)


def create_filter_field(column):
    graphene_type = convert_sqlalchemy_type(column.type, column)
    if graphene_type.__class__ == Field:
        return None

    name = "{}Filter".format(str(graphene_type.__class__))
    if name in field_cache:
        return Field(field_cache[name])

    fields = OrderedDict((key, Field(graphene_type.__class__))
                         for key in ["equal", "notEqual", "lessThan", "greaterThan", "like"])

    field_class = type(name, (FilterField, InputObjectType), {})
    field_class._meta.fields.update(fields)

    field_cache[name] = field_class

    return Field(field_class)

@cw1427
Copy link

cw1427 commented Feb 18, 2019

Hi, is there any progress about this filterableConnectionFiled feature in graphene-sqlalchemy. It looks is helpful.

@comtihon
Copy link
Author

comtihon commented Feb 19, 2019

Hi, sorry, but I don't have time to prettify it right now.

if operator == 'eq':
query = query.filter(getattr(model, field) == value)
elif operator == 'ne':
query = query.filter(getattr(model, field) == value)
Copy link

Choose a reason for hiding this comment

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

should == actually be !=?

Copy link
Author

Choose a reason for hiding this comment

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

yes, nice catch :)

@@ -484,3 +484,36 @@ def makeNodes(nodeList):
node["node"]["name"] for node in expectedNoSort[key]["edges"]
)


def test_filter(session):
Copy link

Choose a reason for hiding this comment

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

I suspect this won't get accepted until all of the possible ways to filter are tested.

Copy link
Contributor

@somada141 somada141 left a comment

Choose a reason for hiding this comment

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

I believe the PR could benefit from docstrings and comments.

elif operator == 'lt':
query = query.filter(getattr(model, field) < value)
elif operator == 'gt':
query = query.filter(getattr(model, field) > value)
Copy link
Contributor

Choose a reason for hiding this comment

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

what about the le and ge operators?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I didn't have time for it.

@leosussan
Copy link

+1 on this or a similar implementation

@leosussan
Copy link

I think my criticisms are stylistic and personal preference. There's some typos and I personally wouldn't leave TODO comments in a contribution.

I also think that some of your function names as well the use of @staticmethod are confusing.

However, I don't dispute that this works. I borrowed and adapted it for my own needs and it helped me to understand how arguments are constructed.

Honestly, I'm really not sure what the stance graphql-python has on contributions. If you look across their projects, there are so many ignored questions and contributions in addition to tutorials and examples that don't even work correctly. Either way, I support adding filtering to this similar to how it is done in the graphene-django implementation.

Here is the relevant part of my implementation for comparison:

from graphene_sqlalchemy import SQLAlchemyConnectionField
from sqlalchemy import inspect
from sqlalchemy.orm.attributes import InstrumentedAttribute

from core.aggregations import create_group_by_argument, create_order_by_argument
from core.exceptions import ArgumentCreationException, RequiredFilterMissingException, RequiredFilterTypeException
from core.filters import create_filter_argument, filter_query
from utilities.types.string import camel_to_snake


class ConnectionField(SQLAlchemyConnectionField):
    def __init__(self, type_, *args, **kwargs):
        try:
            model = type_.Edge.node._type._meta.model
            kwargs.setdefault("filter", create_filter_argument(model))
        except Exception:
            raise ArgumentCreationException(type_.__name__)

        self.required = kwargs.pop("required") if "required" in kwargs else None
        for required in self.required:
            if not issubclass(type(required), InstrumentedAttribute):
                raise RequiredFilterTypeException(type(required))

        super(ConnectionField, self).__init__(type_, *args, **kwargs)

    @classmethod
    def get_query(cls, model, info, filter=None, group_by=None, order_by=None, **kwargs):
        query = super(ConnectionField, cls).get_query(model, info, **kwargs)
        columns = inspect(model).columns.values()

        if filter:
            for k, v in filter.items():
                query = filter_query(query, model, k, v)

        return query

    @classmethod
    def resolve_connection(cls, connection_type, model, info, args, resolved):
        filters = args.get("filter", {})
        required_filters = [rf.key for rf in getattr(info.schema._query, camel_to_snake(info.field_name)).required]

        missing_filters = set(required_filters) - set(filters.keys())
        if missing_filters:
            raise RequiredFilterMissingException(missing_filters)

        return super(ConnectionField, cls).resolve_connection(
            connection_type, model, info, args, resolved)
from collections import OrderedDict

from graphene import Argument, Field, InputObjectType
from graphene_sqlalchemy.converter import convert_sqlalchemy_type
from sqlalchemy import inspect

argument_cache = {}
field_cache = {}


class FilterArgument:
    pass


class FilterField:
    pass


def filter_query(query, model, field, value):
    [(operator, value)] = value.items()
    if operator is "equal":
        query = query.filter(getattr(model, field) == value)
    elif operator is "notEqual":
        query = query.filter(getattr(model, field) != value)
    elif operator is "lessThan":
        query = query.filter(getattr(model, field) < value)
    elif operator is "greaterThan":
        query = query.filter(getattr(model, field) > value)
    elif operator is "like":
        query = query.filter(getattr(model, field).like(value))
    return query


def create_filter_argument(cls):
    name = "{}Filter".format(cls.__name__)
    if name in argument_cache:
        return Argument(argument_cache[name])

    fields = OrderedDict((column.name, field)
                         for column, field in [(column, create_filter_field(column))
                                               for column in inspect(cls).columns.values()] if field)

    argument_class = type(name, (FilterArgument, InputObjectType), {})
    argument_class._meta.fields.update(fields)

    argument_cache[name] = argument_class

    return Argument(argument_class)


def create_filter_field(column):
    graphene_type = convert_sqlalchemy_type(column.type, column)
    if graphene_type.__class__ == Field:
        return None

    name = "{}Filter".format(str(graphene_type.__class__))
    if name in field_cache:
        return Field(field_cache[name])

    fields = OrderedDict((key, Field(graphene_type.__class__))
                         for key in ["equal", "notEqual", "lessThan", "greaterThan", "like"])

    field_class = type(name, (FilterField, InputObjectType), {})
    field_class._meta.fields.update(fields)

    field_cache[name] = field_class

    return Field(field_class)

In the code, you reference a series of aggregations & custom exceptions - what code are you referencing here?

Thanks!

from core.aggregations import create_group_by_argument, create_order_by_argument from core.exceptions import ArgumentCreationException, RequiredFilterMissingException, RequiredFilterTypeException from core.filters import create_filter_argument, filter_query from utilities.types.string import camel_to_snake

@maquino1985
Copy link

maquino1985 commented May 22, 2019

I think my criticisms are stylistic and personal preference. There's some typos and I personally wouldn't leave TODO comments in a contribution.

I also think that some of your function names as well the use of @staticmethod are confusing.

However, I don't dispute that this works. I borrowed and adapted it for my own needs and it helped me to understand how arguments are constructed.

Honestly, I'm really not sure what the stance graphql-python has on contributions. If you look across their projects, there are so many ignored questions and contributions in addition to tutorials and examples that don't even work correctly. Either way, I support adding filtering to this similar to how it is done in the graphene-django implementation.

Here is the relevant part of my implementation for comparison:

from graphene_sqlalchemy import SQLAlchemyConnectionField
from sqlalchemy import inspect
from sqlalchemy.orm.attributes import InstrumentedAttribute

from core.aggregations import create_group_by_argument, create_order_by_argument
from core.exceptions import ArgumentCreationException, RequiredFilterMissingException, RequiredFilterTypeException
from core.filters import create_filter_argument, filter_query
from utilities.types.string import camel_to_snake


class ConnectionField(SQLAlchemyConnectionField):
    def __init__(self, type_, *args, **kwargs):
        try:
            model = type_.Edge.node._type._meta.model
            kwargs.setdefault("filter", create_filter_argument(model))
        except Exception:
            raise ArgumentCreationException(type_.__name__)

        self.required = kwargs.pop("required") if "required" in kwargs else None
        for required in self.required:
            if not issubclass(type(required), InstrumentedAttribute):
                raise RequiredFilterTypeException(type(required))

        super(ConnectionField, self).__init__(type_, *args, **kwargs)

    @classmethod
    def get_query(cls, model, info, filter=None, group_by=None, order_by=None, **kwargs):
        query = super(ConnectionField, cls).get_query(model, info, **kwargs)
        columns = inspect(model).columns.values()

        if filter:
            for k, v in filter.items():
                query = filter_query(query, model, k, v)

        return query

    @classmethod
    def resolve_connection(cls, connection_type, model, info, args, resolved):
        filters = args.get("filter", {})
        required_filters = [rf.key for rf in getattr(info.schema._query, camel_to_snake(info.field_name)).required]

        missing_filters = set(required_filters) - set(filters.keys())
        if missing_filters:
            raise RequiredFilterMissingException(missing_filters)

        return super(ConnectionField, cls).resolve_connection(
            connection_type, model, info, args, resolved)
from collections import OrderedDict

from graphene import Argument, Field, InputObjectType
from graphene_sqlalchemy.converter import convert_sqlalchemy_type
from sqlalchemy import inspect

argument_cache = {}
field_cache = {}


class FilterArgument:
    pass


class FilterField:
    pass


def filter_query(query, model, field, value):
    [(operator, value)] = value.items()
    if operator is "equal":
        query = query.filter(getattr(model, field) == value)
    elif operator is "notEqual":
        query = query.filter(getattr(model, field) != value)
    elif operator is "lessThan":
        query = query.filter(getattr(model, field) < value)
    elif operator is "greaterThan":
        query = query.filter(getattr(model, field) > value)
    elif operator is "like":
        query = query.filter(getattr(model, field).like(value))
    return query


def create_filter_argument(cls):
    name = "{}Filter".format(cls.__name__)
    if name in argument_cache:
        return Argument(argument_cache[name])

    fields = OrderedDict((column.name, field)
                         for column, field in [(column, create_filter_field(column))
                                               for column in inspect(cls).columns.values()] if field)

    argument_class = type(name, (FilterArgument, InputObjectType), {})
    argument_class._meta.fields.update(fields)

    argument_cache[name] = argument_class

    return Argument(argument_class)


def create_filter_field(column):
    graphene_type = convert_sqlalchemy_type(column.type, column)
    if graphene_type.__class__ == Field:
        return None

    name = "{}Filter".format(str(graphene_type.__class__))
    if name in field_cache:
        return Field(field_cache[name])

    fields = OrderedDict((key, Field(graphene_type.__class__))
                         for key in ["equal", "notEqual", "lessThan", "greaterThan", "like"])

    field_class = type(name, (FilterField, InputObjectType), {})
    field_class._meta.fields.update(fields)

    field_cache[name] = field_class

    return Field(field_class)

Hey there is a lot to like here but there's also a lot of problems that should be glaringly obvious.

You have obviously never tried your implementation without required fields or you would have run into the first glaring bug in your initializer where you would run into an exception for attempting to iterate over None:

self.required = kwargs.pop("required") if "required" in kwargs else None
        for required in self.required:
                ...

Later on you also use the required fields in a list comp that blows up if it's None

Also I'm not sure why but you can't just pass this custom ConnectionField a regular SQLAlchemyObjectType that implements Node like you can with the standard SQLAlchemyConnectionField. Seems bizarre but probably stuff in your field generators does something after the normal constructor creates Edges on the object. (Honestly Graphene in general is one of the worst documented APIs I have ever used and I'm only about a week into it so I have no idea how anything really works yet)

And lastly SQLAlchemy adds anonymous BindParameters to models as columns in more complex ORM use cases (or something, see https://docs.sqlalchemy.org/en/13/core/sqlelement.html) which then blows up code as it generates Fields for every column when you introspect the Model class. In these cases, there will be a filter named "'%(4539858784 anon)s" and the code will throw an Exception that something like "Field names must match (the regexp below) but the field name is "'%(4539858784 anon)s". So I had to go and change your create_filter_argument method like so:

def create_filter_argument(cls):
    name = "{}Filter".format(cls.__name__)
    if name in argument_cache:
        return Argument(argument_cache[name])
    import re

    NAME_PATTERN = r"^[_a-zA-Z][_a-zA-Z0-9]*$"
    COMPILED_NAME_PATTERN = re.compile(NAME_PATTERN)
    fields = OrderedDict((column.name, field)
                         for column, field in [(column, create_filter_field(column))
                                               for column in inspect(cls).columns.values()] if field and COMPILED_NAME_PATTERN.match(column.name))
    log.info(f'here: {fields}')
    argument_class: InputObjectType = type(name, (FilterArgument, InputObjectType), {})
    argument_class._meta.fields.update(fields)

    argument_cache[name] = argument_class

    return Argument(argument_class)

@palisadoes
Copy link
Collaborator

My organization has been using graphene-sqlalchemy-filter for filtering, however the repo has become inactive. Would anyone on this thread be interested in integrating the graphene-sqlalchemy-filter features into graphene-sqlalchemy as a sponsored activity?

@sabard
Copy link
Collaborator

sabard commented Mar 30, 2022

@palisadoes We had the same issue at my former organization and I've always wanted this functionality included in graphene-sqlalchemy. I'd be willing to take this on given that this thread has become stale, but would defer to any main contributors here first if they'd like or have a solution already underway.

@palisadoes
Copy link
Collaborator

palisadoes commented Mar 30, 2022

@palisadoes We had the same issue at my former organization and I've always wanted this functionality included in graphene-sqlalchemy. I'd be willing to take this on given that this thread has become stale, but would defer to any main contributors here first if they'd like or have a solution already underway.

Thanks I created a tracking issue that we can use. I'm on the Graphene slack channel (Peter Harrison) if you need a direct contact.

@Cito would be a good point of contact to get things started. He commented on this here.

@Cito
Copy link
Member

Cito commented Mar 30, 2022

@sabard @palisados If you could push this forward that would be highly appretiated. This will probably also involve pushing Graphene itself forward. You can try to contact @syrusakbary who is the original author of Graphene and Graphene-SQLAlchemy if you have any questions or want to become maintainer.

@palisadoes
Copy link
Collaborator

@Cito I've reached out @syrusakbary without response. I took the liberty of adding both of you to our new #graphene-sqlalchemy-filter slack channel. We are requesting maintainer access or guidance on how to get it.

@erikwrede
Copy link
Member

I am closing all old issues&PRs related to this. The discussion will continue in #347 (WIP). A proposal for the implementation of filters is currently being worked on and will be posted there once it is ready.

@erikwrede erikwrede closed this May 13, 2022
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.