Fix for issue#153-relative server paths in OpenAPI

APIDevTools / swagger-parser

Swagger 2.0 and OpenAPI 3.0 parser/validator

https://apitools.dev/swagger-parser

MIT License

1.09k stars 154 forks source link

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

Closed jaishirole closed 3 years ago

jaishirole commented 3 years ago

Issue reference: https://github.com/APIDevTools/swagger-parser/issues/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 commented 3 years ago

@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 commented 3 years ago

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?

jaishirole commented 3 years ago

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 commented 3 years ago

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

jaishirole commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@philsturgeon I am seeing what was reported here: https://github.com/APIDevTools/swagger-parser/pull/178#issuecomment-897211445 and perhaps it is a known issue and nothing with the code checked-in here.

jaishirole commented 3 years ago

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