From 66e7d4b1b13e3fa89681d196e05b0c3861479e4f Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Thu, 1 Apr 2021 20:19:51 +0700 Subject: [PATCH] fix(gatsby): fix incorrect intersection of filtered results (#30594) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * only newly created nodes should get a counter * tweak comment Co-authored-by: Michal Piechowiak Co-authored-by: Michal Piechowiak (cherry picked from commit e432c231eb65e66208ab29605aa670e6c873303f) --- .../artifacts/__tests__/index.js | 81 ++++++++++++------- integration-tests/artifacts/gatsby-node.js | 50 +++++++++++- .../__tests__/__snapshots__/index.js.snap | 1 + .../src/redux/__tests__/run-fast-filters.js | 52 ++++++++++++ packages/gatsby/src/redux/actions/public.js | 19 +++-- packages/gatsby/src/redux/nodes.ts | 5 ++ packages/gatsby/src/redux/reducers/status.ts | 4 + packages/gatsby/src/redux/types.ts | 1 + 8 files changed, 178 insertions(+), 35 deletions(-) diff --git a/integration-tests/artifacts/__tests__/index.js b/integration-tests/artifacts/__tests__/index.js index 7dc2bfc03e6b5..d484f9c22921a 100644 --- a/integration-tests/artifacts/__tests__/index.js +++ b/integration-tests/artifacts/__tests__/index.js @@ -3,7 +3,7 @@ const path = require(`path`) const { murmurhash } = require(`babel-plugin-remove-graphql-queries`) const { readPageData } = require(`gatsby/dist/utils/page-data`) const { stripIgnoredCharacters } = require(`gatsby/graphql`) -const fs = require(`fs`) +const fs = require(`fs-extra`) jest.setTimeout(100000) @@ -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 { @@ -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`], @@ -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 () => { @@ -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` @@ -332,4 +355,6 @@ describe(`Second run`, () => { }) }) }) + + assertNodeCorrectness(runNumber) }) diff --git a/integration-tests/artifacts/gatsby-node.js b/integration-tests/artifacts/gatsby-node.js index b444e095ef492..48f5b61bdab24 100644 --- a/integration-tests/artifacts/gatsby-node.js +++ b/integration-tests/artifacts/gatsby-node.js @@ -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) { @@ -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]), + } + ) +} diff --git a/packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap b/packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap index 04a263a7d7c4f..0dc18a4c85df8 100644 --- a/packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap +++ b/packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap @@ -79,6 +79,7 @@ Object { "staticQueriesByTemplate": Map {}, "staticQueryComponents": Map {}, "status": Object { + "LAST_NODE_COUNTER": 0, "PLUGINS_HASH": "", "plugins": Object {}, }, diff --git a/packages/gatsby/src/redux/__tests__/run-fast-filters.js b/packages/gatsby/src/redux/__tests__/run-fast-filters.js index 1581944bbab71..22d1703d21b87 100644 --- a/packages/gatsby/src/redux/__tests__/run-fast-filters.js +++ b/packages/gatsby/src/redux/__tests__/run-fast-filters.js @@ -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 { @@ -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` + ) + }) +}) diff --git a/packages/gatsby/src/redux/actions/public.js b/packages/gatsby/src/redux/actions/public.js index 4c8e0744c47e0..939feff370b31 100644 --- a/packages/gatsby/src/redux/actions/public.js +++ b/packages/gatsby/src/redux/actions/public.js @@ -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 = {} @@ -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 = [] @@ -793,6 +798,8 @@ const createNode = ( .map(createDeleteAction) } + node.internal.counter = getNextNodeCounter() + updateNodeAction = { ...actionOptions, type: `CREATE_NODE`, diff --git a/packages/gatsby/src/redux/nodes.ts b/packages/gatsby/src/redux/nodes.ts index d262b114d1332..cad0ac0585bc4 100644 --- a/packages/gatsby/src/redux/nodes.ts +++ b/packages/gatsby/src/redux/nodes.ts @@ -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 diff --git a/packages/gatsby/src/redux/reducers/status.ts b/packages/gatsby/src/redux/reducers/status.ts index 7913b60d40ea3..46b7222543898 100644 --- a/packages/gatsby/src/redux/reducers/status.ts +++ b/packages/gatsby/src/redux/reducers/status.ts @@ -3,6 +3,7 @@ import { ActionsUnion, IGatsbyState } from "../types" const defaultState: IGatsbyState["status"] = { PLUGINS_HASH: ``, + LAST_NODE_COUNTER: 0, plugins: {}, } @@ -42,6 +43,9 @@ export const statusReducer = ( ), }, } + case `CREATE_NODE`: + state.LAST_NODE_COUNTER = action.payload.internal.counter + return state default: return state } diff --git a/packages/gatsby/src/redux/types.ts b/packages/gatsby/src/redux/types.ts index 84119ec150ead..4f8433c218bab 100644 --- a/packages/gatsby/src/redux/types.ts +++ b/packages/gatsby/src/redux/types.ts @@ -208,6 +208,7 @@ export interface IGatsbyState { status: { plugins: Record PLUGINS_HASH: Identifier + LAST_NODE_COUNTER: number } queries: { byNode: Map>