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.

Sign up for GitHub

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

Jump to bottom

Fixing issue #309 - References outside components and paths are not resolved #645

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Fixing issue #309 - References outside components and paths are not resolved #645

wants to merge 11 commits into from

Conversation

erfemega
Copy link
Contributor

@erfemega erfemega commented Nov 24, 2022 •
edited
Loading

This PR fixes issue #309

  • The fix was extended to dereference all the elements that are out of components element
  • It was added support for the case when the user references all/individual paths from '#/components/pathItems' in OAS 3.1
  • Added support to circular reference cases (when there is a circular ref in schemas)

It was added a suite of tests to validate that all referenceable elements:

  • Responses individually
  • Schemas
  • RequestBodies individually
  • Examples individually
  • Paths individually and all together
  • Parameters individually
  • Headers individually
    are well resolved in both cases:
  • When user references from out of components
  • When user references from in of components

@VShingala
Copy link
Member

@erfemega There are some conflicts that need resolution. Also, if you see deref.js file, inside resolveRefs() function at line no. 212, we have handling that restricts such usage based on no. of segments. Can we also add handling there? Current PR doesn't support such refs. (i.e. refs inside schema components)

VShingala
VShingala requested changes Nov 30, 2022
View reviewed changes
Copy link
Member

@VShingala VShingala left a comment

Choose a reason for hiding this comment

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

This PR is not addressing all the places we're r4esolving references. See here.

Do take a look at all places where we resolve $ref objects and add appropriate changes.

@erfemega
Dereferencing elements from out of components/schemas before processi…
b244969 
…ng spec
@erfemega
Merge branch 'develop' into fix/issue#309
3e81c24
@erfemega
adding method descriptions
e6b108d
@erfemega
Adding fix to issue 309 and related scenarios
663ab9d 
- Referencing from out of components
- Referencing paths from pathItems in oas 3.1
- Adding circular references in dereference process
- Adding test scenarios
@erfemega
Removing duplicated methods
a0c249f 
The method and implementation were changed to other location
@erfemega erfemega requested a review from VShingala December 13, 2022 22:16
LuisTejedaS
LuisTejedaS reviewed Dec 14, 2022
View reviewed changes
lib/deref.js Show resolved Hide resolved
lib/deref.js Show resolved Hide resolved
VShingala
VShingala reviewed Dec 27, 2022
View reviewed changes
Copy link
Member

@VShingala VShingala left a comment

Choose a reason for hiding this comment

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

PR looks good overall, there are some conflicts and one comment pending. Let's address it.

lib/utils.js
* @returns {boolean} whether is circular reference or not.
*/
isCircularReference: function (traverseContext, contentFromTrace) {
return traverseContext.parents.find((parent) => { return parent.node === contentFromTrace; }) !== undefined;
Copy link
Member

Choose a reason for hiding this comment

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

@erfemega Is this a deep comparison?

@erfemega erfemega requested a review from VShingala December 28, 2022 23:15
@eumenhofer
Copy link

Is there anything I can do to get this cleared out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LuisTejedaS LuisTejedaS LuisTejedaS approved these changes

@Carlos-Veloz Carlos-Veloz Carlos-Veloz approved these changes

@VShingala VShingala Awaiting requested review from VShingala

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erfemega @VShingala @eumenhofer @LuisTejedaS @Carlos-Veloz