Skip to content

fix: unit test for graphene pr#1412 #1315

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 23, 2022
Merged

fix: unit test for graphene pr#1412 #1315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d564a0
Issue #1111: foreign key should also call get_queryset method
Feb 10, 2021
c4aa139
Merge pull request #12 from loft-orbital/users/tcleonard/issue-#1111-…
tcleonard Mar 20, 2022
ca55529
fix: test for graphene PR https://github.com/graphql-python/graphene/…
Mar 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion graphene_django/converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,24 @@ def dynamic_type():
if not _type:
return

return Field(
class CustomField(Field):
def wrap_resolve(self, parent_resolver):
"""
Implements a custom resolver which go through the `get_node` method to insure that
it goes through the `get_queryset` method of the DjangoObjectType.
"""
resolver = super().wrap_resolve(parent_resolver)

def custom_resolver(root, info, **args):
fk_obj = resolver(root, info, **args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You run the resolver, get a result (django model) back and then run _type.get_node to fetch it again making a 2nd unnecessary query?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an issue I've also been getting, queries are forced every time an object is resolved, making an exponential number of queries when nesting without any possibility of optimizing

Copy link
Collaborator

@mahdyhamad mahdyhamad Mar 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fairly new to this code base and I am trying to resolve the issue raised by @ekampf

In my attempt to fix this:

  • I started by getting the id of the field to be resolved (which is the foreign-key object) from the root in an attempt to stop calling one of the functions that resulting in a hit to the database.

  • Then I check the resolver

    • No Custom resolver
      • if the resolver equals to the default resolver (meaning that there is no custom resolver for the field in the root node), call get_node and return.
      • At this point and this scenario, the issue is solved, because there is one resolver that was called resulting in one hit to the database and thats fine!
    • Custom Resolver Exists
      • At this point, there is a custom resolver, which means that the node should go through multiple resolvers: the resolver method + get_node (I say should because the tests and the code written to make sure that the processes of resolving the node always goes through the get_node)
      • ** In this case, I did not include when the resolver is awaitable

  • But the wall I faced after that is the fact that if is required to call get_node even though the node is resolved as a result of calling the resolver function. Which stopped me for a second, why there are two functions responsible for resolving the

@tcleonard any thoughts how this can be fixed?

I think this is not the right way to assure that get_node is always called. The solution should be more concrete.
I was thinking of a way to pass the instance returned from get_node to the resolver function if node exists. Because there is no reason to get the instance 2 times. But I am not sure how this is can be done.

Copy link
Collaborator

@sjdemartini sjdemartini Apr 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out the issues, and for attempting to fix! In the meantime until a solution can be worked out that doesn't cause N+1 queries and doesn't prevent query-optimization (assuming the original PR author or someone else do care to support the original intention of this PR), I've opened a PR to revert the problematic code here #1401, since this caused a significant regression relative to graphene-django v3.0.0b7 is a blocker for many people looking to upgrade to graphene-django v3.

if fk_obj is None:
return None
else:
return _type.get_node(info, fk_obj.pk)

return custom_resolver

return CustomField(
_type,
description=get_django_field_description(field),
required=not field.null,
Expand Down
5 changes: 1 addition & 4 deletions graphene_django/filter/tests/test_array_field_exact_filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,7 @@ def test_array_field_filter_schema_type(Query):
"randomField": "[Boolean!]",
}
filters_str = ", ".join(
[
f"{filter_field}: {gql_type} = null"
for filter_field, gql_type in filters.items()
]
[f"{filter_field}: {gql_type}" for filter_field, gql_type in filters.items()]
)
assert (
f"type Query {{\n events({filters_str}): EventTypeConnection\n}}" in schema_str
Expand Down
5 changes: 1 addition & 4 deletions graphene_django/filter/tests/test_enum_filtering.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,6 @@ def test_filter_enum_field_schema_type(schema):
"reporter_AChoice_In": "[TestsReporterAChoiceChoices]",
}
filters_str = ", ".join(
[
f"{filter_field}: {gql_type} = null"
for filter_field, gql_type in filters.items()
]
[f"{filter_field}: {gql_type}" for filter_field, gql_type in filters.items()]
)
assert f" allArticles({filters_str}): ArticleTypeConnection\n" in schema_str
4 changes: 2 additions & 2 deletions graphene_django/filter/tests/test_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1008,7 +1008,7 @@ class Query(ObjectType):
assert str(schema) == dedent(
"""\
type Query {
pets(offset: Int = null, before: String = null, after: String = null, first: Int = null, last: Int = null, age: Int = null): PetTypeConnection
pets(offset: Int, before: String, after: String, first: Int, last: Int, age: Int): PetTypeConnection
}

type PetTypeConnection {
Expand Down Expand Up @@ -1077,7 +1077,7 @@ class Query(ObjectType):
assert str(schema) == dedent(
"""\
type Query {
pets(offset: Int = null, before: String = null, after: String = null, first: Int = null, last: Int = null, age: Int = null, age_Isnull: Boolean = null, age_Lt: Int = null): PetTypeConnection
pets(offset: Int, before: String, after: String, first: Int, last: Int, age: Int, age_Isnull: Boolean, age_Lt: Int): PetTypeConnection
}

type PetTypeConnection {
Expand Down
2 changes: 1 addition & 1 deletion graphene_django/filter/tests/test_typed_filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def test_typed_filter_schema(schema):
)

for filter_field, gql_type in filters.items():
assert "{}: {} = null".format(filter_field, gql_type) in all_articles_filters
assert "{}: {}".format(filter_field, gql_type) in all_articles_filters


def test_typed_filters_work(schema):
Expand Down
3 changes: 3 additions & 0 deletions graphene_django/tests/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ class Person(models.Model):
class Pet(models.Model):
name = models.CharField(max_length=30)
age = models.PositiveIntegerField()
owner = models.ForeignKey(
"Person", on_delete=models.CASCADE, null=True, blank=True, related_name="pets"
)


class FilmDetails(models.Model):
Expand Down
Loading