github-api icon indicating copy to clipboard operation
github-api copied to clipboard

OkHttpGitHubConnector constructor seems to override OkHttpClient variables/settings

Open willemevenwel opened this issue 3 years ago • 1 comments

OkHttpGitHubConnector constructor seems to override OkHttpClient variables/settings. I explain.

I am developing behind a corporate proxy. So while I run my code locally, I need to have my OkHttpClient initialized/built with proxy configurations. Like this:

        OkHttpClient client = new OkHttpClient.Builder()
                .connectTimeout(15, TimeUnit.SECONDS)
                .writeTimeout(15, TimeUnit.SECONDS)
                .readTimeout(15, TimeUnit.SECONDS)
                .proxy(proxy)
                .proxyAuthenticator(proxyAuthenticator)
                .build();

Before you make the point, I have tested a simple GET with this OkHttpClient instance, with success:

        Request request = new Request.Builder()
                .url("https://api.github.com")
                .build();

When initializing my GitHub instance I do the following:

        GitHubBuilder builder = new GitHubBuilder().withConnector(new OkHttpGitHubConnector(client));
        GitHub github = builder.withAppInstallationToken(githubapitoken).build();

Where the above client which is used is the one initialized with the proxy.

The assumption is that the GitHub builder will honour this client when any http requests are made.

However when looking into the OkHttpGitHubConnector constructor, it seems a new OkHttpClient is built by calling newBuilder(). Here is the code which I inspected:

public OkHttpGitHubConnector(OkHttpClient client, int cacheMaxAge) {
    **Builder builder = client.newBuilder();**
    builder.connectionSpecs(this.TlsConnectionSpecs());
    this.client = builder.build();
    if (cacheMaxAge >= 0 && this.client != null && this.client.cache() != null) {
        this.maxAgeHeaderValue = (new okhttp3.CacheControl.Builder()).maxAge(cacheMaxAge, TimeUnit.SECONDS).build().toString();
    } else {
        this.maxAgeHeaderValue = null;
    }

}

My assumption is then that .newBuilder() overrides any existing settings/properties set for this OkHttpClient.

Any help would be aprpeciated.

willemevenwel avatar Jan 19 '22 08:01 willemevenwel

@willemevenwel
The builder returned at Builder builder = client.newBuilder(); should retain all the settings from client aside from the specific ones it overrides. Since we don't set the proxy values they should be retained. Is this not the case?

bitwiseman avatar Mar 07 '22 19:03 bitwiseman