Fixes filtering in nested nodes.

[pchinea]

Fixes error filtering sub-nodes. The relationships were resolved but not filtered as it was notified in issues #21 and #30.

For example:

query{
  allCategories {
    edges {
       node {
           id,
           ingredients(name_Icontains:"beef") {
              edges {
                 node {
                    id,
                    name
                 }
              }
           }
        }
     }
  }
}

[coveralls]

Coverage increased (+0.009%) to 92.45% when pulling b26f914 on gamingexperience:fix/node-filtering into 4246cea on graphql-python:master.

[syrusakbary]

Great PR! Could be possible to have a test too showcasing how it the filtering got fixed so we don't fall in this bug in the future?

Thanks! (and happy holidays!)

[drakon]

Ah, very nice.
On question though: What if the resolver returns a list? Wouldn't this list remain unaffected by the filter?

[coveralls]

Coverage decreased (-3.2%) to 89.233% when pulling dfb55cd on gamingexperience:fix/node-filtering into 4246cea on graphql-python:master.

[coveralls]

Coverage increased (+0.009%) to 92.45% when pulling b5a450c on gamingexperience:fix/node-filtering into 4246cea on graphql-python:master.

[coveralls]

Coverage increased (+0.009%) to 92.45% when pulling 16a0d9c on gamingexperience:fix/node-filtering into 4246cea on graphql-python:master.

[coveralls]

Coverage increased (+0.009%) to 92.45% when pulling 16a0d9c on gamingexperience:fix/node-filtering into 4246cea on graphql-python:master.

[coveralls]

Coverage increased (+0.009%) to 92.45% when pulling 16a0d9c on gamingexperience:fix/node-filtering into 4246cea on graphql-python:master.

[coveralls]

Coverage increased (+0.009%) to 92.45% when pulling 16a0d9c on gamingexperience:fix/node-filtering into 4246cea on graphql-python:master.

[pchinea]

@syrusakbary I added a test for this issue. Please, check it to ensure everything is right.

Greetings.

[pchinea]

@drakon The problem in #80 was in the line: qs = filterset_class(data=filter_kwargs, queryset=qs), if qs is a list instead a queryset it fails. Sincerely I don't have a deep knowledge on this module, but I think in a real application it always will be a queryset so your modification will work too. Anyway I'm not sure about it so I made the simplest modification assuming resolve could return a list.

[drakon]

Yeah, that I got. Thanks for the hint! :)

My point was that in that case the connection_resolver method still is broken (not an issue that is new with your PR). Assuming it returns a list, default_manager never gets considered and therefore I'd argue that a test considering this would fail.

Otherwise your solution is better. Mine originally was more of an easy workaround to make it work in case someone needs this functionality. So I didn't have to fork and fix the issue. Your fix seems like a better solution.

//Update
See here: #84

[pchinea]

@drakon You are right, when I coded the fix I realized it won't be filtered if a list is returned by resolver and I think there are no efficient way to filter the list. But this is my question: Is there a real case in which a list is returned? I think it shouldn't do it, because you need a queryset to be able to apply a filter. Maybe using lists in tests is an error.

Anyway, I would like to know @syrusakbary 's opinion on this matter

[drakon]

Yeah, exactly my thoughts! Maybe it is required by some GraphQL spec. Also interested in @syrusakbary 's thoughts on this. If it's really a non intended use case we could at least issue a warning that the filters get ignored when the resolver returns a list.

[syrusakbary]

In general a DjangoFilterConnectionField would only work when the resolver returns a QuerySet.
The DjangoConnectionField supports a list returned in the resolver because list/Iterables are accepted as return values in normal ConnectionFields, plus the willing to filter manually a queryset and return a list.

However this PR handles this cases perfectly and also with a good test coverage... so everything looks great!

[nickhudkins]

@syrusakbary thanks for getting this merged into master. Is there a planned release that will have this included in it?

[imsickofmaps]

Also curious as to next pypi release... 1.2.1 was released on 2016-12-16.

[syrusakbary]

Hi @nickhudkins @imsickofmaps.
The release is waiting on a new feature of graphene that will let this package to improve significantly the registry logic.
I expect to release graphene 1.2 today, so today or tomorrow will be the release of graphene-django 1.3.

Sorry for the delay!

[pchinea]

@syrusakbary This PR introduces a serious efficiency problem, I'll try to push a fix today. I hope this can be included in the next release.

[syrusakbary]

@pchinea no worries, I will wait few days and revert in case is not solved so we can release a new version.

[antonkhelou]

Just thought I would let you guys know that there's an issue with this fix, if you do something like:

    def resolve_tasks(self, args, context, info):
        tasks = Task.objects.filter(
            project=context.user.project
        )[:OFFSET]

        return tasks

It will cause an error due to the slicing with [:OFFSET]:

"Cannot combine queries once a slice has been taken."

The message says it all: Code does some combination of QuerySets after the resolver has finished it computation. This operation: iterable &= maybe_queryset(default_manager)

I feel like slicing data inside the resolver is a reasonable operation, the code should be able to handle it.

[pchinea]

@antonkhelou If you want to use a custom resolver instead of the automatic resolver you should return a list to avoid further filtering, in this case:

    def resolve_tasks(self, args, context, info):
        tasks = Task.objects.filter(
            project=context.user.project
        )[:OFFSET]

        return list(tasks)

Greetings

[antonkhelou]

@pchinea I see. But now the filtering does not work. Both of the following queries return the same thing:

query{
  tasks {
    edges {
      node {
        id
        name
      }
    }
  }
}

query{
  tasks(name:"asd") {
    edges {
      node {
        id
        name
      }
    }
  }
}

#84

[pchinea]

@antonkhelou Anyway, I think there are no way to fix this, you can't filter a queryset after slicing. This isn't a graphene or django limitation, this is a SQL limitation (you can't use WHERE clause after LIMIT clause). In this case I'm afraid you'll need to find a way to get the final query in the resolver or using Relay to slice with the GraphQL query (with before, after, ....).