From 6bc0f6d9d6ba9f242491be3945a494ba8a5b2f40 Mon Sep 17 00:00:00 2001 From: Paul Craciunoiu Date: Tue, 9 Jun 2020 12:19:11 -0600 Subject: [PATCH 1/7] Fix hasNextPage - revert to count --- graphene_django/debug/tests/test_query.py | 61 ++++++++--------------- graphene_django/fields.py | 17 ++++--- graphene_django/tests/test_query.py | 40 ++++++++++++++- 3 files changed, 69 insertions(+), 49 deletions(-) diff --git a/graphene_django/debug/tests/test_query.py b/graphene_django/debug/tests/test_query.py index 4c057ed93..d71c3fb4e 100644 --- a/graphene_django/debug/tests/test_query.py +++ b/graphene_django/debug/tests/test_query.py @@ -56,8 +56,8 @@ def resolve_reporter(self, info, **args): assert result.data == expected -@pytest.mark.parametrize("max_limit,does_count", [(None, True), (100, False)]) -def test_should_query_nested_field(graphene_settings, max_limit, does_count): +@pytest.mark.parametrize("max_limit", [None, 100]) +def test_should_query_nested_field(graphene_settings, max_limit): graphene_settings.RELAY_CONNECTION_MAX_LIMIT = max_limit r1 = Reporter(last_name="ABA") @@ -117,18 +117,11 @@ def resolve_reporter(self, info, **args): assert not result.errors query = str(Reporter.objects.order_by("pk")[:1].query) assert result.data["__debug"]["sql"][0]["rawSql"] == query - if does_count: - assert "COUNT" in result.data["__debug"]["sql"][1]["rawSql"] - assert "tests_reporter_pets" in result.data["__debug"]["sql"][2]["rawSql"] - assert "COUNT" in result.data["__debug"]["sql"][3]["rawSql"] - assert "tests_reporter_pets" in result.data["__debug"]["sql"][4]["rawSql"] - assert len(result.data["__debug"]["sql"]) == 5 - else: - assert len(result.data["__debug"]["sql"]) == 3 - for i in range(len(result.data["__debug"]["sql"])): - assert "COUNT" not in result.data["__debug"]["sql"][i]["rawSql"] - assert "tests_reporter_pets" in result.data["__debug"]["sql"][1]["rawSql"] - assert "tests_reporter_pets" in result.data["__debug"]["sql"][2]["rawSql"] + assert "COUNT" in result.data["__debug"]["sql"][1]["rawSql"] + assert "tests_reporter_pets" in result.data["__debug"]["sql"][2]["rawSql"] + assert "COUNT" in result.data["__debug"]["sql"][3]["rawSql"] + assert "tests_reporter_pets" in result.data["__debug"]["sql"][4]["rawSql"] + assert len(result.data["__debug"]["sql"]) == 5 assert result.data["reporter"] == expected["reporter"] @@ -175,8 +168,8 @@ def resolve_all_reporters(self, info, **args): assert result.data == expected -@pytest.mark.parametrize("max_limit,does_count", [(None, True), (100, False)]) -def test_should_query_connection(graphene_settings, max_limit, does_count): +@pytest.mark.parametrize("max_limit", [None, 100]) +def test_should_query_connection(graphene_settings, max_limit): graphene_settings.RELAY_CONNECTION_MAX_LIMIT = max_limit r1 = Reporter(last_name="ABA") @@ -219,20 +212,14 @@ def resolve_all_reporters(self, info, **args): ) assert not result.errors assert result.data["allReporters"] == expected["allReporters"] - if does_count: - assert len(result.data["__debug"]["sql"]) == 2 - assert "COUNT" in result.data["__debug"]["sql"][0]["rawSql"] - query = str(Reporter.objects.all()[:1].query) - assert result.data["__debug"]["sql"][1]["rawSql"] == query - else: - assert len(result.data["__debug"]["sql"]) == 1 - assert "COUNT" not in result.data["__debug"]["sql"][0]["rawSql"] - query = str(Reporter.objects.all()[:1].query) - assert result.data["__debug"]["sql"][0]["rawSql"] == query - - -@pytest.mark.parametrize("max_limit,does_count", [(None, True), (100, False)]) -def test_should_query_connectionfilter(graphene_settings, max_limit, does_count): + assert len(result.data["__debug"]["sql"]) == 2 + assert "COUNT" in result.data["__debug"]["sql"][0]["rawSql"] + query = str(Reporter.objects.all()[:1].query) + assert result.data["__debug"]["sql"][1]["rawSql"] == query + + +@pytest.mark.parametrize("max_limit", [None, 100]) +def test_should_query_connectionfilter(graphene_settings, max_limit): graphene_settings.RELAY_CONNECTION_MAX_LIMIT = max_limit from ...filter import DjangoFilterConnectionField @@ -278,13 +265,7 @@ def resolve_all_reporters(self, info, **args): ) assert not result.errors assert result.data["allReporters"] == expected["allReporters"] - if does_count: - assert len(result.data["__debug"]["sql"]) == 2 - assert "COUNT" in result.data["__debug"]["sql"][0]["rawSql"] - query = str(Reporter.objects.all()[:1].query) - assert result.data["__debug"]["sql"][1]["rawSql"] == query - else: - assert len(result.data["__debug"]["sql"]) == 1 - assert "COUNT" not in result.data["__debug"]["sql"][0]["rawSql"] - query = str(Reporter.objects.all()[:1].query) - assert result.data["__debug"]["sql"][0]["rawSql"] == query + assert len(result.data["__debug"]["sql"]) == 2 + assert "COUNT" in result.data["__debug"]["sql"][0]["rawSql"] + query = str(Reporter.objects.all()[:1].query) + assert result.data["__debug"]["sql"][1]["rawSql"] == query diff --git a/graphene_django/fields.py b/graphene_django/fields.py index 9b102bdc8..f647dbf1d 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -129,25 +129,26 @@ def resolve_queryset(cls, connection, queryset, info, args): @classmethod def resolve_connection(cls, connection, args, iterable, max_limit=None): iterable = maybe_queryset(iterable) - # When slicing from the end, need to retrieve the iterable length. - if args.get("last"): - max_limit = None + if isinstance(iterable, QuerySet): - _len = max_limit or iterable.count() + list_length = iterable.count() + list_slice_length = max_limit or list_length else: - _len = max_limit or len(iterable) + list_length = len(iterable) + list_slice_length = max_limit or list_length + connection = connection_from_list_slice( iterable, args, slice_start=0, - list_length=_len, - list_slice_length=_len, + list_length=list_length, + list_slice_length=list_slice_length, connection_type=connection, edge_type=connection.Edge, pageinfo_type=PageInfo, ) connection.iterable = iterable - connection.length = _len + connection.length = list_length return connection @classmethod diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index e6ed49e8e..758b36c91 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -1126,6 +1126,44 @@ class Query(graphene.ObjectType): assert len(result.data["allReporters"]["edges"]) == 4 +def test_should_have_next_page(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) + # Need first: 4 here to trigger the `has_next_page` logic in graphql-relay + # See `arrayconnection.py::connection_from_list_slice`: + # has_next_page=isinstance(first, int) and end_offset < upper_bound + query = """ + query AllReporters { + allReporters(first: 4) { + pageInfo { + hasNextPage + } + edges { + node { + id + } + } + } + } + """ + + result = schema.execute(query) + assert not result.errors + assert len(result.data["allReporters"]["edges"]) == 4 + assert result.data["allReporters"]["pageInfo"]["hasNextPage"] + + def test_should_preserve_prefetch_related(django_assert_num_queries): class ReporterType(DjangoObjectType): class Meta: @@ -1172,7 +1210,7 @@ def resolve_films(root, info): } """ schema = graphene.Schema(query=Query) - with django_assert_num_queries(2) as captured: + with django_assert_num_queries(3) as captured: result = schema.execute(query) assert not result.errors From 1cb7fc63bf2d0f20bddf4a5b84a28b99c4055d1b Mon Sep 17 00:00:00 2001 From: Paul Craciunoiu Date: Tue, 9 Jun 2020 12:35:14 -0600 Subject: [PATCH 2/7] Fix pagination using 'after' --- graphene_django/fields.py | 8 +++++++- graphene_django/tests/test_query.py | 23 ++++++++++++++++++++--- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/graphene_django/fields.py b/graphene_django/fields.py index f647dbf1d..f86f99753 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -2,7 +2,10 @@ import six from django.db.models.query import QuerySet -from graphql_relay.connection.arrayconnection import connection_from_list_slice +from graphql_relay.connection.arrayconnection import ( + connection_from_list_slice, + get_offset_with_default, +) from promise import Promise from graphene import NonNull @@ -137,6 +140,9 @@ def resolve_connection(cls, connection, args, iterable, max_limit=None): list_length = len(iterable) list_slice_length = max_limit or list_length + after = get_offset_with_default(args.get("after"), -1) + 1 + list_slice_length += after + connection = connection_from_list_slice( iterable, args, diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index 758b36c91..64d720a16 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -1130,6 +1130,7 @@ def test_should_have_next_page(graphene_settings): graphene_settings.RELAY_CONNECTION_MAX_LIMIT = 4 reporters = [Reporter(**kwargs) for kwargs in REPORTERS] Reporter.objects.bulk_create(reporters) + db_reporters = Reporter.objects.all() class ReporterType(DjangoObjectType): class Meta: @@ -1144,10 +1145,11 @@ class Query(graphene.ObjectType): # See `arrayconnection.py::connection_from_list_slice`: # has_next_page=isinstance(first, int) and end_offset < upper_bound query = """ - query AllReporters { - allReporters(first: 4) { + query AllReporters($first: Int, $after: String) { + allReporters(first: $first, after: $after) { pageInfo { hasNextPage + endCursor } edges { node { @@ -1158,11 +1160,26 @@ class Query(graphene.ObjectType): } """ - result = schema.execute(query) + result = schema.execute(query, variable_values=dict(first=4)) assert not result.errors assert len(result.data["allReporters"]["edges"]) == 4 assert result.data["allReporters"]["pageInfo"]["hasNextPage"] + last_result = result.data["allReporters"]["pageInfo"]["endCursor"] + result2 = schema.execute(query, variable_values=dict(first=4, after=last_result)) + assert not result2.errors + assert len(result2.data["allReporters"]["edges"]) == 2 + assert not result2.data["allReporters"]["pageInfo"]["hasNextPage"] + gql_reporters = result.data["allReporters"]["edges"] + result2.data["allReporters"]["edges"] + + assert { + to_global_id("ReporterType", reporter.id) + for reporter in db_reporters + } == { + gql_reporter["node"]["id"] + for gql_reporter in gql_reporters + } + def test_should_preserve_prefetch_related(django_assert_num_queries): class ReporterType(DjangoObjectType): From 4df05e5c389ace9cf8f14c883b6711950bb0165a Mon Sep 17 00:00:00 2001 From: Paul Craciunoiu Date: Tue, 9 Jun 2020 12:38:21 -0600 Subject: [PATCH 3/7] Lint --- graphene_django/tests/test_query.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index 64d720a16..3440586bf 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -1170,14 +1170,12 @@ class Query(graphene.ObjectType): assert not result2.errors assert len(result2.data["allReporters"]["edges"]) == 2 assert not result2.data["allReporters"]["pageInfo"]["hasNextPage"] - gql_reporters = result.data["allReporters"]["edges"] + result2.data["allReporters"]["edges"] - - assert { - to_global_id("ReporterType", reporter.id) - for reporter in db_reporters - } == { - gql_reporter["node"]["id"] - for gql_reporter in gql_reporters + gql_reporters = ( + result.data["allReporters"]["edges"] + result2.data["allReporters"]["edges"] + ) + + assert {to_global_id("ReporterType", reporter.id) for reporter in db_reporters} == { + gql_reporter["node"]["id"] for gql_reporter in gql_reporters } From 70f696ea2f7a77bda45319034e2d28af3ede46a3 Mon Sep 17 00:00:00 2001 From: Paul Craciunoiu Date: Wed, 10 Jun 2020 11:06:51 -0600 Subject: [PATCH 4/7] Update graphene_django/fields.py Co-authored-by: Jonathan Kim --- graphene_django/fields.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphene_django/fields.py b/graphene_django/fields.py index f86f99753..8b30f3c77 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -135,7 +135,7 @@ def resolve_connection(cls, connection, args, iterable, max_limit=None): if isinstance(iterable, QuerySet): list_length = iterable.count() - list_slice_length = max_limit or list_length + list_slice_length = max(max_limit, list_length) else: list_length = len(iterable) list_slice_length = max_limit or list_length From 81df2bf244fcd6460e0da3a781afc0c0f29860cf Mon Sep 17 00:00:00 2001 From: Paul Craciunoiu Date: Wed, 10 Jun 2020 11:06:58 -0600 Subject: [PATCH 5/7] Update graphene_django/fields.py Co-authored-by: Jonathan Kim --- graphene_django/fields.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphene_django/fields.py b/graphene_django/fields.py index 8b30f3c77..9d727eb1f 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -138,7 +138,7 @@ def resolve_connection(cls, connection, args, iterable, max_limit=None): list_slice_length = max(max_limit, list_length) else: list_length = len(iterable) - list_slice_length = max_limit or list_length + list_slice_length = max(max_limit, list_length) after = get_offset_with_default(args.get("after"), -1) + 1 list_slice_length += after From 9b3344d5d9f89dc144d137ffc4d4f7c142eda054 Mon Sep 17 00:00:00 2001 From: Paul Craciunoiu Date: Wed, 10 Jun 2020 12:26:06 -0600 Subject: [PATCH 6/7] Fix tests. --- graphene_django/fields.py | 1 + 1 file changed, 1 insertion(+) diff --git a/graphene_django/fields.py b/graphene_django/fields.py index 9d727eb1f..980b9e825 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -133,6 +133,7 @@ def resolve_queryset(cls, connection, queryset, info, args): def resolve_connection(cls, connection, args, iterable, max_limit=None): iterable = maybe_queryset(iterable) + max_limit = max_limit or 0 if isinstance(iterable, QuerySet): list_length = iterable.count() list_slice_length = max(max_limit, list_length) From 6fd5cc9522c22f5ab4a59b3531e30cd558ae9a9c Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Wed, 24 Jun 2020 15:01:49 +0100 Subject: [PATCH 7/7] Fix arguments to connection_from_list_slice --- graphene_django/fields.py | 14 ++++++++------ graphene_django/tests/test_query.py | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/graphene_django/fields.py b/graphene_django/fields.py index 980b9e825..ac7ce45d0 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -133,21 +133,23 @@ def resolve_queryset(cls, connection, queryset, info, args): def resolve_connection(cls, connection, args, iterable, max_limit=None): iterable = maybe_queryset(iterable) - max_limit = max_limit or 0 if isinstance(iterable, QuerySet): list_length = iterable.count() - list_slice_length = max(max_limit, list_length) + list_slice_length = ( + min(max_limit, list_length) if max_limit is not None else list_length + ) else: list_length = len(iterable) - list_slice_length = max(max_limit, list_length) + list_slice_length = ( + min(max_limit, list_length) if max_limit is not None else list_length + ) after = get_offset_with_default(args.get("after"), -1) + 1 - list_slice_length += after connection = connection_from_list_slice( - iterable, + iterable[after:], args, - slice_start=0, + slice_start=after, list_length=list_length, list_slice_length=list_slice_length, connection_type=connection, diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index 3440586bf..64f54bb46 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -1127,7 +1127,7 @@ class Query(graphene.ObjectType): def test_should_have_next_page(graphene_settings): - graphene_settings.RELAY_CONNECTION_MAX_LIMIT = 4 + graphene_settings.RELAY_CONNECTION_MAX_LIMIT = 6 reporters = [Reporter(**kwargs) for kwargs in REPORTERS] Reporter.objects.bulk_create(reporters) db_reporters = Reporter.objects.all()