Fix for issue#153-relative server paths in OpenAPI #181

jaishirole · 2021-08-06T09:53:34Z

Issue reference: #153

OpenAPI V3 allows servers to be at root, path or at operations level.
The relative path could show up in any of these places.
The PR tries to address that when SwaggerParser is given an URL to load the file. The fix doesn't work for non http(s) urls or local file system uploaded files.
For unit test, I've used a petstore file, which has relative paths. However, for other case of servers at path/operation, I have tested it by hosting a file on a cloud site. But, do not have a place to host there forever. Other possibility is mocking and using a file that is resident in the codebase. But, let me know your suggestion.

jaishirole · 2021-08-06T10:19:21Z

@philsturgeon @P0lip
This is my first PR, please review and let me know your comments.
I would also need your help to trigger the pipeline tests for sanity of the PR.
I've locally run the unit tests and they're passing, except a known failure of unsupported 3.1.0 version in real-world APIs test suite.

philsturgeon · 2021-08-06T21:10:15Z

Sorry @jaishirole, bagsy not it. I'm out in the mountains on a big vacation and cannot help.

@P0lip if you're around can you help? Or can you pass it over to somebody who can?

Fix lint

philsturgeon · 2021-08-15T09:23:58Z

test/specs/oas-relative-servers/v3-relative-servers.spec.js

+const fetch = require("node-fetch");
+const {expect} = require("chai");
+
+const RELATIVE_SERVERS_OAS3_URL = "https://petstore3.swagger.io/api/v3/openapi.json"; //Petstore v3 json has relative path in "servers"


Thank you for this PR, glad I'm getting a chance to review it now!

I would prefer to avoid adding dependencies to random URLs, as it can have impacts on stability of the test suite (already not good), and can make things hard for those on unstable or highly locked down environments.

Could you create the smallest replicatable file and save it in there, like test/specs/deep-circular/deep-circular.yaml has done. Then this test will not need to go over the wire.

Hey Phil @philsturgeon , thanks much for the review comment. I'll have to mock the fetch from an https URL to get the proposed changes tested without going over the internet. Let me add that and push a commit by tomorrow.

Hey Phil @philsturgeon , please take a look at the latest push. I had to resolve conflicts as the upstream repo had new changes on 08/15. So, there are some merge conflicts that I had to solve. I was able to mock the http url fetch and even added more test cases. Let me know your review comments. Thanks!

…nto main

philsturgeon

Looking good. If you could trim down the test files I'll be happy to merge this one! Thank you 🕺🏻

philsturgeon · 2021-08-17T10:54:35Z

test/specs/oas-relative-servers/v3-relative-server.json

+    "tags": [
+        {
+            "name": "pet",
+            "description": "Everything about your Pets",
+            "externalDocs": {
+                "description": "Find out more",
+                "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"
+            }
+        }
+    ],


thanks for these updates. when I mentioned making the smallest recreatable fixture files i meant can you trim them down a bunch so its easier to see what theyre actually doing? These megafixtures tend to get used for multiple things as people "find the most relevant one" and that leads to brittle test suites over time.

Sure, Phil! Let me trim the size further.

Pushed the new commit, Phil.

philsturgeon · 2021-08-17T10:55:10Z

test/specs/oas-relative-servers/v3-relative-server.json

+                ]
+            }
+        },
+        "/pet/findByStatus": {


Could probably wipe out all paths other than the one you need to test, trim off parameters, etc.

Sure, let me keep it to a smaller size.

Pushed the new commit, Phil.

jaishirole · 2021-08-17T12:55:58Z

Phil @philsturgeon , did the last push too! Hope you'll like it, halved the size of the json files but no compromise on what we wanted to test. :)

jaishirole · 2021-08-17T19:01:43Z

I'm on it to fix the lint errors, not sure why my VS Code didn't catch it earlier.

jaishirole · 2021-08-17T20:14:54Z

@philsturgeon
I pushed another commit to address the lint errors that showed failures in checks. That has overridden your approval. Can you help run the workflows again? Locally npm run lint doesn't show any errors now and the unit tests too are passing still.

jaishirole · 2021-08-18T11:21:27Z

Hi Phil @philsturgeon , it looks like the usage of 'Optional chaining' aka ? in my code is causing a problem with some of the tests: node12 on Ubuntu and Browser Tests. I guess I need to move away from ? as it probably is perceived to be used only as a ternary operator by these test environments?

jaishirole · 2021-08-18T12:14:45Z

Hi Phil @philsturgeon , it looks like the usage of 'Optional chaining' aka ? in my code is causing a problem with some of the tests: node12 on Ubuntu and Browser Tests. I guess I need to move away from ? as it probably is perceived to be used only as a ternary operator by these test environments?

@philsturgeon Another push for moving away from ? chaining parameter. Please help to kick off the check workflows.

jaishirole · 2021-08-18T15:17:16Z

Phil @philsturgeon , the browser tests failed for not having the 'url' module available there. I've changed the code to use the 'url' exported from existing dependency of @apidevtools/json-schema-ref-parser which takes care of browser v/s node environments.
Now the npm run coverage:browser is passing on my local system where I've Firefox & Safari browsers.

If you trigger the checks workflow, looking forward to next results now.

jaishirole · 2021-08-19T02:46:26Z

Hi Phil (@philsturgeon ) , not sure what the latest failure is about. Can you help me understand what is still left?
I guess it failed for?

18 08 2021 21:01:36.741:ERROR [launcher]: No binary for Edge browser on your platform.
  Please, set "EDGE_BIN" env variable.
18 08 2021 21:01:36.995:ERROR [launcher]: No binary for Safari browser on your platform.
  Please, set "SAFARI_BIN" env variable.

jaishirole · 2021-08-20T06:05:19Z

@philsturgeon I am seeing what was reported here: #178 (comment) and perhaps it is a known issue and nothing with the code checked-in here.

jaishirole · 2021-08-23T12:24:00Z

@philsturgeon Thanks much for all your guidance on this first PR of mine!

shenie and others added 4 commits July 21, 2021 21:38

Skip real work api that hangs during validate

6ca92de

Skip openapi 3.1.0 apis as it is not supported yet

b7c2bd5

Add other known errors to the list

cb4dff1

FIx for issue#153-relative paths in OpenAPI

e538e3a

jaishirole changed the title ~~FIx for issue#153-relative paths in OpenAPI~~ FIx for issue#153-relative server paths in OpenAPI Aug 6, 2021

jaishirole changed the title ~~FIx for issue#153-relative server paths in OpenAPI~~ Fix for issue#153-relative server paths in OpenAPI Aug 6, 2021

shenie and others added 7 commits August 12, 2021 03:05

Update fetch-api-list.js

5ed33b3

Fix lint

skip 3.1.0

a8ca2c8

Ignore other openapi 3.1.0 errors

fc1c1ae

Skip another spec with invalid yaml

a391da9

fix: Skip known errors or unsupported api in real-world api spec

7f27db7

preparing release 10.0.3

753e928

release v10.0.3

e3a09bd

philsturgeon reviewed Aug 15, 2021

View reviewed changes

jaishirole added 5 commits August 17, 2021 03:13

Incorporate review comment for mocking http file fetch

c17864b

FIx for issue#153-relative paths in OpenAPI

049edf1

Incorporate review comment for mocking http file fetch

c1a8637

Merge branch 'main' of https://github.com/jaishirole/swagger-parser i…

25e3993

…nto main

Latest npm install modified lock file

aa7995f

philsturgeon reviewed Aug 17, 2021

View reviewed changes

Trimmed the test files' content

28ac37f

philsturgeon approved these changes Aug 17, 2021

View reviewed changes

Resolved failing lint errors

a92f479

Removed operational chaining parameter

ef4a23e

Use url exported from the util of json-schema-ref-parser dependency

f8b769b

philsturgeon merged commit 2f22fe7 into APIDevTools:main Aug 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue#153-relative server paths in OpenAPI #181

Fix for issue#153-relative server paths in OpenAPI #181

jaishirole commented Aug 6, 2021 •

edited

Loading

jaishirole commented Aug 6, 2021

philsturgeon commented Aug 6, 2021

philsturgeon Aug 15, 2021

jaishirole Aug 15, 2021

jaishirole Aug 16, 2021

philsturgeon left a comment

philsturgeon Aug 17, 2021

jaishirole Aug 17, 2021

jaishirole Aug 17, 2021

philsturgeon Aug 17, 2021

jaishirole Aug 17, 2021

jaishirole Aug 17, 2021

jaishirole commented Aug 17, 2021

jaishirole commented Aug 17, 2021

jaishirole commented Aug 17, 2021

jaishirole commented Aug 18, 2021

jaishirole commented Aug 18, 2021

jaishirole commented Aug 18, 2021

jaishirole commented Aug 19, 2021

jaishirole commented Aug 20, 2021

jaishirole commented Aug 23, 2021

Fix for issue#153-relative server paths in OpenAPI #181

Fix for issue#153-relative server paths in OpenAPI #181

Conversation

jaishirole commented Aug 6, 2021 • edited Loading

jaishirole commented Aug 6, 2021

philsturgeon commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philsturgeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaishirole commented Aug 17, 2021

jaishirole commented Aug 17, 2021

jaishirole commented Aug 17, 2021

jaishirole commented Aug 18, 2021

jaishirole commented Aug 18, 2021

jaishirole commented Aug 18, 2021

jaishirole commented Aug 19, 2021

jaishirole commented Aug 20, 2021

jaishirole commented Aug 23, 2021

jaishirole commented Aug 6, 2021 •

edited

Loading