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

fix(appsync): make contextual data accessible for async functions #5317

Merged
merged 6 commits into from
Oct 24, 2024

Conversation

leandrodamascena
Copy link
Contributor

Issue number: #5290

Summary

Changes

This PR addresses an issue where the App/Router context was not available when using AppSyncResolver with async functions. The problem stemmed from the context being cleared too early in the execution flow, specifically before async functions could access it.

This change allows the context to persist throughout the execution of async functions, ensuring that contextual data remains accessible when needed. However, it shifts the responsibility of context clearing to the developer for async functions to prevent potential data leaks or unintended data persistence between invocations.

We've also updated the documentation to clearly communicate this change to users, providing guidance on how to manually clear the context when using async resolvers.

This bug doesn't affect batch resolvers.

What I fixed:

  1. Modifying the resolve method in the AppSyncResolver class class to detect if the response is a coroutine.
  2. If the response is a coroutine (indicating an async function), the context is not cleared automatically.
  3. For non-coroutine responses, the context continues to be cleared automatically for safety.

User experience

def test_route_context_is_manually_cleared_after_resolve_async():
    app = AppSyncResolver()

    mock_event = {"typeName": "Customer", "fieldName": "field", "arguments": {}}

    @app.resolver(field_name="field")
    async def get_async():
        app.context.clear()
        await asyncio.sleep(0.0001)
        return "value"

    # WHEN
    mock_context = LambdaContext()
    app.append_context(is_admin=True)
    result = app.resolve(mock_event, mock_context)

    # THEN
    assert asyncio.run(result) == "value"
    assert app.context == {}

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@leandrodamascena leandrodamascena requested a review from a team as a code owner October 5, 2024 23:24
@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation event_handlers tests labels Oct 5, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 5, 2024
@github-actions github-actions bot added bug Something isn't working and removed documentation Improvements or additions to documentation labels Oct 5, 2024
Copy link

codecov bot commented Oct 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.15%. Comparing base (c11d25c) to head (1696526).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #5317   +/-   ##
========================================
  Coverage    96.15%   96.15%           
========================================
  Files          229      229           
  Lines        10806    10807    +1     
  Branches      2006     2007    +1     
========================================
+ Hits         10391    10392    +1     
  Misses         327      327           
  Partials        88       88           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Oct 7, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Oct 7, 2024
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Oct 24, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Oct 24, 2024
dreamorosi
dreamorosi previously approved these changes Oct 24, 2024
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Oct 24, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Oct 24, 2024
dreamorosi
dreamorosi previously approved these changes Oct 24, 2024
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Oct 24, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Oct 24, 2024
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Oct 24, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Oct 24, 2024
Copy link

@leandrodamascena leandrodamascena merged commit ccfbc94 into develop Oct 24, 2024
11 checks passed
@leandrodamascena leandrodamascena deleted the fix/appsync-async-resolver branch October 24, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working event_handlers size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Router context not being available when using AppSync with async resolvers
3 participants