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

ApolloTracingExtension regression populating resolvers field. #3656

Open
coady opened this issue Oct 5, 2024 · 2 comments
Open

ApolloTracingExtension regression populating resolvers field. #3656

coady opened this issue Oct 5, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@coady
Copy link
Member

coady commented Oct 5, 2024

Describe the Bug

import asyncio
import strawberry
from strawberry.extensions import tracing

@strawberry.type
class Query:
    @strawberry.field
    def node(self) -> str:
        return ''

schema = strawberry.Schema(Query, extensions=[tracing.ApolloTracingExtension])

for i in range(2):
    result = asyncio.run(schema.execute('{ node }'))
    assert result.extensions['tracing']['execution']['resolvers'], i  # second time fails

outputs

    assert result.extensions['tracing']['execution']['resolvers'], i  # second time fails
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
AssertionError: 1

Only the first request populates resolvers as expected. This worked in previous version.

System Information

  • Strawberry version (if applicable): >=0.240

Additional Context

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@coady coady added the bug Something isn't working label Oct 5, 2024
@coady
Copy link
Member Author

coady commented Oct 5, 2024

The regression occurred in 0dcf23d, which changed how extensions are created. It appears a new extension instance is created on each request, resolve is called on the old instance, then get_results is called on the new instance. So this should be affecting other schema extensions as well; any which maintain state in resolve.

@erikwrede
Copy link
Member

Hey @coady,
thanks for reporting this and investigating where the regression occured. Looking deeper into the issue, I see the following conflict we introduced:

The Middleware Manager is cached, at the same time it caches all resolver functions, including the extension resolvers wrapping them. Resolver functions are bound to a specific extension instance, and recently we started creating a new extension instance for each request. That means that for each request after the first, the resolver function we are calling is still bound to the first-ever schema extension instance.

My suggestion: let's create a custom MiddlewareManager subclass that injects the current instance of each extension into the resolvers. Alternatively, we can just disable caching for now and take the performance penalty. Tagging @bellini666 and @DoctorJohn for a second opinion. The relevant code is graphql_core.middleware @ L46 and strawberry.schema._get_middleware_manager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants