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): do not cause stack overflow over circular refs #19802

Merged
merged 3 commits into from
Nov 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions packages/gatsby/src/db/sanitize-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,25 @@ const _ = require(`lodash`)
/**
* Make data serializable
* @param {(Object|Array)} data to sanitize
* @param {boolean} isNode = true
* @param {Set<string>} path = new Set
*/
const sanitizeNode = (data, isNode = true) => {
const sanitizeNode = (data, isNode = true, path = new Set()) => {
const isPlainObject = _.isPlainObject(data)

if (isPlainObject || _.isArray(data)) {
path.add(data)

const returnData = isPlainObject ? {} : []
let anyFieldChanged = false
_.each(data, (o, key) => {
if (path.has(o)) return

if (isNode && key === `internal`) {
returnData[key] = o
return
}
returnData[key] = sanitizeNode(o, false)
returnData[key] = sanitizeNode(o, false, path)

if (returnData[key] !== o) {
anyFieldChanged = true
Expand All @@ -26,13 +32,16 @@ const sanitizeNode = (data, isNode = true) => {
data = omitUndefined(returnData)
}

path.add(data)

// arrays and plain objects are supported - no need to to sanitize
return data
}

if (!isTypeSupported(data)) {
return undefined
} else {
path.add(data)
return data
}
}
Expand Down
93 changes: 93 additions & 0 deletions packages/gatsby/src/schema/__tests__/node-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -691,4 +691,97 @@ describe(`NodeModel`, () => {
})
})
})

describe(`circular references`, () => {
describe(`directly on a node`, () => {
beforeEach(async () => {
// This tests whether addRootNodeToInlineObject properly prevents re-traversing the same key-value pair infinitely
const circular = { i_am: `recursion!` }
circular.circled = circular

const node = {
id: `circleId`,
parent: null,
children: [],
inlineObject: {
field: `fieldOfFirstNode`,
},
inlineArray: [1, 2, 3],
circular,
internal: {
type: `Test`,
contentDigest: `digest1`,
},
}
actions.createNode(node, { name: `test` })(store.dispatch)

await build({})
const {
schemaCustomization: { composer: schemaComposer },
} = store.getState()
schema = store.getState().schema

nodeModel = new LocalNodeModel({
schema,
schemaComposer,
nodeStore,
createPageDependency,
})
})

it(`trackInlineObjectsInRootNode should not infinitely loop on a circular reference`, () => {
const node = nodeModel.getAllNodes({ type: `Test` })[0]
const copiedInlineObject = { ...node.inlineObject }
nodeModel.trackInlineObjectsInRootNode(copiedInlineObject)

expect(nodeModel._trackedRootNodes instanceof Set).toBe(true)
expect(nodeModel._trackedRootNodes.has(node.id)).toEqual(true)
})
})
describe(`not directly on a node`, () => {
beforeEach(async () => {
// This tests whether addRootNodeToInlineObject properly prevents re-traversing the same key-value pair infinitely
const circular = { i_am: `recursion!` }
circular.circled = { bar: { circular } }

const node = {
id: `circleId`,
parent: null,
children: [],
inlineObject: {
field: `fieldOfFirstNode`,
},
inlineArray: [1, 2, 3],
foo: { circular },
internal: {
type: `Test`,
contentDigest: `digest1`,
},
}
actions.createNode(node, { name: `test` })(store.dispatch)

await build({})
const {
schemaCustomization: { composer: schemaComposer },
} = store.getState()
schema = store.getState().schema

nodeModel = new LocalNodeModel({
schema,
schemaComposer,
nodeStore,
createPageDependency,
})
})

it(`trackInlineObjectsInRootNode should not infinitely loop on a circular reference`, () => {
const node = nodeModel.getAllNodes({ type: `Test` })[0]
const copiedInlineObject = { ...node.inlineObject }
nodeModel.trackInlineObjectsInRootNode(copiedInlineObject)

expect(nodeModel._trackedRootNodes instanceof Set).toBe(true)
expect(nodeModel._trackedRootNodes.has(node.id)).toEqual(true)
})
})
})
})
90 changes: 64 additions & 26 deletions packages/gatsby/src/schema/infer/inference-metadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,19 +113,47 @@ const getType = (value, key) => {
}
}

const updateValueDescriptor = ({
nodeId,
key,
value,
operation = `add`,
descriptor = {},
}) => {
const updateValueDescriptor = (
{ nodeId, key, value, operation = `add` /*: add | del*/, descriptor = {} },
path = []
Copy link
Contributor

Choose a reason for hiding this comment

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

other places use Set for keeping track if already visited things, was there special reason to use array here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I benchmarked it, this seemed more efficient (although it could be variation). Perhaps because this is an explicit push/pop stack, where the other is arbitrary push.

) => {
// The object may be traversed multiple times from root.
// Each time it does it should not revisit the same node twice
if (path.includes(value)) {
return [descriptor, false]
}

const typeName = getType(value, key)

if (typeName === `null`) {
return [descriptor, false]
}

path.push(value)

const ret = _updateValueDescriptor(
nodeId,
key,
value,
operation,
descriptor,
path,
typeName
)

path.pop()

return ret
}
const _updateValueDescriptor = (
nodeId,
key,
value,
operation,
descriptor,
path,
typeName
) => {
const delta = operation === `del` ? -1 : 1
const typeInfo = descriptor[typeName] || { total: 0 }
typeInfo.total += delta
Expand All @@ -138,25 +166,31 @@ const updateValueDescriptor = ({
// Keeping track of the first node for this type. Only used for better conflict reporting.
// (see Caveats section in the header comments)
if (operation === `add`) {
typeInfo.first = typeInfo.first || nodeId
if (!typeInfo.first) {
typeInfo.first = nodeId
}
} else if (operation === `del`) {
typeInfo.first =
typeInfo.first === nodeId || typeInfo.total === 0
? undefined
: typeInfo.first
if (typeInfo.first === nodeId || typeInfo.total === 0) {
typeInfo.first = undefined
}
}

switch (typeName) {
case `object`: {
const { props = {} } = typeInfo
Object.keys(value).forEach(key => {
const [propDescriptor, propDirty] = updateValueDescriptor({
nodeId,
key,
value: value[key],
operation,
descriptor: props[key],
})
const v = value[key]

const [propDescriptor, propDirty] = updateValueDescriptor(
{
nodeId,
key,
value: v,
operation,
descriptor: props[key],
},
path
)
props[key] = propDescriptor
dirty = dirty || propDirty
})
Expand All @@ -165,13 +199,17 @@ const updateValueDescriptor = ({
}
case `array`: {
value.forEach(item => {
const [itemDescriptor, itemDirty] = updateValueDescriptor({
nodeId,
descriptor: typeInfo.item,
operation,
value: item,
key,
})
const [itemDescriptor, itemDirty] = updateValueDescriptor(
{
nodeId,
descriptor: typeInfo.item,
operation,
value: item,
key,
},
path
)

typeInfo.item = itemDescriptor
dirty = dirty || itemDirty
})
Expand Down
7 changes: 6 additions & 1 deletion packages/gatsby/src/schema/node-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -703,16 +703,21 @@ const addRootNodeToInlineObject = (
rootNodeMap,
data,
nodeId,
isNode = false
isNode = false,
path = new Set()
) => {
path.add(data)

const isPlainObject = _.isPlainObject(data)

if (isPlainObject || _.isArray(data)) {
_.each(data, (o, key) => {
if (path.has(o)) return
if (!isNode || key !== `internal`) {
addRootNodeToInlineObject(rootNodeMap, o, nodeId)
}
})

// don't need to track node itself
if (!isNode) {
rootNodeMap.set(data, nodeId)
Expand Down