thecodingmachine / graphqlite

Use PHP Attributes/Annotations to declare your GraphQL API
https://graphqlite.thecodingmachine.io
MIT License
554 stars 95 forks source link

Fix prefetching the same GQL type for batch request #658

Closed grynchuk closed 4 months ago

grynchuk commented 4 months ago

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"}]}}}}]
codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

andrew-demb commented 2 weeks ago

@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 - https://github.com/thecodingmachine/graphqlite-bundle/pull/203#issuecomment-2201182492 And issue in graphqlite - https://github.com/thecodingmachine/graphqlite/issues/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 commented 2 weeks ago

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