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(gatsby): fix potentially wrong query results when querying fields with custom resolvers #35369

Merged

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Apr 7, 2022

Description

This fixes quite a bit od edge cases that can cause wrong query results when using "materialized fields" (fields that have custom resolvers) for sorting/filtering/grouping/aggregating:

  1. Ensure that concurrent materialization runs don't overwrite each other results - this is done by merging already existing fields in resolved-nodes reducer (that's source of truth for getting "computed" values for sorting/filtering/grouping/aggregating on fields that need to be resolved).
  2. Don't attach resolved fields on node objects (don't set node.__gatsby_resolved). With concurret runs (point above) it's not safe to rely on __gatsby_resolved being set (as it might have been set for different fields). Instead we do check if the fields should be resolved or raw and pull resolved fields straight from source of truth (redux store) or use raw fields from node (usage in fast filters and in aggregation methods)

Related Issues

Fixes #35366
[ch51808]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Apr 7, 2022
@pieh pieh added topic: GraphQL Related to Gatsby's GraphQL layer and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Apr 7, 2022
@pieh pieh force-pushed the fix/materialization/concurrent-runs-losing-resolved-fields branch from bf2b490 to 04ad19e Compare June 13, 2022 12:34
@pieh pieh force-pushed the fix/materialization/concurrent-runs-losing-resolved-fields branch from d19283a to b85ca7d Compare June 17, 2022 08:45
@pieh pieh marked this pull request as ready for review June 20, 2022 06:52
wardpeet
wardpeet previously approved these changes Jun 20, 2022
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Small question but looks good

Co-authored-by: Ward Peeters <ward@coding-tech.com>
@LekoArts LekoArts merged commit 69824f6 into master Jun 28, 2022
@LekoArts LekoArts deleted the fix/materialization/concurrent-runs-losing-resolved-fields branch June 28, 2022 06:55
@tyhopp tyhopp added this to To cherry-pick in V4 Release hotfixes via automation Jun 28, 2022
tyhopp pushed a commit that referenced this pull request Jun 29, 2022
… with custom resolvers (#35369)

Co-authored-by: Ward Peeters <ward@coding-tech.com>
(cherry picked from commit 69824f6)
tyhopp pushed a commit that referenced this pull request Jun 29, 2022
… with custom resolvers (#35369) (#36003)

Co-authored-by: Ward Peeters <ward@coding-tech.com>
(cherry picked from commit 69824f6)

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
@tyhopp tyhopp moved this from To cherry-pick to Backported in V4 Release hotfixes Jun 29, 2022
@tyhopp tyhopp moved this from Backported to Published in V4 Release hotfixes Jun 29, 2022
@tyhopp
Copy link
Contributor

tyhopp commented Jun 29, 2022

Published in gatsby@4.17.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: GraphQL Related to Gatsby's GraphQL layer
Projects
Development

Successfully merging this pull request may close these issues.

Regression in #35271
4 participants