Unwrap django lazy objects in mutation resolvers

[ryanprobus]

Description

• Django lazy objects have a proxy __iter__ method that causes them to be considered iterables even if the wrapped object isn't. This causes errors when calling list on the lazy object. Avoid this by checking and unwrapping any lazy objects. A lazy object could be passed to these mutation resolvers if accessing the django request's user.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Documentation

Issues Fixed or Closed by This PR

• Django LazyObject is not considered in mutation resolvers

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[bellini666]

The changes look great!

Can we have a test for it? Just to avoid that from regressing in the future for whatever reason :)

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.12% 🎉

Comparison is base (db978aa) 87.87% compared to head (37500b1) 87.99%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
+ Coverage   87.87%   87.99%   +0.12%     
==========================================
  Files          33       33              
  Lines        2935     2940       +5     
==========================================
+ Hits         2579     2587       +8     
+ Misses        356      353       -3     

Files Changed	Coverage Δ
strawberry_django/mutations/resolvers.py	87.00% <100.00%> (+0.26%)	⬆️

... and 1 file with indirect coverage changes

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

[ryanprobus]

@bellini666 I am not familiar with the pyright issues that are reported by the Run Tests / Typing check. To me, they seem unrelated to this change. Is this test known to be fragile? I didn't dive into it, but I am guessing it just reports issues on any files with diff in the PR?

[bellini666]

@bellini666 I am not familiar with the pyright issues that are reported by the Run Tests / Typing check. To me, they seem unrelated to this change. Is this test known to be fragile? I didn't dive into it, but I am guessing it just reports issues on any files with diff in the PR?

Indeed they are currently not your fault =P. It just happened to a new version of pyright to be released while this PR was opened, and it did have some regressions in our code which I fixed in aed240e

You can ignore those or just merge/rebase on top of the current main branch.

Btw, the PR is still marked as draft. Is there anything missing or can I review it?

[bellini666]

@ryanprobus taking a look at your PR, maybe some of the pyright issues are due to your PR. More specifically, the issues inside mutations/resolvers.py

The reason is that instance = instance.__reduce__()[1][0] isn't typed in a way that pyright would know that it would be the instance and is being interpreted as Unknown. You can instance = cast(_M, instance.__reduce__()[1][0]) to avoid that in this case

[bellini666]

Hi @ryanprobus ,

pyright still thinks that the instance. I checked out your branch and the issue is that the function itself is not typed (because the overloads are)

You might want to change it from:

@transaction.atomic
def update(
    info,
    instance,
    data,
    *,
    full_clean: bool | FullCleanOptions = True,
    pre_save_hook: Callable[[_M], None] | None = None,
):

to:

@transaction.atomic
def update(
    info: Info,
    instance: _M | Iterable[_M],
    data: dict[str, Any],
    *,
    full_clean: bool | FullCleanOptions = True,
    pre_save_hook: Callable[[_M], None] | None = None,
) -> _M | list[_M]:

That should fix everything and IMO the patch is ready to be merged

[bellini666]

Hey @ryanprobus ,

I took the liberty of updating your PR and fixing the remaining typing issues.

I removed its "Draft" status because I believe it is ready to be merged.

Is there anything else missing in here? Or can I merge it as soon as the tests pass?

[ryanprobus]

@bellini666 Its all good to go if you it looks good to you. Thanks for taking a look at the pyright issues.