DjangoConnectionField slice: use max_limit first, if set

[pcraciunoiu]

I found an interesting performance issue, where if I query without first or last, and there is a setting for RELAY_CONNECTION_MAX_LIMIT, the resolver attempts to pull in the entire queryset.

This is way overkill when there are thousands of items, so it seems wise to use max_limit when available.

I opted for optional kwarg in resolve_connection to maintain backwards compatibility if anyone overwrote this method, but it would be cleaner to require it, since the calling method connection_resolver always has it.

On a query that has multiple nested DjangoConnectionFields, I imagine this would reduce queries and data over the SQL wire by a lot.

[jkimbo]

This makes sense @pcraciunoiu but can you fix up the tests and add a new one to assert this new behaviour?

[pcraciunoiu]

@jkimbo updated tests to show the changes (basically with and without max limit). And also fixed a bug in my approach.

[pcraciunoiu]

@jkimbo any thoughts on this?

Apparently before this fix, DjangoConnectionField and DjangoFilterConnectionField did not respect the max limit, and returned all the items in the queryset. I can write a test for that as well, but just FYI... it is a breaking change for people who were relying on this (IMO incorrect) behavior.

This is a backwards incompatible change. I'd be open to putting it behind a flag/setting, e.g.

GRAPHENE = {
    "RELAY_CONNECTION_MAX_LIMIT": 50,
    "RELAY_CONNECTION_FIELD_ENABLE_LIMIT": True,
}

But IMO the max limit should actually be respected by default, as it's used in the constructor:

class DjangoConnectionField(ConnectionField):
    def __init__(self, *args, **kwargs):
        self.on = kwargs.pop("on", False)
        self.max_limit = kwargs.pop(
            "max_limit", graphene_settings.RELAY_CONNECTION_MAX_LIMIT
        )

I have confirmed in my projects that these fields return e.g. 1,000 items, even if my limit is set to the default of 100. And testing against this branch, the limit is respected.

[jkimbo]

@pcraciunoiu IMO the incorrect behaviour is a bug so I don't consider this a breaking change. If you could write a test to assert this new behaviour that would be good though. That we can know in the future if it breaks again.

[pcraciunoiu]

This test fails without changes to max_limit or ... in resolve_connection:

_____________________________________ test_should_return_max_limit ______________________________________

graphene_settings = <graphene_django.settings.GrapheneSettings object at 0x7fb4b4f54550>

    def test_should_return_max_limit(graphene_settings):
        graphene_settings.RELAY_CONNECTION_MAX_LIMIT = 4
        reporters = [Reporter(**kwargs) for kwargs in REPORTERS]
        Reporter.objects.bulk_create(reporters)
    
        class ReporterType(DjangoObjectType):
            class Meta:
                model = Reporter
                interfaces = (Node,)
    
        class Query(graphene.ObjectType):
            all_reporters = DjangoConnectionField(ReporterType)
    
        schema = graphene.Schema(query=Query)
        query = """
            query AllReporters {
                allReporters {
                    edges {
                        node {
                            id
                        }
                    }
                }
            }
        """
    
        result = schema.execute(query)
        assert not result.errors
>       assert len(result.data["allReporters"]["edges"]) == 4
E       AssertionError: assert 6 == 4
E        +  where 6 = len([{'node': {'id': 'UmVwb3J0ZXJUeXBlOjE='}}, {'node': {'id': 'UmVwb3J0ZXJUeXBlOjI='}}, {'node': {'id': 'UmVwb3J0ZXJUeXBl...': {'id': 'UmVwb3J0ZXJUeXBlOjQ='}}, {'node': {'id': 'UmVwb3J0ZXJUeXBlOjU='}}, {'node': {'id': 'UmVwb3J0ZXJUeXBlOjY='}}])

[pcraciunoiu]

@jkimbo updated with a test, take a peek?

[jkimbo]

Thanks @pcraciunoiu !

[pcraciunoiu]

@jkimbo looks like we do need to do a query count to get next page info. I fixed that + another bug (seems that after wasn't working) and added a test for both in #986

[jkimbo]

Released in v2.11.0