-
-
Notifications
You must be signed in to change notification settings - Fork 120
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
Fix calculation of has_next_page
in resolve_connection_from_cache
#622
Fix calculation of has_next_page
in resolve_connection_from_cache
#622
Conversation
Reviewer's Guide by SourceryThis pull request fixes a bug in the File-Level Changes
Tips
|
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.
Hey @SupImDos - I've reviewed your changes - here's some feedback:
Overall Comments:
- The fix looks good, but it would be beneficial to add test cases that reproduce the original issue and verify the fix. This will help prevent regression in the future.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
has_previous_page = ( | ||
nodes[0]._strawberry_row_number > 1 # type: ignore | ||
result[0]._strawberry_row_number > 1 # type: ignore | ||
if result | ||
else False | ||
) |
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: Consider refactoring to improve robustness and performance
The current implementation could raise an IndexError if result
is empty. Additionally, accessing result[0]
and result[-1]
multiple times may impact performance for larger lists. Consider refactoring like this:
if not result:
has_previous_page = False
has_next_page = False
else:
first_item = result[0]
last_item = result[-1]
has_previous_page = first_item._strawberry_row_number > 1
has_next_page = last_item._strawberry_row_number < last_item._strawberry_total_count
This approach handles the empty list case first and optimizes list access. It also maintains the corrected logic for has_next_page
, which is an improvement over the previous version.
has_previous_page = ( | |
nodes[0]._strawberry_row_number > 1 # type: ignore | |
result[0]._strawberry_row_number > 1 # type: ignore | |
if result | |
else False | |
) | |
if not result: | |
has_previous_page = False | |
else: | |
first_item = result[0] | |
has_previous_page = first_item._strawberry_row_number > 1 |
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.
Ty! :)
Going to merge this even with the typing issue, as it is actually related to the newest release of pyright, and I even need to fix other unrelated code
Description
The
strawberry_django.relay.ListConnectionWithTotalCount.resolve_connection_from_cache
method currently incorrectly attempts to determinehas_next_page
from theQuerySet._result_cache
, as opposed to the cached records within it, resulting in anAttributeError
being raised.This PR ensures that
has_next_page
is determined via the last record in theQuerySet._result_cache
(if applicable), so that we don't get anAttributeError
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Fix the calculation of
has_next_page
inresolve_connection_from_cache
to prevent anAttributeError
by using the last record in theQuerySet._result_cache
.Bug Fixes:
has_next_page
inresolve_connection_from_cache
to prevent anAttributeError
by correctly determining it from the last record in theQuerySet._result_cache
.