Added support for handling Remote URL refs in the bundle method #747
Conversation
…nto feature/add-support-for-remote-url-refs-first-draft
lib/bundle.js
Outdated
|
|
||
| let isPartial = partialComponents.length > 1; | ||
| if (isPartial && !isReferenceRemoteURL) { | ||
| referencePath = partialComponents.slice(0, partialComponents.length - 1).join('#'); |
There was a problem hiding this comment.
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.
@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
lib/bundle.js
Outdated
| return JSON.stringify(parsedFile.oasObject); | ||
| } | ||
|
|
||
| let contentFromRemote = await remoteRefResolver(property.$ref), |
There was a problem hiding this comment.
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.
Handled err scenario
lib/bundle.js
Outdated
| 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.
Let's keep the existing condition logic for else statement as is and use (!isExtRef(property, '$ref') && isExtURLRef(property, '$ref') as condition and add new logic here.
Reason behind it is as logic for HTTP is very specific and to handle future scenarios better as existing logic is more generic.
| expect(res.result).to.be.true; | ||
| expect(res.output.specification.version).to.equal('3.0'); | ||
| expect(JSON.stringify(JSON.parse(res.output.data[0].bundledContent), null, 2)).to.be.equal(expected); | ||
| }); |
There was a problem hiding this comment.
Let's add few more tests to make sure the below scenarios also works.
- Circular refs (A -> B -> C -> A). We should have this handled and circular ref should not be resolved to avoid obvious further issues.
- If remoteRefResolver throws error, depending upon our handling expect the result instead entire function throwing error.
- Should return bundled file as yaml with yaml remote refs
- Should return bundled file as yaml with json content remote refs - remote_url_refs
|
@prashantRaghu Let's also add a changelog for this feature. We can add it under Here's sample PR and change done in changelog for reference: #732 |
|
@VShingala Added changelog, please verify if its done correctly |
| * For nodes that are fetched for remote URLs we ensure they | ||
| * aren't visited more than once | ||
| */ | ||
| (!node.href || (!hrefsVisited.has(node.href))) |
There was a problem hiding this comment.
In which cases !node.href can be true? i.e. if node.href is empty, in subsequent logic line no. 54 here, we're adding it to hrefsVisited. Not sure if this is intended.
There was a problem hiding this comment.
!node.href would be true for all non remote reference nodes, I'll add an extra check at line 54
CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Added support for resolving of remote URL refs in bundle method | |||
There was a problem hiding this comment.
Added support for remote $ref resolution in bundle() API.
More details here: https://postmanlabs.atlassian.net/wiki/spaces/AE/pages/4555636748/Resolving+refs