Fix OAS3 relative urls #5341

geraldglynn · 2019-05-03T10:53:12Z

OAS3 support relative URLs. These use the Server currently selected in the dropdown as the base.
This PR appends the relative URL to the selected Server

Description

Files modified:
src/core/containers/info.jsx
src/core/components/info.jsx
src/core/components/operations.jsx
src/core/components/operation-tag.jsx
src/core/components/operation.jsx

Motivation and Context

Currently relative URLs in OAS3 APIs render as relative to SPA URL; not the selected Server.

How Has This Been Tested?

This has been tested by running SwaggerUI locally and confirming URLs rendered correctly in:
Info
– Terms of service URL
– Contact URL
– License URL
– External Docs URL

Tag
– Tag External Docs URL

Operation
– Operation External Docs
-- Operation Tag

Screenshots (if appropriate):

Checklist

My PR contains...

No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
Dependency changes (any modification to dependencies in package.json)
Bug fixes (non-breaking change which fixes an issue)
Improvements (misc. changes to existing features)
Features (non-breaking change which adds functionality)

My changes...

are breaking changes to a public API (config options, System API, major UI change, etc).
are breaking changes to a private API (Redux, component props, utility functions, etc.).
are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
are not breaking changes.

Documentation

My changes do not require a change to the project documentation.
My changes require a change to the project documentation.
If yes to above: I have updated the documentation accordingly.

Automated tests

My changes can not or do not need to be tested.
My changes can and should be tested by unit and/or integration tests.
If yes to above: I have added tests to cover my changes.
If yes to above: I have taken care to cover edge cases in my tests.
All new and existing tests passed.

…Server host

shockey · 2019-05-03T21:22:56Z

@webron: thought this would need your review for a moment, but I now see that OpenAPI 3.0 is clear about Server Object URLs being the general source for relative URL bases. From there, it's not a stretch to assume we can use the "currently selected Server Object" specifically here.

Unless specified otherwise, all properties that are URLs MAY be relative references [...] relative references are resolved using the URLs defined in the Server Object as a Base URI.

source

shockey

Functionality looks good - change requests and questions attached inline.

shockey · 2019-05-03T20:57:58Z

src/core/components/operation-tag.jsx

    let tagDescription = tagObj.getIn(["tagDetails", "description"], null)
    let tagExternalDocsDescription = tagObj.getIn(["tagDetails", "externalDocs", "description"])
    let tagExternalDocsUrl = tagObj.getIn(["tagDetails", "externalDocs", "url"])
+    tagExternalDocsUrl = buildUrl( tagExternalDocsUrl, oas3Selectors.selectedServer() )


Change request: would prefer to avoid reassignment here! some ideas:

keep L54 and rename L55 to something like canonicalTagExternalDocsUrl

rename L54 to rawTagExternalDocsUrl, keep L55

Have opted for option 2. rename L54 to rawTagExternalDocsUrl, keep L55
to avoid making changes further down in code

shockey · 2019-05-03T21:15:38Z

src/core/utils.js

  return braintreeSanitizeUrl(url)
 }

+export function isAbsoluteUrl(url) {


Change request: new util functions need accompanying tests - ideally that would include cases for expected outputs, graceful handling of malformed inputs, and basic type safety (esp. undefined/null).

shockey · 2019-05-03T21:15:44Z

src/core/utils.js

+  return url.match(/^(?:[a-z]+:)?\/\//i) // Matches http://, HTTP://, https://, ftp://, //example.com,
+}
+
+export function buildUrl(url="", selectedServer="") {


Change request: new util functions need accompanying tests - ideally that would include cases for expected outputs, graceful handling of malformed inputs, and basic type safety (esp. undefined/null).

it's not that I don't trust your code @geraldglynn - it's that I don't trust myself to remember all the edge cases relevant to this function when a first-time contributor opens a PR that touches it in 10 months 😄

shockey · 2019-05-03T21:24:54Z

src/core/components/info.jsx

    const Link = getComponent("Link")

-    return <Link target="_blank" href={ sanitizeUrl(url) }><span className="url"> { url } </span></Link>
+    return <Link target="_blank" href={ sanitizeUrl(url) }><span className="url"> { url } !!!</span></Link>


Question: is this an intentional change? Innovative....

No apologies

shockey · 2019-05-03T21:29:53Z

src/core/components/info.jsx

    let description = info.get("description")
    let title = info.get("title")
-    let termsOfService = info.get("termsOfService")
+    let termsOfService = buildUrl(info.get("termsOfService"), selectedServer)


Change request: can this be termsOfServiceUrl instead, for consistency with externalDocsUrl below? I know it wasn't introduced here but it would be nice 😄

shockey · 2019-05-03T21:34:26Z

src/core/components/info.jsx

    let license = info.get("license")
-    const { url:externalDocsUrl, description:externalDocsDescription } = (externalDocs || fromJS({})).toJS()
+    let { url:externalDocsUrl, description:externalDocsDescription } = (externalDocs || fromJS({})).toJS()
+    externalDocsUrl = buildUrl( externalDocs.url, selectedServer )


Change request: would prefer to use ImmutableMap.getIn to set externalDocsUrl and externalDocsDescription, instead of futzing with toJS. Again, aware that this PR didn't introduce it, but since it's been touched let's improve it!

Would also like to avoid reassignment on externalDocsUrl - see my note on operation-tag.jsx L55, same idea here.

shockey · 2020-01-15T02:04:16Z

@geraldglynn — any idea if you'll be able to handle the change requests here? 🙂

shockey · 2020-03-01T20:16:15Z

closing due to inactivity!

…js for testing

geraldglynn · 2020-07-22T10:50:17Z

src/core/window.js

  var win = {
-    location: {},
+    location: {
+      href: "https://app.swaggerhub.com/apis/smartbear/petstore/1.0.0##/pet/addPet"


Added this for unit testing servers with relative urls

I hope this is just for draft purposes. You should be able to define a new window with custom data in Mocha.

This was the wrong approach to what I was trying to achieve, so probably better for to explain motivation...
In purpose of buildUrl():

If URL is absolute, all is good; return the URL

If URL is relative, then we want the base to be the Server URL

However, if the Server URL itself is relative, we want the base to be the location of Definition YAML

In case #3 I was (incorrectly) setting the base URL to window location

Is there a way to get the location of the Definition YAML?
– I know what it is for our system, but it need to ben generic for Swagger-UI's system

Is there a way to get the location of the Definition YAML?

e.g. in petstore, you enter an external url path of a definition in the topbar? in that case, you can use spec.url
@geraldglynn

…Selected

…luteUrl()` and `buildUrl()`

tim-lai · 2020-07-28T21:00:18Z

please build

changes implemented

tim-lai · 2020-07-29T02:02:25Z

@geraldglynn One last refactor request, then I think ready to merge... We've been wanting to split out the big utils.js file. So let's create a new file for these new utility functions. I'm proposing utils-oas3-urls.js, but open to something more generic or descriptive. Also separate out the unit tests into its own matching file. Thanks.

…nn/swagger-ui into fix/oas3-relative-urls

test/mocha/core/utils.js

marcelstoer · 2021-01-06T15:17:34Z

src/core/components/info.jsx

    let description = info.get("description")
    let title = info.get("title")
-    let termsOfService = info.get("termsOfService")
+    let termsOfServiceUrl = buildUrl(info.get("termsOfService"), specUrl, {selectedServer})


This was a breaking change for us. Our OAS2 documents have inline termsOfService rather than a URL e.g.

termsOfService: 'Information in this document is subject to change ...'

Contrary to OAS 3 with OAS2 it is not required for the termsOfService to be a URL (as per https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#fixed-fields-1). Hence, before building this URL the termsOfService value should first be verified to be a URL.

geraldglynn added 3 commits May 2, 2019 17:23

Added tooling for appending OAS3 relative URLs to selected Server

7d39036

Merge branch 'master' into fix/oas3-relative-urls

95bf267

Modified buildUrl util to append realtive URLs beginning with '/' to …

619cbfe

…Server host

shockey requested review from shockey and webron and removed request for webron May 3, 2019 21:17

shockey previously requested changes May 3, 2019

View reviewed changes

shockey closed this Mar 1, 2020

Merge branch 'master' into fix/oas3-relative-urls

ebef288

tim-lai reopened this Jun 23, 2020

geraldglynn added 12 commits July 3, 2020 09:49

Merge branch 'master' into fix/oas3-relative-urls

4fe710a

Removed erroroneous exclamations marks!

9d43142

Updated varibale to termsOfServiceUrl

79d61e4

Changed how externalDocs url & description are retrieved and named

e218574

Changed naming of tagExternalDocsUrl

1c5b39b

Merge branch 'master' into fix/oas3-relative-urls

2518603

Added unit test for util isAbsoluteUrl()

b44ac98

Added unit tests for util buildUrl()

2852fb1

Cleaned up utils buildUrl() to use URL class for constructing URL

74c252e

Improved unit test for utils buildUrl()

d0887a9

Removed unused fromJS and cleaned up whitespace

2228717

Merge branch 'master' into fix/oas3-relative-urls

111af62

geraldglynn requested a review from shockey July 21, 2020 14:39

geraldglynn added 2 commits July 22, 2020 11:26

Added support for server relative urls in utils buildUrl()

a170d64

Cleaned up buildUrl unit tests and added location.href to window.…

e526ef4

…js for testing

geraldglynn commented Jul 22, 2020

View reviewed changes

Replaced app.swaggerhub.com with example.com

9fd7f39

geraldglynn added 14 commits July 23, 2020 09:38

Merge branch 'master' into fix/oas3-relative-urls

2e62a08

Fixed whitespacing in mocha test utils.js

e9c91f4

Added guard to externalDocs in <Info />

2d427e0

Commented out location.href in window.js; it was breaking another test

c7390ed

Added extra check to utils buildUrl()

b47d397

Added selectedServer to test mock props

a1edf30

Skipped test "build url from server with relative url"

8ac390b

Removed "location" from window.js props

4e69e75

Added specUrl to buildUrl to support relative server url or no server…

40493da

…Selected

Added unit tests for buildBaseUrl() and improved unit tests `isAbso…

80b8438

…luteUrl()` and `buildUrl()`

Added utils addProtocol()

53cad46

Removed console.log() from utils

795355e

Add PropTypes url

0632862

Added specSelector.url() to mock props for unit test

867b0aa

geraldglynn requested a review from tim-lai July 27, 2020 10:50

Merge branch 'master' into fix/oas3-relative-urls

86e9af7

geraldglynn added 3 commits August 4, 2020 11:22

Merge branch 'master' into fix/oas3-relative-urls

3c6cf6e

Merge branch 'fix/oas3-relative-urls' of https://github.com/geraldgly…

9206b7e

…nn/swagger-ui into fix/oas3-relative-urls

Moved new url util functions to utils/url

cba7b4f

tim-lai reviewed Aug 4, 2020

View reviewed changes

test/mocha/core/utils.js Outdated Show resolved Hide resolved

style: Update test/mocha/core/utils.js

89b8d1a

tim-lai merged commit d9f5691 into swagger-api:master Aug 4, 2020

marcelstoer reviewed Jan 6, 2021

View reviewed changes

marcelstoer mentioned this pull request Jan 7, 2021

fix: re-add support for inline termsOfService #6788

Closed

17 tasks

Fix OAS3 relative urls #5341

Fix OAS3 relative urls #5341

Uh oh!

Conversation

geraldglynn commented May 3, 2019

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist

My PR contains...

My changes...

Documentation

Automated tests

Uh oh!

shockey commented May 3, 2019

Uh oh!

shockey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shockey May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shockey commented Jan 15, 2020

Uh oh!

shockey commented Mar 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tim-lai Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tim-lai commented Jul 28, 2020

Uh oh!

tim-lai commented Jul 29, 2020

Uh oh!

Uh oh!

marcelstoer Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shockey May 3, 2019 •

edited

Loading

tim-lai Jul 24, 2020 •

edited

Loading

marcelstoer Jan 6, 2021 •

edited

Loading