Skip to content
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

Do not access the internals of SimpleLazyObject #945

Merged
merged 3 commits into from May 9, 2020

Conversation

pcraciunoiu
Copy link
Contributor

This API is not meant to be used, and it breaks when double-wrapped lazy objects occur, which appears to be a long standing practice.

See original issue in django-otp django-otp/django-otp#36

This was introduced a long time ago, in v1.1.0, surprised it hasn't come up before?
d73f4aa

At any rate, if you have both AuthenticationMiddleware and Django OTP, this will blow up with:

Exception: Received incompatible instance

@pcraciunoiu
Copy link
Contributor Author

For context, see also Django source code, which implies __class__ is the right way to check the class of a lazy object:
https://github.com/django/django/blob/4b146e0c83891fc67a422aa22f846bb7654c4d38/django/utils/functional.py#L332

@pcraciunoiu
Copy link
Contributor Author

@jkimbo any thoughts on this? It's a fairly small change and tests pass.

@jkimbo
Copy link
Member

jkimbo commented May 9, 2020

@pcraciunoiu sorry for the delay. Could you add a test to this PR to show the changes working?

@pcraciunoiu
Copy link
Contributor Author

Here you go. Before the changes, the test results in:

=============================================== FAILURES ================================================
_____________________________ test_should_query_wrapped_simplelazy_objects ______________________________

    def test_should_query_wrapped_simplelazy_objects():
        class ReporterType(DjangoObjectType):
            class Meta:
                model = Reporter
                fields = ("id",)
    
        class Query(graphene.ObjectType):
            reporter = graphene.Field(ReporterType)
    
            def resolve_reporter(self, info):
                return SimpleLazyObject(lambda: SimpleLazyObject(lambda: Reporter(id=1)))
    
        schema = graphene.Schema(query=Query)
        query = """
            query {
              reporter {
                id
              }
            }
        """
        result = schema.execute(query)
>       assert not result.errors
E       assert not [Exception('Received incompatible instance " ".',)]
E        +  where [Exception('Received incompatible instance " ".',)] = <graphql.execution.base.ExecutionResult object at 0x7f434877bdc8>.errors

graphene_django/tests/test_query.py:84: AssertionError
------------------------------------------- Captured log call -------------------------------------------
ERROR    graphql.execution.utils:utils.py:155 Traceback (most recent call last):
  File "/home/paul/.local/share/virtualenvs/graphene-django-aq2KKQnM/lib/python3.6/site-packages/graphql/execution/executor.py", line 481, in complete_value_catching_error
    exe_context, return_type, field_asts, info, path, result
  File "/home/paul/.local/share/virtualenvs/graphene-django-aq2KKQnM/lib/python3.6/site-packages/graphql/execution/executor.py", line 572, in complete_value
    exe_context, return_type, field_asts, info, path, result
  File "/home/paul/.local/share/virtualenvs/graphene-django-aq2KKQnM/lib/python3.6/site-packages/graphql/execution/executor.py", line 712, in complete_object_value
    if return_type.is_type_of and not return_type.is_type_of(result, info):
  File "/home/paul/Projects/graphene-django/graphene_django/types.py", line 277, in is_type_of
    raise Exception(('Received incompatible instance "{}".').format(root))
Exception: Received incompatible instance " ".

I tried setting up the middleware to do it, but there's no pattern of tests that use Django's middleware yet, so I hope this will suffice. Lazy should be allowed to work when they're chained and now that part's tested.

Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

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

Great thanks!

@jkimbo jkimbo merged commit 2225ed6 into graphql-python:master May 9, 2020
@pcraciunoiu
Copy link
Contributor Author

pcraciunoiu commented May 9, 2020

Thank you too @jkimbo can you say if this will make it into graphene-django 2 as well as 3?

Ah looks like I just missed today's release :/ hope there will be one in the next couple of weeks.

@jkimbo
Copy link
Member

jkimbo commented May 10, 2020

@pcraciunoiu yep I'll release this as part of v2.10.1 soon. I was just going to wait a couple of days to see if there are any issues that come up as a result of the v2.10.0 release.

@pcraciunoiu
Copy link
Contributor Author

@jkimbo thoughts on a release with the fix this week?

@jkimbo
Copy link
Member

jkimbo commented May 18, 2020

@pcraciunoiu sorry for the delay. Just released v2.10.1 with your fix: https://github.com/graphql-python/graphene-django/releases/tag/v2.10.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants