Make ApiClient in retrofit2 be able to use own OkHttpClient #6699

tgerth · 2020-06-17T15:05:48Z

Description

If I want to set a customized okHttpClient to the adapterBuilder like this:

val apiClient = ApiClient()
apiClient.adapterBuilder
      .baseUrl(baseUrl)
      .client(customizedOkHttpClient)

the client gets overwritten by this:

public <S> S createService(Class<S> serviceClass) {
        return this.adapterBuilder.client(this.okBuilder.build()).build().create(serviceClass);
    }

and unfortunately this method:

public void configureFromOkclient(OkHttpClient okClient) {
        this.okBuilder = okClient.newBuilder();
        this.addAuthsToOkBuilder(this.okBuilder);
    }

is of no use to me because the okBuilder would just get the clients fields but I need/want to have the client itself respectively its instance. So I added a boolean field "okBuilderUsed" which checks if the okBuilder is used and if it's used then createService will return its client and if not then createService will just return the adapterBuilder:

public <S> S createService(Class<S> serviceClass) {
        return this.okBuilderUsed ? this.adapterBuilder.client(this.okBuilder.build()).build().create(serviceClass) : this.adapterBuilder.build().create(serviceClass);
    }

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. 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/config/java*. For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Update my fork

auto-labeler · 2020-06-17T15:05:53Z

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

wing328 · 2020-06-18T09:41:41Z

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) @bkabrda (2020/01)

wing328 · 2020-06-18T09:57:52Z

Please update the samples by running ./bin/generate-samples.sh ./bin/configs/java-retrofit2*

https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-the-samples-i-couldnt-find-the-script-under-bin-or-windows-batch-files-under-binwindows

cbornet · 2020-06-18T10:34:32Z

I don't really like the okBuilderUsed variable. IMO it would be better to have an immutable ApiClient with a builder and a setClient method. This would be a breaking change but maybe it can be done for v5 ?
Meanwhile, can't you call adapterBuilder.build().createService() yourself ?

wing328 · 2020-06-18T10:37:15Z

This would be a breaking change but maybe it can be done for v5 ?

Yes we can include in the v5.0.0 release (release date not yet finalized)

tgerth · 2020-06-18T11:44:35Z

I don't really like the okBuilderUsed variable. IMO it would be better to have an immutable ApiClient with a builder and a setClient method. This would be a breaking change but maybe it can be done for v5 ?
Meanwhile, can't you call adapterBuilder.build().createService() yourself ?

I actually kind of expected that this is not a beautiful solution but I thought why not give a try. Could we also do your proposed solution for a CallFactory like setCallFactory?
And yeah you are right haha I don't know why I didn't see it, thanks.

tgerth · 2020-06-18T14:08:17Z

@cbornet I'm not sure what exactly your thought was, but I would suggest something like this

  public <S> S createService(Class<S> serviceClass) {
    return adapterBuilder.build().create(serviceClass);
  }
  
  public void setOwnClient(OkHttpClient client) {
    adapterBuilder.client(client)
  }
  
  public void setOwnCallFactory(Call.Factory callFactory) {
    adapterBuilder.callFactory(callFactory)
  }
  
  public void setOkBuilderClient() {
    adapterBuilder.client(okBuilder.build())
  }

so the change for the current user would be that they would have to make a call to setOkBuilderClient but I think that would be a reasonable change(?)

jonnii · 2020-06-18T14:22:01Z

I did a similar change for the regular okhttp client #6401. Would be good if we can make this the same for both?

cbornet · 2020-06-18T16:04:57Z

setOkBuilderClient but I think that would be a reasonable change

That would be breaking and very unexpected...

Looking a little more, I think your solution can be accepted but I would change the logic a bit.
I propose to add an OkHttpClient attribute and a setClient method.

public ApiClient setClient(OkHttpClient client) {
    this.okHttpClient = client;
    return this;

then in createService

if (okHttpClient != null) {
    adapterBuilder.client(okHttpClient);
else {
    adapterBuilder.client(okBuilder.build());
}

and make clear in setClient's javadoc that the client provided will be used and the APIClient's method's that would have configured the client (such as the auth methods which configure an interceptor) will not have any effect anymore.

jonnii · 2020-06-18T16:34:18Z

@cbornet what about taking the okhttp client as a constructor param? this removes any uncertainty about which order things need to be called but more importantly doesn't instantiate an OkHttpClient which isn't free.

tgerth · 2020-06-19T11:50:51Z

@jonnii Taking the okhttp client as a constructor param instead of the setClient method? Or what do you mean?

jonnii · 2020-06-19T11:57:09Z

@tgerth if you look at createDefaultAdapter() (which is called from the default constructor) it creates an instance of the okhttp client. if you're supplying your own okhttp client then this operation is redundant and should be avoided because instantiating a client isnt' free. my suggestion is to do something like

... = new ApiClient(okHttpClient)

instead of setClient because then you have the option of skipping creating the default client and using the one you supply.

cbornet · 2020-06-19T12:40:41Z

if you look at createDefaultAdapter() (which is called from the default constructor) it creates an instance of the okhttp client.

No it creates a builder not the actual client. A client is created only when calling createService.

jonnii · 2020-06-19T13:14:11Z

Sorry, i meant a builder - that's also not free:

https://github.com/square/okhttp/blob/master/okhttp/src/main/kotlin/okhttp3/OkHttpClient.kt#L470

it instantiates a dispatcher, a connection pool etc...

cbornet · 2020-06-19T13:41:42Z

Are those allocations really that heavy compared to the rest ?
That said, it's not hard to do the constructor so why not. If you don't instantiate the okhttp builder, beware to send a proper exception in the methods that would need it.

jonnii · 2020-06-19T14:10:33Z

I think it comes down to your usage pattern. If you can create a single ApiClient and re-use it then tbh the overhead of creating a building you don't use isn't a big deal. Unfortunately for my system we need to generate a new jwt for every request which requires first hitting our auth server before sending our request out. We used to do this by generating a new ApiClient for each request but noticed that we were adding ~20-30ms to each request by doing this - instead we now reuse the same client and some okhttp interceptor magic so we can share the okhttp client across ApiClient. We didn't see the full benefit of this until we were passing in the client via the ctor eliding the creating of the builder.

tgerth · 2020-06-22T08:34:01Z

so maybe something like this:

private Map<String, Interceptor> apiAuthorizations;
private OkHttpClient.Builder okBuilder;
private Retrofit.Builder adapterBuilder;
private JSON json;
private OkHttpClient okHttpClient;

public ApiClient() {
    apiAuthorizations = new LinkedHashMap<String, Interceptor>();
    createDefaultAdapter();
    okBuilder = new OkHttpClient.Builder();
}

public ApiClient(OkHttpClient client){
    apiAuthorizations = new LinkedHashMap<String, Interceptor>();
    createDefaultAdapter();
    okHttpClient = client;
}

public void createDefaultAdapter() {
    json = new JSON();

    String baseUrl = "{{{basePath}}}";
    if (!baseUrl.endsWith("/"))
      baseUrl = baseUrl + "/";

    adapterBuilder = new Retrofit
      .Builder()
      .baseUrl(baseUrl)
      {{#useRxJava}}
      .addCallAdapterFactory(RxJavaCallAdapterFactory.create())
      {{/useRxJava}}
      {{#useRxJava2}}
      .addCallAdapterFactory(RxJava2CallAdapterFactory.create())
      {{/useRxJava2}}
      {{#useRxJava3}}
      .addCallAdapterFactory(RxJava3CallAdapterFactory.create())
      {{/useRxJava3}}
      .addConverterFactory(ScalarsConverterFactory.create())
      .addConverterFactory(GsonCustomConverterFactory.create(json.getGson()));
}

public <S> S createService(Class<S> serviceClass) {
    if (okHttpClient != null) {
        return adapterBuilder.client(okHttpClient).build().create(serviceClass);
    else {
        return adapterBuilder.client(okBuilder.build()).build().create(serviceClass);
    }
}

cbornet · 2020-06-22T09:37:31Z

Yes. And null checks on okBuilder where needed.

wing328 · 2020-06-26T09:21:35Z

CircleCI reports some failure in building the Java client, e.g.

[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/retrofit2/src/main/java/org/openapitools/client/ApiClient.java:[352,10] illegal start of expression
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/retrofit2/src/main/java/org/openapitools/client/ApiClient.java:[352,36] ';' expected
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/retrofit2/src/main/java/org/openapitools/client/ApiClient.java:[352,58] ';' expected
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/retrofit2/src/main/java/org/openapitools/client/ApiClient.java:[411,2] reached end of file while parsing
[ERROR] -> [Help 1]

Please refer to https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/3722/workflows/3fe3d6fa-0df5-4e7e-bf30-7c5e135c23a7/jobs/17090/steps for the details.

cbornet · 2020-06-26T09:26:12Z

modules/openapi-generator/src/main/resources/Java/libraries/retrofit2/ApiClient.mustache

@@ -357,7 +365,9 @@ public class ApiClient {
      throw new RuntimeException("auth name \"" + authName + "\" already in api authorizations");
    }
    apiAuthorizations.put(authName, authorization);
-    okBuilder.addInterceptor(authorization);
+    if(okBuilder != null){


May be better to throw an Exception with an explicit message if okBuilder is null ?

tgerth · 2020-06-26T13:20:36Z

sry I forgot a semicolon😂😅

cbornet · 2020-06-26T14:51:47Z

samples/client/petstore/java/retrofit2/src/main/java/org/openapitools/client/ApiClient.java

@@ -137,7 +137,7 @@ public void createDefaultAdapter() {
  public <S> S createService(Class<S> serviceClass) {
    if (okHttpClient != null) {
        return adapterBuilder.client(okHttpClient).build().create(serviceClass);
-    else {
+    }else {


space missing

cbornet · 2020-06-26T14:52:35Z

samples/client/petstore/java/retrofit2/src/main/java/org/openapitools/client/ApiClient.java

-    if(okBuilder != null){
-      okBuilder.addInterceptor(authorization);
+    if(okBuilder == null){
+    	throw new Exception("okBuilder is null")


Use RuntimeException ? (or even better : create an ApiClientException)

The message could be more explicit. Something like "the ApiClient was created with a built okClient so it's not possible to add an authorization interceptor to it".

What is the advantage of creating an own ApiClientException comparing to the RuntimeException here in this case?

See https://rules.sonarsource.com/java/RSPEC-112 . But we already have a bunch of RuntimeException so...

Ah ok I see but as you said we already have many RuntimeExceptions so I will use a RuntimeException here if that's ok and then maybe in future you/we can change them to own Exceptions.

…ssage

Update my fork

* master: (142 commits) update python samples clarify direction of py client side validation flag (#6850) fix erronous cmd arg example for docker in readme (#6846) [BUG] [JAVA] Fix multiple files upload (#4803) (#6808) [kotlin][client] fix retrofit dependencies (#6836) [PowerShell] add more fields to be customized (#6835) [Java][WebClient]remove the dead code from java ApiClient.mustache (#6556) [PHP] Better handling of invalid data (array) (#6760) Make ApiClient in retrofit2 be able to use own OkHttpClient (#6699) mark python2 support in flask as deprecated (#6653) update samples [Java][jersey2] Add a getter for the User-Agent header value (#6831) Provides a default nil value for optional init parameters (#6827) [Java] Deprecate feignVersion option (#6824) [R] Enum R6Class Support, closes #3367 (#5728) [Rust][Client] Unify sync/async client structure (#6753) [php-ze-ph] Set required PHP version to ^7.2 (#6763) [Java][client][native][Gradle] Add missing jackson-databind-nullable (#6802) Improve sttpOpenApiClient generator (#6684) Update docker-tag-latest-release.yml ...

tgerth and others added 2 commits June 17, 2020 15:17

Merge pull request #1 from OpenAPITools/master

c46b027

Update my fork

set adapterBuilder.client() only if okBuilder was used in retrofit2

b4fec1c

auto-labeler bot added the Client: Retrofit label Jun 17, 2020

tgerth changed the title ~~Make ApiClient in retrofit2 to be able to use own OkHttpClient~~ Make ApiClient in retrofit2 be able to use own OkHttpClient Jun 18, 2020

wing328 added Client: Java Enhancement: General labels Jun 18, 2020

updated the samples

6f646db

added field okHttpClient and updated samples

b0855b3

cbornet reviewed Jun 26, 2020

View reviewed changes

bug fixed, added exception if okBuilder is null

26f777a

added semicolon

ef3a88e

cbornet reviewed Jun 26, 2020

View reviewed changes

Tatiana Gerth and others added 3 commits June 29, 2020 16:32

added space, changed Exception to RuntimeException and changed its me…

d7634ae

…ssage

updated the samples

d1de6b8

Merge pull request #2 from OpenAPITools/master

ccf3c20

Update my fork

wing328 added this to the 5.0.0 milestone Jul 2, 2020

wing328 merged commit 38ab738 into OpenAPITools:master Jul 2, 2020

tgerth mentioned this pull request Jul 3, 2020

[Kotlin] Make ApiClient in jvm-retrofit2 be able to use own OkHttpClient #6855

Closed

5 tasks

tgerth mentioned this pull request Jul 20, 2020

[Kotlin] Make ApiClient in jvm-retrofit2 be able to use own OkHttpClient #6999

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ApiClient in retrofit2 be able to use own OkHttpClient #6699

Make ApiClient in retrofit2 be able to use own OkHttpClient #6699

tgerth commented Jun 17, 2020 •

edited

Loading

auto-labeler bot commented Jun 17, 2020

wing328 commented Jun 18, 2020

wing328 commented Jun 18, 2020

cbornet commented Jun 18, 2020

wing328 commented Jun 18, 2020

tgerth commented Jun 18, 2020

tgerth commented Jun 18, 2020

jonnii commented Jun 18, 2020

cbornet commented Jun 18, 2020 •

edited

Loading

jonnii commented Jun 18, 2020

tgerth commented Jun 19, 2020 •

edited

Loading

jonnii commented Jun 19, 2020

cbornet commented Jun 19, 2020

jonnii commented Jun 19, 2020

cbornet commented Jun 19, 2020

jonnii commented Jun 19, 2020

tgerth commented Jun 22, 2020

cbornet commented Jun 22, 2020

wing328 commented Jun 26, 2020

cbornet Jun 26, 2020

tgerth commented Jun 26, 2020

cbornet Jun 26, 2020

cbornet Jun 26, 2020

cbornet Jun 26, 2020

tgerth Jun 29, 2020

cbornet Jun 29, 2020

tgerth Jun 29, 2020

Make ApiClient in retrofit2 be able to use own OkHttpClient #6699

Make ApiClient in retrofit2 be able to use own OkHttpClient #6699

Conversation

tgerth commented Jun 17, 2020 • edited Loading

Description

PR checklist

auto-labeler bot commented Jun 17, 2020

wing328 commented Jun 18, 2020

wing328 commented Jun 18, 2020

cbornet commented Jun 18, 2020

wing328 commented Jun 18, 2020

tgerth commented Jun 18, 2020

tgerth commented Jun 18, 2020

jonnii commented Jun 18, 2020

cbornet commented Jun 18, 2020 • edited Loading

jonnii commented Jun 18, 2020

tgerth commented Jun 19, 2020 • edited Loading

jonnii commented Jun 19, 2020

cbornet commented Jun 19, 2020

jonnii commented Jun 19, 2020

cbornet commented Jun 19, 2020

jonnii commented Jun 19, 2020

tgerth commented Jun 22, 2020

cbornet commented Jun 22, 2020

wing328 commented Jun 26, 2020

cbornet Jun 26, 2020

Choose a reason for hiding this comment

tgerth commented Jun 26, 2020

cbornet Jun 26, 2020

Choose a reason for hiding this comment

cbornet Jun 26, 2020

Choose a reason for hiding this comment

cbornet Jun 26, 2020

Choose a reason for hiding this comment

tgerth Jun 29, 2020

Choose a reason for hiding this comment

cbornet Jun 29, 2020

Choose a reason for hiding this comment

tgerth Jun 29, 2020

Choose a reason for hiding this comment

tgerth commented Jun 17, 2020 •

edited

Loading

cbornet commented Jun 18, 2020 •

edited

Loading

tgerth commented Jun 19, 2020 •

edited

Loading