-
-
Notifications
You must be signed in to change notification settings - Fork 116
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: Make sure the optimizer calls the type's get_queryset
for nested lists/connections
#568
Conversation
…ed lists/connections Fix #562
Reviewer's Guide by SourceryThis pull request addresses issue #562 by ensuring that the optimizer calls the type's 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 @bellini666 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 5 issues found
- 🟢 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.
else: | ||
qs = remote_model._base_manager.all() # type: ignore | ||
|
||
f_type = field.type |
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: Resolved field type
The resolution of field.type
to f_type
is a good step. Ensure that all possible types are correctly handled in the subsequent logic.
f_type = field.type | |
f_type = field.type | |
if isinstance(f_type, LazyType): | |
f_type = f_type.resolve_type() | |
elif isinstance(f_type, SomeOtherType): | |
f_type = handle_some_other_type(f_type) | |
elif isinstance(f_type, AnotherType): | |
f_type = handle_another_type(f_type) |
@@ -1325,3 +1325,43 @@ def test_nested_prefetch_with_multiple_levels(db, gql_client: GraphQLTestClient) | |||
"issues": expected_issues, | |||
}, | |||
} | |||
|
|||
|
|||
@pytest.mark.django_db(transaction=True) |
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 (testing): Consider adding a test for when get_queryset
is not called
It would be beneficial to add a test case to ensure that get_queryset
is not called when prefetch_custom_queryset
is disabled. This will help verify that the default behavior is maintained.
@pytest.mark.django_db(transaction=True) | |
@pytest.mark.django_db(transaction=True) | |
def test_nested_prefetch_with_get_queryset_disabled(db): | |
# Setup your test data here | |
# Call the function or method that should not call get_queryset | |
# Assert that get_queryset was not called |
|
||
|
||
@pytest.mark.django_db(transaction=True) | ||
def test_nested_prefetch_with_get_queryset( |
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 (testing): Test function name could be more descriptive
Consider renaming the test function to something more descriptive, such as test_nested_prefetch_calls_get_queryset_when_enabled
. This will make it clearer what specific behavior is being tested.
def test_nested_prefetch_with_get_queryset( | |
def test_nested_prefetch_calls_get_queryset_when_enabled( |
gql_client: GraphQLTestClient, | ||
mocker: MockerFixture, | ||
): | ||
mock_get_queryset = mocker.spy(StaffType, "get_queryset") |
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 (testing): Consider adding assertions for the query result
It would be helpful to add assertions to verify the structure and content of the query result. This ensures that the query returns the expected data.
mock_get_queryset = mocker.spy(StaffType, "get_queryset") | |
mock_get_queryset = mocker.spy(StaffType, "get_queryset") | |
response = gql_client.execute(query) | |
assert response is not None | |
assert "data" in response | |
assert "errors" not in response |
for u in [user, staff]: | ||
Assignee.objects.create(user=u, issue=issue) | ||
|
||
res = gql_client.query( |
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 (testing): Consider adding error handling tests
It would be beneficial to add tests that handle potential errors, such as when the id
provided does not exist. This will help ensure that the code handles such cases gracefully.
res = gql_client.query( | |
try: | |
res = gql_client.query( | |
query, | |
{"id": to_base64("IssueType", issue.pk)}, | |
) | |
except SomeSpecificException as e: | |
# Handle the exception or log it | |
res = None |
{"id": to_base64("IssueType", issue.pk)}, | ||
) | ||
|
||
assert isinstance(res.data, dict) |
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 (testing): Consider using more specific assertions
Instead of just checking if res.data
is a dictionary, consider using more specific assertions to verify the exact structure and content of the response. This will make the test more robust.
assert isinstance(res.data, dict) | |
assert "issue" in res.data | |
assert "id" in res.data["issue"] | |
assert res.data["issue"]["id"] == to_base64("IssueType", issue.pk) |
for u in [user, staff]: | ||
Assignee.objects.create(user=u, issue=issue) |
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.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests
)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #568 +/- ##
==========================================
+ Coverage 88.88% 89.00% +0.11%
==========================================
Files 41 41
Lines 3545 3547 +2
==========================================
+ Hits 3151 3157 +6
+ Misses 394 390 -4 ☔ View full report in Codecov by Sentry. |
Fix #562
Summary by Sourcery
This pull request fixes an issue where the optimizer was not calling the type's
get_queryset
method for nested lists and connections. It also refactors the optimizer to use_default_manager
for prefetching whenprefetch_custom_queryset
is enabled. Additionally, a new test is added to ensure the correct behavior.get_queryset
method for nested lists and connections._default_manager
for prefetching whenprefetch_custom_queryset
is enabled, otherwise use_base_manager
.get_queryset
method for nested lists and connections.