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
Bump to OkHttp 4.X #2709
Bump to OkHttp 4.X #2709
Conversation
@@ -79,7 +79,7 @@ public boolean verify(String host, SSLSession session) { | |||
} | |||
|
|||
public boolean verify(String host, X509Certificate certificate) { | |||
return verifyAsIpAddress(host) ? verifyIpAddress(host, certificate) : verifyHostname(host, certificate); | |||
return canParseAsIpAddress(host) ? verifyIpAddress(host, certificate) : verifyHostname(host, certificate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we remove the static import and instead import okhttp3.internal.Util;
return canParseAsIpAddress(host) ? verifyIpAddress(host, certificate) : verifyHostname(host, certificate); | |
return Util.canParseAsIpAddress(host) ? verifyIpAddress(host, certificate) : verifyHostname(host, certificate); |
@@ -586,7 +585,7 @@ private static void retryIfAllowed(Callback callback, Call call, Exception excep | |||
} | |||
|
|||
private static boolean isStreamingBody(Call call) { | |||
return call.request().body() instanceof UnrepeatableRequestBody; | |||
return call.request().body() != null && call.request().body().isOneShot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR removing the type and introducing the new API
@@ -79,7 +79,7 @@ public boolean verify(String host, SSLSession session) { | |||
} | |||
|
|||
public boolean verify(String host, X509Certificate certificate) { | |||
return verifyAsIpAddress(host) ? verifyIpAddress(host, certificate) : verifyHostname(host, certificate); | |||
return canParseAsIpAddress(host) ? verifyIpAddress(host, certificate) : verifyHostname(host, certificate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we remove the static import and instead import okhttp3.internal.Util;
return canParseAsIpAddress(host) ? verifyIpAddress(host, certificate) : verifyHostname(host, certificate); | |
return Util.canParseAsIpAddress(host) ? verifyIpAddress(host, certificate) : verifyHostname(host, certificate); |
okhttp-clients/src/main/java/com/palantir/conjure/java/okhttp/Okhttp39HostnameVerifier.java
Outdated
Show resolved
Hide resolved
👍 |
Released 7.67.0 |
Before this PR
ForwardingOkHttpClient
is incompatible with 4.0.0 because all of the fields are made final. This removes the abstraction (as it was not used, more info on that below) and hasRemotingOkHttpClient
simply override thenewCall
method that it was concerned about.After this PR
We should be able to upgrade our version of OkHttp. This should fix #1933.
Possible downsides?
Subtle behavior change? Very unlikely that this breaks anyones usage of
RemotingOkHttpClient
Info on usage
Public usage of
ForwardingOkHttpClient
Public usage of
RemotingOkHttpClient
Internal usage of
ForwardingOkHttpClient
Internal usage of
RemotingOkHttpClient
As you can see from all of the links, the only usages are internal to this repo (or internal forks) making this ABI change a strictly internal concern.
There are currently 258 internal repos that have both a version 4.x+ of
OkHttp
andokhttp-clients
on their classpath, i.e. have the potential for a runtime error should they ever callOkHttpClients#create
.So I had a shower thought thought, we have so many "incompatible" repos (see above) I wondered if this was hitting us in the field and we just didn't know it. So I did an Aries search and did not find any errors similar to the one described in #1933. Which makes sense as I imagine that would be devastating and crash most services, but also means that
okhttp-clients
is being overly broadcasted by$something
. This PR feels like a strict improvement, but the thing it is improving smells.Edit: I have spent more time burning down OkHttp internally and have two
$somethings
.magritte-sdk
but their movement off of Okhttp/retrofit is a WIP. There is a draft PR. But the biggest offender are references to old versions of libraries i.e. stalled excavators are inhibiting flow through the system. I have internal links to back this up if you need.