Skip to content

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
@pieh pieh self-assigned this Mar 31, 2021
Co-authored-by: Michal Piechowiak <[email protected]>
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 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 <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit e432c23)
pieh 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 <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit e432c23)
pieh 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 <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit e432c23)
pieh 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 <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit e432c23)
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 <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit e432c23)
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 <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit e432c23)

Co-authored-by: Vladimir Razuvaev <[email protected]>
pieh 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 <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit e432c23)
@vladar
Copy link
Contributor Author

vladar commented Apr 1, 2021

Published in [email protected]

pieh 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 <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>
(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 <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit e432c23)

Co-authored-by: Vladimir Razuvaev <[email protected]>
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 <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>
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

None yet

Development

Successfully merging this pull request may close these issues.

3 participants