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.

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

Address getting configUrl value from options in query #9819

Closed
glowcloud opened this issue Apr 15, 2024 · 5 comments · May be fixed by codingfragments/tvh-guide#134
Closed

Address getting configUrl value from options in query #9819

glowcloud opened this issue Apr 15, 2024 · 5 comments · May be fixed by codingfragments/tvh-guide#134

Comments

@glowcloud
Copy link
Contributor

According to our docs, the setting for getting configuration options from an URL is configUrl.

In our code, we have the following for getting the configUrl from our defined options:

const configURL = queryOptions.config ?? mergedOptions.configUrl

We need to investigate whether this config option is expected somewhere else or if we can safely change this to

const configURL = mergedOptions.configUrl 

as mergedOptions should have the configUrl from queryOptions, if it was defined there.

If we do this change, we need to properly document it, as some users might be using config instead of configUrl.

@glowcloud
Copy link
Contributor Author

glowcloud commented Apr 18, 2024

It looks like this line

 const configURL = queryOptions.config ?? mergedOptions.configUrl 

was added in 8130168 as:

let configUrl = queryConfig.config || constructorConfig.configUrl

It actually included this logic that was there before, added in f22a628:

let config = parseSeach()
let configUrl = config.config

For the mentions of configUrl and config in the docs, here's the commit where they were added: 8e80ca9
And the relevant lines about config:

There are three options of passing config:
- add a query parameter `config` with URL to a server where the configs are hosted. For ex. http://petstore.swagger.io/?configs=http://localhost:3001/config.yaml
- add a config `configUrl` with URL to SwaggerUIBundle
- change default configs in `swagger-config.yaml` *Note: after changing, the project must be re-built*

These options can be used altogether, the order of inheritance is following (from the lowest priority to the highest):
`swagger-config.yaml` -> config passed to `SwaggerUIBundle` -> config fetched from `configUrl` passed to `SwaggerUIBundle` -> config fetched from URL passed as a query parameter `config`

So it looks like this difference might have been done so that the query config takes precedence as we didn't merge it into the configs before:

const constructorConfig = deepExtend({}, defaults, opts)

we just got query options separately later:
let queryConfig = parseSeach()

Currently, queryOptions and configUrl are only used in https://github.com/swagger-api/swagger-ui/blob/master/src/core/index.js. I also tried search for config but nothing looks like it's actually this parameter.

In my opinion, we can remove the queryOptions.config - we don't have this option in our documentation and the configUrl value should take precedence in mergedOptions at this point.

@char0n
Copy link
Member

char0n commented Apr 18, 2024

These options can be used altogether, the order of inheritance is following (from the lowest priority to the highest):
swagger-config.yaml -> config passed to SwaggerUIBundle -> config fetched from configUrl passed to SwaggerUIBundle -> config fetched from URL passed as a query parameter config

So this at least does confirm my assumptions in #9818

@char0n
Copy link
Member

char0n commented Apr 18, 2024

  • change default configs in swagger-config.yaml Note: after changing, the project must be re-built

This confirms my stipulation that it's a compile time thing

@char0n
Copy link
Member

char0n commented Apr 18, 2024

There are three options of passing config:

Notice the config vs configs discrepancy.

Currently this works:

http://petstore.swagger.io/?config=http://localhost:3001/config.yaml
http://petstore.swagger.io/?configUrl=http://localhost:3001/config.yaml

This doesn't work at all:
http://petstore.swagger.io/?configs=http://localhost:3001/config.yaml

@glowcloud
Copy link
Contributor Author

Addressed in #9840

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants