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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
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_countThis 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 |
Description
The
strawberry_django.relay.ListConnectionWithTotalCount.resolve_connection_from_cachemethod currently incorrectly attempts to determinehas_next_pagefrom theQuerySet._result_cache, as opposed to the cached records within it, resulting in anAttributeErrorbeing raised.This PR ensures that
has_next_pageis determined via the last record in theQuerySet._result_cache(if applicable), so that we don't get anAttributeErrorTypes of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Fix the calculation of
has_next_pageinresolve_connection_from_cacheto prevent anAttributeErrorby using the last record in theQuerySet._result_cache.Bug Fixes:
has_next_pageinresolve_connection_from_cacheto prevent anAttributeErrorby correctly determining it from the last record in theQuerySet._result_cache.