-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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): use lmdb.removeSync so getNode can't return deleted nodes #33554
Changes from 4 commits
2c173f1
2c760ca
02321d9
23b2620
be7ded7
28c4c3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,6 @@ | ||||||||||||||||||||||||
import { RootDatabase, open } from "lmdb-store" | ||||||||||||||||||||||||
import { RootDatabase, open, ArrayLikeIterable } from "lmdb-store" | ||||||||||||||||||||||||
// import { performance } from "perf_hooks" | ||||||||||||||||||||||||
import { ActionsUnion, IGatsbyNode } from "../../redux/types" | ||||||||||||||||||||||||
import { ActionsUnion, IDeleteNodeAction, IGatsbyNode } from "../../redux/types" | ||||||||||||||||||||||||
import { updateNodes } from "./updates/nodes" | ||||||||||||||||||||||||
import { updateNodesByType } from "./updates/nodes-by-type" | ||||||||||||||||||||||||
import { IDataStore, ILmdbDatabases, IQueryResult } from "../types" | ||||||||||||||||||||||||
|
@@ -27,6 +27,8 @@ const lmdbDatastore = { | |||||||||||||||||||||||
getNodesByType, | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const preSyncDeletedNodeIdsCache = new Set() | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
function getDefaultDbPath(): string { | ||||||||||||||||||||||||
const dbFileName = | ||||||||||||||||||||||||
process.env.NODE_ENV === `test` | ||||||||||||||||||||||||
|
@@ -122,10 +124,8 @@ function iterateNodes(): GatsbyIterable<IGatsbyNode> { | |||||||||||||||||||||||
return new GatsbyIterable( | ||||||||||||||||||||||||
nodesDb | ||||||||||||||||||||||||
.getKeys({ snapshot: false }) | ||||||||||||||||||||||||
.map( | ||||||||||||||||||||||||
nodeId => (typeof nodeId === `string` ? getNode(nodeId) : undefined)! | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
.filter(Boolean) | ||||||||||||||||||||||||
.map(nodeId => (typeof nodeId === `string` ? getNode(nodeId) : undefined)) | ||||||||||||||||||||||||
.filter(Boolean) as ArrayLikeIterable<IGatsbyNode> | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
@@ -134,13 +134,16 @@ function iterateNodesByType(type: string): GatsbyIterable<IGatsbyNode> { | |||||||||||||||||||||||
return new GatsbyIterable( | ||||||||||||||||||||||||
nodesByType | ||||||||||||||||||||||||
.getValues(type) | ||||||||||||||||||||||||
.map(nodeId => getNode(nodeId)!) | ||||||||||||||||||||||||
.filter(Boolean) | ||||||||||||||||||||||||
.map(nodeId => getNode(nodeId)) | ||||||||||||||||||||||||
.filter(Boolean) as ArrayLikeIterable<IGatsbyNode> | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
function getNode(id: string): IGatsbyNode | undefined { | ||||||||||||||||||||||||
if (!id) return undefined | ||||||||||||||||||||||||
if (!id || preSyncDeletedNodeIdsCache.has(id)) { | ||||||||||||||||||||||||
return undefined | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const { nodes } = getDatabases() | ||||||||||||||||||||||||
return nodes.get(id) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
@@ -151,9 +154,11 @@ function getTypes(): Array<string> { | |||||||||||||||||||||||
|
||||||||||||||||||||||||
function countNodes(typeName?: string): number { | ||||||||||||||||||||||||
if (!typeName) { | ||||||||||||||||||||||||
const stats = getDatabases().nodes.getStats() | ||||||||||||||||||||||||
// @ts-ignore | ||||||||||||||||||||||||
return Number(stats.entryCount || 0) // FIXME: add -1 when restoring shared structures key | ||||||||||||||||||||||||
const stats = getDatabases().nodes.getStats() as { entryCount: number } | ||||||||||||||||||||||||
return Math.max( | ||||||||||||||||||||||||
Number(stats.entryCount) - preSyncDeletedNodeIdsCache.size, | ||||||||||||||||||||||||
0 | ||||||||||||||||||||||||
) // FIXME: add -1 when restoring shared structures key | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const { nodesByType } = getDatabases() | ||||||||||||||||||||||||
|
@@ -192,15 +197,33 @@ function updateDataStore(action: ActionsUnion): void { | |||||||||||||||||||||||
break | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
case `CREATE_NODE`: | ||||||||||||||||||||||||
case `DELETE_NODE`: | ||||||||||||||||||||||||
case `ADD_FIELD_TO_NODE`: | ||||||||||||||||||||||||
case `ADD_CHILD_NODE_TO_PARENT_NODE`: | ||||||||||||||||||||||||
case `DELETE_NODE`: | ||||||||||||||||||||||||
case `MATERIALIZE_PAGE_MODE`: { | ||||||||||||||||||||||||
const dbs = getDatabases() | ||||||||||||||||||||||||
lastOperationPromise = Promise.all([ | ||||||||||||||||||||||||
const operationPromise = Promise.all([ | ||||||||||||||||||||||||
updateNodes(dbs.nodes, action), | ||||||||||||||||||||||||
updateNodesByType(dbs.nodesByType, action), | ||||||||||||||||||||||||
]) | ||||||||||||||||||||||||
lastOperationPromise = operationPromise | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// if create is used in the same transaction as delete we should remove it from cache | ||||||||||||||||||||||||
if (action.type === `CREATE_NODE`) { | ||||||||||||||||||||||||
preSyncDeletedNodeIdsCache.delete(action.payload.id) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
if (action.type === `DELETE_NODE`) { | ||||||||||||||||||||||||
preSyncDeletedNodeIdsCache.add( | ||||||||||||||||||||||||
((action as IDeleteNodeAction).payload as IGatsbyNode).id | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://app.circleci.com/pipelines/github/gatsbyjs/gatsby/76288/workflows/e843318b-e1c5-4cdf-b155-b82bf562e44e/jobs/898140 shows cases where
so we should check if gatsby/packages/gatsby/src/datastore/lmdb/updates/nodes.ts Lines 16 to 21 in 0f421db
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also casting gatsby/packages/gatsby/src/redux/types.ts Lines 836 to 840 in 0f421db
I really think we should avoid casts like this. In fact none of casts in this line are needed because after checking for type |
||||||||||||||||||||||||
) | ||||||||||||||||||||||||
operationPromise.then(() => { | ||||||||||||||||||||||||
// only clear if no other operations have been done in the meantime | ||||||||||||||||||||||||
if (lastOperationPromise === operationPromise) { | ||||||||||||||||||||||||
preSyncDeletedNodeIdsCache.clear() | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
}) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍