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: Make sure that async fields always return Awaitables #646

Merged
merged 1 commit into from
Oct 19, 2024

Conversation

bellini666
Copy link
Member

@bellini666 bellini666 commented Oct 19, 2024

Async fields sometimes can return non-awaitable values, like when the optimizer is enabled and already prefetched the nested field.

This can break some places that expect an awaitable, like strawberry's base field permissions.

This change ensures that async fields always return an awaitable.

Fix #639

Summary by Sourcery

Fix async fields to consistently return awaitables, addressing issues with components expecting awaitable results, and add tests to validate this behavior.

Bug Fixes:

  • Ensure async fields always return an awaitable to prevent issues with components expecting awaitable results.

Tests:

  • Add tests to verify async field permissions and behavior with and without optimizers, ensuring consistent awaitable returns.

Async fields sometimes can return non-awaitable values, like when
the optimizer is enabled and already prefetched the nested field.

This can break some places that expect an awaitable, like strawberry's
base field permissions.

This change ensures that async fields always return an awaitable.
@bellini666 bellini666 self-assigned this Oct 19, 2024
Copy link
Contributor

sourcery-ai bot commented Oct 19, 2024

Reviewer's Guide by Sourcery

This pull request fixes an issue where async fields could return non-awaitable values, potentially breaking functionality that expects awaitables. The change ensures that async fields always return an awaitable, addressing issue #639.

Class diagram for AsyncPermission and Query classes

classDiagram
    class AsyncPermission {
        +async has_permission(source: Any, info: Info, **kwargs: Any) Union[bool, Awaitable[bool]]
    }
    class Query {
        +list[Color] colors
    }
    class Color {
        +strawberry.auto name
        +list[Fruit] fruits
    }
    class Fruit {
        +strawberry.auto name
    }
    Query --> Color : has
    Color --> Fruit : has
    Color --> AsyncPermission : uses
    Fruit --> AsyncPermission : uses
Loading

File-Level Changes

Change Details Files
Modify the get_result function to always return an awaitable for async fields
  • Change the condition to check for both is_awaitable and self.is_async
  • Use await_maybe function to handle both awaitable and non-awaitable results
strawberry_django/fields/field.py
Add comprehensive test cases for field permissions with async and sync scenarios
  • Create test cases for async permissions
  • Create test cases for sync permissions
  • Add tests with and without the DjangoOptimizerExtension
  • Test both async and sync schema execution
tests/test_field_permissions.py

Assessment against linked issues

Issue Objective Addressed Explanation
#639 Fix the 'object list can't be used in 'await' expression' error when using async has_permission with optimizer's enable_prefetch_related_optimization set to True
#639 Ensure async fields always return Awaitables

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

tests/test_field_permissions.py Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.77%. Comparing base (92c1221) to head (ab3bbe3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #646      +/-   ##
==========================================
+ Coverage   88.74%   88.77%   +0.03%     
==========================================
  Files          41       41              
  Lines        3606     3607       +1     
==========================================
+ Hits         3200     3202       +2     
+ Misses        406      405       -1     

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

@bellini666 bellini666 merged commit 31bb262 into main Oct 19, 2024
24 checks passed
@bellini666 bellini666 deleted the fix_async_fields branch October 19, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants