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 incorrect intersection of filtered results #30594

Merged
merged 12 commits into from Apr 1, 2021

Conversation

vladar
Copy link
Contributor

@vladar vladar commented Mar 31, 2021

Description

This PR fixes a bug with query filters that occurs in a very edge case. All of the following must be true in order for you to hit this bug:

  1. You must have at least one source plugin that relies on touchNode (such as gatsby-source-contentful, gatsby-source-contentstack, etc)
  2. You must use multiple filters in a single query
  3. Filtered results must have an intersection
  4. You must start the build/develop with the warm cache
  5. The stars must converge in a certain way

If all of the above is true - you will see this issue.

The problem is that we rely on node.internal.counter to perform fast intersections of filtered results here:

const counterA = nodeA.internal.counter
const counterB = nodeB.internal.counter
if (counterA < counterB) {
pointerA++
} else if (counterA > counterB) {
pointerB++
} else {
// nodeA===nodeB. Make sure we didn't just add this node already.
// Since input arrays are sorted, the same node should be grouped
// back to back, so even if both input arrays contained the same node
// twice, this check would prevent the result from getting duplicate nodes
if (lastAdded !== nodeA) {
result.push(nodeA)
lastAdded = nodeA
}
pointerA++
pointerB++
}

And this node.internal.counter is reset when we restart the gatsby process:

NODE_COUNTER++
node.internal.counter = NODE_COUNTER

So after process restart, multiple nodes may end up with the same counter and mess with filter results intersection.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Mar 31, 2021
@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 Mar 31, 2021
@vladar vladar marked this pull request as ready for review March 31, 2021 20:53
@vladar vladar added this to To cherry-pick in V2 Release hotfixes via automation Mar 31, 2021
@vladar vladar added this to To cherry-pick in V3 Release Hotfixes via automation Mar 31, 2021
@pieh pieh self-assigned this Mar 31, 2021
Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much for diving in this problem, reverse engineering how problem could occur and finding fix for it!

@vladar vladar merged commit e432c23 into master Apr 1, 2021
@vladar vladar deleted the vladar/fix-broken-filters branch April 1, 2021 13:19
pieh pushed a commit that referenced this pull request Apr 1, 2021
* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit e432c23)
pieh pushed a commit that referenced this pull request Apr 1, 2021
* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit e432c23)
pieh pushed a commit that referenced this pull request Apr 1, 2021
* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit e432c23)
pieh pushed a commit that referenced this pull request Apr 1, 2021
* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit e432c23)
@pieh pieh moved this from To cherry-pick to Backport PR opened in V2 Release hotfixes Apr 1, 2021
vladar added a commit that referenced this pull request Apr 1, 2021
* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit e432c23)
@vladar vladar moved this from To cherry-pick to Backport PR opened in V3 Release Hotfixes Apr 1, 2021
vladar added a commit that referenced this pull request Apr 1, 2021
…30626)

* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit e432c23)

Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>
pieh pushed a commit that referenced this pull request Apr 1, 2021
* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit e432c23)
@vladar vladar moved this from Backport PR opened to Backported in V3 Release Hotfixes Apr 1, 2021
@vladar
Copy link
Contributor Author

vladar commented Apr 1, 2021

Published in gatsby@3.2.1

pieh pushed a commit that referenced this pull request Apr 1, 2021
* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit e432c23)
pieh pushed a commit that referenced this pull request Apr 1, 2021
…30619)

* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit e432c23)

Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>
@pieh pieh moved this from Backport PR opened to Backported in V2 Release hotfixes Apr 1, 2021
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
…#30594)

* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
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
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants