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

Handling query config options #9807

Closed
glowcloud opened this issue Apr 11, 2024 · 5 comments
Closed

Handling query config options #9807

glowcloud opened this issue Apr 11, 2024 · 5 comments

Comments

@glowcloud
Copy link
Contributor

glowcloud commented Apr 11, 2024

Currently some configuration options can be set as syntaxHighlight.activated, while also setting syntaxHighlight, which can be an object { "theme": "agate", "activated": true } or false.

We need to investigate the behaviour of such configurations and make them consistent. For example, when getting syntaxHighlight.activated we should probably assume that it is actually accessing the activated parameter from the syntaxHighlight config instead of a separate syntaxHighlight.activated setting. Because of this, when we set syntaxHighlight.activated in the configs, we should actually merge it into syntaxHighlight.

@glowcloud
Copy link
Contributor Author

glowcloud commented Apr 12, 2024

According to docs there are 4 settings that contain names with dots:

  1. syntaxHighlight.activated
  2. syntaxHighlight.theme
  3. urls.primaryName

It looks to me like request.curlOptions is not a separate option but something that we should set inside requestInterceptor, ex.:

requestInterceptor: (request) => {
  request.curlOptions = ["-g", "--limit-rate 20k"]
  return request
}

In case of urls.primaryName, there exists the urls setting but it's actually an array. The way we access it is correct, as in we take urls.primaryName as a name of a config and not as a path inside an object:

let primaryName = search["urls.primaryName"] || configs["urls.primaryName"]

It seems like the only settings that would need to be merged into another are to do with syntaxHighlight.

@glowcloud
Copy link
Contributor Author

glowcloud commented Apr 12, 2024

It looks like the place to merge the options would be after merging the different configs

let mergedConfig = deepExtend({}, localConfig, constructorConfig, fetchedConfig || {}, queryConfig)

We need to take care of the precedence of the configs, which is:

From lowest to highest precedence:

The swagger-config.yaml in the project root directory, if it exists, is baked into the application
configuration object passed as an argument to Swagger UI (SwaggerUI({ ... }))
configuration document fetched from a specified configUrl
configuration items passed as key/value pairs in the URL query string

so we can't just overwrite the syntaxHighlight properties if we have syntaxHighlight.activated and/or syntaxHighlight.theme.

For example if we have:

  1. syntaxHighlight.activated: false passed to Swagger UI as an argument
  2. syntaxHighlight: { theme: "agate", activated: true } in the config fetched from the specified configUrl

The one that should take precedence is syntaxHighlight: { theme: "agate", activated: true }, so we can't simply do something like this:

if (mergedConfig["syntaxHighlight.activated"]) {
   mergedConfig.syntaxHighlight.activated = mergedConfig["syntaxHighlight.activated"]
}

as the result would be: syntaxHighlight: { theme: "agate", activated: false }.

Additionally, it's possible that we could initially set syntaxHighlight: false and then in the URL search params specify syntaxHighlight.activated=true, so trying to access mergedConfig.syntaxHighlight.activated would throw an error.

@glowcloud
Copy link
Contributor Author

glowcloud commented Apr 12, 2024

A rough idea for setting syntaxHighlight:

const setSyntaxHighlight = (config) => {
  if (typeof config.syntaxHighlight === "object") {
    mergedConfig.syntaxHighlight = config.syntaxHighlight
  } else if (config.syntaxHighlight === false) {
    mergedConfig.syntaxHighlight = false
  } 

  if (config["syntaxHighlight.activated"]!= null) {
    mergedConfig.syntaxHighlight = !mergedConfig.syntaxHighlight 
      ? { activated: config["syntaxHighlight.activated"], theme: "agate" }
      : { ...mergedConfig.syntaxHighlight, activated: config["syntaxHighlight.activated"] }
  } 
  if (config["syntaxHighlight.theme"] != null) {
    mergedConfig.syntaxHighlight = !mergedConfig.syntaxHighlight 
      ? { theme: config["syntaxHighlight.theme"], activated: true}
      : { ...mergedConfig.syntaxHighlight, theme: config["syntaxHighlight.theme"] }
  }
}

setSyntaxHighlight(localConfig)
setSyntaxHighlight(combinedConfig)
setSyntaxHighlight(fetchedConfig)
setSyntaxHighlight(queryConfig)

delete mergedConfig["syntaxHighlight.activated"]
delete mergedConfig["syntaxHighlight.theme"]

@char0n char0n changed the title Fix Swagger UI configuration access Handling query config options Apr 15, 2024
@glowcloud
Copy link
Contributor Author

According to docs there are 3 settings that contain names with dots:

  1. syntaxHighlight.activated
  2. syntaxHighlight.theme
  3. urls.primaryName

We've decided that the only way to set these types of options would be through query parameters.

As such, we'll only need to check for them after getting query options and then set them inside the objects by using the immutable lodash set (from lodash/fp/set). After doing so, we can safely delete these settings from the query config.

glowcloud added a commit that referenced this issue Apr 15, 2024
glowcloud added a commit that referenced this issue Apr 16, 2024
Refs #9807

---------

Co-authored-by: Vladimír Gorej <[email protected]>
@glowcloud
Copy link
Contributor Author

Addressed in 6923111

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

No branches or pull requests

1 participant