-
-
Notifications
You must be signed in to change notification settings - Fork 131
Add support for "true" cursor based pagination in connections #730
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
base: main
Are you sure you want to change the base?
Changes from all commits
5f57eb6
cea6194
86e2262
ed2abab
f5a54de
5e5fee3
3d4cdb4
d90a9ee
c018206
5b0e8fc
4ea320b
7321244
d8ef9b2
2885f05
b307c9d
61e9a97
b78b6fc
1611301
074b2b4
cfa740f
7bed380
ab3a7e4
7237d01
e4fae12
2692f3e
b009e65
bb317b7
50d7f35
2d8541c
4f3213f
c9bc3e3
3ed07d9
39dcdd6
b57dfae
866589f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -41,7 +41,7 @@ | |||
from strawberry.relay.utils import SliceMetadata | ||||
from strawberry.schema.schema import Schema | ||||
from strawberry.schema.schema_converter import get_arguments | ||||
from strawberry.types import get_object_definition | ||||
from strawberry.types import get_object_definition, has_object_definition | ||||
from strawberry.types.base import StrawberryContainer | ||||
from strawberry.types.info import Info | ||||
from strawberry.types.lazy_type import LazyType | ||||
|
@@ -94,7 +94,6 @@ | |||
"optimize", | ||||
] | ||||
|
||||
NESTED_PREFETCH_MARK = "_strawberry_nested_prefetch_optimized" | ||||
_M = TypeVar("_M", bound=models.Model) | ||||
|
||||
_sentinel = object() | ||||
|
@@ -496,6 +495,8 @@ def _optimize_prefetch_queryset( | |||
StrawberryDjangoField, | ||||
) | ||||
|
||||
from .relay_cursor import DjangoCursorConnection, apply_cursor_pagination | ||||
|
||||
if ( | ||||
not config | ||||
or not config.enable_nested_relations_prefetch | ||||
|
@@ -571,6 +572,17 @@ def _optimize_prefetch_queryset( | |||
limit=slice_metadata.end - slice_metadata.start, | ||||
max_results=connection_extension.max_results, | ||||
) | ||||
elif connection_type is DjangoCursorConnection: | ||||
qs, _ = apply_cursor_pagination( | ||||
qs, | ||||
related_field_id=related_field_id, | ||||
info=Info(_raw_info=info, _field=field), | ||||
first=field_kwargs.get("first"), | ||||
last=field_kwargs.get("last"), | ||||
before=field_kwargs.get("before"), | ||||
after=field_kwargs.get("after"), | ||||
max_results=connection_extension.max_results, | ||||
) | ||||
else: | ||||
mark_optimized = False | ||||
|
||||
|
@@ -1246,13 +1258,20 @@ def _get_model_hints_from_connection( | |||
if edge.name.value != "edges": | ||||
continue | ||||
|
||||
e_definition = get_object_definition(relay.Edge, strict=True) | ||||
e_type = e_definition.resolve_generic( | ||||
relay.Edge[cast("type[relay.Node]", n_type)], | ||||
) | ||||
e_field = object_definition.get_field("edges") | ||||
if e_field is None: | ||||
break | ||||
|
||||
e_definition = e_field.type | ||||
while isinstance(e_definition, StrawberryContainer): | ||||
e_definition = e_definition.of_type | ||||
if has_object_definition(e_definition): | ||||
e_definition = get_object_definition(e_definition, strict=True) | ||||
assert isinstance(e_definition, StrawberryObjectDefinition) | ||||
|
||||
e_gql_definition = _get_gql_definition( | ||||
schema, | ||||
get_object_definition(e_type, strict=True), | ||||
e_definition, | ||||
) | ||||
assert isinstance(e_gql_definition, (GraphQLObjectType, GraphQLInterfaceType)) | ||||
e_info = _generate_selection_resolve_info( | ||||
|
@@ -1451,20 +1470,17 @@ def optimize( | |||
|
||||
|
||||
def is_optimized(qs: QuerySet) -> bool: | ||||
return get_queryset_config(qs).optimized or is_optimized_by_prefetching(qs) | ||||
config = get_queryset_config(qs) | ||||
return config.optimized or config.optimized_by_prefetching | ||||
|
||||
|
||||
def mark_optimized_by_prefetching(qs: QuerySet[_M]) -> QuerySet[_M]: | ||||
# This is a bit of a hack, but there is no easy way to mark a related manager | ||||
# as optimized at this phase, so we just add a mark to the queryset that | ||||
# we can check leater on using is_optimized_by_prefetching | ||||
return qs.annotate(**{ | ||||
NESTED_PREFETCH_MARK: models.Value(True), | ||||
}) | ||||
get_queryset_config(qs).optimized_by_prefetching = True | ||||
return qs | ||||
Comment on lines
+1478
to
+1479
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: is this change safe? If I remember correctly, the reason we would do this was to mark a prefetch query as optimized, because that config there could be lost Is that not an issue anymore with the changes here? I sincerely don't remember how it would be lost and even if we have a test that would break without this (hopefully we do) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my understanding it is safe, yes. If I change
My understanding of why this code was written as it was before my change is as follows:
|
||||
|
||||
|
||||
def is_optimized_by_prefetching(qs: QuerySet) -> bool: | ||||
return NESTED_PREFETCH_MARK in qs.query.annotations | ||||
return get_queryset_config(qs).optimized_by_prefetching | ||||
|
||||
|
||||
optimizer: contextvars.ContextVar[DjangoOptimizerExtension | None] = ( | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Clarify the meaning of "true cursors". Suggest using "offset-based cursors" vs "range-based cursors" to distinguish the approaches.
The term "true cursors" might be confusing to users. Using more descriptive terms like "offset-based cursors" and "range-based cursors" would improve clarity.
Suggested implementation: