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

Add operation level servers support for java okhttp-gson client #10925

Conversation

ajrice6713
Copy link
Contributor

@ajrice6713 ajrice6713 commented Nov 22, 2021

Add operation level server support for Java okhttp-gson http client

Closes #10847

At the API level - user can specify a custom base URL. If no custom base url is specified, the API selects the 1st path level server (if any), unless a different host index is specified. If no operation level servers are defined, the http request is built using the spec default server url.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog @lwlee2608 @nmuesch

Add methods for hostIndex and customBaseUrl
if you dont specifically declare a custom base url using the set method then it uses the 1st server in the operation host index array

if no custom url is set and the operation base path array is empty however, the call throws an exception
First checks to see if a custom url is provided

If not, checks to see if operation level server is defined and uses the supplied host index (default 0)

If neither is supplied, uses the ApiClient default base path
@ajrice6713 ajrice6713 changed the title Dx 2273 task/add servers support for api Dx 2273 task/add operation level servers support for okhttp-gson Nov 22, 2021
@ajrice6713 ajrice6713 changed the title Dx 2273 task/add operation level servers support for okhttp-gson Add operation level servers support for java okhttp-gson client Nov 22, 2021
@wing328 wing328 added this to the 5.3.1 milestone Nov 24, 2021
@wing328
Copy link
Member

wing328 commented Nov 25, 2021

LGTM. We'll add a test or 2 later when we update the samples to use OAS 3.0 spec.

@wing328 wing328 merged commit 08eaafa into OpenAPITools:master Nov 25, 2021
@Philipp-Borchert-ISH
Copy link

Hi @wing328 / @ajrice6713 - this caused breaking changes in generated code (ApiClient::buildCall/buildRequest etc) - furthermore the necessary code migration is a bit odd because the new parameter for these methods is not documented in the generated javadoc.
Just two remarks:

  1. Shouldn't this have been part of 5.4.x instead of patch 5.3.1? That's how I interpret https://openapi-generator.tech/docs/release-summary/ at least
  2. Maybe javadoc for the baseUrl should be added, it seems to be optional/nullable

@ajrice6713
Copy link
Contributor Author

Hey @Philipp-Borchert-ISH

Can you provide the error snippet of whats broken in the buildCall function? It wasnt intended to be breaking (and worked in my testing) 😢

@Philipp-Borchert-ISH
Copy link

Hi @ajrice6713, I'm referring to a "breaking" API change, not a bug in your code :-)
Specifically the "baseUrl" parameter has been added on multiple public methods, which breaks compatibility with code accessing these methods. I reckon that shouldn't be allowed in patch releases, hence the question to William.

@wing328
Copy link
Member

wing328 commented Jan 17, 2022

@Philipp-Borchert-ISH very good question. From what I understand, a developer shouldn't be calling these buildSomething methods directly and therefore when they use a newer version of the Java SDK generated with this particular enhancement (PR), they shouldn't need to update their code and it should work as usual.

Do you mind elaborating a bit more on your use cases that require calling these buildSomething method directly?

@Philipp-Borchert-ISH
Copy link

Hi @wing328, we're using them primarily in artificial test cases to generate "bad" requests to validate server side error handling in our APIs - so it's probably an edge case.

a developer shouldn't be calling these buildSomething methods directly

I guess this is a bit of a tricky situation - the generated clients the developers "should" be using are exposing getters/setters and a constructor including the ApiClient class, so right now it is not obvious to developers (both providing + consuming SDKs) that they should not actually use it. Other classes / interfaces in the same package - e.g. ApiCallback or ApiException - are essential to the generated API clients as well. I think it would be beneficial for the future to have a clear distinction between "this is public -> it's an API" vs. "this should not be public -> shouldn't be visible" (visible as in access modifier).

Thx for the quick feedback btw!

@ckoegel ckoegel deleted the dx-2273-task/add-servers-support-for-api branch July 11, 2023 18:17
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.

[BUG][java] Path level servers overrides are ignored
3 participants