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

[core][maven][gradle] User-defined server variable substitutions #3363

Merged
merged 6 commits into from
Aug 11, 2019

Conversation

jimschubert
Copy link
Member

@jimschubert jimschubert commented Jul 15, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This provides an initial capability for users to define values for server variable substitutions.

Given a spec with a server URL definition such as:

{
  "openapi" : "3.0.1",
  "info" : {
    "title" : "OpenAPI Petstore",
    "description" : "This is a sample server Petstore server. For this sample, you can use the api key `special-key` to test the authorization filters.",
    "license" : {
      "name" : "Apache-2.0",
      "url" : "http://www.apache.org/licenses/LICENSE-2.0.html"
    },
    "version" : "1.0.0"
  },
  "servers" : [ {
      "url": "https://{api}.{server}/{basePath}",
      "description": "The production API server",
      "variables": {
        "api": {
          "default": "petstore",
          "description": "This is the specific API to work with."
        },
        "server": {
          "default": "swagger.io"
        },
        "basePath": {
          "default": "v2"
        }
      }
    }],
…

The default will be https://petstore.swagger.io/v2, but we can now generate with user-declared options. For example:

defaults (generates targeting https://petstore.swagger.io/v2) :

generate -g kotlin -i .bak/openapi/openapi-servers.json -o .bak/server-variables/defaults

user defined values (generates targeting https://petstore.local/api):

openapi-generator generate -g kotlin \
    -i .bak/openapi/openapi-servers.json \
    -o .bak/server-variables/ours \
    --server-variables=api=petstore,basePath=api,server=local

The results:

.bak/server-variables/user/s/m/k/o/o/c/a/UserApi.kt:28:class UserApi(basePath: kotlin.String = "https://petstore.local/api") : ApiClient(basePath) {
.bak/server-variables/user/s/m/k/o/o/c/a/PetApi.kt:29:class PetApi(basePath: kotlin.String = "https://petstore.local/api") : ApiClient(basePath) {
.bak/server-variables/user/s/m/k/o/o/c/a/StoreApi.kt:28:class StoreApi(basePath: kotlin.String = "https://petstore.local/api") : ApiClient(basePath) {
.bak/server-variables/defaults/s/m/k/o/o/c/a/UserApi.kt:28:class UserApi(basePath: kotlin.String = "https://petstore.swagger.io/v2") : ApiClient(basePath) {
.bak/server-variables/defaults/s/m/k/o/o/c/a/PetApi.kt:29:class PetApi(basePath: kotlin.String = "https://petstore.swagger.io/v2") : ApiClient(basePath) {
.bak/server-variables/defaults/s/m/k/o/o/c/a/StoreApi.kt:28:class StoreApi(basePath: kotlin.String = "https://petstore.swagger.io/v2") : ApiClient(basePath) {

The design/usage of URLPathUtils.getServerURL was a little weird (it offloads generation logic to a static utility). Rather than rewriting this, I've simply passed the overrides to it. I anticipate some URLs being missed due to the static method call, but we can address these as found. I hope to have workflows rewritten by the 5.0 release and will refactor URLPathUtils.getServerURL at some point.

cc @OpenAPITools/generator-core-team @OpenAPITools/openapi-generator-collaborators

@auto-labeler
Copy link

auto-labeler bot commented Jul 15, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@jimschubert
Copy link
Member Author

This is a 3.x enhancement (for server variable substitutions). See: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#server-object-example

@jmini
Copy link
Member

jmini commented Jul 15, 2019

How does you change affect the enum case in server variables?

{
  "openapi" : "3.0.1",
  "info" : {
    "title" : "OpenAPI Petstore",
    "description" : "This is a sample server Petstore server. For this sample, you can use the api key `special-key` to test the authorization filters.",
    "license" : {
      "name" : "Apache-2.0",
      "url" : "http://www.apache.org/licenses/LICENSE-2.0.html"
    },
    "version" : "1.0.0"
  },
  "servers" : [ {
      "url": "https://{variant}.example.com/",
      "description": "The production API server",
      "variables": {
        "variant": {
          "default": "v3",
          "enum": ["v1", "v2", "v3"],
          "description": "The version you want to work with"
        }
      }
    }],
    "paths" : {}
}

I was thinking that a case like this create 3 server-urls:

  • https://v1.example.com/
  • https://v2.example.com/
  • https://v3.example.com/

But maybe this is not the way it works.


Second question related to enum:

Is there some validation that the value provided are compatible with what is defined in the OpenAPI spec? According to the enum documentation in server-variables:

An enumeration of string values to be used if the substitution options are from a limited set.

so in my previous example, it is not allowed to set the value to variant=v4 (or at least it should log a warning)

@jimschubert
Copy link
Member Author

@jmini thanks for the review. The enums are allowed values. Do you think we should warn if the value doesn't exist? I'd like to use the strict spec option for that validation, but I would need to do the rework of the static helpers to do it in a good way. A warning log might be the lowest impact. I can add that later today.

To your enum question, your example would work to create a constrained set of server urls only if there's only enum options, and if we strictly stick to only those values. It's possible that someone may expose a v4 preview/beta version without updating the public doc in your example, and I don't want to be so strict that users wouldn't be able to generate the undocumented version.

@jimschubert
Copy link
Member Author

I would add, as well, we currently only pull the first server url and don't do any construction of server sets which involves enums (at least not where I can tell)

@jimschubert
Copy link
Member Author

I had some stuff come up. Will try to add that warning in the next day or so.

@TiFu
Copy link
Contributor

TiFu commented Jul 16, 2019

I added support for server variables in DefaultCodegen in #816 by adding the servers & variables to the generated supporting files json. What effect does this PR have on that implementation?

@jimschubert
Copy link
Member Author

@TiFu looking over your changes, it seems we'll want to figure out how to get the options passed by the user to the templates. We could do that on a separate PR.

This PR allows users to define the values for a variable, so that the baseUrl which is generally passed into an API client targets the desired version of the server URL.

@jimschubert
Copy link
Member Author

@TiFu after digging into how I'd pass these values to the template, I began to see your concern. I need to rename serverVariables() in this PR to serverVariableOverrides() to make it clear these are user values, not spec values.

@wing328 wing328 modified the milestones: 4.1.0, 4.1.1 Aug 9, 2019
@ghost
Copy link

ghost commented Aug 9, 2019

Wow this is exactly what is needed, hopefully it will be merged soon.

* master: (122 commits)
  Fix #3604 by adding undefined as return type to headers and credentials methods in runtime.ts (#3605)
  Prepare 4.1.1-SNAPSHOT (#3603)
  Prepare 4.1.0 release (#3597)
  [java][client][jax-rs] Add a constant for Jackson @JsonProperty (#3560)
  restore openapi3 petstore.yaml (#3590)
  Add a new NodeJS Express server generator (#3567)
  [C#][client][csharp-netcore] Fix csharp netcore defaultheaders (#3562)
  Fix issue deserializing to nullptr (#3572)
  [OCaml] Add file post-processing (#3583)
  [dart2] Fix up test code generation, double deserialization, 'new' keyword deprecation (#3576)
  Run Qt5 client sample test (#3415)
  typescript-fetch: allow configuration of headers and credentials (#3586)
  using partials in ruby api_client (#3564)
  [OCaml] Added optional params support in API operations (#3568)
  [Rust Server] Generate valid Rustdocs for lazy_static items (#3556)
  Fix NPM build for Typescript-fetch (#3403)
  Expand path templates via resttemplate's uriTemplateHandler (#3500)
  Readme updated with a new tutorial and company using OpenAPI Generator (#3566)
  Fix logic of `getNullableType` of csharp server and client. (#3537)
  [Ruby] clean up Ruby dev dependencies (#3551)
  ...
@jimschubert
Copy link
Member Author

Moved maven implementation to #3609 due to CI issues with resolving the maven plugin when it is a SNAPSHOT.

jimschubert added a commit that referenced this pull request Aug 11, 2019
) (#3609)

*  Adds server variables overrides option to the Maven plugin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants