Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix: DjangoOptimizerExtension corrupts nested objects' fields' prefetch objects #380

Conversation

aprams
Copy link
Contributor

@aprams aprams commented Sep 30, 2023

Description

This PR aims to resolve issue #379 .

I included a reproducible test and a suggested fix, where the Prefetch object gets deepcopied to avoid the side effects from add_prefix.

Considerations:

  • There were multiple ways to do this, including copying the OptimizerStore at a higher level in the execution stack, but this should be the least invasive one
  • I used deepcopy now as Prefetch didn't offer an easier way of copying the object that I know of, I'm happy about suggestions on improving this

I'm also very open for improving the test case included. If you know a better way on how to do it without the custom type setup, please let me know.

Types of Changes

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

Issues Fixed or Closed by This PR

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).

I love the work you do here, thanks a lot for the really awesome work! ❤️

@aprams aprams changed the title added test reproducing the described behavior Fix: DjangoOptimizerExtension corrupts nested objects' fields' prefetch objects Sep 30, 2023
@aprams aprams marked this pull request as ready for review September 30, 2023 14:35
Copy link
Member

@bellini666 bellini666 left a comment

Choose a reason for hiding this comment

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

Thanks for this! :)

The -1 is due to the copy vs deepcopy comment I've made. Solving it and we should be good to merge

p.add_prefix(prefix)
prefetch_related.append(p)
# add_prefix modifies the field's prefetch object, so we copy it before
p_copy = deepcopy(p)
Copy link
Member

Choose a reason for hiding this comment

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

Hrm, the only solution I can think right now would be to create a Prefetch object by hand, but then we would need to update it in case it gets changed in django.

The only question here would be: Any reason to use deepcopy instead of copy? Unless I'm missing something, a copy should be enough to fix the issue here

Copy link
Contributor Author

@aprams aprams Sep 30, 2023

Choose a reason for hiding this comment

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

Yeah, I tried instantiating directly, creating the Prefetch object by hand and it's also not super trivial with the internal levels and protected variables. Kind of a messy hard-to-copy class.

I don't really have a good reason for deepcopy, fair point, I changed it to copy.copy and the test still works :)

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

codecov-commenter commented Sep 30, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (a17b51b) 87.98% compared to head (a6695b5) 87.99%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #380   +/-   ##
=======================================
  Coverage   87.98%   87.99%           
=======================================
  Files          33       33           
  Lines        2971     2973    +2     
=======================================
+ Hits         2614     2616    +2     
  Misses        357      357           
Files Coverage Δ
strawberry_django/optimizer.py 89.13% <100.00%> (+0.06%) ⬆️

... and 2 files with indirect coverage changes

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

@aprams aprams requested a review from bellini666 September 30, 2023 18:13
@bellini666 bellini666 enabled auto-merge (squash) October 1, 2023 16:07
@bellini666 bellini666 disabled auto-merge October 1, 2023 16:12
@bellini666 bellini666 merged commit e24596d into strawberry-graphql:main Oct 1, 2023
34 of 35 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants