-
Notifications
You must be signed in to change notification settings - Fork 1
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
Added support for handling Remote URL refs in the bundle method #747
Changes from 15 commits
77578d2
2359345
72bc9bc
41059e0
c9cbf14
8b6eb7b
8171baa
eb1f18c
10741f2
b8c928b
82ecc91
ac6f740
7cb2ba4
a8da616
f8cb8aa
142defa
88e461e
da3d42c
b0ffbb9
37003c1
67b9fe3
8b045f8
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,10 +1,14 @@ | ||
const _ = require('lodash'), | ||
{ | ||
isExtRef, | ||
isExtURLRef, | ||
stringIsAValidUrl, | ||
isExtRemoteRef, | ||
getKeyInComponents, | ||
getJsonPointerRelationToRoot, | ||
removeLocalReferenceFromPath, | ||
localPointer, | ||
httpSeparator, | ||
jsonPointerLevelSeparator, | ||
isLocalRef, | ||
jsonPointerDecodeAndReplace, | ||
|
@@ -83,15 +87,20 @@ | |
* @returns {object} - Detect root files result object | ||
*/ | ||
function findNodeFromPath(referencePath, allData) { | ||
const partialComponents = referencePath.split(localPointer); | ||
let isPartial = partialComponents.length > 1, | ||
node = allData.find((node) => { | ||
if (isPartial) { | ||
referencePath = partialComponents[0]; | ||
} | ||
return comparePaths(node.fileName, referencePath); | ||
}); | ||
const isReferenceRemoteURL = stringIsAValidUrl(referencePath), | ||
partialComponents = referencePath.split(localPointer), | ||
isPartial = partialComponents.length > 1; | ||
if (isPartial && !isReferenceRemoteURL) { | ||
referencePath = partialComponents.slice(0, partialComponents.length - 1).join('#'); | ||
} | ||
|
||
let node = allData.find((node) => { | ||
if (isReferenceRemoteURL) { | ||
return _.startsWith(node.path, referencePath); | ||
} | ||
|
||
return comparePaths(node.fileName, referencePath); | ||
}); | ||
return node; | ||
} | ||
|
||
|
@@ -290,13 +299,78 @@ | |
* @param {string} commonPathFromData - The common path in the file's paths | ||
* @param {Array} allData - array of { path, content} objects | ||
* @param {object} globalReferences - The accumulated global references from all nodes | ||
* @param {function} remoteRefResolver - The function that would be called to fetch remote ref contents | ||
* @returns {object} - The references in current node and the new content from the node | ||
*/ | ||
function getReferences (currentNode, isOutOfRoot, pathSolver, parentFilename, version, rootMainKeys, | ||
commonPathFromData, allData, globalReferences) { | ||
async function getReferences (currentNode, isOutOfRoot, pathSolver, parentFilename, version, rootMainKeys, | ||
commonPathFromData, allData, globalReferences, remoteRefResolver) { | ||
let referencesInNode = [], | ||
nodeReferenceDirectory = {}, | ||
mainKeys = {}; | ||
mainKeys = {}, | ||
remoteRefContentMap = new Map(), | ||
remoteRefSet = new Set(), | ||
remoteRefResolutionPromises = []; | ||
|
||
remoteRefResolver && traverseUtility(currentNode).forEach(function (property) { | ||
if (property) { | ||
let hasReferenceTypeKey; | ||
|
||
hasReferenceTypeKey = Object.keys(property) | ||
.find( | ||
(key) => { | ||
const isExternal = isExtURLRef(property, key), | ||
isReferenciable = isExternal; | ||
|
||
return isReferenciable; | ||
} | ||
); | ||
|
||
if (hasReferenceTypeKey) { | ||
const tempRef = calculatePath(parentFilename, property.$ref), | ||
isRefEncountered = remoteRefSet.has(tempRef); | ||
|
||
if (isRefEncountered) { | ||
return; | ||
} | ||
|
||
remoteRefResolutionPromises.push( | ||
new Promise(async (resolveInner) => { | ||
|
||
/** | ||
Check warning on line 339 in lib/bundle.js
|
||
* Converts contents received from remoteRefResolver into stringified JSON | ||
VShingala marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @param {string | object} content - contents from remoteRefResolver | ||
* @returns Stringified JSON contents | ||
*/ | ||
function convertToJSONString (content) { | ||
if (typeof content === 'object') { | ||
return JSON.stringify(content); | ||
} | ||
|
||
const parsedFile = parseFile(content); | ||
|
||
return JSON.stringify(parsedFile.oasObject); | ||
} | ||
|
||
let contentFromRemote = await remoteRefResolver(property.$ref), | ||
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. We should have some handling in cases where this may fail and throw error. we can either ignore and keep the field as is or mention it missing in bundled content somehow. But not handling it at all can cause issues. 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. Handled err scenario |
||
nodeTemp = { | ||
fileName: tempRef, | ||
path: tempRef, | ||
content: convertToJSONString(contentFromRemote) | ||
}; | ||
|
||
remoteRefContentMap.set(tempRef, contentFromRemote); | ||
|
||
allData.push(nodeTemp); | ||
resolveInner(); | ||
}) | ||
); | ||
|
||
remoteRefSet.add(tempRef); | ||
} | ||
} | ||
}); | ||
|
||
await Promise.all(remoteRefResolutionPromises); | ||
|
||
traverseUtility(currentNode).forEach(function (property) { | ||
if (property) { | ||
|
@@ -371,6 +445,87 @@ | |
referencesInNode.push({ path: pathSolver(property), keyInComponents: nodeTrace, newValue: this.node }); | ||
} | ||
} | ||
|
||
const hasRemoteReferenceTypeKey = Object.keys(property) | ||
.find( | ||
(key) => { | ||
const isExternal = isExtURLRef(property, key), | ||
|
||
// Only process URL refs if remoteRefResolver is provided and a valid function | ||
isReferenciable = isExternal && _.isFunction(remoteRefResolver); | ||
|
||
return isReferenciable; | ||
} | ||
); | ||
|
||
if (hasRemoteReferenceTypeKey) { | ||
const tempRef = calculatePath(parentFilename, property.$ref), | ||
nodeTrace = handleLocalCollisions( | ||
getTraceFromParentKeyInComponents(this, tempRef, mainKeys, version, commonPathFromData), | ||
rootMainKeys | ||
), | ||
componentKey = nodeTrace[nodeTrace.length - 1], | ||
referenceInDocument = getJsonPointerRelationToRoot( | ||
tempRef, | ||
nodeTrace, | ||
version | ||
), | ||
traceToParent = [...this.parents.map((item) => { | ||
return item.key; | ||
}).filter((item) => { | ||
return item !== undefined; | ||
}), this.key]; | ||
|
||
let newValue = Object.assign({}, this.node), | ||
[, local] = tempRef.split(localPointer), | ||
nodeFromData, | ||
refHasContent = false, | ||
parseResult, | ||
newRefInDoc, | ||
inline, | ||
contentFromRemote = remoteRefContentMap.get(tempRef), | ||
nodeTemp = { | ||
fileName: tempRef, | ||
path: tempRef, | ||
content: contentFromRemote | ||
}; | ||
|
||
nodeFromData = nodeTemp; | ||
|
||
if (nodeFromData && nodeFromData.content) { | ||
parseResult = parseFile(JSON.stringify(nodeFromData.content)); | ||
if (parseResult.result) { | ||
newValue.$ref = referenceInDocument; | ||
refHasContent = true; | ||
nodeFromData.parsed = parseResult; | ||
} | ||
} | ||
this.update({ $ref: tempRef }); | ||
|
||
if (nodeTrace.length === 0) { | ||
inline = true; | ||
} | ||
|
||
if (_.isNil(globalReferences[tempRef])) { | ||
nodeReferenceDirectory[tempRef] = { | ||
local, | ||
keyInComponents: nodeTrace, | ||
node: newValue, | ||
reference: inline ? newRefInDoc : referenceInDocument, | ||
traceToParent, | ||
parentNodeKey: parentFilename, | ||
mainKeyInTrace: nodeTrace[nodeTrace.length - 1], | ||
refHasContent, | ||
inline | ||
}; | ||
} | ||
|
||
mainKeys[componentKey] = tempRef; | ||
|
||
if (!added(property.$ref, referencesInNode)) { | ||
referencesInNode.push({ path: pathSolver(property), keyInComponents: nodeTrace, newValue: this.node }); | ||
} | ||
} | ||
} | ||
}); | ||
|
||
|
@@ -386,10 +541,11 @@ | |
* @param {object} rootMainKeys - A dictionary with the component keys in local components object and its mainKeys | ||
* @param {string} commonPathFromData - The common path in the file's paths | ||
* @param {object} globalReferences - The accumulated global refernces from all nodes | ||
* @param {function} remoteRefResolver - The function that would be called to fetch remote ref contents | ||
* @returns {object} - Detect root files result object | ||
*/ | ||
function getNodeContentAndReferences (currentNode, allData, specRoot, version, rootMainKeys, | ||
commonPathFromData, globalReferences) { | ||
async function getNodeContentAndReferences (currentNode, allData, specRoot, version, rootMainKeys, | ||
commonPathFromData, globalReferences, remoteRefResolver) { | ||
let graphAdj = [], | ||
missingNodes = [], | ||
nodeContent, | ||
|
@@ -406,7 +562,7 @@ | |
nodeContent = parseResult.oasObject; | ||
} | ||
|
||
const { referencesInNode, nodeReferenceDirectory } = getReferences( | ||
const { referencesInNode, nodeReferenceDirectory } = await getReferences( | ||
nodeContent, | ||
currentNode.fileName !== specRoot.fileName, | ||
removeLocalReferenceFromPath, | ||
|
@@ -415,7 +571,8 @@ | |
rootMainKeys, | ||
commonPathFromData, | ||
allData, | ||
globalReferences | ||
globalReferences, | ||
remoteRefResolver | ||
); | ||
|
||
referencesInNode.forEach((reference) => { | ||
|
@@ -516,9 +673,11 @@ | |
* @param {function} refTypeResolver - The resolver function to test if node has a reference | ||
* @param {object} components - The global components object | ||
* @param {string} version - The current version | ||
* @param {function} remoteRefResolver - The function that would be called to fetch remote ref contents | ||
* @returns {object} The components object related to the file | ||
*/ | ||
function generateComponentsObject (documentContext, rootContent, refTypeResolver, components, version) { | ||
function generateComponentsObject(documentContext, rootContent, | ||
refTypeResolver, components, version, remoteRefResolver) { | ||
let notInLine = Object.entries(documentContext.globalReferences).filter(([, value]) => { | ||
return value.keyInComponents.length !== 0; | ||
}), | ||
|
@@ -555,15 +714,39 @@ | |
isMissingNode = documentContext.missing.find((missingNode) => { | ||
return missingNode.path === nodeRef; | ||
}); | ||
|
||
if (isMissingNode) { | ||
refData.nodeContent = refData.node; | ||
refData.local = false; | ||
} | ||
else if (!refData) { | ||
return; | ||
} | ||
else if (isExtRef(property, '$ref') && !isExtURLRef(property, '$ref')) { | ||
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. Let's keep the existing condition logic for Reason behind it is as logic for HTTP is very specific and to handle future scenarios better as existing logic is more generic. |
||
refData.nodeContent = documentContext.nodeContents[nodeRef]; | ||
} | ||
else { | ||
let splitPathByHttp = property.$ref.split(httpSeparator), | ||
prefix = splitPathByHttp | ||
.slice(0, splitPathByHttp.length - 1).join(httpSeparator) + | ||
httpSeparator + splitPathByHttp[splitPathByHttp.length - 1] | ||
.split(localPointer)[0], | ||
separatedPaths = [prefix, splitPathByHttp[splitPathByHttp.length - 1].split(localPointer)[1]]; | ||
|
||
nodeRef = separatedPaths[0]; | ||
local = separatedPaths[1]; | ||
|
||
refData.nodeContent = documentContext.nodeContents[nodeRef]; | ||
|
||
const isReferenceRemoteURL = stringIsAValidUrl(nodeRef); | ||
|
||
if (isReferenceRemoteURL && _.isFunction(remoteRefResolver)) { | ||
Object.keys(documentContext.nodeContents).forEach((key) => { | ||
if (_.startsWith(key, nodeRef) && !key.split(nodeRef)[1].includes(httpSeparator)) { | ||
refData.nodeContent = documentContext.nodeContents[key]; | ||
} | ||
}); | ||
} | ||
} | ||
if (local) { | ||
let contentFromTrace = getContentFromTrace(refData.nodeContent, local); | ||
|
@@ -697,9 +880,10 @@ | |
* @param {Array} allData - array of { path, content} objects | ||
* @param {Array} origin - process origin (BROWSER or node) | ||
* @param {string} version - The version we are using | ||
* @param {function} remoteRefResolver - The function that would be called to fetch remote ref contents | ||
* @returns {object} - Detect root files result object | ||
*/ | ||
getBundleContentAndComponents: function (specRoot, allData, origin, version) { | ||
getBundleContentAndComponents: async function (specRoot, allData, origin, version, remoteRefResolver) { | ||
if (origin === BROWSER) { | ||
path = pathBrowserify; | ||
} | ||
|
@@ -716,15 +900,16 @@ | |
commonPathFromData = Utils.findCommonSubpath(allData.map((fileData) => { | ||
return fileData.fileName; | ||
})); | ||
rootContextData = algorithm.traverseAndBundle(specRoot, (currentNode, globalReferences) => { | ||
rootContextData = await algorithm.traverseAndBundle(specRoot, (currentNode, globalReferences) => { | ||
return getNodeContentAndReferences( | ||
currentNode, | ||
allData, | ||
specRoot, | ||
version, | ||
initialMainKeys, | ||
commonPathFromData, | ||
globalReferences | ||
globalReferences, | ||
remoteRefResolver | ||
); | ||
}); | ||
components = generateComponentsWrapper( | ||
|
@@ -735,10 +920,12 @@ | |
finalElements = generateComponentsObject( | ||
rootContextData, | ||
rootContextData.nodeContents[specRoot.fileName], | ||
isExtRef, | ||
isExtRemoteRef, | ||
components, | ||
version | ||
version, | ||
remoteRefResolver | ||
); | ||
|
||
return { | ||
fileContent: finalElements.resRoot, | ||
components: finalElements.newComponents, | ||
|
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.
Can we add a comment in the code on why this is needed with an example if possible? I'm not getting the need of joining it with
#
compared to the previous logic, as we're changing this code that's applicable for non-remote refs.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.
@VShingala had done this refactor to test out some deep ref scenarios, but this check is just a more generic check in case we encounter reference paths that contain multiple hash fragments, but turns out we don't need this for any scenario so reverting this back to the original check