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 prefetching the same GQL type for batch request #658

Merged

Conversation

grynchuk
Copy link
Contributor

@grynchuk grynchuk commented Mar 8, 2024

I have issue with getting wrong prefetch data while query in batch mode. In short, first query prefetch specific data and set it to prefetch buffer, second query use previous query prefetch buffer instead of using its own and get incomplete data set. I have to note that this issue is relates to prefetching of nested data, see test for mo details.

Request:

[
  {
    "query": "query { contact (name: "Joe") { name  posts { id  title } } }",
  },
  {
    "query": "query { company (id: "1"){ name contact  { name  posts { id  title } } } }",
  }
]

Response
Actual:

[{"data":{"contact":{"name":"Joe","posts":[{"id":1,"title":"First Joe post"}]}}},{"data":{"company":{"name":"Company","contact":{"name":"Kate","posts":null}}}}]

Expected:

[{"data":{"contact":{"name":"Joe","posts":[{"id":1,"title":"First Joe post"}]}}},{"data":{"company":{"name":"Company","contact":{"name":"Kate","posts":[{"id":3,"title":"First Kate post"}]}}}}]

@grynchuk grynchuk force-pushed the fix-prefetch-in-batch-requests branch from 479c103 to 8c648a5 Compare March 8, 2024 13:06
Copy link
Collaborator

@oojacoboo oojacoboo 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 the PR @grynchuk! Everything looks pretty good - some CS fixes needed.

tests/Fixtures/Integration/Models/Company.php Outdated Show resolved Hide resolved
tests/Fixtures/Integration/Types/ContactType.php Outdated Show resolved Hide resolved
tests/Fixtures/Integration/Types/CompanyType.php Outdated Show resolved Hide resolved
tests/Fixtures/Integration/Types/ContactType.php Outdated Show resolved Hide resolved
tests/Fixtures/Integration/Types/PostType.php Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.29%. Comparing base (53f9d49) to head (5364cde).
Report is 68 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #658      +/-   ##
============================================
- Coverage     95.72%   95.29%   -0.44%     
- Complexity     1773     1818      +45     
============================================
  Files           154      171      +17     
  Lines          4586     4841     +255     
============================================
+ Hits           4390     4613     +223     
- Misses          196      228      +32     

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

@grynchuk grynchuk requested a review from oojacoboo March 11, 2024 08:18
@oojacoboo oojacoboo merged commit 256da31 into thecodingmachine:master Mar 11, 2024
8 checks passed
@grynchuk grynchuk deleted the fix-prefetch-in-batch-requests branch March 12, 2024 08:37
@andrew-demb
Copy link
Contributor

@oojacoboo These fixes merged for the 7.x version of graphqlite, but there's a major performance problem with 7.x which is blocked update for 7.x for the Symfony bundle - thecodingmachine/graphqlite-bundle#203 (comment)
And issue in graphqlite - #691

Would you approve PR with the same changes, but for 6.x branch of graphqlite (and tag an additional 6.x release after merge)?

@oojacoboo
Copy link
Collaborator

@andrew-demb maybe it's just a better idea to test the changes in class-finder as per the comments in #691 and get that resolved, instead of just back patching, to prevent resolving the actual issue, assuming that you're even seeing any degraded performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants