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

[FIX] OAS 3.0 yaml spec bundling error #779

Merged
merged 2 commits into from Jan 18, 2024
Merged

[FIX] OAS 3.0 yaml spec bundling error #779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
303d546
[FIX] OAS 3.0 yaml spec bundling error
aman-v-singh Jan 16, 2024
d1e6378
Use v4.1.0 for js-yaml
aman-v-singh Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion assets/json-schema-faker.js
View file
Open in desktop
Expand Up @@ -10919,7 +10919,7 @@ module.exports = {
*/
parse: function yamlParse (text, reviver) {
try {
return yaml.safeLoad(text);
return yaml.load(text);
}
catch (e) {
if (e instanceof Error) {
Expand Down
10 changes: 5 additions & 5 deletions examples/sampleswagger.yaml
View file
Open in desktop
Expand Up @@ -14,20 +14,20 @@ info:
email: apiteam@wordnik.com
license:
name: Apache 2.0
url: 'http://www.apache.org/licenses/LICENSE-2.0.html'
url: http://www.apache.org/licenses/LICENSE-2.0.html
Copy link
Member

Choose a reason for hiding this comment

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

@aman-v-singh Why are we making this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I locally ran npm test and the test cases were failing, showing there is a difference between actual and expected output.
This could have possibly happened as we changed the js-yaml version.

Copy link
Member

Choose a reason for hiding this comment

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

Can we define what actually changed? This much changes means there might be some functionality that changes and we should be very aware of such change. It could suddenly break the existing working definitions for users.

Let's pinpoint the exact issue and if there's way to avoid it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was possibly introduced here here.
We can use quotingType and forceQuotes, but this will force the string style to be a specific way and we would need to change the expected files anyway.

Copy link
Member

Choose a reason for hiding this comment

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

@aman-v-singh So with new version, what would these URLs be treated as? Are they not string anymore? I get that we have to change expected files but what would happen with users that have such files, would they be able to generate the collection? would there collection generated be any different?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are still strings. Only the bundled files in yaml format would be produced without any string quotes, and yes they would be able to generate collections. The generated collections will not be different.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, in that case it's fine.

tags:
- name: pet
description: Everything about your Pets
externalDocs:
description: Find out more
url: 'http://swagger.io'
url: http://swagger.io
- name: store
description: Operations about user
- name: user
description: Access to Petstore orders
externalDocs:
description: Find out more about our store
url: 'http://swagger.io'
url: http://swagger.io
paths:
/pet:
post:
Expand Down Expand Up @@ -542,7 +542,7 @@ paths:
description: User not found
externalDocs:
description: Find out more about Swagger
url: 'http://swagger.io'
url: http://swagger.io
components:
requestBodies:
UserArray:
Expand Down Expand Up @@ -688,4 +688,4 @@ components:
message:
type: string
xml:
name: '##default'
name: '##default'
4 changes: 2 additions & 2 deletions lib/bundle.js
View file
Open in desktop
Expand Up @@ -701,12 +701,12 @@ function generateComponentsObject(documentContext, rootContent,
circularRefsSet = new Set();
const { COMPONENTS_KEYS } = getBundleRulesDataByVersion(version);
notInLine.forEach(([key, value]) => {
let [, partial] = key.split(localPointer);
let [nodeRef, partial] = key.split(localPointer);
if (documentContext.globalReferences[key].refHasContent) {
setValueInComponents(
value.keyInComponents,
components,
getContentFromTrace(documentContext.nodeContents[key], partial),
getContentFromTrace(documentContext.nodeContents[nodeRef], partial),
COMPONENTS_KEYS
);
}
Expand Down
3 changes: 2 additions & 1 deletion lib/parse.js
View file
Open in desktop
Expand Up @@ -33,10 +33,11 @@ module.exports = {

asYaml: function (spec) {
try {
let obj = yaml.safeLoad(spec, {
let obj = yaml.load(spec, {
schema: yaml.JSON_SCHEMA
});
// yaml.safeLoad does not throw errors for most of the cases in invalid yaml
// yaml.safeLoad is removed in js-yaml 4. Hence Using yaml.load instead, which is now safe by default.
// hence check if it returned an object
if (typeof obj !== 'object') {
throw new Error('');
Expand Down
90 changes: 76 additions & 14 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -121,7 +121,7 @@
"ajv-formats": "2.1.1",
"async": "3.2.4",
"commander": "2.20.3",
"js-yaml": "3.14.1",
"js-yaml": "4.1.0",
"json-schema-merge-allof": "0.8.1",
"lodash": "4.17.21",
"oas-resolver-browser": "2.5.6",
Expand Down