Skip to content

Conversation

@jirikuncar
Copy link
Contributor

@jirikuncar jirikuncar commented Jan 14, 2020

Adds support for multiple templated servers from OpenAPI specification.

See related PR for Go: #4635
Addresses #590

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10

@auto-labeler
Copy link

auto-labeler bot commented Jan 14, 2020

👍 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.

@jirikuncar jirikuncar force-pushed the java-client-endpoint-servers branch from 847ba8a to f1ddc18 Compare January 15, 2020 14:47
@jirikuncar jirikuncar force-pushed the java-client-endpoint-servers branch from f1ddc18 to 594f055 Compare January 15, 2020 16:28
Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I left two comments inline to consider, but they are really minor things. (Not a member of Java language committee, but hopefully the comments would be helpful anyway).

@jirikuncar jirikuncar requested a review from wing328 January 16, 2020 15:13
@jirikuncar jirikuncar force-pushed the java-client-endpoint-servers branch from 63219ab to d98c7dd Compare February 11, 2020 15:51
@jimschubert jimschubert self-assigned this Feb 11, 2020
Copy link
Member

@jimschubert jimschubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks excellent!

// to support (constant) query string in `path`, e.g. "/posts?draft=1"
WebTarget target = httpClient.target(this.basePath + path);
String targetURL;
if (serverIndex != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice here to consumers that if new server variable functionality causes issue, one may call setServerIndex(null) to revert to previous behavior. Current default behavior with the server index 0 behaves in the same way as before, but moves the URL configuration to the servers array.

@jimschubert jimschubert added this to the 4.3.0 milestone Feb 16, 2020
@jimschubert
Copy link
Member

This is great and adds functionality that most clients don't support. The samples are out of date which caused the build failures. I'll fix that and let the builds run, but then I'd like to merge this so people have some time to evaluate on SNAPSHOT before the 4.3.0 release in a couple weeks.

@jimschubert jimschubert merged commit a09271e into OpenAPITools:master Feb 19, 2020
@jirikuncar jirikuncar deleted the java-client-endpoint-servers branch February 19, 2020 15:52
MikailBag pushed a commit to MikailBag/openapi-generator that referenced this pull request Mar 23, 2020
* [java] Support templated servers

* Deprecate old signature of invokeAPI method

* throw ArrayIndexOutOfBoundsException

* Update modules/openapi-generator/src/main/resources/Java/ServerConfiguration.mustache

* [java] Regenerate samples

Co-authored-by: Jim Schubert <[email protected]>
@ChristianCiach
Copy link
Contributor

Please see a09271e#r40779493

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.

5 participants