Change pydantic conversion to not load field data unless requested#3812
Conversation
Reviewer's Guide by SourceryThis pull request modifies the Pydantic model conversion to avoid unnecessary data retrieval for unrequested fields. The changes involve moving data retrieval logic inside a conditional block that checks if the field is initialized. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @Mark90 - I've reviewed your changes - here's some feedback:
Overall Comments:
- It would be helpful to add a test case that specifically demonstrates the performance improvement from this change.
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
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
If tests to measure the performance difference are required I'll be happy to supply them; I haven't delved into the testsuite deep enough yet to know how you typically do this. I just wanted to get this PR up before I forget to do so again. :) |
|
@Mark90 thanks!
They aren't really needed, unless you think there was a big change! |
|
@Mark90 but it would be nice to have some test to check the behaviour is what we expect now 😄 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3812 +/- ##
=======================================
Coverage 97.23% 97.23%
=======================================
Files 503 503
Lines 33580 33580
Branches 1717 1717
=======================================
Hits 32653 32653
Misses 708 708
Partials 219 219 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #3812 will not alter performanceComparing Summary
|
Done! The added test fails without the change in The test uses |
|
@patrick91 Have you had a chance to look at this? |
|
Thanks for contributing to Strawberry! 🎉 You've been invited to join You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67 And don't forget to join our discord server: https://strawberry.rocks/discord 🔥 |


Changes the Pydantic model conversion so that it doesn't retrieve data for fields that aren't going to be used.
Description
It's been 8 months since I manually patched this in our own codebase, I wanted to create a PR here after some time when I was sure it didn't cause any regressions.
Without this patch I observed unnecessary requests (lazyloading through our ORM) being made which originated from fields on "Pydantic strawberry types" that weren't requested in the query. Moving these lines inside the
if field.init:section appeared sufficient to fix this.Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Enhancements: