-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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.
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
Fix OAS3 relative urls #5341
Fix OAS3 relative urls #5341
Conversation
@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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality looks good - change requests and questions attached inline.
@@ -51,6 +52,7 @@ export default class OperationTag extends React.Component { | |||
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() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have opted for option 2. rename L54 to rawTagExternalDocsUrl, keep L55
to avoid making changes further down in code
src/core/utils.js
Outdated
@@ -722,6 +722,19 @@ export function sanitizeUrl(url) { | |||
return braintreeSanitizeUrl(url) | |||
} | |||
|
|||
export function isAbsoluteUrl(url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).
src/core/utils.js
Outdated
return url.match(/^(?:[a-z]+:)?\/\//i) // Matches http://, HTTP://, https://, ftp://, //example.com, | ||
} | ||
|
||
export function buildUrl(url="", selectedServer="") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
src/core/components/info.jsx
Outdated
render() { | ||
const { url, getComponent } = this.props | ||
|
||
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: is this an intentional change? Innovative....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No apologies
src/core/components/info.jsx
Outdated
let version = info.get("version") | ||
let description = info.get("description") | ||
let title = info.get("title") | ||
let termsOfService = info.get("termsOfService") | ||
let termsOfService = buildUrl(info.get("termsOfService"), selectedServer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change request: can this be termsOfServiceUrl
instead, for consistency with externalDocsUrl
below? I know it wasn't introduced here but it would be nice 😄
src/core/components/info.jsx
Outdated
let contact = info.get("contact") | ||
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 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@geraldglynn — any idea if you'll be able to handle the change requests here? 🙂 |
closing due to inactivity! |
src/core/window.js
Outdated
@@ -1,6 +1,8 @@ | |||
function makeWindow() { | |||
var win = { | |||
location: {}, | |||
location: { | |||
href: "https://app.swaggerhub.com/apis/smartbear/petstore/1.0.0##/pet/addPet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this for unit testing servers with relative urls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this is just for draft purposes. You should be able to define a new window with custom data in Mocha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…luteUrl()` and `buildUrl()`
please build |
@geraldglynn One last refactor request, then I think ready to merge... We've been wanting to split out the big |
let version = info.get("version") | ||
let description = info.get("description") | ||
let title = info.get("title") | ||
let termsOfService = info.get("termsOfService") | ||
let termsOfServiceUrl = buildUrl(info.get("termsOfService"), specUrl, {selectedServer}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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...
src/
is unmodified: changes to documentation, CI, metadata, etc.)package.json
)My changes...
Documentation
Automated tests