Skip to content

Commit

Permalink
fix(gatsby): fix incorrect intersection of filtered results (#30594)
Browse files Browse the repository at this point in the history
* 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)
  • Loading branch information
vladar authored and pieh committed Apr 1, 2021
1 parent 322e550 commit 0e7dc96
Show file tree
Hide file tree
Showing 8 changed files with 177 additions and 34 deletions.
79 changes: 52 additions & 27 deletions integration-tests/artifacts/__tests__/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,31 @@ const publicDir = path.join(process.cwd(), `public`)

const gatsbyBin = path.join(`node_modules`, `.bin`, `gatsby`)

const manifest = {}

function runGatsbyWithRunTestSetup(runNumber = 1) {
return function beforeAllImpl() {
return new Promise(resolve => {
const gatsbyProcess = spawn(gatsbyBin, [`build`], {
stdio: [`inherit`, `inherit`, `inherit`, `inherit`],
env: {
...process.env,
NODE_ENV: `production`,
ARTIFACTS_RUN_SETUP: runNumber.toString(),
},
})

gatsbyProcess.on(`exit`, () => {
manifest[runNumber] = fs.readJSONSync(
path.join(process.cwd(), `.cache`, `build-manifest-for-test-1.json`)
)

resolve()
})
})
}
}

const titleQuery = `
{
site {
Expand Down Expand Up @@ -90,6 +115,24 @@ function assertFileExistenceForPagePaths({ pagePaths, type, shouldExist }) {
)
}

function assertNodeCorrectness(runNumber) {
describe(`node correctness`, () => {
it(`nodes do not have repeating counters`, () => {
const seenCounters = new Map()
const duplicates = []
// Just a convenience step to display node ids with duplicate counters
manifest[runNumber].allNodeCounters.forEach(([id, counter]) => {
if (seenCounters.has(counter)) {
duplicates.push({ counter, nodeIds: [id, seenCounters.get(counter)] })
}
seenCounters.set(counter, id)
})
expect(manifest[runNumber].allNodeCounters.length).toBeGreaterThan(0)
expect(duplicates).toEqual([])
})
})
}

beforeAll(async done => {
const gatsbyCleanProcess = spawn(gatsbyBin, [`clean`], {
stdio: [`inherit`, `inherit`, `inherit`, `inherit`],
Expand All @@ -105,20 +148,9 @@ beforeAll(async done => {
})

describe(`First run`, () => {
beforeAll(async done => {
const gatsbyProcess = spawn(gatsbyBin, [`build`], {
stdio: [`inherit`, `inherit`, `inherit`, `inherit`],
env: {
...process.env,
NODE_ENV: `production`,
RUN_FOR_STALE_PAGE_ARTIFICATS: `1`,
},
})
const runNumber = 1

gatsbyProcess.on(`exit`, exitCode => {
done()
})
})
beforeAll(runGatsbyWithRunTestSetup(runNumber))

describe(`Static Queries`, () => {
test(`are written correctly when inline`, async () => {
Expand Down Expand Up @@ -272,26 +304,17 @@ describe(`First run`, () => {
})
})
})

assertNodeCorrectness(runNumber)
})

describe(`Second run`, () => {
const runNumber = 2

const expectedPages = [`stale-pages/stable`, `stale-pages/only-in-second`]
const unexpectedPages = [`stale-pages/only-in-first`]

beforeAll(async done => {
const gatsbyProcess = spawn(gatsbyBin, [`build`], {
stdio: [`inherit`, `inherit`, `inherit`, `inherit`],
env: {
...process.env,
NODE_ENV: `production`,
RUN_FOR_STALE_PAGE_ARTIFICATS: `2`,
},
})

gatsbyProcess.on(`exit`, exitCode => {
done()
})
})
beforeAll(runGatsbyWithRunTestSetup(runNumber))

describe(`html files`, () => {
const type = `html`
Expand Down Expand Up @@ -332,4 +355,6 @@ describe(`Second run`, () => {
})
})
})

assertNodeCorrectness(runNumber)
})
50 changes: 49 additions & 1 deletion integration-tests/artifacts/gatsby-node.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,36 @@
const isFirstRun = process.env.RUN_FOR_STALE_PAGE_ARTIFICATS !== `2`
const path = require(`path`)
const fs = require(`fs-extra`)

const runNumber = parseInt(process.env.ARTIFACTS_RUN_SETUP, 10) || 1

const isFirstRun = runNumber === 1

exports.sourceNodes = ({ actions, createContentDigest, reporter, getNode }) => {
reporter.info(`Using test setup #${runNumber}`)

function createNodeHelper(type, nodePartial) {
const node = {
template: `default`,
...nodePartial,
internal: {
type,
contentDigest: createContentDigest(nodePartial),
},
}
actions.createNode(node)
}

for (let prevRun = 1; prevRun < runNumber; prevRun++) {
const node = getNode(`node-created-in-run-${prevRun}`)
if (node) {
actions.touchNode(node)
}
}
createNodeHelper(`NodeCounterTest`, {
id: `node-created-in-run-${runNumber}`,
label: `Node created in run ${runNumber}`,
})
}

exports.createPages = ({ actions }) => {
function createPageHelper(dummyId) {
Expand All @@ -22,3 +54,19 @@ exports.createPages = ({ actions }) => {
createPageHelper(`only-in-second`)
}
}

let counter = 1
exports.onPostBuild = ({ getNodes }) => {
console.log(`[test] onPostBuild`)

fs.writeJSONSync(
path.join(
process.cwd(),
`.cache`,
`build-manifest-for-test-${counter++}.json`
),
{
allNodeCounters: getNodes().map(node => [node.id, node.internal.counter]),
}
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ Object {
"staticQueriesByTemplate": Map {},
"staticQueryComponents": Map {},
"status": Object {
"LAST_NODE_COUNTER": 0,
"PLUGINS_HASH": "",
"plugins": Object {},
},
Expand Down
52 changes: 52 additions & 0 deletions packages/gatsby/src/redux/__tests__/run-fast-filters.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const {
applyFastFilters,
} = require(`../run-fast-filters`)
const { store } = require(`../index`)
const { getNode } = require(`../nodes`)
const { createDbQueriesFromObject } = require(`../../db/common/query`)
const { actions } = require(`../actions`)
const {
Expand Down Expand Up @@ -459,3 +460,54 @@ describe(`applyFastFilters`, () => {
expect(result.length).toBe(2)
})
})

describe(`edge cases (yay)`, () => {
beforeAll(() => {
store.dispatch({ type: `DELETE_CACHE` })
mockNodes().forEach(node =>
actions.createNode(node, { name: `test` })(store.dispatch)
)
})

it(`throws when node counters are messed up`, () => {
const filter = {
slog: { $eq: `def` }, // matches id_2 and id_4
deep: { flat: { search: { chain: { $eq: 500 } } } }, // matches id_2
}

const result = applyFastFilters(
createDbQueriesFromObject(filter),
[typeName],
new Map()
)

// Sanity-check
expect(result.length).toEqual(1)
expect(result[0].id).toEqual(`id_2`)

// After process restart node.internal.counter is reset and conflicts with counters from the previous run
// in some situations this leads to incorrect intersection of filtered results.
// Below we set node.internal.counter to same value that existing node id_4 has and leads
// to bad intersection of filtered results
const badNode = {
id: `bad-node`,
deep: { flat: { search: { chain: 500 } } },
internal: {
type: typeName,
contentDigest: `bad-node`,
counter: getNode(`id_4`).internal.counter,
},
}
store.dispatch({
type: `CREATE_NODE`,
payload: badNode,
})

const run = () =>
applyFastFilters(createDbQueriesFromObject(filter), [typeName], new Map())

expect(run).toThrow(
`Invariant violation: inconsistent node counters detected`
)
})
})
19 changes: 13 additions & 6 deletions packages/gatsby/src/redux/actions/public.js
Original file line number Diff line number Diff line change
Expand Up @@ -531,9 +531,17 @@ actions.deleteNodes = (nodes: any[], plugin: Plugin) => {
return deleteNodesAction
}

// We add a counter to internal to make sure we maintain insertion order for
// backends that don't do that out of the box
let NODE_COUNTER = 0
// We add a counter to node.internal for fast comparisons/intersections
// of various node slices. The counter must increase even across builds.
function getNextNodeCounter() {
const lastNodeCounter = store.getState().status.LAST_NODE_COUNTER ?? 0
if (lastNodeCounter >= Number.MAX_SAFE_INTEGER) {
throw new Error(
`Could not create more nodes. Maximum node count is reached: ${lastNodeCounter}`
)
}
return lastNodeCounter + 1
}

const typeOwners = {}

Expand Down Expand Up @@ -633,9 +641,6 @@ const createNode = (
node.internal = {}
}

NODE_COUNTER++
node.internal.counter = NODE_COUNTER

// Ensure the new node has a children array.
if (!node.array && !_.isArray(node.children)) {
node.children = []
Expand Down Expand Up @@ -793,6 +798,8 @@ const createNode = (
.map(createDeleteAction)
}

node.internal.counter = getNextNodeCounter()

updateNodeAction = {
...actionOptions,
type: `CREATE_NODE`,
Expand Down
5 changes: 5 additions & 0 deletions packages/gatsby/src/redux/nodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1104,6 +1104,11 @@ export function intersectNodesByCounter(
} else if (counterA > counterB) {
pointerB++
} else {
if (nodeA !== nodeB) {
throw new Error(
`Invariant violation: inconsistent node counters detected`
)
}
// 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
Expand Down
4 changes: 4 additions & 0 deletions packages/gatsby/src/redux/reducers/status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { ActionsUnion, IGatsbyState } from "../types"

const defaultState: IGatsbyState["status"] = {
PLUGINS_HASH: ``,
LAST_NODE_COUNTER: 0,
plugins: {},
}

Expand Down Expand Up @@ -42,6 +43,9 @@ export const statusReducer = (
),
},
}
case `CREATE_NODE`:
state.LAST_NODE_COUNTER = action.payload.internal.counter
return state
default:
return state
}
Expand Down
1 change: 1 addition & 0 deletions packages/gatsby/src/redux/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ export interface IGatsbyState {
status: {
plugins: Record<string, IGatsbyPlugin>
PLUGINS_HASH: Identifier
LAST_NODE_COUNTER: number
}
queries: {
byNode: Map<Identifier, Set<Identifier>>
Expand Down

0 comments on commit 0e7dc96

Please sign in to comment.